Skip to content

Commit 00238c3

Browse files
julien-ncjulien-nc
committed
feat(oauth2): remove support for plain method, validate challenge and verifier according to th RFC, adjust tests
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
1 parent dc22b12 commit 00238c3

7 files changed

Lines changed: 199 additions & 63 deletions

File tree

‎apps/oauth2/lib/Controller/LoginRedirectorController.php‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@
2929

3030
#[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)]
3131
class LoginRedirectorController extends Controller {
32+
private const PKCE_STRING_PATTERN = '/^[A-Za-z0-9._~-]{43,128}$/';
33+
3234
/**
3335
* @param string $appName
3436
* @param IRequest $request
@@ -58,8 +60,8 @@ public function __construct(
5860
* @param string $state State of the flow
5961
* @param string $response_type Response type for the flow
6062
* @param string $redirect_uri URI to redirect to after the flow (is only used for legacy ownCloud clients)
61-
* @param string $code_challenge PKCE code challenge (optional)
62-
* @param string $code_challenge_method PKCE code challenge method "S256" or "plain" (optional)
63+
* @param string $code_challenge PKCE code challenge (optional, RFC 7636 format)
64+
* @param string $code_challenge_method PKCE code challenge method. Only "S256" is supported. If omitted, RFC 7636 defaults to "plain", which is rejected.
6365
* @return TemplateResponse<Http::STATUS_OK, array{}>|RedirectResponse<Http::STATUS_SEE_OTHER, array{}>
6466
*
6567
* 200: Client not found
@@ -89,18 +91,21 @@ public function authorize($client_id,
8991
return new RedirectResponse($url);
9092
}
9193

92-
if ($code_challenge_method !== '' && $code_challenge_method !== 'S256' && $code_challenge_method !== 'plain') {
93-
$url = $client->getRedirectUri() . '?error=invalid_request&error_description=Invalid+code_challenge_method&state=' . \urlencode($state);
94+
if ($code_challenge === '' && $code_challenge_method !== '') {
95+
$url = $client->getRedirectUri() . '?error=invalid_request&error_description=code_challenge+required&state=' . \urlencode($state);
9496
return new RedirectResponse($url);
9597
}
9698

97-
if ($code_challenge !== '' && $code_challenge_method === '') {
98-
$url = $client->getRedirectUri() . '?error=invalid_request&error_description=code_challenge_method+required&state=' . \urlencode($state);
99+
if ($code_challenge !== '' && preg_match(self::PKCE_STRING_PATTERN, $code_challenge) !== 1) {
100+
$url = $client->getRedirectUri() . '?error=invalid_request&error_description=Invalid+code_challenge&state=' . \urlencode($state);
99101
return new RedirectResponse($url);
100102
}
101103

102-
if ($code_challenge === '' && $code_challenge_method !== '') {
103-
$url = $client->getRedirectUri() . '?error=invalid_request&error_description=code_challenge+required&state=' . \urlencode($state);
104+
$effectiveCodeChallengeMethod = $code_challenge_method === '' && $code_challenge !== ''
105+
? 'plain'
106+
: $code_challenge_method;
107+
if ($effectiveCodeChallengeMethod !== '' && $effectiveCodeChallengeMethod !== 'S256') {
108+
$url = $client->getRedirectUri() . '?error=invalid_request&error_description=Transform+algorithm+not+supported&state=' . \urlencode($state);
104109
return new RedirectResponse($url);
105110
}
106111

@@ -113,7 +118,7 @@ public function authorize($client_id,
113118

114119
$this->session->set('oauth.state', $state);
115120
$this->session->set('oauth.code_challenge', $code_challenge);
116-
$this->session->set('oauth.code_challenge_method', $code_challenge_method);
121+
$this->session->set('oauth.code_challenge_method', $effectiveCodeChallengeMethod);
117122

118123
if (in_array($client->getName(), $this->appConfig->getValueArray('oauth2', 'skipAuthPickerApplications', []))) {
119124
/** @see ClientFlowLoginController::showAuthPickerPage **/

‎apps/oauth2/lib/Controller/OauthApiController.php‎

Lines changed: 25 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
class OauthApiController extends Controller {
3636
// the authorization code expires after 10 minutes
3737
public const AUTHORIZATION_CODE_EXPIRES_AFTER = 10 * 60;
38+
private const PKCE_STRING_PATTERN = '/^[A-Za-z0-9._~-]{43,128}$/';
3839

3940
public function __construct(
4041
string $appName,
@@ -61,7 +62,7 @@ public function __construct(
6162
* @param ?string $refresh_token Refresh token
6263
* @param ?string $client_id Client ID
6364
* @param ?string $client_secret Client secret
64-
* @param ?string $code_verifier PKCE code verifier (required if code_challenge was provided)
65+
* @param ?string $code_verifier PKCE code verifier in RFC 7636 format (required if code_challenge was provided)
6566
* @throws Exception
6667
* @return JSONResponse<Http::STATUS_OK, array{access_token: string, token_type: string, expires_in: int, refresh_token: string, user_id: string}, array{}>|JSONResponse<Http::STATUS_BAD_REQUEST, array{error: string}, array{}>
6768
*
@@ -138,24 +139,30 @@ public function getToken(
138139
return $response;
139140
}
140141

142+
if (preg_match(self::PKCE_STRING_PATTERN, $code_verifier) !== 1) {
143+
$response = new JSONResponse([
144+
'error' => 'invalid_grant',
145+
], Http::STATUS_BAD_REQUEST);
146+
$response->throttle(['invalid_grant' => 'invalid_code_verifier']);
147+
return $response;
148+
}
149+
141150
$codeChallengeMethod = $accessToken->getCodeChallengeMethod();
142-
if ($codeChallengeMethod === 'S256') {
143-
$expectedHash = rtrim(strtr(base64_encode(hash('sha256', $code_verifier, true)), '+/', '-_'), '=');
144-
if (!hash_equals($hashedCodeChallenge, $expectedHash)) {
145-
$response = new JSONResponse([
146-
'error' => 'invalid_grant',
147-
], Http::STATUS_BAD_REQUEST);
148-
$response->throttle(['invalid_grant' => 'pkce_verification_failed']);
149-
return $response;
150-
}
151-
} else {
152-
if (!hash_equals($hashedCodeChallenge, $code_verifier)) {
153-
$response = new JSONResponse([
154-
'error' => 'invalid_grant',
155-
], Http::STATUS_BAD_REQUEST);
156-
$response->throttle(['invalid_grant' => 'pkce_verification_failed']);
157-
return $response;
158-
}
151+
if ($codeChallengeMethod !== 'S256') {
152+
$response = new JSONResponse([
153+
'error' => 'invalid_grant',
154+
], Http::STATUS_BAD_REQUEST);
155+
$response->throttle(['invalid_grant' => 'unsupported_code_challenge_method']);
156+
return $response;
157+
}
158+
159+
$expectedHash = rtrim(strtr(base64_encode(hash('sha256', $code_verifier, true)), '+/', '-_'), '=');
160+
if (!hash_equals($hashedCodeChallenge, $expectedHash)) {
161+
$response = new JSONResponse([
162+
'error' => 'invalid_grant',
163+
], Http::STATUS_BAD_REQUEST);
164+
$response->throttle(['invalid_grant' => 'pkce_verification_failed']);
165+
return $response;
159166
}
160167
}
161168
}

‎apps/oauth2/openapi.json‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@
7878
{
7979
"name": "code_challenge",
8080
"in": "query",
81-
"description": "PKCE code challenge (optional)",
81+
"description": "PKCE code challenge (optional, RFC 7636 format)",
8282
"schema": {
8383
"type": "string",
8484
"default": ""
@@ -87,7 +87,7 @@
8787
{
8888
"name": "code_challenge_method",
8989
"in": "query",
90-
"description": "PKCE code challenge method \"S256\" or \"plain\" (optional)",
90+
"description": "PKCE code challenge method. Only \"S256\" is supported. If omitted, RFC 7636 defaults to \"plain\", which is rejected.",
9191
"schema": {
9292
"type": "string",
9393
"default": ""
@@ -176,7 +176,7 @@
176176
"type": "string",
177177
"nullable": true,
178178
"default": null,
179-
"description": "PKCE code verifier (required if code_challenge was provided)"
179+
"description": "PKCE code verifier in RFC 7636 format (required if code_challenge was provided)"
180180
}
181181
}
182182
}

‎apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php‎

Lines changed: 89 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -70,9 +70,18 @@ public function testAuthorize(): void {
7070
->with('MyClientId')
7171
->willReturn($client);
7272
$this->session
73-
->expects($this->once())
73+
->expects($this->exactly(3))
7474
->method('set')
75-
->with('oauth.state', 'MyState');
75+
->willReturnCallback(function (string $key, string $value): void {
76+
switch ([$key, $value]) {
77+
case ['oauth.state', 'MyState']:
78+
case ['oauth.code_challenge', '']:
79+
case ['oauth.code_challenge_method', '']:
80+
break;
81+
default:
82+
throw new LogicException();
83+
}
84+
});
7685
$this->urlGenerator
7786
->expects($this->once())
7887
->method('linkToRouteAbsolute')
@@ -104,11 +113,13 @@ public function testAuthorizeSkipPicker(): void {
104113
->with('MyClientId')
105114
->willReturn($client);
106115
$this->session
107-
->expects(static::exactly(2))
116+
->expects(static::exactly(4))
108117
->method('set')
109118
->willReturnCallback(function (string $key, string $value): void {
110119
switch ([$key, $value]) {
111120
case ['oauth.state', 'MyState']:
121+
case ['oauth.code_challenge', '']:
122+
case ['oauth.code_challenge_method', '']:
112123
case [ClientFlowLoginController::STATE_NAME, 'MyStateToken']:
113124
/* Expected */
114125
break;
@@ -182,6 +193,59 @@ public function testAuthorizeRejectsCodeChallengeMethodWithoutChallenge(): void
182193
$this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'code', '', '', 'S256'));
183194
}
184195

196+
public function testAuthorizeRejectsPkceWithoutMethodBecausePlainIsUnsupported(): void {
197+
$client = new Client();
198+
$client->setClientIdentifier('MyClientIdentifier');
199+
$client->setRedirectUri('http://foo.bar');
200+
$this->clientMapper
201+
->expects($this->once())
202+
->method('getByIdentifier')
203+
->with('MyClientId')
204+
->willReturn($client);
205+
$this->session
206+
->expects($this->never())
207+
->method('set');
208+
209+
$codeChallenge = str_repeat('a', 43);
210+
$expected = new RedirectResponse('http://foo.bar?error=invalid_request&error_description=Transform+algorithm+not+supported&state=MyState');
211+
$this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'code', '', $codeChallenge));
212+
}
213+
214+
public function testAuthorizeRejectsUnsupportedCodeChallengeMethod(): void {
215+
$client = new Client();
216+
$client->setClientIdentifier('MyClientIdentifier');
217+
$client->setRedirectUri('http://foo.bar');
218+
$this->clientMapper
219+
->expects($this->once())
220+
->method('getByIdentifier')
221+
->with('MyClientId')
222+
->willReturn($client);
223+
$this->session
224+
->expects($this->never())
225+
->method('set');
226+
227+
$codeChallenge = str_repeat('a', 43);
228+
$expected = new RedirectResponse('http://foo.bar?error=invalid_request&error_description=Transform+algorithm+not+supported&state=MyState');
229+
$this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'code', '', $codeChallenge, 'plain'));
230+
}
231+
232+
public function testAuthorizeRejectsInvalidCodeChallengeFormat(): void {
233+
$client = new Client();
234+
$client->setClientIdentifier('MyClientIdentifier');
235+
$client->setRedirectUri('http://foo.bar');
236+
$this->clientMapper
237+
->expects($this->once())
238+
->method('getByIdentifier')
239+
->with('MyClientId')
240+
->willReturn($client);
241+
$this->session
242+
->expects($this->never())
243+
->method('set');
244+
245+
$expected = new RedirectResponse('http://foo.bar?error=invalid_request&error_description=Invalid+code_challenge&state=MyState');
246+
$this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'code', '', 'short', 'S256'));
247+
}
248+
185249
public function testAuthorizeWithLegacyOcClient(): void {
186250
$client = new Client();
187251
$client->setClientIdentifier('MyClientIdentifier');
@@ -192,9 +256,18 @@ public function testAuthorizeWithLegacyOcClient(): void {
192256
->with('MyClientId')
193257
->willReturn($client);
194258
$this->session
195-
->expects($this->once())
259+
->expects($this->exactly(3))
196260
->method('set')
197-
->with('oauth.state', 'MyState');
261+
->willReturnCallback(function (string $key, string $value): void {
262+
switch ([$key, $value]) {
263+
case ['oauth.state', 'MyState']:
264+
case ['oauth.code_challenge', '']:
265+
case ['oauth.code_challenge_method', '']:
266+
break;
267+
default:
268+
throw new LogicException();
269+
}
270+
});
198271
$this->urlGenerator
199272
->expects($this->once())
200273
->method('linkToRouteAbsolute')
@@ -225,9 +298,18 @@ public function testAuthorizeNotForwardingUntrustedURIs(): void {
225298
->with('MyClientId')
226299
->willReturn($client);
227300
$this->session
228-
->expects($this->once())
301+
->expects($this->exactly(3))
229302
->method('set')
230-
->with('oauth.state', 'MyState');
303+
->willReturnCallback(function (string $key, string $value): void {
304+
switch ([$key, $value]) {
305+
case ['oauth.state', 'MyState']:
306+
case ['oauth.code_challenge', '']:
307+
case ['oauth.code_challenge_method', '']:
308+
break;
309+
default:
310+
throw new LogicException();
311+
}
312+
});
231313
$this->urlGenerator
232314
->expects($this->once())
233315
->method('linkToRouteAbsolute')

0 commit comments

Comments
 (0)