Drop credentialed CORS reflection on ActivityPub REST endpoints#3237
Merged
Drop credentialed CORS reflection on ActivityPub REST endpoints#3237
Conversation
ActivityPub data is meant to be publicly readable, and the Access-Control-Allow-Credentials: true paired with a reflected Origin was the wrong shape for that. With credentials in the mix, echoing the request Origin lets a third-party page in principle read authenticated cross-origin responses; in practice WP core's REST nonce check rejects cookie auth without a nonce and SameSite defaults stop the cookies from being sent at all, so the bug is not directly exploitable today, but the headers should not invite the attempt. The C2S browser flow uses OAuth bearer tokens via the Authorization header, which travels under Allow-Headers and does not need Allow-Credentials. So switching to plain "*" preserves every intended use case and removes a defense-in-depth liability.
There was a problem hiding this comment.
Pull request overview
This PR hardens CORS behavior for ActivityPub REST endpoints by removing credentialed, origin-reflecting CORS headers and standardizing on wildcard CORS suitable for publicly readable ActivityPub resources.
Changes:
- Always emit
Access-Control-Allow-Origin: *for ActivityPub REST responses and stop settingAccess-Control-Allow-Credentials/Vary: Origin. - Remove the now-unused
get_cors_origin()helper and simplifysend_cors_headers(). - Add PHPUnit coverage for the new CORS header shape and for route bypass cases (non-ActivityPub routes and OAuth authorize).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
includes/rest/class-server.php |
Switches ActivityPub REST CORS headers to wildcard-only and removes credentialed origin reflection logic. |
tests/phpunit/tests/includes/rest/class-test-server.php |
Adds tests asserting wildcard-without-credentials behavior and validating bypass behavior for specific routes. |
.github/changelog/fix-cors-drop-allow-credentials |
Adds a security changelog entry describing the CORS hardening. |
str_contains would also match a hypothetical /oauth/authorize/foo subpath. Within our namespace only /oauth/authorize itself is registered, so the behavior doesn't change today, but str_starts_with reads more clearly and won't quietly disable CORS if someone later registers a sibling route whose path happens to contain that substring.
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.
Summary
Why
ActivityPub data is meant to be publicly readable, so the credentialed cross-origin shape was the wrong one. The reflect + `Allow-Credentials: true` combination would in principle let a third-party page read authenticated cross-origin responses; in practice WordPress core's REST nonce check rejects cookie auth without a nonce, and modern browsers' SameSite=Lax default stops the cookies from being sent on cross-site `fetch()` at all — so this isn't directly exploitable today. But the headers shouldn't invite the attempt, and a misconfigured cookie or a third-party plugin removing the nonce check would turn it into a real bug.
The C2S browser flow uses OAuth Bearer tokens via the `Authorization` header, which is permitted via `Allow-Headers` and does not require `Allow-Credentials` — so this change preserves every intended use case.
Test plan