From a79b4ad1af13c86e0524452eebdbc42164f919a4 Mon Sep 17 00:00:00 2001 From: Almir Bijedic Date: Tue, 5 Sep 2017 13:01:02 +0200 Subject: [PATCH 1/3] Validating redirect_uri according to https://tools.ietf.org/html/rfc6749#section-4.1.3 --- .gitignore | 5 ++- inc/class-client.php | 4 +- inc/endpoints/class-token.php | 2 +- inc/tokens/class-authorization-code.php | 51 +++++++++++++++++++++++-- inc/types/class-authorization-code.php | 3 +- inc/types/class-base.php | 26 ++++++------- 6 files changed, 68 insertions(+), 23 deletions(-) diff --git a/.gitignore b/.gitignore index 7b3dea9..876025b 100644 --- a/.gitignore +++ b/.gitignore @@ -14,4 +14,7 @@ _book *.epub *.mobi *.pdf -.idea \ No newline at end of file +.idea +.vscode +vendor +composer.lock \ No newline at end of file diff --git a/inc/class-client.php b/inc/class-client.php index ddba759..3b0f845 100644 --- a/inc/class-client.php +++ b/inc/class-client.php @@ -229,8 +229,8 @@ public function check_redirect_uri( $uri ) { * * @return Authorization_Code|WP_Error */ - public function generate_authorization_code( WP_User $user ) { - return Authorization_Code::create( $this, $user ); + public function generate_authorization_code( WP_User $user, $redirect_uri = '' ) { + return Authorization_Code::create( $this, $user, $redirect_uri ); } /** diff --git a/inc/endpoints/class-token.php b/inc/endpoints/class-token.php index 6b24a82..85b29a3 100644 --- a/inc/endpoints/class-token.php +++ b/inc/endpoints/class-token.php @@ -71,7 +71,7 @@ public function exchange_token( WP_REST_Request $request ) { return $auth_code; } - $is_valid = $auth_code->validate(); + $is_valid = $auth_code->validate( [ 'redirect_uri' => $request['redirect_uri'], 'code' => $request['code'] ] ); if ( is_wp_error( $is_valid ) ) { // Invalid request, but code itself exists, so we should delete // (and silently ignore errors). diff --git a/inc/tokens/class-authorization-code.php b/inc/tokens/class-authorization-code.php index e5f212e..a2f7ef7 100644 --- a/inc/tokens/class-authorization-code.php +++ b/inc/tokens/class-authorization-code.php @@ -129,6 +129,50 @@ public function validate( $args = [] ) { ); } + if ( ! empty( $args['code'] ) ) { + $key = static::KEY_PREFIX . $args['code']; + $value = get_post_meta( $this->client->get_post_id(), wp_slash( $key ), false ); + } + + if ( ! empty( $value[0]['redirect_uri'] ) ) { + + if ( empty( $args['redirect_uri'] ) ) { + return new WP_Error( + 'oauth2.tokens.authorization_code.redirect_uri.wrong', + __( 'Missing redirect_uri.', 'oauth2' ), + [ + 'status' => WP_Http::BAD_REQUEST, + 'expiration' => $expiration, + 'time' => $now, + ] + ); + } + + if ( $value[0]['redirect_uri'] !== $args['redirect_uri'] ) { + return new WP_Error( + 'oauth2.tokens.authorization_code.redirect_uri.mismatch', + __( 'redirect_uri does not match the one in the initial request.', 'oauth2' ), + [ + 'status' => WP_Http::BAD_REQUEST, + 'expiration' => $expiration, + 'time' => $now, + ] + ); + } + } + + if ( empty( $value[0]['redirect_uri'] ) && ! empty( $args['redirect_uri'] ) ) { + return new WP_Error( + 'oauth2.tokens.authorization_code.redirect_uri.mismatch', + __( 'redirect_uri does not match the one in the initial request.', 'oauth2' ), + [ + 'status' => WP_Http::BAD_REQUEST, + 'expiration' => $expiration, + 'time' => $now, + ] + ); + } + return true; } @@ -183,12 +227,13 @@ public static function get_by_code( Client $client, $code ) { * * @return Authorization_Code|WP_Error Authorization code instance, or error on failure. */ - public static function create( Client $client, WP_User $user ) { + public static function create( Client $client, WP_User $user, $redirect_uri = '' ) { $code = wp_generate_password( static::KEY_LENGTH, false ); $meta_key = static::KEY_PREFIX . $code; $data = [ - 'user' => (int) $user->ID, - 'expiration' => time() + static::MAX_AGE, + 'user' => (int) $user->ID, + 'expiration' => time() + static::MAX_AGE, + 'redirect_uri' => $redirect_uri, ]; $result = add_post_meta( $client->get_post_id(), wp_slash( $meta_key ), wp_slash( $data ), true ); if ( ! $result ) { diff --git a/inc/types/class-authorization-code.php b/inc/types/class-authorization-code.php index cff7e86..446556c 100644 --- a/inc/types/class-authorization-code.php +++ b/inc/types/class-authorization-code.php @@ -33,7 +33,8 @@ protected function handle_authorization_submission( $submit, Client $client, $da case 'authorize': // Generate authorization code and redirect back. $user = wp_get_current_user(); - $code = $client->generate_authorization_code( $user ); + $code = $client->generate_authorization_code( $user, $redirect_uri ); + if ( is_wp_error( $code ) ) { return $code; } diff --git a/inc/types/class-base.php b/inc/types/class-base.php index cde81a1..25e2442 100644 --- a/inc/types/class-base.php +++ b/inc/types/class-base.php @@ -52,9 +52,12 @@ public function handle_authorisation() { } // Validate the redirection URI. - $redirect_uri = $this->validate_redirect_uri( $client, $redirect_uri ); - if ( is_wp_error( $redirect_uri ) ) { - return $redirect_uri; + if ( ! empty( $redirect_uri ) ) { + $redirect_uri = $this->validate_redirect_uri( $client, $redirect_uri ); + + if ( is_wp_error( $redirect_uri ) ) { + return $redirect_uri; + } } // Valid parameters, ensure the user is logged in. @@ -69,8 +72,7 @@ public function handle_authorisation() { } // Check nonce. - $nonce_action = $this->get_nonce_action( $client ); - if ( ! wp_verify_nonce( wp_unslash( $_POST['_wpnonce'] ), $none_action ) ) { + if ( ! wp_verify_nonce( wp_unslash( $_POST['_wpnonce'] ), $this->get_nonce_action( $client ) ) ) { return new WP_Error( 'oauth2.types.authorization_code.handle_authorisation.invalid_nonce', __( 'Invalid nonce.', 'oauth2' ) @@ -106,16 +108,10 @@ public function handle_authorisation() { */ protected function validate_redirect_uri( Client $client, $redirect_uri = null ) { if ( empty( $redirect_uri ) ) { - $registered = $client->get_redirect_uris(); - if ( count( $registered ) !== 1 ) { - // Either none registered, or more than one, so error. - return new WP_Error( - 'oauth2.types.authorization_code.handle_authorisation.missing_redirect_uri', - __( 'Redirect URI was required, but not found.', 'oauth2' ) - ); - } - - $redirect_uri = $registered[0]; + return new WP_Error( + 'oauth2.types.authorization_code.handle_authorisation.missing_redirect_uri', + __( 'Redirect URI was required, but not found.', 'oauth2' ) + ); } else { if ( ! $client->check_redirect_uri( $redirect_uri ) ) { return new WP_Error( From 7516d6c5c733b446dc75cded6a0f4a4958beca68 Mon Sep 17 00:00:00 2001 From: Almir Bijedic Date: Wed, 6 Sep 2017 00:20:48 +0200 Subject: [PATCH 2/3] Adapt code for easier merge with pkce support branch --- inc/class-client.php | 4 +- inc/endpoints/class-token.php | 2 +- inc/tokens/class-authorization-code.php | 70 +++++++++++++------------ 3 files changed, 40 insertions(+), 36 deletions(-) diff --git a/inc/class-client.php b/inc/class-client.php index 3b0f845..31e588c 100644 --- a/inc/class-client.php +++ b/inc/class-client.php @@ -229,8 +229,8 @@ public function check_redirect_uri( $uri ) { * * @return Authorization_Code|WP_Error */ - public function generate_authorization_code( WP_User $user, $redirect_uri = '' ) { - return Authorization_Code::create( $this, $user, $redirect_uri ); + public function generate_authorization_code( WP_User $user, $data ) { + return Authorization_Code::create( $this, $user, $data ); } /** diff --git a/inc/endpoints/class-token.php b/inc/endpoints/class-token.php index 85b29a3..54e28db 100644 --- a/inc/endpoints/class-token.php +++ b/inc/endpoints/class-token.php @@ -71,7 +71,7 @@ public function exchange_token( WP_REST_Request $request ) { return $auth_code; } - $is_valid = $auth_code->validate( [ 'redirect_uri' => $request['redirect_uri'], 'code' => $request['code'] ] ); + $is_valid = $auth_code->validate( $request ); if ( is_wp_error( $is_valid ) ) { // Invalid request, but code itself exists, so we should delete // (and silently ignore errors). diff --git a/inc/tokens/class-authorization-code.php b/inc/tokens/class-authorization-code.php index a2f7ef7..54f1fec 100644 --- a/inc/tokens/class-authorization-code.php +++ b/inc/tokens/class-authorization-code.php @@ -108,35 +108,11 @@ public function get_expiration() { return (int) $value['expiration']; } - /** - * Validate the code for use. - * - * @param array $args Other request arguments to validate. - * @return bool|WP_Error True if valid, error describing problem otherwise. - */ - public function validate( $args = [] ) { - $expiration = $this->get_expiration(); - $now = time(); - if ( $expiration <= $now ) { - return new WP_Error( - 'oauth2.tokens.authorization_code.validate.expired', - __( 'Authorization code has expired.', 'oauth2' ), - [ - 'status' => WP_Http::BAD_REQUEST, - 'expiration' => $expiration, - 'time' => $now, - ] - ); - } - - if ( ! empty( $args['code'] ) ) { - $key = static::KEY_PREFIX . $args['code']; - $value = get_post_meta( $this->client->get_post_id(), wp_slash( $key ), false ); - } - - if ( ! empty( $value[0]['redirect_uri'] ) ) { + private function validate_redirect_uri( $args ) { + $value = $this->get_value(); - if ( empty( $args['redirect_uri'] ) ) { + if ( ! empty( $args['redirect_uri'] ) ) { + if ( empty( $value['redirect_uri'] ) ) { return new WP_Error( 'oauth2.tokens.authorization_code.redirect_uri.wrong', __( 'Missing redirect_uri.', 'oauth2' ), @@ -148,20 +124,17 @@ public function validate( $args = [] ) { ); } - if ( $value[0]['redirect_uri'] !== $args['redirect_uri'] ) { + if ( $value['redirect_uri'] !== $args['redirect_uri'] ) { return new WP_Error( 'oauth2.tokens.authorization_code.redirect_uri.mismatch', __( 'redirect_uri does not match the one in the initial request.', 'oauth2' ), [ 'status' => WP_Http::BAD_REQUEST, - 'expiration' => $expiration, - 'time' => $now, ] ); } } - - if ( empty( $value[0]['redirect_uri'] ) && ! empty( $args['redirect_uri'] ) ) { + if ( empty( $value['redirect_uri'] ) && ! empty( $args['redirect_uri'] ) ) { return new WP_Error( 'oauth2.tokens.authorization_code.redirect_uri.mismatch', __( 'redirect_uri does not match the one in the initial request.', 'oauth2' ), @@ -176,6 +149,37 @@ public function validate( $args = [] ) { return true; } + /** + * Validate the code for use. + * + * @param array $args Other request arguments to validate. + * @return bool|WP_Error True if valid, error describing problem otherwise. + */ + public function validate( $args = [] ) { + $expiration = $this->get_expiration(); + $now = time(); + if ( $expiration <= $now ) { + return new WP_Error( + 'oauth2.tokens.authorization_code.validate.expired', + __( 'Authorization code has expired.', 'oauth2' ), + [ + 'status' => WP_Http::BAD_REQUEST, + 'expiration' => $expiration, + 'time' => $now, + ] + ); + } + + $redirect_uri = $this->validate_redirect_uri( [ + 'redirect_uri' => $args['redirect_uri'], + ] ); + if ( is_wp_error( $redirect_uri ) ) { + return $redirect_uri; + } + + return true; + } + /** * Delete the authorization code. * From b06fe6e23ca3fbe49f3b00116764da432ee0b806 Mon Sep 17 00:00:00 2001 From: Almir Bijedic Date: Sat, 16 Sep 2017 13:57:51 +0200 Subject: [PATCH 3/3] Review changes; redirect_uri validation according to the standard; --- inc/tokens/class-authorization-code.php | 32 ++++++++++--------------- inc/types/class-base.php | 13 +++------- 2 files changed, 16 insertions(+), 29 deletions(-) diff --git a/inc/tokens/class-authorization-code.php b/inc/tokens/class-authorization-code.php index 54f1fec..d4e027f 100644 --- a/inc/tokens/class-authorization-code.php +++ b/inc/tokens/class-authorization-code.php @@ -108,23 +108,17 @@ public function get_expiration() { return (int) $value['expiration']; } - private function validate_redirect_uri( $args ) { + /** + * Validate whether the redirect_uri matches the one in the initial OAuth2 request + * + * @param String $redirect_uri + * @return Boolean|WP_Error + */ + protected function validate_redirect_uri( $redirect_uri ) { $value = $this->get_value(); - if ( ! empty( $args['redirect_uri'] ) ) { - if ( empty( $value['redirect_uri'] ) ) { - return new WP_Error( - 'oauth2.tokens.authorization_code.redirect_uri.wrong', - __( 'Missing redirect_uri.', 'oauth2' ), - [ - 'status' => WP_Http::BAD_REQUEST, - 'expiration' => $expiration, - 'time' => $now, - ] - ); - } - - if ( $value['redirect_uri'] !== $args['redirect_uri'] ) { + if ( ! empty( $value['redirect_uri'] ) && ! empty( $redirect_uri ) ) { + if ( $value['redirect_uri'] !== $redirect_uri ) { return new WP_Error( 'oauth2.tokens.authorization_code.redirect_uri.mismatch', __( 'redirect_uri does not match the one in the initial request.', 'oauth2' ), @@ -134,7 +128,9 @@ private function validate_redirect_uri( $args ) { ); } } - if ( empty( $value['redirect_uri'] ) && ! empty( $args['redirect_uri'] ) ) { + + if ( ( empty( $value['redirect_uri'] ) && ! empty( $redirect_uri ) && ! $this->client->check_redirect_uri( $redirect_uri ) ) || + ( ! empty( $value['redirect_uri'] ) && empty( $redirect_uri ) ) ) { return new WP_Error( 'oauth2.tokens.authorization_code.redirect_uri.mismatch', __( 'redirect_uri does not match the one in the initial request.', 'oauth2' ), @@ -170,9 +166,7 @@ public function validate( $args = [] ) { ); } - $redirect_uri = $this->validate_redirect_uri( [ - 'redirect_uri' => $args['redirect_uri'], - ] ); + $redirect_uri = $this->validate_redirect_uri( $args['redirect_uri'] ); if ( is_wp_error( $redirect_uri ) ) { return $redirect_uri; } diff --git a/inc/types/class-base.php b/inc/types/class-base.php index 25e2442..2dc8bcb 100644 --- a/inc/types/class-base.php +++ b/inc/types/class-base.php @@ -107,18 +107,11 @@ public function handle_authorisation() { * @return string|WP_Error Valid redirect URI on success, error otherwise. */ protected function validate_redirect_uri( Client $client, $redirect_uri = null ) { - if ( empty( $redirect_uri ) ) { + if ( ! $client->check_redirect_uri( $redirect_uri ) ) { return new WP_Error( - 'oauth2.types.authorization_code.handle_authorisation.missing_redirect_uri', - __( 'Redirect URI was required, but not found.', 'oauth2' ) + 'oauth2.types.authorization_code.handle_authorisation.invalid_redirect_uri', + __( 'Specified redirect URI is not valid for this client.', 'oauth2' ) ); - } else { - if ( ! $client->check_redirect_uri( $redirect_uri ) ) { - return new WP_Error( - 'oauth2.types.authorization_code.handle_authorisation.invalid_redirect_uri', - __( 'Specified redirect URI is not valid for this client.', 'oauth2' ) - ); - } } return $redirect_uri;