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 ); + } +}