FOUR-32464: [Octane] CRITICAL Data Leaks Between Requests, MEDIUM — Accumulating State

ProcessMaker/Listeners/HandleRedirectListener.php

@@ -44,9 +44,7 @@ public static function sendRedirectToEvent()
             event($event);
 
             // Clean params to prevent sending the same redirect multiple times
-            self::$redirectionParams = [];
-            self::$redirectionMethod = '';
-            self::$processRequest = null;
+            self::reset();
         }
     }
 }

ProcessMaker/Nayra/Repositories/EntityRepository.php

@@ -11,7 +11,7 @@
 
 abstract class EntityRepository
 {
-    private static $uid2id = ['requests' =>[], 'tokens' =>[]];
+    private $uid2id = ['requests' =>[], 'tokens' =>[]];
 
     abstract public function create(array $transaction): ? Model;
 
@@ -41,16 +41,16 @@ public function resolveId(string $uid): int
         }
 
         // Get record if is not stored previously
-        if (!isset(self::$uid2id[$type][$uid])) {
+        if (!isset($this->uid2id[$type][$uid])) {
             $record = $instance->select('id')->where('uuid', $uid)->first();
             if ($record) {
-                self::$uid2id[$type][$uid] = $record->getKey();
+                $this->uid2id[$type][$uid] = $record->getKey();
             } else {
                 throw new Exception("The uid {$uid} does not exist in the database");
             }
         }
 
-        return self::$uid2id[$type][$uid] ?? 0;
+        return $this->uid2id[$type][$uid] ?? 0;
     }
 
     /**
@@ -71,6 +71,6 @@ public function storeUid(string $uid, int $id): void
                 break;
         }
 
-        self::$uid2id[$type][$uid] = $id;
+        $this->uid2id[$type][$uid] = $id;
     }
 }

tests/unit/ProcessMaker/Listeners/HandleRedirectListenerTest.php

@@ -0,0 +1,235 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Tests\Unit\ProcessMaker\Listeners;
+
+use ProcessMaker\Events\RedirectToEvent;
+use ProcessMaker\Listeners\HandleRedirectListener;
+use ProcessMaker\Models\ProcessRequest;
+use ProcessMaker\Octane\ResetRequestState;
+use ReflectionProperty;
+use Tests\TestCase;
+
+class HandleRedirectListenerTest extends TestCase
+{
+    /**
+     * Create a test subclass that exposes the protected setRedirectTo method.
+     */
+    private function createProbe(): HandleRedirectListener
+    {
+        return new class extends HandleRedirectListener {
+            public function queue(ProcessRequest $processRequest, string $method, ...$params): void
+            {
+                $this->setRedirectTo($processRequest, $method, ...$params);
+            }
+        };
+    }
+
+    /**
+     * Read a private static property from HandleRedirectListener.
+     */
+    private function readStaticProperty(string $property): mixed
+    {
+        $reflection = new ReflectionProperty(HandleRedirectListener::class, $property);
+        $reflection->setAccessible(true);
+
+        return $reflection->getValue();
+    }
+
+    /**
+     * Assert that all 3 static properties are in their default/clean state.
+     */
+    private function assertStateIsClean(): void
+    {
+        $this->assertNull($this->readStaticProperty('processRequest'));
+        $this->assertSame('', $this->readStaticProperty('redirectionMethod'));
+        $this->assertSame([], $this->readStaticProperty('redirectionParams'));
+    }
+
+    /**
+     * Test that reset() clears the static $processRequest property.
+     */
+    public function test_reset_clears_process_request(): void
+    {
+        $probe = $this->createProbe();
+        $request = ProcessRequest::factory()->create();
+        $probe->queue($request, 'processUpdated');
+
+        HandleRedirectListener::reset();
+
+        $this->assertNull($this->readStaticProperty('processRequest'));
+    }
+
+    /**
+     * Test that reset() clears the static $redirectionMethod property.
+     */
+    public function test_reset_clears_redirection_method(): void
+    {
+        $probe = $this->createProbe();
+        $request = ProcessRequest::factory()->create();
+        $probe->queue($request, 'processCompletedRedirect');
+
+        HandleRedirectListener::reset();
+
+        $this->assertSame('', $this->readStaticProperty('redirectionMethod'));
+    }
+
+    /**
+     * Test that reset() clears the static $redirectionParams property.
+     */
+    public function test_reset_clears_redirection_params(): void
+    {
+        $probe = $this->createProbe();
+        $request = ProcessRequest::factory()->create();
+        $probe->queue($request, 'processUpdated', ['key' => 'value']);
+
+        HandleRedirectListener::reset();
+
+        $this->assertSame([], $this->readStaticProperty('redirectionParams'));
+    }
+
+    /**
+     * Critical test for Octane: verify that reset() prevents data leaks.
+     * After reset(), the stored redirect data should be gone.
+     */
+    public function test_reset_prevents_stale_redirect_from_leaking(): void
+    {
+        $probe = $this->createProbe();
+        $request = ProcessRequest::factory()->create();
+        $probe->queue($request, 'processUpdated', ['tokenId' => 123]);
+
+        // Simulate Octane reset between requests
+        HandleRedirectListener::reset();
+
+        // sendRedirectToEvent should NOT dispatch RedirectToEvent after reset
+        $this->expectNotToPerformAssertions();
+        HandleRedirectListener::sendRedirectToEvent();
+    }
+
+    /**
+     * Test that reset() can be called multiple times safely.
+     */
+    public function test_reset_can_be_called_multiple_times(): void
+    {
+        HandleRedirectListener::reset();
+        HandleRedirectListener::reset();
+        HandleRedirectListener::reset();
+
+        // Should not throw any errors
+        $this->assertStateIsClean();
+    }
+
+    /**
+     * Test that sendRedirectToEvent dispatches the event and clears state.
+     */
+    public function test_send_redirect_to_event_dispatches_and_clears_state(): void
+    {
+        \Illuminate\Support\Facades\Event::fake([RedirectToEvent::class]);
+
+        $probe = $this->createProbe();
+        $request = ProcessRequest::factory()->create();
+        $probe->queue($request, 'processUpdated');
+
+        HandleRedirectListener::sendRedirectToEvent();
+
+        // Assert the event was dispatched
+        \Illuminate\Support\Facades\Event::assertDispatched(RedirectToEvent::class);
+
+        // After dispatch, the state should be cleared
+        $this->assertNull($this->readStaticProperty('processRequest'));
+    }
+
+    /**
+     * CRITICAL: Simulate the full Octane request cycle to guarantee no data leak.
+     *
+     * Flow:
+     *   1. Request A stores data with different values
+     *   2. Reset (simulating Octane's RequestTerminated event)
+     *   3. Verify ALL 3 properties are clean
+     *   4. Request B stores NEW data with different values
+     *   5. Verify Request B's data is correct (not contaminated by Request A)
+     *   6. Reset again
+     *   7. Verify clean again
+     */
+    public function test_full_octane_cycle_guarantees_no_data_leak(): void
+    {
+        // === Request A ===
+        $requestA = ProcessRequest::factory()->create();
+        $probeA = $this->createProbe();
+        $probeA->queue($requestA, 'processCompletedRedirect', ['tokenA' => 111]);
+
+        // Verify Request A data is stored (setRedirectTo uses ...$params, so it's nested)
+        $this->assertSame($requestA->getKey(), $this->readStaticProperty('processRequest')->getKey());
+        $this->assertSame('processCompletedRedirect', $this->readStaticProperty('redirectionMethod'));
+        $this->assertSame([['tokenA' => 111]], $this->readStaticProperty('redirectionParams'));
+
+        // === Octane reset after Request A ===
+        HandleRedirectListener::reset();
+
+        // === Verify ALL properties are clean after reset ===
+        $this->assertStateIsClean();
+
+        // === Request B (simulating a DIFFERENT user/request) ===
+        $requestB = ProcessRequest::factory()->create();
+        $probeB = $this->createProbe();
+        $probeB->queue($requestB, 'processUpdated', ['tokenB' => 222, 'userId' => 999]);
+
+        // Verify Request B's data is correct (NOT contaminated by Request A)
+        $this->assertSame($requestB->getKey(), $this->readStaticProperty('processRequest')->getKey());
+        $this->assertSame('processUpdated', $this->readStaticProperty('redirectionMethod'));
+        $this->assertSame([['tokenB' => 222, 'userId' => 999]], $this->readStaticProperty('redirectionParams'));
+
+        // Verify Request A's data is GONE (no leak)
+        $this->assertNotSame($requestA->getKey(), $this->readStaticProperty('processRequest')?->getKey());
+
+        // === Octane reset after Request B ===
+        HandleRedirectListener::reset();
+
+        // === Verify clean again ===
+        $this->assertStateIsClean();
+    }
+
+    /**
+     * CRITICAL: Verify that ResetRequestState orchestrator triggers the reset correctly.
+     */
+    public function test_reset_request_state_triggers_handle_redirect_reset(): void
+    {
+        $probe = $this->createProbe();
+        $request = ProcessRequest::factory()->create();
+        $probe->queue($request, 'processUpdated', ['tokenId' => 456]);
+
+        // Verify state is dirty before reset
+        $this->assertNotNull($this->readStaticProperty('processRequest'));
+
+        // Execute the orchestrator (same as Octane's RequestTerminated listener)
+        $resetState = new ResetRequestState();
+        $resetState->handle();
+
+        // Verify orchestrator cleaned everything
+        $this->assertStateIsClean();
+    }
+
+    /**
+     * CRITICAL: Simulate the scenario where sendRedirectToEvent() fails,
+     * but reset() still cleans up (edge case in Octane).
+     */
+    public function test_reset_cleans_up_even_when_send_redirect_fails(): void
+    {
+        $probe = $this->createProbe();
+        $request = ProcessRequest::factory()->create();
+        $probe->queue($request, 'processUpdated', ['data' => 'sensitive']);
+
+        // Simulate that sendRedirectToEvent is NEVER called (e.g., error in BPMN flow)
+        // But Octane's RequestTerminated event still fires and calls reset()
+
+        // This should NOT be called in this scenario:
+        // HandleRedirectListener::sendRedirectToEvent();
+
+        // Octane reset still happens
+        HandleRedirectListener::reset();
+
+        // Verify no sensitive data leaked
+        $this->assertStateIsClean();
+    }
+}

tests/unit/ProcessMaker/Nayra/Repositories/EntityRepositoryTest.php

@@ -0,0 +1,66 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Tests\Unit\ProcessMaker\Nayra\Repositories;
+
+use Illuminate\Database\Eloquent\Model;
+use ProcessMaker\Nayra\Repositories\EntityRepository;
+use ReflectionProperty;
+use Tests\TestCase;
+
+class EntityRepositoryTest extends TestCase
+{
+    /**
+     * Create a concrete implementation of EntityRepository for testing.
+     */
+    private function createRepository(): EntityRepository
+    {
+        return new class extends EntityRepository {
+            public function create(array $transaction): ?Model
+            {
+                return null;
+            }
+
+            public function update(array $transaction): ?Model
+            {
+                return null;
+            }
+
+            public function save(array $transaction): ?Model
+            {
+                return null;
+            }
+        };
+    }
+
+    /**
+     * Test that $uid2id is NOT a static property (fix for Octane data leak).
+     *
+     * In Octane, static properties persist across requests. The critical fix
+     * changes $uid2id from "private static" to "private" so that each instance
+     * has its own isolated cache, preventing data leaks between requests.
+     */
+    public function test_uid2id_is_not_static(): void
+    {
+        $reflection = new ReflectionProperty(EntityRepository::class, 'uid2id');
+
+        $this->assertFalse(
+            $reflection->isStatic(),
+            'uid2id must NOT be static to prevent data leaks between requests in Octane'
+        );
+    }
+
+    /**
+     * Test that $uid2id is a private property.
+     */
+    public function test_uid2id_is_private(): void
+    {
+        $reflection = new ReflectionProperty(EntityRepository::class, 'uid2id');
+
+        $this->assertTrue(
+            $reflection->isPrivate(),
+            'uid2id should be private'
+        );
+    }
+}