Skip to content

Commit 65e467e

Browse files
smarcetsmarcet
committed
fix(oauth2): serialize Native client custom-scheme create/update behind a Redis lock
Closes the TOCTOU race in IClientRepository::hasCustomSchemeRegisteredOnAnotherClientThan(): it's a count-then-write check with no DB-level uniqueness constraint or locking. Two concurrent create()/update() requests registering the same custom scheme (e.g. two different developers self-registering clients on this IDP) could both pass the "is this scheme already claimed" check before either transaction commits, letting both end up with the same scheme - defeating the OS-level interception-prevention guarantee the check exists for (flagged Major by CodeRabbit on PR #147, left unaddressed; raised again by /review-pr-deep as Finding 3). Fix: ClientService::create()/update() now acquire a single global lock (ILockManagerService, Redis-backed via LockManagerService - the same distributed-lock abstraction TokenService already uses) around the entire transaction for Native-client payloads, so the uniqueness check and the write it gates are no longer split across two unsynchronized transactions. Non-Native payloads are unaffected (no lock, no behavior change). SHORTCUT: one lock name serializes ALL Native client scheme create/update calls, not just ones that would actually collide on the same scheme. Acceptable given self-service client registration is low-throughput. Upgrade trigger: move to per-scheme locks (sorted acquisition order to avoid deadlock) if registration volume makes this a bottleneck. Residual gap (pre-existing, not introduced or worsened here): a row reaching storage via ClientFactory::build() directly (e.g. a seeder) still bypasses this lock, same as it already bypasses assertNativeCustomSchemesAllowed() - unchanged by this fix. Regression test added to ClientApiTest: pre-acquires the same lock name the service uses to simulate contention, asserts a clean 412 instead of a successful registration. True concurrent-transaction races aren't reproducible in this synchronous test harness, so this proves the locking mechanism is wired in and fails closed rather than proving the race itself is gone. Verified: ClientApiTest 17/17, full "Application Test Suite" 164/164 (0 failures/errors).
1 parent 2d091b2 commit 65e467e

2 files changed

Lines changed: 187 additions & 91 deletions

File tree

‎app/Services/OAuth2/ClientService.php‎

Lines changed: 155 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -39,14 +39,32 @@
3939
use models\exceptions\ValidationException;
4040
use Utils\Db\ITransactionService;
4141
use models\exceptions\EntityNotFoundException;
42+
use Utils\Exceptions\UnacquiredLockException;
4243
use Utils\Http\HttpUtils;
4344
use Utils\Services\IAuthService;
45+
use Utils\Services\ILockManagerService;
46+
use Closure;
4447
/**
4548
* Class ClientService
4649
* @package Services\OAuth2
4750
*/
4851
final class ClientService extends AbstractService implements IClientService
4952
{
53+
/**
54+
* SHORTCUT: a single global lock name serializes ALL Native client create()/update() calls that
55+
* touch a custom URI scheme, not just the ones that would actually collide on the same scheme -
56+
* acceptable given self-service client registration is low-throughput. Upgrade trigger: move to
57+
* per-scheme locks (sorted acquisition order to avoid deadlock) if native client registration
58+
* volume makes this a bottleneck.
59+
*/
60+
const NATIVE_CUSTOM_SCHEME_REGISTRATION_LOCK = 'client.native.custom_scheme.registration';
61+
62+
/**
63+
* seconds - long enough to cover one create()/update() transaction, short enough that a crashed
64+
* request doesn't block native client scheme registration for long.
65+
*/
66+
const NATIVE_CUSTOM_SCHEME_REGISTRATION_LOCK_LIFETIME = 30;
67+
5068
/**
5169
* @var IAuthService
5270
*/
@@ -74,6 +92,11 @@ final class ClientService extends AbstractService implements IClientService
7492
*/
7593
private $scope_repository;
7694

95+
/**
96+
* @var ILockManagerService
97+
*/
98+
private $lock_manager_service;
99+
77100
/**
78101
* ClientService constructor.
79102
* @param IUserRepository $user_repository
@@ -83,6 +106,7 @@ final class ClientService extends AbstractService implements IClientService
83106
* @param IClientCredentialGenerator $client_credential_generator
84107
* @param IApiScopeRepository $scope_repository
85108
* @param ITransactionService $tx_service
109+
* @param ILockManagerService $lock_manager_service
86110
*/
87111
public function __construct
88112
(
@@ -92,7 +116,8 @@ public function __construct
92116
IApiScopeService $scope_service,
93117
IClientCredentialGenerator $client_credential_generator,
94118
IApiScopeRepository $scope_repository,
95-
ITransactionService $tx_service
119+
ITransactionService $tx_service,
120+
ILockManagerService $lock_manager_service
96121
)
97122
{
98123
parent::__construct($tx_service);
@@ -102,6 +127,31 @@ public function __construct
102127
$this->client_credential_generator = $client_credential_generator;
103128
$this->client_repository = $client_repository;
104129
$this->scope_repository = $scope_repository;
130+
$this->lock_manager_service = $lock_manager_service;
131+
}
132+
133+
/**
134+
* Closes the TOCTOU race in IClientRepository::hasCustomSchemeRegisteredOnAnotherClientThan():
135+
* without this, two concurrent create()/update() calls can both pass the "is this scheme already
136+
* registered" uniqueness check before either transaction commits, letting two different clients
137+
* claim the same custom scheme - defeating the OS-level interception-prevention guarantee the
138+
* uniqueness check exists for.
139+
*
140+
* @param Closure $fn
141+
* @return IEntity
142+
* @throws ValidationException
143+
*/
144+
private function withNativeCustomSchemeLock(Closure $fn): IEntity
145+
{
146+
try {
147+
return $this->lock_manager_service->lock(
148+
self::NATIVE_CUSTOM_SCHEME_REGISTRATION_LOCK,
149+
$fn,
150+
self::NATIVE_CUSTOM_SCHEME_REGISTRATION_LOCK_LIFETIME
151+
);
152+
} catch (UnacquiredLockException $ex) {
153+
throw new ValidationException('another native client custom URI scheme registration is in progress, please retry.');
154+
}
105155
}
106156

107157

@@ -262,43 +312,50 @@ private function assertNativeCustomSchemesAllowed(array $payload, int $exclude_c
262312
*/
263313
public function create(array $payload):IEntity
264314
{
315+
$do_create = function () use ($payload) {
316+
return $this->tx_service->transaction(function () use ($payload) {
265317

266-
return $this->tx_service->transaction(function () use ($payload) {
267-
268-
$current_user = $this->auth_service->getCurrentUser();
318+
$current_user = $this->auth_service->getCurrentUser();
269319

270-
$app_name = trim($payload['app_name']);
320+
$app_name = trim($payload['app_name']);
271321

272-
if($this->client_repository->getByApplicationName($app_name) != null){
273-
throw new ValidationException('there is already another application with that name, please choose another one.');
274-
}
322+
if($this->client_repository->getByApplicationName($app_name) != null){
323+
throw new ValidationException('there is already another application with that name, please choose another one.');
324+
}
275325

276-
// same scheme deny-list + cross-client uniqueness rule update() enforces (only reachable
277-
// for native clients, where the runtime https gate is relaxed) - now covers redirect_uris too.
278-
if (($payload['application_type'] ?? null) === IClient::ApplicationType_Native) {
279-
$this->assertNativeCustomSchemesAllowed($payload);
280-
}
326+
// same scheme deny-list + cross-client uniqueness rule update() enforces (only reachable
327+
// for native clients, where the runtime https gate is relaxed) - now covers redirect_uris too.
328+
if (($payload['application_type'] ?? null) === IClient::ApplicationType_Native) {
329+
$this->assertNativeCustomSchemesAllowed($payload);
330+
}
281331

282-
$client = ClientFactory::build($payload);
283-
$client = $this->client_credential_generator->generate($client);
284-
285-
if(isset($payload['admin_users']) && is_array($payload['admin_users'])) {
286-
$admin_users = $payload['admin_users'];
287-
//add admin users
288-
foreach ($admin_users as $user_id) {
289-
$user = $this->user_repository->getById(intval($user_id));
290-
if (is_null($user)) throw new EntityNotFoundException(sprintf('user %s not found.', $user_id));
291-
if(!$user instanceof User) continue;
292-
$client->addAdminUser($user);
332+
$client = ClientFactory::build($payload);
333+
$client = $this->client_credential_generator->generate($client);
334+
335+
if(isset($payload['admin_users']) && is_array($payload['admin_users'])) {
336+
$admin_users = $payload['admin_users'];
337+
//add admin users
338+
foreach ($admin_users as $user_id) {
339+
$user = $this->user_repository->getById(intval($user_id));
340+
if (is_null($user)) throw new EntityNotFoundException(sprintf('user %s not found.', $user_id));
341+
if(!$user instanceof User) continue;
342+
$client->addAdminUser($user);
343+
}
293344
}
294-
}
295345

296-
$client->setOwner($current_user);
346+
$client->setOwner($current_user);
297347

298-
$this->client_repository->add($client);
348+
$this->client_repository->add($client);
299349

300-
return $client;
301-
});
350+
return $client;
351+
});
352+
};
353+
354+
if (($payload['application_type'] ?? null) === IClient::ApplicationType_Native) {
355+
return $this->withNativeCustomSchemeLock($do_create);
356+
}
357+
358+
return $do_create();
302359
}
303360

304361

@@ -311,86 +368,93 @@ public function create(array $payload):IEntity
311368
*/
312369
public function update(int $id, array $payload):IEntity
313370
{
371+
$do_update = function () use ($id, $payload) {
372+
return $this->tx_service->transaction(function () use ($id, $payload) {
314373

315-
return $this->tx_service->transaction(function () use ($id, $payload) {
316-
317-
$editing_user = $this->auth_service->getCurrentUser();
374+
$editing_user = $this->auth_service->getCurrentUser();
318375

319-
$client = $this->client_repository->getById($id);
376+
$client = $this->client_repository->getById($id);
320377

321-
if (is_null($client) || !$client instanceof Client) {
322-
throw new EntityNotFoundException(sprintf('client id %s does not exists.', $id));
323-
}
324-
$app_name = isset($payload['app_name']) ? trim($payload['app_name']) : null;
325-
if(!empty($app_name)) {
326-
$old_client = $this->client_repository->getByApplicationName($app_name);
327-
if(!is_null($old_client) && $old_client->getId() !== $client->getId())
328-
throw new ValidationException('there is already another application with that name, please choose another one.');
329-
}
330-
$current_app_type = $client->getApplicationType();
331-
if($current_app_type !== $payload['application_type'])
332-
{
333-
throw new ValidationException('application type does not match.');
334-
}
378+
if (is_null($client) || !$client instanceof Client) {
379+
throw new EntityNotFoundException(sprintf('client id %s does not exists.', $id));
380+
}
381+
$app_name = isset($payload['app_name']) ? trim($payload['app_name']) : null;
382+
if(!empty($app_name)) {
383+
$old_client = $this->client_repository->getByApplicationName($app_name);
384+
if(!is_null($old_client) && $old_client->getId() !== $client->getId())
385+
throw new ValidationException('there is already another application with that name, please choose another one.');
386+
}
387+
$current_app_type = $client->getApplicationType();
388+
if($current_app_type !== $payload['application_type'])
389+
{
390+
throw new ValidationException('application type does not match.');
391+
}
335392

336-
ClientFactory::populate($client, $payload);
393+
ClientFactory::populate($client, $payload);
337394

338-
// validate uris
339-
switch($client->getApplicationType()) {
340-
case IClient::ApplicationType_Native: {
341-
// redirect_uris, allowed_origins, and post_logout_redirect_uris all share the same
342-
// scheme deny-list + cross-client uniqueness rule; assertNativeCustomSchemesAllowed
343-
// validates whichever of the three are present in the payload.
344-
$this->assertNativeCustomSchemesAllowed($payload, $id);
345-
}
346-
break;
347-
case IClient::ApplicationType_Web_App:
348-
case IClient::ApplicationType_JS_Client: {
349-
if (isset($payload['redirect_uris'])){
350-
if (!empty($payload['redirect_uris'])) {
351-
$redirect_uris = explode(',', $payload['redirect_uris']);
352-
foreach ($redirect_uris as $uri) {
395+
// validate uris
396+
switch($client->getApplicationType()) {
397+
case IClient::ApplicationType_Native: {
398+
// redirect_uris, allowed_origins, and post_logout_redirect_uris all share the same
399+
// scheme deny-list + cross-client uniqueness rule; assertNativeCustomSchemesAllowed
400+
// validates whichever of the three are present in the payload.
401+
$this->assertNativeCustomSchemesAllowed($payload, $id);
402+
}
403+
break;
404+
case IClient::ApplicationType_Web_App:
405+
case IClient::ApplicationType_JS_Client: {
406+
if (isset($payload['redirect_uris'])){
407+
if (!empty($payload['redirect_uris'])) {
408+
$redirect_uris = explode(',', $payload['redirect_uris']);
409+
foreach ($redirect_uris as $uri) {
410+
$uri = @parse_url($uri);
411+
if (!isset($uri['scheme'])) {
412+
throw new ValidationException('invalid scheme on redirect uri.');
413+
}
414+
if (!HttpUtils::isHttpsSchema($uri['scheme'])) {
415+
throw new ValidationException(sprintf('scheme %s:// is invalid.', $uri['scheme']));
416+
}
417+
}
418+
}
419+
}
420+
if($client->getApplicationType() === IClient::ApplicationType_JS_Client && isset($payload['allowed_origins']) &&!empty($payload['allowed_origins'])){
421+
$allowed_origins = explode(',', $payload['allowed_origins']);
422+
foreach ($allowed_origins as $uri) {
353423
$uri = @parse_url($uri);
354424
if (!isset($uri['scheme'])) {
355-
throw new ValidationException('invalid scheme on redirect uri.');
425+
throw new ValidationException('invalid scheme on allowed origin uri.');
356426
}
357427
if (!HttpUtils::isHttpsSchema($uri['scheme'])) {
358428
throw new ValidationException(sprintf('scheme %s:// is invalid.', $uri['scheme']));
359429
}
360430
}
361431
}
362432
}
363-
if($client->getApplicationType() === IClient::ApplicationType_JS_Client && isset($payload['allowed_origins']) &&!empty($payload['allowed_origins'])){
364-
$allowed_origins = explode(',', $payload['allowed_origins']);
365-
foreach ($allowed_origins as $uri) {
366-
$uri = @parse_url($uri);
367-
if (!isset($uri['scheme'])) {
368-
throw new ValidationException('invalid scheme on allowed origin uri.');
369-
}
370-
if (!HttpUtils::isHttpsSchema($uri['scheme'])) {
371-
throw new ValidationException(sprintf('scheme %s:// is invalid.', $uri['scheme']));
372-
}
373-
}
374-
}
433+
break;
375434
}
376-
break;
377-
}
378435

379-
if(isset($payload['admin_users']) && is_array($payload['admin_users'])) {
380-
$admin_users = $payload['admin_users'];
381-
//add admin users
382-
$client->removeAllAdminUsers();
383-
foreach ($admin_users as $user_id) {
384-
$user = $this->user_repository->getById(intval($user_id));
385-
if (is_null($user)) throw new EntityNotFoundException(sprintf('user %s not found.', $user_id));
386-
if(!$user instanceof User) continue;
387-
$client->addAdminUser($user);
436+
if(isset($payload['admin_users']) && is_array($payload['admin_users'])) {
437+
$admin_users = $payload['admin_users'];
438+
//add admin users
439+
$client->removeAllAdminUsers();
440+
foreach ($admin_users as $user_id) {
441+
$user = $this->user_repository->getById(intval($user_id));
442+
if (is_null($user)) throw new EntityNotFoundException(sprintf('user %s not found.', $user_id));
443+
if(!$user instanceof User) continue;
444+
$client->addAdminUser($user);
445+
}
388446
}
389-
}
390447

391-
$client->setEditedBy($editing_user);
392-
return $client;
393-
});
448+
$client->setEditedBy($editing_user);
449+
return $client;
450+
});
451+
};
452+
453+
if (($payload['application_type'] ?? null) === IClient::ApplicationType_Native) {
454+
return $this->withNativeCustomSchemeLock($do_update);
455+
}
456+
457+
return $do_update();
394458
}
395459

396460
/**

‎tests/ClientApiTest.php‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,12 @@
1414
use OAuth2\Models\IClient;
1515
use Auth\User;
1616
use Models\OAuth2\Client;
17+
use Illuminate\Support\Facades\App;
1718
use Illuminate\Support\Facades\Session;
1819
use Illuminate\Support\Facades\Config;
1920
use LaravelDoctrine\ORM\Facades\EntityManager;
21+
use Services\OAuth2\ClientService;
22+
use Utils\Services\ILockManagerService;
2023
/**
2124
* Class ClientApiTest
2225
*/
@@ -413,4 +416,33 @@ public function testCreateNativeClientRejectsArrayValueForRedirectUrisCleanly(){
413416
$this->assertResponseStatus(412);
414417
}
415418

419+
public function testUpdateNativeClientRejectsCustomSchemeRegistrationWhenAnotherIsInProgress(){
420+
421+
// Closes the TOCTOU race in hasCustomSchemeRegisteredOnAnotherClientThan(): without a lock,
422+
// two concurrent create()/update() calls can both pass the "is this scheme already registered"
423+
// uniqueness check before either transaction commits, letting two different clients claim the
424+
// same custom scheme. create()/update() now serialize behind a single Redis-backed lock
425+
// (ILockManagerService). Simulate contention directly by pre-acquiring that same lock.
426+
$lock_manager = App::make(ILockManagerService::class);
427+
$lock_manager->acquireLock(ClientService::NATIVE_CUSTOM_SCHEME_REGISTRATION_LOCK, 5);
428+
429+
try {
430+
$client = EntityManager::getRepository(Client::class)->findOneBy(['app_name' => 'oauth2_native_app']);
431+
432+
$response = $this->action("PUT", "Api\\ClientApiController@update",
433+
array(
434+
'id' => $client->id,
435+
'application_type' => IClient::ApplicationType_Native,
436+
'redirect_uris' => 'lockcontention://callback',
437+
),
438+
[],
439+
[],
440+
[]);
441+
442+
$this->assertResponseStatus(412);
443+
} finally {
444+
$lock_manager->releaseLock(ClientService::NATIVE_CUSTOM_SCHEME_REGISTRATION_LOCK);
445+
}
446+
}
447+
416448
}

0 commit comments

Comments
 (0)