diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..c83d7c4 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,37 @@ +name: CI + +on: + push: + branches: [ main ] + pull_request: + +permissions: + contents: read + +jobs: + test: + name: PHPUnit (PHP ${{ matrix.php }}) + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + php: ['8.2', '8.3', '8.4'] + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Setup PHP + uses: shivammathur/setup-php@v2 + with: + php-version: ${{ matrix.php }} + coverage: none + tools: composer:v2 + + # The unit suite runs WITHOUT the native ePHPm engine: the Ephpm\Worker\* + # primitives are provided by test shims / the IDE stub, so `composer install` + # + phpunit is all that is needed. + - name: Install dependencies + run: composer install --prefer-dist --no-interaction --no-progress + + - name: Run PHPUnit + run: vendor/bin/phpunit diff --git a/README.md b/README.md index 19d89dd..9ea47f2 100644 --- a/README.md +++ b/README.md @@ -61,6 +61,14 @@ inclusion, not request handling. Handlers on them typically do class wiring, translation loading, or user-derivation — safe to run once. Re-firing would double-fire class-wiring code with no request-scoped upside. +The **current user** is handled differently, though: the per-request reset +*unsets* `$current_user` (and the legacy `$user_ID`/`$userdata`/… identity +globals — see `Worker::RESET_GLOBALS`), so the next `wp_get_current_user()` +re-derives the user from **this** request's engine-fresh `$_COOKIE` and re-fires +`set_current_user` lazily. Without that reset a logged-in request would pin its +identity onto the resident worker and the next anonymous visitor would be seen +as that user — a cross-request identity leak. + **Why fire `shutdown` per iteration?** In classic FPM, `shutdown` fires when PHP tears the script down at request end (via `register_shutdown_function` → WordPress' `shutdown_action_hook()`). In @@ -91,8 +99,8 @@ broken plugin should not take down the worker pool. `register_post_type(…)` inside its own `init` body will register a fresh callback / a fresh post-type on every request, growing `$wp_filter` / `$wp_post_types` unboundedly. Well-behaved plugins register hooks at - plugin-load time (or self-gate with a static flag); the `worker_max_requests` - ceiling bounds the blast radius for the ones that don't. + plugin-load time (or self-gate with a static flag); the `[php.worker] + max_requests` ceiling bounds the blast radius for the ones that don't. - **Handlers on `shutdown` that call `exit`.** Under FPM, `shutdown` fires from PHP's tear-down and any `exit`/`die` inside a handler is harmless (the script was ending anyway). Under the worker, an `exit` inside a @@ -111,7 +119,7 @@ broken plugin should not take down the worker pool. ## Requirements - ePHPm built with worker mode (`[php] mode = "worker"`). -- **`worker_populate_superglobals = true`** — WordPress assumes real +- **`[php.worker] populate_superglobals = true`** — WordPress assumes real `$_SERVER/$_GET/$_COOKIE` superglobals. This is **not** the default; you must turn it on. Without it, WordPress routing and `$wpdb`-driven queries misbehave. @@ -153,12 +161,21 @@ document_root = "/var/www/html" # the WordPress root (ABSPATH) [php] mode = "worker" -worker_script = "worker.php" # your loop; see below -worker_populate_superglobals = true # REQUIRED for WordPress -worker_count = 4 # WordPress ~40 MB/worker — size accordingly -worker_max_requests = 500 # recycle to reclaim slow memory growth +concurrency = 4 # worker-thread pool size — WordPress ~40 MB/worker, size accordingly (0 = auto) + +[php.worker] +script = "worker.php" # your loop; see below +populate_superglobals = true # REQUIRED for WordPress +max_requests = 500 # recycle to reclaim slow memory growth ``` +The worker knobs live in the `[php.worker]` table (`script`, `max_requests`, +`boot_timeout`, `populate_superglobals`, `stream_threshold`); pool sizing is the +whole-server `[php] concurrency` knob. The old flat `worker_*` keys +(`worker_script`, `worker_count`, `worker_populate_superglobals`, +`worker_max_requests`, `worker_boot_timeout`) were removed and are now a **hard +startup error** — migrate any existing config. + Set `EPHPM_WP_PATH` to the WordPress root (defaults to `document_root` via the `wp-load.php` walk if omitted): @@ -166,15 +183,12 @@ Set `EPHPM_WP_PATH` to the WordPress root (defaults to `document_root` via the EPHPM_WP_PATH=/var/www/html ``` -Your `worker.php` (pointed to by `worker_script`) is a one-liner: - -```php -run()); -``` - -or just use the bundled `bin/ephpm-wp-worker` as your `worker_script`. +WordPress' request cycle must run at **global scope**, so this adapter does not +expose a `run()` method — the request loop lives in the worker entry script. Use +the bundled `bin/ephpm-wp-worker` as your `[php.worker] script` (it boots +WordPress once and runs the loop), or copy it into your project as `worker.php` +and point the config at your copy. See `bin/ephpm-wp-worker` for the canonical +global-scope loop. ## Running WooCommerce @@ -271,7 +285,7 @@ before shipping: global not on the list, or a **dynamically-registered hook** will leak across requests within a worker. (The bundled mu-plugin demonstrates the hook-leak trap and how to avoid it — register filters once, self-gate per request.) - Recycling (`worker_max_requests`) bounds the blast radius. + Recycling (`[php.worker] max_requests`) bounds the blast radius. - **`$_SERVER` is populated by this adapter.** The engine populates `$_GET` / `$_COOKIE` natively but leaves `$_SERVER` empty in worker mode, so the adapter fills `$_SERVER` (REQUEST_URI/HTTP_HOST/…) from the Envelope. It deliberately @@ -287,7 +301,7 @@ before shipping: - **Fatals recycle the worker.** An uncatchable fatal in a hook takes down the worker; ePHPm respawns it and serves the next request from a clean boot. A single request's fatal never serves stale state, but costs one worker boot. - Run enough workers (`worker_count`) that recycling doesn't starve the pool. + Run enough workers (`[php] concurrency`) that recycling doesn't starve the pool. ## Testing diff --git a/bin/ephpm-wp-worker b/bin/ephpm-wp-worker index b7fd363..eb37bcf 100644 --- a/bin/ephpm-wp-worker +++ b/bin/ephpm-wp-worker @@ -7,14 +7,14 @@ declare(strict_types=1); * ephpm-wp-worker — WordPress entrypoint for ePHPm persistent worker mode. * * ePHPm launches this script for each persistent worker (point - * `[php] worker_script` at it, or at a copy of it inside your project). It: + * `[php.worker] script` at it, or at a copy of it inside your project). It: * * 1. locates and loads this package's Composer autoloader, * 2. locates the WordPress root (EPHPM_WP_PATH env var, or ABSPATH if already * defined, or a walk upward looking for wp-load.php), * 3. boots WordPress ONCE and runs the worker loop via Ephpm\WordPress\Worker. * - * Requires `[php] worker_populate_superglobals = true` in ephpm.toml — WordPress + * Requires `[php.worker] populate_superglobals = true` in ephpm.toml — WordPress * assumes real $_SERVER/$_GET/$_COOKIE superglobals. See the package README. */ diff --git a/composer.json b/composer.json index 78694a1..b533d75 100644 --- a/composer.json +++ b/composer.json @@ -40,7 +40,7 @@ }, "extra": { "ephpm": { - "comment": "ephpm packages are distributed via their GitHub repositories, not Packagist. The `repositories` VCS entry is what makes this package's own ephpm/worker dependency (tag v0.1.0) resolve when running this package's tests. Consumers of ephpm/wordpress-worker must likewise add a Composer vcs repository for EACH ephpm package in the tree (Composer does not resolve VCS deps transitively) — see README. WordPress worker mode REQUIRES `[php] worker_populate_superglobals = true` in ephpm.toml — see README." + "comment": "ephpm packages are distributed via their GitHub repositories, not Packagist. The `repositories` VCS entry is what makes this package's own ephpm/worker dependency (tag v0.1.0) resolve when running this package's tests. Consumers of ephpm/wordpress-worker must likewise add a Composer vcs repository for EACH ephpm package in the tree (Composer does not resolve VCS deps transitively) — see README. WordPress worker mode REQUIRES `[php.worker] populate_superglobals = true` in ephpm.toml — see README." } } } diff --git a/e2e/app.toml b/e2e/app.toml index 4c5f99b..3c9e0f7 100644 --- a/e2e/app.toml +++ b/e2e/app.toml @@ -5,10 +5,12 @@ document_root = "/var/www/html" # the WordPress root (ABSPATH) [php] mode = "worker" -worker_script = "worker.php" -worker_populate_superglobals = true # REQUIRED for WordPress -worker_count = 3 # a few workers so exit-based routes - # (wp-login/REST end in exit -> recycle) - # don't starve the pool -worker_max_requests = 2000 -worker_boot_timeout = 120 # generous boot window for WordPress +concurrency = 3 # worker-thread pool size — a few workers so + # exit-based routes (wp-login/REST end in + # exit -> recycle) don't starve the pool + +[php.worker] +script = "worker.php" +populate_superglobals = true # REQUIRED for WordPress +max_requests = 2000 +boot_timeout = 120 # generous boot window for WordPress diff --git a/e2e/worker.php b/e2e/worker.php index 272c60d..7365a21 100644 --- a/e2e/worker.php +++ b/e2e/worker.php @@ -4,7 +4,7 @@ /** * Worker entrypoint for the e2e image. Lives under document_root - * (/var/www/html) so ephpm's worker_script validation accepts it. + * (/var/www/html) so ephpm's [php.worker] script validation accepts it. * * WordPress is booted ONCE at GLOBAL scope (a top-level require, not inside a * function), and the request loop also runs at global scope — wp() + the diff --git a/muplugins/elementor-idempotent-lifecycle.php b/muplugins/elementor-idempotent-lifecycle.php index b97c2ba..507892b 100644 --- a/muplugins/elementor-idempotent-lifecycle.php +++ b/muplugins/elementor-idempotent-lifecycle.php @@ -38,7 +38,7 @@ * SCOPE: this fixes the redeclaration fatal only. It does NOT audit the * remaining Elementor codepaths for per-request idempotency (e.g. widget-cache * clearing, breakpoint state, editor-preview flow). Ship together with a - * conservative `worker_max_requests` and instrument for regressions. + * conservative `[php.worker] max_requests` and instrument for regressions. */ add_action( diff --git a/src/Worker.php b/src/Worker.php index e0f5a04..8bcd5a3 100644 --- a/src/Worker.php +++ b/src/Worker.php @@ -33,8 +33,8 @@ * EMPIRICAL engine behaviour this adapter is built around (observed in the e2e * harness against a real ePHPm worker build — these CORRECT some of the * originally-assumed facts): - * - With `worker_populate_superglobals = true` the engine natively populates - * `$_GET` and `$_COOKIE`, but leaves `$_SERVER` essentially EMPTY (no + * - With `[php.worker] populate_superglobals = true` the engine natively + * populates `$_GET` and `$_COOKIE`, but leaves `$_SERVER` essentially EMPTY (no * `REQUEST_URI`/`REQUEST_METHOD`/`HTTP_HOST`). So this adapter populates * `$_SERVER` from the Envelope itself, and MUST NOT reassign `$_GET`/ * `$_COOKIE` — replacing the engine-owned superglobal zvals crashes the @@ -71,6 +71,15 @@ final class Worker * {@see resetRequestGlobals()}. Kept as a named constant so the exact * key-list is directly unit-testable and reviewable. * + * SECURITY: the current-user identity globals are in this list. A logged-in + * request sets `$current_user` (and the legacy `$user_ID`/`$userdata`/… + * globals `setup_userdata()` writes) on the resident worker; if they are not + * cleared, the NEXT — possibly anonymous — request served by the same worker + * inherits the previous visitor's identity. Unsetting `$current_user` forces + * WordPress to re-derive the user from THIS request's engine-fresh + * `$_COOKIE` on the next `wp_get_current_user()` call (which re-runs the + * `determine_current_user` filter and re-fires `set_current_user`). + * * @var list */ public const RESET_GLOBALS = [ @@ -90,6 +99,17 @@ final class Worker 'pagenow', 'typenow', 'taxnow', + // Current-user identity (SECURITY: must not leak request-to-request). + // `$current_user` is the load-bearing one WordPress re-derives from the + // auth cookie; the rest are the legacy globals `setup_userdata()` writes. + 'current_user', + 'user_ID', + 'userdata', + 'user_login', + 'user_email', + 'user_url', + 'user_identity', + 'user_level', ]; /** @@ -145,6 +165,17 @@ public function __construct(private readonly string $absPath) { } + /** + * The `$_SERVER` keys this adapter set from the previous request's Envelope, + * so {@see marshalSuperglobals()} can unset the ones the next request does + * not re-supply (stale HTTP_AUTHORIZATION / CONTENT_* / HTTP_X_* bleed). + * Static because a resident worker services one request at a time and the + * loop lives in the entry script, not on a single Worker instance. + * + * @var list + */ + private static array $marshaledServerKeys = []; + /** * Define the constants WordPress needs for a headless worker boot. * @@ -436,12 +467,30 @@ public function marshalSuperglobals(object $envelope): void $server = $envelope->serverVars(); $rawBody = (string) $envelope->rawBody(); + // Remove the $_SERVER keys THIS adapter set from a PRIOR request's + // Envelope that the current request does not re-supply. Without this, + // per-request server vars — most dangerously HTTP_AUTHORIZATION, but also + // CONTENT_TYPE / CONTENT_LENGTH and any HTTP_X_* header — bleed from an + // earlier request into a later one that omits them, because the engine + // leaves $_SERVER for the adapter to own and we only ever assigned keys. + // Engine-owned / adapter-later keys (e.g. SCRIPT_NAME set in + // beforeRequest()) are never in this tracked set, so they are untouched. + foreach (self::$marshaledServerKeys as $prevKey) { + if (!\array_key_exists($prevKey, $server)) { + unset($_SERVER[$prevKey]); + } + } + // Populate $_SERVER from the Envelope (engine leaves it empty). Assigning // individual keys (rather than replacing the whole array) avoids - // disturbing any engine-owned $_SERVER entries. + // disturbing any engine-owned $_SERVER entries. Track exactly the keys we + // set so the next request can retire the ones it does not re-supply. + $marshaled = []; foreach ($server as $k => $v) { $_SERVER[$k] = $v; + $marshaled[] = $k; } + self::$marshaledServerKeys = $marshaled; $contentType = self::headerValue($server, 'CONTENT_TYPE'); $method = \strtoupper((string) ($server['REQUEST_METHOD'] ?? ($_SERVER['REQUEST_METHOD'] ?? 'GET'))); @@ -559,7 +608,11 @@ public static function fireShutdownActions(): void * * Two-part reset: * 1. Unset the transient loop/template globals in {@see RESET_GLOBALS} — - * WordPress re-derives them during the template stack. + * WordPress re-derives them during the template stack. This includes + * the current-user identity globals (`$current_user`, `$user_ID`, …): + * clearing them makes WordPress re-derive the user from this request's + * engine-fresh `$_COOKIE`, so a logged-in request never leaks its + * identity onto the next visitor served by the same worker (SECURITY). * 2. Re-instantiate the query objects (`$wp_query`, `$wp_the_query`) as * fresh `WP_Query` instances and reset the `$wp` router's per-request * fields IN PLACE (not unset — `wp()` calls `$wp->main()` on it). @@ -831,6 +884,14 @@ public static function cleanupSpooledFiles(): void * Spool one uploaded file to a temp path and register it in `$files` * in `$_FILES` shape. * + * A bracketed field name (`photos[]`, `docs[main]`, …) is placed in PHP's + * pivoted `$_FILES` shape — the five per-file attributes are lifted ABOVE the + * bracket path, so `name="photos[]"` yields `$_FILES['photos']['name'][0]`, + * `['tmp_name'][0]`, … and a second `photos[]` part appends index `1` (rather + * than the old last-wins collapse under the literal key `"photos[]"`, which + * broke WordPress' Media Library "add multiple files" and any `name="x[]"` + * upload). A plain name keeps the scalar `$_FILES['photo']['name']` shape. + * * @param array $files */ private static function assignFile( @@ -854,13 +915,94 @@ private static function assignFile( } } - $files[$name] = [ + $spec = [ 'name' => $filename, 'type' => $type, 'tmp_name' => $tmp, 'error' => $error, 'size' => \strlen($content), ]; + + [$base, $path] = self::splitFieldName($name); + + if ($path === []) { + // Plain field name: the classic single-file $_FILES[base][attr] shape. + foreach ($spec as $attr => $value) { + $files[$base][$attr] = $value; + } + + return; + } + + // Bracketed name: PHP pivots the attributes above the bracket path, so + // each attribute array mirrors the field's structure. `[]` appends. + foreach ($spec as $attr => $value) { + if (!isset($files[$base]) || !\is_array($files[$base])) { + $files[$base] = []; + } + if (!isset($files[$base][$attr]) || !\is_array($files[$base][$attr])) { + $files[$base][$attr] = []; + } + self::assignByPath($files[$base][$attr], $path, $value); + } + } + + /** + * Split a form field name into its base and bracket path: + * `photos[]` → `['photos', ['']]`, `docs[a][b]` → `['docs', ['a', 'b']]`, + * `photo` → `['photo', []]`. An empty path segment (`''`) means append. + * + * @return array{0: string, 1: list} + */ + private static function splitFieldName(string $name): array + { + $pos = \strpos($name, '['); + if ($pos === false) { + return [$name, []]; + } + + $path = []; + if (\preg_match_all('/\[([^\]]*)\]/', \substr($name, $pos), $m) !== false) { + $path = $m[1]; + } + + return [\substr($name, 0, $pos), $path]; + } + + /** + * Assign `$value` into `$target` following a bracket `$path`. An empty + * segment (`''`) appends at the next integer index, matching PHP's `name[]`. + * + * @param array $target + * @param list $path + */ + private static function assignByPath(array &$target, array $path, mixed $value): void + { + $ref = &$target; + $last = \count($path) - 1; + foreach ($path as $i => $segment) { + if ($segment === '') { + if ($i === $last) { + $ref[] = $value; + + return; + } + $ref[] = []; + $ref = &$ref[\array_key_last($ref)]; + + continue; + } + + if ($i === $last) { + $ref[$segment] = $value; + + return; + } + if (!isset($ref[$segment]) || !\is_array($ref[$segment])) { + $ref[$segment] = []; + } + $ref = &$ref[$segment]; + } } /** diff --git a/tests/BodyParseTest.php b/tests/BodyParseTest.php index 0e5c815..73126b4 100644 --- a/tests/BodyParseTest.php +++ b/tests/BodyParseTest.php @@ -172,6 +172,37 @@ public function testMultipartArrayField(): void self::assertSame(['1', '2'], $post['cats']); } + public function testMultipartArrayOfFilesPivotsToPhpFilesShape(): void + { + // Two files under one name="photos[]" field plus a scalar name="avatar". + // PHP builds the pivoted $_FILES shape: the five attributes are lifted + // above the [] index, so BOTH files survive instead of the second + // last-winning under the literal key "photos[]". + $boundary = 'B'; + $body = self::multipart($boundary, [ + ['name' => 'photos[]', 'filename' => 'a.jpg', 'ctype' => 'image/jpeg', 'value' => 'AAA'], + ['name' => 'photos[]', 'filename' => 'b.jpg', 'ctype' => 'image/jpeg', 'value' => 'BBBB'], + ['name' => 'avatar', 'filename' => 'me.png', 'ctype' => 'image/png', 'value' => 'PNG'], + ]); + + [, $files] = Worker::parseBody('POST', "multipart/form-data; boundary={$boundary}", $body, self::env()); + + // Bracketed field: pivoted parallel arrays, index-aligned across attributes. + self::assertSame(['a.jpg', 'b.jpg'], $files['photos']['name']); + self::assertSame(['image/jpeg', 'image/jpeg'], $files['photos']['type']); + self::assertSame([3, 4], $files['photos']['size']); + self::assertSame([\UPLOAD_ERR_OK, \UPLOAD_ERR_OK], $files['photos']['error']); + self::assertCount(2, $files['photos']['tmp_name']); + self::assertSame('AAA', \file_get_contents($files['photos']['tmp_name'][0])); + self::assertSame('BBBB', \file_get_contents($files['photos']['tmp_name'][1])); + + // Scalar field: classic single-file shape (unchanged). + self::assertSame('me.png', $files['avatar']['name']); + self::assertIsString($files['avatar']['tmp_name']); + + Worker::cleanupSpooledFiles(); + } + /** * Build a multipart/form-data body from a list of parts. * diff --git a/tests/RequestIsolationTest.php b/tests/RequestIsolationTest.php new file mode 100644 index 0000000..21c0a7b --- /dev/null +++ b/tests/RequestIsolationTest.php @@ -0,0 +1,71 @@ + 7, 'user_login' => 'admin']; + $GLOBALS['user_ID'] = 7; + $GLOBALS['userdata'] = (object) ['ID' => 7, 'user_login' => 'admin']; + $GLOBALS['user_login'] = 'admin'; + + // --- Request N+1 begins (an anonymous visitor). The per-request reset + // must clear the pinned identity so WordPress re-derives the user from + // this request's (cookie-less) $_COOKIE instead of reusing admin. --- + $worker->resetRequestGlobals(); + + self::assertArrayNotHasKey('current_user', $GLOBALS, 'current_user leaked across requests'); + self::assertArrayNotHasKey('user_ID', $GLOBALS, 'user_ID leaked across requests'); + self::assertArrayNotHasKey('userdata', $GLOBALS, 'userdata leaked across requests'); + self::assertArrayNotHasKey('user_login', $GLOBALS, 'user_login leaked across requests'); + } +} diff --git a/tests/SuperglobalMarshalTest.php b/tests/SuperglobalMarshalTest.php index 2402dc8..2dcfc70 100644 --- a/tests/SuperglobalMarshalTest.php +++ b/tests/SuperglobalMarshalTest.php @@ -56,6 +56,41 @@ public function testServerVarsDoNotBleedRequestToRequest(): void self::assertSame('POST', $_SERVER['REQUEST_METHOD']); } + public function testStaleServerKeysDoNotBleedAcrossRequests(): void + { + $worker = new Worker('/tmp/wp/'); + + // Request 1 carries an Authorization header, a request body content type, + // and a custom HTTP_X_* header. + $worker->marshalSuperglobals(new FakeEnvelope( + server: [ + 'REQUEST_METHOD' => 'POST', + 'REQUEST_URI' => '/one', + 'HTTP_AUTHORIZATION' => 'Bearer secret-token', + 'CONTENT_TYPE' => 'application/json', + 'CONTENT_LENGTH' => '17', + 'HTTP_X_CUSTOM' => 'first', + ], + rawBody: '{"title":"hello"}', + )); + self::assertSame('Bearer secret-token', $_SERVER['HTTP_AUTHORIZATION']); + self::assertSame('application/json', $_SERVER['CONTENT_TYPE']); + + // Request 2 is a plain GET that supplies NONE of those keys. They must be + // retired — a leftover HTTP_AUTHORIZATION would let one visitor's bearer + // token be seen by the next request served by the same worker. + $worker->marshalSuperglobals(new FakeEnvelope( + server: ['REQUEST_METHOD' => 'GET', 'REQUEST_URI' => '/two'], + )); + + self::assertSame('/two', $_SERVER['REQUEST_URI']); + self::assertSame('GET', $_SERVER['REQUEST_METHOD']); + self::assertArrayNotHasKey('HTTP_AUTHORIZATION', $_SERVER); + self::assertArrayNotHasKey('CONTENT_TYPE', $_SERVER); + self::assertArrayNotHasKey('CONTENT_LENGTH', $_SERVER); + self::assertArrayNotHasKey('HTTP_X_CUSTOM', $_SERVER); + } + public function testUrlencodedPostIsParsedIntoPost(): void { $worker = new Worker('/tmp/wp/');