Repository navigation
REST API: Add the rest_allowed_cors_methods filter and stop discarding a response's method list. - #13151
REST API: Add the rest_allowed_cors_methods filter and stop discarding a response's method list.#13151moonmeister wants to merge 43 commits into
Conversation
`Access-Control-Allow-Methods` is the only CORS list header REST API responses send that has no extension point. Its two siblings gained one in 5.5.0 -- `rest_exposed_cors_headers` and `rest_allowed_cors_headers`, both array-filtered then imploded in `WP_REST_Server::send_headers()`, and both given the request in context in 6.3.0. This one was missed each time, because it is sent from `rest_send_cors_headers()` rather than from the server. Adds a filter with the same shape, so a site can compose the list rather than unhook `rest_send_cors_headers` and reimplement it. `$request` is available because `rest_pre_serve_request` already passes it; the filter accepts three arguments to reach it. The filter runs only when the response has not set the header itself, which is the preceding change on this branch. A value on the response is the more specific instruction, and there is no default left to shape once it is present. Adds three tests to the existing CORS header coverage. See #46992.
Keeps the two facts the code does not show -- that this runs after the response's own headers have been sent, and that `header()` replaces by default -- and drops the sentence restating the conditional below it. See #46992. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
Thanks for the continued effort here. Overall this looks good to me @moonmeister! Claude went over the PR against the ticket discussion, here is most of what came back:
What do you think - is there a reason to keep the response winning outright that I'm not seeing? |
…d list. The filter previously ran only when the response had not set `Access-Control-Allow-Methods` itself, on the reasoning that a value on the response is the more specific instruction. That reverses here, per review: the filter now always runs, seeded with the response's value when there is one and with the default list otherwise. Seeding is a strict superset of the previous behavior. It can express all three outcomes -- filter wins by returning its own list, response wins by returning `$allow_methods` untouched or by registering nothing, and the two compose by appending. Skipping the filter made "filter overrides a response value" unreachable at any priority, and gave the callback no way to observe that it had been bypassed. Default output is unchanged either way. Parsing the header back into an array also normalizes the append case: `WP_HTTP_Response::header()` comma-joins when called with `$replace` false, so `explode()` and `trim()` recover the individual methods. Also types `$result` as `WP_HTTP_Response|null`, matching its default, and notes that `envelope_response()` relocates the response's headers into the body before this runs, so under `?_envelope=1` a response value is not seen here. Inverts the test that pinned the old precedence and adds two: one asserting the filter receives the response's value rather than the default, one covering the envelope case. See #46992. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks for the review @adamsilverstein! No, there's no reason I can find to keep it — taking your suggestion as written. The filter now always runs, seeded with the response's value when available. The argument I'd hang it on is that seeding is a strict superset of the current behavior. With it, you can express all three outcomes: filter wins (return your own list), response wins (return Small bonus from parsing the header back out: On
|
…meister/wordpress-develop into fix/rest-cors-allow-methods-clobber * 'fix/rest-cors-allow-methods-clobber' of github.com:moonmeister/wordpress-develop: Docs: Add missing descriptions and declare types for `Translation_Entry` properties. Docs: Remove a blank line between `@param` and `@return` in core docblocks. Build/Test Tools: Stop committing changes to tracked files in pull requests. Code Quality: Resolve return.empty PHPStan errors in baseline. Build/Test Tools: Make `SLACK_GHA_TIMEOUT_WEBHOOK` optional. Filesystem API: Reject UNC and device paths in `validate_file()`. Tests: Use `assertIsCallable()` for callable assertions. Build/Test Tools: Distinguish between cancelled and timed out workflow runs. Build/Test Tools: Remove all `npx` commands. Build/Test Tools: Allow text compression in the local environment.
|
Really happy to see progress on a ticket that I opened in trac 7 years ago. |
| * serve requests through a real WP_REST_Server and read the emitted headers with | ||
| * xdebug_get_headers(), following tests/phpunit/tests/oembed/headers.php. | ||
| * | ||
| * @runTestsInSeparateProcesses |
There was a problem hiding this comment.
Tests that run in a separate process are very slow. We should avoid them if possible.
…pty. The response's own headers are sent before this runs, so skipping the write left a response-supplied list on the wire. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011PkCnymaZCDVy1WUqzxYbN
…default list. Intersecting a response's `Access-Control-Allow-Methods` value with the default list meant a response could only remove methods, never add one. A route handling `PROPFIND`, or `QUERY`, had that method silently dropped, though advertising such methods is part of what this ticket is for. Keep only entries that are valid method tokens per RFC 9110, section 9.1, so junk is still rejected without discarding real methods. Methods are case-sensitive, so they now pass through as given rather than being uppercased, which was only needed for the comparison against the default list. See #46992. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…der() calls. Refactors rest_send_cors_headers() to use the server object's send_header() and remove_header() methods, improving testability and allowing Vary headers to be appended correctly. Adds a $replace parameter to WP_REST_Server::send_header() to support both replace and append modes. Benefits: - Tests can now check Spy_REST_Server::$sent_headers directly, eliminating the need for xdebug_get_headers(), separate processes, or warning suppression. - The empty-list removal path and all CORS headers (including Allow-Origin and Allow-Credentials) now have proper test coverage. - Vary: Origin is now sent via the server with $replace=false for proper appending. The function takes the server as its 4th argument, which rest_pre_serve_request already provides. Backward compatible with existing remove_filter() calls. See #46992. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…meister/wordpress-develop into fix/rest-cors-allow-methods-clobber * 'fix/rest-cors-allow-methods-clobber' of github.com:moonmeister/wordpress-develop: (92 commits) Upgrade/Install: Explain when automatic updates cannot run. Script Loader: Retire script and style concatenation. Tests: Verify timezone persistence and restore original settings. Administration: Replace Dashicons in admin bar and sidebar with wp-icons. Upgrade/Install: Report fatal errors in auto-update emails. Editor: Register the core entity fields on the server. Tests: Make media data providers static. Editor: Fix link color in the "Large title" query pattern. Tests: Use the error assertion in term tests. Tests: Restore entity action module coverage annotations. Tests: Use the error assertion in spam authentication tests. Tests: Cover registered entity action modules. Tests: Use the error assertion in image resize tests. Tests: Cover feed link output. Tests: Cover tag edit links. Tests: Strengthen mobile detection fixtures. Tests: Cover JSONP callback edge cases. Tests: Cover media state labels. Tests: Cover tag retrieval. Editor: Add the attachment entity view config for the media editor. ...
Adding a `$replace` parameter to the public send_header() method makes any subclass that overrides it with the original two parameters fail with a fatal error on PHP 8, since the override is no longer compatible. Such overrides exist in plugin test suites that ship their own spy server. Restore the original signature, and send the `Vary: Origin` header with header() directly again, since it must be appended rather than replace an existing Vary header. The call is guarded with headers_sent(), as the branch for requests without an Origin already is. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The function is a 'rest_pre_serve_request' callback, and plugins call it directly or re-run that filter with their own arguments. A nullable class type on `$result`, `$request` or `$server` turns any unexpected value, such as a WP_Error passed as the result, into a TypeError instead of being ignored by the instanceof checks in the function body. Make the test for unexpected arguments pass values of the wrong type, rather than null, so it covers this. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s(). When no server is passed, rest_send_cors_headers() fell back to rest_get_server(), which builds the server, registers every route, and fires 'rest_api_init'. Plugins call the function directly with only the response value, including on requests that are not REST API requests, where this set up the whole REST server as a side effect. Without a server, send the headers with header() directly, as before. This also resolves the server once instead of in both branches. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The value returned by the 'rest_allowed_cors_methods' filter went straight into implode(), so a callback returning the list as a string, such as 'GET, POST', caused a TypeError on PHP 8 and a 500 error for every cross-origin REST response. Filtered entries were also not checked as method tokens, unlike the methods taken from the response. Run both values through the same parser: accept a string or an array, keep only valid method tokens, and treat any other value as an empty list. This also replaces the array_filter() call on the response value, which dropped the valid method token '0'. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nse value. When a response set an Access-Control-Allow-Methods header naming no valid method tokens, such as an empty string or 'GET;POST', the parsed list was empty and the header was removed. Preflighted cross-origin PUT, PATCH and DELETE requests then failed, where the default list had always been sent before. Use the default list in that case. Only an empty list from the 'rest_allowed_cors_methods' filter now omits the header. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… since tag. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Without the D modifier, `$` in the method token pattern also matches before a final newline, so an entry such as "PUT\n" passed validation. Sent without a server, header() then rejects the whole Access-Control-Allow-Methods value. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
westonruter
left a comment
There was a problem hiding this comment.
Claude found some more stuff at high effort. One moment.
wp_parse_list() keeps any scalar in an array, and preg_grep() matches the stringified value but returns the original, so a filter returning array( 'GET', true ) produced "GET, 1" and the parsed list held values that were not strings. Keep only the string entries. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A filter returning array( 'GET, POST' ) or array( ' GET' ) had every entry fail the method token check, so the header was omitted, while the same text returned as a string worked. Join the array entries before parsing, so they are split on commas and whitespace the same way. Line breaks are now separators too, so they cannot reach the header. The test for invalid tokens is split accordingly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…server. The 'rest_allowed_cors_methods' filter documents that an empty list omits the header, but without a server an Access-Control-Allow-Methods header that was already set stayed in place. Remove it with header_remove() in that case. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The class docblock still described an earlier approach, reading the method list from a late filter callback. The tests now read the headers that rest_send_cors_headers() sends through Spy_REST_Server. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DEFAULT_ALLOW_METHODS was defined but never used, while five assertions repeated the default list as a literal. Make the constant the header value and assert against it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
set_up() sets `$_SERVER['REQUEST_METHOD']` to 'GET', but tear_down() did not restore it, so the value leaked into later tests and made their results depend on test order. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A 'rest_allowed_cors_methods' callback that forgets to return the list, or returns false or null, made the parsed list empty and so removed the Access-Control-Allow-Methods header, breaking preflighted cross-origin PUT, PATCH and DELETE requests across the site. Keep the unfiltered list when the filtered value is neither an array nor a string. An empty array or string still omits the header. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A filter appending a method already in the list, such as GET, repeated it in the Access-Control-Allow-Methods header. Remove duplicates when parsing the list. Methods are case-sensitive, so entries differing only in case are kept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
tear_down() unset `$_SERVER['HTTP_ORIGIN']` rather than restoring any value it had before the test, unlike the request method. Save and restore both the same way. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Trac ticket: https://core.trac.wordpress.org/ticket/46992 - background, prior art and the caveat are there.
Approach discussed in person with @adamsilverstein, who closed the ticket in 2019 and has since agreed on option C.
Description refreshed by Claude to match the current head, 674e883.
Changes
1. Don't clobber a value set on the response.
rest_send_cors_headers()runs onrest_pre_serve_request, after the response's own headers have been sent, andheader()replaces by default - so the hardcoded list discarded anything set viarest_post_dispatch. A response value now seeds the method list instead, narrowed to the methods in the default list. Header names and method names are matched without regard to case.get_headers()rather thanheaders_list(): it reads what the response intends rather than what PHP already emitted, and works under CLI.2. Add
rest_allowed_cors_methods. Same shape as the sibling CORS list filtersrest_exposed_cors_headersandrest_allowed_cors_headers- array in, imploded out,$requestin context.rest_pre_serve_requestalready passes$request, henceadd_filter( ..., 10, 3 ).The filter always runs, so a callback can add methods outside the default list, narrow the list, or replace a value the response set. An empty list omits the header, and removes one the response already sent.
Both are needed: (1) alone forces a plugin to restate the whole list, so two plugins conflict instead of composing; (2) alone still gets discarded when a response sets its own value.
$resultand$requestare untyped, so an unexpected value passed throughrest_pre_serve_requestis ignored rather than fatal. The default output is unchanged for every site.Caveat: under
?_envelope=1,envelope_response()moves the response's headers into the body before this runs, so a response value is not seen and the filtered default is sent.Tests
rest_send_cors_headers()had no coverage at all. New file, 11 tests: default list, no Origin, response value passed through and narrowed, case-insensitive header and method matching, unexpected arguments ignored, filter add and override, the request in context when serving, and the envelope caveat.header()under the CLI SAPI, so the tests read the final list from arest_allowed_cors_methodscallback atPHP_INT_MAXinstead of from the wire. No Xdebug needed, and nothing skips silently.header_remove()branch for an empty list has no test. Xdebug doesn't trackheader_remove()either, so there is no way to observe it under CLI.--group restapi: 3746 tests, no failures.Proposed commit message
Getting this one ready to land, here is the message I plan to commit it with.
Claude drafted this from the PR and the ticket history:
Shout if the props list is missing anyone, or if the framing is off.
AI Use
My follow-up commits on this branch and the refreshed description came out of a session with 🤖 Claude Code. I will review and test.