diff --git a/.phpcs.xml.dist b/.phpcs.xml.dist index ab5e57d..330b103 100644 --- a/.phpcs.xml.dist +++ b/.phpcs.xml.dist @@ -28,5 +28,6 @@ + diff --git a/README.md b/README.md index 5950059..4f64a79 100755 --- a/README.md +++ b/README.md @@ -6,6 +6,54 @@ This plugin uses the OAuth 2 protocol to allow delegated authorization; that is, This plugin only supports WordPress >= 4.8. +## Proof Key for Code Exchange (PKCE) + +The plugin supports PKCE ([RFC 7636](https://tools.ietf.org/html/rfc7636)) for +the `authorization_code` grant, as protection against authorization code +interception. To use it, add two extra parameters to the initial +authorization request: + +* `code_challenge` (required) +* `code_challenge_method` (optional, defaults to `plain`; use `S256`) + +`S256` derives the challenge from a code verifier by SHA-256 hashing it, then +base64url-encoding the digest with no padding. For example: + +``` +code_verifier = dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk +code_challenge = E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM +``` + +This is **not** the same as `base64_encode( hash( 'sha256', $verifier ) )` — +that encodes the hex digest with standard base64, which is a different, wrong +value that RFC 7636's own worked example above will catch. + +When exchanging the code for a token, pass the original `code_verifier` as an +extra parameter to the token endpoint. The server derives the challenge from +it the same way and checks it matches the one supplied at authorization time. + +A client can be marked to require PKCE from its edit screen under +**Users → Applications**. When required, only `S256` satisfies the +requirement — `plain` remains an accepted method generally, but does not +count as PKCE having been used, since it offers no protection against a +malicious app on the same device reading the authorization request. + +Filters: `oauth2.pkce.supported_methods` (accepted `code_challenge_method` +values, default `S256` and `plain`), `oauth2.pkce.required_methods` (methods +that satisfy "PKCE required", default `S256` only), and `oauth2.pkce.required` +(override whether PKCE is required for a given client). + +## CLI Commands + +### PKCE + +Generate a random code verifier and its matching code challenge, for manually +testing a PKCE flow: + +``` +wp oauth2 generate-code-challenge +``` + ## Contributors Welcome! This plugin works and is in use in several production environments, but the user experience and documentation could be substantially improved. We welcome input and contributions to make this tool better! diff --git a/inc/admin/namespace.php b/inc/admin/namespace.php index f72af1b..f5cc67e 100644 --- a/inc/admin/namespace.php +++ b/inc/admin/namespace.php @@ -174,6 +174,7 @@ function validate_parameters( $params ) { $valid['type'] = wp_kses_post( $params['type'] ); $valid['client_credentials_enabled'] = ! empty( $params['client_credentials_enabled'] ); + $valid['pkce_required'] = ! empty( $params['pkce_required'] ); // Callback is required unless this client only uses client_credentials. if ( empty( $params['callback'] ) && ! $valid['client_credentials_enabled'] ) { @@ -219,6 +220,7 @@ function handle_edit_submit( Client $consumer = null ) { 'type' => $params['type'], 'callback' => $params['callback'], 'client_credentials_enabled' => $params['client_credentials_enabled'], + 'pkce_required' => $params['pkce_required'], ], ]; @@ -233,6 +235,7 @@ function handle_edit_submit( Client $consumer = null ) { 'type' => $params['type'], 'callback' => $params['callback'], 'client_credentials_enabled' => $params['client_credentials_enabled'], + 'pkce_required' => $params['pkce_required'], ], ]; @@ -326,12 +329,21 @@ function render_edit_page() { $data[ $key ] = empty( $form_data[ $key ] ) ? '' : $form_data[ $key ]; } $data['client_credentials_enabled'] = ! empty( $form_data['client_credentials_enabled'] ); + + if ( empty( $consumer ) && empty( $form_data ) ) { + // A genuinely fresh "Add Application" page, not a failed submission + // redisplay: default new clients to requiring PKCE. + $data['pkce_required'] = true; + } else { + $data['pkce_required'] = ! empty( $form_data['pkce_required'] ); + } } else { $data['name'] = $consumer->get_name(); $data['description'] = $consumer->get_description( true ); $data['type'] = $consumer->get_type(); $data['callback'] = $consumer->get_redirect_uris(); $data['client_credentials_enabled'] = $consumer->is_client_credentials_enabled(); + $data['pkce_required'] = $consumer->is_pkce_required(); if ( is_array( $data['callback'] ) ) { $data['callback'] = implode( ',', $data['callback'] ); @@ -457,6 +469,26 @@ function render_edit_page() {

+ + + + + + +

+ +

+ + get_post_id(), static::CLIENT_CREDENTIALS_ENABLED_KEY, true ); } + /** + * Check whether this client requires PKCE for the authorization_code grant. + * + * @return bool True if PKCE is required, false otherwise. + */ + public function is_pkce_required() { + $required = (bool) get_post_meta( $this->get_post_id(), static::PKCE_REQUIRED_KEY, true ); + + /** + * Filter whether PKCE is required for a client. + * + * @param bool $required True if PKCE is required for this client. + * @param Client $client Client being checked. + */ + return apply_filters( 'oauth2.pkce.required', $required, $this ); + } + /** * Get registered URI for the client. * @@ -249,11 +267,13 @@ public function check_redirect_uri( $uri ) { /** * @param WP_User $user + * @param array $data Optional extra data for the code, e.g. PKCE `code_challenge` + * and `code_challenge_method`. See Authorization_Code::create(). * * @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, array $data = [] ) { + return Authorization_Code::create( $this, $user, $data ); } /** @@ -361,6 +381,7 @@ public static function create( $data ) { static::TYPE_KEY => $data['meta']['type'], static::CLIENT_SECRET_KEY => wp_generate_password( static::CLIENT_SECRET_LENGTH, false ), static::CLIENT_CREDENTIALS_ENABLED_KEY => ! empty( $data['meta']['client_credentials_enabled'] ) ? '1' : '', + static::PKCE_REQUIRED_KEY => ! empty( $data['meta']['pkce_required'] ) ? '1' : '', ]; foreach ( $meta as $key => $value ) { @@ -394,14 +415,28 @@ public function update( $data ) { return $post_id; } - $meta = [ - static::REDIRECT_URI_KEY => $data['meta']['callback'], - static::TYPE_KEY => $data['meta']['type'], - static::CLIENT_CREDENTIALS_ENABLED_KEY => ! empty( $data['meta']['client_credentials_enabled'] ) ? '1' : '', + // Map of meta key => data key. Built this way, rather than as a fixed + // array of values, so that a caller omitting a key leaves the existing + // meta value untouched instead of silently resetting it to empty/false. + $fields = [ + static::REDIRECT_URI_KEY => 'callback', + static::TYPE_KEY => 'type', + static::CLIENT_CREDENTIALS_ENABLED_KEY => 'client_credentials_enabled', + static::PKCE_REQUIRED_KEY => 'pkce_required', ]; + $boolean_fields = [ static::CLIENT_CREDENTIALS_ENABLED_KEY, static::PKCE_REQUIRED_KEY ]; - foreach ( $meta as $key => $value ) { - update_post_meta( $post_id, wp_slash( $key ), wp_slash( $value ) ); + foreach ( $fields as $meta_key => $data_key ) { + if ( ! array_key_exists( $data_key, $data['meta'] ) ) { + continue; + } + + $value = $data['meta'][ $data_key ]; + if ( in_array( $meta_key, $boolean_fields, true ) ) { + $value = ! empty( $value ) ? '1' : ''; + } + + update_post_meta( $post_id, wp_slash( $meta_key ), wp_slash( $value ) ); } $post = get_post( $post_id ); diff --git a/inc/class-pkce.php b/inc/class-pkce.php new file mode 100644 index 0000000..f13cf25 --- /dev/null +++ b/inc/class-pkce.php @@ -0,0 +1,155 @@ + 'string', 'validate_callback' => 'rest_validate_request_arg', ], + 'code_verifier' => [ + 'required' => false, + 'type' => 'string', + 'validate_callback' => 'rest_validate_request_arg', + ], ], ] ); @@ -112,7 +117,14 @@ public function exchange_token( WP_REST_Request $request ) { return $auth_code; } - $is_valid = $auth_code->validate(); + // Read from the body only, not get_param(), which also reads $_GET on a + // POST route — a verifier belongs in the body, not the URL, since a URL + // is far more likely to end up in access logs or a Referer header. + $body_params = $request->get_body_params(); + $json_params = (array) $request->get_json_params(); + $code_verifier = $body_params['code_verifier'] ?? $json_params['code_verifier'] ?? null; + + $is_valid = $auth_code->validate( [ 'code_verifier' => $code_verifier ] ); if ( is_wp_error( $is_valid ) ) { // Invalid request, but code itself exists, so we should delete // (and silently ignore errors). diff --git a/inc/namespace.php b/inc/namespace.php index ea681f1..bc0a9b0 100644 --- a/inc/namespace.php +++ b/inc/namespace.php @@ -8,6 +8,7 @@ namespace WP\OAuth2; use WP\OAuth2\Types\Type; +use WP_CLI; use WP_REST_Response; function bootstrap() { @@ -27,6 +28,11 @@ function bootstrap() { add_action( 'init', __NAMESPACE__ . '\\rest_oauth2_load_authorize_page' ); add_action( 'admin_menu', __NAMESPACE__ . '\\Admin\\register' ); Admin\Profile\bootstrap(); + + // WP-CLI. + if ( class_exists( __NAMESPACE__ . '\\Utilities\\Command' ) ) { + WP_CLI::add_command( 'oauth2', __NAMESPACE__ . '\\Utilities\\Command' ); + } } /** @@ -97,11 +103,12 @@ function register_in_index( WP_REST_Response $response ) { $data = $response->get_data(); $data['authentication']['oauth2'] = [ - 'endpoints' => [ + 'endpoints' => [ 'authorization' => get_authorization_url(), 'token' => get_token_url(), ], - 'grant_types' => array_keys( get_grant_types() ), + 'grant_types' => array_keys( get_grant_types() ), + 'code_challenge_methods_supported' => PKCE::supported_methods(), ]; $response->set_data( $data ); diff --git a/inc/tokens/class-authorization-code.php b/inc/tokens/class-authorization-code.php index 111ee15..5c920e2 100644 --- a/inc/tokens/class-authorization-code.php +++ b/inc/tokens/class-authorization-code.php @@ -10,6 +10,7 @@ use WP_Error; use WP_Http; use WP\OAuth2\Client; +use WP\OAuth2\PKCE; use WP_User; /** @@ -113,14 +114,49 @@ public function get_expiration() { return (int) $value['expiration']; } + /** + * Get the PKCE code challenge stored against this code, if any. + * + * @return string|null Code challenge, or null if this code was not issued with one. + */ + public function get_code_challenge() { + $value = $this->get_value(); + if ( empty( $value ) || empty( $value['code_challenge'] ) ) { + return null; + } + + return $value['code_challenge']; + } + + /** + * Get the PKCE code challenge method stored against this code, if any. + * + * @return string|null Code challenge method, or null if this code was not issued with a challenge. + */ + public function get_code_challenge_method() { + $value = $this->get_value(); + if ( empty( $value ) || empty( $value['code_challenge_method'] ) ) { + return null; + } + + return $value['code_challenge_method']; + } + /** * Validate the code for use. * + * @param array $args Other request arguments to validate. { + * @var string $code_verifier PKCE code verifier, if the client is using PKCE. + * } * @return bool|WP_Error True if valid, error describing problem otherwise. */ - public function validate() { + public function validate( $args = [] ) { $expiration = $this->get_expiration(); - $now = time(); + if ( is_wp_error( $expiration ) ) { + return $expiration; + } + + $now = time(); if ( $expiration <= $now ) { return new WP_Error( 'oauth2.tokens.authorization_code.validate.expired', @@ -133,6 +169,86 @@ public function validate() { ); } + $verifier_check = $this->validate_code_verifier( $args ); + if ( is_wp_error( $verifier_check ) ) { + return $verifier_check; + } + + return true; + } + + /** + * Validate a PKCE code verifier against the stored code challenge. + * + * @param array $args Request arguments, as passed to validate(). + * @return true|WP_Error True if valid (including when this code has no + * PKCE challenge and none was supplied), error otherwise. + */ + protected function validate_code_verifier( $args ) { + $challenge = $this->get_code_challenge(); + $verifier = isset( $args['code_verifier'] ) && is_string( $args['code_verifier'] ) ? $args['code_verifier'] : null; + + if ( null === $challenge ) { + if ( null === $verifier ) { + return true; + } + + /** + * Filter whether a code_verifier is accepted for a code that has no stored code_challenge. + * + * A verifier arriving for a non-PKCE code is unexpected: either the + * client is misconfigured, or the code was obtained some other way. + * Rejecting it by default is the safer choice. + * + * @param bool $reject True to reject the request, the default. + * @param string $verifier Code verifier that was supplied. + */ + if ( apply_filters( 'oauth2.pkce.reject_unexpected_verifier', true, $verifier ) ) { + return new WP_Error( + 'oauth2.tokens.authorization_code.validate.unexpected_verifier', + __( 'This authorization code was not issued with a PKCE challenge.', 'oauth2' ), + [ + 'status' => WP_Http::BAD_REQUEST, + 'error' => 'invalid_grant', + ] + ); + } + + return true; + } + + $method = $this->get_code_challenge_method(); + if ( null === $method ) { + // A stored challenge with no stored method means corrupted data; fail closed. + return new WP_Error( + 'oauth2.tokens.authorization_code.validate.missing_challenge_method', + __( 'Authorization code data is not valid.', 'oauth2' ), + [ 'status' => WP_Http::BAD_REQUEST ] + ); + } + + if ( null === $verifier ) { + return new WP_Error( + 'oauth2.tokens.authorization_code.validate.missing_verifier', + __( 'This authorization code requires a code_verifier to be exchanged.', 'oauth2' ), + [ + 'status' => WP_Http::BAD_REQUEST, + 'error' => 'invalid_grant', + ] + ); + } + + if ( ! PKCE::is_valid_verifier( $verifier ) || ! PKCE::verify( $verifier, $challenge, $method ) ) { + return new WP_Error( + 'oauth2.tokens.authorization_code.validate.invalid_verifier', + __( 'The supplied code_verifier does not match the code_challenge for this authorization code.', 'oauth2' ), + [ + 'status' => WP_Http::BAD_REQUEST, + 'error' => 'invalid_grant', + ] + ); + } + return true; } @@ -184,17 +300,29 @@ public static function get_by_code( Client $client, $code ) { * * @param Client $client * @param WP_User $user + * @param array $data Optional extra data for this code. Only `code_challenge` + * and `code_challenge_method` are recognised; anything + * else is ignored, and neither `user` nor `expiration` + * can be overridden this way. * * @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, array $data = [] ) { $code = wp_generate_password( static::KEY_LENGTH, false ); $meta_key = static::KEY_PREFIX . $code; - $data = [ + $value = [ 'user' => (int) $user->ID, 'expiration' => time() + static::MAX_AGE, ]; - $result = add_post_meta( $client->get_post_id(), wp_slash( $meta_key ), wp_slash( $data ), true ); + + // Only store the challenge when one was actually supplied, so a + // non-PKCE code's meta shape is unchanged from before this existed. + if ( ! empty( $data['code_challenge'] ) ) { + $value['code_challenge'] = $data['code_challenge']; + $value['code_challenge_method'] = ! empty( $data['code_challenge_method'] ) ? $data['code_challenge_method'] : PKCE::METHOD_PLAIN; + } + + $result = add_post_meta( $client->get_post_id(), wp_slash( $meta_key ), wp_slash( $value ), true ); if ( ! $result ) { return new WP_Error( 'oauth2.tokens.authorization_code.create.could_not_create', diff --git a/inc/types/class-authorization-code.php b/inc/types/class-authorization-code.php index dae2470..93dac36 100644 --- a/inc/types/class-authorization-code.php +++ b/inc/types/class-authorization-code.php @@ -9,6 +9,7 @@ use WP_Error; use WP\OAuth2\Client; +use WP\OAuth2\PKCE; class Authorization_Code extends Base { /** @@ -22,6 +23,79 @@ public function get_response_type_code() { return 'code'; } + /** + * Gather and validate PKCE parameters from the authorisation request. + * + * @param Client $client Client being authorised. + * @param array $request Unslashed request parameters, from $_GET. + * @return array|WP_Error `code_challenge`/`code_challenge_method` to carry + * through to the minted code, or an error. + */ + protected function gather_extra_params( Client $client, array $request ) { + $code_challenge = isset( $request['code_challenge'] ) && is_string( $request['code_challenge'] ) ? $request['code_challenge'] : null; + $code_challenge_method = isset( $request['code_challenge_method'] ) && is_string( $request['code_challenge_method'] ) ? $request['code_challenge_method'] : null; + + if ( null === $code_challenge ) { + if ( $client->is_pkce_required() ) { + return new WP_Error( + 'oauth2.types.authorization_code.gather_extra_params.pkce_required', + __( 'This client requires a code_challenge for the authorization_code grant.', 'oauth2' ), + [ 'error' => 'invalid_request' ] + ); + } + + return []; + } + + // The method defaults to 'plain' when omitted, per RFC 7636 section 4.3. + if ( null === $code_challenge_method ) { + $code_challenge_method = PKCE::METHOD_PLAIN; + } + + if ( ! in_array( $code_challenge_method, PKCE::supported_methods(), true ) ) { + return new WP_Error( + 'oauth2.types.authorization_code.gather_extra_params.unsupported_method', + __( 'Unsupported code_challenge_method. Note that this value is case-sensitive; use S256.', 'oauth2' ), + [ 'error' => 'invalid_request' ] + ); + } + + if ( ! PKCE::is_valid_challenge( $code_challenge, $code_challenge_method ) ) { + return new WP_Error( + 'oauth2.types.authorization_code.gather_extra_params.invalid_challenge', + __( 'Invalid code_challenge.', 'oauth2' ), + [ 'error' => 'invalid_request' ] + ); + } + + if ( $client->is_pkce_required() ) { + /** + * Filter the code_challenge_method values that satisfy a client's PKCE requirement. + * + * Defaults to S256 only. `plain` offers no protection against a + * malicious app on the same device reading the authorization + * request, which is exactly the threat PKCE exists to mitigate + * for public clients (RFC 9700 section 2.1.1), so it does not + * satisfy a requirement even though it remains an accepted method. + * + * @param string[] $methods Methods that satisfy "PKCE required". + */ + $required_methods = apply_filters( 'oauth2.pkce.required_methods', [ PKCE::METHOD_S256 ] ); + if ( ! in_array( $code_challenge_method, $required_methods, true ) ) { + return new WP_Error( + 'oauth2.types.authorization_code.gather_extra_params.weak_method', + __( 'This client requires PKCE with the S256 code_challenge_method.', 'oauth2' ), + [ 'error' => 'invalid_request' ] + ); + } + } + + return [ + 'code_challenge' => $code_challenge, + 'code_challenge_method' => $code_challenge_method, + ]; + } + /** * Handles the authorization. * @@ -38,11 +112,28 @@ 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, $data ); if ( is_wp_error( $code ) ) { return $code; } + // Defends against a third-party Client subclass overriding + // generate_authorization_code() with the pre-PKCE arity, + // which would silently mint a code with no challenge. + if ( ! empty( $data['code_challenge'] ) && $code->get_code_challenge() !== $data['code_challenge'] ) { + $code->delete(); + // phpcs:ignore WordPress.Security.SafeRedirect -- Intentionally external redirect, secured via client registration. + wp_redirect( + $this->get_error_redirect_url( + $redirect_uri, + 'server_error', + __( 'Could not persist the PKCE challenge for this authorization code.', 'oauth2' ), + ! empty( $data['state'] ) ? $data['state'] : null + ) + ); + exit; + } + $redirect_args = [ 'code' => $code->get_code(), ]; diff --git a/inc/types/class-base.php b/inc/types/class-base.php index d04031f..ebb5872 100644 --- a/inc/types/class-base.php +++ b/inc/types/class-base.php @@ -21,6 +21,8 @@ abstract class Base implements Type { * @var string $redirect_uri Specified redirection URI. * @var string $scope Requested scope. * @var string $state State parameter from the client. + * @var string $code_challenge PKCE code challenge, if supplied. Grant-type specific. + * @var string $code_challenge_method PKCE code challenge method, if supplied. Grant-type specific. * } * @return WP_Error|void Method should output form and exit, or return encountered error. */ @@ -65,6 +67,19 @@ public function handle_authorisation() { return $redirect_uri; } + // Gather and validate any grant-type-specific parameters (e.g. PKCE). + // This runs after the redirect URI is known to be valid, and before the + // login redirect, so a malformed request fails fast and can be reported + // back to the client rather than dying with an HTML error page. + $extra_params = $this->gather_extra_params( $client, wp_unslash( $_GET ) ); + if ( is_wp_error( $extra_params ) ) { + $error_data = $extra_params->get_error_data(); + $error_code = ! empty( $error_data['error'] ) ? $error_data['error'] : 'invalid_request'; + // phpcs:ignore WordPress.Security.SafeRedirect -- Intentionally external redirect, secured via client registration. + wp_redirect( $this->get_error_redirect_url( $redirect_uri, $error_code, $extra_params->get_error_message(), $state ) ); + exit; + } + // Valid parameters, ensure the user is logged in. if ( ! is_user_logged_in() ) { $redirect = ''; @@ -105,10 +120,49 @@ public function handle_authorisation() { $submit = sanitize_text_field( wp_unslash( $_POST['wp-submit'] ) ); - $data = compact( 'redirect_uri', 'scope', 'state' ); + $data = array_merge( compact( 'redirect_uri', 'scope', 'state' ), $extra_params ); return $this->handle_authorization_submission( $submit, $client, $data ); } + /** + * Gather and validate any grant-type-specific parameters from the request. + * + * Runs on both the initial GET and the consent-form POST, since the + * authorisation form posts back to the original request URI, so $_GET is + * populated on both passes. The default implementation adds nothing; + * override to add grant-type-specific request parameters (e.g. PKCE's + * code_challenge for the authorization_code grant). + * + * @param Client $client Client being authorised. + * @param array $request Unslashed request parameters, from $_GET. + * @return array|WP_Error Extra data to merge into the $data passed to + * handle_authorization_submission(), or an error. + */ + protected function gather_extra_params( Client $client, array $request ) { + return []; + } + + /** + * Build a URL for reporting an authorisation error back to the client. + * + * @param string $redirect_uri Validated redirect URI for the client. + * @param string $error Error code, e.g. `invalid_request`. + * @param string $description Human-readable error description. + * @param string|null $state State parameter from the original request, if any. + * @return string URL to redirect the client to. + */ + protected function get_error_redirect_url( $redirect_uri, $error, $description, $state = null ) { + $args = [ + 'error' => $error, + 'error_description' => $description, + ]; + if ( ! empty( $state ) ) { + $args['state'] = $state; + } + + return add_query_arg( urlencode_deep( $args ), $redirect_uri ); + } + /** * Validate the supplied redirect URI. * @@ -169,7 +223,10 @@ protected function get_nonce_action( Client $client ) { * @param array $redirect_args Redirect args. * @param boolean $authorized True if authorized, false otherwise. * @param Client $client Client being authorised. - * @param array $data Data for the request. + * @param array $data Data for the request. May include PKCE's + * `code_challenge`/`code_challenge_method` for the + * authorization_code grant; never `code_verifier`, + * which is only ever supplied at the token endpoint. */ protected function filter_redirect_args( $redirect_args, $authorized, Client $client, $data ) { if ( ! $authorized ) { @@ -178,7 +235,7 @@ protected function filter_redirect_args( $redirect_args, $authorized, Client $cl * * @param array $redirect_args Redirect args. * @param Client $client Client being authorised. - * @param array $data Data for the request. + * @param array $data Data for the request. See filter_redirect_args(). */ return apply_filters( 'oauth2.redirect_args.cancelled', $redirect_args, $client, $data ); } @@ -188,7 +245,7 @@ protected function filter_redirect_args( $redirect_args, $authorized, Client $cl * * @param array $redirect_args Redirect args. * @param Client $client Client being authorised. - * @param array $data Data for the request. + * @param array $data Data for the request. See filter_redirect_args(). */ return apply_filters( 'oauth2.redirect_args.authorized', $redirect_args, $client, $data ); } diff --git a/inc/types/class-implicit.php b/inc/types/class-implicit.php index 35151f5..d727a62 100644 --- a/inc/types/class-implicit.php +++ b/inc/types/class-implicit.php @@ -22,6 +22,30 @@ public function get_response_type_code() { return 'token'; } + /** + * Refuse the implicit grant for clients that require PKCE. + * + * The implicit grant issues a token directly and mints no authorization + * code, so there is nothing for a PKCE challenge to bind to. Silently + * accepting a code_challenge here would give a false sense of protection, + * so a client marked as requiring PKCE cannot use this grant at all. + * + * @param Client $client Client being authorised. + * @param array $request Unslashed request parameters, from $_GET. + * @return array|WP_Error + */ + protected function gather_extra_params( Client $client, array $request ) { + if ( $client->is_pkce_required() ) { + return new WP_Error( + 'oauth2.types.implicit.gather_extra_params.pkce_required', + __( 'This client requires PKCE, which the implicit grant cannot support. Use the authorization_code grant instead.', 'oauth2' ), + [ 'error' => 'unauthorized_client' ] + ); + } + + return []; + } + /** * Handles the authorization. * @@ -78,4 +102,28 @@ protected function handle_authorization_submission( $submit, Client $client, $da wp_safe_redirect( $generated_redirect ); exit; } + + /** + * Build a URL for reporting an authorisation error back to the client. + * + * Access tokens are returned in the URL fragment for this grant, so + * errors are reported the same way, per RFC 6749 section 4.2.2.1. + * + * @param string $redirect_uri Validated redirect URI for the client. + * @param string $error Error code, e.g. `unauthorized_client`. + * @param string $description Human-readable error description. + * @param string|null $state State parameter from the original request, if any. + * @return string URL to redirect the client to. + */ + protected function get_error_redirect_url( $redirect_uri, $error, $description, $state = null ) { + $args = [ + 'error' => $error, + 'error_description' => $description, + ]; + if ( ! empty( $state ) ) { + $args['state'] = $state; + } + + return $redirect_uri . '#' . build_query( $args ); + } } diff --git a/inc/utilities/class-command.php b/inc/utilities/class-command.php new file mode 100644 index 0000000..15923aa --- /dev/null +++ b/inc/utilities/class-command.php @@ -0,0 +1,93 @@ +] + * : Use this code verifier instead of generating a random one. + * + * [--length=] + * : Length of the randomly generated code verifier. Ignored if a verifier is given. + * --- + * default: 64 + * --- + * + * [--method=] + * : Code challenge method to derive the challenge with. + * --- + * default: S256 + * options: + * - S256 + * - plain + * --- + * + * [--format=] + * : Render output in a particular format. + * --- + * default: table + * options: + * - table + * - json + * - csv + * - yaml + * --- + * + * ## EXAMPLES + * + * wp oauth2 generate-code-challenge + * wp oauth2 generate-code-challenge --method=plain + * + * @alias generate-code-challenge + * + * @param array $args Positional arguments. + * @param array $assoc_args Associative arguments. + */ + public function generate_code_challenge( $args, $assoc_args ) { + $method = $assoc_args['method']; + + if ( ! empty( $args[0] ) ) { + $verifier = $args[0]; + + if ( ! PKCE::is_valid_verifier( $verifier ) ) { + WP_CLI::error( 'The supplied verifier must be 43-128 characters from [A-Za-z0-9-._~].' ); + } + } else { + $length = (int) $assoc_args['length']; + if ( $length < PKCE::VERIFIER_MIN_LENGTH || $length > PKCE::VERIFIER_MAX_LENGTH ) { + WP_CLI::error( sprintf( 'Length must be between %d and %d.', PKCE::VERIFIER_MIN_LENGTH, PKCE::VERIFIER_MAX_LENGTH ) ); + } + + $verifier = PKCE::generate_verifier( $length ); + } + + $challenge = PKCE::derive_challenge( $verifier, $method ); + + $items = [ + [ + 'code_verifier' => $verifier, + 'code_challenge' => $challenge, + 'code_challenge_method' => $method, + ], + ]; + + Utils\format_items( $assoc_args['format'], $items, [ 'code_verifier', 'code_challenge', 'code_challenge_method' ] ); + } +} diff --git a/plugin.php b/plugin.php index 9b8ae34..2af0681 100644 --- a/plugin.php +++ b/plugin.php @@ -20,7 +20,7 @@ * Text Domain: oauth2 * Domain Path: /languages * Requires at least: 4.8 - * Requires PHP: 5.6 + * Requires PHP: 7.4 */ namespace WP\OAuth2; @@ -35,6 +35,7 @@ require __DIR__ . '/inc/class-client.php'; require __DIR__ . '/inc/class-personalclient.php'; require __DIR__ . '/inc/class-scopes.php'; +require __DIR__ . '/inc/class-pkce.php'; require __DIR__ . '/inc/authentication/namespace.php'; require __DIR__ . '/inc/endpoints/namespace.php'; require __DIR__ . '/inc/endpoints/class-authorization.php'; @@ -51,4 +52,8 @@ require __DIR__ . '/inc/admin/profile/namespace.php'; require __DIR__ . '/inc/admin/profile/personaltokens/namespace.php'; +if ( defined( 'WP_CLI' ) && WP_CLI ) { + require __DIR__ . '/inc/utilities/class-command.php'; +} + bootstrap(); diff --git a/tests/class-test-case.php b/tests/class-test-case.php index 9035d85..76facb2 100644 --- a/tests/class-test-case.php +++ b/tests/class-test-case.php @@ -8,6 +8,7 @@ namespace WP\OAuth2\Tests; use WP\OAuth2\Client; +use WP\OAuth2\PKCE; use WP_UnitTestCase; /** @@ -30,8 +31,9 @@ protected function create_client( array $meta_overrides = [], $name = 'Test Clie 'meta' => array_merge( [ 'callback' => 'https://example.com/callback', - 'type' => 'web', + 'type' => 'public', 'client_credentials_enabled' => false, + 'pkce_required' => false, ], $meta_overrides ), @@ -43,4 +45,25 @@ protected function create_client( array $meta_overrides = [], $name = 'Test Clie return $client; } + + /** + * Generate a matching PKCE verifier/challenge pair for use in tests. + * + * @param string $method Challenge method, `S256` or `plain`. Default `S256`. + * + * @return array { + * @var string $code_verifier Code verifier. + * @var string $code_challenge Derived code challenge. + * @var string $code_challenge_method Method used to derive the challenge. + * } + */ + protected function make_pkce_pair( $method = PKCE::METHOD_S256 ) { + $verifier = PKCE::generate_verifier(); + + return [ + 'code_verifier' => $verifier, + 'code_challenge' => PKCE::derive_challenge( $verifier, $method ), + 'code_challenge_method' => $method, + ]; + } } diff --git a/tests/test-authorization-code.php b/tests/test-authorization-code.php index f7a2a40..ab6223a 100644 --- a/tests/test-authorization-code.php +++ b/tests/test-authorization-code.php @@ -105,4 +105,176 @@ public function test_code_is_valid_only_for_its_client() { $result = Authorization_Code::get_by_code( $other_client, $code->get_code() ); $this->assertWPError( $result ); } + + // RFC 7636 Appendix B worked example. + const RFC_VERIFIER = 'dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk'; + const RFC_CHALLENGE = 'E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM'; + + public function test_create_without_pkce_data_stores_no_challenge() { + $code = Authorization_Code::create( $this->client, $this->user ); + $this->assertNull( $code->get_code_challenge() ); + $this->assertNull( $code->get_code_challenge_method() ); + } + + public function test_create_with_pkce_data_stores_challenge_and_method() { + $code = Authorization_Code::create( + $this->client, + $this->user, + [ + 'code_challenge' => static::RFC_CHALLENGE, + 'code_challenge_method' => 'S256', + ] + ); + + $this->assertSame( static::RFC_CHALLENGE, $code->get_code_challenge() ); + $this->assertSame( 'S256', $code->get_code_challenge_method() ); + } + + public function test_create_defaults_challenge_method_to_plain_when_omitted() { + $code = Authorization_Code::create( + $this->client, + $this->user, + [ 'code_challenge' => static::RFC_VERIFIER ] + ); + + $this->assertSame( 'plain', $code->get_code_challenge_method() ); + } + + public function test_create_data_cannot_override_user_or_expiration() { + $code = Authorization_Code::create( + $this->client, + $this->user, + [ + 'user' => 999999, + 'expiration' => 1, + ] + ); + + $user = $code->get_user(); + $this->assertInstanceOf( WP_User::class, $user ); + $this->assertEquals( $this->user->ID, $user->ID ); + $this->assertGreaterThan( time(), $code->get_expiration() ); + } + + public function test_validate_with_no_args_still_passes_for_non_pkce_code() { + // Back-compat guarantee: existing callers pass no args at all. + $code = Authorization_Code::create( $this->client, $this->user ); + $this->assertTrue( $code->validate() ); + } + + public function test_validate_passes_with_correct_s256_verifier() { + $code = Authorization_Code::create( + $this->client, + $this->user, + [ + 'code_challenge' => static::RFC_CHALLENGE, + 'code_challenge_method' => 'S256', + ] + ); + + $this->assertTrue( $code->validate( [ 'code_verifier' => static::RFC_VERIFIER ] ) ); + } + + public function test_validate_passes_with_correct_plain_verifier() { + $code = Authorization_Code::create( + $this->client, + $this->user, + [ + 'code_challenge' => static::RFC_VERIFIER, + 'code_challenge_method' => 'plain', + ] + ); + + $this->assertTrue( $code->validate( [ 'code_verifier' => static::RFC_VERIFIER ] ) ); + } + + public function test_validate_fails_with_wrong_verifier() { + $code = Authorization_Code::create( + $this->client, + $this->user, + [ + 'code_challenge' => static::RFC_CHALLENGE, + 'code_challenge_method' => 'S256', + ] + ); + + $result = $code->validate( [ 'code_verifier' => 'wrong-verifier-wrong-verifier-wrong-verifier' ] ); + $this->assertWPError( $result ); + $this->assertSame( 'invalid_grant', $result->get_error_data()['error'] ); + } + + public function test_validate_fails_with_missing_verifier() { + $code = Authorization_Code::create( + $this->client, + $this->user, + [ + 'code_challenge' => static::RFC_CHALLENGE, + 'code_challenge_method' => 'S256', + ] + ); + + $result = $code->validate(); + $this->assertWPError( $result ); + $this->assertEquals( 'oauth2.tokens.authorization_code.validate.missing_verifier', $result->get_error_code() ); + } + + public function test_validate_fails_with_malformed_verifier() { + $code = Authorization_Code::create( + $this->client, + $this->user, + [ + 'code_challenge' => static::RFC_CHALLENGE, + 'code_challenge_method' => 'S256', + ] + ); + + $result = $code->validate( [ 'code_verifier' => 'too-short' ] ); + $this->assertWPError( $result ); + } + + public function test_validate_rejects_verifier_for_code_with_no_stored_challenge() { + $code = Authorization_Code::create( $this->client, $this->user ); + $result = $code->validate( [ 'code_verifier' => static::RFC_VERIFIER ] ); + + $this->assertWPError( $result ); + $this->assertEquals( 'oauth2.tokens.authorization_code.validate.unexpected_verifier', $result->get_error_code() ); + } + + public function test_validate_allows_unexpected_verifier_when_filtered() { + $code = Authorization_Code::create( $this->client, $this->user ); + + $filter = '__return_false'; + add_filter( 'oauth2.pkce.reject_unexpected_verifier', $filter ); + $result = $code->validate( [ 'code_verifier' => static::RFC_VERIFIER ] ); + remove_filter( 'oauth2.pkce.reject_unexpected_verifier', $filter ); + + $this->assertTrue( $result ); + } + + public function test_validate_fails_closed_when_stored_method_is_missing() { + $code = Authorization_Code::create( $this->client, $this->user ); + $meta_key = Authorization_Code::KEY_PREFIX . $code->get_code(); + + // Simulate corrupted meta: a challenge with no method. + $value = get_post_meta( $this->client->get_post_id(), $meta_key, true ); + $value['code_challenge'] = static::RFC_CHALLENGE; + update_post_meta( $this->client->get_post_id(), $meta_key, $value ); + + $result = $code->validate( [ 'code_verifier' => static::RFC_VERIFIER ] ); + $this->assertWPError( $result ); + $this->assertEquals( 'oauth2.tokens.authorization_code.validate.missing_challenge_method', $result->get_error_code() ); + } + + public function test_validate_with_malformed_meta_does_not_pass_expiry_check() { + $code = Authorization_Code::create( $this->client, $this->user ); + $meta_key = Authorization_Code::KEY_PREFIX . $code->get_code(); + + // Corrupt the expiration entirely; get_expiration() now returns a WP_Error. + $value = get_post_meta( $this->client->get_post_id(), $meta_key, true ); + unset( $value['expiration'] ); + update_post_meta( $this->client->get_post_id(), $meta_key, $value ); + + $result = $code->validate(); + $this->assertWPError( $result ); + } } diff --git a/tests/test-client.php b/tests/test-client.php index f63fb51..cadebb0 100644 --- a/tests/test-client.php +++ b/tests/test-client.php @@ -51,7 +51,7 @@ public function test_create_stores_redirect_uri() { } public function test_create_stores_type() { - $this->assertEquals( 'web', $this->client->get_type() ); + $this->assertEquals( 'public', $this->client->get_type() ); } public function test_create_stores_secret() { @@ -217,4 +217,85 @@ public function test_check_redirect_uri_rejects_different_path() { public function test_check_redirect_uri_rejects_unregistered() { $this->assertFalse( $this->client->check_redirect_uri( 'https://evil.com/steal' ) ); } + + public function test_pkce_required_defaults_to_false() { + $this->assertFalse( $this->client->is_pkce_required() ); + } + + public function test_pkce_required_true_when_set() { + $client = $this->create_client( [ 'pkce_required' => true ] ); + $this->assertTrue( $client->is_pkce_required() ); + } + + public function test_pkce_required_filter_can_force_true() { + $filter = '__return_true'; + add_filter( 'oauth2.pkce.required', $filter ); + $this->assertTrue( $this->client->is_pkce_required() ); + remove_filter( 'oauth2.pkce.required', $filter ); + } + + public function test_update_omitting_pkce_required_preserves_existing_value() { + $client = $this->create_client( [ 'pkce_required' => true ] ); + + $client->update( [ + 'name' => 'Updated Name', + 'description' => 'Updated description.', + 'meta' => [ + 'callback' => 'https://example.com/callback', + 'type' => 'public', + ], + ] ); + + $this->assertTrue( $client->is_pkce_required() ); + } + + public function test_update_omitting_client_credentials_enabled_preserves_existing_value() { + $client = $this->create_client( [ 'client_credentials_enabled' => true ] ); + + $client->update( [ + 'name' => 'Updated Name', + 'description' => 'Updated description.', + 'meta' => [ + 'callback' => 'https://example.com/callback', + 'type' => 'public', + ], + ] ); + + $this->assertTrue( $client->is_client_credentials_enabled() ); + } + + public function test_update_can_still_explicitly_change_pkce_required() { + $client = $this->create_client( [ 'pkce_required' => true ] ); + + $client->update( [ + 'name' => 'Updated Name', + 'description' => 'Updated description.', + 'meta' => [ + 'callback' => 'https://example.com/callback', + 'type' => 'public', + 'pkce_required' => false, + ], + ] ); + + $this->assertFalse( $client->is_pkce_required() ); + } + + public function test_generate_authorization_code_without_data_still_works() { + $user = $this->factory->user->create_and_get(); + $code = $this->client->generate_authorization_code( $user ); + $this->assertNull( $code->get_code_challenge() ); + } + + public function test_generate_authorization_code_stores_pkce_challenge() { + $user = $this->factory->user->create_and_get(); + $pair = $this->make_pkce_pair(); + + $code = $this->client->generate_authorization_code( $user, [ + 'code_challenge' => $pair['code_challenge'], + 'code_challenge_method' => $pair['code_challenge_method'], + ] ); + + $this->assertSame( $pair['code_challenge'], $code->get_code_challenge() ); + $this->assertSame( $pair['code_challenge_method'], $code->get_code_challenge_method() ); + } } diff --git a/tests/test-namespace.php b/tests/test-namespace.php index 340156c..54f8138 100644 --- a/tests/test-namespace.php +++ b/tests/test-namespace.php @@ -12,6 +12,7 @@ use WP\OAuth2\Client; use WP\OAuth2\ClientInterface; use WP\OAuth2\PersonalClient; +use WP\OAuth2\PKCE; use WP\OAuth2\Types\Type; use WP_REST_Response; use WP_REST_Server; @@ -118,4 +119,30 @@ public function test_register_in_index_lists_grant_types() { $this->assertArrayHasKey( 'grant_types', $data['authentication']['oauth2'] ); $this->assertContains( 'authorization_code', $data['authentication']['oauth2']['grant_types'] ); } + + public function test_register_in_index_lists_pkce_methods() { + $response = new WP_REST_Response( [] ); + $response = register_in_index( $response ); + + $data = $response->get_data(); + $this->assertSame( + [ PKCE::METHOD_S256, PKCE::METHOD_PLAIN ], + $data['authentication']['oauth2']['code_challenge_methods_supported'] + ); + } + + public function test_register_in_index_tracks_pkce_methods_filter() { + $filter = function () { + return [ PKCE::METHOD_S256 ]; + }; + add_filter( 'oauth2.pkce.supported_methods', $filter ); + + $response = new WP_REST_Response( [] ); + $response = register_in_index( $response ); + $data = $response->get_data(); + + remove_filter( 'oauth2.pkce.supported_methods', $filter ); + + $this->assertSame( [ PKCE::METHOD_S256 ], $data['authentication']['oauth2']['code_challenge_methods_supported'] ); + } } diff --git a/tests/test-pkce.php b/tests/test-pkce.php new file mode 100644 index 0000000..9b6b113 --- /dev/null +++ b/tests/test-pkce.php @@ -0,0 +1,160 @@ +assertSame( static::RFC_CHALLENGE, PKCE::derive_challenge( static::RFC_VERIFIER, PKCE::METHOD_S256 ) ); + } + + public function test_s256_challenge_is_unpadded_base64url() { + $challenge = PKCE::derive_challenge( static::RFC_VERIFIER, PKCE::METHOD_S256 ); + + $this->assertSame( 43, strlen( $challenge ) ); + $this->assertMatchesRegularExpression( '/^[A-Za-z0-9\-_]+$/', $challenge ); + $this->assertStringNotContainsString( '=', $challenge ); + $this->assertStringNotContainsString( '+', $challenge ); + $this->assertStringNotContainsString( '/', $challenge ); + } + + public function test_plain_challenge_is_the_verifier_verbatim() { + $this->assertSame( static::RFC_VERIFIER, PKCE::derive_challenge( static::RFC_VERIFIER, PKCE::METHOD_PLAIN ) ); + } + + public function test_derive_challenge_returns_null_for_unknown_method() { + $this->assertNull( PKCE::derive_challenge( static::RFC_VERIFIER, 'md5' ) ); + } + + public function test_derive_challenge_is_case_sensitive() { + $this->assertNull( PKCE::derive_challenge( static::RFC_VERIFIER, 's256' ) ); + } + + public function test_verify_true_for_matching_s256_pair() { + $this->assertTrue( PKCE::verify( static::RFC_VERIFIER, static::RFC_CHALLENGE, PKCE::METHOD_S256 ) ); + } + + public function test_verify_false_for_wrong_verifier() { + $this->assertFalse( PKCE::verify( 'wrong-verifier-wrong-verifier-wrong-verifier', static::RFC_CHALLENGE, PKCE::METHOD_S256 ) ); + } + + public function test_verify_true_for_matching_plain_pair() { + $this->assertTrue( PKCE::verify( static::RFC_VERIFIER, static::RFC_VERIFIER, PKCE::METHOD_PLAIN ) ); + } + + public function test_verify_returns_false_not_typeerror_for_unknown_method() { + $this->assertFalse( PKCE::verify( static::RFC_VERIFIER, static::RFC_CHALLENGE, 'md5' ) ); + } + + public function test_verify_returns_false_not_typeerror_for_array_verifier() { + $this->assertFalse( PKCE::verify( [ 'x' ], static::RFC_CHALLENGE, PKCE::METHOD_S256 ) ); + } + + public function test_verify_returns_false_not_typeerror_for_array_challenge() { + $this->assertFalse( PKCE::verify( static::RFC_VERIFIER, [ 'x' ], PKCE::METHOD_S256 ) ); + } + + public function test_is_valid_verifier_accepts_boundary_lengths() { + $this->assertTrue( PKCE::is_valid_verifier( str_repeat( 'a', 43 ) ) ); + $this->assertTrue( PKCE::is_valid_verifier( str_repeat( 'a', 128 ) ) ); + } + + public function test_is_valid_verifier_rejects_lengths_outside_boundary() { + $this->assertFalse( PKCE::is_valid_verifier( str_repeat( 'a', 42 ) ) ); + $this->assertFalse( PKCE::is_valid_verifier( str_repeat( 'a', 129 ) ) ); + } + + public function test_is_valid_verifier_rejects_disallowed_characters() { + $base = str_repeat( 'a', 42 ); + + $this->assertFalse( PKCE::is_valid_verifier( $base . '+' ) ); + $this->assertFalse( PKCE::is_valid_verifier( $base . '/' ) ); + $this->assertFalse( PKCE::is_valid_verifier( $base . '=' ) ); + $this->assertFalse( PKCE::is_valid_verifier( $base . ' ' ) ); + $this->assertFalse( PKCE::is_valid_verifier( $base . '%' ) ); + } + + public function test_is_valid_verifier_rejects_trailing_newline() { + // A `$`-anchored regex would accept a trailing newline; `\z` must not. + $this->assertFalse( PKCE::is_valid_verifier( str_repeat( 'a', 43 ) . "\n" ) ); + } + + public function test_is_valid_verifier_rejects_non_string() { + $this->assertFalse( PKCE::is_valid_verifier( [ 'x' ] ) ); + } + + public function test_is_valid_challenge_for_s256_requires_exactly_43_characters() { + $this->assertFalse( PKCE::is_valid_challenge( str_repeat( 'a', 42 ), PKCE::METHOD_S256 ) ); + $this->assertTrue( PKCE::is_valid_challenge( str_repeat( 'a', 43 ), PKCE::METHOD_S256 ) ); + $this->assertFalse( PKCE::is_valid_challenge( str_repeat( 'a', 44 ), PKCE::METHOD_S256 ) ); + } + + public function test_is_valid_challenge_for_s256_rejects_padding() { + $this->assertFalse( PKCE::is_valid_challenge( str_repeat( 'a', 42 ) . '=', PKCE::METHOD_S256 ) ); + } + + public function test_is_valid_challenge_for_s256_rejects_verifier_only_characters() { + // '.' and '~' are valid in a verifier, but not in base64url. + $this->assertFalse( PKCE::is_valid_challenge( str_repeat( 'a', 42 ) . '.', PKCE::METHOD_S256 ) ); + $this->assertFalse( PKCE::is_valid_challenge( str_repeat( 'a', 42 ) . '~', PKCE::METHOD_S256 ) ); + } + + public function test_is_valid_challenge_for_plain_uses_verifier_rules() { + $this->assertTrue( PKCE::is_valid_challenge( static::RFC_VERIFIER, PKCE::METHOD_PLAIN ) ); + $this->assertFalse( PKCE::is_valid_challenge( str_repeat( 'a', 42 ), PKCE::METHOD_PLAIN ) ); + } + + public function test_is_valid_challenge_rejects_unknown_method() { + $this->assertFalse( PKCE::is_valid_challenge( static::RFC_CHALLENGE, 'md5' ) ); + } + + public function test_supported_methods_defaults_to_s256_and_plain() { + $this->assertSame( [ PKCE::METHOD_S256, PKCE::METHOD_PLAIN ], PKCE::supported_methods() ); + } + + public function test_supported_methods_filter_removes_plain() { + $filter = function () { + return [ PKCE::METHOD_S256 ]; + }; + add_filter( 'oauth2.pkce.supported_methods', $filter ); + + $this->assertSame( [ PKCE::METHOD_S256 ], PKCE::supported_methods() ); + + remove_filter( 'oauth2.pkce.supported_methods', $filter ); + } + + public function test_generate_verifier_produces_a_valid_verifier() { + $verifier = PKCE::generate_verifier(); + $this->assertTrue( PKCE::is_valid_verifier( $verifier ) ); + } + + public function test_generate_verifier_respects_requested_length() { + $this->assertSame( 64, strlen( PKCE::generate_verifier( 64 ) ) ); + $this->assertSame( 100, strlen( PKCE::generate_verifier( 100 ) ) ); + } + + public function test_generate_verifier_clamps_to_valid_range() { + $this->assertSame( PKCE::VERIFIER_MIN_LENGTH, strlen( PKCE::generate_verifier( 10 ) ) ); + $this->assertSame( PKCE::VERIFIER_MAX_LENGTH, strlen( PKCE::generate_verifier( 500 ) ) ); + } + + public function test_generate_verifier_is_not_deterministic() { + $this->assertNotSame( PKCE::generate_verifier(), PKCE::generate_verifier() ); + } +} diff --git a/tests/test-token-endpoint.php b/tests/test-token-endpoint.php index e9fe11e..06fcd7a 100644 --- a/tests/test-token-endpoint.php +++ b/tests/test-token-endpoint.php @@ -11,6 +11,7 @@ use WP\OAuth2\Client; use WP\OAuth2\Endpoints\Token; +use WP\OAuth2\PKCE; use WP\OAuth2\Tokens\Authorization_Code; use WP_REST_Request; use WP_REST_Server; @@ -148,6 +149,143 @@ public function test_exchange_token_deletes_code_after_use() { $this->assertWPError( $reuse ); } + // ------------------------------------------------------------------------- + // Authorization code grant, PKCE + // ------------------------------------------------------------------------- + + public function test_exchange_token_with_correct_s256_verifier() { + $user = $this->factory->user->create_and_get(); + $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); + $code = Authorization_Code::create( $this->client, $user, [ + 'code_challenge' => $pair['code_challenge'], + 'code_challenge_method' => $pair['code_challenge_method'], + ] ); + + $request = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $request->set_param( 'grant_type', 'authorization_code' ); + $request->set_param( 'client_id', $this->client->get_id() ); + $request->set_param( 'code', $code->get_code() ); + $request->set_param( 'code_verifier', $pair['code_verifier'] ); + + $response = $this->server->dispatch( $request ); + + $this->assertEquals( 200, $response->get_status() ); + $data = $response->get_data(); + $this->assertArrayHasKey( 'access_token', $data ); + } + + public function test_exchange_token_with_correct_plain_verifier() { + $user = $this->factory->user->create_and_get(); + $pair = $this->make_pkce_pair( PKCE::METHOD_PLAIN ); + $code = Authorization_Code::create( $this->client, $user, [ + 'code_challenge' => $pair['code_challenge'], + 'code_challenge_method' => $pair['code_challenge_method'], + ] ); + + $request = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $request->set_param( 'grant_type', 'authorization_code' ); + $request->set_param( 'client_id', $this->client->get_id() ); + $request->set_param( 'code', $code->get_code() ); + $request->set_param( 'code_verifier', $pair['code_verifier'] ); + + $response = $this->server->dispatch( $request ); + + $this->assertEquals( 200, $response->get_status() ); + } + + public function test_exchange_token_with_wrong_verifier_fails_and_burns_code() { + $user = $this->factory->user->create_and_get(); + $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); + $code = Authorization_Code::create( $this->client, $user, [ + 'code_challenge' => $pair['code_challenge'], + 'code_challenge_method' => $pair['code_challenge_method'], + ] ); + + $request = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $request->set_param( 'grant_type', 'authorization_code' ); + $request->set_param( 'client_id', $this->client->get_id() ); + $request->set_param( 'code', $code->get_code() ); + $request->set_param( 'code_verifier', 'wrong-verifier-wrong-verifier-wrong-verifier' ); + + $response = $this->server->dispatch( $request ); + $this->assertEquals( 400, $response->get_status() ); + + // The code must be consumed, not just rejected once: it's deleted along + // with the failed attempt, so re-using it (even with the right verifier + // this time) now fails as an unknown code, not as a bad verifier. + $retry = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $retry->set_param( 'grant_type', 'authorization_code' ); + $retry->set_param( 'client_id', $this->client->get_id() ); + $retry->set_param( 'code', $code->get_code() ); + $retry->set_param( 'code_verifier', $pair['code_verifier'] ); + + $retry_response = $this->server->dispatch( $retry ); + $this->assertEquals( 404, $retry_response->get_status() ); + } + + public function test_exchange_token_missing_verifier_for_pkce_code_fails() { + $user = $this->factory->user->create_and_get(); + $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); + $code = Authorization_Code::create( $this->client, $user, [ + 'code_challenge' => $pair['code_challenge'], + 'code_challenge_method' => $pair['code_challenge_method'], + ] ); + + $request = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $request->set_param( 'grant_type', 'authorization_code' ); + $request->set_param( 'client_id', $this->client->get_id() ); + $request->set_param( 'code', $code->get_code() ); + + $response = $this->server->dispatch( $request ); + $this->assertEquals( 400, $response->get_status() ); + } + + public function test_exchange_token_verifier_for_non_pkce_code_fails() { + $user = $this->factory->user->create_and_get(); + $code = Authorization_Code::create( $this->client, $user ); + + $request = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $request->set_param( 'grant_type', 'authorization_code' ); + $request->set_param( 'client_id', $this->client->get_id() ); + $request->set_param( 'code', $code->get_code() ); + $request->set_param( 'code_verifier', PKCE::generate_verifier() ); + + $response = $this->server->dispatch( $request ); + $this->assertEquals( 400, $response->get_status() ); + } + + public function test_exchange_token_rejects_array_valued_verifier_via_schema() { + $user = $this->factory->user->create_and_get(); + $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); + $code = Authorization_Code::create( $this->client, $user, [ + 'code_challenge' => $pair['code_challenge'], + 'code_challenge_method' => $pair['code_challenge_method'], + ] ); + + $request = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $request->set_param( 'grant_type', 'authorization_code' ); + $request->set_param( 'client_id', $this->client->get_id() ); + $request->set_param( 'code', $code->get_code() ); + $request->set_param( 'code_verifier', [ 'x' ] ); + + $response = $this->server->dispatch( $request ); + $this->assertEquals( 400, $response->get_status() ); + } + + public function test_exchange_token_non_pkce_code_with_no_verifier_still_works() { + // Back-compat: a code minted without PKCE needs no verifier at all. + $user = $this->factory->user->create_and_get(); + $code = Authorization_Code::create( $this->client, $user ); + + $request = new WP_REST_Request( 'POST', '/oauth2/access_token' ); + $request->set_param( 'grant_type', 'authorization_code' ); + $request->set_param( 'client_id', $this->client->get_id() ); + $request->set_param( 'code', $code->get_code() ); + + $response = $this->server->dispatch( $request ); + $this->assertEquals( 200, $response->get_status() ); + } + // ------------------------------------------------------------------------- // Client credentials grant // ------------------------------------------------------------------------- diff --git a/tests/test-types-authorization-code.php b/tests/test-types-authorization-code.php new file mode 100644 index 0000000..7861da9 --- /dev/null +++ b/tests/test-types-authorization-code.php @@ -0,0 +1,286 @@ +gather_extra_params( $client, $request ); + } + + public function get_error_redirect_url_public( $redirect_uri, $error, $description, $state = null ) { + return $this->get_error_redirect_url( $redirect_uri, $error, $description, $state ); + } +} + +/** + * Exposes the protected methods under test for the implicit grant. + */ +class Test_Exposed_Implicit_Type extends Implicit { + public function gather_extra_params_public( Client $client, array $request ) { + return $this->gather_extra_params( $client, $request ); + } + + public function get_error_redirect_url_public( $redirect_uri, $error, $description, $state = null ) { + return $this->get_error_redirect_url( $redirect_uri, $error, $description, $state ); + } +} + +/** + * Test cases for PKCE parameter handling in the authorization_code grant. + */ +class Test_Types_Authorization_Code extends Test_Case { + + /** + * @var Client + */ + protected $client; + + /** + * @var Test_Exposed_Authorization_Code_Type + */ + protected $type; + + public function set_up() { + parent::set_up(); + $this->client = $this->create_client(); + $this->type = new Test_Exposed_Authorization_Code_Type(); + } + + public function test_no_challenge_returns_empty_array() { + $this->assertSame( [], $this->type->gather_extra_params_public( $this->client, [] ) ); + } + + public function test_valid_s256_challenge_returns_both_keys() { + $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); + $result = $this->type->gather_extra_params_public( + $this->client, + [ + 'code_challenge' => $pair['code_challenge'], + 'code_challenge_method' => 'S256', + ] + ); + + $this->assertSame( $pair['code_challenge'], $result['code_challenge'] ); + $this->assertSame( 'S256', $result['code_challenge_method'] ); + } + + public function test_unsupported_method_is_rejected() { + $result = $this->type->gather_extra_params_public( + $this->client, + [ + 'code_challenge' => str_repeat( 'a', 43 ), + 'code_challenge_method' => 'md5', + ] + ); + + $this->assertWPError( $result ); + $this->assertSame( 'invalid_request', $result->get_error_data()['error'] ); + } + + public function test_wrongly_cased_method_is_rejected() { + $result = $this->type->gather_extra_params_public( + $this->client, + [ + 'code_challenge' => str_repeat( 'a', 43 ), + 'code_challenge_method' => 's256', + ] + ); + + $this->assertWPError( $result ); + } + + public function test_malformed_challenge_is_rejected() { + $result = $this->type->gather_extra_params_public( + $this->client, + [ + 'code_challenge' => 'too-short', + 'code_challenge_method' => 'S256', + ] + ); + + $this->assertWPError( $result ); + } + + public function test_method_without_challenge_is_ignored() { + $result = $this->type->gather_extra_params_public( + $this->client, + [ 'code_challenge_method' => 'S256' ] + ); + + $this->assertSame( [], $result ); + } + + public function test_omitted_method_normalizes_to_plain() { + $verifier = PKCE::generate_verifier(); + $result = $this->type->gather_extra_params_public( + $this->client, + [ 'code_challenge' => $verifier ] + ); + + $this->assertSame( 'plain', $result['code_challenge_method'] ); + } + + public function test_array_valued_challenge_is_treated_as_absent() { + $result = $this->type->gather_extra_params_public( + $this->client, + [ 'code_challenge' => [ 'x' ] ] + ); + + $this->assertSame( [], $result ); + } + + public function test_pkce_required_client_without_challenge_is_rejected() { + $client = $this->create_client( [ 'pkce_required' => true ] ); + $result = $this->type->gather_extra_params_public( $client, [] ); + + $this->assertWPError( $result ); + $this->assertEquals( 'oauth2.types.authorization_code.gather_extra_params.pkce_required', $result->get_error_code() ); + } + + public function test_pkce_required_client_with_plain_is_rejected() { + $client = $this->create_client( [ 'pkce_required' => true ] ); + $verifier = PKCE::generate_verifier(); + $result = $this->type->gather_extra_params_public( + $client, + [ + 'code_challenge' => $verifier, + 'code_challenge_method' => 'plain', + ] + ); + + $this->assertWPError( $result ); + $this->assertEquals( 'oauth2.types.authorization_code.gather_extra_params.weak_method', $result->get_error_code() ); + } + + public function test_pkce_required_client_with_s256_is_accepted() { + $client = $this->create_client( [ 'pkce_required' => true ] ); + $pair = $this->make_pkce_pair( PKCE::METHOD_S256 ); + $result = $this->type->gather_extra_params_public( + $client, + [ + 'code_challenge' => $pair['code_challenge'], + 'code_challenge_method' => 'S256', + ] + ); + + $this->assertIsArray( $result ); + } + + public function test_error_redirect_url_uses_query_string() { + $url = $this->type->get_error_redirect_url_public( 'https://example.com/callback', 'invalid_request', 'Bad request.' ); + $this->assertStringContainsString( 'https://example.com/callback?', $url ); + $this->assertStringContainsString( 'error=invalid_request', $url ); + } + + public function test_error_redirect_url_omits_state_when_absent() { + $url = $this->type->get_error_redirect_url_public( 'https://example.com/callback', 'invalid_request', 'Bad request.' ); + $this->assertStringNotContainsString( 'state=', $url ); + } + + public function test_error_redirect_url_includes_state_when_present() { + $url = $this->type->get_error_redirect_url_public( 'https://example.com/callback', 'invalid_request', 'Bad request.', 'xyz' ); + $this->assertStringContainsString( 'state=xyz', $url ); + } + + /** + * Regression test: an authorize-time PKCE error must redirect to the + * client's registered callback, not silently fall back to wp-admin. + * + * wp_safe_redirect() rejects a callback on a host other than the current + * site (there is no allowed_redirect_hosts filter registered anywhere in + * this plugin) and substitutes admin_url() instead, so the client would + * never learn the request failed. wp_redirect() and wp_safe_redirect() + * both funnel through the 'wp_redirect' filter, so hooking it captures + * the final location actually sent, whichever function was used, and + * lets the test escape before handle_authorisation()'s exit. + */ + public function test_pkce_error_redirects_to_client_callback_not_wp_admin() { + $client = $this->create_client( [ 'pkce_required' => true ] ); + $_GET['client_id'] = $client->get_id(); + $captured_location = null; + + add_filter( + 'wp_redirect', + function ( $location ) use ( &$captured_location ) { + $captured_location = $location; + throw new Redirect_Interrupt(); + } + ); + + try { + ( new Authorization_Code() )->handle_authorisation(); + $this->fail( 'Expected handle_authorisation() to redirect on a PKCE error.' ); + } catch ( Redirect_Interrupt $e ) { + // Expected: escapes before the real exit(); see filter above. + } + + unset( $_GET['client_id'] ); + + $this->assertNotNull( $captured_location ); + $this->assertStringStartsWith( 'https://example.com/callback', $captured_location ); + $this->assertStringContainsString( 'error=invalid_request', $captured_location ); + } +} + +/** + * Test cases for the implicit grant's PKCE-required bypass fix. + */ +class Test_Types_Implicit extends Test_Case { + + /** + * @var Client + */ + protected $client; + + /** + * @var Test_Exposed_Implicit_Type + */ + protected $type; + + public function set_up() { + parent::set_up(); + $this->client = $this->create_client(); + $this->type = new Test_Exposed_Implicit_Type(); + } + + public function test_non_pkce_client_is_allowed() { + $this->assertSame( [], $this->type->gather_extra_params_public( $this->client, [] ) ); + } + + public function test_pkce_required_client_is_refused() { + $client = $this->create_client( [ 'pkce_required' => true ] ); + $result = $this->type->gather_extra_params_public( $client, [] ); + + $this->assertWPError( $result ); + $this->assertEquals( 'oauth2.types.implicit.gather_extra_params.pkce_required', $result->get_error_code() ); + } + + public function test_error_redirect_url_uses_fragment() { + $url = $this->type->get_error_redirect_url_public( 'https://example.com/callback', 'unauthorized_client', 'Nope.' ); + $this->assertStringContainsString( '#', $url ); + $this->assertStringContainsString( 'error=unauthorized_client', $url ); + } +}