Added PKCE support #46
Conversation
There was a problem hiding this comment.
Phew, massive review, but looking really good overall. Thanks so much for this PR!
Mostly minor things to fix up here; if you don't have time for them, I can follow them up instead.
Haven't tested this for functionality or against the spec yet, but will do so soonish.
README.md
Outdated
|
|
||
| code_verifier = 052edd3941bb8040ecac75d2359d7cd1abe2518911b<br> | ||
| code_challenge = base64( sha256( code_verifier ) ) = MmNmZTJlNGZhYmNmYzQ3YTI4MmRhY2Q1NGEwZDUzZTFiZGFhNTNlODI4MGY3NjM0YWUwNjA1YjYzMmQwNDMxNQ==<br> | ||
| code_challenge_method = s256 |
There was a problem hiding this comment.
This should probably be wrapped in a code block. (Actually, we should eventually move into the proper docs, but that can happen later.)
inc/endpoints/class-token.php
Outdated
| @@ -71,7 +76,7 @@ public function exchange_token( WP_REST_Request $request ) { | |||
| return $auth_code; | |||
| } | |||
|
|
|||
| $is_valid = $auth_code->validate(); | |||
| $is_valid = $auth_code->validate( $request ); | |||
There was a problem hiding this comment.
I'd rather pass the args in separately here to avoid having the validate() method depend on the request parameter names.
| @@ -108,6 +108,36 @@ public function get_expiration() { | |||
| return (int) $value['expiration']; | |||
| } | |||
|
|
|||
| private function validate_code_verifier( $args ) { | |||
There was a problem hiding this comment.
This should be protected, not private
| $is_valid = $decoded === $value['code_challenge']; | ||
| break; | ||
| case 'plain': | ||
| $is_valid = $code_verifier === $value['code_challenge']; |
There was a problem hiding this comment.
Both this equality check and the one above should use hash_equals() to ensure constant-time string comparison (to avoid timing attacks).
|
|
||
| switch ( strtolower( $value['code_challenge_method'] ) ) { | ||
| case 's256': | ||
| $decoded = base64_encode( hash( 'sha256', $code_verifier ) ); |
| } else { | ||
| $is_strong_crypto = true; | ||
| $random_seed = \bin2hex( \openssl_random_pseudo_bytes( $length / 2 + $length % 2, $is_strong_crypto ) ); | ||
| $random_seed = \substr( $random_seed, 0, $length ); |
| ], | ||
| ]; | ||
|
|
||
| \WP_CLI\Utils\format_items( 'table', $items, [ 'code_verifier', 'code_challenge = base64( sha256( code_verifier ) )' ] ); |
| } | ||
| } | ||
|
|
||
| $code_challenge = \base64_encode( hash( 'sha256', $random_seed ) ); |
| $items = [ | ||
| [ | ||
| 'code_verifier' => $random_seed, | ||
| 'code_challenge = base64( sha256( code_verifier ) )' => $code_challenge, |
There was a problem hiding this comment.
I think we should keep the title a little shorter, but not sure what this actually looks like in practice.
There was a problem hiding this comment.
The keys are longer anyways
inc/namespace.php
Outdated
|
|
||
| // WP-Cli | ||
| if ( class_exists( __NAMESPACE__ . '\\Utilities\\Oauth2_Wp_Cli' ) ) { | ||
| \WP_CLI::add_command( 'oauth2', __NAMESPACE__ . '\\Utilities\\Oauth2_Wp_Cli' ); |
There was a problem hiding this comment.
WP_CLI should be used at the top of the file instead of an absolute reference.
#18