From ebd40337cb4e86d12b9ec90da9a73e839431a9de Mon Sep 17 00:00:00 2001 From: Ashfame Date: Wed, 21 Sep 2022 11:30:55 +0400 Subject: [PATCH 1/6] switch to GET method when taking user consent Currently bshaffer oauth library has a bug when POST is used on AuthorizeEndpoint along with nonce (optional parameter) which fails to set the nonce in id_token --- src/Http/Handlers/AuthorizeHandler.php | 2 +- templates/authenticate/form.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Http/Handlers/AuthorizeHandler.php b/src/Http/Handlers/AuthorizeHandler.php index fa5a38b..a54dac4 100644 --- a/src/Http/Handlers/AuthorizeHandler.php +++ b/src/Http/Handlers/AuthorizeHandler.php @@ -36,7 +36,7 @@ public function handle( Request $request, Response $response ): Response { $user = wp_get_current_user(); if ( $this->consent_storage->needs_consent( $user->ID ) ) { - if ( ! isset( $_POST['authorize'] ) || 'Authorize' !== $_POST['authorize'] ) { + if ( ! isset( $_REQUEST['authorize'] ) || 'Authorize' !== $_REQUEST['authorize'] ) { $response->send(); exit; } diff --git a/templates/authenticate/form.php b/templates/authenticate/form.php index ce6be7a..eddefcb 100644 --- a/templates/authenticate/form.php +++ b/templates/authenticate/form.php @@ -1,6 +1,6 @@ -
+ form_fields as $key => $value ) : ?> From 5ec0967fc4812ff84c03c18ac48d859db590d1d3 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Wed, 21 Sep 2022 12:02:00 +0400 Subject: [PATCH 2/6] suppress phpcs warning since its irrelevant in this case --- src/Http/Handlers/AuthorizeHandler.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Http/Handlers/AuthorizeHandler.php b/src/Http/Handlers/AuthorizeHandler.php index a54dac4..b3d81ea 100644 --- a/src/Http/Handlers/AuthorizeHandler.php +++ b/src/Http/Handlers/AuthorizeHandler.php @@ -36,6 +36,7 @@ public function handle( Request $request, Response $response ): Response { $user = wp_get_current_user(); if ( $this->consent_storage->needs_consent( $user->ID ) ) { + // phpcs:disable WordPress.Security.NonceVerification.Recommended We already have REST API nonce validating the user intention with this request if ( ! isset( $_REQUEST['authorize'] ) || 'Authorize' !== $_REQUEST['authorize'] ) { $response->send(); exit; From 1ca02bc10961dee34d7d3bfcc49ccaaf88f18789 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Wed, 21 Sep 2022 12:16:20 +0400 Subject: [PATCH 3/6] separate explanation for phpcs:disable by -- --- src/Http/Handlers/AuthorizeHandler.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Http/Handlers/AuthorizeHandler.php b/src/Http/Handlers/AuthorizeHandler.php index b3d81ea..b97cfcf 100644 --- a/src/Http/Handlers/AuthorizeHandler.php +++ b/src/Http/Handlers/AuthorizeHandler.php @@ -36,7 +36,7 @@ public function handle( Request $request, Response $response ): Response { $user = wp_get_current_user(); if ( $this->consent_storage->needs_consent( $user->ID ) ) { - // phpcs:disable WordPress.Security.NonceVerification.Recommended We already have REST API nonce validating the user intention with this request + // phpcs:disable WordPress.Security.NonceVerification.Recommended -- We already have REST API nonce validating the user intention with this request if ( ! isset( $_REQUEST['authorize'] ) || 'Authorize' !== $_REQUEST['authorize'] ) { $response->send(); exit; From f0b158a24748d1604c6cf20901d5667f4e733974 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Wed, 21 Sep 2022 18:14:36 +0400 Subject: [PATCH 4/6] Revert changes in this PR for different approach This reverts commit 1ca02bc10961dee34d7d3bfcc49ccaaf88f18789. This reverts commit 5ec0967fc4812ff84c03c18ac48d859db590d1d3. This reverts commit ebd40337cb4e86d12b9ec90da9a73e839431a9de. --- src/Http/Handlers/AuthorizeHandler.php | 3 +-- templates/authenticate/form.php | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Http/Handlers/AuthorizeHandler.php b/src/Http/Handlers/AuthorizeHandler.php index b97cfcf..fa5a38b 100644 --- a/src/Http/Handlers/AuthorizeHandler.php +++ b/src/Http/Handlers/AuthorizeHandler.php @@ -36,8 +36,7 @@ public function handle( Request $request, Response $response ): Response { $user = wp_get_current_user(); if ( $this->consent_storage->needs_consent( $user->ID ) ) { - // phpcs:disable WordPress.Security.NonceVerification.Recommended -- We already have REST API nonce validating the user intention with this request - if ( ! isset( $_REQUEST['authorize'] ) || 'Authorize' !== $_REQUEST['authorize'] ) { + if ( ! isset( $_POST['authorize'] ) || 'Authorize' !== $_POST['authorize'] ) { $response->send(); exit; } diff --git a/templates/authenticate/form.php b/templates/authenticate/form.php index eddefcb..ce6be7a 100644 --- a/templates/authenticate/form.php +++ b/templates/authenticate/form.php @@ -1,6 +1,6 @@ - + form_fields as $key => $value ) : ?> From 7d090a24f8697b5aa3b2c4b342740a03e45f0906 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Wed, 21 Sep 2022 18:23:51 +0400 Subject: [PATCH 5/6] copy over the nonce from parsed POST parameters to parsed GET parameters in $request object as a temporary fix --- src/Http/Handlers/AuthorizeHandler.php | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/Http/Handlers/AuthorizeHandler.php b/src/Http/Handlers/AuthorizeHandler.php index fa5a38b..e9987a3 100644 --- a/src/Http/Handlers/AuthorizeHandler.php +++ b/src/Http/Handlers/AuthorizeHandler.php @@ -22,6 +22,15 @@ public function __construct( OAuth2Server $server, ConsentStorage $consent_stora } public function handle( Request $request, Response $response ): Response { + // Our dependency bshaffer's OAuth library currently has a bug where it doesn't pick up nonce correctly + // if it's a POST method to the Authorize endpoint + // Fix has been contributed upstream but it doesn't look it would be merged anytime soon based on recent activity + // https://github.com/bshaffer/oauth2-server-php/pull/1032 + // Hence, as a temporary fix, we are copying over the nonce from parsed $_POST values to parsed $_GET values in $request object here + if ( isset( $request->request['nonce'] ) && ! isset( $request->query['nonce'] ) ) { + $request->query['nonce'] = $request->request['nonce']; + } + if ( ! $this->server->validateAuthorizeRequest( $request, $response ) ) { return $response; } From d061d400c827c1c7f8487a784c6881ba61f78001 Mon Sep 17 00:00:00 2001 From: Ashfame Date: Wed, 21 Sep 2022 18:40:27 +0400 Subject: [PATCH 6/6] reformat comment to satisfy linter --- src/Http/Handlers/AuthorizeHandler.php | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/Http/Handlers/AuthorizeHandler.php b/src/Http/Handlers/AuthorizeHandler.php index e9987a3..edb0e18 100644 --- a/src/Http/Handlers/AuthorizeHandler.php +++ b/src/Http/Handlers/AuthorizeHandler.php @@ -22,11 +22,9 @@ public function __construct( OAuth2Server $server, ConsentStorage $consent_stora } public function handle( Request $request, Response $response ): Response { - // Our dependency bshaffer's OAuth library currently has a bug where it doesn't pick up nonce correctly - // if it's a POST method to the Authorize endpoint - // Fix has been contributed upstream but it doesn't look it would be merged anytime soon based on recent activity - // https://github.com/bshaffer/oauth2-server-php/pull/1032 - // Hence, as a temporary fix, we are copying over the nonce from parsed $_POST values to parsed $_GET values in $request object here + // Our dependency bshaffer's OAuth library currently has a bug where it doesn't pick up nonce correctly if it's a POST request to the Authorize endpoint. + // Fix has been contributed upstream (https://github.com/bshaffer/oauth2-server-php/pull/1032) but it doesn't look it would be merged anytime soon based on recent activity. + // Hence, as a temporary fix, we are copying over the nonce from parsed $_POST values to parsed $_GET values in $request object here. if ( isset( $request->request['nonce'] ) && ! isset( $request->query['nonce'] ) ) { $request->query['nonce'] = $request->request['nonce']; }