Backup: require a site token for capability-less JSON API endpoints - #50871
Merged
LiamSarsfield merged 14 commits intoAug 7, 2026
Conversation
Contributor
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
github-actions
Bot
added
[Feature] WPCOM API
[Plugin] Jetpack
Contributor
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
github-actions
Bot
added
the
[Status] Needs Author Reply
LiamSarsfield
removed
the
[Status] Needs Author Reply
LiamSarsfield
force-pushed
the
jetpack-2031-json-api-empty-capabilities-site-token
branch
from
July 30, 2026 08:58
977be06 to
6efec15
Compare
Endpoints that declare an empty $needed_capabilities (the Backup helper-script install/delete endpoints and the get-*-backup endpoints) rely on the empty list to mean "site token only". But check_capability() treats an empty capability set as a pass: with an empty list $must_pass is 0, so any connected user token -- including a low-privileged one -- satisfied the check when the request was not site-based (blog token) authenticated. Deny an empty capability set unless the request is site-based authenticated, which is the intended access mode for these endpoints. Legitimate blog-token requests are unaffected; they short-circuit earlier via accepts_site_based_authentication(). JETPACK-2031
Review follow-up to 278167a. The previous commit denied the literal empty capability array, but that is one producer of the underlying condition rather than the condition itself: check_capability() authorizes whenever the resolved must_pass threshold is 0, and two further declaration shapes reach that state. - Hoist the emptiness check above the is_array() split, so scalar declarations meaning "nothing required" deny as well. Numeric 0 previously resolved to level_0, a capability every role holds. - Deny when the resolved must_pass is below 1. This covers a caller-supplied threshold of 0 over a populated capability list, and a wrapper array with no capabilities key. must_pass has no caller anywhere in projects/, so nothing in tree changes behaviour. is_int() also admitted negatives. - Give the two new denials their own error codes rather than reusing the generic 'unauthorized' already emitted twice in this method, so a fail-closed regression is separable in aggregate. Nothing consumes the old code here. - Record in the guard comment that the site-based authentication short-circuit is the only way past the deny, that allow_jetpack_site_auth (not the fallback flag) enables it, and that its is_jetpack_authorized_for_site() half is overridden on WordPress.com, so the effective policy there lives outside this repository. - Correct the two helper-script docblocks, which still documented an empty capability set with its pre-fix permissive meaning, and fix the base property @var, which claimed string while the property holds string|array|null. Tests: administrator tokens are rejected identically to subscriber tokens (the deny consults no privilege); nine declaration shapes covering both new denials and a sanity row that still fails on the capability itself; and a registry invariant asserting every capability-less production endpoint is registered with allow_jetpack_site_auth, since nothing otherwise links the declaration to the registration and a missing flag is now a permanent 403. That invariant is scoped to endpoints reaching check_capability() through the base callback() -- Jetpack_JSON_API_Check_Capabilities_Endpoint overrides callback() and passes 'read' by hand, so its unset default is not a site-token declaration. Changelog now states the enforced rule: every user token is rejected, not only low-privileged ones. JETPACK-2031
Third round on JETPACK-2031, closing the last member of the fail-open class and unblocking required CI. Required Static analysis was failing: the `@var string|array|null` correction in the previous commit made a Phan baseline suppression obsolete, and the baseline check fails on the uncommitted regeneration. Drop the now-unused `PhanTypeMismatchArgument` entry. A non-empty capability list could still authorize any connected user. An entry of `0` or `'0'` clears the emptiness guard, then `WP_User::has_cap()` routes anything `is_numeric()` through its legacy user-level shim and checks `level_0`, which every default role holds. The same applies to a wrapper whose `capabilities` key is missing, where the wrapper's own metadata gets capability-checked. Validate every resolved entry is a non-numeric, non-empty string and deny otherwise, under a third distinct error code. No endpoint in this repository declares such a shape, so nothing in tree changes behaviour. Unifying the scalar and array paths onto one normalized list is what keeps this a single check rather than two. It also lets Phan see that a null declaration cannot reach `current_user_can()`, and removes a duplicated call site from the authorization method. The scalar path is behaviourally unchanged: a one-element list produces the same message and error code the scalar branch built directly. Corrections from review: - The guard comment claimed `null` and `''` were fail-open. Measured false: only the numeric forms were. Both already denied and now deny with a distinguishable code. - Under `IS_WPCOM` this file is not the one that runs -- json-endpoints.php resolves the endpoint directory to the WordPress.com tree. Recorded in the guard comment and against the registry test, whose scope it bounds. - The registry test's callback() carve-out was justified by a false claim. Jetpack_JSON_API_Plugins_Modify_Endpoint overrides callback() and still delegates to the parent; the carve-out is load-bearing for exactly one class, Jetpack_JSON_API_Check_Capabilities_Endpoint. - `@param array $capability` contradicted the property's own contract; both methods pin `true|WP_Error` now, since validate_call() authorizes on anything that is not a WP_Error. - Changelog reworded to the imperative mood the repository requires. Tests grow 24 -> 30. Nested `0`/`'0'`/`''` and the malformed wrapper are pinned, the threshold guard gains the blog-token row that proves it sits below the site-auth short-circuit, and rows that are green on trunk are annotated as such rather than reading as regression coverage. Verified: phpcs clean, Phan 0 issues, changelogger validate clean, 289/289 in tests/php/json-api on PHP 8.4.19.
…overage Review follow-up. No change to what check_capability() authorizes. The guard comment and a provider annotation claimed that `null` and `''` already denied before this fix. That is true on single-site only. On multisite `map_meta_cap()` falls through to `$caps[] = $cap`, so `current_user_can( null )` reaches `WP_User::has_cap()` with `array( null )`, which contains no `do_not_allow`, and the super-admin short-circuit returns true. The guard therefore closes a network super admin fail-open there too. Both statements are corrected and scoped. The registry test exempted every endpoint whose `callback()` is declared outside the base class, on the theory that such a class defines its own authorization path. Jetpack_JSON_API_Plugins_Modify_Endpoint falsifies that proxy: it overrides `callback()`, assigns a capability per action and then delegates to `parent::callback()`. A future endpoint in that shape declaring no capabilities would have been skipped silently. The exemption is now by name -- Jetpack_JSON_API_Check_Capabilities_Endpoint, the only registered class that both hand-passes its capability and omits the site-auth flag. Jetpack_JSON_API_Themes_Active_Endpoint is asserted rather than exempted. New coverage: * `test_capability_declaration_still_accepts_a_blog_token()`, sibling to the existing threshold test, so all three guards are pinned below the `accepts_site_based_authentication()` short-circuit. Hoisting one above it would 403 legitimate blog-token traffic with the rest of the suite green. * `test_positive_must_pass_thresholds()`, covering the supported N-of-M wrapper in both directions. The existing rows only exercised thresholds below 1, and this refactor rewrote the counting path the valid side uses. * A row for `array( 'capabilities' => '<scalar>' )`, the one shape the refactor makes less strict: it previously never reached a capability check at all (`count()` on a string warned to 1 with an inert foreach on PHP 7.x, and threw a TypeError on PHP 8+). It now normalizes to a one-element list and runs the real check. Nothing in this repository declares the shape. Also notes at the short-circuit that `accepts_site_based_authentication()` infers site authentication from a zero current user id and does not verify that a blog token signed the request; drops the trailing inline comments this branch added next to the helper-script DocBlocks that already say the same thing; moves the `IS_WPCOM` scope note to a one-line pointer; and gives the two capability loops one variable name.
LiamSarsfield
force-pushed
the
jetpack-2031-json-api-empty-capabilities-site-token
branch
from
July 30, 2026 13:22
fcc7921 to
b94099a
Compare
…ontract The declaration guard rejected only the literal empty string, so a whitespace-only entry still named no capability while passing validation. WP_User::has_cap() grants a network super admin any capability that does not map to do_not_allow, so " " authorized them. Reject entries on their trimmed value instead. Corrects an overbroad claim in the same comment block: a wrapper with no 'capabilities' key is caught by this guard only when its metadata is not capability-shaped. array( 'must_pass' => 0 ) is rejected, but array( 'must_pass' => 'read' ) is indistinguishable from the list array( 'read' ) and gets an ordinary capability check. That behaviour predates this branch; it is now stated accurately and pinned by a test. Adds the authorized direction for the two wrapper shapes that reach a real capability check, so a regression that denied every wrapper declaration would fail rather than leave the suite green, plus deny rows for the whitespace-only shapes and a reworded comment on the blog-token declaration test, which described a guard the short-circuit above it reaches first.
The previous guard rejected only entries whose ASCII-trimmed value was empty. That left three ways for a malformed declaration to reach current_user_can(), where core grants a network super admin any capability it cannot map to do_not_allow: - Padding around a real name: 'read ' is not the capability read, but it passed the guard and was checked verbatim. - Whitespace PHP's byte-wise trim() does not strip: the form feed, NBSP and the other Unicode separators, and the BOM. - '0 ', which is is_numeric() from PHP 8.0 on but not before it, so the numeric rejection behaved differently across the supported matrix. Entries are now compared against a canonical form that strips Unicode separators and control characters from both ends, and a declaration that is not already canonical is rejected rather than repaired. Comparing before the is_numeric() test removes the version split. Invalid UTF-8 makes preg_replace() return null and is rejected for the same reason. No behaviour change in tree: all thirteen capability names declared in this repository are already canonical, so the comparison is a no-op for every existing endpoint. Also in this commit: - Narrow the .phan/baseline.php diff to the single suppression this change makes obsolete, restoring the unrelated regenerated entries to their trunk state. phan still reports FOUND 0 ISSUES TOTAL. - Pin the registry invariant's population with assertGreaterThanOrEqual( 10 ) rather than "more than none", so a loop that stopped reaching most capability-less endpoints fails instead of passing on one. - Add the deny direction for the wrapper-metadata shape and rename the test to match what it now covers, so its documented result type is accurate. - Extract the repeated dummy-endpoint reflection setup into one helper. - Condense the guard commentary onto the durable invariants, moving the case-by-case history to the tests and the PR description.
Fifth round on JETPACK-2031. No change to the guard predicate: the only edits to check_capability() are comment corrections. Two required checks were red, both self-inflicted by the previous commit. Static analysis was failing because that commit hand-narrowed .phan/baseline.php to the single suppression this PR obsoletes. The CI job regenerates the baseline and diffs it, so the committed file has to be the generated one; regeneration also drops now-unused suppressions in five sibling endpoint files that the normalized capability list resolves. This restores the generated output verbatim -- byte-identical to what the earlier commit had -- rather than a hand-picked subset of it. Phan still reports 0 issues. PHPCS was failing on two array-alignment warnings in the accessibility test, introduced when a provider row was added last round. The previous round's phpcs run reported exit 0 and was read as clean; it was not checking warnings. The multisite fail-open the guards exist to close was never executed. The guard comments name the network super admin -- granted any capability core cannot map to do_not_allow -- as the reason a malformed name must be rejected, but tests/php.multisite.11.xml lists the json-api suite file by file and never included this one, so nothing in this PR ran under multisite at all. Add the file to that suite and cover all three guards with a super admin fixture. Every guard returns before current_user_can(), so this confirms rather than extends the fix, and it pins the scenario against a future identity shortcut inserted above the guards. Comment corrections, each a claim that was measurably wrong: - The '0 ' row claimed only the canonical comparison denies it on every version. From PHP 8.0 on is_numeric( '0 ' ) is true and denies it too, so the row discriminates the canonical arm on the < 8.0 legs only. The padded-name rows are what discriminate it everywhere. - The null-canonical arm was attributed to invalid UTF-8, which the mismatch arm also rejects. Its load-bearing case is a non-string entry: null !== null is false, so nothing else catches array( null ). Now stated, and pinned by a row so it cannot be deleted as redundant. - "Every entry must name an actual capability" overstated an anchored regex. It validates boundaries; an interior-mangled name still reaches current_user_can(), where it is unmappable and grants only a super admin, who holds everything already. Declarations are never request-derived, so this is a hardening gap, not a reachable one. Left as-is: no character class closes it, since blank-rendering glyphs like U+3164 sit outside \pZ and \pC. - The registry floor of 10 counts both Themes_Active registrations. That was disclosed as deliberate over-inclusion but the number was not, so a future exemption would false-fail it. Verified: phpcs clean, Phan 0 issues, changelogger clean. Accessibility suite 51 tests single-site (4 multisite rows skipped) and 51 tests / 66 assertions multisite. Group json-api 180 tests / 496 assertions single-site, 53 tests / 68 assertions multisite.
…anifest grant_super_admin() writes the fixture's login into the network's site_admins option, which lives in sitemeta. The shared teardown truncates the users and usermeta tables but never sitemeta, so the login of a deleted user stayed in the network's super admin list for the rest of the run. Revoke it in wpTearDownAfterClass(), which runs before that teardown deletes the user. Also add the accessibility test to tests/php.multisite.9.xml. Required CI only selects the PHPUnit 11/12 manifest, so this adds no CI coverage today; it keeps the two multisite manifests from disagreeing about which json-api files exist, which is how this file came to run on no multisite configuration at all. The remaining changes are comments: the guard now states that \pC also matches whatever PCRE's Unicode table still considers unassigned, so a boundary character newer than that table is denied rather than admitted, and the super admin provider no longer claims one row per guard when it has four rows for three.
Three comments described the guards' fail-opens as a single multisite-only class. They are not. An empty capability set and a `must_pass` threshold below 1 bypass the capability loop for ordinary users on any site; a numeric entry resolves to a legacy user level every default role holds, also on any site. Only an entry core cannot map to a capability is multisite-specific, because that is the case core grants to a network super admin. The provider comment added in the previous commit attributed both declaration sub-conditions to the super admin grant, which contradicted this same file's existing note on the numeric rows. Correct that, split the same conflation in the test DocBlock, and reword the multisite manifest comment so it claims coverage of the one variant that needs multisite rather than of all of them. Comment-only: no test, guard or suite-selection behaviour changes.
The note said neither the WordPress.com registrations nor "that copy of the guard" are visible to this test, which implies WordPress.com reimplements the capability check. It does not. Under IS_WPCOM the endpoint directory resolves to the WordPress.com tree, whose endpoints extend WPCOM_JSON_API_Endpoint directly, so the guarded base class is not loaded there at all rather than loaded in a second form. Only the registrations are out of view, which is what the test's stated limit actually rests on. Comment-only.
The comments carried the same facts more than once. The guard explained the network super admin grant in two adjacent blocks and stated the boundary-class tradeoff twice in one sentence. The registry test spent sixteen lines recounting what an earlier version of itself did, and three docblocks argued at paragraph length for their own existence. Consolidate each to one statement. Every load-bearing fact is kept: core's legacy user-level shim, the super admin grant, the anchoring residual, the PHP 8.0 change in is_numeric(), why the null test cannot be folded into the mismatch test, and why the registry exemption is by name rather than by structure. Comment-only.
TEMPORARY, to be reverted before review. The multisite suite -- which only runs on the WP trunk job -- fails this pre-existing test on the allow_jetpack_site_auth: true; logged_in_user: false row, and it does not reproduce against WP 7.0.2 locally. accepts_site_based_authentication() needs both a zero current user and a token_details blog_id of at least 1, so the assertion message now reports the multisite flag, the $blog_id global that set_up() reads, the current blog and user ids, and the resolved token details. That identifies which precondition is not holding without guessing at it.
Reverts the temporary diagnostic from the previous commit and fixes what it found. The multisite run failed test_accepts_site_based_authentication on its no-logged-in-user row because a user was already set: the diagnostic reported current_user=182 with a healthy blog_id of 1. This class asserts blog-token behaviour, and is_jetpack_authorized_for_site() only recognises a blog token while no user is logged in. WP_UnitTestCase clears the current user in tear_down(), but wpSetUpBeforeClass() and the previous class's wpTearDownAfterClass() both run after that, so the first test in the class inherited whatever they left set. Establish the precondition in set_up() rather than relying on another class's teardown. No production behaviour was wrong: returning false with a user present is correct. Single-site never hit it because this file only runs alongside those classes in the multisite suite.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens authorization for Jetpack’s legacy JSON API endpoints by ensuring that endpoints declaring an empty capability set cannot be accessed with any connected user token, and instead require site (blog) token authentication.
Changes:
- Fix
Jetpack_JSON_API_Endpoint::check_capability()to fail closed for “no capabilities” declarations unless the request is site-authenticated. - Add extensive unit coverage for the previously fail-open shapes (empty sets, malformed capability entries, and invalid
must_passthresholds), including multisite-only super-admin execution. - Update Backup helper-script endpoint documentation, add a changelog entry, and refresh the Phan baseline accordingly.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| projects/plugins/jetpack/json-endpoints/jetpack/class.jetpack-json-api-endpoint.php | Fixes the authorization logic by denying empty / malformed capability declarations for user tokens while preserving the site-auth short-circuit. |
| projects/plugins/jetpack/tests/php/json-api/Jetpack_Json_Api_Endpoints_Accessibility_Test.php | Adds regression and invariant tests covering the fail-open cases and ensuring capability-less endpoints are registered with site-auth enabled. |
| projects/plugins/jetpack/tests/php.multisite.9.xml | Ensures the new accessibility tests run in multisite to exercise the network super-admin-only variant. |
| projects/plugins/jetpack/tests/php.multisite.11.xml | Same as above for the alternate multisite test suite configuration. |
| projects/plugins/jetpack/json-endpoints/jetpack/class-jetpack-json-api-install-backup-helper-script-endpoint.php | Updates inline documentation to reflect the new “empty capabilities == site-token-only” enforcement. |
| projects/plugins/jetpack/json-endpoints/jetpack/class.jetpack-json-api-delete-backup-helper-script-endpoint.php | Updates inline documentation to reflect the new “empty capabilities == site-token-only” enforcement. |
| projects/plugins/jetpack/changelog/jetpack-2031-json-api-empty-capabilities-site-token | Adds a user-facing changelog entry describing the tightened auth requirement. |
| projects/plugins/jetpack/.phan/baseline.php | Regenerates/normalizes the Phan baseline after the refactor. |
Member
|
Reviewed this in depth — the diagnosis is accurate and the fix is correctly scoped. A few things I checked independently rather than taking on trust:
Two non-blocking suggestions, neither of which should hold up the merge. 1. Comment density in The method runs roughly 42 comment lines to 40 lines of code, including a 22-line block above a 9-line loop. The content is genuinely valuable — the Consider keeping the two or three load-bearing sentences per guard inline and moving the full rationale into the test file, where the data-provider rows already carry it case by case. 2. Changelog entry is written from the implementation's perspective.
conveys about as much to someone reading the release notes. One separate thing, flagged for whoever owns the call rather than as review feedback: the description publicly describes the fail-open and names the affected endpoints. You've clearly reasoned about this already — a validly signed request is still required, so it's defense in depth — and Jetpack develops in the open, so this may well be fine. Just worth a second opinion from someone with the full threat picture before it lands. Test suite is unusually strong, incidentally. The declaration↔registration invariant with a |
enejb
added
[Status] Ready to Merge
LiamSarsfield
deleted the
jetpack-2031-json-api-empty-capabilities-site-token
branch
August 7, 2026 10:21
github-actions
Bot
removed
the
[Status] Ready to Merge
Contributor
|
Cherry-picked to |
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.
Proposed changes
Eight legacy Backup endpoints declare an empty
$needed_capabilitiesarray, meaning "site (blog) token only".check_capability()read it the other way round: an empty list makes$must_passzero, and zero required checks always pass, so any connected user token was authorized, including on the endpoint that installs a Backup helper script. Reaching it still needs a validly signed request, so this is defense in depth rather than a live hole.check_capability()now returns a 403 for an empty capability set unless the request is site authenticated; blog-token requests short-circuit above the guard and are unaffected. Later commits close two sibling shapes that authorize everyone the same way: amust_passthreshold below 1, and an entry that is not a capability name. No endpoint here declares either, so nothing else changes.The guards sit below
accepts_site_based_authentication(), which infers site authentication from a zero current user id rather than the token type. And underIS_WPCOMthis file never loads, so only self-hosted and Atomic sites change. The WordPress.com side was checked separately and needs no equivalent change.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
Live instructions
Only these reach real request signing. The unit tests simulate the post-authentication state instead.
unauthorized_site_token_required. Nothing should appear, and that is the code to watch after deploy.A backup exercises normal Backup operation, which is what a regression here would break. It does not reach the capability-less endpoints: a full backup and scan on a connected test site was instrumented and made no call to any of them.
Both directions were then confirmed on that site with signed requests against one of the capability-less endpoints. A site token is accepted and reaches the endpoint; a user token is refused with
unauthorized_site_token_required. On stock code the same user-token request succeeded, which is the fail-open this closes.Unit testing
jp docker phpunit jetpack -- --filter=Jetpack_Json_Api_Endpoints_Accessibility_Test(51 tests, 4 multisite-only rows skipped). Blog token accepted; subscriber and administrator both denied; every zero-resolving shape denied under its own code; wrapper shapes pinned in both directions.jp docker phpunit jp-multisite -- --filter=Jetpack_Json_Api_Endpoints_Accessibility_Test(51 tests, 66 assertions, nothing skipped). One fail-open only reaches a network super admin, so it now runs rather than being argued for.jp docker phpunit jetpack -- --group json-api(180 single-site, 53 multisite). No regressions to the site-auth short-circuit or the ordinary capability path.jp phan plugins/jetpackreports 0 issues. The baseline diff is regenerated output: normalizing the list makes several suppressions obsolete, and removing one can only expose an issue.allow_jetpack_site_auth => true, since a missing flag is now a permanent 403. Green on trunk too, so it is a forward-looking invariant.Compatibility note
The producer for these endpoints was traced separately. It always sends requests with no user context, so the new deny does not reach it.
If a caller ever does turn up with one attached, do not fix it by swapping the site-auth check for
Rest_Authentication::is_signed_with_blog_token(). That status is only populated insidewp_rest_authenticate(), which the legacy XML-RPC bridge never runs, so the swap would fail closed on self-hosted Jetpack and cause the outage it was meant to prevent.