[Website] Validate plugin proxy redirect destinations#4137
Open
brandonpayton wants to merge 4 commits into
Open
[Website] Validate plugin proxy redirect destinations#4137brandonpayton wants to merge 4 commits into
brandonpayton wants to merge 4 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the website’s plugin proxy “url” mode to validate every redirect destination against an allowlist, preventing off-list redirects from being contacted while still supporting common redirect-based downloads.
Changes:
- Added manual redirect-following loop with per-hop allowlist validation and a dedicated
UrlNotAllowedException. - Refactored streaming to buffer headers until it’s known whether a response is an intermediate redirect.
- Added Playwright E2E coverage that spins up a real PHP endpoint plus an in-process HTTP server to assert redirect behavior and “not contacted” guarantees.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/playground/website/public/plugin-proxy.php | Implements per-redirect allowlist validation and header buffering to avoid leaking intermediate redirect responses while streaming. |
| packages/playground/website/playwright/e2e/plugin-proxy.spec.ts | Adds an integration-style Playwright test harness that validates redirect chains and ensures blocked destinations are never requested. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
Author
|
This reads well and should be good to go. I plan to give it a final glance and a small test in the morning before merging. |
This was referenced Jul 23, 2026
brandonpayton
pushed a commit
that referenced
this pull request
Jul 23, 2026
## What changed Remove the standalone `$pluginResponse;` expression from `plugin-proxy.php`. ## Why The variable is never assigned or read, and it has no declaration or control-flow effect in PHP. The statement was introduced in 2023 with the Gutenberg pull-request preview flow and has remained unused. Evaluating the undefined variable can emit a warning even though `display_errors` hides it from the HTTP response. ## Impact There is no intended behavior change. The cleanup removes a no-op expression and avoids a possible undefined-variable warning in server logs. ## Stack This PR is stacked on #4157 and targets its head branch. It should be merged after #4157, which is itself stacked on #4137. ## Validation - Focused plugin-proxy Playwright suite: 7 passed - Website lint: passed - PHP syntax check: passed
brandonpayton
added a commit
that referenced
this pull request
Jul 24, 2026
## What changed This stacked PR adds regression coverage for filtered upstream response headers and preserves configured default response headers when a same-named upstream header is filtered out. ## Why PR #4137 buffers response headers while following redirects. The new helper marked every received header as seen before applying the response-header allowlist. A filtered upstream `Content-Type` therefore suppressed the configured `Content-Type: application/zip` fallback even though neither header reached the client. The fix records a response header as seen only after it passes the allowlist and is forwarded. ## Impact Plugin, theme, release, and artifact downloads retain their intended ZIP content type and download metadata when upstream headers are filtered. ## Commit and CI sequence 1. Commit `7161ff666` added the regression test only. [Chromium shard 2 failed](https://github.com/WordPress/wordpress-playground/actions/runs/30019151059/job/89246998902) with `Expected: "application/zip"` and `Received: undefined`. 2. Commit `75c40e2b5` applied the one-line production fix. [The same Chromium shard passed](https://github.com/WordPress/wordpress-playground/actions/runs/30020038612/job/89250060487), including `keeps a default header when the matching upstream header is filtered`. This PR is stacked on #4137 and targets its head branch. ## Validation - Focused Playwright suite after the fix: 7 passed - Website lint: passed - PHP syntax checks for `plugin-proxy.php` and the test router: passed - GitHub Actions regression test: failed before the fix and passed after the fix --------- Co-authored-by: Brandon Payton <brandon@happycode.net>
brandonpayton
force-pushed
the
emdash/plugin-proxy-redirect-hardening-prp48
branch
from
July 24, 2026 04:05
6c63ae8 to
c61cbbe
Compare
## What changed This stacked PR adds regression coverage for filtered upstream response headers and preserves configured default response headers when a same-named upstream header is filtered out. ## Why PR #4137 buffers response headers while following redirects. The new helper marked every received header as seen before applying the response-header allowlist. A filtered upstream `Content-Type` therefore suppressed the configured `Content-Type: application/zip` fallback even though neither header reached the client. The fix records a response header as seen only after it passes the allowlist and is forwarded. ## Impact Plugin, theme, release, and artifact downloads retain their intended ZIP content type and download metadata when upstream headers are filtered. ## Commit and CI sequence 1. Commit `7161ff666` added the regression test only. [Chromium shard 2 failed](https://github.com/WordPress/wordpress-playground/actions/runs/30019151059/job/89246998902) with `Expected: "application/zip"` and `Received: undefined`. 2. Commit `75c40e2b5` applied the one-line production fix. [The same Chromium shard passed](https://github.com/WordPress/wordpress-playground/actions/runs/30020038612/job/89250060487), including `keeps a default header when the matching upstream header is filtered`. This PR is stacked on #4137 and targets its head branch. ## Validation - Focused Playwright suite after the fix: 7 passed - Website lint: passed - PHP syntax checks for `plugin-proxy.php` and the test router: passed - GitHub Actions regression test: failed before the fix and passed after the fix --------- Co-authored-by: Brandon Payton <brandon@happycode.net>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation for the change, related issues
The proxy's
urlmode accepts a URL and limits requests to a short list of host names. A redirect points to another URL, so checking only the first URL does not apply that rule to the rest of the redirect chain.Redirects still need to work because downloads commonly use them. For
urlmode, the proxy should check each destination before contacting it and stop if any destination is outside the allowed-host list.The proxy also supplies default response headers for several download routes. If an upstream header is filtered out, it has not been sent and should not prevent a configured default header from being used. In addition, parsed header names are lowercase, so response-header allowlists must use lowercase names too.
Implementation details
streamHttpResponse()now handles all redirects in one bounded PHP loop. cURL makes one request at a time and reports the next redirect URL. When a caller supplies an allowed-host list, PHP checks the hostname before making each request.Internal download routes use the same redirect loop without the four-host list because their starting URLs are selected by application code and use different download hosts.
The loop follows cURL's redirect behavior:
PHP briefly buffers each response's headers so it can tell whether the response is an intermediate redirect. Intermediate redirect bodies are discarded. The final response body continues streaming to the caller and is never buffered in full.
Response headers are now recorded as present only after they pass the response-header allowlist and are forwarded. This means a filtered upstream header no longer suppresses a configured default header. The
Cache-Controlentries in the response-header allowlists are also stored as lowercasecache-control, matching the parsed header names.Testing Instructions (or ideally a Blueprint)
Run the focused endpoint tests:
npm exec nx -- run playground-website:e2e:playwright -- --project=chromium plugin-proxy.spec.tsRun PHP syntax and website lint checks:
php -l packages/playground/website/public/plugin-proxy.php php -l packages/playground/website/playwright/e2e/plugin-proxy-test-router.php npm exec nx -- lint playground-website --skipNxCacheThe Playwright suite starts the real PHP endpoint and an in-process Node HTTP proxy. The Node server creates controlled redirect chains and records every requested destination, so the tests can prove that rejected destinations are not contacted. A small PHP test router verifies the headers emitted by PHP's
header()function. The tests do not make external requests.The seven focused tests cover: