Skip to content

REST API: Add the rest_allowed_cors_methods filter and stop discarding a response's method list. - #13151

Open
moonmeister wants to merge 43 commits into
WordPress:trunkfrom
moonmeister:fix/rest-cors-allow-methods-clobber
Open

moonmeister wants to merge 43 commits into
WordPress:trunkfrom
moonmeister:fix/rest-cors-allow-methods-clobber

Conversation

@moonmeister

@moonmeister moonmeister commented Aug 18, 2026 •

Copy link
Copy Markdown

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 on rest_pre_serve_request, after the response's own headers have been sent, and header() replaces by default - so the hardcoded list discarded anything set via rest_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.

$response_headers = ( $result instanceof WP_HTTP_Response ) ? $result->get_headers() : array();
$response_headers = array_change_key_case( $response_headers );

$allow_methods = array( 'OPTIONS', 'GET', 'POST', 'PUT', 'PATCH', 'DELETE' );
if ( isset( $response_headers['access-control-allow-methods'] ) ) {
	$allow_methods = array_values( array_intersect( $allow_methods, array_map( 'strtoupper', wp_parse_list( $response_headers['access-control-allow-methods'] ) ) ) );
}

get_headers() rather than headers_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 filters rest_exposed_cors_headers and rest_allowed_cors_headers - array in, imploded out, $request in context. rest_pre_serve_request already passes $request, hence add_filter( ..., 10, 3 ).

$allow_methods = apply_filters( 'rest_allowed_cors_methods', $allow_methods, $request instanceof WP_REST_Request ? $request : null );

if ( ! empty( $allow_methods ) ) {
	header( 'Access-Control-Allow-Methods: ' . implode( ', ', $allow_methods ) );
} else {
	header_remove( 'Access-Control-Allow-Methods' );
}

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.

$result and $request are untyped, so an unexpected value passed through rest_pre_serve_request is 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.

  • PHP does not record header() under the CLI SAPI, so the tests read the final list from a rest_allowed_cors_methods callback at PHP_INT_MAX instead of from the wire. No Xdebug needed, and nothing skips silently.
  • That also means the header_remove() branch for an empty list has no test. Xdebug doesn't track header_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:

REST API: Introduce the `rest_allowed_cors_methods` filter.

Add a filter to `rest_send_cors_headers()` so the `Access-Control-Allow-Methods` header sent with CORS responses can be adjusted. The list of methods is a fixed default and is not derived from the registered routes, so a site serving routes that accept other methods can now advertise them. The filter receives the method list as an array and the request in context, matching the shape of the sibling `rest_exposed_cors_headers` and `rest_allowed_cors_headers` filters, so a plugin can append to or narrow the list rather than restate it. An empty list omits the header.

The filter is seeded from an `Access-Control-Allow-Methods` header already set on the response, limited to the methods in the default list, falling back to the default list itself. A value set through `rest_post_dispatch` was previously discarded: `rest_send_cors_headers()` runs on `rest_pre_serve_request`, after the response's own headers have been emitted, and `header()` replaces by default.

The default output is unchanged for sites that neither set the header nor use the filter. Under `?_envelope=1` a response header is relocated into the response body before this runs, so it is not seen here and the filtered default is sent. This also adds the first test coverage for `rest_send_cors_headers()`.

Follow-up to [56096].

Props moonmeister, sudar, TimothyBlynJacobs, aaemnnosttv, westonruter.
Fixes #46992.

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.

moonmeister and others added 3 commits August 17, 2026 16:02
`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>
@moonmeister
moonmeister marked this pull request as ready for review August 18, 2026 23:03
@github-actions

github-actions Bot commented Aug 18, 2026 •

Copy link
Copy Markdown

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props moonmeister, adamsilverstein, westonruter, sudar.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions

Copy link
Copy Markdown

Test using WordPress Playground

The 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

  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@adamsilverstein

adamsilverstein commented Aug 31, 2026 •

Copy link
Copy Markdown
Member

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:

The filter can add methods but it cannot take any away. When the response has set Access-Control-Allow-Methods, rest_allowed_cors_methods never runs at all, which test_response_value_takes_precedence_over_the_filter pins down with assertFalse( $filter_ran ).

That rules out the hardening case. A site narrowing the list to OPTIONS, GET is silently overridden by any plugin that sets the header on its own response, with no way to observe it. It also diverges from the two sibling filters this one is modeled on: rest_exposed_cors_headers and rest_allowed_cors_headers always apply.

Comment 11 on the ticket did not settle precedence either way, so this looks open. Seeding the filter with the response value rather than skipping it keeps option C intact and gives the site the last word:

$allow_methods = isset( $response_headers['access-control-allow-methods'] )
	? array_map( 'trim', explode( ',', $response_headers['access-control-allow-methods'] ) )
	: array( 'OPTIONS', 'GET', 'POST', 'PUT', 'PATCH', 'DELETE' );

$allow_methods = apply_filters( 'rest_allowed_cors_methods', $allow_methods, $request );
header( 'Access-Control-Allow-Methods: ' . implode( ', ', $allow_methods ) );

The response still wins over the default, and the one test above inverts.

Smaller things:

  • @param WP_HTTP_Response $result should be WP_HTTP_Response|null since it defaults to null, matching the WP_REST_Request|null on the next line. Column alignment does not change, both are 21 characters.
  • Under ?_envelope=1 the guard sees an empty header list. envelope_response() runs after rest_post_dispatch and builds a fresh response with the old headers relocated into the body, so a plugin's value lands in the JSON and core's default goes on the wire. That is arguably what _envelope is for, so a docblock line seems like enough here rather than a code change.

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>
@moonmeister

moonmeister commented Sep 15, 2026 •

Copy link
Copy Markdown
Author

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 $methods untouched, or register nothing at all), and compose ($methods[] = 'X'). With "skip-when-set", the "filter overrides a response value" is unreachable at any priority, and the callback can't even observe that it was bypassed. Default output is identical either way, so it's a free gain in expressiveness. Secondary support: nothing in WP privileges a value already set on an object over a filter that runs later, and rest_allowed_cors_methods on rest_pre_serve_request is later than rest_post_dispatch — so the skip was the deviation, not the fix.

Small bonus from parsing the header back out: WP_HTTP_Response::header( $key, $value, false ) comma-joins on append, so explode + trim normalizes the multi-append case for free.

On _envelope: agreed, docblock only. It's narrower than it first looks, too — Access-Control-Allow-Methods is only ever read by the browser off a preflight response, so the enveloped case only bites on an OPTIONS preflight to a URL already carrying ?_envelope=1. And it's a pre-existing property of the envelope that relocates all headers, not something this change introduces. Documented it and added a test so the behavior is at least pinned as intentional.

$result is now WP_HTTP_Response|null. Both small ones done.

moonmeister and others added 3 commits September 15, 2026 15:52
…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.
@sudar

sudar commented Oct 1, 2026

Copy link
Copy Markdown

Really happy to see progress on a ticket that I opened in trac 7 years ago.

@adamsilverstein adamsilverstein left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work.

* 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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@adamsilverstein adamsilverstein changed the title REST API: Stop rest_send_cors_headers() clobbering Access-Control-Allow-Methods, and make the list filterable. REST API: Add the rest_allowed_cors_methods filter and stop discarding a response's method list. Oct 19, 2026
Comment thread src/wp-includes/rest-api.php
moonmeister and others added 14 commits October 19, 2026 16:43
…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 westonruter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude found some more stuff at high effort. One moment.

westonruter and others added 9 commits October 19, 2026 20:37
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants