Connection: add Proxy_Request, the forward to WordPress.com as its own class

[retrofox]

Fixes CONNECT-495. Built on #53238, whose commit shows in this diff until it merges.

Proposed changes

WPCOM_REST_API_Proxy_Request is the connection package's way of forwarding a REST request to wpcom, signed as the user or as the blog.

Its one public method does four things at once:

• it builds the path from the controller's rest_base
• signs and sends the request
• decodes the body
• turns an upstream error into a WP_Error.

It needs a WP_REST_Request to do any of it, so code with no route builds an empty one to satisfy the parameter.

A caller that needs the response as wpcom sent it, with its status and headers, has no way in and ends up signing its own request against Client.

This PR moves the forward into a class of its own and leaves the trait as its adapter, the shape agreed in pdWQjU-1Ke-p2.

Proxy_Request

Static, with no route and no WP_REST_Request:

• to_path( $path, $args ) forwards to a wpcom path, and to_site( $site_path, $args ) to a path under /sites/<blog id>/.
• $args takes context (user, blog, or none for an unsigned request), allow_fallback_to_blog, method, query, body, version, base_api_path, request_options, and unauthorized_error.
• It returns status, body, and headers as wpcom sent them, or Client's own WP_Error. A missing token returns the caller's unauthorized_error, which is rest_unauthorized by default.
• decode( $response ) is the typed presentation: the decoded body, or a WP_Error with the upstream code, message, and status for a status of 400 or more.

The trait

proxy_request_to_wpcom() keeps its signature and behavior.
It reads the method, the query, and the body from the request, calls Proxy_Request::to_site(), and decodes.

proxy_request_to_wpcom_as_user() and proxy_request_to_wpcom_as_blog() are untouched.

The class also takes over the query string the trait builds since #53238. The rule moves from the trait to the forward; it does not change.

Tests

Proxy_Request_Test drives the class directly and through the trait, down to the HTTP layer.
It covers the request assembly (path, query string minus rest_route, method, body, headers, blog signature), the four token contexts, the error translation, a transport error passing through untouched, the raw response (status, body, and headers as received), the unsigned request, and the caller's unauthorized_error.

Not in this PR

The read cache declared by the consumer, and Proxy_Controller on top of this class (#53098). Each follows in its own PR.

Related product discussion/links

• pdWQjU-1Ke-p2
• Connection: encode the query params the proxy trait forwards to WordPress.com #53238
• Connection: add Proxy_Controller, a shared REST proxy to WordPress.com #53098

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

No.

Testing instructions

jp test php packages/connection
jp test php packages/publicize

Both suites are green. Proxy_Request_Test covers the class and the trait, and the Publicize suite drives the trait through its controllers.

Behavior for existing callers is unchanged. On a connected site with Jetpack Social active, open the Social settings and the Jetpack editor sidebar.

The connections list (/wpcom/v2/publicize/connections) and the services list load as on trunk, since both go through proxy_request_to_wpcom_as_user().

In the browser console, with a site connected:

wp.apiFetch( { path: '/wpcom/v2/publicize/connections' } )
    .then( console.log)
    .catch( console.log )

With the site disconnected, the same call still returns rest_unauthorized:

wp.apiFetch( { path: '/wpcom/v2/publicize/connections' } )
    .then( console.log)
    .catch( console.log )

To call the class directly, rebuild the plugin so its autoloader knows the new class (jp build plugins/jetpack), then:

jp docker wp eval 'print_r( Automattic\Jetpack\Connection\Proxy_Request::to_site( "stats/summary", array( "context" => "blog", "version" => "1.1", "base_api_path" => "rest" ) ) );'

It prints the status, the body as a JSON string, and the headers, as wpcom answered.

[github-actions]

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

• To test on WoA, go to the Plugins menu on a WoA dev site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin (Jetpack or WordPress.com Site Helper), and enable the update/connection-proxy-forward-core branch.
• To test on Simple, run the following command on your sandbox:

bin/jetpack-downloader test jetpack update/connection-proxy-forward-core

bin/jetpack-downloader test jetpack-mu-wpcom-plugin update/connection-proxy-forward-core

Interested in more tips and information?

• In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
• Read more about our development workflow here: PCYsg-eg0-p2
• Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

[github-actions]

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!

[jp-launch-control]

Code Coverage Summary

Coverage changed in 1 file.

File	Coverage	Δ%	Δ Uncovered
projects/packages/connection/src/traits/trait-wpcom-rest-api-proxy-request.php	19/20 (95.00%)	95.00%	-43 💚

1 file is newly checked for coverage.

File	Coverage
projects/packages/connection/src/class-proxy-request.php	74/74 (100.00%) 💚

Full summary · PHP report · JS report

[fgiannar]

Great work here @retrofox 👍

Reviewed this for security, performance and architecture. The shape is right and matches what we landed on in pdWQjU-1Ke-p2: the trait keeps its three public methods and their behaviour, the forward is reusable without a route or a request object, and Proxy_Request_Test is a real gain on a trait that had no tests at all.

Fix before merge

1. The unsigned path has no timeout or redirection defaults. class-proxy-request.php, send()'s 'none' branch.

Client::build_signed_request() sets 'timeout' => 10, 'redirection' => 0 in its defaults, so both signed paths refuse redirects. Client::validate_args_for_wpcom_json_api_request() only passes those keys through via array_intersect_key and supplies no defaults of its own, so the unsigned branch calling wp_remote_request() directly inherits WP_Http's: 5 s timeout and up to 5 redirects.

Two problems. The timeout is tighter than its siblings, so a slow WordPress.com response a signed forward tolerates will fail here. And following redirects on a server-side request is something we deliberately don't do anywhere else in Client — it means a Location: on a proxied path gets chased from the WordPress server to wherever it points.

Nothing reaches it today (the trait always passes user or blog), so it's new API shipping with worse characteristics than the rest rather than a regression — but Premium Analytics is the intended first consumer, so it's worth pinning now:

$options = array_replace_recursive(
    array( 
        'headers'     => $headers,
        'method'      => strtoupper( (string) ( $args['method'] ?? 'GET' ) ),
        'timeout'     => 10,
        'redirection' => 0,
    ),
    (array) ( $args['request_options'] ?? array() )
);

2. context => 'none' silently widens the trait's public contract. trait-wpcom-rest-api-proxy-request.php.

On trunk, proxy_request_to_wpcom() seeded $response to rest_unauthorized and overwrote it only inside the user and blog branches — so any other $context, 'none' included, returned an error without issuing a request. Now 'none' reaches the wire.

Every in-repo call site passes user or blog, so nothing here changes. But this is public API of automattic/jetpack-connection with $context as a plain string parameter, and the trait's docblock still says "Can be Either 'user' or 'blog'". An out-of-repo consumer passing a dynamic context goes from "always refuses" to "issues an outbound request with the caller's path and query". Either whitelist user|blog in the trait before delegating, or update the docblock — the class keeping 'none' is fine either way.

3. The REST_Endpoints_Test fix is conditioned on the wrong signal, and the comment contradicts the code. tests/php/REST_Endpoints_Test.php

I traced the pollution and your diagnosis is right: Proxy_Request_Test extends WorDBless\BaseTestCase, whose tear_down_wordbless() → _restore_hooks() resets $wp_filter and $wp_actions to a snapshot taken at the first WorDBless test in the run, and the new file sorts into a gap (REST < Remote, because E < e) where no hook-restoring test previously ran. REST_Endpoints_Test extends PHPUnit\Framework\TestCase with no hook isolation, so it had been living off hooks leaked by earlier classes.

But the guard is ! has_filter( 'jetpack_register_request_body' ), and Identity_Crisis::init(), Manager::configure() and Jetpack_Network all hook that same filter. If any of them ran first, Partner stays unhooked — which is precisely the leaked-hook dependency the comment says it avoids. And Partner::reset() only nulls the singleton, so this adds six filters that tearDown never removes, into a class with no isolation.

The cleaner fix is to stop restoring hooks mid-suite: have Proxy_Request_Test extends PHPUnit\Framework\TestCase and use WorDBless\Options / WorDBless\Users directly, as Protected_Owner_Test and REST_Endpoints_Test already do. Then this change isn't needed at all. If you'd rather keep BaseTestCase, hook Partner unconditionally and remove the filters in tearDown — and please run the whole connection suite in file order, because _restore_hooks() rolls back $wp_actions too and Partner may not be the only casualty.

4. $path isn't validated, and that reopens the injection build_query() closes. class-proxy-request.php

Client only trims and sprintf()s the path. On a signed request build_signed_request() then runs the whole URL through add_query_arg(), which splits on the first ? and re-parses the remainder with keys written back decoded. So stats?_method=DELETE sitting in $path reaches WordPress.com as a parameter — drop_unsafe_query_keys() never sees it, because it only inspects $args['query']. A # truncates the path instead.

.. behaves differently per context. WP's Requests library never sets CURLOPT_PATH_AS_IS, so curl squashes dot segments before sending: signed, the escape dies on the signature and logs a connection error, but on 'none' nothing stops /sites/<id>/x/../../sites/<other>/posts. The trait also rawurldecode()s $path a second time after WP has already decoded it, so a capture admitting % or . could carry %253F or %2e%2e through.

No consumer is exposed today — all eight forwarded captures are digits or [a-z_-] (connection_id, action_id, product_id, subscription_id, postId, id, plus list_id and task_id). Suggested fix: reject ?, #, %, whitespace and ./.. segments in to_path(), and drop the trait's second decode. This pairs with point 2, because 'none' is what turns a signature failure into a real scope escape.

One thing to settle alongside it: #53098's path option is a path-with-query template, documented as /upgrades?site=%d, for groups that don't sit under /sites/<id>/. So a flat ? ban needs that case to pass site through query instead — otherwise the rule and your next consumer disagree. That also lets the str_contains( $path, '?' ) branch in to_path() go away rather than stay as the thing that accommodates the hazard.

5. to_site() guards the blog id only for 'none'. class-proxy-request.php

is_user_connected() is just get_current_user_id() plus get_access_token( $user_id ) — it never checks the blog id, where is_connected() short-circuits on $has_blog_id. So a site holding a user token with no id option signs and sends /sites/0/…. One line to refuse it for every context rather than only the unsigned one.

Worth fixing

unauthorized_error accepts a 2xx status. $error['status'] ?? rest_authorization_required_code() isn't validated. rest_convert_error_to_response() only does is_numeric() and a cast — there's no floor — so 'status' => 200 yields a WP_Error served as HTTP 200 with the error payload as the body, i.e. a missing-token refusal that a JS client reads as success.

Flagging it because it's new rather than inherited: trunk hardcoded array( 'status' => rest_authorization_required_code() ), so a caller had no way to influence the status. (decode()'s status is also caller-visible but gated on >= 400, so that path can't produce a sub-400 error.) Nothing passes the key today except the test, so this is about not shipping the footgun — clamp anything under 400 back to rest_authorization_required_code(), or drop the status override and keep code/message.

$args is a nine-key bag with no validation. A misspelling fails silently and wrongly: allow_fallback_to_blogs means "no fallback", so a user-context endpoint quietly starts refusing users without a user token, with no error anywhere; contex => 'blog' means "sign as the current user". Cheapest version is four lines — array_diff_key against the known keys plus _doing_it_wrong() under WP_DEBUG. Worth deciding on an options object now rather than at twelve keys.

request_options hands Client's whole allowlist to the caller. It passes headers, method, format, timeout, redirection, stream, filename, sslverify straight through — so stream/filename write the response to a path of the caller's choosing, sslverify can be turned off, and method beats $args['method'] because array_replace_recursive() puts request_options last. On 'none' a caller Authorization or Cookie header also goes out untouched (on the signed paths build_signed_request() overwrites Authorization, so that part is 'none'-only). For a proxy entry point I'd accept headers and timeout only, and set method after the merge rather than before. Same family as the $args point above.

Hygiene: the stacked commits leave a duplicate test suite. This diff carries #53238's commits and its changelog, and once #53238 merges its tests/php/WPCOM_REST_API_Proxy_Request_Test.php survives alongside the new Proxy_Request_Test.php — which repeats the same data_queries provider, case names included. The production code did get deduped (the trait no longer carries build_wpcom_query/drop_unsafe_query_keys), so it's only the tests. Worth deleting the older file here, or folding the cases in.

Behaviour deltas I checked — all benign or improvements

• A raw body of exactly '0' is now forwarded; trunk's truthiness test turned it into null. A fix.
• With $version / $base_api_path unset, the ?? '2' / ?? 'wpcom' defaults now match Client's own. Trunk passed null straight through to trim(null, '/'), which is also a PHP 8.1 deprecation. Strict improvement.
• Blog id rawurldecode( $blog_id ) → (int), which also means a corrupted id option can no longer reach the path.
• allow_fallback_to_blog is exactly equivalent to trunk for every value a consumer passes (false refuses; true, null, 0, '', '0' fall back). Though the concept now carries false !== $x in the trait and empty() in the class — worth collapsing to one rule, or a line saying the trait's is the old API's quirk.
• Empty rest_base collapses /sites/ID//path to /sites/ID/path.