Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validating redirect_uri according to rfc6749 4.1.3 #45

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Validating redirect_uri according to rfc6749 4.1.3 #45

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a79b4ad
Validating redirect_uri according to https://tools.ietf.org/html/rfc6…
almirbi Sep 5, 2017
7516d6c
Adapt code for easier merge with pkce support branch
almirbi Sep 5, 2017
b06fe6e
Review changes; redirect_uri validation according to the standard;
almirbi Sep 16, 2017
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
5 changes: 4 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,7 @@ _book
*.epub
*.mobi
*.pdf
.idea
.idea
.vscode
vendor
composer.lock
4 changes: 2 additions & 2 deletions inc/class-client.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, $data ) {
return Authorization_Code::create( $this, $user, $data );
}

/**
Expand Down
2 changes: 1 addition & 1 deletion inc/endpoints/class-token.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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( $request );
if ( is_wp_error( $is_valid ) ) {
// Invalid request, but code itself exists, so we should delete
// (and silently ignore errors).
Expand Down
55 changes: 52 additions & 3 deletions inc/tokens/class-authorization-code.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,47 @@ public function get_expiration() {
return (int) $value['expiration'];
}

private function validate_redirect_uri( $args ) {
Copy link
Member

Choose a reason for hiding this comment

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

This should be protected instead, and should have a phpDoc block.

$value = $this->get_value();

if ( ! empty( $args['redirect_uri'] ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we invert this check instead to return early?

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'] ) {
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,
]
);
}
}
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' ),
[
'status' => WP_Http::BAD_REQUEST,
'expiration' => $expiration,
'time' => $now,
]
);
}

return true;
}

/**
* Validate the code for use.
*
Expand All @@ -129,6 +170,13 @@ public function validate( $args = [] ) {
);
}

$redirect_uri = $this->validate_redirect_uri( [
'redirect_uri' => $args['redirect_uri'],
] );
if ( is_wp_error( $redirect_uri ) ) {
return $redirect_uri;
}

return true;
}

Expand Down Expand Up @@ -183,12 +231,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 ) {
Expand Down
3 changes: 2 additions & 1 deletion inc/types/class-authorization-code.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
26 changes: 11 additions & 15 deletions inc/types/class-base.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this should always pass the $redirect_uri in?

$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.
Expand All @@ -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' )
Expand Down Expand Up @@ -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(
Expand Down