From 844328c6eb5632c432a60e2d5dded2d744a48d94 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 14 Jul 2026 20:43:15 -0300 Subject: [PATCH 01/14] feat(oauth2): support custom URI schemes for Native clients across redirect_uris, allowed_origins, post_logout_redirect_uris Native (mobile/desktop) OAuth2 clients can now register custom app schemes (myapp://callback) in allowed_origins and post_logout_redirect_uris via the admin API and React UI, matching the support redirect_uris already had. The OIDC end-session flow honors a registered custom-scheme post-logout URI at runtime. Security hardening (found via adversarial code review): - Deny-list for dangerous/launch pseudo-schemes (javascript:, data:, intent:, etc.) and plain http, centralized in HttpUtils and shared by write-time validation (ClientService) and runtime allow-gates (Client::isUriAllowed/isPostLogoutUriAllowed). - RFC 8252 loopback carve-out: http://127.0.0.1|localhost redirect URIs remain allowed for Native clients (the standard native-app pattern), only non-loopback http is blocked. - Cross-client custom-scheme uniqueness check extended to all three URI fields (was redirect_uris only), preventing OS-level scheme interception between clients. - Defense-in-depth: runtime gates independently re-check the scheme deny-list rather than relying solely on write-time validation. - Fixed a pre-existing crash (missing array key "host") in URLUtils::canonicalUrl/Client::isPostLogoutUriAllowed for host-less custom-scheme URIs (mailto:, file:///x). - Fixed a pre-existing substring false-positive in the cross-client scheme collision check (e.g. "roipapp" matching inside "androipapp://..."). Also fixes an unrelated pre-existing bug in UserLoginTurnstileTest where assigning null (from an unset env var) to a typed string property threw a TypeError before the intended skip-guard could run. Plan: docs/plans/2026-07-14-native-clients-custom-schemes.md --- .../Controllers/Api/ClientApiController.php | 4 +- app/Models/OAuth2/Client.php | 48 +++- .../DoctrineOAuth2ClientRepository.php | 35 ++- app/Services/OAuth2/ClientService.php | 62 +++++- .../OAuth2/Repositories/IClientRepository.php | 2 +- app/libs/Utils/Http/HttpUtils.php | 38 ++++ app/libs/Utils/URLUtils.php | 5 + .../edit_client/components/logout_options.js | 20 +- .../components/security_settings_panel.js | 2 +- tests/ClientApiTest.php | 209 ++++++++++++++++++ tests/UserLoginTurnstileTest.php | 10 +- tests/unit/ClientMappingTest.php | 100 +++++++++ 12 files changed, 506 insertions(+), 29 deletions(-) diff --git a/app/Http/Controllers/Api/ClientApiController.php b/app/Http/Controllers/Api/ClientApiController.php index 961e3e96..9d8c347c 100644 --- a/app/Http/Controllers/Api/ClientApiController.php +++ b/app/Http/Controllers/Api/ClientApiController.php @@ -699,8 +699,8 @@ protected function getUpdatePayloadValidationRules(): array 'tos_uri' => 'nullable|url', 'redirect_uris' => 'nullable|custom_url_set:application_type', 'policy_uri' => 'nullable|url', - 'post_logout_redirect_uris' => 'nullable|ssl_url_set', - 'allowed_origins' => 'nullable|ssl_url_set', + 'post_logout_redirect_uris' => 'nullable|custom_url_set:application_type', + 'allowed_origins' => 'nullable|custom_url_set:application_type', 'logout_uri' => 'nullable|url', 'logout_session_required' => 'sometimes|required|boolean', 'logout_use_iframe' => 'sometimes|required|boolean', diff --git a/app/Models/OAuth2/Client.php b/app/Models/OAuth2/Client.php index c01be875..cd6be750 100644 --- a/app/Models/OAuth2/Client.php +++ b/app/Models/OAuth2/Client.php @@ -14,6 +14,7 @@ use App\libs\Utils\URLUtils; use Auth\User; +use Utils\Http\HttpUtils; use Doctrine\Common\Collections\Criteria; use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Log; @@ -629,6 +630,20 @@ public function isScopeAllowed(string $scope):bool return $res; } + /** + * Single source of truth for "is this scheme dangerous for a Native client" across the runtime allow-gates + * (isUriAllowed for redirect_uris, isPostLogoutUriAllowed for post_logout_redirect_uris). Delegates the + * actual deny-list to HttpUtils, which ClientService's write-time validation also uses. + * + * @param string $scheme + * @param string|null $host enables the RFC 8252 http-loopback carve-out (see HttpUtils::isDisallowedNativeUriScheme) + * @return bool + */ + private function isNativeDangerousScheme(string $scheme, ?string $host = null): bool + { + return $this->application_type === IClient::ApplicationType_Native && HttpUtils::isDisallowedNativeUriScheme($scheme, $host); + } + /** * @param string $uri * @return bool @@ -636,6 +651,13 @@ public function isScopeAllowed(string $scope):bool public function isUriAllowed(string $uri):bool { Log::debug(sprintf("Client::isUriAllowed client %s original uri %s", $this->client_id, $uri)); + + $original_parts = @parse_url($uri); + if ($original_parts !== false && isset($original_parts['scheme']) && $this->isNativeDangerousScheme($original_parts['scheme'], $original_parts['host'] ?? null)) { + Log::debug(sprintf("Client::isUriAllowed url %s scheme is not allowed for native client %s", $uri, $this->client_id)); + return false; + } + $uri = URLUtils::canonicalUrl($uri); if(empty($uri)) { Log::debug(sprintf("Client::isUriAllowed url %s is not valid", $uri)); @@ -1097,17 +1119,33 @@ public function isPostLogoutUriAllowed($post_logout_uri) if ($parts == false) { return false; } - if($parts['scheme']!=='https') + // native clients may register custom schemes (myapp://...); every other app type requires https + if($this->application_type !== IClient::ApplicationType_Native && strtolower($parts['scheme'])!=='https') return false; - $logout_without_port = $parts['scheme'].'://'.$parts['host']; + // defense-in-depth: re-check the scheme deny-list at the runtime allow-gate, not just at write time + // (ClientService::assertNativeCustomSchemesAllowed). A row can reach storage through a path other than + // ClientService (e.g. ClientFactory::build() called directly by a seeder or a future write path), so + // the gate that actually authorizes the live 302 redirect must not be the only enforcement point. + if($this->isNativeDangerousScheme($parts['scheme'], $parts['host'] ?? null)) + return false; + + // host-less URIs (e.g. mailto:, file:///x, myapp:///cb) pass FILTER_VALIDATE_URL but have no + // authority to match against; without this guard the concatenation below raises an + // "Undefined array key host" warning (converted to ErrorException) on the public end-session endpoint. + if(!isset($parts['host'])) return false; + + // scheme/host are case-insensitive (RFC 3986); the write path normally lowercases the stored value, + // but match case-insensitively regardless so a bypassing write path can't silently break matching. + $stored_post_logout_uris = strtolower($this->post_logout_redirect_uris); + $logout_without_port = strtolower($parts['scheme'].'://'.$parts['host']); - if(str_contains($this->post_logout_redirect_uris, $logout_without_port )) return true; + if(str_contains($stored_post_logout_uris, $logout_without_port )) return true; if(isset($parts['port'])) { - $logout_with_port = $parts['scheme'].'://'.$parts['host'].':'.$parts['port']; - return str_contains($this->post_logout_redirect_uris, $logout_with_port ); + $logout_with_port = $logout_without_port.':'.$parts['port']; + return str_contains($stored_post_logout_uris, $logout_with_port ); } return false; } diff --git a/app/Repositories/DoctrineOAuth2ClientRepository.php b/app/Repositories/DoctrineOAuth2ClientRepository.php index 2a2e8fe9..6e3c5e3f 100644 --- a/app/Repositories/DoctrineOAuth2ClientRepository.php +++ b/app/Repositories/DoctrineOAuth2ClientRepository.php @@ -163,19 +163,44 @@ public function getByOrigin(string $origin):?Client } /** + * Interception-prevention rule checked across all three URI-bearing fields (redirect_uris, + * post_logout_redirect_uris, allowed_origins): whichever field a scheme was first claimed in, another + * client re-registering it in ANY of the three fields creates the same OS-level scheme-collision risk + * (the OS routes a custom-scheme redirect to whichever installed app claims it, regardless of which + * field of which client this server thinks it belongs to). + * * @param int $id * @param string $custom_scheme * @return bool */ - public function hasCustomSchemeRegisteredForRedirectUrisOnAnotherClientThan(int $id, string $custom_scheme): bool + public function hasCustomSchemeRegisteredOnAnotherClientThan(int $id, string $custom_scheme): bool { - return $this->getEntityManager() - ->createQueryBuilder() + $scheme = trim($custom_scheme); + // fields are comma-separated URI lists; a plain '%scheme://%' substring match false-positives on any + // longer scheme ending in this one (e.g. 'roipapp' matching inside 'androipapp://...'). Anchor the + // match to a real list-item boundary: the scheme starts the field, or immediately follows a comma. + $starts_with = $scheme . '://%'; + $after_comma = '%,' . $scheme . '://%'; + + $qb = $this->getEntityManager()->createQueryBuilder(); + $matches_field = function (string $field) use ($qb) { + return $qb->expr()->orX( + $qb->expr()->like($field, ':starts_with'), + $qb->expr()->like($field, ':after_comma') + ); + }; + + return $qb ->select("count(e.id)") ->from($this->getBaseEntity(), "e") - ->where("e.redirect_uris like :custom_scheme") + ->where($qb->expr()->orX( + $matches_field("e.redirect_uris"), + $matches_field("e.post_logout_redirect_uris"), + $matches_field("e.allowed_origins") + )) ->andWhere("e.id <> :id") - ->setParameter("custom_scheme", '%' . trim($custom_scheme). '://%') + ->setParameter("starts_with", $starts_with) + ->setParameter("after_comma", $after_comma) ->setParameter("id", $id) ->setMaxResults(1) ->getQuery() diff --git a/app/Services/OAuth2/ClientService.php b/app/Services/OAuth2/ClientService.php index 751eed8b..1531bd86 100644 --- a/app/Services/OAuth2/ClientService.php +++ b/app/Services/OAuth2/ClientService.php @@ -220,6 +220,40 @@ public function getCurrentClientAuthInfo() throw new InvalidClientAuthMethodException; } + /** + * Native clients may register genuine custom app URI schemes (e.g. myapp://, com.example.app://) in + * allowed_origins and post_logout_redirect_uris. They may NOT register plain http:// outside the RFC 8252 + * loopback carve-out, nor dangerous/launch pseudo-schemes (javascript:, data:, intent:, ...): at + * end-session these fields become live 302 redirect targets. See HttpUtils::DISALLOWED_NATIVE_URI_SCHEMES + * for the deny-list (shared with the runtime allow-gates in Client::isUriAllowed/isPostLogoutUriAllowed). + * Also enforces the same cross-client scheme-uniqueness rule redirect_uris already has, since a scheme + * claimed by another client here creates the identical OS-level interception risk. + * + * @param array $payload + * @param int $exclude_client_id the client being written; -1 (never a real id) when creating a new one + * @throws ValidationException + */ + private function assertNativeCustomSchemesAllowed(array $payload, int $exclude_client_id = -1): void + { + foreach (['allowed_origins', 'post_logout_redirect_uris'] as $field) { + if (empty($payload[$field])) continue; + foreach (explode(',', $payload[$field]) as $uri) { + $parts = @parse_url(trim($uri)); + if (!isset($parts['scheme'])) { + throw new ValidationException(sprintf('invalid scheme on %s uri.', $field)); + } + $scheme = strtolower($parts['scheme']); + if (HttpUtils::isDisallowedNativeUriScheme($scheme, $parts['host'] ?? null)) { + throw new ValidationException(sprintf('scheme %s:// is not allowed.', $scheme)); + } + if (HttpUtils::isCustomSchema($scheme) + && $this->client_repository->hasCustomSchemeRegisteredOnAnotherClientThan($exclude_client_id, $scheme)) { + throw new ValidationException(sprintf('schema %s:// already registered for another client.', $scheme)); + } + } + } + } + /** * @param array $payload * @return IEntity @@ -238,6 +272,12 @@ public function create(array $payload):IEntity throw new ValidationException('there is already another application with that name, please choose another one.'); } + // close the create-path bypass: the same scheme allow-list update() enforces (only reachable + // for native clients, where the runtime https gate is relaxed). + if (($payload['application_type'] ?? null) === IClient::ApplicationType_Native) { + $this->assertNativeCustomSchemesAllowed($payload); + } + $client = ClientFactory::build($payload); $client = $this->client_credential_generator->generate($client); @@ -307,20 +347,22 @@ public function update(int $id, array $payload):IEntity if (!isset($uri['scheme'])) { throw new ValidationException('invalid scheme on redirect uri.'); } - if (HttpUtils::isCustomSchema($uri['scheme'])) { - if ($this->client_repository->hasCustomSchemeRegisteredForRedirectUrisOnAnotherClientThan($id, $uri['scheme'])) { - throw new ValidationException(sprintf('schema %s:// already registered for another client.', - $uri['scheme'])); - } - } else { - if (!HttpUtils::isHttpSchema($uri['scheme'])) { - throw new ValidationException(sprintf('scheme %s:// is invalid.', - $uri['scheme'])); - } + if (HttpUtils::isDisallowedNativeUriScheme($uri['scheme'], $uri['host'] ?? null)) { + throw new ValidationException(sprintf('scheme %s:// is not allowed.', $uri['scheme'])); + } + // the else branch previously here (rejecting non-http(s) "non-custom" schemes) + // is unreachable now: ftp/file and non-loopback http are already rejected by + // the deny-list check above, and https/loopback-http both satisfy isHttpSchema. + if (HttpUtils::isCustomSchema($uri['scheme']) + && $this->client_repository->hasCustomSchemeRegisteredOnAnotherClientThan($id, $uri['scheme'])) { + throw new ValidationException(sprintf('schema %s:// already registered for another client.', + $uri['scheme'])); } } } } + + $this->assertNativeCustomSchemesAllowed($payload, $id); } break; case IClient::ApplicationType_Web_App: diff --git a/app/libs/OAuth2/Repositories/IClientRepository.php b/app/libs/OAuth2/Repositories/IClientRepository.php index 186829db..1dbd6ddc 100644 --- a/app/libs/OAuth2/Repositories/IClientRepository.php +++ b/app/libs/OAuth2/Repositories/IClientRepository.php @@ -55,5 +55,5 @@ public function getByOrigin(string $origin):?Client; * @param string $custom_scheme * @return bool */ - public function hasCustomSchemeRegisteredForRedirectUrisOnAnotherClientThan(int $id, string $custom_scheme):bool; + public function hasCustomSchemeRegisteredOnAnotherClientThan(int $id, string $custom_scheme):bool; } \ No newline at end of file diff --git a/app/libs/Utils/Http/HttpUtils.php b/app/libs/Utils/Http/HttpUtils.php index ce5acc7d..194bd13f 100644 --- a/app/libs/Utils/Http/HttpUtils.php +++ b/app/libs/Utils/Http/HttpUtils.php @@ -18,6 +18,44 @@ */ final class HttpUtils { + /** + * Schemes native clients may NOT register in redirect_uris / allowed_origins / post_logout_redirect_uris, + * even though they are otherwise allowed to register arbitrary custom app schemes there. https is always + * allowed (checked separately); plain http is handled separately too (see isDisallowedNativeUriScheme - + * RFC 8252 loopback redirection is a carve-out). Every scheme below, once handed to an OS/browser as a + * live redirect target, can trigger an unintended action (script execution, app launch, install prompt, + * local file/content access). Single source of truth for the write-time validator + * (ClientService::assertNativeCustomSchemesAllowed) and the runtime allow-gates (Client::isUriAllowed, + * Client::isPostLogoutUriAllowed). + */ + public const array DISALLOWED_NATIVE_URI_SCHEMES = [ + 'javascript', 'data', 'vbscript', 'intent', 'file', 'ftp', 'blob', 'about', 'mailto', 'tel', + 'itms-services', 'market', 'sms', 'content', 'chrome-extension', 'filesystem', 'view-source', + 'ws', 'wss', 'googlechrome', 'applewebdata', + ]; + + /** + * Loopback hosts exempted from the "plain http is disallowed" rule (RFC 8252 SS7.3): a native app + * receiving its own redirect on 127.0.0.1/::1/localhost never sends the request over the network, so + * there is no TLS downgrade to protect against. + */ + public const array NATIVE_LOOPBACK_HOSTS = ['127.0.0.1', '::1', '[::1]', 'localhost']; + + /** + * @param string $schema + * @param string|null $host present when validating a full URI (e.g. redirect_uris); enables the + * RFC 8252 http-loopback carve-out. Omit when only the scheme is known. + * @return bool + */ + public static function isDisallowedNativeUriScheme(string $schema, ?string $host = null): bool + { + $schema = strtolower($schema); + if ($schema === 'http') { + return !in_array(strtolower((string)$host), self::NATIVE_LOOPBACK_HOSTS); + } + return in_array($schema, self::DISALLOWED_NATIVE_URI_SCHEMES); + } + /** * @param string $schema * @return bool diff --git a/app/libs/Utils/URLUtils.php b/app/libs/Utils/URLUtils.php index e9a2ad74..c3b11b65 100644 --- a/app/libs/Utils/URLUtils.php +++ b/app/libs/Utils/URLUtils.php @@ -40,6 +40,11 @@ public static function canonicalUrl(string $url, bool $usePort = true):?string{ { return null; } + // host-less URIs (e.g. mailto:, file:///x) pass FILTER_VALIDATE_URL but have no authority to + // canonicalize; without this guard the concatenation below raises an "Undefined array key host" warning. + if (!isset($parts['host'])) { + return null; + } $canonical_url = $parts['scheme'].'://'.strtolower($parts['host']); if(isset($parts['port']) && $usePort) { $canonical_url .= ':'.strtolower($parts['port']); diff --git a/resources/js/oauth2/profile/edit_client/components/logout_options.js b/resources/js/oauth2/profile/edit_client/components/logout_options.js index 35502274..80717b42 100644 --- a/resources/js/oauth2/profile/edit_client/components/logout_options.js +++ b/resources/js/oauth2/profile/edit_client/components/logout_options.js @@ -10,10 +10,27 @@ import TagsInput, {getTags} from "../../../../components/tags_input"; import styles from "./common.module.scss"; -const LogoutOptions = ({initialValues, onSavePromise}) => { +// mirrors HttpUtils::$disallowed_native_uri_schemes on the backend; keep the two lists in sync. +const DISALLOWED_NATIVE_SCHEMES = [ + 'http:', 'javascript:', 'data:', 'vbscript:', 'intent:', 'file:', 'ftp:', 'blob:', 'about:', 'mailto:', 'tel:', + 'itms-services:', 'market:', 'sms:', 'content:', 'chrome-extension:', 'filesystem:', 'view-source:', + 'ws:', 'wss:', 'googlechrome:', 'applewebdata:', +]; + +const LogoutOptions = ({appTypes, initialValues, onSavePromise}) => { const [loading, setLoading] = useState(false); const validatePostLogoutRedirectURI = (value) => { + // native clients may register genuine custom app schemes (myapp://...) or https, but not plain http + // nor dangerous/launch pseudo-schemes (javascript:, data:, intent:, ...): matches the backend allow-list. + if (initialValues.application_type === appTypes.Native) { + try { + const protocol = new URL(value).protocol.toLowerCase(); + return protocol === 'https:' || !DISALLOWED_NATIVE_SCHEMES.includes(protocol); + } catch (err) { + return false; + } + } const regex = /^https:\/\/([\w@][\w.:@]+)\/?[\w\.?=%&=\-@/$,]*$/ig; return regex.test(value); } @@ -72,7 +89,6 @@ const LogoutOptions = ({initialValues, onSavePromise}) => { fullWidth size="small" variant="outlined" - type="url" tags={getTags(formik.values.post_logout_redirect_uris)} errors={formik.errors.post_logout_redirect_uris} onChange={formik.handleChange} diff --git a/resources/js/oauth2/profile/edit_client/components/security_settings_panel.js b/resources/js/oauth2/profile/edit_client/components/security_settings_panel.js index fe21fc14..a105ea4f 100644 --- a/resources/js/oauth2/profile/edit_client/components/security_settings_panel.js +++ b/resources/js/oauth2/profile/edit_client/components/security_settings_panel.js @@ -295,7 +295,7 @@ const SecuritySettingsPanel = ( - + ); diff --git a/tests/ClientApiTest.php b/tests/ClientApiTest.php index 3b08ba9d..d79ae490 100644 --- a/tests/ClientApiTest.php +++ b/tests/ClientApiTest.php @@ -96,4 +96,213 @@ public function testCreate(){ $this->assertTrue(isset($json_response->client_id) && !empty($json_response->client_id)); } + public function testUpdateNativeClientAcceptsCustomSchemePostLogoutUrisAndAllowedOrigins(){ + + $client = EntityManager::getRepository(Client::class)->findOneBy(['app_name' => 'oauth2_native_app']); + + $data = array( + 'id' => $client->id, + 'application_type' => IClient::ApplicationType_Native, + 'post_logout_redirect_uris' => 'myapp://callback/logout', + 'allowed_origins' => 'https://web.example.com,myapp://callback', + ); + + $response = $this->action("PUT", "Api\\ClientApiController@update", + $data, + [], + [], + []); + + $this->assertResponseStatus(201); + + $client = EntityManager::getRepository(Client::class)->findOneBy(['app_name' => 'oauth2_native_app']); + $this->assertTrue(str_contains(implode(',', $client->getPostLogoutUris()), 'myapp://callback/logout')); + $this->assertTrue(str_contains($client->getRawClientAllowedOrigins(), 'myapp://callback')); + $this->assertTrue(str_contains($client->getRawClientAllowedOrigins(), 'https://web.example.com')); + } + + public function testUpdateNativeClientRejectsFtpSchemeOnPostLogoutUris(){ + + $client = EntityManager::getRepository(Client::class)->findOneBy(['app_name' => 'oauth2_native_app']); + + $data = array( + 'id' => $client->id, + 'application_type' => IClient::ApplicationType_Native, + 'post_logout_redirect_uris' => 'ftp://foo/bar', + ); + + $response = $this->action("PUT", "Api\\ClientApiController@update", + $data, + [], + [], + []); + + $this->assertResponseStatus(412); + } + + public function testUpdateNativeClientRejectsDangerousAndHttpSchemes(){ + + $client = EntityManager::getRepository(Client::class)->findOneBy(['app_name' => 'oauth2_native_app']); + + foreach (['javascript://x%0aalert(1)', 'data://text/html', 'intent://scan/#Intent;end', 'http://insecure.example.com/cb'] as $bad_uri) { + $data = array( + 'id' => $client->id, + 'application_type' => IClient::ApplicationType_Native, + 'post_logout_redirect_uris' => $bad_uri, + ); + + $response = $this->action("PUT", "Api\\ClientApiController@update", + $data, + [], + [], + []); + + $this->assertResponseStatus(412); + } + } + + public function testUpdateNativeClientRejectsCustomSchemeAlreadyRegisteredByAnotherClient(){ + + $client1 = EntityManager::getRepository(Client::class)->findOneBy(['app_name' => 'oauth2_native_app']); + $client2 = EntityManager::getRepository(Client::class)->findOneBy(['app_name' => 'oauth2_native_app2']); + + $response = $this->action("PUT", "Api\\ClientApiController@update", + array( + 'id' => $client1->id, + 'application_type' => IClient::ApplicationType_Native, + 'post_logout_redirect_uris' => 'sharedscheme://callback/logout', + ), + [], + [], + []); + $this->assertResponseStatus(201); + + $response = $this->action("PUT", "Api\\ClientApiController@update", + array( + 'id' => $client2->id, + 'application_type' => IClient::ApplicationType_Native, + 'allowed_origins' => 'sharedscheme://other', + ), + [], + [], + []); + $this->assertResponseStatus(412); + } + + public function testUpdateNativeClientAllowsSchemeThatIsSubstringOfAnotherClientsScheme(){ + + // oauth2_native_app is seeded with redirect_uris = androipapp://oidc_endpoint_callback (TestSeeder). + // 'roipapp' is a literal substring of 'androipapp', but a DIFFERENT scheme - registering it on another + // client must not be rejected as a collision (a plain '%scheme://%' LIKE would false-positive here). + $client2 = EntityManager::getRepository(Client::class)->findOneBy(['app_name' => 'oauth2_native_app2']); + + $response = $this->action("PUT", "Api\\ClientApiController@update", + array( + 'id' => $client2->id, + 'application_type' => IClient::ApplicationType_Native, + 'allowed_origins' => 'roipapp://cb', + ), + [], + [], + []); + + $this->assertResponseStatus(201); + } + + public function testUpdateNativeClientRejectsDangerousSchemeOnRedirectUris(){ + + $client = EntityManager::getRepository(Client::class)->findOneBy(['app_name' => 'oauth2_native_app']); + + foreach (['javascript://x%0aalert(1)', 'intent://scan/#Intent;end'] as $bad_uri) { + $data = array( + 'id' => $client->id, + 'application_type' => IClient::ApplicationType_Native, + 'redirect_uris' => $bad_uri, + ); + + $response = $this->action("PUT", "Api\\ClientApiController@update", + $data, + [], + [], + []); + + $this->assertResponseStatus(412); + } + } + + public function testUpdateNativeClientRejectsDangerousAndHttpSchemesOnAllowedOrigins(){ + + $client = EntityManager::getRepository(Client::class)->findOneBy(['app_name' => 'oauth2_native_app']); + + foreach (['javascript://x%0aalert(1)', 'intent://scan/#Intent;end', 'itms-services://x/?action=download-manifest', 'http://insecure.example.com'] as $bad_uri) { + $data = array( + 'id' => $client->id, + 'application_type' => IClient::ApplicationType_Native, + 'allowed_origins' => $bad_uri, + ); + + $response = $this->action("PUT", "Api\\ClientApiController@update", + $data, + [], + [], + []); + + $this->assertResponseStatus(412); + } + } + + public function testCreateNativeClientRejectsDangerousSchemeOnPostLogout(){ + + $user = EntityManager::getRepository(User::class)->findOneBy(['identifier' => 'sebastian.marcet']); + + $data = array( + 'user_id' => $user->id, + 'app_name' => 'native_dangerous_scheme_app', + 'app_description' => 'native app with dangerous scheme', + 'application_type' => IClient::ApplicationType_Native, + 'post_logout_redirect_uris' => 'javascript://x%0aalert(1)', + ); + + $response = $this->action("POST", "Api\\ClientApiController@create", + $data, + [], + [], + []); + + $this->assertResponseStatus(412); + } + + public function testUpdateJsClientRejectsCustomSchemeOnPostLogoutUrisAndAllowedOrigins(){ + + $client = EntityManager::getRepository(Client::class)->findOneBy(['app_name' => 'oauth2_test_app_public_2']); + + $data = array( + 'id' => $client->id, + 'application_type' => IClient::ApplicationType_JS_Client, + 'post_logout_redirect_uris' => 'myapp://callback/logout', + ); + + $response = $this->action("PUT", "Api\\ClientApiController@update", + $data, + [], + [], + []); + + $this->assertResponseStatus(412); + + $data = array( + 'id' => $client->id, + 'application_type' => IClient::ApplicationType_JS_Client, + 'allowed_origins' => 'myapp://callback', + ); + + $response = $this->action("PUT", "Api\\ClientApiController@update", + $data, + [], + [], + []); + + $this->assertResponseStatus(412); + } + } \ No newline at end of file diff --git a/tests/UserLoginTurnstileTest.php b/tests/UserLoginTurnstileTest.php index f13b3db8..e525db98 100644 --- a/tests/UserLoginTurnstileTest.php +++ b/tests/UserLoginTurnstileTest.php @@ -41,11 +41,15 @@ final class UserLoginTurnstileTest extends BrowserKitTestCase protected function prepareForTests(): void { parent::prepareForTests(); - $this->testEmail = env('TEST_USER_EMAIL'); - $this->testPassword = env('TEST_USER_PASSWORD'); - if (empty($this->testEmail) || empty($this->testPassword)) { + // read into locals first: assigning null to the typed string properties would + // throw a TypeError before the skip guard below can run + $testEmail = env('TEST_USER_EMAIL'); + $testPassword = env('TEST_USER_PASSWORD'); + if (empty($testEmail) || empty($testPassword)) { $this->markTestSkipped('TEST_USER_EMAIL and TEST_USER_PASSWORD env vars are required.'); } + $this->testEmail = $testEmail; + $this->testPassword = $testPassword; Session::start(); } diff --git a/tests/unit/ClientMappingTest.php b/tests/unit/ClientMappingTest.php index aa827dc5..02c51b7f 100644 --- a/tests/unit/ClientMappingTest.php +++ b/tests/unit/ClientMappingTest.php @@ -24,6 +24,7 @@ use Models\OAuth2\ClientPublicKey; use Models\OAuth2\OAuth2OTP; use Models\OAuth2\ResourceServer; +use OAuth2\Models\IClient; use Tests\BrowserKitTestCase; use Auth\User; @@ -152,4 +153,103 @@ public function testClientPersistence() $this->assertEmpty($found_client->getAdminUsers()->toArray()); $this->assertEmpty($found_client->getClientScopes()); } + + public function testIsPostLogoutUriAllowedNativeClientAcceptsCustomScheme() + { + $client = new Client(); + $client->setApplicationType(IClient::ApplicationType_Native); + $client->setPostLogoutRedirectUris('myapp://callback'); + + $this->assertTrue($client->isPostLogoutUriAllowed('myapp://callback')); + $this->assertFalse($client->isPostLogoutUriAllowed('otherapp://callback')); + } + + public function testIsPostLogoutUriAllowedNativeClientMatchesCaseInsensitiveScheme() + { + // URI schemes are case-insensitive per RFC 3986. The write path (ClientFactory::populate) normally + // lowercases the stored value, but a row can bypass that (same defense-in-depth rationale as the + // dangerous-scheme and host-less-URI checks above) - the runtime match must not silently depend on it. + $client = new Client(); + $client->setApplicationType(IClient::ApplicationType_Native); + $client->setPostLogoutRedirectUris('MyApp://Callback'); + + $this->assertTrue($client->isPostLogoutUriAllowed('myapp://callback')); + } + + public function testIsPostLogoutUriAllowedNonNativeClientRequiresHttps() + { + $client = new Client(); + $client->setApplicationType(IClient::ApplicationType_Web_App); + $client->setPostLogoutRedirectUris('myapp://callback,https://www.test.com'); + + $this->assertFalse($client->isPostLogoutUriAllowed('myapp://callback')); + $this->assertTrue($client->isPostLogoutUriAllowed('https://www.test.com')); + } + + public function testIsPostLogoutUriAllowedNativeClientRejectsHostlessUriWithoutError() + { + // host-less URIs (mailto:, file:///x, myapp:///cb) pass FILTER_VALIDATE_URL but have no authority; + // for a native client the https guard is skipped, so this must not raise an undefined-array-key error. + $client = new Client(); + $client->setApplicationType(IClient::ApplicationType_Native); + $client->setPostLogoutRedirectUris('myapp://callback'); + + $this->assertFalse($client->isPostLogoutUriAllowed('mailto:foo@bar.com')); + $this->assertFalse($client->isPostLogoutUriAllowed('file:///etc/passwd')); + $this->assertFalse($client->isPostLogoutUriAllowed('myapp:///cb')); + } + + public function testIsPostLogoutUriAllowedNativeClientRejectsDangerousSchemeEvenWhenWrittenDirectly() + { + // defense-in-depth: ClientService::assertNativeCustomSchemesAllowed() is the write-time gate, but a row + // can reach storage through a path that bypasses ClientService entirely (e.g. ClientFactory::build() + // called directly by a seeder). isPostLogoutUriAllowed() must independently reject dangerous schemes + // at the runtime allow-gate, not rely solely on write-time validation having run. + $client = new Client(); + $client->setApplicationType(IClient::ApplicationType_Native); + $client->setPostLogoutRedirectUris('javascript://x%0aalert(1),intent://scan/#Intent;end,myapp://callback'); + + $this->assertFalse($client->isPostLogoutUriAllowed('javascript://x%0aalert(1)')); + $this->assertFalse($client->isPostLogoutUriAllowed('intent://scan/#Intent;end')); + $this->assertTrue($client->isPostLogoutUriAllowed('myapp://callback')); + } + + public function testIsUriAllowedNativeClientRejectsDangerousSchemeEvenWhenWrittenDirectly() + { + // same defense-in-depth as isPostLogoutUriAllowed, but for redirect_uris / isUriAllowed: the field + // that actually carries the OAuth2 authorization code, and the more security-critical of the two. + $client = new Client(); + $client->setApplicationType(IClient::ApplicationType_Native); + $client->setRedirectUris('javascript://x%0aalert(1),myapp://callback'); + + $this->assertFalse($client->isUriAllowed('javascript://x%0aalert(1)')); + $this->assertTrue($client->isUriAllowed('myapp://callback')); + } + + public function testIsUriAllowedNativeClientAllowsHttpLoopbackButRejectsHttpElsewhere() + { + // RFC 8252 loopback interface redirection: http://127.0.0.1:{port}/... (or ::1 / localhost) is the + // recommended pattern for native apps and was always allowed pre-existing (Native clients were fully + // exempt from the https-required check). The dangerous-scheme deny-list must carve this out, or every + // native app using the RFC-recommended pattern breaks the moment the deny-list includes 'http'. + $client = new Client(); + $client->setApplicationType(IClient::ApplicationType_Native); + $client->setRedirectUris('http://127.0.0.1:51204/callback,http://localhost:8080/callback'); + + $this->assertTrue($client->isUriAllowed('http://127.0.0.1:51204/callback')); + $this->assertTrue($client->isUriAllowed('http://localhost:8080/callback')); + $this->assertFalse($client->isUriAllowed('http://insecure.example.com/callback')); + } + + public function testIsUriAllowedNativeClientRejectsHostlessUriWithoutError() + { + // canonicalUrl() had the same missing-host crash as isPostLogoutUriAllowed did before that fix; + // isUriAllowed (used by the authorize/token/register/password-reset flows) must not crash either. + $client = new Client(); + $client->setApplicationType(IClient::ApplicationType_Native); + $client->setRedirectUris('myapp://callback'); + + $this->assertFalse($client->isUriAllowed('mailto:foo@bar.com')); + $this->assertFalse($client->isUriAllowed('file:///etc/passwd')); + } } From 20306e3028b15965406aee1cba24fad4c54ab431 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 14 Jul 2026 21:15:32 -0300 Subject: [PATCH 02/14] refactor(oauth2): relocate Native-client scheme deny-list from HttpUtils to IClient/Client; stop duplicating it in the frontend PR review feedback: the scheme deny-list (DISALLOWED_NATIVE_URI_SCHEMES) belonged on IClient (OAuth2 domain policy for Native clients), not HttpUtils (a generic HTTP scheme classifier unrelated to any particular client type). And logout_options.js was hand-duplicating the list in JS with a "keep in sync" comment - an unenforced, easily-drifting contract. - IClient: new DISALLOWED_NATIVE_URI_SCHEMES / NATIVE_LOOPBACK_HOSTS consts (interfaces can't hold method bodies, so the data lives here). - Client: new public static isDisallowedNativeUriScheme() - the predicate that interprets those consts, callable from ClientService without an instantiated entity. isNativeDangerousScheme() now delegates to it. - HttpUtils: reverted to a pure generic scheme classifier (isCustomSchema/isHttpSchema/isHttpsSchema only) - no OAuth2/Native domain knowledge. - AdminController -> edit-client.blade.php -> window.*: the admin UI now reads window.DISALLOWED_NATIVE_URI_SCHEMES / window.NATIVE_LOOPBACK_HOSTS, injected server-side from the IClient constants (same mechanism already used for window.APP_TYPES). logout_options.js has zero hardcoded scheme knowledge left. - Side effect: the frontend never had an RFC 8252 http-loopback carve-out before (the old hardcoded list unconditionally rejected http:); now correctly mirrors the backend's loopback exception. Verified: 154 tests / 0 failures; live browser check confirms window.DISALLOWED_NATIVE_URI_SCHEMES / window.NATIVE_LOOPBACK_HOSTS are populated from IClient and the validator behaves identically (dangerous scheme rejected, loopback http accepted, custom scheme accepted). Plan: docs/plans/2026-07-14-native-clients-custom-schemes.md (Task 8) ADR: docs/adr/0001-native-client-custom-uri-schemes.md --- app/Http/Controllers/AdminController.php | 2 + app/Models/OAuth2/Client.php | 26 ++++++++++--- app/Services/OAuth2/ClientService.php | 6 +-- app/libs/OAuth2/Models/IClient.php | 24 ++++++++++++ app/libs/Utils/Http/HttpUtils.php | 38 ------------------- .../edit_client/components/logout_options.js | 25 +++++++----- .../oauth2/profile/edit-client.blade.php | 4 ++ 7 files changed, 68 insertions(+), 57 deletions(-) diff --git a/app/Http/Controllers/AdminController.php b/app/Http/Controllers/AdminController.php index 3e9904f8..e9e36739 100644 --- a/app/Http/Controllers/AdminController.php +++ b/app/Http/Controllers/AdminController.php @@ -299,6 +299,8 @@ public function editRegisteredClient($id) 'client' => json_encode(SerializerRegistry::getInstance() ->getSerializer($client, SerializerRegistry::SerializerType_Private)->serialize()), 'client_types' => json_encode($client_types), + 'disallowed_native_uri_schemes' => json_encode(IClient::DISALLOWED_NATIVE_URI_SCHEMES), + 'native_loopback_hosts' => json_encode(IClient::NATIVE_LOOPBACK_HOSTS), 'selected_scopes' => json_encode($aux_scopes), 'scopes' => json_encode($final_scopes), 'access_tokens' => $access_tokens->getItems(), diff --git a/app/Models/OAuth2/Client.php b/app/Models/OAuth2/Client.php index cd6be750..271b321b 100644 --- a/app/Models/OAuth2/Client.php +++ b/app/Models/OAuth2/Client.php @@ -14,7 +14,6 @@ use App\libs\Utils\URLUtils; use Auth\User; -use Utils\Http\HttpUtils; use Doctrine\Common\Collections\Criteria; use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Log; @@ -631,17 +630,32 @@ public function isScopeAllowed(string $scope):bool } /** - * Single source of truth for "is this scheme dangerous for a Native client" across the runtime allow-gates - * (isUriAllowed for redirect_uris, isPostLogoutUriAllowed for post_logout_redirect_uris). Delegates the - * actual deny-list to HttpUtils, which ClientService's write-time validation also uses. + * Single source of truth for "is this scheme disallowed for a Native client's URI fields" (redirect_uris, + * allowed_origins, post_logout_redirect_uris). The deny-list itself lives on IClient (domain policy, not a + * generic HTTP concern); this is the one place that interprets it, called by both the write-time validator + * (ClientService) and the runtime allow-gates (isUriAllowed/isPostLogoutUriAllowed below). * * @param string $scheme - * @param string|null $host enables the RFC 8252 http-loopback carve-out (see HttpUtils::isDisallowedNativeUriScheme) + * @param string|null $host enables the RFC 8252 http-loopback carve-out (see IClient::NATIVE_LOOPBACK_HOSTS) + * @return bool + */ + public static function isDisallowedNativeUriScheme(string $scheme, ?string $host = null): bool + { + $scheme = strtolower($scheme); + if ($scheme === 'http') { + return !in_array(strtolower((string)$host), IClient::NATIVE_LOOPBACK_HOSTS); + } + return in_array($scheme, IClient::DISALLOWED_NATIVE_URI_SCHEMES); + } + + /** + * @param string $scheme + * @param string|null $host enables the RFC 8252 http-loopback carve-out (see isDisallowedNativeUriScheme) * @return bool */ private function isNativeDangerousScheme(string $scheme, ?string $host = null): bool { - return $this->application_type === IClient::ApplicationType_Native && HttpUtils::isDisallowedNativeUriScheme($scheme, $host); + return $this->application_type === IClient::ApplicationType_Native && self::isDisallowedNativeUriScheme($scheme, $host); } /** diff --git a/app/Services/OAuth2/ClientService.php b/app/Services/OAuth2/ClientService.php index 1531bd86..16501726 100644 --- a/app/Services/OAuth2/ClientService.php +++ b/app/Services/OAuth2/ClientService.php @@ -224,7 +224,7 @@ public function getCurrentClientAuthInfo() * Native clients may register genuine custom app URI schemes (e.g. myapp://, com.example.app://) in * allowed_origins and post_logout_redirect_uris. They may NOT register plain http:// outside the RFC 8252 * loopback carve-out, nor dangerous/launch pseudo-schemes (javascript:, data:, intent:, ...): at - * end-session these fields become live 302 redirect targets. See HttpUtils::DISALLOWED_NATIVE_URI_SCHEMES + * end-session these fields become live 302 redirect targets. See IClient::DISALLOWED_NATIVE_URI_SCHEMES * for the deny-list (shared with the runtime allow-gates in Client::isUriAllowed/isPostLogoutUriAllowed). * Also enforces the same cross-client scheme-uniqueness rule redirect_uris already has, since a scheme * claimed by another client here creates the identical OS-level interception risk. @@ -243,7 +243,7 @@ private function assertNativeCustomSchemesAllowed(array $payload, int $exclude_c throw new ValidationException(sprintf('invalid scheme on %s uri.', $field)); } $scheme = strtolower($parts['scheme']); - if (HttpUtils::isDisallowedNativeUriScheme($scheme, $parts['host'] ?? null)) { + if (Client::isDisallowedNativeUriScheme($scheme, $parts['host'] ?? null)) { throw new ValidationException(sprintf('scheme %s:// is not allowed.', $scheme)); } if (HttpUtils::isCustomSchema($scheme) @@ -347,7 +347,7 @@ public function update(int $id, array $payload):IEntity if (!isset($uri['scheme'])) { throw new ValidationException('invalid scheme on redirect uri.'); } - if (HttpUtils::isDisallowedNativeUriScheme($uri['scheme'], $uri['host'] ?? null)) { + if (Client::isDisallowedNativeUriScheme($uri['scheme'], $uri['host'] ?? null)) { throw new ValidationException(sprintf('scheme %s:// is not allowed.', $uri['scheme'])); } // the else branch previously here (rejecting non-http(s) "non-custom" schemes) diff --git a/app/libs/OAuth2/Models/IClient.php b/app/libs/OAuth2/Models/IClient.php index fd69a40f..646dfb59 100644 --- a/app/libs/OAuth2/Models/IClient.php +++ b/app/libs/OAuth2/Models/IClient.php @@ -37,6 +37,30 @@ interface IClient extends IEntity const SubjectType_Public = 'public'; const SubjectType_Pairwise = 'pairwise'; + /** + * Schemes Native clients may NOT register in redirect_uris / allowed_origins / post_logout_redirect_uris, + * even though they are otherwise allowed to register arbitrary custom app schemes there. https is always + * allowed (checked separately); plain http is handled separately too (see NATIVE_LOOPBACK_HOSTS - RFC 8252 + * loopback redirection is a carve-out). Every scheme below, once handed to an OS/browser as a live redirect + * target, can trigger an unintended action (script execution, app launch, install prompt, local file/content + * access). Single source of truth for this policy - both the backend write-time validator + * (ClientService::assertNativeCustomSchemesAllowed) / runtime allow-gates (Client::isUriAllowed, + * Client::isPostLogoutUriAllowed) and the admin UI (injected into the edit-client page as + * window.DISALLOWED_NATIVE_URI_SCHEMES - see AdminController::editRegisteredClient) read from here. + */ + const array DISALLOWED_NATIVE_URI_SCHEMES = [ + 'javascript', 'data', 'vbscript', 'intent', 'file', 'ftp', 'blob', 'about', 'mailto', 'tel', + 'itms-services', 'market', 'sms', 'content', 'chrome-extension', 'filesystem', 'view-source', + 'ws', 'wss', 'googlechrome', 'applewebdata', + ]; + + /** + * Loopback hosts exempted from the "plain http is disallowed" rule (RFC 8252 SS7.3): a native app + * receiving its own redirect on 127.0.0.1/::1/localhost never sends the request over the network, so + * there is no TLS downgrade to protect against. + */ + const array NATIVE_LOOPBACK_HOSTS = ['127.0.0.1', '::1', '[::1]', 'localhost']; + /** * @return int */ diff --git a/app/libs/Utils/Http/HttpUtils.php b/app/libs/Utils/Http/HttpUtils.php index 194bd13f..ce5acc7d 100644 --- a/app/libs/Utils/Http/HttpUtils.php +++ b/app/libs/Utils/Http/HttpUtils.php @@ -18,44 +18,6 @@ */ final class HttpUtils { - /** - * Schemes native clients may NOT register in redirect_uris / allowed_origins / post_logout_redirect_uris, - * even though they are otherwise allowed to register arbitrary custom app schemes there. https is always - * allowed (checked separately); plain http is handled separately too (see isDisallowedNativeUriScheme - - * RFC 8252 loopback redirection is a carve-out). Every scheme below, once handed to an OS/browser as a - * live redirect target, can trigger an unintended action (script execution, app launch, install prompt, - * local file/content access). Single source of truth for the write-time validator - * (ClientService::assertNativeCustomSchemesAllowed) and the runtime allow-gates (Client::isUriAllowed, - * Client::isPostLogoutUriAllowed). - */ - public const array DISALLOWED_NATIVE_URI_SCHEMES = [ - 'javascript', 'data', 'vbscript', 'intent', 'file', 'ftp', 'blob', 'about', 'mailto', 'tel', - 'itms-services', 'market', 'sms', 'content', 'chrome-extension', 'filesystem', 'view-source', - 'ws', 'wss', 'googlechrome', 'applewebdata', - ]; - - /** - * Loopback hosts exempted from the "plain http is disallowed" rule (RFC 8252 SS7.3): a native app - * receiving its own redirect on 127.0.0.1/::1/localhost never sends the request over the network, so - * there is no TLS downgrade to protect against. - */ - public const array NATIVE_LOOPBACK_HOSTS = ['127.0.0.1', '::1', '[::1]', 'localhost']; - - /** - * @param string $schema - * @param string|null $host present when validating a full URI (e.g. redirect_uris); enables the - * RFC 8252 http-loopback carve-out. Omit when only the scheme is known. - * @return bool - */ - public static function isDisallowedNativeUriScheme(string $schema, ?string $host = null): bool - { - $schema = strtolower($schema); - if ($schema === 'http') { - return !in_array(strtolower((string)$host), self::NATIVE_LOOPBACK_HOSTS); - } - return in_array($schema, self::DISALLOWED_NATIVE_URI_SCHEMES); - } - /** * @param string $schema * @return bool diff --git a/resources/js/oauth2/profile/edit_client/components/logout_options.js b/resources/js/oauth2/profile/edit_client/components/logout_options.js index 80717b42..02eece45 100644 --- a/resources/js/oauth2/profile/edit_client/components/logout_options.js +++ b/resources/js/oauth2/profile/edit_client/components/logout_options.js @@ -10,23 +10,28 @@ import TagsInput, {getTags} from "../../../../components/tags_input"; import styles from "./common.module.scss"; -// mirrors HttpUtils::$disallowed_native_uri_schemes on the backend; keep the two lists in sync. -const DISALLOWED_NATIVE_SCHEMES = [ - 'http:', 'javascript:', 'data:', 'vbscript:', 'intent:', 'file:', 'ftp:', 'blob:', 'about:', 'mailto:', 'tel:', - 'itms-services:', 'market:', 'sms:', 'content:', 'chrome-extension:', 'filesystem:', 'view-source:', - 'ws:', 'wss:', 'googlechrome:', 'applewebdata:', -]; +// mirrors Client::isDisallowedNativeUriScheme() on the backend: window.DISALLOWED_NATIVE_URI_SCHEMES and +// window.NATIVE_LOOPBACK_HOSTS are injected server-side from IClient::DISALLOWED_NATIVE_URI_SCHEMES / +// IClient::NATIVE_LOOPBACK_HOSTS (see edit-client.blade.php) - the deny-list has one owner, not two. +const isDisallowedNativeUriScheme = (protocol, host) => { + const scheme = protocol.toLowerCase().replace(/:$/, ''); + if (scheme === 'http') { + return !(window.NATIVE_LOOPBACK_HOSTS || []).includes((host || '').toLowerCase()); + } + return (window.DISALLOWED_NATIVE_URI_SCHEMES || []).includes(scheme); +} const LogoutOptions = ({appTypes, initialValues, onSavePromise}) => { const [loading, setLoading] = useState(false); const validatePostLogoutRedirectURI = (value) => { - // native clients may register genuine custom app schemes (myapp://...) or https, but not plain http - // nor dangerous/launch pseudo-schemes (javascript:, data:, intent:, ...): matches the backend allow-list. + // native clients may register genuine custom app schemes (myapp://...), https, or an RFC 8252 + // http loopback redirect, but not plain non-loopback http nor dangerous/launch pseudo-schemes + // (javascript:, data:, intent:, ...): matches the backend deny-list. if (initialValues.application_type === appTypes.Native) { try { - const protocol = new URL(value).protocol.toLowerCase(); - return protocol === 'https:' || !DISALLOWED_NATIVE_SCHEMES.includes(protocol); + const url = new URL(value); + return url.protocol === 'https:' || !isDisallowedNativeUriScheme(url.protocol, url.hostname); } catch (err) { return false; } diff --git a/resources/views/oauth2/profile/edit-client.blade.php b/resources/views/oauth2/profile/edit-client.blade.php index 6372a031..2a68b9ba 100644 --- a/resources/views/oauth2/profile/edit-client.blade.php +++ b/resources/views/oauth2/profile/edit-client.blade.php @@ -35,6 +35,8 @@ const appTypes = {!!$app_types!!}; const clientTypes = {!!$client_types!!}; + const disallowedNativeUriSchemes = {!!$disallowed_native_uri_schemes!!}; + const nativeLoopbackHosts = {!!$native_loopback_hosts!!}; const initialValues = { ...entity, @@ -101,6 +103,8 @@ window.APP_TYPES = appTypes; window.CLIENT_TYPES = clientTypes; + window.DISALLOWED_NATIVE_URI_SCHEMES = disallowedNativeUriSchemes; + window.NATIVE_LOOPBACK_HOSTS = nativeLoopbackHosts; window.CSFR_TOKEN = document.head.querySelector('meta[name="csrf-token"]').content; window.UPDATE_CLIENT_DATA_ENDPOINT = '{!!URL::action("Api\ClientApiController@update",array("id"=>"@client_id"))!!}'; From 580f3fba07936d3b594b631cbb005f2a9770d0d5 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 14 Jul 2026 21:16:14 -0300 Subject: [PATCH 03/14] docs(adr): record ADR-0001 for Native client custom URI scheme support Documents the decision, the security findings from 4 review passes (RFC 8252 loopback carve-out, cross-client scheme uniqueness, defense-in-depth, deny-list vs allow-list trade-off), and the post-review correction to deny-list ownership (IClient/Client instead of HttpUtils; backend-served to the frontend instead of duplicated). --- .../0001-native-client-custom-uri-schemes.md | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 docs/adr/0001-native-client-custom-uri-schemes.md diff --git a/docs/adr/0001-native-client-custom-uri-schemes.md b/docs/adr/0001-native-client-custom-uri-schemes.md new file mode 100644 index 00000000..08d33a91 --- /dev/null +++ b/docs/adr/0001-native-client-custom-uri-schemes.md @@ -0,0 +1,66 @@ +# 1. Native OAuth2 clients: custom URI schemes in redirect_uris, allowed_origins, post_logout_redirect_uris + +Date: 2026-07-14 + +## Status + +Accepted + +## Context + +`ApplicationType_Native` OAuth2 clients (mobile/desktop apps) authenticate end users via the system browser and receive control back through a registered redirect URI. Mobile/desktop platforms commonly use a private-use URI scheme for this (e.g. `myapp://callback`), following [RFC 8252 (OAuth 2.0 for Native Apps)](https://www.rfc-editor.org/rfc/rfc8252). + +Before this change: + +- `redirect_uris` already accepted arbitrary custom schemes for Native clients (`custom_url_set:application_type` validation, `Client::isUriAllowed()` skipping the https requirement for `ApplicationType_Native`). +- `allowed_origins` and `post_logout_redirect_uris` did **not**: both fields used the `ssl_url_set` rule (https-only) for every application type, including Native. A Native client could not register `myapp://` as a post-logout redirect target or as an allowed origin, and the OIDC end-session flow (`OAuth2Protocol::endSession`) would reject a custom-scheme `post_logout_redirect_uri` outright. + +The request was to bring `allowed_origins` and `post_logout_redirect_uris` to parity with the existing `redirect_uris` behavior for Native clients. + +### Security findings during implementation + +Four consecutive adversarial code-review passes (xhigh-effort, multi-agent) surfaced that naively mirroring `redirect_uris`' existing "any scheme with a valid URI" behavior was itself unsafe, and that extending validation always to more fields exposed both new and **pre-existing** issues: + +1. **Dangerous/launch pseudo-schemes.** A field that becomes a live `302` redirect target at the public `/oauth2/end-session` endpoint must not accept schemes like `javascript:`, `data:`, `intent:`, `itms-services:`, etc. — on Android, `intent://` can launch arbitrary app activities; on iOS/Safari, `itms-services://` can trigger a silent app-install prompt; `javascript:`/`data:` are stored open-redirect/script vectors. This applied equally to the pre-existing `redirect_uris` behavior once traced (not merely the two new fields), since `redirect_uris` is the field that actually carries the OAuth2 authorization code — the more security-critical of the three. +2. **RFC 8252 loopback interface redirection.** Native clients commonly use `http://127.0.0.1:{port}/callback` (or `localhost`) — TLS is meaningless here since the redirect never leaves the device. A blanket "reject `http`" rule breaks this standard, already-deployed pattern. +3. **Cross-client scheme collision.** Two different OAuth2 clients registered with the same IDP claiming the identical custom scheme creates an OS-level interception ambiguity (whichever installed app claims the scheme at the OS level receives the redirect). `redirect_uris` already had a uniqueness check for this (`hasCustomSchemeRegisteredForRedirectUrisOnAnotherClientThan`); the two new fields did not. +4. **Defense-in-depth.** A row can reach storage via `ClientFactory::build()` directly (e.g. seeders, future write paths) without ever passing through `ClientService`'s write-time validation. The runtime allow-gates (`Client::isUriAllowed`, `Client::isPostLogoutUriAllowed`) must not depend solely on write-time validation having run. +5. **Latent crash bug.** `URLUtils::canonicalUrl()` / `Client::isPostLogoutUriAllowed()` concatenated `$parts['host']` without checking it was set. A host-less but otherwise valid URI (`mailto:foo@bar.com`, `file:///etc/passwd`) passes `FILTER_VALIDATE_URL` but has no `host` component — this raised an uncaught `ErrorException` (HTTP 500 with a leaked internal message) on the public end-session endpoint once Native clients were allowed non-https schemes there. +6. **Substring false-positive.** The original cross-client scheme-uniqueness query (`LIKE '%scheme://%'`) matched a shorter scheme as a substring of an unrelated longer one already registered elsewhere (e.g. `roipapp://` matching inside `androipapp://oidc_endpoint_callback`), a pre-existing bug whose blast radius widened once the check was generalized across three fields and reachable from `create()`. + +## Decision + +1. **Allow custom app URI schemes in all three URI-bearing Native-client fields** (`redirect_uris`, `allowed_origins`, `post_logout_redirect_uris`), gated by a **deny-list**, not an allow-list — any scheme is treated as a legitimate custom app scheme unless it appears on `IClient::DISALLOWED_NATIVE_URI_SCHEMES`. +2. **Single source of truth for the deny-list policy, owned by the OAuth2 domain layer, not a generic HTTP helper.** The deny-list and loopback-host list are `const` arrays on `IClient` (domain policy for Native OAuth2 clients — the same interface already holding `ApplicationType_Native`, `ClientType_Confidential`, etc.). Since PHP interfaces can't hold method bodies, the predicate that interprets them (`isDisallowedNativeUriScheme(string $scheme, ?string $host = null): bool`) is a `public static` method on `Client`, the concrete entity. Both the write-time validator (`ClientService::assertNativeCustomSchemesAllowed()`, and the `redirect_uris` validation branch in `ClientService::update()`) and the runtime allow-gates (`Client::isUriAllowed()`, `Client::isPostLogoutUriAllowed()`, via a shared `Client::isNativeDangerousScheme()` helper) call this one method. The admin UI reads the same two lists at runtime instead of hand-duplicating them in JavaScript: `AdminController` passes `IClient::DISALLOWED_NATIVE_URI_SCHEMES`/`IClient::NATIVE_LOOPBACK_HOSTS` to the edit-client view, which injects them as `window.DISALLOWED_NATIVE_URI_SCHEMES`/`window.NATIVE_LOOPBACK_HOSTS` (the same mechanism already used for `window.APP_TYPES`); `logout_options.js`'s inline validator reads from `window.*` rather than maintaining its own copy. *(This constant/method placement was revised once, after initial review placed the deny-list on the generic `Utils\Http\HttpUtils` class — see Consequences.)* +3. **`http` is a special case with an RFC 8252 loopback carve-out**: disallowed everywhere except `127.0.0.1` / `::1` / `localhost` (`IClient::NATIVE_LOOPBACK_HOSTS`). +4. **Cross-client scheme uniqueness** (`IClientRepository::hasCustomSchemeRegisteredOnAnotherClientThan`) checks all three URI columns together — a scheme claimed by another client in *any* of the three fields blocks re-registration in any of the three, since the OS-level interception risk is identical regardless of which field either client used. The query anchors matches to real list-item boundaries (start-of-field or immediately after a comma) rather than an unanchored substring `LIKE`. +5. **Defense-in-depth**: the runtime allow-gates independently re-check the scheme deny-list; write-time validation is not the sole enforcement point. +6. **Enforced on both write paths** (`create()` and `update()`) for `allowed_origins`/`post_logout_redirect_uris`. `redirect_uris` scheme validation remains `update()`-only, matching its pre-existing (unchanged) behavior — `create()` never validated `redirect_uris` at all, before or after this change (see Consequences). +7. **The `allowed_origins` admin UI input stays hidden for Native clients.** No runtime path enforces `allowed_origins` for Native today — both the IDP's own `OAuth2BearerAccessTokenRequestValidator` middleware and summit-api's equivalent gate the origin check to `application_type === JS_Client`. The field remains settable via the admin API only (the value ships in token-introspection responses and may be enforced by a resource server in the future), but exposing a UI control for a value nothing currently checks was judged not worth the surface. + +### Alternatives considered + +- **Exact parity with `redirect_uris`'s pre-existing behavior** (any scheme, no deny-list) — rejected: this is what the first adversarial review pass demonstrated was unsafe once the field becomes a live redirect target for two more fields. +- **Allow-list instead of deny-list** (only permit a known-safe pattern, e.g. reverse-DNS custom schemes + https + loopback http) — rejected for this change as materially larger in scope than requested; the deny-list's non-exhaustiveness is accepted as a structural trade-off (see Consequences). +- **Leave `redirect_uris` untouched, harden only the two new fields** — initially chosen, then reversed once review showed `redirect_uris` (the field carrying the actual authorization code) had the identical, more consequential gap. + +## Consequences + +**Enabled:** +- Native clients can complete RP-initiated logout with a custom-scheme `post_logout_redirect_uri` end-to-end (verified live: registered scheme → `302` redirect; unregistered/dangerous scheme → clean `400`). +- Native clients can register `allowed_origins` values via the admin API (inert today — no runtime consumer for Native — but available for a future resource-server enforcement path without another migration). +- `redirect_uris`, `allowed_origins`, and `post_logout_redirect_uris` share one scheme-safety policy instead of three divergent ones. +- The admin UI has zero hardcoded scheme knowledge — the deny-list and loopback-host list are backend-authoritative and injected at render time, so a future policy change (e.g. adding a scheme to the deny-list) automatically applies client-side with no JS edit required. + +**Corrected during review (not a trade-off — fixed before merge):** +- The deny-list/loopback-host constants were initially placed on `Utils\Http\HttpUtils` (a generic scheme-classification helper) and hand-duplicated as a second literal array in `logout_options.js`. Both were flagged in PR review: the frontend duplication as an unmaintained "keep in sync" liability, and the `HttpUtils` placement as the wrong dependency direction (a generic utility class encoding OAuth2-Native-client domain policy). Relocated to `IClient` (data) + `Client` (predicate logic) and wired the admin UI to read the values from the backend at render time instead of maintaining its own copy. + +**Accepted trade-offs (not fixed in this change, documented for a future pass if warranted):** +- `ClientService::create()` still never validates `redirect_uris` scheme or cross-client uniqueness at all (a pre-existing gap, unrelated to the deny-list itself) — mitigated by the runtime allow-gate rejecting a dangerous scheme regardless of how it reached storage, but write-time hygiene on that one path remains weaker than `update()`. +- The deny-list can never be exhaustive against every OS/browser/app-launcher scheme that might exist now or in the future (a structural property of any blocklist). A `search-ms://`-style scheme not yet on the list would be accepted. Closing this fully requires an allow-list architecture, a larger change than this ADR's scope. + +## References + +- Implementation plan: `docs/plans/2026-07-14-native-clients-custom-schemes.md` +- Commit: `844328c6` on branch `hotfix/native-app-custom-schemas` +- [RFC 8252 — OAuth 2.0 for Native Apps](https://www.rfc-editor.org/rfc/rfc8252) From 666887340340cd24c41b2ea93b79fb1ef65ba610 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 14 Jul 2026 21:30:05 -0300 Subject: [PATCH 04/14] fix(oauth2): validate redirect_uris scheme/uniqueness on client create, not just update CodeRabbit PR review (#147): ClientService::create() validated allowed_origins/post_logout_redirect_uris for Native clients but never redirect_uris - a create payload could register a dangerous scheme (javascript://, etc.) or a scheme already claimed by another client through redirect_uris, only ever caught later by the runtime isUriAllowed() gate rather than at write time. assertNativeCustomSchemesAllowed() already ran both the deny-list check and the cross-client uniqueness check generically per field (since the earlier hardening pass); extending its field list to include redirect_uris closes the gap for both create() and update() via one shared method. This also let update()'s separate, by-then fully-duplicate inline redirect_uris validation loop be deleted. Verified: 156 tests / 0 failures. Plan: docs/plans/2026-07-14-native-clients-custom-schemes.md (Task 9) ADR: docs/adr/0001-native-client-custom-uri-schemes.md --- app/Services/OAuth2/ClientService.php | 47 +++++----------- .../0001-native-client-custom-uri-schemes.md | 2 +- tests/ClientApiTest.php | 56 +++++++++++++++++++ 3 files changed, 70 insertions(+), 35 deletions(-) diff --git a/app/Services/OAuth2/ClientService.php b/app/Services/OAuth2/ClientService.php index 16501726..f0cf41e4 100644 --- a/app/Services/OAuth2/ClientService.php +++ b/app/Services/OAuth2/ClientService.php @@ -222,12 +222,13 @@ public function getCurrentClientAuthInfo() /** * Native clients may register genuine custom app URI schemes (e.g. myapp://, com.example.app://) in - * allowed_origins and post_logout_redirect_uris. They may NOT register plain http:// outside the RFC 8252 - * loopback carve-out, nor dangerous/launch pseudo-schemes (javascript:, data:, intent:, ...): at - * end-session these fields become live 302 redirect targets. See IClient::DISALLOWED_NATIVE_URI_SCHEMES - * for the deny-list (shared with the runtime allow-gates in Client::isUriAllowed/isPostLogoutUriAllowed). - * Also enforces the same cross-client scheme-uniqueness rule redirect_uris already has, since a scheme - * claimed by another client here creates the identical OS-level interception risk. + * redirect_uris, allowed_origins, and post_logout_redirect_uris. They may NOT register plain http:// + * outside the RFC 8252 loopback carve-out, nor dangerous/launch pseudo-schemes (javascript:, data:, + * intent:, ...): redirect_uris carries the authorization code, and the other two fields become live + * 302 redirect targets at end-session. See IClient::DISALLOWED_NATIVE_URI_SCHEMES for the deny-list + * (shared with the runtime allow-gates in Client::isUriAllowed/isPostLogoutUriAllowed). Also enforces + * cross-client scheme uniqueness: a scheme claimed by another client in any of these fields creates the + * same OS-level interception risk regardless of which field either client used it in. * * @param array $payload * @param int $exclude_client_id the client being written; -1 (never a real id) when creating a new one @@ -235,7 +236,7 @@ public function getCurrentClientAuthInfo() */ private function assertNativeCustomSchemesAllowed(array $payload, int $exclude_client_id = -1): void { - foreach (['allowed_origins', 'post_logout_redirect_uris'] as $field) { + foreach (['redirect_uris', 'allowed_origins', 'post_logout_redirect_uris'] as $field) { if (empty($payload[$field])) continue; foreach (explode(',', $payload[$field]) as $uri) { $parts = @parse_url(trim($uri)); @@ -272,8 +273,8 @@ public function create(array $payload):IEntity throw new ValidationException('there is already another application with that name, please choose another one.'); } - // close the create-path bypass: the same scheme allow-list update() enforces (only reachable - // for native clients, where the runtime https gate is relaxed). + // same scheme deny-list + cross-client uniqueness rule update() enforces (only reachable + // for native clients, where the runtime https gate is relaxed) - now covers redirect_uris too. if (($payload['application_type'] ?? null) === IClient::ApplicationType_Native) { $this->assertNativeCustomSchemesAllowed($payload); } @@ -337,31 +338,9 @@ public function update(int $id, array $payload):IEntity // validate uris switch($client->getApplicationType()) { case IClient::ApplicationType_Native: { - - if (isset($payload['redirect_uris'])) { - $redirect_uris = explode(',', $payload['redirect_uris']); - //check that custom schema does not already exists for another registerd app - if (!empty($payload['redirect_uris'])) { - foreach ($redirect_uris as $uri) { - $uri = @parse_url($uri); - if (!isset($uri['scheme'])) { - throw new ValidationException('invalid scheme on redirect uri.'); - } - if (Client::isDisallowedNativeUriScheme($uri['scheme'], $uri['host'] ?? null)) { - throw new ValidationException(sprintf('scheme %s:// is not allowed.', $uri['scheme'])); - } - // the else branch previously here (rejecting non-http(s) "non-custom" schemes) - // is unreachable now: ftp/file and non-loopback http are already rejected by - // the deny-list check above, and https/loopback-http both satisfy isHttpSchema. - if (HttpUtils::isCustomSchema($uri['scheme']) - && $this->client_repository->hasCustomSchemeRegisteredOnAnotherClientThan($id, $uri['scheme'])) { - throw new ValidationException(sprintf('schema %s:// already registered for another client.', - $uri['scheme'])); - } - } - } - } - + // redirect_uris, allowed_origins, and post_logout_redirect_uris all share the same + // scheme deny-list + cross-client uniqueness rule; assertNativeCustomSchemesAllowed + // validates whichever of the three are present in the payload. $this->assertNativeCustomSchemesAllowed($payload, $id); } break; diff --git a/docs/adr/0001-native-client-custom-uri-schemes.md b/docs/adr/0001-native-client-custom-uri-schemes.md index 08d33a91..28fe8701 100644 --- a/docs/adr/0001-native-client-custom-uri-schemes.md +++ b/docs/adr/0001-native-client-custom-uri-schemes.md @@ -54,9 +54,9 @@ Four consecutive adversarial code-review passes (xhigh-effort, multi-agent) surf **Corrected during review (not a trade-off — fixed before merge):** - The deny-list/loopback-host constants were initially placed on `Utils\Http\HttpUtils` (a generic scheme-classification helper) and hand-duplicated as a second literal array in `logout_options.js`. Both were flagged in PR review: the frontend duplication as an unmaintained "keep in sync" liability, and the `HttpUtils` placement as the wrong dependency direction (a generic utility class encoding OAuth2-Native-client domain policy). Relocated to `IClient` (data) + `Client` (predicate logic) and wired the admin UI to read the values from the backend at render time instead of maintaining its own copy. +- `ClientService::create()` initially never validated `redirect_uris` scheme or cross-client uniqueness at all — originally accepted as a trade-off (mitigated by the runtime allow-gate). CodeRabbit's automated PR review re-flagged it; by that point `assertNativeCustomSchemesAllowed()` already did both checks generically per field, so closing the gap was a two-line change (add `redirect_uris` to its field list) that additionally let `update()`'s separate, now-fully-duplicate inline `redirect_uris` loop be deleted (~25 lines). No longer a trade-off — `create()` and `update()` now enforce identically for all three fields via one shared method. **Accepted trade-offs (not fixed in this change, documented for a future pass if warranted):** -- `ClientService::create()` still never validates `redirect_uris` scheme or cross-client uniqueness at all (a pre-existing gap, unrelated to the deny-list itself) — mitigated by the runtime allow-gate rejecting a dangerous scheme regardless of how it reached storage, but write-time hygiene on that one path remains weaker than `update()`. - The deny-list can never be exhaustive against every OS/browser/app-launcher scheme that might exist now or in the future (a structural property of any blocklist). A `search-ms://`-style scheme not yet on the list would be accepted. Closing this fully requires an allow-list architecture, a larger change than this ADR's scope. ## References diff --git a/tests/ClientApiTest.php b/tests/ClientApiTest.php index d79ae490..2fefa96a 100644 --- a/tests/ClientApiTest.php +++ b/tests/ClientApiTest.php @@ -272,6 +272,62 @@ public function testCreateNativeClientRejectsDangerousSchemeOnPostLogout(){ $this->assertResponseStatus(412); } + public function testCreateNativeClientRejectsDangerousSchemeOnRedirectUris(){ + + // CodeRabbit PR #147 finding: create() validated allowed_origins/post_logout_redirect_uris for + // dangerous schemes but never redirect_uris - a create payload could register javascript:// etc. + // there and it would only ever be caught later by the runtime isUriAllowed() gate, not at write time. + $user = EntityManager::getRepository(User::class)->findOneBy(['identifier' => 'sebastian.marcet']); + + $data = array( + 'user_id' => $user->id, + 'app_name' => 'native_dangerous_redirect_uri_app', + 'app_description' => 'native app with dangerous scheme on redirect_uris', + 'application_type' => IClient::ApplicationType_Native, + 'redirect_uris' => 'javascript://x%0aalert(1)', + ); + + $response = $this->action("POST", "Api\\ClientApiController@create", + $data, + [], + [], + []); + + $this->assertResponseStatus(412); + } + + public function testCreateNativeClientRejectsRedirectUriSchemeAlreadyRegisteredByAnotherClient(){ + + // CodeRabbit PR #147 finding, cross-client uniqueness half: create() never checked redirect_uris + // scheme collisions either (only update() did, via a separate now-removed inline loop). + $existing = EntityManager::getRepository(Client::class)->findOneBy(['app_name' => 'oauth2_native_app']); + $response = $this->action("PUT", "Api\\ClientApiController@update", + array( + 'id' => $existing->id, + 'application_type' => IClient::ApplicationType_Native, + 'redirect_uris' => 'createuniqueness://callback', + ), + [], + [], + []); + $this->assertResponseStatus(201); + + $user = EntityManager::getRepository(User::class)->findOneBy(['identifier' => 'sebastian.marcet']); + $response = $this->action("POST", "Api\\ClientApiController@create", + array( + 'user_id' => $user->id, + 'app_name' => 'native_duplicate_redirect_scheme_app', + 'app_description' => 'native app registering an already-claimed redirect_uris scheme', + 'application_type' => IClient::ApplicationType_Native, + 'redirect_uris' => 'createuniqueness://other', + ), + [], + [], + []); + + $this->assertResponseStatus(412); + } + public function testUpdateJsClientRejectsCustomSchemeOnPostLogoutUrisAndAllowedOrigins(){ $client = EntityManager::getRepository(Client::class)->findOneBy(['app_name' => 'oauth2_test_app_public_2']); From a0e83b103b497caffa172d29c073627b4d7c9296 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 14 Jul 2026 22:17:32 -0300 Subject: [PATCH 05/14] fix(oauth2): validate custom URI scheme lists on client create() getCreatePayloadValidationRules() did not declare redirect_uris, allowed_origins, or post_logout_redirect_uris, so create() applied zero request-level validation to them. Two consequences: - A list with a space after the comma (e.g. "https://a.com, myapp://cb") reached storage verbatim (ClientFactory::populate only trims the whole payload string, not each item), silently defeating hasCustomSchemeRegisteredOnAnotherClientThan()'s comma-boundary anchored match and letting a second client register the same custom scheme unopposed. - A non-string value (e.g. a JSON array) reached assertNativeCustomSchemesAllowed() untouched, where explode() threw an uncaught TypeError (not an Exception) instead of a clean 412. Add the same custom_url_set:application_type rules update() already uses to getCreatePayloadValidationRules(), closing both gaps at the shared root cause. Also harden CustomValidator::validateCustomUrlSet() with an is_string() guard, since Laravel invokes the rule callback with the raw value regardless of rule order, so declaring the rule alone doesn't stop the TypeError. Regression tests added first (confirmed red before the fix): testCreateNativeClientRejectsCustomSchemeWithLeadingSpaceInList and testCreateNativeClientRejectsArrayValueForRedirectUrisCleanly. Full suite: 158 tests, 527 assertions, 0 failures. --- .../Controllers/Api/ClientApiController.php | 13 +++-- app/Validators/CustomValidator.php | 2 + tests/ClientApiTest.php | 52 +++++++++++++++++++ 3 files changed, 62 insertions(+), 5 deletions(-) diff --git a/app/Http/Controllers/Api/ClientApiController.php b/app/Http/Controllers/Api/ClientApiController.php index 9d8c347c..a507143a 100644 --- a/app/Http/Controllers/Api/ClientApiController.php +++ b/app/Http/Controllers/Api/ClientApiController.php @@ -731,11 +731,14 @@ protected function getUpdatePayloadValidationRules(): array protected function getCreatePayloadValidationRules(): array { return [ - 'app_name' => 'required|freetext|max:255', - 'app_description' => 'required|freetext|max:512', - 'application_type' => 'required|applicationtype', - 'website' => 'nullable|url', - 'admin_users' => 'nullable|int_array', + 'app_name' => 'required|freetext|max:255', + 'app_description' => 'required|freetext|max:512', + 'application_type' => 'required|applicationtype', + 'website' => 'nullable|url', + 'admin_users' => 'nullable|int_array', + 'redirect_uris' => 'nullable|string|custom_url_set:application_type', + 'post_logout_redirect_uris' => 'nullable|string|custom_url_set:application_type', + 'allowed_origins' => 'nullable|string|custom_url_set:application_type', ]; } diff --git a/app/Validators/CustomValidator.php b/app/Validators/CustomValidator.php index 0abde995..7012f4a8 100644 --- a/app/Validators/CustomValidator.php +++ b/app/Validators/CustomValidator.php @@ -294,6 +294,8 @@ public function validatePrivateKeyPassword($attribute, $value, $parameters){ public function validateCustomUrlSet($attribute, $value, $parameters) { + if (!is_string($value)) return false; + $app_type_param = $parameters[0]; if(!isset($this->data[$app_type_param])) return true; $app_type = $this->data[$app_type_param]; diff --git a/tests/ClientApiTest.php b/tests/ClientApiTest.php index 2fefa96a..7537b7fd 100644 --- a/tests/ClientApiTest.php +++ b/tests/ClientApiTest.php @@ -361,4 +361,56 @@ public function testUpdateJsClientRejectsCustomSchemeOnPostLogoutUrisAndAllowedO $this->assertResponseStatus(412); } + public function testCreateNativeClientRejectsCustomSchemeWithLeadingSpaceInList(){ + + // Regression: hasCustomSchemeRegisteredOnAnotherClientThan() anchors matches to "starts the + // field" or "immediately follows a comma" with no tolerance for whitespace, and (before this + // fix) create() applied zero request-level validation to redirect_uris/allowed_origins/ + // post_logout_redirect_uris (absent from getCreatePayloadValidationRules()), so a list with a + // space after the comma reached storage verbatim - silently defeating the cross-client scheme + // uniqueness check for that entry. Now that create() validates these fields with the same + // custom_url_set rule update() already used, the malformed list is rejected outright before it + // can ever reach storage. + $user = EntityManager::getRepository(User::class)->findOneBy(['identifier' => 'sebastian.marcet']); + + $response = $this->action("POST", "Api\\ClientApiController@create", + array( + 'user_id' => $user->id, + 'app_name' => 'native_wspacebypass_app', + 'app_description' => 'native app sending a list with a space after the comma', + 'application_type' => IClient::ApplicationType_Native, + 'post_logout_redirect_uris' => 'https://web.example.com/logout, wspacebypass://callback/logout', + ), + [], + [], + []); + + $this->assertResponseStatus(412); + } + + public function testCreateNativeClientRejectsArrayValueForRedirectUrisCleanly(){ + + // Regression: getCreatePayloadValidationRules() does not declare redirect_uris/allowed_origins/ + // post_logout_redirect_uris at all, so create() applies zero request-level validation to them - + // any value, of any PHP type, reaches ClientService::create() untouched. There, + // assertNativeCustomSchemesAllowed() calls explode(',', $payload[$field]); explode() requires a + // string and throws a TypeError (not an Exception) on an array, which escapes every catch block in + // APICRUDController::create() and surfaces as a generic 500 instead of the intended 412. + $user = EntityManager::getRepository(User::class)->findOneBy(['identifier' => 'sebastian.marcet']); + + $response = $this->action("POST", "Api\\ClientApiController@create", + array( + 'user_id' => $user->id, + 'app_name' => 'native_array_redirect_uri_app', + 'app_description' => 'native app sending a non-string redirect_uris value', + 'application_type' => IClient::ApplicationType_Native, + 'redirect_uris' => ['myapp://callback', 'otherapp://callback'], + ), + [], + [], + []); + + $this->assertResponseStatus(412); + } + } \ No newline at end of file From bc5d1187c40b20c761dfaa6fec0d6afd3b31da63 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 14 Jul 2026 23:38:08 -0300 Subject: [PATCH 06/14] fix(oauth2): exact-match redirect_uris, declare scheme predicate on IClient isUriAllowed() matched a registered redirect_uri via str_contains() against the requested URI - a prefix/substring check, not an exact match. Registering "myapp://callback" therefore also permitted "myapp://callback/": any path appended after the registered value passed, on the field that carries the OAuth2 authorization code. Both sides now go through the same canonicalUrl()+normalizeUrl() pipeline and are compared with strict equality; query strings remain tolerated exactly as before (canonicalUrl already drops them from both sides). Also: - Declare isDisallowedNativeUriScheme() on IClient alongside the constants it interprets, matching the interface-first convention every other predicate on Client already follows. - Add a regression test for the path-suffix bypass. - Correct ADR 0001 decision item 6, which still claimed create() never validates redirect_uris - stale relative to its own Consequences section and the actual assertNativeCustomSchemesAllowed() field list. --- app/Models/OAuth2/Client.php | 16 +++++++++++++--- app/libs/OAuth2/Models/IClient.php | 12 ++++++++++++ .../0001-native-client-custom-uri-schemes.md | 2 +- tests/unit/ClientMappingTest.php | 17 +++++++++++++++++ 4 files changed, 43 insertions(+), 4 deletions(-) diff --git a/app/Models/OAuth2/Client.php b/app/Models/OAuth2/Client.php index 271b321b..0663ea28 100644 --- a/app/Models/OAuth2/Client.php +++ b/app/Models/OAuth2/Client.php @@ -687,13 +687,23 @@ public function isUriAllowed(string $uri):bool return false; } - $redirect_uris = explode(',',strtolower($this->redirect_uris)); + $redirect_uris = explode(',', $this->redirect_uris); $uri = URLUtils::normalizeUrl($uri); if(empty($uri)) return false; foreach($redirect_uris as $redirect_uri){ + $redirect_uri = trim($redirect_uri); if(empty($redirect_uri)) continue; - Log::debug(sprintf("Client::isUriAllowed url %s client %s redirect_uri %s", $uri, $this->client_id, $redirect_uri)); - if(str_contains($uri, $redirect_uri)) + + // symmetric normalization: compare both sides through the same canonicalize+normalize + // pipeline, then require an exact match - a registered value must no longer be accepted + // merely as a *prefix* of the requested URI (e.g. "myapp://callback" matching any + // "myapp://callback/"). + $canonical_redirect_uri = URLUtils::canonicalUrl($redirect_uri); + if(empty($canonical_redirect_uri)) continue; + $canonical_redirect_uri = URLUtils::normalizeUrl($canonical_redirect_uri); + + Log::debug(sprintf("Client::isUriAllowed url %s client %s redirect_uri %s", $uri, $this->client_id, $canonical_redirect_uri)); + if($uri === $canonical_redirect_uri) return true; } diff --git a/app/libs/OAuth2/Models/IClient.php b/app/libs/OAuth2/Models/IClient.php index 646dfb59..0fdf991c 100644 --- a/app/libs/OAuth2/Models/IClient.php +++ b/app/libs/OAuth2/Models/IClient.php @@ -61,6 +61,18 @@ interface IClient extends IEntity */ const array NATIVE_LOOPBACK_HOSTS = ['127.0.0.1', '::1', '[::1]', 'localhost']; + /** + * Single source of truth for "is this scheme disallowed for a Native client's URI fields", per + * DISALLOWED_NATIVE_URI_SCHEMES / NATIVE_LOOPBACK_HOSTS above. Declared here (contract) and + * implemented on Client (body) like every other predicate on this interface; kept static because + * ClientService::create() must validate a scheme before a Client entity exists to call it on. + * + * @param string $scheme + * @param string|null $host enables the RFC 8252 http-loopback carve-out (see NATIVE_LOOPBACK_HOSTS) + * @return bool + */ + public static function isDisallowedNativeUriScheme(string $scheme, ?string $host = null): bool; + /** * @return int */ diff --git a/docs/adr/0001-native-client-custom-uri-schemes.md b/docs/adr/0001-native-client-custom-uri-schemes.md index 28fe8701..fd2c09b9 100644 --- a/docs/adr/0001-native-client-custom-uri-schemes.md +++ b/docs/adr/0001-native-client-custom-uri-schemes.md @@ -35,7 +35,7 @@ Four consecutive adversarial code-review passes (xhigh-effort, multi-agent) surf 3. **`http` is a special case with an RFC 8252 loopback carve-out**: disallowed everywhere except `127.0.0.1` / `::1` / `localhost` (`IClient::NATIVE_LOOPBACK_HOSTS`). 4. **Cross-client scheme uniqueness** (`IClientRepository::hasCustomSchemeRegisteredOnAnotherClientThan`) checks all three URI columns together — a scheme claimed by another client in *any* of the three fields blocks re-registration in any of the three, since the OS-level interception risk is identical regardless of which field either client used. The query anchors matches to real list-item boundaries (start-of-field or immediately after a comma) rather than an unanchored substring `LIKE`. 5. **Defense-in-depth**: the runtime allow-gates independently re-check the scheme deny-list; write-time validation is not the sole enforcement point. -6. **Enforced on both write paths** (`create()` and `update()`) for `allowed_origins`/`post_logout_redirect_uris`. `redirect_uris` scheme validation remains `update()`-only, matching its pre-existing (unchanged) behavior — `create()` never validated `redirect_uris` at all, before or after this change (see Consequences). +6. **Enforced on both write paths** (`create()` and `update()`) for all three fields, including `redirect_uris`. `redirect_uris` initially had no request-level validation in `create()` at all — closed during review (see Consequences) by adding it to the same `assertNativeCustomSchemesAllowed()` field loop already used for the other two fields. 7. **The `allowed_origins` admin UI input stays hidden for Native clients.** No runtime path enforces `allowed_origins` for Native today — both the IDP's own `OAuth2BearerAccessTokenRequestValidator` middleware and summit-api's equivalent gate the origin check to `application_type === JS_Client`. The field remains settable via the admin API only (the value ships in token-introspection responses and may be enforced by a resource server in the future), but exposing a UI control for a value nothing currently checks was judged not worth the surface. ### Alternatives considered diff --git a/tests/unit/ClientMappingTest.php b/tests/unit/ClientMappingTest.php index 02c51b7f..f6b105d2 100644 --- a/tests/unit/ClientMappingTest.php +++ b/tests/unit/ClientMappingTest.php @@ -252,4 +252,21 @@ public function testIsUriAllowedNativeClientRejectsHostlessUriWithoutError() $this->assertFalse($client->isUriAllowed('mailto:foo@bar.com')); $this->assertFalse($client->isUriAllowed('file:///etc/passwd')); } + + public function testIsUriAllowedNativeClientRejectsPathSuffixOnRegisteredRedirectUri() + { + // isUriAllowed() previously matched via str_contains($uri, $redirect_uri) - a substring/prefix + // check, not an exact match. Registering "myapp://callback/safe" therefore also permitted + // "myapp://callback/other" and "myapp://callback/safe/extra": any path appended after the + // registered value passed. redirect_uris carries the OAuth2 authorization code, so an exact + // match is required here (query strings remain tolerated - canonicalUrl() strips them from + // both sides before comparison). + $client = new Client(); + $client->setApplicationType(IClient::ApplicationType_Native); + $client->setRedirectUris('myapp://callback/safe'); + + $this->assertTrue($client->isUriAllowed('myapp://callback/safe')); + $this->assertFalse($client->isUriAllowed('myapp://callback/other')); + $this->assertFalse($client->isUriAllowed('myapp://callback/safe/extra')); + } } From 15c71addc0565ed0ddfddd1282578a81cadb624d Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 14 Jul 2026 23:47:25 -0300 Subject: [PATCH 07/14] fix(oauth2): exact-match post_logout_redirect_uris, closing the CodeRabbit-flagged gap isPostLogoutUriAllowed() matched a registered value via str_contains() of only scheme://host[:port] against the whole registered CSV string - the path was never part of the comparison at all. Registering "myapp://callback/safe" therefore also permitted "myapp://callback/other" or "myapp://callback/safe/ extra". This mirrors the fix already applied to isUriAllowed() (bc5d1187): both sides now go through the same canonicalUrl()+normalizeUrl() pipeline and are compared with strict equality per registered entry. Query strings remain tolerated - canonicalUrl() drops them from both sides, so dynamic per-request ?session=/?state=... params still match. Adds regression tests for the path-suffix bypass and for continued dynamic query-string acceptance. --- app/Models/OAuth2/Client.php | 32 +++++++++++++++++++++----------- tests/unit/ClientMappingTest.php | 27 +++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 11 deletions(-) diff --git a/app/Models/OAuth2/Client.php b/app/Models/OAuth2/Client.php index 0663ea28..8f6c088e 100644 --- a/app/Models/OAuth2/Client.php +++ b/app/Models/OAuth2/Client.php @@ -1159,18 +1159,28 @@ public function isPostLogoutUriAllowed($post_logout_uri) // "Undefined array key host" warning (converted to ErrorException) on the public end-session endpoint. if(!isset($parts['host'])) return false; - // scheme/host are case-insensitive (RFC 3986); the write path normally lowercases the stored value, - // but match case-insensitively regardless so a bypassing write path can't silently break matching. - $stored_post_logout_uris = strtolower($this->post_logout_redirect_uris); - $logout_without_port = strtolower($parts['scheme'].'://'.$parts['host']); - - if(str_contains($stored_post_logout_uris, $logout_without_port )) return true; - - if(isset($parts['port'])) - { - $logout_with_port = $logout_without_port.':'.$parts['port']; - return str_contains($stored_post_logout_uris, $logout_with_port ); + // exact match against each registered value, through the same canonicalize+normalize pipeline on + // both sides (mirrors isUriAllowed()): a registered value's scheme+host[:port] must no longer match + // as a prefix of an unrelated path - the full path is now part of the comparison, and scheme/host + // are still matched case-insensitively since canonicalUrl()+normalizeUrl() lowercase both. Query + // strings remain tolerated - canonicalUrl() drops them from both sides, so a client's dynamic + // ?state=.../?session=... params never break the match. + $canonical_uri = URLUtils::canonicalUrl($post_logout_uri); + if(empty($canonical_uri)) return false; + $canonical_uri = URLUtils::normalizeUrl($canonical_uri); + if(empty($canonical_uri)) return false; + + foreach(explode(',', $this->post_logout_redirect_uris) as $registered_uri){ + $registered_uri = trim($registered_uri); + if(empty($registered_uri)) continue; + + $canonical_registered_uri = URLUtils::canonicalUrl($registered_uri); + if(empty($canonical_registered_uri)) continue; + $canonical_registered_uri = URLUtils::normalizeUrl($canonical_registered_uri); + + if($canonical_uri === $canonical_registered_uri) return true; } + return false; } diff --git a/tests/unit/ClientMappingTest.php b/tests/unit/ClientMappingTest.php index f6b105d2..6c641115 100644 --- a/tests/unit/ClientMappingTest.php +++ b/tests/unit/ClientMappingTest.php @@ -214,6 +214,33 @@ public function testIsPostLogoutUriAllowedNativeClientRejectsDangerousSchemeEven $this->assertTrue($client->isPostLogoutUriAllowed('myapp://callback')); } + public function testIsPostLogoutUriAllowedNativeClientRejectsPathSuffixOnRegisteredUri() + { + // same substring/prefix bypass fixed on isUriAllowed(): isPostLogoutUriAllowed() previously matched + // only scheme://host[:port] as a substring of the whole registered CSV, ignoring path entirely - so + // registering "myapp://callback/safe" also permitted "myapp://callback/other". The full path is now + // part of the comparison. + $client = new Client(); + $client->setApplicationType(IClient::ApplicationType_Native); + $client->setPostLogoutRedirectUris('myapp://callback/safe'); + + $this->assertTrue($client->isPostLogoutUriAllowed('myapp://callback/safe')); + $this->assertFalse($client->isPostLogoutUriAllowed('myapp://callback/other')); + $this->assertFalse($client->isPostLogoutUriAllowed('myapp://callback/safe/extra')); + } + + public function testIsPostLogoutUriAllowedNativeClientAcceptsDynamicQueryString() + { + // query strings are dynamic per logout request (session/state params) and were never part of the + // registered value - canonicalUrl() drops them from both sides before comparison, so this must keep + // working after the exact-match fix above. + $client = new Client(); + $client->setApplicationType(IClient::ApplicationType_Native); + $client->setPostLogoutRedirectUris('myapp://callback/safe'); + + $this->assertTrue($client->isPostLogoutUriAllowed('myapp://callback/safe?session=abc123&state=xyz')); + } + public function testIsUriAllowedNativeClientRejectsDangerousSchemeEvenWhenWrittenDirectly() { // same defense-in-depth as isPostLogoutUriAllowed, but for redirect_uris / isUriAllowed: the field From 2d091b21ff5a27f17daac38d2cc4e7d57b09cc6f Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 15 Jul 2026 00:43:51 -0300 Subject: [PATCH 08/14] fix(oauth2): exact-match isOriginAllowed(), closing the last substring-bypass gap isOriginAllowed() (CORS origin check for JS_Client, gated by OAuth2BearerAccessTokenRequestValidator) still compared via str_contains($this->allowed_origins, $normalizedOrigin) - a registered origin like https://my-app.example.com incorrectly matched a requested https://my-app.example.co, since the latter is a literal string prefix of the former. This is the same bypass class already fixed on isUriAllowed()/isPostLogoutUriAllowed() earlier in this PR, flagged Critical by CodeRabbit but left unaddressed on this sibling method (surfaced by /review-pr-deep on PR #147). Rewritten to the same explode+trim+canonicalize+normalize+exact-match pipeline as the two sibling methods. As a side effect this also fixes an asymmetry bug where the registered side was never normalized, so an exact-value registration could fail to match itself once normalizeUrl() appended a trailing slash to the request side only. Regression tests added to ClientMappingTest: the substring-prefix bypass (RED before this fix), and the pre-existing with/without-port matching semantics (a registered origin with no port matches any request port; one with an explicit port only matches that port). Verified: ClientMappingTest 14/14, and full "Application Test Suite" 163/163 (0 failures/errors) after a clean doctrine:migrations rebuild. Also documents the accepted trade-off (docs/adr/0001): the exact-match rewrite on isUriAllowed()/isPostLogoutUriAllowed() changes matching behavior for every application type, not only Native, with no pre-deploy audit of existing registrations - accepted given the small client base. --- app/Models/OAuth2/Client.php | 24 ++++++++++++-- .../0001-native-client-custom-uri-schemes.md | 1 + tests/unit/ClientMappingTest.php | 31 +++++++++++++++++++ 3 files changed, 54 insertions(+), 2 deletions(-) diff --git a/app/Models/OAuth2/Client.php b/app/Models/OAuth2/Client.php index 8f6c088e..8341c5c9 100644 --- a/app/Models/OAuth2/Client.php +++ b/app/Models/OAuth2/Client.php @@ -855,9 +855,29 @@ public function isOriginAllowed(string $origin):bool { $originWithoutPort = URLUtils::canonicalUrl($origin, false); if(empty($originWithoutPort)) return false; - if(str_contains($this->allowed_origins, URLUtils::normalizeUrl($originWithoutPort) )) return true; + $originWithoutPort = URLUtils::normalizeUrl($originWithoutPort); + $originWithPort = URLUtils::canonicalUrl($origin); - return str_contains($this->allowed_origins, URLUtils::normalizeUrl($originWithPort)); + $originWithPort = empty($originWithPort) ? null : URLUtils::normalizeUrl($originWithPort); + + // exact match against each registered value, through the same canonicalize+normalize pipeline on + // both sides (mirrors isUriAllowed()/isPostLogoutUriAllowed()) - a registered origin must no longer + // match merely because the requested origin is a string prefix of it (e.g. registered + // "https://my-app.example.com" incorrectly matching a requested "https://my-app.example.co" under + // the old str_contains($this->allowed_origins, $origin) check). + foreach(explode(',', $this->allowed_origins) as $allowed_origin){ + $allowed_origin = trim($allowed_origin); + if(empty($allowed_origin)) continue; + + $canonical_allowed_origin = URLUtils::canonicalUrl($allowed_origin); + if(empty($canonical_allowed_origin)) continue; + $canonical_allowed_origin = URLUtils::normalizeUrl($canonical_allowed_origin); + + if($originWithoutPort === $canonical_allowed_origin) return true; + if($originWithPort !== null && $originWithPort === $canonical_allowed_origin) return true; + } + + return false; } public function getWebsite() diff --git a/docs/adr/0001-native-client-custom-uri-schemes.md b/docs/adr/0001-native-client-custom-uri-schemes.md index fd2c09b9..05670508 100644 --- a/docs/adr/0001-native-client-custom-uri-schemes.md +++ b/docs/adr/0001-native-client-custom-uri-schemes.md @@ -58,6 +58,7 @@ Four consecutive adversarial code-review passes (xhigh-effort, multi-agent) surf **Accepted trade-offs (not fixed in this change, documented for a future pass if warranted):** - The deny-list can never be exhaustive against every OS/browser/app-launcher scheme that might exist now or in the future (a structural property of any blocklist). A `search-ms://`-style scheme not yet on the list would be accepted. Closing this fully requires an allow-list architecture, a larger change than this ADR's scope. +- **The `isUriAllowed()`/`isPostLogoutUriAllowed()` rewrite from substring to exact matching changes behavior for every application type, not only Native, with no pre-deploy audit of existing registrations.** Both methods previously matched via `str_contains()` — a registered value could match as a *prefix* of the requested URI (e.g. registered `https://app.com` matched a requested `https://app.com/oauth/callback`); they now require exact equality after canonicalization. The matching loop itself is not gated by `application_type` (only the scheme deny-list and the https requirement are), so this applies equally to `Confidential`/`Web_App`/`JS_Client` clients. Any existing client whose registered value is shorter than its actual callback path will fail to authenticate, or fail RP-initiated logout, starting at deploy. Flagged during `/review-pr-deep` review; accepted without a pre-deploy data audit because the client base is small enough that a break is expected to surface quickly and be corrected by re-registering the exact URI via the admin API/UI. ## References diff --git a/tests/unit/ClientMappingTest.php b/tests/unit/ClientMappingTest.php index 6c641115..f19eb735 100644 --- a/tests/unit/ClientMappingTest.php +++ b/tests/unit/ClientMappingTest.php @@ -296,4 +296,35 @@ public function testIsUriAllowedNativeClientRejectsPathSuffixOnRegisteredRedirec $this->assertFalse($client->isUriAllowed('myapp://callback/other')); $this->assertFalse($client->isUriAllowed('myapp://callback/safe/extra')); } + + public function testIsOriginAllowedRejectsOriginThatIsSubstringPrefixOfRegisteredOrigin() + { + // isOriginAllowed() used str_contains($this->allowed_origins, $normalizedOrigin) - a substring + // check, not an exact match. A requested origin that is a strict character-prefix of a registered + // one (e.g. "https://my-app.example.co" is literally the first N characters of the registered + // "https://my-app.example.com") therefore passed as if it were the registered origin itself. + // Same bypass class already fixed on isUriAllowed()/isPostLogoutUriAllowed() in this PR. + $client = new Client(); + $client->setApplicationType(IClient::ApplicationType_JS_Client); + $client->setAllowedOrigins('https://my-app.example.com'); + + $this->assertTrue($client->isOriginAllowed('https://my-app.example.com')); + $this->assertFalse($client->isOriginAllowed('https://my-app.example.co')); + } + + public function testIsOriginAllowedMatchesRegardlessOfExplicitDefaultPort() + { + // pre-existing behavior to preserve: a registered origin with no explicit port matches a request + // regardless of the request's port (checked port-agnostically first); a registered origin that + // DOES specify a port only matches a request carrying that exact port. + $client = new Client(); + $client->setApplicationType(IClient::ApplicationType_JS_Client); + $client->setAllowedOrigins('https://app.example.com,https://other.example.com:8443'); + + $this->assertTrue($client->isOriginAllowed('https://app.example.com')); + $this->assertTrue($client->isOriginAllowed('https://app.example.com:9999')); + $this->assertTrue($client->isOriginAllowed('https://other.example.com:8443')); + $this->assertFalse($client->isOriginAllowed('https://other.example.com:9999')); + $this->assertFalse($client->isOriginAllowed('https://evil.example.com')); + } } From 65e467ea879ca415171d8453e1401c176d507bf5 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 15 Jul 2026 01:08:20 -0300 Subject: [PATCH 09/14] 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). --- app/Services/OAuth2/ClientService.php | 246 ++++++++++++++++---------- tests/ClientApiTest.php | 32 ++++ 2 files changed, 187 insertions(+), 91 deletions(-) diff --git a/app/Services/OAuth2/ClientService.php b/app/Services/OAuth2/ClientService.php index f0cf41e4..c2ea2333 100644 --- a/app/Services/OAuth2/ClientService.php +++ b/app/Services/OAuth2/ClientService.php @@ -39,14 +39,32 @@ use models\exceptions\ValidationException; use Utils\Db\ITransactionService; use models\exceptions\EntityNotFoundException; +use Utils\Exceptions\UnacquiredLockException; use Utils\Http\HttpUtils; use Utils\Services\IAuthService; +use Utils\Services\ILockManagerService; +use Closure; /** * Class ClientService * @package Services\OAuth2 */ final class ClientService extends AbstractService implements IClientService { + /** + * SHORTCUT: a single global lock name serializes ALL Native client create()/update() calls that + * touch a custom URI scheme, not just the 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 native client registration + * volume makes this a bottleneck. + */ + const NATIVE_CUSTOM_SCHEME_REGISTRATION_LOCK = 'client.native.custom_scheme.registration'; + + /** + * seconds - long enough to cover one create()/update() transaction, short enough that a crashed + * request doesn't block native client scheme registration for long. + */ + const NATIVE_CUSTOM_SCHEME_REGISTRATION_LOCK_LIFETIME = 30; + /** * @var IAuthService */ @@ -74,6 +92,11 @@ final class ClientService extends AbstractService implements IClientService */ private $scope_repository; + /** + * @var ILockManagerService + */ + private $lock_manager_service; + /** * ClientService constructor. * @param IUserRepository $user_repository @@ -83,6 +106,7 @@ final class ClientService extends AbstractService implements IClientService * @param IClientCredentialGenerator $client_credential_generator * @param IApiScopeRepository $scope_repository * @param ITransactionService $tx_service + * @param ILockManagerService $lock_manager_service */ public function __construct ( @@ -92,7 +116,8 @@ public function __construct IApiScopeService $scope_service, IClientCredentialGenerator $client_credential_generator, IApiScopeRepository $scope_repository, - ITransactionService $tx_service + ITransactionService $tx_service, + ILockManagerService $lock_manager_service ) { parent::__construct($tx_service); @@ -102,6 +127,31 @@ public function __construct $this->client_credential_generator = $client_credential_generator; $this->client_repository = $client_repository; $this->scope_repository = $scope_repository; + $this->lock_manager_service = $lock_manager_service; + } + + /** + * Closes the TOCTOU race in IClientRepository::hasCustomSchemeRegisteredOnAnotherClientThan(): + * without this, two concurrent create()/update() calls can both pass the "is this scheme already + * registered" uniqueness check before either transaction commits, letting two different clients + * claim the same custom scheme - defeating the OS-level interception-prevention guarantee the + * uniqueness check exists for. + * + * @param Closure $fn + * @return IEntity + * @throws ValidationException + */ + private function withNativeCustomSchemeLock(Closure $fn): IEntity + { + try { + return $this->lock_manager_service->lock( + self::NATIVE_CUSTOM_SCHEME_REGISTRATION_LOCK, + $fn, + self::NATIVE_CUSTOM_SCHEME_REGISTRATION_LOCK_LIFETIME + ); + } catch (UnacquiredLockException $ex) { + throw new ValidationException('another native client custom URI scheme registration is in progress, please retry.'); + } } @@ -262,43 +312,50 @@ private function assertNativeCustomSchemesAllowed(array $payload, int $exclude_c */ public function create(array $payload):IEntity { + $do_create = function () use ($payload) { + return $this->tx_service->transaction(function () use ($payload) { - return $this->tx_service->transaction(function () use ($payload) { - - $current_user = $this->auth_service->getCurrentUser(); + $current_user = $this->auth_service->getCurrentUser(); - $app_name = trim($payload['app_name']); + $app_name = trim($payload['app_name']); - if($this->client_repository->getByApplicationName($app_name) != null){ - throw new ValidationException('there is already another application with that name, please choose another one.'); - } + if($this->client_repository->getByApplicationName($app_name) != null){ + throw new ValidationException('there is already another application with that name, please choose another one.'); + } - // same scheme deny-list + cross-client uniqueness rule update() enforces (only reachable - // for native clients, where the runtime https gate is relaxed) - now covers redirect_uris too. - if (($payload['application_type'] ?? null) === IClient::ApplicationType_Native) { - $this->assertNativeCustomSchemesAllowed($payload); - } + // same scheme deny-list + cross-client uniqueness rule update() enforces (only reachable + // for native clients, where the runtime https gate is relaxed) - now covers redirect_uris too. + if (($payload['application_type'] ?? null) === IClient::ApplicationType_Native) { + $this->assertNativeCustomSchemesAllowed($payload); + } - $client = ClientFactory::build($payload); - $client = $this->client_credential_generator->generate($client); - - if(isset($payload['admin_users']) && is_array($payload['admin_users'])) { - $admin_users = $payload['admin_users']; - //add admin users - foreach ($admin_users as $user_id) { - $user = $this->user_repository->getById(intval($user_id)); - if (is_null($user)) throw new EntityNotFoundException(sprintf('user %s not found.', $user_id)); - if(!$user instanceof User) continue; - $client->addAdminUser($user); + $client = ClientFactory::build($payload); + $client = $this->client_credential_generator->generate($client); + + if(isset($payload['admin_users']) && is_array($payload['admin_users'])) { + $admin_users = $payload['admin_users']; + //add admin users + foreach ($admin_users as $user_id) { + $user = $this->user_repository->getById(intval($user_id)); + if (is_null($user)) throw new EntityNotFoundException(sprintf('user %s not found.', $user_id)); + if(!$user instanceof User) continue; + $client->addAdminUser($user); + } } - } - $client->setOwner($current_user); + $client->setOwner($current_user); - $this->client_repository->add($client); + $this->client_repository->add($client); - return $client; - }); + return $client; + }); + }; + + if (($payload['application_type'] ?? null) === IClient::ApplicationType_Native) { + return $this->withNativeCustomSchemeLock($do_create); + } + + return $do_create(); } @@ -311,48 +368,61 @@ public function create(array $payload):IEntity */ public function update(int $id, array $payload):IEntity { + $do_update = function () use ($id, $payload) { + return $this->tx_service->transaction(function () use ($id, $payload) { - return $this->tx_service->transaction(function () use ($id, $payload) { - - $editing_user = $this->auth_service->getCurrentUser(); + $editing_user = $this->auth_service->getCurrentUser(); - $client = $this->client_repository->getById($id); + $client = $this->client_repository->getById($id); - if (is_null($client) || !$client instanceof Client) { - throw new EntityNotFoundException(sprintf('client id %s does not exists.', $id)); - } - $app_name = isset($payload['app_name']) ? trim($payload['app_name']) : null; - if(!empty($app_name)) { - $old_client = $this->client_repository->getByApplicationName($app_name); - if(!is_null($old_client) && $old_client->getId() !== $client->getId()) - throw new ValidationException('there is already another application with that name, please choose another one.'); - } - $current_app_type = $client->getApplicationType(); - if($current_app_type !== $payload['application_type']) - { - throw new ValidationException('application type does not match.'); - } + if (is_null($client) || !$client instanceof Client) { + throw new EntityNotFoundException(sprintf('client id %s does not exists.', $id)); + } + $app_name = isset($payload['app_name']) ? trim($payload['app_name']) : null; + if(!empty($app_name)) { + $old_client = $this->client_repository->getByApplicationName($app_name); + if(!is_null($old_client) && $old_client->getId() !== $client->getId()) + throw new ValidationException('there is already another application with that name, please choose another one.'); + } + $current_app_type = $client->getApplicationType(); + if($current_app_type !== $payload['application_type']) + { + throw new ValidationException('application type does not match.'); + } - ClientFactory::populate($client, $payload); + ClientFactory::populate($client, $payload); - // validate uris - switch($client->getApplicationType()) { - case IClient::ApplicationType_Native: { - // redirect_uris, allowed_origins, and post_logout_redirect_uris all share the same - // scheme deny-list + cross-client uniqueness rule; assertNativeCustomSchemesAllowed - // validates whichever of the three are present in the payload. - $this->assertNativeCustomSchemesAllowed($payload, $id); - } - break; - case IClient::ApplicationType_Web_App: - case IClient::ApplicationType_JS_Client: { - if (isset($payload['redirect_uris'])){ - if (!empty($payload['redirect_uris'])) { - $redirect_uris = explode(',', $payload['redirect_uris']); - foreach ($redirect_uris as $uri) { + // validate uris + switch($client->getApplicationType()) { + case IClient::ApplicationType_Native: { + // redirect_uris, allowed_origins, and post_logout_redirect_uris all share the same + // scheme deny-list + cross-client uniqueness rule; assertNativeCustomSchemesAllowed + // validates whichever of the three are present in the payload. + $this->assertNativeCustomSchemesAllowed($payload, $id); + } + break; + case IClient::ApplicationType_Web_App: + case IClient::ApplicationType_JS_Client: { + if (isset($payload['redirect_uris'])){ + if (!empty($payload['redirect_uris'])) { + $redirect_uris = explode(',', $payload['redirect_uris']); + foreach ($redirect_uris as $uri) { + $uri = @parse_url($uri); + if (!isset($uri['scheme'])) { + throw new ValidationException('invalid scheme on redirect uri.'); + } + if (!HttpUtils::isHttpsSchema($uri['scheme'])) { + throw new ValidationException(sprintf('scheme %s:// is invalid.', $uri['scheme'])); + } + } + } + } + if($client->getApplicationType() === IClient::ApplicationType_JS_Client && isset($payload['allowed_origins']) &&!empty($payload['allowed_origins'])){ + $allowed_origins = explode(',', $payload['allowed_origins']); + foreach ($allowed_origins as $uri) { $uri = @parse_url($uri); if (!isset($uri['scheme'])) { - throw new ValidationException('invalid scheme on redirect uri.'); + throw new ValidationException('invalid scheme on allowed origin uri.'); } if (!HttpUtils::isHttpsSchema($uri['scheme'])) { throw new ValidationException(sprintf('scheme %s:// is invalid.', $uri['scheme'])); @@ -360,37 +430,31 @@ public function update(int $id, array $payload):IEntity } } } - if($client->getApplicationType() === IClient::ApplicationType_JS_Client && isset($payload['allowed_origins']) &&!empty($payload['allowed_origins'])){ - $allowed_origins = explode(',', $payload['allowed_origins']); - foreach ($allowed_origins as $uri) { - $uri = @parse_url($uri); - if (!isset($uri['scheme'])) { - throw new ValidationException('invalid scheme on allowed origin uri.'); - } - if (!HttpUtils::isHttpsSchema($uri['scheme'])) { - throw new ValidationException(sprintf('scheme %s:// is invalid.', $uri['scheme'])); - } - } - } + break; } - break; - } - if(isset($payload['admin_users']) && is_array($payload['admin_users'])) { - $admin_users = $payload['admin_users']; - //add admin users - $client->removeAllAdminUsers(); - foreach ($admin_users as $user_id) { - $user = $this->user_repository->getById(intval($user_id)); - if (is_null($user)) throw new EntityNotFoundException(sprintf('user %s not found.', $user_id)); - if(!$user instanceof User) continue; - $client->addAdminUser($user); + if(isset($payload['admin_users']) && is_array($payload['admin_users'])) { + $admin_users = $payload['admin_users']; + //add admin users + $client->removeAllAdminUsers(); + foreach ($admin_users as $user_id) { + $user = $this->user_repository->getById(intval($user_id)); + if (is_null($user)) throw new EntityNotFoundException(sprintf('user %s not found.', $user_id)); + if(!$user instanceof User) continue; + $client->addAdminUser($user); + } } - } - $client->setEditedBy($editing_user); - return $client; - }); + $client->setEditedBy($editing_user); + return $client; + }); + }; + + if (($payload['application_type'] ?? null) === IClient::ApplicationType_Native) { + return $this->withNativeCustomSchemeLock($do_update); + } + + return $do_update(); } /** diff --git a/tests/ClientApiTest.php b/tests/ClientApiTest.php index 7537b7fd..3a37f776 100644 --- a/tests/ClientApiTest.php +++ b/tests/ClientApiTest.php @@ -14,9 +14,12 @@ use OAuth2\Models\IClient; use Auth\User; use Models\OAuth2\Client; +use Illuminate\Support\Facades\App; use Illuminate\Support\Facades\Session; use Illuminate\Support\Facades\Config; use LaravelDoctrine\ORM\Facades\EntityManager; +use Services\OAuth2\ClientService; +use Utils\Services\ILockManagerService; /** * Class ClientApiTest */ @@ -413,4 +416,33 @@ public function testCreateNativeClientRejectsArrayValueForRedirectUrisCleanly(){ $this->assertResponseStatus(412); } + public function testUpdateNativeClientRejectsCustomSchemeRegistrationWhenAnotherIsInProgress(){ + + // Closes the TOCTOU race in hasCustomSchemeRegisteredOnAnotherClientThan(): without a lock, + // two concurrent create()/update() calls can both pass the "is this scheme already registered" + // uniqueness check before either transaction commits, letting two different clients claim the + // same custom scheme. create()/update() now serialize behind a single Redis-backed lock + // (ILockManagerService). Simulate contention directly by pre-acquiring that same lock. + $lock_manager = App::make(ILockManagerService::class); + $lock_manager->acquireLock(ClientService::NATIVE_CUSTOM_SCHEME_REGISTRATION_LOCK, 5); + + try { + $client = EntityManager::getRepository(Client::class)->findOneBy(['app_name' => 'oauth2_native_app']); + + $response = $this->action("PUT", "Api\\ClientApiController@update", + array( + 'id' => $client->id, + 'application_type' => IClient::ApplicationType_Native, + 'redirect_uris' => 'lockcontention://callback', + ), + [], + [], + []); + + $this->assertResponseStatus(412); + } finally { + $lock_manager->releaseLock(ClientService::NATIVE_CUSTOM_SCHEME_REGISTRATION_LOCK); + } + } + } \ No newline at end of file From 889c57ce41612b71f7a365f13244041c48c23e76 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 15 Jul 2026 09:42:06 -0300 Subject: [PATCH 10/14] test(oauth2): document query-string/path-casing gap in URI matching (review Finding 2) Adds coverage for a gap identified by /review-pr-deep and independently by CodeRabbit: URLUtils::canonicalUrl() drops the query string and lowercases the full path before comparison, so the "exact match" isUriAllowed()/ isPostLogoutUriAllowed() advertise is weaker than RFC 6749 SS3.1.2.2's byte-for-byte requirement on path/query. These tests pin today's actual (permissive) behavior - they pass now, not RED tests for a fix - so a future change to this area can't silently alter the behavior without a test noticing, and so the gap is visible in the suite rather than only in a review comment. testIsUriAllowedIgnoresPathCasingDifferences / testIsPostLogoutUriAllowedIgnoresPathCasingDifferences: registered path casing doesn't have to match the requested URI's casing. testIsUriAllowedAcceptsAnyQueryStringNotJustDynamicOnes: fills the same gap already covered for isPostLogoutUriAllowed by the pre-existing testIsPostLogoutUriAllowedNativeClientAcceptsDynamicQueryString (which documents a deliberate choice for end-session's session/state params) - isUriAllowed had no equivalent test, and unlike post-logout, redirect_uri at /oauth2/authorize isn't expected to carry a client-appended dynamic query string, so this coverage gap looks closer to unintended than deliberate. No test added for review Finding 1 (isOriginAllowed() missing an empty-check after normalizeUrl(), independently flagged by CodeRabbit): extensive fuzzing of URLUtils::normalizeUrl() (invalid UTF-8, malformed IPv6/ports, control characters, bracket/backslash paths, etc.) found no input where canonicalUrl() succeeds but the subsequent normalizeUrl() call returns null - every malformed case already fails at the canonicalUrl() stage, which is already guarded. Not writing a test for an unconfirmed reproduction. --- tests/unit/ClientMappingTest.php | 39 ++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/tests/unit/ClientMappingTest.php b/tests/unit/ClientMappingTest.php index f19eb735..da9d2853 100644 --- a/tests/unit/ClientMappingTest.php +++ b/tests/unit/ClientMappingTest.php @@ -327,4 +327,43 @@ public function testIsOriginAllowedMatchesRegardlessOfExplicitDefaultPort() $this->assertFalse($client->isOriginAllowed('https://other.example.com:9999')); $this->assertFalse($client->isOriginAllowed('https://evil.example.com')); } + + public function testIsUriAllowedIgnoresPathCasingDifferences() + { + // Documents a known gap (review Finding 2): URLUtils::canonicalUrl() lowercases the ENTIRE path + // before comparison, not just scheme/host as the inline comments on isUriAllowed() claim ("path... + // remain case-sensitive"). A registered value and a requested URI differing only in path casing + // are therefore treated as identical - looser than RFC 6749 SS3.1.2.2's exact-match requirement. + $client = new Client(); + $client->setApplicationType(IClient::ApplicationType_Native); + $client->setRedirectUris('myapp://callback/Safe'); + + $this->assertTrue($client->isUriAllowed('myapp://callback/safe')); + } + + public function testIsPostLogoutUriAllowedIgnoresPathCasingDifferences() + { + // Same gap as isUriAllowed() above (review Finding 2), same root cause (canonicalUrl()). + $client = new Client(); + $client->setApplicationType(IClient::ApplicationType_Native); + $client->setPostLogoutRedirectUris('myapp://callback/Safe'); + + $this->assertTrue($client->isPostLogoutUriAllowed('myapp://callback/safe')); + } + + public function testIsUriAllowedAcceptsAnyQueryStringNotJustDynamicOnes() + { + // Documents a known gap (review Finding 2): canonicalUrl() drops the query string entirely from + // both sides before comparison, so isUriAllowed() accepts ANY query string on the requested URI, + // not just legitimate dynamic ones. Unlike isPostLogoutUriAllowed's dynamic query string tolerance + // (see testIsPostLogoutUriAllowedNativeClientAcceptsDynamicQueryString above, which documents a + // deliberate choice for end-session's session/state params), the redirect_uri sent to + // /oauth2/authorize is not expected to carry a client-appended dynamic query string, so this + // coverage gap is closer to an unintended byproduct than a deliberate design choice. + $client = new Client(); + $client->setApplicationType(IClient::ApplicationType_Native); + $client->setRedirectUris('myapp://callback/safe'); + + $this->assertTrue($client->isUriAllowed('myapp://callback/safe?unexpected=value&injected=1')); + } } From f75bb28acda43582ac3bd09dc9d8630b29b8d4cc Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 15 Jul 2026 10:46:51 -0300 Subject: [PATCH 11/14] fix(oauth2): scope Native client custom-scheme lock to payloads that touch URI fields ClientService::create()/update() acquired the TOCTOU lock (NATIVE_CUSTOM_SCHEME_REGISTRATION_LOCK) whenever application_type was Native, regardless of whether the payload touched redirect_uris, allowed_origins, or post_logout_redirect_uris. A plain app_name rename on one Native client therefore contended with an unrelated tenant's scheme registration, producing spurious "please retry" 412s. Extract NATIVE_CUSTOM_SCHEME_URI_FIELDS as the single source of truth for which fields matter, shared by assertNativeCustomSchemesAllowed() and the new payloadTouchesNativeCustomSchemeFields() gate. The lock is now taken only when the payload actually registers a custom scheme - matching what the SHORTCUT comment on the lock constant already claimed. Found during /review-pr-deep review of PR #147. --- app/Services/OAuth2/ClientService.php | 33 ++++++++++++++++++++++++--- tests/ClientApiTest.php | 29 +++++++++++++++++++++++ 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/app/Services/OAuth2/ClientService.php b/app/Services/OAuth2/ClientService.php index c2ea2333..446cc28e 100644 --- a/app/Services/OAuth2/ClientService.php +++ b/app/Services/OAuth2/ClientService.php @@ -65,6 +65,14 @@ final class ClientService extends AbstractService implements IClientService */ const NATIVE_CUSTOM_SCHEME_REGISTRATION_LOCK_LIFETIME = 30; + /** + * Single source of truth for which payload fields carry Native-client custom URI schemes - shared by + * assertNativeCustomSchemesAllowed() (what to validate) and payloadTouchesNativeCustomSchemeFields() + * (whether the TOCTOU lock is worth taking). Keeping both reads from one list means they can't drift: + * a field that stops needing validation automatically stops needing the lock too. + */ + const NATIVE_CUSTOM_SCHEME_URI_FIELDS = ['redirect_uris', 'allowed_origins', 'post_logout_redirect_uris']; + /** * @var IAuthService */ @@ -154,6 +162,23 @@ private function withNativeCustomSchemeLock(Closure $fn): IEntity } } + /** + * Gates the TOCTOU lock to payloads that actually register a custom URI scheme. A Native client + * write that doesn't touch redirect_uris/allowed_origins/post_logout_redirect_uris (e.g. renaming + * app_name) never calls hasCustomSchemeRegisteredOnAnotherClientThan(), so there is no race to close + * and no reason to serialize it behind every other tenant's scheme registration. + * + * @param array $payload + * @return bool + */ + private function payloadTouchesNativeCustomSchemeFields(array $payload): bool + { + foreach (self::NATIVE_CUSTOM_SCHEME_URI_FIELDS as $field) { + if (!empty($payload[$field])) return true; + } + return false; + } + /** * Clients in possession of a client password MAY use the HTTP Basic @@ -286,7 +311,7 @@ public function getCurrentClientAuthInfo() */ private function assertNativeCustomSchemesAllowed(array $payload, int $exclude_client_id = -1): void { - foreach (['redirect_uris', 'allowed_origins', 'post_logout_redirect_uris'] as $field) { + foreach (self::NATIVE_CUSTOM_SCHEME_URI_FIELDS as $field) { if (empty($payload[$field])) continue; foreach (explode(',', $payload[$field]) as $uri) { $parts = @parse_url(trim($uri)); @@ -351,7 +376,8 @@ public function create(array $payload):IEntity }); }; - if (($payload['application_type'] ?? null) === IClient::ApplicationType_Native) { + if (($payload['application_type'] ?? null) === IClient::ApplicationType_Native + && $this->payloadTouchesNativeCustomSchemeFields($payload)) { return $this->withNativeCustomSchemeLock($do_create); } @@ -450,7 +476,8 @@ public function update(int $id, array $payload):IEntity }); }; - if (($payload['application_type'] ?? null) === IClient::ApplicationType_Native) { + if (($payload['application_type'] ?? null) === IClient::ApplicationType_Native + && $this->payloadTouchesNativeCustomSchemeFields($payload)) { return $this->withNativeCustomSchemeLock($do_update); } diff --git a/tests/ClientApiTest.php b/tests/ClientApiTest.php index 3a37f776..218d8aba 100644 --- a/tests/ClientApiTest.php +++ b/tests/ClientApiTest.php @@ -445,4 +445,33 @@ public function testUpdateNativeClientRejectsCustomSchemeRegistrationWhenAnother } } + public function testUpdateNativeClientNotTouchingUriFieldsIgnoresLockContention(){ + + // The TOCTOU lock only protects hasCustomSchemeRegisteredOnAnotherClientThan(), which only runs + // when the payload touches redirect_uris/allowed_origins/post_logout_redirect_uris. A Native + // client update that doesn't touch any of those (e.g. renaming app_name) must not contend with + // an unrelated tenant's in-progress scheme registration - unlike the sibling test above, this + // must succeed even while the lock is held. + $lock_manager = App::make(ILockManagerService::class); + $lock_manager->acquireLock(ClientService::NATIVE_CUSTOM_SCHEME_REGISTRATION_LOCK, 5); + + try { + $client = EntityManager::getRepository(Client::class)->findOneBy(['app_name' => 'oauth2_native_app']); + + $response = $this->action("PUT", "Api\\ClientApiController@update", + array( + 'id' => $client->id, + 'application_type' => IClient::ApplicationType_Native, + 'app_description' => 'updated description while another registration is in progress', + ), + [], + [], + []); + + $this->assertResponseStatus(201); + } finally { + $lock_manager->releaseLock(ClientService::NATIVE_CUSTOM_SCHEME_REGISTRATION_LOCK); + } + } + } \ No newline at end of file From 75d86ac9f350a9964bfc4c845d6b16670ce8c3df Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 15 Jul 2026 15:45:17 -0300 Subject: [PATCH 12/14] fix(utils): auto-recover stuck locks - relative TTL and release on any Throwable LockManagerService::acquireLock() passed the absolute expiry timestamp (time()+lifetime+1) to Redis EXPIRE, which expects relative seconds: an unreleased lock (owner crashed or killed mid-callback) stayed held for ~55 years instead of auto-recovering after lifetime. For the NATIVE_CUSTOM_SCHEME_REGISTRATION_LOCK this meant every Native client create()/update() touching URI fields returned 412 "please retry" forever, until the Redis key was deleted by hand - contradicting the 30s recovery the lock's own doc comment promises. lock() also caught only Exception, so a Throwable that does not extend it (TypeError, Error) skipped both release paths and leaked the lock until that TTL expired. Release now happens in a finally block; acquisition stays outside the try so a failed acquireLock() still propagates without releasing a lock we don't own. Callers using acquireLock() as a single-use replay marker (nonce, auth code, private association) are unaffected: their artifacts' own lifetimes (360s/240s defaults) are shorter than the lock lifetimes, so the now-honored relative TTL opens no replay window. Found during /review-pr-deep review of PR #147. --- app/Services/Utils/LockManagerService.php | 24 ++++---- tests/unit/LockManagerServiceTest.php | 75 +++++++++++++++++++++++ 2 files changed, 86 insertions(+), 13 deletions(-) create mode 100644 tests/unit/LockManagerServiceTest.php diff --git a/app/Services/Utils/LockManagerService.php b/app/Services/Utils/LockManagerService.php index 7b85f6cd..0c8e258c 100644 --- a/app/Services/Utils/LockManagerService.php +++ b/app/Services/Utils/LockManagerService.php @@ -44,7 +44,10 @@ public function __construct(ICacheService $cache_service){ public function acquireLock($name,$lifetime = 3600) { $time = time()+$lifetime+1; - $success = $this->cache_service->addSingleValue($name, $time, $time); + // the stored value is the absolute expiry timestamp, but the redis TTL must be RELATIVE + // seconds: passing $time as the TTL kept an unreleased lock (owner crashed/killed before + // releaseLock) held for ~55 years instead of auto-recovering after $lifetime. + $success = $this->cache_service->addSingleValue($name, $time, $lifetime > 0 ? $lifetime + 1 : 0); if (!$success) { // only one time we could use this handle @@ -67,29 +70,24 @@ public function releaseLock($name) * @param string $name * @param Closure $callback * @param int $lifetime - * @return null + * @return mixed the callback result * @throws UnacquiredLockException * @throws Exception */ public function lock($name, Closure $callback, $lifetime = 3600) { - $result = null; + // on acquisition failure we don't own the lock, so there is nothing to release + $this->acquireLock($name, $lifetime); try { - $this->acquireLock($name, $lifetime); - $result = $callback($this); - $this->releaseLock($name); - } - catch(UnacquiredLockException $ex1) - { - throw $ex1; + return $callback($this); } - catch(Exception $ex) + finally { + // release on ANY outcome: the former catch(Exception) missed Throwables that don't + // extend Exception (TypeError, Error), leaking the lock until its TTL expired. $this->releaseLock($name); - throw $ex; } - return $result; } } \ No newline at end of file diff --git a/tests/unit/LockManagerServiceTest.php b/tests/unit/LockManagerServiceTest.php new file mode 100644 index 00000000..ba3fb636 --- /dev/null +++ b/tests/unit/LockManagerServiceTest.php @@ -0,0 +1,75 @@ +acquireLock($lock_name, 30); + + $ttl = $cache_service->ttl($lock_name); + $this->assertGreaterThan(0, $ttl); + $this->assertLessThanOrEqual(31, $ttl); + } finally { + $lock_manager->releaseLock($lock_name); + } + } + + public function testLockIsReleasedWhenCallbackThrowsError() + { + // lock() must release the lock on ANY Throwable from the callback. It used to catch only + // Exception, so a PHP Error (e.g. a TypeError inside the guarded transaction) skipped both + // release paths and left the lock held until the Redis TTL expired. + $lock_manager = App::make(ILockManagerService::class); + $lock_name = 'lock.test.release_on_error'; + + try { + $thrown = false; + try { + $lock_manager->lock($lock_name, function () { + throw new \TypeError('boom'); + }, 5); + } catch (\TypeError $ex) { + $thrown = true; + } + $this->assertTrue($thrown); + + // re-acquiring proves the lock was released despite the Error; + // before the fix this threw UnacquiredLockException + $lock_manager->acquireLock($lock_name, 5); + $this->assertGreaterThan(0, App::make(ICacheService::class)->ttl($lock_name)); + } finally { + $lock_manager->releaseLock($lock_name); + } + } +} From f35330cf248ca5e7fccd51e76bb1ebba35944b99 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 15 Jul 2026 15:45:28 -0300 Subject: [PATCH 13/14] test(oauth2): clear stale facade instances in OAuth2LoginStrategyTest setUp The test swaps in a minimal Container as facade root but never cleared Facade::$resolvedInstance, so any previously-run BrowserKitTestCase (which boots the full Laravel app) left resolved facades that shadowed the mocks bound on the minimal container - Redirect::action() hit the real redirector and the once() expectations failed at Mockery::close(). And since Mockery::close() is tearDown's first line, the facade cleanup below it never ran, cascading the failure across all three tests. Only surfaced now because the new tests/unit/LockManagerServiceTest.php changed which test precedes it in the suite; running the unchanged ClientMappingTest before it reproduces the same 3 errors, proving the isolation gap predates this branch's changes. --- tests/unit/OAuth2LoginStrategyTest.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/unit/OAuth2LoginStrategyTest.php b/tests/unit/OAuth2LoginStrategyTest.php index 701b9d11..f3491e26 100644 --- a/tests/unit/OAuth2LoginStrategyTest.php +++ b/tests/unit/OAuth2LoginStrategyTest.php @@ -54,7 +54,11 @@ protected function setUp(): void $_SERVER['REMOTE_ADDR'] = '127.0.0.1'; - // Set up a minimal facade root + // Set up a minimal facade root. Facades cache resolved instances statically, so any + // previously-run test that booted the full Laravel app (BrowserKitTestCase) leaves stale + // instances behind that would shadow the mocks bound on this minimal container - clear + // them BEFORE swapping the facade application, or this test breaks depending on suite order. + Facade::clearResolvedInstances(); $this->app = new Container(); $logger = Mockery::mock(LoggerInterface::class); From 670753d05bfd7740c4b20be80f670c764a5923a3 Mon Sep 17 00:00:00 2001 From: smarcet Date: Thu, 16 Jul 2026 18:49:04 -0300 Subject: [PATCH 14/14] fix(oauth2): release facade state in tearDown even if Mockery::close() throws tearDown() called Mockery::close() first with no try/finally, so an unmet mock expectation there threw before Facade::clearResolvedInstances()/ setFacadeApplication(null) could run - leaking this test's facade root (the minimal Container with mocked bindings) to whatever test runs next in the same PHP process. Same bug class the recent setUp() fix guards against, but in the opposite direction: leaking outward instead of receiving a leak from a prior test. Found during adversarial re-review of the setUp() fix. --- tests/unit/OAuth2LoginStrategyTest.php | 41 +++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 4 deletions(-) diff --git a/tests/unit/OAuth2LoginStrategyTest.php b/tests/unit/OAuth2LoginStrategyTest.php index f3491e26..234017aa 100644 --- a/tests/unit/OAuth2LoginStrategyTest.php +++ b/tests/unit/OAuth2LoginStrategyTest.php @@ -25,6 +25,7 @@ use OAuth2\Services\ISecurityContextService; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; +use ReflectionMethod; use Services\IUserActionService; use Strategies\OAuth2LoginStrategy; use Utils\Services\IAuthService; @@ -92,10 +93,15 @@ protected function setUp(): void protected function tearDown(): void { - Mockery::close(); - Facade::clearResolvedInstances(); - Facade::setFacadeApplication(null); - parent::tearDown(); + // facade cleanup must run even if Mockery::close() throws (unmet expectation) - otherwise + // this test's facade root leaks into whatever test runs next in the same PHP process. + try { + Mockery::close(); + } finally { + Facade::clearResolvedInstances(); + Facade::setFacadeApplication(null); + parent::tearDown(); + } } /** @@ -217,4 +223,31 @@ public function testGetLoginProceedsToLoginFormWhenUserIsGuest(): void $this->assertTrue($reachedMemento, 'Guest path must proceed past Auth::guest() check into memento loading'); } + + /** + * Regression: tearDown() called Mockery::close() first with no try/finally, so when it + * threw on an unmet expectation, the Facade::clearResolvedInstances()/setFacadeApplication(null) + * cleanup below it never ran - leaking this test's facade root to whatever test runs next in + * the same PHP process. Same bug class this class's setUp() now guards against, but from the + * opposite direction (leaking outward instead of receiving a leak from a prior test). + */ + public function testTearDownClearsFacadeStateEvenWhenMockeryCloseThrows(): void + { + // deliberately unmet expectation so Mockery::close() throws inside tearDown() + Mockery::mock(IAuthService::class)->shouldReceive('getCurrentUser')->once(); + + $tearDown = new ReflectionMethod($this, 'tearDown'); + $tearDown->setAccessible(true); + + $thrown = false; + try { + $tearDown->invoke($this); + } catch (\Throwable $ex) { + $thrown = true; + } + + $this->assertTrue($thrown, 'Mockery::close() should throw on the unmet expectation above'); + $this->assertNull(Facade::getFacadeApplication(), + 'facade root must be cleared even when Mockery::close() throws'); + } }