From ddf0a0e3ee4a89526da6959bf915fbbf5bbe7e7a Mon Sep 17 00:00:00 2001 From: Robert O'Rourke Date: Wed, 23 Sep 2026 13:00:19 +0100 Subject: [PATCH] Return the OAuth state parameter to the client unchanged RFC 6749 section 4.1.2 requires the authorization response to carry the exact state value the client sent, so the client can match the response to its request and reject a forged one. Three things broke that: - state went through sanitize_text_field(), which strips percent-encoded octets, tags and surrounding whitespace. A state of "%41" came back empty, so the client saw a CSRF mismatch. State is opaque and is only ever written into the redirect URL, where it is encoded, so it is now kept as the raw unslashed string. A non-string (state[]=x) is treated as absent. - Both grants used empty() to decide whether to echo state, which drops a valid state of "0". They now use isset(). - The implicit grant built its fragment with build_query(), which does not encode. A state containing & or # corrupted the fragment. Values are now encoded first, as the authorization code grant already does through add_query_arg(). Co-Authored-By: Claude Opus 5.5 --- inc/types/class-authorization-code.php | 2 +- inc/types/class-base.php | 3 +- inc/types/class-implicit.php | 4 +- tests/test-state.php | 124 +++++++++++++++++++++++++ 4 files changed, 129 insertions(+), 4 deletions(-) create mode 100644 tests/test-state.php diff --git a/inc/types/class-authorization-code.php b/inc/types/class-authorization-code.php index dae2470..62a9679 100644 --- a/inc/types/class-authorization-code.php +++ b/inc/types/class-authorization-code.php @@ -61,7 +61,7 @@ protected function handle_authorization_submission( $submit, Client $client, $da ); } - if ( ! empty( $data['state'] ) ) { + if ( isset( $data['state'] ) ) { $redirect_args['state'] = $data['state']; } diff --git a/inc/types/class-base.php b/inc/types/class-base.php index 3c675b0..a35cf13 100644 --- a/inc/types/class-base.php +++ b/inc/types/class-base.php @@ -44,7 +44,8 @@ public function handle_authorisation() { $client_id = sanitize_text_field( wp_unslash( $_GET['client_id'] ) ); $redirect_uri = isset( $_GET['redirect_uri'] ) ? sanitize_text_field( wp_unslash( $_GET['redirect_uri'] ) ) : null; $scope = isset( $_GET['scope'] ) ? sanitize_text_field( wp_unslash( $_GET['scope'] ) ) : null; - $state = isset( $_GET['state'] ) ? sanitize_text_field( wp_unslash( $_GET['state'] ) ) : null; + // State is opaque to the server and must be returned to the client unchanged. + $state = isset( $_GET['state'] ) && is_string( $_GET['state'] ) ? wp_unslash( $_GET['state'] ) : null; // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized $client = Client::get_by_id( $client_id ); if ( empty( $client ) ) { diff --git a/inc/types/class-implicit.php b/inc/types/class-implicit.php index 35151f5..c9e9162 100644 --- a/inc/types/class-implicit.php +++ b/inc/types/class-implicit.php @@ -62,7 +62,7 @@ protected function handle_authorization_submission( $submit, Client $client, $da ); } - if ( ! empty( $data['state'] ) ) { + if ( isset( $data['state'] ) ) { $redirect_args['state'] = $data['state']; } @@ -73,7 +73,7 @@ protected function handle_authorization_submission( $submit, Client $client, $da $data ); - $fragment = build_query( $redirect_args ); + $fragment = build_query( urlencode_deep( $redirect_args ) ); $generated_redirect = $redirect_uri . '#' . $fragment; wp_safe_redirect( $generated_redirect ); exit; diff --git a/tests/test-state.php b/tests/test-state.php new file mode 100644 index 0000000..6ea80d8 --- /dev/null +++ b/tests/test-state.php @@ -0,0 +1,124 @@ +user->create() ); + add_filter( 'wp_redirect', [ $this, 'interrupt_redirect' ] ); + } + + public function tear_down() { + remove_filter( 'wp_redirect', [ $this, 'interrupt_redirect' ] ); + $_GET = []; + $_POST = []; + parent::tear_down(); + } + + public function interrupt_redirect( $location ) { + throw new State_Redirect_Interrupt( $location ); + } + + /** + * Approve an authorisation request and return the redirect location. + * + * @param Base $type Grant type handler. + * @param Client $client Client being authorised. + * @param mixed $state Raw state value, or null to omit it. + * @return string Redirect location. + */ + protected function authorize( Base $type, Client $client, $state ) { + $_GET = [ + 'client_id' => $client->get_id(), + 'redirect_uri' => $client->get_redirect_uris()[0], + ]; + if ( null !== $state ) { + $_GET['state'] = is_string( $state ) ? wp_slash( $state ) : $state; + } + $_POST = [ + '_wpnonce' => wp_create_nonce( sprintf( 'oauth2_authorize:%s', $client->get_id() ) ), + 'wp-submit' => 'authorize', + ]; + + try { + $type->handle_authorisation(); + } catch ( State_Redirect_Interrupt $redirect ) { + return $redirect->getMessage(); + } + + $this->fail( 'Expected a redirect.' ); + } + + public function state_provider() { + return [ + 'percent-encoded octets' => [ '%41%42' ], + 'tag-like text' => [ 'x' ], + 'zero' => [ '0' ], + 'reserved characters' => [ 'a&b=c#d' ], + 'surrounding spaces' => [ ' padded ' ], + 'quotes and slashes' => [ 'it\'s "a\\b"' ], + ]; + } + + /** + * @dataProvider state_provider + */ + public function test_authorization_code_returns_exact_state( $state ) { + $client = $this->create_client(); + $location = $this->authorize( new Authorization_Code(), $client, $state ); + + parse_str( wp_parse_url( $location, PHP_URL_QUERY ), $args ); + $this->assertSame( $state, $args['state'] ); + } + + /** + * @dataProvider state_provider + */ + public function test_implicit_returns_exact_state( $state ) { + // A same-site callback, since the implicit grant redirects with wp_safe_redirect(). + $client = $this->create_client( [ 'callback' => home_url( '/callback' ) ] ); + $location = $this->authorize( new Implicit(), $client, $state ); + + parse_str( wp_parse_url( $location, PHP_URL_FRAGMENT ), $args ); + $this->assertSame( $state, $args['state'] ); + $this->assertArrayHasKey( 'access_token', $args ); + } + + public function test_absent_state_is_not_returned() { + $client = $this->create_client(); + $location = $this->authorize( new Authorization_Code(), $client, null ); + + parse_str( wp_parse_url( $location, PHP_URL_QUERY ), $args ); + $this->assertArrayNotHasKey( 'state', $args ); + } + + public function test_array_state_is_treated_as_absent() { + $client = $this->create_client(); + $location = $this->authorize( new Authorization_Code(), $client, [ 'x' ] ); + + parse_str( wp_parse_url( $location, PHP_URL_QUERY ), $args ); + $this->assertArrayNotHasKey( 'state', $args ); + } +}