Skip to content

Premium Analytics: extend the API proxy to cover Stats-Admin endpoints#49571

Open
kangzj wants to merge 5 commits into
trunkfrom
add/premium-analytics-stats-proxy-endpoints
Open

Premium Analytics: extend the API proxy to cover Stats-Admin endpoints#49571
kangzj wants to merge 5 commits into
trunkfrom
add/premium-analytics-stats-proxy-endpoints

Conversation

@kangzj

@kangzj kangzj commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Proposed changes

Replaces the Premium Analytics REST proxy's per-endpoint registry with a single endpoint-agnostic route under jetpack-premium-analytics/v1 that serves the whole surface — analytics and the re-exposed stats-admin WPCOM endpoints — without the blog id in the URL, ahead of deprecating the stats-admin package.

How it works

  • One route forwards <prefix>/<sub-path>/sites/<id>/<prefix>/<sub-path> (and upgrades/upgrades?site=<id> for purchases).
  • The frontend passes ?version= (e.g. 1.1, 1.2, 2); the proxy derives the base (2wpcom, 1.xrest). Analytics defaults to version=2. Cache keys include the version so ?version=1.1/?version=2 don't collide.
  • Requests are blog-token-signed, pinned to this site's own ID server-side, and capability-gated (analytics → manage_options; WordAds → activate_wordads; the rest → manage_options/view_stats).

Security boundary

  • Prefix allowlist in the route regex — only analytics, stats, wordads, subscribers, jetpack-stats, jetpack-stats-dashboard, commercial-classification, upgrades match (anchored so prefix-extension strings 404). The blog token can't be driven against the whole WPCOM site API.
  • Write-method allowlist — only POST mutates, and only jetpack-stats-dashboard/, commercial-classification, stats/referrers/spam/; everything else 405s. Prefix checks are case-insensitive (WP routes case-insensitively).
  • Traversal guard, site/version stripped from forwarded params, force_refresh cache bypass, write-then-read cache invalidation for the dashboard endpoints.

⚠️ Breaking (consumer-coordinated): analytics moves from /jetpack-premium-analytics/v1/proxy/<subpath> to /jetpack-premium-analytics/v1/analytics/<subpath> (version defaults to 2). The dedicated proxy/* route is removed. The dashboard data layer must migrate — tracked in WOOA7S-1539.

Out of scope (deferred — sub-issues of WOOA7S-1534): local / non-WPCOM-proxy endpoints (posts, posts/<id>, posts/<id>/likes, notices, jetpack-stats/user-feedback) and unregistering the stats-admin REST_Controller.

Related product discussion/links

Does this pull request change what data or activity we track or use?

No. This re-routes existing WPCOM analytics/stats requests through one REST namespace; the data fetched and the blog-token auth are unchanged. Permissions match the originating controllers.

Testing instructions

  • cd projects/packages/premium-analytics && composer test-php — 58 tests pass (route registration, prefix/version/endpoint validation, permission dispatch incl. analytics→manage_options, write-method policy, path building, caching).
  • composer phpcs:lint -- projects/packages/premium-analytics/src/REST/class-api-proxy-controller.php — clean.
  • On a Jetpack-connected site, confirm (admin / view_stats / activate_wordads as appropriate):
    • GET /wp-json/jetpack-premium-analytics/v1/analytics/<subpath> (defaults to v2)
    • GET /wp-json/jetpack-premium-analytics/v1/stats/top-posts?version=1.1
    • GET /wp-json/jetpack-premium-analytics/v1/subscribers/counts?version=2
    • GET /wp-json/jetpack-premium-analytics/v1/wordads/earnings?version=1.1 (requires activate_wordads; mixed-case WordAds/... is still gated)
    • GET /wp-json/jetpack-premium-analytics/v1/upgrades?version=1.2
    • Non-allowed path (e.g. /posts) and the old /proxy/... both 404; POST/PUT to a read-only endpoint 405s.

Re-expose the stats-admin WPCOM pass-through endpoints under the
jetpack-premium-analytics/v1 namespace, without the blog id in the URL,
ahead of deprecating the stats-admin package.

Extract a shared forward() core from the analytics catch-all,
parameterised by version/base/method/body/cache, and drive the stats
routes from a declarative table. A single stats/(?P<subpath>.+) route
absorbs every /sites/<id>/stats/* endpoint; purchases uses a path
override. Adds check_stats_permission and check_wordads_permission;
the analytics route is unchanged.

Local/non-proxy endpoints (posts, notices, user-feedback) and the
stats-admin deprecation are deferred to follow-ups.
@kangzj

This comment has been minimized.

@claude

This comment has been minimized.

@jp-launch-control

jp-launch-control Bot commented Jun 12, 2026

Copy link
Copy Markdown

Code Coverage Summary

Coverage changed in 1 file.

File Coverage Δ% Δ Uncovered
projects/packages/premium-analytics/src/REST/class-api-proxy-controller.php 123/163 (75.46%) 26.74% -20 💚

Full summary · PHP report

@github-actions

github-actions Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ Add a "[Status]" label (In Progress, Needs Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

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:

  1. Ensure all required checks appearing at the bottom of this PR are passing.
  2. Make sure to test your changes on all platforms that it applies to. You're responsible for the quality of the code you ship.
  3. You can use GitHub's Reviewers functionality to request a review.
  4. When it's reviewed and merged, you will be pinged in Slack to deploy the changes to WordPress.com simple once the build is done.

If you have questions about anything, reach out in #jetpack-developers for guidance!

@github-actions github-actions Bot added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Jun 12, 2026
- Add validate_subpath() (rejects traversal/absolute, allows UTM commas) on
  the stats/(?P<subpath>.+) route, for parity with the analytics endpoint guard.
- Invalidate the matching read cache on a successful write to the dashboard
  modules / module-settings routes, mirroring stats-admin.
- Make the proxy error strings route-neutral (no longer always "analytics").
- Guard build_stats_path against a missing wpcom template; add subpath tests.
@kangzj

This comment has been minimized.

@kangzj

This comment has been minimized.

@claude

This comment has been minimized.

Replace the per-endpoint route table with a single data proxy that accepts
any sub-path under an allowed top-level prefix and lets the caller pick the
WPCOM API version (base is derived: v2 -> wpcom, v1.x -> rest).

Security boundary is the prefix allowlist baked into the route regex
(stats, wordads, subscribers, jetpack-stats, jetpack-stats-dashboard,
commercial-classification, upgrades), plus a write-method allowlist so only
a few endpoints may mutate; everything else is read-only (405). Traversal
guard, force_refresh cache bypass, and per-version cache keys included.

The analytics proxy/* route is unchanged.
@kangzj

This comment has been minimized.

@claude

This comment has been minimized.

… writes

- Classify the endpoint prefix case-insensitively in check_data_permission,
  is_write_allowed and busts_cache. WordPress matches REST routes case-
  insensitively, so a mixed-case `WordAds/...` no longer slips past the
  activate_wordads gate as a stats endpoint.
- Strip the caller-supplied `site` param before forwarding, so it can't
  duplicate / override the server-pinned site on the `upgrades` path.
- Only POST may mutate; PUT/PATCH (the rest of EDITABLE) now 405 locally,
  matching the originating stats-admin verbs.
@kangzj

This comment has been minimized.

@kangzj

This comment has been minimized.

@claude

This comment has been minimized.

@kangzj

kangzj commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Review-cycle summary — 5485a3ccc9ed8c9aad0a

5 round(s); CI green; security review (@claude) LGTM; 0 reviewer threads left open.

What changed during the cycle

Commits added:

  • ed8c9aad0a — Address security review: case-insensitive checks, pin site, POST-only writes
  • f10f8e45c4 — Premium Analytics: make the stats proxy endpoint-agnostic
  • 57db73b19f — Address review: validate stats subpath, bust write cache, neutral errors

Diff summary: 2 files changed, 430 insertions(+), 232 deletions(-)

Review items addressed (all from @claude):

# Finding Severity Resolution
1 subpath not traversal-validated like the analytics route medium Added validation (later folded into the agnostic validate_data_endpoint)
2 Write-then-read cache staleness on dashboard modules/settings medium Bust the matching read cache on successful write (mirrors stats-admin)
4 Error strings always said "analytics" nit Route-neutral messages
5 build_*_path missing-template guard nit ?? '' guard
Design pivot: per-endpoint registry → agnostic proxy Prefix allowlist in route regex + caller-supplied version; write-method allowlist
S1 Case-insensitive routing bypassed the WordAds authz gate medium Case-insensitive prefix classification across all checks
S2 Caller ?site= could duplicate the pinned upgrades site medium Strip site from forwarded params
S3 EDITABLE widened writes to PUT/PATCH low Only POST may mutate; others 405 locally

Tests: 48 → 63 passing. phpcs clean. Coverage requirement met.

Unaddressed (flagged for owner): None.

CI: all required checks passing.

@kangzj kangzj added [Status] Needs Team Review Obsolete. Use Needs Review instead. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Jun 12, 2026
Fold analytics into the single agnostic route as the `analytics` prefix and
remove the dedicated proxy/* route, so both surfaces share one shape: the
caller passes ?version= (analytics defaults to 2) and the path is
/sites/<id>/<prefix>/<subpath>. Analytics keeps its manage_options gate via
check_data_permission dispatch.

BREAKING: the analytics data layer must now call
/jetpack-premium-analytics/v1/analytics/<subpath> instead of /proxy/<subpath>
(tracked as a frontend follow-up). Drops the now-dead handle_proxy_request,
validate_endpoint and get_item_schema.
@kangzj kangzj added [Status] In Progress and removed [Status] Needs Team Review Obsolete. Use Needs Review instead. labels Jun 12, 2026
@kangzj

kangzj commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up to the agnostic pivot: per maintainer feedback, analytics is now unified onto the same route (commit b4215e1b). Analytics is the analytics prefix on the single data route (defaults to version=2, keeps its manage_options gate via check_data_permission); the dedicated proxy/* route is removed. handle_proxy_request/validate_endpoint/get_item_schema are gone.

This is a breaking change for the analytics data layer (proxy/<subpath>analytics/<subpath>), tracked in WOOA7S-1539. @claude please re-review (security lens on the unified permission dispatch + the analytics→manage_options branch). 58 tests pass, phpcs clean.

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kangzj's task in 3m 5s —— View job


Re-review: unified permission dispatch + analytics→manage_options

  • Read the unified controller after the analytics pivot
  • Security review: permission dispatch + prefix classification
  • Review the analytics→manage_options branch
  • Check write-method allowlist & cache invalidation
  • Post findings

Reviewed class-api-proxy-controller.php at b4215e1b with a security lens on the unified dispatch. The analytics→manage_options branch itself is correct, but I found one high-severity issue that I think undermines the central security boundary of this PR. Details below.


🔴 High — the prefix allowlist can be bypassed via WP parameter precedence

The PR's stated security boundary is "the route only matches these prefixes, so the blog token can never be driven against the whole WPCOM site API." That guarantee holds for the URL path, but not for the value actually used to build the WPCOM path.

Both the permission dispatch and the request handler read the endpoint with get_param():

// check_data_permission()
$endpoint = strtolower( (string) $request->get_param( 'endpoint' ) );
// handle_data_request()
$endpoint = (string) $request->get_param( 'endpoint' );

In WordPress, WP_REST_Request::get_param() resolves through get_parameter_order(), and URL (route-matched) params have lower precedence than JSON / POST / GET params — the order is roughly JSON → POST → GET → URL → defaults. So a caller can shadow the route-matched endpoint capture with a query/body param of the same name, and the regex allowlist only ever constrained the URL capture, not the override.

validate_data_endpoint() is the only other gate on the effective value, and it checks traversal + charset only — it does not re-check the prefix against ALLOWED_PREFIXES:

public function validate_data_endpoint( $value ): bool {
    $value = (string) $value;
    if ( str_contains( $value, '..' ) ) {
        return false;
    }
    return (bool) preg_match( '#^[\w.,/-]+$#', $value );  // no prefix check
}

Proof of concept (a user with only view_stats — which can be granted to non-admins via the Stats roles setting):

GET /wp-json/jetpack-premium-analytics/v1/stats/x?endpoint=me/settings
  • Route matches via the stats URL capture → regex satisfied.
  • get_param('endpoint') returns the GET value me/settings.
  • validate_data_endpoint('me/settings') → no .., charset OK → passes.
  • check_data_permission: not analytics, not wordads → falls through to check_stats_permission()view_stats passes.
  • build_data_path('me/settings')/sites/<id>/me/settings, forwarded blog-token-signed.

The net effect: any view_stats-capable user can drive the privileged blog token against arbitrary /sites/<id>/<anything> WPCOM endpoints (anything matching ^[\w.,/-]+$), which is exactly what the prefix allowlist was meant to prevent. The permission tiers (manage_options / activate_wordads) can also be sidestepped by choosing an endpoint whose first segment isn't analytics/wordads, since the dispatcher classifies on the same overridable value.

Fix — re-enforce the prefix allowlist on the effective value, in validate_data_endpoint() (it runs against the get_param result, so it closes the hole regardless of which param source wins, and rejects with a 400 before the handler forwards):

public function validate_data_endpoint( $value ): bool {
    $value = (string) $value;
    if ( str_contains( $value, '..' ) ) {
        return false;
    }
    if ( ! preg_match( '#^[\w.,/-]+$#', $value ) ) {
        return false;
    }
    // The route regex only constrains the URL capture; get_param() can be shadowed by a
    // GET/JSON/POST `endpoint`, so re-check the top-level prefix on the effective value.
    $prefix = strtolower( explode( '/', $value )[0] );
    return in_array( $prefix, self::ALLOWED_PREFIXES, true );
}

(ALLOWED_PREFIXES is already lowercase, and the case-insensitive routing is preserved by lowercasing the prefix.) Fix this →

A test that locks this down: build a request whose URL capture is an allowed prefix but whose query endpoint is me/settings, and assert validate_data_endpoint returns false.


What checks out ✅

  • analytics→manage_options branchcheck_data_permission correctly orders analytics (strictest) before wordads before the stats fallback, and test_analytics_requires_manage_options confirms view_stats is rejected for the analytics prefix. Good.
  • Case-insensitive classification — lowercasing in check_data_permission, is_write_allowed, and busts_cache correctly mirrors WP's case-insensitive (@i) route matching, closing the earlier WordAds-gate bypass.
  • Write-method allowlisthandle_data_request rejecting everything but GET and allowlisted POST (incl. PUT/PATCH → 405) is sound, and consistent across the overridable value (so the bypass above can't widen writes beyond WRITE_PREFIXES).
  • site stripping + upgrades pinning, write-then-read cache busting, version/base derivation, and neutral error strings all look correct.

Note

Once the prefix re-check lands, the route regex still has value as a first-line filter, but validate_data_endpoint becomes the real boundary — worth a comment to that effect so a future refactor doesn't "simplify" it away.
· add/premium-analytics-stats-proxy-endpoints

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant