Skip to content

Commit c686d19

Browse files
committed
fix(oauth2): hash client ID in result cache key to avoid PSR-6 reserved character violation
symfony/cache v7+ enforces PSR-6 key validation in all adapters, including array. Client IDs may contain reserved characters (e.g. '/'), which caused getClientByIdCacheable() to throw InvalidArgumentException and return a server_error 400 during token exchange. Replace the raw client ID with its md5 hash in the cache key so the key is always a valid hex string, regardless of the client ID's content. Adds OAuth2ClientCacheTest to cover retrieval with reserved-character IDs and to confirm distinct IDs hash to distinct cache keys.
1 parent 34d0c3c commit c686d19

File tree

2 files changed

+73
-1
lines changed

2 files changed

+73
-1
lines changed

app/Repositories/DoctrineOAuth2ClientRepository.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ public function getClientByIdCacheable(string $client_id, bool $withResourceServ
122122
$q = $qb->getQuery();
123123

124124
$q->useQueryCache(true);
125-
$q->enableResultCache(600, 'client_by_id_'.$client_id); // TTL 10 min
125+
$q->enableResultCache(600, 'client_by_id_'.md5($client_id)); // TTL 10 min
126126
$q->setHint(Query::HINT_READ_ONLY, true);
127127

128128
return $q->getOneOrNullResult();

tests/OAuth2ClientCacheTest.php

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
<?php namespace Tests;
2+
/**
3+
* Copyright 2025 OpenStack Foundation
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
* http://www.apache.org/licenses/LICENSE-2.0
8+
* Unless required by applicable law or agreed to in writing, software
9+
* distributed under the License is distributed on an "AS IS" BASIS,
10+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11+
* See the License for the specific language governing permissions and
12+
* limitations under the License.
13+
**/
14+
use Illuminate\Support\Facades\App;
15+
use OAuth2\Repositories\IClientRepository;
16+
/**
17+
* Class OAuth2ClientCacheTest
18+
*/
19+
final class OAuth2ClientCacheTest extends BrowserKitTestCase
20+
{
21+
/**
22+
* @var IClientRepository
23+
*/
24+
private IClientRepository $repo;
25+
26+
protected function prepareForTests(): void
27+
{
28+
parent::prepareForTests();
29+
$this->repo = App::make(IClientRepository::class);
30+
}
31+
32+
/**
33+
* A client ID containing '/' must be retrievable without throwing
34+
* InvalidArgumentException due to PSR-6 reserved characters in the
35+
* cache key. A second call must also succeed, confirming the cached
36+
* entry is readable.
37+
*/
38+
public function testGetClientByIdCacheableWithReservedCharactersIsRetrievable(): void
39+
{
40+
$client_id = '.-_~87D8/Vcvr6fvQbH4HyNgwTlfSyQ3x.openstack.client';
41+
42+
// First call: fetches from DB and writes the hashed cache key.
43+
$client = $this->repo->getClientByIdCacheable($client_id);
44+
$this->assertNotNull($client);
45+
$this->assertEquals($client_id, $client->getClientId());
46+
47+
// Second call: reads from result cache — must not throw on the hashed key.
48+
$client_cached = $this->repo->getClientByIdCacheable($client_id);
49+
$this->assertNotNull($client_cached);
50+
$this->assertEquals($client_id, $client_cached->getClientId());
51+
}
52+
53+
/**
54+
* Two distinct client IDs that both contain '/' must hash to different
55+
* cache keys, so that each call returns the correct client and the
56+
* md5 hashing does not accidentally collapse them to the same entry.
57+
*/
58+
public function testGetClientByIdCacheableDistinctReservedCharacterIdsReturnDifferentClients(): void
59+
{
60+
$client_id_a = '.-_~87D8/Vcvr6fvQbH4HyNgwTlfSyQ3x.openstack.client';
61+
$client_id_b = 'Jiz87D8/Vcvr6fvQbH4HyNgwTlfSyQ3x2.openstack.client';
62+
63+
$client_a = $this->repo->getClientByIdCacheable($client_id_a);
64+
$client_b = $this->repo->getClientByIdCacheable($client_id_b);
65+
66+
$this->assertNotNull($client_a);
67+
$this->assertNotNull($client_b);
68+
$this->assertEquals($client_id_a, $client_a->getClientId());
69+
$this->assertEquals($client_id_b, $client_b->getClientId());
70+
$this->assertNotEquals($client_a->getClientId(), $client_b->getClientId());
71+
}
72+
}

0 commit comments

Comments
 (0)