Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
a24433a
Jetpack Connection: Add inline comments when whole input is processed
fgiannar May 15, 2024
96d39a1
changelog
fgiannar May 15, 2024
f2e877d
Jetpack Connection: Bump package version
fgiannar May 15, 2024
82a5f66
More inline comments
fgiannar May 15, 2024
2281c4d
Jetpack Connection: Add explanation why we need the whole request in …
fgiannar May 15, 2024
45bd688
Manager: Avoid passing whole GET when calling setup_xmlrpc_handlers
fgiannar May 15, 2024
d05d62b
Jetpack: Update related unit tests
fgiannar May 15, 2024
fed7693
changelog
fgiannar May 15, 2024
cb77207
Webhooks: Explain why we need the whole request data in handle_author…
fgiannar May 15, 2024
914cd6b
SSO: Add explanation on missing nonce check in 'should_send_wp_mail_n…
fgiannar May 15, 2024
cdbf018
Update projects/packages/connection/legacy/class-jetpack-signature.php
fgiannar May 16, 2024
7ea4300
Update projects/packages/connection/legacy/class-jetpack-signature.php
fgiannar May 16, 2024
1cbe47d
Manager: Update annotation for removed param in setup_xmlrpc_handlers
fgiannar May 16, 2024
27fce9d
Update changelog
fgiannar May 16, 2024
43180fc
changelog
fgiannar May 16, 2024
d469817
Bump project versions
fgiannar May 16, 2024
ebab7e2
Merge branch 'trunk' into update/connection-processing-whole-input-co…
fgiannar May 16, 2024
b88ccae
Authorize_Json_Api: Use only the request params we need in verify_jso…
fgiannar May 16, 2024
ada5a67
Merge branch 'update/connection-processing-whole-input-comments' of g…
fgiannar May 16, 2024
369d209
Authorize_Json_Api: Use array_intersect_key in verify_json_api_author…
fgiannar May 17, 2024
3d68471
Connection Manager: Deprecate request_params param in setup_xmlrpc_ha…
fgiannar May 17, 2024
1153459
Merge branch 'trunk' into update/connection-processing-whole-input-co…
fgiannar May 17, 2024
8aa9581
Changelog
fgiannar May 17, 2024
ac19937
Hotfix
fgiannar May 17, 2024
1fe0ed0
Fixup project versions
fgiannar May 17, 2024
3090b1a
Cleanup
fgiannar May 17, 2024
18b2de3
Update missing nonce verification explanation
fgiannar May 17, 2024
4ae0f89
Connection Manager: Update docblock for setup_xmlrpc_handlers - para…
fgiannar May 17, 2024
4c94ded
Connection Manager: Add deprecation notice for request_params arg in …
fgiannar May 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: deprecated

Jetpack Connection Manager: Deprecate `request_params` arg in setup_xmlrpc_handlers method
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,9 @@ public function sign_current_request( $override = array() ) {
// Convert the $_POST to the body, if the body was empty. This is how arrays are hashed
// and encoded on the Jetpack side.
if ( defined( 'IS_WPCOM' ) && IS_WPCOM ) {
// phpcs:ignore WordPress.Security.NonceVerification.Missing
// phpcs:ignore WordPress.Security.NonceVerification.Missing -- Used to generate a cryptographic signature of the post data.
if ( empty( $body ) && is_array( $_POST ) && $_POST !== array() ) {
$body = $_POST; // phpcs:ignore WordPress.Security.NonceVerification.Missing
$body = $_POST; // phpcs:ignore WordPress.Security.NonceVerification.Missing -- We need all of $_POST in order to generate a cryptographic signature of the post data.
}
}
} elseif ( isset( $_SERVER['REQUEST_METHOD'] ) && 'PUT' === strtoupper( $_SERVER['REQUEST_METHOD'] ) ) { // phpcs:ignore WordPress.Security.ValidatedSanitizedInput -- This is validating.
Expand Down
14 changes: 11 additions & 3 deletions projects/packages/connection/src/class-authorize-json-api.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,17 @@ class Authorize_Json_Api {
* @return void
*/
public function verify_json_api_authorization_request( $environment = null ) {
$environment = $environment === null
? $_REQUEST // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- nonce verification handled later in function.
: $environment;
if ( null === $environment ) {
$request_params_needed_for_auth = array(
'data',
'jetpack_json_api_original_query',
'nonce',
'signature',
'timestamp',
'token',
);
$environment = array_intersect_key( $request_params_needed_for_auth, $_REQUEST ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended,WordPress.Security.ValidatedSanitizedInput.MissingUnslash,WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- nonce verification handled later in function and request data are 1) used to verify a cryptographic signature of the request data and 2) sanitized later in function.
Comment on lines +44 to +52
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work right, for two reasons:

  1. You need to put $_REQUEST first so it uses the values from the request.
  2. The $request_params_needed_for_auth needs to have keys 'data', 'nonce', and so on. So either define it like 'data' => true, or use array_flip( $request_params_needed_for_auth ).

As written, $environment will probably be empty. If there's a $_REQUEST[0] or the like, it might be array( 0 => 'data' ) or the like.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, thanks Brad!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you Brad and Sergey and apologies for any trouble caused.

I decided to keep things simple and removed the whole logic of extracting the auth request params in verify_json_api_authorization_request so we can focus on exactly what the original issue was about.
The debugger related test instructions/unit tests are only applicable for deprecating the input arg in setup_xmlrpc_handlers anyways.

I also audited where we actually use verify_json_api_authorization_request and it looks like it’s mainly under the context of SSO anyways where the $environment is passed here.
The other usage, comes from Jetpack-the-plugin as an action handler for login_form_jetpack_json_api_authorization here

Final PR: #37445

}

if ( ! isset( $environment['token'] ) ) {
wp_die( esc_html__( 'You must connect your Jetpack plugin to WordPress.com to use this feature.', 'jetpack-connection' ) );
Expand Down
27 changes: 13 additions & 14 deletions projects/packages/connection/src/class-manager.php
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ public static function configure() {
);

$manager->setup_xmlrpc_handlers(
$_GET, // phpcs:ignore WordPress.Security.NonceVerification.Recommended
null,
$manager->has_connected_owner(),
$manager->verify_xml_rpc_signature()
);
Expand Down Expand Up @@ -161,32 +161,31 @@ public static function configure() {
* Sets up the XMLRPC request handlers.
*
* @since 1.25.0 Deprecate $is_active param.
* @since $$next-version$$ Deprecate $request_params param.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not just deprecated, you removed it entirely.

Deprecated would probably mean the parameter still exists, but is no longer used inside the function. Or it's still used but you raise a deprecation message if it's not passed $_GET.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds correct, thanks Brad! The idea was to be consistent with the $is_active param deprecation annotation, introduced with #19264
In this case, to serve more as a reminder that there used to be an additional parameter here.
Updated with 1cbe47d

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Back to deprecated as per @sergeymitr 's suggestion :)

*
* @param array $request_params incoming request parameters.
* @param array|null $deprecated Deprecated. Not used.
* @param bool $has_connected_owner Whether the site has a connected owner.
* @param bool $is_signed whether the signature check has been successful.
* @param Jetpack_XMLRPC_Server $xmlrpc_server (optional) an instance of the server to use instead of instantiating a new one.
*/
public function setup_xmlrpc_handlers(
$request_params,
Copy link
Contributor

Choose a reason for hiding this comment

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

While it looks safe to change the function signature, I believe this is something we usually avoid for public methods.
Should we use some sort of backward compatibility safeguard here, like deprecating the $request_params argument in phpdoc and passing null in there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, updated it as suggested

Copy link
Contributor

@bindlegirl bindlegirl May 17, 2024

Choose a reason for hiding this comment

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

Sorry if I'm being too nitpicky here, but can I suggest we go with something like this:

public function setup_xmlrpc_handlers( $deprecated = null, $whatever_args_go_next ) {
    if ( ! is_null( $deprecated ) ) {
        _deprecated_argument('$request_params', '$$next-version$$', 'Passing $request_params to setup_xmlrpc_handlers'  is deprecated);
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice suggestion, thanks!
I went with:

_deprecated_argument( __METHOD__, '$$next-version$$' );

the first argument in _deprecated_argument is the method. Given that, I think the message might be redundant :)

Also I'd avoid setting it to null by default because there are more required arguments after that and optional arguments should go after the required ones.

$deprecated,
$has_connected_owner,
$is_signed,
Jetpack_XMLRPC_Server $xmlrpc_server = null
) {
add_filter( 'xmlrpc_blog_options', array( $this, 'xmlrpc_options' ), 1000, 2 );

if (
! isset( $request_params['for'] )
|| 'jetpack' !== $request_params['for']
) {
if ( $deprecated !== null ) {
_deprecated_argument( __METHOD__, '$$next-version$$' );
}
// phpcs:ignore WordPress.Security.NonceVerification.Recommended -- We are using the 'for' request param to early return unless it's 'jetpack'.
if ( ! isset( $_GET['for'] ) || 'jetpack' !== $_GET['for'] ) {
return false;
}

// Alternate XML-RPC, via ?for=jetpack&jetpack=comms.
if (
isset( $request_params['jetpack'] )
&& 'comms' === $request_params['jetpack']
) {
// phpcs:ignore WordPress.Security.NonceVerification.Recommended -- This just determines whether to handle the request as an XML-RPC request. The actual XML-RPC endpoints do the appropriate nonce checking where applicable. Plus we make sure to clear all cookies via require_jetpack_authentication called later in method.
if ( isset( $_GET['jetpack'] ) && 'comms' === $_GET['jetpack'] ) {
if ( ! Constants::is_defined( 'XMLRPC_REQUEST' ) ) {
// Use the real constant here for WordPress' sake.
define( 'XMLRPC_REQUEST', true );
Expand Down Expand Up @@ -455,9 +454,9 @@ private function internal_verify_xml_rpc_signature() {
}

$jetpack_signature = new \Jetpack_Signature( $token->secret, (int) \Jetpack_Options::get_option( 'time_diff' ) );
// phpcs:disable WordPress.Security.NonceVerification.Missing
// phpcs:disable WordPress.Security.NonceVerification.Missing -- Used to verify a cryptographic signature of the post data.
if ( isset( $_POST['_jetpack_is_multipart'] ) ) {
$post_data = $_POST;
$post_data = $_POST; // We need all of $_POST in order to verify a cryptographic signature of the post data.
$file_hashes = array();
foreach ( $post_data as $post_data_key => $post_data_value ) {
if ( ! str_starts_with( $post_data_key, '_jetpack_file_hmac_' ) ) {
Expand Down
2 changes: 1 addition & 1 deletion projects/packages/connection/src/class-webhooks.php
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ public function handle_authorize() {
}
do_action( 'jetpack_client_authorize_processing' );

$data = stripslashes_deep( $_GET );
$data = stripslashes_deep( $_GET ); // We need all request data under the context of an authorization request.
$data['auth_type'] = 'client';
$roles = new Roles();
$role = $roles->translate_current_user_to_role();
Expand Down
2 changes: 1 addition & 1 deletion projects/packages/connection/src/sso/class-user-admin.php
Original file line number Diff line number Diff line change
Expand Up @@ -835,7 +835,7 @@ public function render_custom_email_message_form_field( $type ) {
* @return boolean Indicating if the core invitation main should be sent.
*/
public function should_send_wp_mail_new_user( $send_wp_email ) {
if ( ! isset( $_POST['invite_user_wpcom'] ) && isset( $_POST['send_user_notification'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Missing
if ( ! isset( $_POST['invite_user_wpcom'] ) && isset( $_POST['send_user_notification'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Missing -- Hooked to 'wp_send_new_user_notification_to_user' to conditionally disable the core invitation email. At this point nonces should be checked already.
return $send_wp_email;
}
return false;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: other
Comment: Jetpack: Update connection related unit tests


Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ public function set_up() {
*/
public function tear_down() {
parent::tear_down();
unset( $_GET['for'] );
Constants::clear_constants();
StatusCache::clear();
}
Expand Down Expand Up @@ -770,11 +771,13 @@ public static function filter_pre_update_option_jetpack_active_modules( $modules
private function mocked_setup_xmlrpc_handlers( $request_params, $has_connected_owner, $is_signed, $user = false ) {
$GLOBALS['HTTP_RAW_POST_DATA'] = '';

$_GET['for'] = $request_params['for'];

Constants::set_constant( 'XMLRPC_REQUEST', true );

$jetpack = new MockJetpack();
$xmlrpc_server = new MockJetpack_XMLRPC_Server( $user );
return $jetpack::connection()->setup_xmlrpc_handlers( $request_params, $has_connected_owner, $is_signed, $xmlrpc_server );
return $jetpack::connection()->setup_xmlrpc_handlers( null, $has_connected_owner, $is_signed, $xmlrpc_server );
}

/**
Expand Down