From 9ffc8bcac7a21cd5ecb045ba9efed7382505c47d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Fri, 30 Oct 2026 13:39:33 +0100 Subject: [PATCH 1/2] fix: only reset request state between worker requests Worker mode shares state across requests like a long-running CLI script. Resetting strtok() and http_get_last_response_headers() made state unrelated to the HTTP request behave differently from userland state. The URL rewriter is still reset because it belongs to the response output. --- docs/security.md | 2 +- docs/worker.md | 4 +++- frankenphp.c | 13 +------------ frankenphp_test.go | 16 +--------------- testdata/worker-last-errors.php | 7 ------- 5 files changed, 6 insertions(+), 36 deletions(-) diff --git a/docs/security.md b/docs/security.md index cbb4c06d19..73c02409c5 100644 --- a/docs/security.md +++ b/docs/security.md @@ -46,7 +46,7 @@ These are the surfaces FrankenPHP owns. A vulnerability here is a FrankenPHP vul - **Request to superglobal mapping** (`cgi.go`, `frankenphp_register_server_vars`): building `$_SERVER`, `REMOTE_ADDR`, `SCRIPT_NAME`, `PATH_INFO`, and the other CGI variables from the request. - **PHP script-path resolution**: the request path is split on `split_path` (`.php` by default) into `SCRIPT_NAME` / `PATH_INFO`, then joined to the document root with `sanitizedPathJoin` (`filepath.Join(root, filepath.Clean("/"+reqPath))`), which keeps `SCRIPT_FILENAME` from escaping the document root (path traversal). The `php_server` directive additionally sets a default `try_files` rewrite that routes requests to existing files or the front controller, mitigating the classic PHP-FPM pitfall of executing the wrong file. -- **Worker-mode state isolation**: FrankenPHP resets `$_GET`, `$_POST`, `$_COOKIE`, `$_FILES`, `$_SERVER`, and `$_REQUEST` between requests, explicitly clears `$_SESSION` (which would otherwise leak between requests) and the errors returned by `error_get_last()` and `libxml_get_errors()`, and the headers returned by `http_get_last_response_headers()`, the `strtok()` state, and the variables added by `output_add_rewrite_var()`, but **`$_ENV` is not reset**, and `putenv()` writes, `static` variables, class static properties, globals, and runtime engine settings (`ini_set()`, the default stream context, the default timezone, the working directory, ...) persist across requests on the same thread. Request- or user-specific data left in that state can leak into a later request (see [Worker Mode](worker.md#state-persistence)). +- **Worker-mode state isolation**: FrankenPHP resets `$_GET`, `$_POST`, `$_COOKIE`, `$_FILES`, `$_SERVER`, and `$_REQUEST` between requests, explicitly clears `$_SESSION` (which would otherwise leak between requests) and the errors returned by `error_get_last()` and `libxml_get_errors()`, and the variables added by `output_add_rewrite_var()`, but **`$_ENV` is not reset**, and `putenv()` writes, the state of standard functions unrelated to the request (`strtok()`, `http_get_last_response_headers()`, ...), `static` variables, class static properties, globals, and runtime engine settings (`ini_set()`, the default stream context, the default timezone, the working directory, ...) persist across requests on the same thread. Request- or user-specific data left in that state can leak into a later request (see [Worker Mode](worker.md#state-persistence)). - **Per-thread environment sandboxing**: `frankenphp_putenv()` / `frankenphp_getenv()` operate on a thread-local `sandboxed_env`. `putenv()` writes stay inside that sandbox and never reach the process-global OS environment, so a secret set by one request cannot leak to child processes (`proc_open`, `exec`, ...), to the Go/Caddy side, or to other requests. A child process that must see such a variable has to be given it explicitly, through the environment argument of `proc_open()` or `exec()` (see [Internals](internals.md#per-thread-environment-sandboxing)). - **CGO memory boundary**: Go string pinning and `C.CString()` / `free()` lifetimes across the Go ↔ C boundary. - **Caddy admin API**: the `/frankenphp/workers/restart` and `/frankenphp/threads` endpoints, exposed through Caddy's admin API (which listens on `localhost:2019` by default). Exposing that endpoint beyond localhost is an operator decision. diff --git a/docs/worker.md b/docs/worker.md index 1b6c86a225..c148dd5144 100644 --- a/docs/worker.md +++ b/docs/worker.md @@ -236,13 +236,15 @@ Avoid storing request-specific or sensitive data in `$_ENV`. ## State persistence -Because worker mode keeps the PHP process alive between requests, the following state persists across requests: +Because worker mode keeps the PHP process alive between requests, FrankenPHP only resets the state tied to the HTTP request being handled: superglobals (except `$_ENV`), the request body, response headers and output, and the session. +Everything else is shared between the requests handled by the same thread, as in a long-running CLI script, including: - **Static variables**: Variables declared with `static` inside functions or methods retain their values between requests. - **Class static properties**: Static properties on classes persist between requests. - **Global variables**: Variables in the global scope of the worker script persist between requests. - **In-memory caches**: Any data stored in memory (arrays, objects) outside the request handler persists. - **Runtime engine settings**: Changes made with `ini_set()`, `stream_context_set_default()`, `date_default_timezone_set()`, `chdir()`, `stream_wrapper_register()`, `set_error_handler()` or `set_exception_handler()` stay in effect for later requests. Never store request- or user-specific values there, such as credentials in the default stream context: pass an explicit context to each call instead. +- **Function state unrelated to the request**: `strtok()` continues the string it was given in a previous request, and `http_get_last_response_headers()` returns the response headers of the last HTTP call made on the thread, even by a previous request. Call `http_clear_last_response_headers()` to clear them. - **Process-wide settings**: `umask()` and `setlocale()` change the whole FrankenPHP process, not only the current thread. Worker mode doesn't restore them between requests, and in every mode they also affect requests running at the same time. Set them once at startup, or call `chmod()` on created files instead of changing the umask. This is by design and is what makes worker mode fast. However, it requires attention to avoid unintended side effects: diff --git a/frankenphp.c b/frankenphp.c index 4066845349..99f5294440 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -545,12 +545,6 @@ static void frankenphp_reset_last_errors(void) { PG(last_error_type) = 0; PG(last_error_lineno) = 0; -#if PHP_VERSION_ID >= 80400 - /* http_get_last_response_headers() */ - zval_ptr_dtor(&BG(last_http_headers)); - ZVAL_UNDEF(&BG(last_http_headers)); -#endif - /* Called by name to avoid depending on libxml2 headers */ zend_function *libxml_clear_errors = zend_hash_str_find_ptr( CG(function_table), ZEND_STRL("libxml_clear_errors")); @@ -574,13 +568,8 @@ static void frankenphp_reset_url_scanner(url_adapt_state_ex_t *ctx) { ctx->attr_type = 0; } -/* Request state of the standard extension, freed by its RSHUTDOWN */ +/* Response state of the standard extension, freed by its RSHUTDOWN */ static void frankenphp_reset_basic_state(void) { - if (BG(strtok_string)) { - zend_string_release(BG(strtok_string)); - BG(strtok_string) = NULL; - } - /* output_add_rewrite_var() and session.use_trans_sid */ frankenphp_reset_url_scanner(&BG(url_adapt_session_ex)); frankenphp_reset_url_scanner(&BG(url_adapt_output_ex)); diff --git a/frankenphp_test.go b/frankenphp_test.go index 84199ba81d..f3bb3d7026 100644 --- a/frankenphp_test.go +++ b/frankenphp_test.go @@ -1260,16 +1260,9 @@ func TestLastErrorsNoLeakBetweenRequests_worker(t *testing.T) { LibxmlErrors *[]string `json:"libxml_errors"` LibxmlLastError *string `json:"libxml_last_error"` InternalErrors *bool `json:"internal_errors"` - LastResponse *[]string `json:"last_response_headers"` - Strtok any `json:"strtok"` Rewriter *bool `json:"rewriter"` } - upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("X-Secret", r.URL.Query().Get("s")) - })) - t.Cleanup(upstream.Close) - get := func(url string, handler func(http.ResponseWriter, *http.Request)) lastErrors { body, _ := testGet(url, handler, t) @@ -1280,22 +1273,15 @@ func TestLastErrorsNoLeakBetweenRequests_worker(t *testing.T) { } runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, i int) { - fetch := url.QueryEscape(fmt.Sprintf("%s/?s=secret_%d", upstream.URL, i)) - leaked := get(fmt.Sprintf("http://example.com/worker-last-errors.php?leak=%d&fetch=%s", i, fetch), handler) + leaked := get(fmt.Sprintf("http://example.com/worker-last-errors.php?leak=%d", i), handler) require.NotNil(t, leaked.LastError) assert.Equal(t, fmt.Sprintf("secret %d", i), *leaked.LastError) e := get("http://example.com/worker-last-errors.php", handler) assert.Nil(t, e.LastError) - assert.Equal(t, false, e.Strtok) require.NotNil(t, e.Rewriter) assert.True(t, *e.Rewriter, "the URL rewriter must restart in each request") - if leaked.LastResponse != nil { - assert.Contains(t, *leaked.LastResponse, fmt.Sprintf("X-Secret: secret_%d", i)) - assert.Nil(t, e.LastResponse) - } - if leaked.LibxmlErrors == nil { t.Log("libxml not available, skipping libxml assertions") diff --git a/testdata/worker-last-errors.php b/testdata/worker-last-errors.php index da707c54c4..2c88d4d748 100644 --- a/testdata/worker-last-errors.php +++ b/testdata/worker-last-errors.php @@ -15,21 +15,14 @@ if (class_exists(DOMDocument::class)) { (new DOMDocument())->loadXML("&secret_{$_GET['leak']};"); } - strtok("secret {$_GET['leak']}", ' '); output_add_rewrite_var('token', "secret_{$_GET['leak']}"); - if (isset($_GET['fetch'])) { - file_get_contents($_GET['fetch']); - } } echo json_encode([ 'last_error' => error_get_last()['message'] ?? null, - // evaluated after last_error: strtok() warns when there is nothing to continue - 'strtok' => isset($_GET['leak']) ? null : @strtok(' '), 'rewriter' => isset($_GET['leak']) ? null : output_add_rewrite_var('x', 'y') && in_array('URL-Rewriter', ob_list_handlers(), true), 'libxml_errors' => $hasLibxml ? array_map(fn ($e) => trim($e->message), libxml_get_errors()) : null, 'libxml_last_error' => $hasLibxml ? (libxml_get_last_error()->message ?? null) : null, 'internal_errors' => $hasLibxml ? libxml_use_internal_errors() : null, - 'last_response_headers' => function_exists('http_get_last_response_headers') ? http_get_last_response_headers() : null, ]); }; From 44edd55def1fa62f08313e348cd4982fc90943d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Fri, 30 Oct 2026 13:40:30 +0100 Subject: [PATCH 2/2] fix: keep last errors across worker requests error_get_last() and libxml_get_errors() report engine and extension state unrelated to the HTTP request. Like other shared state in worker mode, the application clears it when needed. --- docs/security.md | 2 +- docs/worker.md | 21 ++++++++++++++- frankenphp.c | 26 ------------------- frankenphp_test.go | 44 ++++---------------------------- testdata/worker-last-errors.php | 28 -------------------- testdata/worker-url-rewriter.php | 13 ++++++++++ 6 files changed, 39 insertions(+), 95 deletions(-) delete mode 100644 testdata/worker-last-errors.php create mode 100644 testdata/worker-url-rewriter.php diff --git a/docs/security.md b/docs/security.md index 73c02409c5..b6f34665b7 100644 --- a/docs/security.md +++ b/docs/security.md @@ -46,7 +46,7 @@ These are the surfaces FrankenPHP owns. A vulnerability here is a FrankenPHP vul - **Request to superglobal mapping** (`cgi.go`, `frankenphp_register_server_vars`): building `$_SERVER`, `REMOTE_ADDR`, `SCRIPT_NAME`, `PATH_INFO`, and the other CGI variables from the request. - **PHP script-path resolution**: the request path is split on `split_path` (`.php` by default) into `SCRIPT_NAME` / `PATH_INFO`, then joined to the document root with `sanitizedPathJoin` (`filepath.Join(root, filepath.Clean("/"+reqPath))`), which keeps `SCRIPT_FILENAME` from escaping the document root (path traversal). The `php_server` directive additionally sets a default `try_files` rewrite that routes requests to existing files or the front controller, mitigating the classic PHP-FPM pitfall of executing the wrong file. -- **Worker-mode state isolation**: FrankenPHP resets `$_GET`, `$_POST`, `$_COOKIE`, `$_FILES`, `$_SERVER`, and `$_REQUEST` between requests, explicitly clears `$_SESSION` (which would otherwise leak between requests) and the errors returned by `error_get_last()` and `libxml_get_errors()`, and the variables added by `output_add_rewrite_var()`, but **`$_ENV` is not reset**, and `putenv()` writes, the state of standard functions unrelated to the request (`strtok()`, `http_get_last_response_headers()`, ...), `static` variables, class static properties, globals, and runtime engine settings (`ini_set()`, the default stream context, the default timezone, the working directory, ...) persist across requests on the same thread. Request- or user-specific data left in that state can leak into a later request (see [Worker Mode](worker.md#state-persistence)). +- **Worker-mode state isolation**: FrankenPHP resets `$_GET`, `$_POST`, `$_COOKIE`, `$_FILES`, `$_SERVER`, and `$_REQUEST` between requests, explicitly clears `$_SESSION` (which would otherwise leak between requests) and the variables added by `output_add_rewrite_var()`, but **`$_ENV` is not reset**, and `putenv()` writes, the last-error state of PHP and its extensions (`error_get_last()`, `libxml_get_errors()`, ...), the state of other functions unrelated to the request (`strtok()`, `http_get_last_response_headers()`, ...), `static` variables, class static properties, globals, and runtime engine settings (`ini_set()`, the default stream context, the default timezone, the working directory, ...) persist across requests on the same thread. Request- or user-specific data left in that state can leak into a later request (see [Worker Mode](worker.md#state-persistence)). - **Per-thread environment sandboxing**: `frankenphp_putenv()` / `frankenphp_getenv()` operate on a thread-local `sandboxed_env`. `putenv()` writes stay inside that sandbox and never reach the process-global OS environment, so a secret set by one request cannot leak to child processes (`proc_open`, `exec`, ...), to the Go/Caddy side, or to other requests. A child process that must see such a variable has to be given it explicitly, through the environment argument of `proc_open()` or `exec()` (see [Internals](internals.md#per-thread-environment-sandboxing)). - **CGO memory boundary**: Go string pinning and `C.CString()` / `free()` lifetimes across the Go ↔ C boundary. - **Caddy admin API**: the `/frankenphp/workers/restart` and `/frankenphp/threads` endpoints, exposed through Caddy's admin API (which listens on `localhost:2019` by default). Exposing that endpoint beyond localhost is an operator decision. diff --git a/docs/worker.md b/docs/worker.md index c148dd5144..b2f019ff88 100644 --- a/docs/worker.md +++ b/docs/worker.md @@ -244,7 +244,7 @@ Everything else is shared between the requests handled by the same thread, as in - **Global variables**: Variables in the global scope of the worker script persist between requests. - **In-memory caches**: Any data stored in memory (arrays, objects) outside the request handler persists. - **Runtime engine settings**: Changes made with `ini_set()`, `stream_context_set_default()`, `date_default_timezone_set()`, `chdir()`, `stream_wrapper_register()`, `set_error_handler()` or `set_exception_handler()` stay in effect for later requests. Never store request- or user-specific values there, such as credentials in the default stream context: pass an explicit context to each call instead. -- **Function state unrelated to the request**: `strtok()` continues the string it was given in a previous request, and `http_get_last_response_headers()` returns the response headers of the last HTTP call made on the thread, even by a previous request. Call `http_clear_last_response_headers()` to clear them. +- **Engine and extension state**: `error_get_last()`, `libxml_get_errors()` and `libxml_get_last_error()` return the errors an earlier request left behind. `http_get_last_response_headers()` returns the response headers of the last HTTP call made on the thread, even by an earlier request. `strtok()` continues the string an earlier request gave it. After `mt_srand()` or `srand()` with a seed, `mt_rand()`, `rand()`, `shuffle()` and `array_rand()` continue that seeded sequence. The errors, headers and strings can contain another client's data, and a seeded sequence makes random values predictable: reset them as shown below. - **Process-wide settings**: `umask()` and `setlocale()` change the whole FrankenPHP process, not only the current thread. Worker mode doesn't restore them between requests, and in every mode they also affect requests running at the same time. Set them once at startup, or call `chmod()` on created files instead of changing the umask. This is by design and is what makes worker mode fast. However, it requires attention to avoid unintended side effects: @@ -266,4 +266,23 @@ while (\frankenphp_handle_request($handler)) { ``` When writing worker scripts, make sure to reset any request-specific state between requests. +PHP provides functions to reset the engine and extension state listed above: + +```php +result); @@ -599,8 +575,6 @@ static void frankenphp_worker_request_shutdown() { frankenphp_reset_session_state(); #endif - frankenphp_reset_last_errors(); - /* Shutdown output layer (send the set HTTP headers, cleanup output handlers, * etc.) */ zend_try { php_output_deactivate(); } diff --git a/frankenphp_test.go b/frankenphp_test.go index f3bb3d7026..458d5e3b4e 100644 --- a/frankenphp_test.go +++ b/frankenphp_test.go @@ -1254,47 +1254,13 @@ func TestFileStreamInWorkerMode(t *testing.T) { }, &testOptions{workerScript: "file-stream.php", nbParallelRequests: 1, nbWorkers: 1}) } -func TestLastErrorsNoLeakBetweenRequests_worker(t *testing.T) { - type lastErrors struct { - LastError *string `json:"last_error"` - LibxmlErrors *[]string `json:"libxml_errors"` - LibxmlLastError *string `json:"libxml_last_error"` - InternalErrors *bool `json:"internal_errors"` - Rewriter *bool `json:"rewriter"` - } - - get := func(url string, handler func(http.ResponseWriter, *http.Request)) lastErrors { - body, _ := testGet(url, handler, t) - - var e lastErrors - require.NoError(t, json.Unmarshal([]byte(body), &e), body) - - return e - } - +func TestURLRewriterRestartsBetweenRequests_worker(t *testing.T) { runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, i int) { - leaked := get(fmt.Sprintf("http://example.com/worker-last-errors.php?leak=%d", i), handler) - require.NotNil(t, leaked.LastError) - assert.Equal(t, fmt.Sprintf("secret %d", i), *leaked.LastError) - - e := get("http://example.com/worker-last-errors.php", handler) - assert.Nil(t, e.LastError) - require.NotNil(t, e.Rewriter) - assert.True(t, *e.Rewriter, "the URL rewriter must restart in each request") - - if leaked.LibxmlErrors == nil { - t.Log("libxml not available, skipping libxml assertions") - - return - } - - require.NotEmpty(t, *leaked.LibxmlErrors) - assert.Contains(t, (*leaked.LibxmlErrors)[0], fmt.Sprintf("secret_%d", i)) + testGet(fmt.Sprintf("http://example.com/worker-url-rewriter.php?leak=%d", i), handler, t) - assert.Empty(t, *e.LibxmlErrors) - assert.Nil(t, e.LibxmlLastError) - assert.True(t, *e.InternalErrors) - }, &testOptions{workerScript: "worker-last-errors.php", nbParallelRequests: 1, nbWorkers: 1}) + body, _ := testGet("http://example.com/worker-url-rewriter.php", handler, t) + assert.Equal(t, "true", body, "the URL rewriter must restart in each request") + }, &testOptions{workerScript: "worker-url-rewriter.php", nbParallelRequests: 1, nbWorkers: 1}) } // To run this fuzzing test use: go test -fuzz FuzzRequest diff --git a/testdata/worker-last-errors.php b/testdata/worker-last-errors.php deleted file mode 100644 index 2c88d4d748..0000000000 --- a/testdata/worker-last-errors.php +++ /dev/null @@ -1,28 +0,0 @@ -loadXML("&secret_{$_GET['leak']};"); - } - output_add_rewrite_var('token', "secret_{$_GET['leak']}"); - } - - echo json_encode([ - 'last_error' => error_get_last()['message'] ?? null, - 'rewriter' => isset($_GET['leak']) ? null : output_add_rewrite_var('x', 'y') && in_array('URL-Rewriter', ob_list_handlers(), true), - 'libxml_errors' => $hasLibxml ? array_map(fn ($e) => trim($e->message), libxml_get_errors()) : null, - 'libxml_last_error' => $hasLibxml ? (libxml_get_last_error()->message ?? null) : null, - 'internal_errors' => $hasLibxml ? libxml_use_internal_errors() : null, - ]); -}; diff --git a/testdata/worker-url-rewriter.php b/testdata/worker-url-rewriter.php new file mode 100644 index 0000000000..760d57f1b3 --- /dev/null +++ b/testdata/worker-url-rewriter.php @@ -0,0 +1,13 @@ +