fix(worker): session leak between requests

Author: xavierleune

frankenphp.c

@@ -155,6 +155,13 @@ static void frankenphp_reset_super_globals() {
     zval *files = &PG(http_globals)[TRACK_VARS_FILES];
     zval_ptr_dtor_nogc(files);
     memset(files, 0, sizeof(*files));
+
+    /* $_SESSION must be explicitly deleted from the symbol table.
+     * Unlike other superglobals, $_SESSION is stored in EG(symbol_table)
+     * with a reference to PS(http_session_vars). The session RSHUTDOWN
+     * only decrements the refcount but doesn't remove it from the symbol
+     * table, causing data to leak between requests. */
+    zend_hash_str_del(&EG(symbol_table), "_SESSION", sizeof("_SESSION") - 1);
   }
   zend_end_try();
 

frankenphp_test.go

@@ -27,6 +27,7 @@ import (
 	"strings"
 	"sync"
 	"testing"
+	"time"
 
 	"github.com/dunglas/frankenphp"
 	"github.com/dunglas/frankenphp/internal/fastabs"
@@ -1306,3 +1307,111 @@ func TestIniPreLoopPreserved_worker(t *testing.T) {
 		realServer:         true,
 	})
 }
+
+func TestSessionNoLeakBetweenRequests_worker(t *testing.T) {
+	runTest(t, func(_ func(http.ResponseWriter, *http.Request), ts *httptest.Server, i int) {
+		// Client A: Set a secret value in session
+		clientA := &http.Client{}
+		resp1, err := clientA.Get(ts.URL + "/session-leak.php?action=set&value=secret_A&client_id=clientA")
+		assert.NoError(t, err)
+		body1, _ := io.ReadAll(resp1.Body)
+		_ = resp1.Body.Close()
+
+		body1Str := string(body1)
+		t.Logf("Client A set session: %s", body1Str)
+		assert.Contains(t, body1Str, "SESSION_SET")
+		assert.Contains(t, body1Str, "secret=secret_A")
+
+		// Client B: Check that session is empty (no cookie, should not see Client A's data)
+		clientB := &http.Client{}
+		resp2, err := clientB.Get(ts.URL + "/session-leak.php?action=check_empty")
+		assert.NoError(t, err)
+		body2, _ := io.ReadAll(resp2.Body)
+		_ = resp2.Body.Close()
+
+		body2Str := string(body2)
+		t.Logf("Client B check empty: %s", body2Str)
+		assert.Contains(t, body2Str, "SESSION_CHECK")
+		assert.Contains(t, body2Str, "SESSION_EMPTY=true",
+			"Client B should have empty session, not see Client A's data.\nResponse: %s", body2Str)
+		assert.NotContains(t, body2Str, "secret_A",
+			"Client A's secret should not leak to Client B.\nResponse: %s", body2Str)
+
+		// Client C: Read session without cookie (should also be empty)
+		clientC := &http.Client{}
+		resp3, err := clientC.Get(ts.URL + "/session-leak.php?action=get")
+		assert.NoError(t, err)
+		body3, _ := io.ReadAll(resp3.Body)
+		_ = resp3.Body.Close()
+
+		body3Str := string(body3)
+		t.Logf("Client C get session: %s", body3Str)
+		assert.Contains(t, body3Str, "SESSION_READ")
+		assert.Contains(t, body3Str, "secret=NOT_FOUND",
+			"Client C should not find any secret.\nResponse: %s", body3Str)
+		assert.Contains(t, body3Str, "client_id=NOT_FOUND",
+			"Client C should not find any client_id.\nResponse: %s", body3Str)
+
+	}, &testOptions{
+		workerScript:       "session-leak.php",
+		nbWorkers:          1,
+		nbParallelRequests: 1,
+		realServer:         true,
+	})
+}
+
+func TestSessionNoLeakAfterExit_worker(t *testing.T) {
+	runTest(t, func(_ func(http.ResponseWriter, *http.Request), ts *httptest.Server, i int) {
+		// Client A: Set a secret value in session and call exit(1)
+		clientA := &http.Client{}
+		resp1, err := clientA.Get(ts.URL + "/session-leak.php?action=set_and_exit&value=exit_secret&client_id=exitClient")
+		assert.NoError(t, err)
+		body1, _ := io.ReadAll(resp1.Body)
+		_ = resp1.Body.Close()
+
+		body1Str := string(body1)
+		t.Logf("Client A set and exit: %s", body1Str)
+		// The response may be incomplete due to exit(1)
+		assert.Contains(t, body1Str, "BEFORE_EXIT")
+
+		// Client B: Check that session is empty (should not see Client A's data)
+		// Retry until the worker has restarted after exit(1)
+		clientB := &http.Client{}
+		var body2Str string
+		assert.Eventually(t, func() bool {
+			resp2, err := clientB.Get(ts.URL + "/session-leak.php?action=check_empty")
+			if err != nil {
+				return false
+			}
+			body2, _ := io.ReadAll(resp2.Body)
+			_ = resp2.Body.Close()
+			body2Str = string(body2)
+			return strings.Contains(body2Str, "SESSION_CHECK")
+		}, 2*time.Second, 10*time.Millisecond, "Worker did not restart in time after exit(1)")
+
+		t.Logf("Client B check empty after exit: %s", body2Str)
+		assert.Contains(t, body2Str, "SESSION_EMPTY=true",
+			"Client B should have empty session after Client A's exit(1).\nResponse: %s", body2Str)
+		assert.NotContains(t, body2Str, "exit_secret",
+			"Client A's secret should not leak to Client B after exit(1).\nResponse: %s", body2Str)
+
+		// Client C: Try to read session (should also be empty)
+		clientC := &http.Client{}
+		resp3, err := clientC.Get(ts.URL + "/session-leak.php?action=get")
+		assert.NoError(t, err)
+		body3, _ := io.ReadAll(resp3.Body)
+		_ = resp3.Body.Close()
+
+		body3Str := string(body3)
+		t.Logf("Client C get session after exit: %s", body3Str)
+		assert.Contains(t, body3Str, "SESSION_READ")
+		assert.Contains(t, body3Str, "secret=NOT_FOUND",
+			"Client C should not find any secret after exit(1).\nResponse: %s", body3Str)
+
+	}, &testOptions{
+		workerScript:       "session-leak.php",
+		nbWorkers:          1,
+		nbParallelRequests: 1,
+		realServer:         true,
+	})
+}

testdata/session-leak.php

@@ -0,0 +1,62 @@
+<?php
+
+require_once __DIR__.'/_executor.php';
+
+return function () {
+    $action = $_GET['action'] ?? 'check';
+    $output = [];
+
+    switch ($action) {
+        case 'set':
+            // Set a value in session
+            session_start();
+            $_SESSION['secret'] = $_GET['value'] ?? 'default_secret';
+            $_SESSION['client_id'] = $_GET['client_id'] ?? 'unknown';
+            session_write_close();
+            $output[] = 'SESSION_SET';
+            $output[] = 'secret=' . $_SESSION['secret'];
+            break;
+
+        case 'get':
+            // Read session and return values
+            session_start();
+            $output[] = 'SESSION_READ';
+            $output[] = 'secret=' . ($_SESSION['secret'] ?? 'NOT_FOUND');
+            $output[] = 'client_id=' . ($_SESSION['client_id'] ?? 'NOT_FOUND');
+            $output[] = 'session_id=' . session_id();
+            session_write_close();
+            break;
+
+        case 'set_and_exit':
+            // Set a value in session and exit without calling session_write_close
+            session_start();
+            $_SESSION['secret'] = $_GET['value'] ?? 'exit_secret';
+            $_SESSION['client_id'] = $_GET['client_id'] ?? 'exit_client';
+            // Intentionally NOT calling session_write_close() before exit
+            $output[] = 'BEFORE_EXIT';
+            echo implode("\n", $output);
+            flush();
+            exit(1);
+            break;
+
+        case 'check_empty':
+            // Check that session is empty (no leak from other clients)
+            // Note: We intentionally do NOT call session_start() here.
+            // $_SESSION should be empty without starting a session.
+            // If data leaks from previous requests, this test will catch it.
+            $output[] = 'SESSION_CHECK';
+            if (empty($_SESSION)) {
+                $output[] = 'SESSION_EMPTY=true';
+            } else {
+                $output[] = 'SESSION_EMPTY=false';
+                $output[] = 'LEAKED_DATA=' . json_encode($_SESSION);
+            }
+            $output[] = 'session_id=' . session_id();
+            break;
+
+        default:
+            $output[] = 'UNKNOWN_ACTION';
+    }
+
+    echo implode("\n", $output);
+};