Merge commit from fork

The standard extension only resets this state in its RSHUTDOWN, which worker
requests skip:

- http_get_last_response_headers() returned the upstream headers fetched by a
  previous request;
- strtok() without a string continued tokenizing a previous request's string;
- the URL rewriter variables accumulated while its output handler never
  restarted after the first request.

Author: dunglas

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()`, 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 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)).
 - **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.

frankenphp.c

@@ -3,8 +3,10 @@
 #include <Zend/zend_alloc.h>
 #include <Zend/zend_exceptions.h>
 #include <Zend/zend_interfaces.h>
+#include <Zend/zend_smart_str.h>
 #include <errno.h>
 #include <ext/spl/spl_exceptions.h>
+#include <ext/standard/basic_functions.h>
 #include <ext/standard/head.h>
 #include <ext/standard/info.h>
 #include <ext/standard/php_fopen_wrappers.h>
@@ -543,6 +545,12 @@ 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"));
@@ -552,6 +560,32 @@ static void frankenphp_reset_last_errors(void) {
   }
 }
 
+/* Adapted from the url_scanner RSHUTDOWN, which isn't exported */
+static void frankenphp_reset_url_scanner(url_adapt_state_ex_t *ctx) {
+  smart_str_free(&ctx->result);
+  smart_str_free(&ctx->buf);
+  smart_str_free(&ctx->tag);
+  smart_str_free(&ctx->arg);
+  smart_str_free(&ctx->attr_val);
+  smart_str_free(&ctx->form_app);
+  smart_str_free(&ctx->url_app);
+  ctx->active = 0;
+  ctx->tag_type = 0;
+  ctx->attr_type = 0;
+}
+
+/* Request 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));
+}
+
 static frankenphp_thread_metrics *thread_metrics = NULL;
 
 /* Adapted from php_request_shutdown */
@@ -582,6 +616,7 @@ static void frankenphp_worker_request_shutdown() {
    * etc.) */
   zend_try { php_output_deactivate(); }
   zend_end_try();
+  frankenphp_reset_basic_state();
 
   /* sapi_deactivate() drops the request body stream without closing it, it
    * would otherwise stay open until the worker script ends */

frankenphp_test.go

@@ -1260,8 +1260,16 @@ 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)
 
@@ -1272,12 +1280,21 @@ func TestLastErrorsNoLeakBetweenRequests_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)
+		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)
 		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")

testdata/worker-last-errors.php

@@ -15,12 +15,21 @@
 		if (class_exists(DOMDocument::class)) {
 			(new DOMDocument())->loadXML("<root>&secret_{$_GET['leak']};</root>");
 		}
+		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,
 	]);
 };