From b62d202e29e53a15703c8dcb86fce4e7b0dc8f53 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 14 Jul 2026 20:43:15 -0300 Subject: [PATCH 01/25] 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 571db04f..975df31d 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 7e7a5a8a506be724a47c238213ac484517ad6e50 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 14 Jul 2026 21:15:32 -0300 Subject: [PATCH 02/25] 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 097458f1e4e835bd04af02a959468d7b9ad29469 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 14 Jul 2026 21:16:14 -0300 Subject: [PATCH 03/25] 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 2c220948104a93a3729213c5ce762a523a503741 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 14 Jul 2026 21:30:05 -0300 Subject: [PATCH 04/25] 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 37bda3681fd2b76e581d302cac728185a4b3356e Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 14 Jul 2026 22:17:32 -0300 Subject: [PATCH 05/25] 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 3c0778cd21f0ba90dc2e879c70f4ed55e0dbc5b5 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 14 Jul 2026 23:38:08 -0300 Subject: [PATCH 06/25] 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 9dde6d1a3106174241fbd69751df1cd16c79cd41 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 14 Jul 2026 23:47:25 -0300 Subject: [PATCH 07/25] 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 d853160a27a6f7dd3ba36342d4d969ac3826ecf6 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 15 Jul 2026 00:43:51 -0300 Subject: [PATCH 08/25] 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 4b3763862f0febdcd170cf887a1cec741a3d75c3 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 15 Jul 2026 01:08:20 -0300 Subject: [PATCH 09/25] 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 dda5e98d4c4b9fe48003e8c1b3dac3145800b34e Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 15 Jul 2026 09:42:06 -0300 Subject: [PATCH 10/25] 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 3ac133b19c740c399ef736df298e3179985cbd1e Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 15 Jul 2026 10:46:51 -0300 Subject: [PATCH 11/25] 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 e8738a2d5559e58b507a3a53209eda736e9a40a5 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 15 Jul 2026 15:45:17 -0300 Subject: [PATCH 12/25] 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 f3940460b9865bb8277bcac5e1f7f15325d50435 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 15 Jul 2026 15:45:28 -0300 Subject: [PATCH 13/25] 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 6c1b1b5a811a785d9a91b07a3a493de4aef981eb Mon Sep 17 00:00:00 2001 From: smarcet Date: Thu, 16 Jul 2026 18:49:04 -0300 Subject: [PATCH 14/25] 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'); + } } From 3652448ad7a2682315f8e10afae79ce237269337 Mon Sep 17 00:00:00 2001 From: smarcet Date: Thu, 30 Jul 2026 17:21:02 -0300 Subject: [PATCH 15/25] =?UTF-8?q?fix(oauth2):=20ignore=20port=20when=20mat?= =?UTF-8?q?ching=20Native=20http-loopback=20redirect=5Furis=20(RFC=208252?= =?UTF-8?q?=20=C2=A77.3)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/Models/OAuth2/Client.php | 14 ++++++++++++-- tests/unit/ClientMappingTest.php | 24 ++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/app/Models/OAuth2/Client.php b/app/Models/OAuth2/Client.php index 8341c5c9..fa08b555 100644 --- a/app/Models/OAuth2/Client.php +++ b/app/Models/OAuth2/Client.php @@ -672,7 +672,17 @@ public function isUriAllowed(string $uri):bool return false; } - $uri = URLUtils::canonicalUrl($uri); + // RFC 8252 SS7.3: native apps doing http loopback redirection bind an EPHEMERAL port at + // request time - "the authorization server MUST allow any port to be specified at the time + // of the request for loopback IP redirect URIs". Only the port is ignored: scheme, host and + // path still require an exact match, and the loopback hosts are not cross-matched. + $use_port = !($this->application_type === IClient::ApplicationType_Native + && $original_parts !== false + && isset($original_parts['scheme'], $original_parts['host']) + && strtolower($original_parts['scheme']) === 'http' + && in_array(strtolower($original_parts['host']), IClient::NATIVE_LOOPBACK_HOSTS)); + + $uri = URLUtils::canonicalUrl($uri, $use_port); if(empty($uri)) { Log::debug(sprintf("Client::isUriAllowed url %s is not valid", $uri)); return false; @@ -698,7 +708,7 @@ public function isUriAllowed(string $uri):bool // 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); + $canonical_redirect_uri = URLUtils::canonicalUrl($redirect_uri, $use_port); if(empty($canonical_redirect_uri)) continue; $canonical_redirect_uri = URLUtils::normalizeUrl($canonical_redirect_uri); diff --git a/tests/unit/ClientMappingTest.php b/tests/unit/ClientMappingTest.php index da9d2853..26320b7c 100644 --- a/tests/unit/ClientMappingTest.php +++ b/tests/unit/ClientMappingTest.php @@ -366,4 +366,28 @@ public function testIsUriAllowedAcceptsAnyQueryStringNotJustDynamicOnes() $this->assertTrue($client->isUriAllowed('myapp://callback/safe?unexpected=value&injected=1')); } + + public function testIsUriAllowedNativeClientMatchesHttpLoopbackRegardlessOfPort() + { + // RFC 8252 SS7.3: native apps doing loopback interface redirection bind an EPHEMERAL port at + // request time, so "the authorization server MUST allow any port to be specified at the time + // of the request for loopback IP redirect URIs". Only the port is ignored in the comparison: + // scheme, host and path stay exact, non-loopback http stays rejected, and the loopback hosts + // are NOT cross-matched against each other (registering 127.0.0.1 does not allow localhost). + $client = new Client(); + $client->setApplicationType(IClient::ApplicationType_Native); + $client->setRedirectUris('http://127.0.0.1/callback,http://[::1]:8080/callback'); + + // registered without a port matches any requested port + $this->assertTrue($client->isUriAllowed('http://127.0.0.1:49152/callback')); + $this->assertTrue($client->isUriAllowed('http://127.0.0.1/callback')); + // registered WITH a port still matches any requested port (the RFC ignores the port entirely) + $this->assertTrue($client->isUriAllowed('http://[::1]:51204/callback')); + // path stays exact + $this->assertFalse($client->isUriAllowed('http://127.0.0.1:49152/other')); + // non-loopback http stays rejected by the deny-list carve-out + $this->assertFalse($client->isUriAllowed('http://insecure.example.com:49152/callback')); + // loopback hosts are distinct - no cross-match + $this->assertFalse($client->isUriAllowed('http://localhost:49152/callback')); + } } From 32921c972ea96b28f6cd1ffb8cdfbc9922203f04 Mon Sep 17 00:00:00 2001 From: smarcet Date: Thu, 30 Jul 2026 17:47:56 -0300 Subject: [PATCH 16/25] fix(oauth2): detect custom-scheme collisions in legacy space-separated URI lists --- app/Models/OAuth2/Factories/ClientFactory.php | 5 ++- .../DoctrineOAuth2ClientRepository.php | 9 +++- tests/ClientApiTest.php | 29 +++++++++++++ tests/unit/ClientFactoryTest.php | 43 +++++++++++++++++++ 4 files changed, 84 insertions(+), 2 deletions(-) create mode 100644 tests/unit/ClientFactoryTest.php diff --git a/app/Models/OAuth2/Factories/ClientFactory.php b/app/Models/OAuth2/Factories/ClientFactory.php index 2d80a1b8..79e1b13f 100644 --- a/app/Models/OAuth2/Factories/ClientFactory.php +++ b/app/Models/OAuth2/Factories/ClientFactory.php @@ -68,7 +68,10 @@ public static function populate(Client $client, array $payload):Client $urls = explode(',', $value); $normalized_uris = ''; foreach ($urls as $url) { - $url = URLUtils::normalizeUrl($url); + // trim BEFORE normalizing: URL\Normalizer preserves a leading space, and a stored + // ", scheme://" item breaks the anchored cross-client scheme-uniqueness LIKE + // (DoctrineOAuth2ClientRepository::hasCustomSchemeRegisteredOnAnotherClientThan) + $url = URLUtils::normalizeUrl(trim($url)); if (!empty($normalized_uris)) { $normalized_uris .= ','; } diff --git a/app/Repositories/DoctrineOAuth2ClientRepository.php b/app/Repositories/DoctrineOAuth2ClientRepository.php index 6e3c5e3f..2788ba70 100644 --- a/app/Repositories/DoctrineOAuth2ClientRepository.php +++ b/app/Repositories/DoctrineOAuth2ClientRepository.php @@ -181,12 +181,18 @@ public function hasCustomSchemeRegisteredOnAnotherClientThan(int $id, string $cu // match to a real list-item boundary: the scheme starts the field, or immediately follows a comma. $starts_with = $scheme . '://%'; $after_comma = '%,' . $scheme . '://%'; + // legacy rows: before the create()-validation hardening, POST create persisted lists verbatim, + // so an item can still sit after ", " (comma + single space - the JSON/forms list artifact). + // ClientFactory::populate now trims per item, so no NEW rows take this shape; N-space/other + // whitespace leftovers are for the pre-deploy audit (... LIKE '%, %'), not this query. + $after_comma_space = '%, ' . $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') + $qb->expr()->like($field, ':after_comma'), + $qb->expr()->like($field, ':after_comma_space') ); }; @@ -201,6 +207,7 @@ public function hasCustomSchemeRegisteredOnAnotherClientThan(int $id, string $cu ->andWhere("e.id <> :id") ->setParameter("starts_with", $starts_with) ->setParameter("after_comma", $after_comma) + ->setParameter("after_comma_space", $after_comma_space) ->setParameter("id", $id) ->setMaxResults(1) ->getQuery() diff --git a/tests/ClientApiTest.php b/tests/ClientApiTest.php index 218d8aba..0d4cfeda 100644 --- a/tests/ClientApiTest.php +++ b/tests/ClientApiTest.php @@ -416,6 +416,35 @@ public function testCreateNativeClientRejectsArrayValueForRedirectUrisCleanly(){ $this->assertResponseStatus(412); } + public function testUpdateNativeClientRejectsSchemeAlreadyRegisteredInLegacySpaceSeparatedList(){ + + // Legacy rows: before the create()-validation hardening in this branch, POST create persisted + // URI lists verbatim (zero request-level validation), so a stored value can still contain + // ", scheme://" (comma + single space - the JSON/forms list artifact). The anchored uniqueness + // LIKE must tolerate exactly that artifact, or a second client can silently claim a scheme a + // legacy row already holds - defeating the OS-level interception protection for those rows. + // Simulate the legacy row via a direct entity write, bypassing service validation just like + // the pre-hardening create() did. + $client1 = EntityManager::getRepository(Client::class)->findOneBy(['app_name' => 'oauth2_native_app']); + $client1->setPostLogoutRedirectUris('https://web.example.com/logout, legacyspaced://cb'); + EntityManager::persist($client1); + EntityManager::flush(); + + $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, + 'redirect_uris' => 'legacyspaced://other', + ), + [], + [], + []); + + $this->assertResponseStatus(412); + } + public function testUpdateNativeClientRejectsCustomSchemeRegistrationWhenAnotherIsInProgress(){ // Closes the TOCTOU race in hasCustomSchemeRegisteredOnAnotherClientThan(): without a lock, diff --git a/tests/unit/ClientFactoryTest.php b/tests/unit/ClientFactoryTest.php new file mode 100644 index 00000000..eaf1ea96 --- /dev/null +++ b/tests/unit/ClientFactoryTest.php @@ -0,0 +1,43 @@ + IClient::ApplicationType_Native, + 'post_logout_redirect_uris' => 'https://web.example.com/logout, myapp://cb', + 'redirect_uris' => 'https://web.example.com/cb, otherapp://cb', + ]); + + $this->assertStringNotContainsString(', ', implode(',', $client->getPostLogoutUris())); + $this->assertStringNotContainsString(', ', $client->getRawRedirectUris()); + } +} From eb950e5fc530f2263f3454c8abce371d20014bfa Mon Sep 17 00:00:00 2001 From: smarcet Date: Thu, 30 Jul 2026 17:50:02 -0300 Subject: [PATCH 17/25] docs(adr): record loopback port matching, legacy-list tolerance, and isOriginAllowed trade-off --- docs/adr/0001-native-client-custom-uri-schemes.md | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/docs/adr/0001-native-client-custom-uri-schemes.md b/docs/adr/0001-native-client-custom-uri-schemes.md index 05670508..2c539269 100644 --- a/docs/adr/0001-native-client-custom-uri-schemes.md +++ b/docs/adr/0001-native-client-custom-uri-schemes.md @@ -32,8 +32,8 @@ Four consecutive adversarial code-review passes (xhigh-effort, multi-agent) surf 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`. +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`). At match time (`Client::isUriAllowed()`), a Native client's http-loopback request is additionally compared **port-agnostically**: RFC 8252 §7.3 requires the AS to allow any port specified at request time, because native apps bind an ephemeral loopback port per run. Only the port is ignored — scheme, host, and path still require an exact match, and the loopback hosts are not cross-matched against each other (registering `127.0.0.1` does not allow `localhost`). +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`, plus a comma-space variant tolerating the legacy `", scheme://"` single-space artifact that the pre-hardening `create()` could persist; `ClientFactory::populate()` now trims each list item before normalizing, so new rows are always canonical regardless of write path. 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 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. @@ -58,7 +58,12 @@ 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. +- **The `isUriAllowed()`/`isPostLogoutUriAllowed()`/`isOriginAllowed()` rewrite from substring to exact matching changes behavior for every application type, not only Native, with no automatic remediation of existing registrations.** All three 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 (port-agnostic only for Native http-loopback, per Decision 3). The matching loops are 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. Two distinct failure modes at deploy: (a) a client whose registered `redirect_uris`/`post_logout_redirect_uris` value is shorter than its actual callback path fails to authenticate or fails RP-initiated logout — loud and quickly corrected by re-registering the exact URI; (b) a JS client whose `allowed_origins` entry was registered as a full URL with a path (e.g. `https://app.example.com/dashboard`) previously matched any Origin header that was a string prefix of it and now never matches — CORS/origin checks fail *silently*, so this one warrants the pre-deploy audit below. Accepted with audit-instead-of-migration because the client base is small. +- **Pre-deploy audit queries** (run before rollout; correct any hits via the admin API/UI — both verified against the local schema): + - Legacy space-separated lists (items after `", "` — tolerated by the uniqueness query's comma-space pattern, but worth normalizing): + `SELECT id, app_name FROM oauth2_client WHERE redirect_uris LIKE '%, %' OR post_logout_redirect_uris LIKE '%, %' OR allowed_origins LIKE '%, %';` + - Path-bearing origins (silently broken by exact origin matching — failure mode (b) above): + `SELECT id, app_name FROM oauth2_client WHERE allowed_origins REGEXP '://[^,/]+(:[0-9]+)?/[^,]';` ## References From 2f8b9c2e24f3f51886893169a814a0a70b457740 Mon Sep 17 00:00:00 2001 From: smarcet Date: Thu, 30 Jul 2026 18:03:46 -0300 Subject: [PATCH 18/25] docs(adr): note create() URI validation is a contract change for non-Native clients too --- docs/adr/0001-native-client-custom-uri-schemes.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/adr/0001-native-client-custom-uri-schemes.md b/docs/adr/0001-native-client-custom-uri-schemes.md index 2c539269..73cdfe53 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`). At match time (`Client::isUriAllowed()`), a Native client's http-loopback request is additionally compared **port-agnostically**: RFC 8252 §7.3 requires the AS to allow any port specified at request time, because native apps bind an ephemeral loopback port per run. Only the port is ignored — scheme, host, and path still require an exact match, and the loopback hosts are not cross-matched against each other (registering `127.0.0.1` does not allow `localhost`). 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`, plus a comma-space variant tolerating the legacy `", scheme://"` single-space artifact that the pre-hardening `create()` could persist; `ClientFactory::populate()` now trims each list item before normalizing, so new rows are always canonical regardless of write path. 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 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. +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. This closed gap is itself a deliberate API contract change beyond Native clients: `getCreatePayloadValidationRules()` previously declared none of the three URI fields, so `create()` accepted *any* value for them for every application type; the new `custom_url_set` rule enforces per-item https for non-Native types at create time. Automation that registered Web_App/JS clients with `http://` URIs (e.g. localhost dev tooling) or malformed lists now receives `412` where it previously got `201` — matching what `update()` always enforced for those types. 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 From 580e4ea0ff112dd28f5357efce6dac7fb77e9964 Mon Sep 17 00:00:00 2001 From: smarcet Date: Thu, 30 Jul 2026 18:37:05 -0300 Subject: [PATCH 19/25] fix(oauth2): guard isOriginAllowed against null normalization results --- app/Models/OAuth2/Client.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/Models/OAuth2/Client.php b/app/Models/OAuth2/Client.php index fa08b555..f4151555 100644 --- a/app/Models/OAuth2/Client.php +++ b/app/Models/OAuth2/Client.php @@ -866,6 +866,11 @@ public function isOriginAllowed(string $origin):bool $originWithoutPort = URLUtils::canonicalUrl($origin, false); if(empty($originWithoutPort)) return false; $originWithoutPort = URLUtils::normalizeUrl($originWithoutPort); + // defensive: no reproducible input reaches this with a null (canonicalUrl()'s + // filter_var/parse_url guard rejects everything malformed first), but the underlying + // Normalizer's mbParseUrl() can diverge from parse_url() and reset to an empty state - + // a null here comparing against a null registered-side normalization would false-match. + if(empty($originWithoutPort)) return false; $originWithPort = URLUtils::canonicalUrl($origin); $originWithPort = empty($originWithPort) ? null : URLUtils::normalizeUrl($originWithPort); @@ -882,6 +887,7 @@ public function isOriginAllowed(string $origin):bool $canonical_allowed_origin = URLUtils::canonicalUrl($allowed_origin); if(empty($canonical_allowed_origin)) continue; $canonical_allowed_origin = URLUtils::normalizeUrl($canonical_allowed_origin); + if(empty($canonical_allowed_origin)) continue; if($originWithoutPort === $canonical_allowed_origin) return true; if($originWithPort !== null && $originWithPort === $canonical_allowed_origin) return true; From db9fe57cafaf3bf1c181f4c1ead4972de06ab3ae Mon Sep 17 00:00:00 2001 From: smarcet Date: Thu, 6 Aug 2026 13:18:28 -0300 Subject: [PATCH 20/25] =?UTF-8?q?feat(oauth2):=20support=20RFC=208252=20?= =?UTF-8?q?=C2=A77.1=20authority-less=20custom-scheme=20URIs=20end-to-end?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit com.example.app:/oauth2redirect (the RFC's own example form, and the default shape AppAuth-based apps register) passed every write-time validator but was silently rejected by every runtime gate: URLUtils::canonicalUrl() opens with FILTER_VALIDATE_URL, which rejects the authority-less shape, so isUriAllowed()/isPostLogoutUriAllowed() could never match it - a registration that validates cleanly and never authenticates. - URLUtils::canonicalUrl(): canonicalize authority-less URIs as scheme + rooted lowercased path (query/fragment dropped, same as the authority form); opaque URIs (mailto:foo@bar - no authority AND no rooted path) keep returning null. - Client::isPostLogoutUriAllowed(): drop its own FILTER_VALIDATE_URL and isset(host) guards - validity is canonicalUrl()'s job now, and the host-less crash those guards prevented cannot recur. - DoctrineOAuth2ClientRepository: anchor the cross-client scheme-uniqueness LIKE on ':/' instead of '://' so a scheme claimed via either URI form collides with the other - the OS-level interception risk is about the scheme, not the shape it was registered in. - IndirectResponseQueryStringStrategy/IndirectResponseUrlFragmentStrategy: Laravel's Redirect::to() relies on the same FILTER_VALIDATE_URL check (UrlGenerator::isValidUrl) and rewrote the already-validated redirect target as a RELATIVE path (Location: https:///com.example.app:/logout) - caught by live verification only, invisible to the unit layer. Absolute URIs Laravel does not recognize are now emitted as a verbatim Location header; Symfony still rejects CR/LF in header values. All four regression tests written and confirmed failing first (TDD). The new end-session feature test lives in tests/OAuth2EndSessionTest.php because tests/OAuth2ProtocolTestCase.php's *TestCase.php suffix keeps that whole file out of PHPUnit's *Test.php discovery - the Application suite never runs it. Full Application suite: 173 tests / 562 assertions, 0 failures. Live-verified against the local docker IDP: registered com.example.app:/logout -> 302 Location: com.example.app:/logout?state=xyz (verbatim); unregistered path -> 400. --- app/Models/OAuth2/Client.php | 17 +++---- .../DoctrineOAuth2ClientRepository.php | 9 ++-- .../IndirectResponseQueryStringStrategy.php | 13 ++++- .../IndirectResponseUrlFragmentStrategy.php | 11 +++- app/libs/Utils/URLUtils.php | 17 +++++-- .../0001-native-client-custom-uri-schemes.md | 4 +- tests/ClientApiTest.php | 34 +++++++++++++ tests/OAuth2EndSessionTest.php | 50 +++++++++++++++++++ tests/unit/ClientMappingTest.php | 35 +++++++++++++ 9 files changed, 169 insertions(+), 21 deletions(-) create mode 100644 tests/OAuth2EndSessionTest.php diff --git a/app/Models/OAuth2/Client.php b/app/Models/OAuth2/Client.php index f4151555..5eb73959 100644 --- a/app/Models/OAuth2/Client.php +++ b/app/Models/OAuth2/Client.php @@ -1170,13 +1170,13 @@ public function isPostLogoutUriAllowed($post_logout_uri) if(empty($this->post_logout_redirect_uris)) return false; if(empty($post_logout_uri)) return false; - if(!filter_var($post_logout_uri, FILTER_VALIDATE_URL)) return false; - if(is_null($this->post_logout_redirect_uris)) return false; - if(empty($this->post_logout_redirect_uris)) return false; - + // no FILTER_VALIDATE_URL gate here: it rejects the RFC 8252 SS7.1 authority-less form + // (com.example.app:/logout) that native clients may register. Validity is enforced by the + // scheme checks below plus canonicalUrl() (which still applies FILTER_VALIDATE_URL to + // authority-bearing URIs and requires a rooted path for authority-less ones). $parts = @parse_url($post_logout_uri); - if ($parts == false) { + if ($parts == false || !isset($parts['scheme'])) { return false; } // native clients may register custom schemes (myapp://...); every other app type requires https @@ -1190,10 +1190,9 @@ public function isPostLogoutUriAllowed($post_logout_uri) 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; + // NOTE: no isset($parts['host']) guard here - authority-less URIs go through canonicalUrl(), + // which either canonicalizes them (RFC 8252 SS7.1 rooted-path form) or returns null (opaque + // forms like mailto:foo@bar), so the host-less crash this gate used to have cannot recur. // 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 diff --git a/app/Repositories/DoctrineOAuth2ClientRepository.php b/app/Repositories/DoctrineOAuth2ClientRepository.php index 2788ba70..be7f4a71 100644 --- a/app/Repositories/DoctrineOAuth2ClientRepository.php +++ b/app/Repositories/DoctrineOAuth2ClientRepository.php @@ -179,13 +179,16 @@ public function hasCustomSchemeRegisteredOnAnotherClientThan(int $id, string $cu // 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 . '://%'; + // The boundary is ':/' rather than '://' so BOTH registered forms are seen - the authority form + // (scheme://host/...) and the RFC 8252 SS7.1 authority-less form (scheme:/path): the OS-level + // interception risk is about the scheme, regardless of which URI form either client registered. + $starts_with = $scheme . ':/%'; + $after_comma = '%,' . $scheme . ':/%'; // legacy rows: before the create()-validation hardening, POST create persisted lists verbatim, // so an item can still sit after ", " (comma + single space - the JSON/forms list artifact). // ClientFactory::populate now trims per item, so no NEW rows take this shape; N-space/other // whitespace leftovers are for the pre-deploy audit (... LIKE '%, %'), not this query. - $after_comma_space = '%, ' . $scheme . '://%'; + $after_comma_space = '%, ' . $scheme . ':/%'; $qb = $this->getEntityManager()->createQueryBuilder(); $matches_field = function (string $field) use ($qb) { diff --git a/app/Strategies/IndirectResponseQueryStringStrategy.php b/app/Strategies/IndirectResponseQueryStringStrategy.php index 04fafeb6..57390ea3 100644 --- a/app/Strategies/IndirectResponseQueryStringStrategy.php +++ b/app/Strategies/IndirectResponseQueryStringStrategy.php @@ -12,8 +12,10 @@ * limitations under the License. **/ use Utils\IHttpResponseStrategy; +use Illuminate\Http\RedirectResponse; use Illuminate\Support\Facades\Redirect; use Illuminate\Support\Facades\Response; +use Illuminate\Support\Facades\URL; /** * Class IndirectResponseQueryStringStrategy * Redirect and http response using a 302 adding params on query string @@ -36,7 +38,16 @@ public function handle($response) } $return_to = (strpos($return_to, "?") == false) ? $return_to . "?" . $query_string : $return_to . "&" . $query_string; - return Redirect::to($return_to) + // RFC 8252 SS7.1 authority-less URIs (com.example.app:/cb?code=...) fail Laravel's + // UrlGenerator::isValidUrl(), so Redirect::to() would treat the already-validated redirect + // target as a RELATIVE path and prefix the site URL, corrupting the redirect. For an absolute + // URI (leading scheme) Laravel does not recognize, emit the Location verbatim - Symfony still + // rejects CR/LF in the header value, so no header-injection surface is opened. + $redirect = (!URL::isValidUrl($return_to) && preg_match('~^[A-Za-z][A-Za-z0-9+.\-]*:~', $return_to) === 1) + ? new RedirectResponse($return_to) + : Redirect::to($return_to); + + return $redirect ->header('Cache-Control', 'no-cache, no-store, max-age=0, must-revalidate') ->header('Pragma','no-cache'); } diff --git a/app/Strategies/IndirectResponseUrlFragmentStrategy.php b/app/Strategies/IndirectResponseUrlFragmentStrategy.php index e0dea760..2333facd 100644 --- a/app/Strategies/IndirectResponseUrlFragmentStrategy.php +++ b/app/Strategies/IndirectResponseUrlFragmentStrategy.php @@ -12,8 +12,10 @@ * limitations under the License. **/ use Utils\IHttpResponseStrategy; +use Illuminate\Http\RedirectResponse; use Illuminate\Support\Facades\Redirect; use Illuminate\Support\Facades\Response; +use Illuminate\Support\Facades\URL; /** * Class IndirectResponseUrlFragmentStrategy * Redirect and http response using a 302 adding params on url fragment @@ -37,7 +39,14 @@ public function handle($response) $return_to = (strpos($return_to, "#") == false) ? $return_to . "#" . $fragment : $return_to . "&" . $fragment; - return Redirect::to($return_to) + // same RFC 8252 SS7.1 authority-less guard as IndirectResponseQueryStringStrategy: an absolute + // URI Laravel's UrlGenerator does not recognize must be emitted verbatim, or Redirect::to() + // prefixes the site URL and corrupts the already-validated redirect target. + $redirect = (!URL::isValidUrl($return_to) && preg_match('~^[A-Za-z][A-Za-z0-9+.\-]*:~', $return_to) === 1) + ? new RedirectResponse($return_to) + : Redirect::to($return_to); + + return $redirect ->header('Cache-Control', 'no-cache, no-store, max-age=0, must-revalidate') ->header('Pragma','no-cache'); } diff --git a/app/libs/Utils/URLUtils.php b/app/libs/Utils/URLUtils.php index c3b11b65..459473ab 100644 --- a/app/libs/Utils/URLUtils.php +++ b/app/libs/Utils/URLUtils.php @@ -34,17 +34,24 @@ public static function normalizeUrl(string $url):?string{ * @return string|null */ public static function canonicalUrl(string $url, bool $usePort = true):?string{ - if(!filter_var($url, FILTER_VALIDATE_URL)) return null; $parts = @parse_url($url); - if ($parts == false) + if ($parts == false || !isset($parts['scheme'])) { 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; + // RFC 8252 SS7.1: private-use scheme redirect URIs may omit the authority entirely - + // "com.example.app:/oauth2redirect/example-provider" is the RFC's own example form. + // FILTER_VALIDATE_URL rejects that shape, so it is canonicalized here from parse_url parts: + // scheme + rooted path (query/fragment dropped, path lowercased, same as the authority form). + // Opaque URIs (mailto:foo@bar - no authority AND no rooted path) keep returning null: there + // is no location to match a redirect against. + if (!isset($parts['path']) || !str_starts_with($parts['path'], '/') || isset($parts['user']) || isset($parts['port'])) { + return null; + } + return rtrim($parts['scheme'].':'.strtolower($parts['path']), '/'); } + if(!filter_var($url, FILTER_VALIDATE_URL)) return null; $canonical_url = $parts['scheme'].'://'.strtolower($parts['host']); if(isset($parts['port']) && $usePort) { $canonical_url .= ':'.strtolower($parts['port']); diff --git a/docs/adr/0001-native-client-custom-uri-schemes.md b/docs/adr/0001-native-client-custom-uri-schemes.md index 73cdfe53..40fbc5f1 100644 --- a/docs/adr/0001-native-client-custom-uri-schemes.md +++ b/docs/adr/0001-native-client-custom-uri-schemes.md @@ -30,10 +30,10 @@ Four consecutive adversarial code-review passes (xhigh-effort, multi-agent) surf ## 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`. +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`. Both custom-scheme URI shapes are supported end-to-end: the authority form (`myapp://callback`) and the **RFC 8252 §7.1 authority-less form** (`com.example.app:/oauth2redirect` — the RFC's own example, and the default shape AppAuth-based apps register). `URLUtils::canonicalUrl()` canonicalizes the authority-less form as scheme + rooted, lowercased path (query/fragment dropped, same as the authority form); the two forms are distinct URIs and never cross-match. Opaque URIs (`mailto:foo@bar` — no authority *and* no rooted path) remain rejected: there is no location to match a redirect against. The redirect *emitters* (`IndirectResponseQueryStringStrategy`/`IndirectResponseUrlFragmentStrategy`) also special-case this form: Laravel's `Redirect::to()` relies on the same `FILTER_VALIDATE_URL` check internally (`UrlGenerator::isValidUrl()`) and would otherwise treat the already-validated target as a relative path, prefixing the site URL — an absolute URI Laravel does not recognize is emitted as a verbatim `Location` header instead (Symfony still rejects CR/LF in header values, so no header-injection surface opens). 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`). At match time (`Client::isUriAllowed()`), a Native client's http-loopback request is additionally compared **port-agnostically**: RFC 8252 §7.3 requires the AS to allow any port specified at request time, because native apps bind an ephemeral loopback port per run. Only the port is ignored — scheme, host, and path still require an exact match, and the loopback hosts are not cross-matched against each other (registering `127.0.0.1` does not allow `localhost`). -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`, plus a comma-space variant tolerating the legacy `", scheme://"` single-space artifact that the pre-hardening `create()` could persist; `ClientFactory::populate()` now trims each list item before normalizing, so new rows are always canonical regardless of write path. +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`, plus a comma-space variant tolerating the legacy `", scheme://"` single-space artifact that the pre-hardening `create()` could persist; `ClientFactory::populate()` now trims each list item before normalizing, so new rows are always canonical regardless of write path. The item boundary is `scheme:/` rather than `scheme://`, so a scheme claimed via the authority-less RFC 8252 §7.1 form (`scheme:/path`) collides with one claimed via the authority form (`scheme://host`) and vice versa — the OS-level interception risk is about the scheme, not the URI shape it was registered in. 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 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. This closed gap is itself a deliberate API contract change beyond Native clients: `getCreatePayloadValidationRules()` previously declared none of the three URI fields, so `create()` accepted *any* value for them for every application type; the new `custom_url_set` rule enforces per-item https for non-Native types at create time. Automation that registered Web_App/JS clients with `http://` URIs (e.g. localhost dev tooling) or malformed lists now receives `412` where it previously got `201` — matching what `update()` always enforced for those types. 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. diff --git a/tests/ClientApiTest.php b/tests/ClientApiTest.php index 0d4cfeda..c3933ee9 100644 --- a/tests/ClientApiTest.php +++ b/tests/ClientApiTest.php @@ -503,4 +503,38 @@ public function testUpdateNativeClientNotTouchingUriFieldsIgnoresLockContention( } } + public function testUpdateNativeClientRejectsSchemeAlreadyRegisteredInAuthorityLessForm(){ + + // RFC 8252 SS7.1 authority-less registrations (com.example.app:/oauth2redirect) store the scheme + // followed by ":/" instead of "://". The cross-client scheme-uniqueness LIKE must see that shape + // too: the OS-level interception risk is about the SCHEME, regardless of which URI form either + // client registered it in - so claiming "authlessscheme" via the authority-less form must block + // another client from claiming it via the authority form (and vice versa). + $client1 = EntityManager::getRepository(Client::class)->findOneBy(['app_name' => 'oauth2_native_app']); + + $response = $this->action("PUT", "Api\\ClientApiController@update", + array( + 'id' => $client1->id, + 'application_type' => IClient::ApplicationType_Native, + 'redirect_uris' => 'authlessscheme:/callback', + ), + [], + [], + []); + $this->assertResponseStatus(201); + + $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, + 'redirect_uris' => 'authlessscheme://other', + ), + [], + [], + []); + $this->assertResponseStatus(412); + } + } \ No newline at end of file diff --git a/tests/OAuth2EndSessionTest.php b/tests/OAuth2EndSessionTest.php new file mode 100644 index 00000000..20b35840 --- /dev/null +++ b/tests/OAuth2EndSessionTest.php @@ -0,0 +1,50 @@ +/com.example.app:/logout) - + // corrupting the redirect at the emitter even once the runtime allow-gates accept the + // authority-less form. The Location header must carry the registered URI verbatim, with the + // state round-tripped on the query string. + $client = EntityManager::getRepository(Client::class)->findOneBy(['app_name' => 'oauth2_native_app']); + $client->setPostLogoutRedirectUris('com.example.app:/logout'); + EntityManager::persist($client); + EntityManager::flush(); + + $this->call('GET', '/oauth2/end-session', [ + 'client_id' => $client->getClientId(), + 'post_logout_redirect_uri' => 'com.example.app:/logout', + 'state' => 'xyz', + ]); + + $this->assertResponseStatus(302); + $this->assertEquals('com.example.app:/logout?state=xyz', $this->response->headers->get('Location')); + } +} diff --git a/tests/unit/ClientMappingTest.php b/tests/unit/ClientMappingTest.php index 26320b7c..9d6194d7 100644 --- a/tests/unit/ClientMappingTest.php +++ b/tests/unit/ClientMappingTest.php @@ -390,4 +390,39 @@ public function testIsUriAllowedNativeClientMatchesHttpLoopbackRegardlessOfPort( // loopback hosts are distinct - no cross-match $this->assertFalse($client->isUriAllowed('http://localhost:49152/callback')); } + + public function testIsUriAllowedNativeClientAcceptsAuthorityLessRfc8252Uri() + { + // RFC 8252 SS7.1 recommends the authority-less form for private-use scheme redirects - + // "com.example.app:/oauth2redirect/example-provider" is the RFC's own example, and it is the + // default shape AppAuth-based apps register. It has a scheme and a rooted path but no host, + // so canonicalUrl()'s FILTER_VALIDATE_URL/host requirements used to reject it unconditionally + // at match time even though every write-time validator accepts it. + $client = new Client(); + $client->setApplicationType(IClient::ApplicationType_Native); + $client->setRedirectUris('com.example.app:/oauth2redirect/example-provider'); + + $this->assertTrue($client->isUriAllowed('com.example.app:/oauth2redirect/example-provider')); + // query strings stay tolerated, same as the authority form + $this->assertTrue($client->isUriAllowed('com.example.app:/oauth2redirect/example-provider?state=xyz')); + // path stays exact + $this->assertFalse($client->isUriAllowed('com.example.app:/other')); + // the authority form is a DIFFERENT uri (host "oauth2redirect" vs path "/oauth2redirect") - no cross-match + $this->assertFalse($client->isUriAllowed('com.example.app://oauth2redirect/example-provider')); + // opaque scheme-only uris (no authority AND no rooted path) stay rejected + $this->assertFalse($client->isUriAllowed('com.example.app:oauth2redirect')); + } + + public function testIsPostLogoutUriAllowedNativeClientAcceptsAuthorityLessUri() + { + // same RFC 8252 SS7.1 authority-less form, at the end-session gate: this one was additionally + // blocked by isPostLogoutUriAllowed()'s own FILTER_VALIDATE_URL and isset(host) guards. + $client = new Client(); + $client->setApplicationType(IClient::ApplicationType_Native); + $client->setPostLogoutRedirectUris('com.example.app:/logout'); + + $this->assertTrue($client->isPostLogoutUriAllowed('com.example.app:/logout')); + $this->assertFalse($client->isPostLogoutUriAllowed('com.example.app:/other')); + $this->assertFalse($client->isPostLogoutUriAllowed('com.example.app://logout')); + } } From 0114035e05b315bfa1a8934eca515de8894cf058 Mon Sep 17 00:00:00 2001 From: smarcet Date: Thu, 6 Aug 2026 14:08:50 -0300 Subject: [PATCH 21/25] test: re-enable the OAuth2 protocol suite excluded by its *TestCase.php filename tests/OAuth2ProtocolTestCase.php (23 tests covering the auth-code, PKCE, implicit, client-credentials, refresh and revocation flows) never ran in the Application Test Suite: PHPUnit's directory discovery only picks up *Test.php, and the file has carried the *TestCase.php suffix since the Laravel 11 upgrade (ae247387). Renamed file + class to OAuth2ProtocolTest so the suite discovers it again. Two latent problems surfaced by re-enabling it, both fixed: - OAUTH2_VALIDATE_RESOURCE_SERVER_IP was unset in the testing env, so the resource-server IP allow-list check (ValidateBearerTokenResourceServerStrategy) silently no-oped and testResourceServerIntrospectionNotValidIP got 200 where it asserts 400. Enabled via phpunit.xml . - With the gate on, the caller IP came from $_SERVER['REMOTE_ADDR'], which is unset under the PHPUnit CLI unless an earlier test happened to assign it and leak it (OAuth2LoginStrategyTest::setUp does) - the same tests passed or failed depending on suite order. tests/bootstrap.php now pins REMOTE_ADDR=127.0.0.1 for the whole run. Full Application suite: 196 tests / 1004 assertions, 0 failures (was 173 - the 23 protocol tests now run). --- phpunit.xml | 5 +++++ tests/OAuth2EndSessionTest.php | 3 --- .../{OAuth2ProtocolTestCase.php => OAuth2ProtocolTest.php} | 2 +- tests/bootstrap.php | 6 ++++++ 4 files changed, 12 insertions(+), 4 deletions(-) rename tests/{OAuth2ProtocolTestCase.php => OAuth2ProtocolTest.php} (99%) diff --git a/phpunit.xml b/phpunit.xml index 7515f39f..0450ff06 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -25,5 +25,10 @@ + + diff --git a/tests/OAuth2EndSessionTest.php b/tests/OAuth2EndSessionTest.php index 20b35840..7f1d9508 100644 --- a/tests/OAuth2EndSessionTest.php +++ b/tests/OAuth2EndSessionTest.php @@ -17,9 +17,6 @@ /** * Class OAuth2EndSessionTest - * NOTE: deliberately NOT placed in OAuth2ProtocolTestCase.php - that file's *TestCase.php suffix - * keeps it OUT of the Application Test Suite (PHPUnit only auto-discovers *Test.php), so a test - * added there would never run in CI. * @package Tests */ final class OAuth2EndSessionTest extends OpenStackIDBaseTestCase diff --git a/tests/OAuth2ProtocolTestCase.php b/tests/OAuth2ProtocolTest.php similarity index 99% rename from tests/OAuth2ProtocolTestCase.php rename to tests/OAuth2ProtocolTest.php index d0a72b9f..bcabbe65 100644 --- a/tests/OAuth2ProtocolTestCase.php +++ b/tests/OAuth2ProtocolTest.php @@ -23,7 +23,7 @@ * Class OAuth2ProtocolTest * Test Suite for OAuth2 Protocol */ -final class OAuth2ProtocolTestCase extends OpenStackIDBaseTestCase +final class OAuth2ProtocolTest extends OpenStackIDBaseTestCase { private $current_realm; diff --git a/tests/bootstrap.php b/tests/bootstrap.php index 604a7dbd..9dcb5833 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -3,6 +3,12 @@ require dirname(__DIR__).'/bootstrap/autoload.php'; +// PHPUnit runs from the CLI, where REMOTE_ADDR is unset. Code under test reads it directly +// (UserIPHelperProvider), so without a deterministic value here the resource-server IP checks +// depended on whichever earlier test happened to set $_SERVER['REMOTE_ADDR'] and leak it - +// the same suite passed or failed depending on test order. +$_SERVER['REMOTE_ADDR'] = '127.0.0.1'; + use Symfony\Component\ErrorHandler\ErrorHandler; From 614ade5b32c01129ffacc1e99713d7943dac3c16 Mon Sep 17 00:00:00 2001 From: smarcet Date: Thu, 6 Aug 2026 14:49:12 -0300 Subject: [PATCH 22/25] refactor(oauth2): consolidate runtime URI matching into a single pipeline The three runtime allow-gates (isUriAllowed, isPostLogoutUriAllowed, isOriginAllowed) each carried a hand-rolled copy of the same algorithm - explode the registered CSV, trim, canonicalize+normalize each side, exact compare - which is how the RFC 8252 SS7.3 loopback port rule ended up patched into one copy and silently absent from its siblings. Knowledge now lives in one place each: - URLUtils::canonicalizeForMatch(): the canonicalUrl->normalizeUrl->null-guard sequence, previously repeated 6+ times. - URLUtils::anyCanonicalMatchesList(): the ONLY registered-list matching loop. Per-field differences are caller arguments, not separate algorithms: the redirect gate passes use_port from the loopback rule, post-logout passes true (port matched exactly - deliberate, see ADR decision 3), origin passes its two canonical forms (without-port matches any requested port when no port is registered; with-port requires the exact one). - Client::isRfc8252LoopbackRedirect(): the named RFC 8252 SS7.3 predicate, extracted from an inline $use_port expression. - AbstractIndirectResponseStrategy::redirectTo(): the verbatim-Location emitter guard, previously duplicated across the query-string and fragment strategies. - Swapped URLUtils' dead 'use AWS\CRT\Log' import for the Log facade the new matcher logs through. Zero behavior change; the existing suite is the net: Application 196 tests / 1004 assertions, OTEL 23+12, all 0 failures - identical counts to pre-refactor. Live re-verified at /oauth2/end-session: authority-less registered URI -> 302 verbatim Location, https registered -> 302, unregistered -> 400. ADR decision 3 now declares the post-logout port asymmetry intentional and points at the single flag that would change it. Note for local dev environments: the new AbstractIndirectResponseStrategy requires a composer dump-autoload (classmap); CI regenerates it on install. --- app/Models/OAuth2/Client.php | 173 +++++++++--------- .../AbstractIndirectResponseStrategy.php | 45 +++++ .../IndirectResponseQueryStringStrategy.php | 19 +- .../IndirectResponseUrlFragmentStrategy.php | 17 +- app/libs/Utils/URLUtils.php | 51 +++++- .../0001-native-client-custom-uri-schemes.md | 2 +- 6 files changed, 182 insertions(+), 125 deletions(-) create mode 100644 app/Strategies/AbstractIndirectResponseStrategy.php diff --git a/app/Models/OAuth2/Client.php b/app/Models/OAuth2/Client.php index 5eb73959..3a17f1a5 100644 --- a/app/Models/OAuth2/Client.php +++ b/app/Models/OAuth2/Client.php @@ -658,6 +658,26 @@ private function isNativeDangerousScheme(string $scheme, ?string $host = null): return $this->application_type === IClient::ApplicationType_Native && self::isDisallowedNativeUriScheme($scheme, $host); } + /** + * RFC 8252 SS7.3: a Native client's http-loopback redirect binds an EPHEMERAL port at request + * time - "the authorization server MUST allow any port to be specified at the time of the + * request for loopback IP redirect URIs". Single place this rule is decided; isUriAllowed() + * feeds it into the matching pipeline as "ignore the port on both sides". Deliberately NOT + * consulted by isPostLogoutUriAllowed() - no spec extends the carve-out to RP-initiated + * logout (see ADR-0001, decision 3). + * + * @param array|false $parts result of parse_url() on the requested URI + * @return bool + */ + private function isRfc8252LoopbackRedirect($parts): bool + { + return $this->application_type === IClient::ApplicationType_Native + && $parts !== false + && isset($parts['scheme'], $parts['host']) + && strtolower($parts['scheme']) === 'http' + && in_array(strtolower($parts['host']), IClient::NATIVE_LOOPBACK_HOSTS); + } + /** * @param string $uri * @return bool @@ -672,52 +692,42 @@ public function isUriAllowed(string $uri):bool return false; } - // RFC 8252 SS7.3: native apps doing http loopback redirection bind an EPHEMERAL port at - // request time - "the authorization server MUST allow any port to be specified at the time - // of the request for loopback IP redirect URIs". Only the port is ignored: scheme, host and - // path still require an exact match, and the loopback hosts are not cross-matched. - $use_port = !($this->application_type === IClient::ApplicationType_Native - && $original_parts !== false - && isset($original_parts['scheme'], $original_parts['host']) - && strtolower($original_parts['scheme']) === 'http' - && in_array(strtolower($original_parts['host']), IClient::NATIVE_LOOPBACK_HOSTS)); - - $uri = URLUtils::canonicalUrl($uri, $use_port); - if(empty($uri)) { + // RFC 8252 SS7.3 loopback redirects are compared port-agnostically - only the port is + // ignored: scheme, host and path still require an exact match, and the loopback hosts are + // not cross-matched (see isRfc8252LoopbackRedirect). + $use_port = !$this->isRfc8252LoopbackRedirect($original_parts); + + $canonical_uri = URLUtils::canonicalUrl($uri, $use_port); + if(empty($canonical_uri)) { Log::debug(sprintf("Client::isUriAllowed url %s is not valid", $uri)); return false; } + // evaluated on the canonical (pre-normalization) form: normalizeUrl() lowercases the scheme, + // and this check has always been case-sensitive on it. if ( - ($this->application_type !== IClient::ApplicationType_Native && !URLUtils::isHTTPS($uri)) + ($this->application_type !== IClient::ApplicationType_Native && !URLUtils::isHTTPS($canonical_uri)) && (ServerConfigurationService::getConfigValue("SSL.Enable")) ) { - Log::debug(sprintf("Client::isUriAllowed url %s is not under ssl schema", $uri)); + Log::debug(sprintf("Client::isUriAllowed url %s is not under ssl schema", $canonical_uri)); return false; } - $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; - - // 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, $use_port); - 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; - } + $requested_uri = URLUtils::normalizeUrl($canonical_uri); + if(empty($requested_uri)) return false; - Log::debug(sprintf("Client::isUriAllowed url %s is not allowed as return url for client %s", $uri, $this->client_id)); + // exact match against each registered value, both sides through the same canonicalize+normalize + // pipeline (URLUtils::anyCanonicalMatchesList) - a registered value must not be accepted merely + // as a *prefix* of the requested URI (e.g. "myapp://callback" matching "myapp://callback/"). + if(URLUtils::anyCanonicalMatchesList( + [$requested_uri], + $this->redirect_uris, + $use_port, + sprintf("Client::isUriAllowed client %s", $this->client_id))) + return true; + + Log::debug(sprintf("Client::isUriAllowed url %s is not allowed as return url for client %s", $requested_uri, $this->client_id)); return false; } @@ -863,37 +873,27 @@ public function getRawClientAllowedOrigins() */ public function isOriginAllowed(string $origin):bool { - $originWithoutPort = URLUtils::canonicalUrl($origin, false); - if(empty($originWithoutPort)) return false; - $originWithoutPort = URLUtils::normalizeUrl($originWithoutPort); - // defensive: no reproducible input reaches this with a null (canonicalUrl()'s - // filter_var/parse_url guard rejects everything malformed first), but the underlying - // Normalizer's mbParseUrl() can diverge from parse_url() and reset to an empty state - - // a null here comparing against a null registered-side normalization would false-match. - if(empty($originWithoutPort)) return false; - - $originWithPort = URLUtils::canonicalUrl($origin); - $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(empty($canonical_allowed_origin)) continue; - - if($originWithoutPort === $canonical_allowed_origin) return true; - if($originWithPort !== null && $originWithPort === $canonical_allowed_origin) return true; - } + // exact match against each registered value, both sides through the same canonicalize+normalize + // pipeline (URLUtils::anyCanonicalMatchesList) - a registered origin must not match merely + // because the requested origin is a string prefix of it. The requested origin is offered in + // TWO canonical forms: without its port (so a registered origin with no explicit port matches + // the request on any port) and with it (so a registered origin WITH a port only matches the + // request carrying that exact port). canonicalizeForMatch() yielding null on either side can + // never false-match - a null requested form is dropped, a null registered item is skipped. + $requested_origins = []; - return false; + $originWithoutPort = URLUtils::canonicalizeForMatch($origin, false); + if(is_null($originWithoutPort)) return false; + $requested_origins[] = $originWithoutPort; + + $originWithPort = URLUtils::canonicalizeForMatch($origin); + if(!is_null($originWithPort)) $requested_origins[] = $originWithPort; + + return URLUtils::anyCanonicalMatchesList( + $requested_origins, + $this->allowed_origins, + true, + sprintf("Client::isOriginAllowed client %s", $this->client_id)); } public function getWebsite() @@ -1190,33 +1190,24 @@ public function isPostLogoutUriAllowed($post_logout_uri) if($this->isNativeDangerousScheme($parts['scheme'], $parts['host'] ?? null)) return false; - // NOTE: no isset($parts['host']) guard here - authority-less URIs go through canonicalUrl(), - // which either canonicalizes them (RFC 8252 SS7.1 rooted-path form) or returns null (opaque - // forms like mailto:foo@bar), so the host-less crash this gate used to have cannot recur. - - // 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; + // NOTE: no isset($parts['host']) guard here - authority-less URIs go through the matching + // pipeline, which either canonicalizes them (RFC 8252 SS7.1 rooted-path form) or yields null + // (opaque forms like mailto:foo@bar), so the host-less crash this gate used to have cannot recur. + + // exact match against each registered value, both sides through the same canonicalize+normalize + // pipeline (URLUtils::anyCanonicalMatchesList): the full path is part of the comparison, + // scheme/host stay case-insensitive, and query strings remain tolerated (dropped from both + // sides), so a client's dynamic ?state=.../?session=... params never break the match. The + // registered port is matched exactly - the RFC 8252 SS7.3 port carve-out deliberately applies + // to isUriAllowed() only (see isRfc8252LoopbackRedirect / ADR-0001 decision 3). + $requested_uri = URLUtils::canonicalizeForMatch($post_logout_uri); + if(is_null($requested_uri)) return false; + + return URLUtils::anyCanonicalMatchesList( + [$requested_uri], + $this->post_logout_redirect_uris, + true, + sprintf("Client::isPostLogoutUriAllowed client %s", $this->client_id)); } public function getAdminUsers(){ diff --git a/app/Strategies/AbstractIndirectResponseStrategy.php b/app/Strategies/AbstractIndirectResponseStrategy.php new file mode 100644 index 00000000..7c0ed4a3 --- /dev/null +++ b/app/Strategies/AbstractIndirectResponseStrategy.php @@ -0,0 +1,45 @@ +header('Cache-Control', 'no-cache, no-store, max-age=0, must-revalidate') + ->header('Pragma','no-cache'); + } +} diff --git a/app/Strategies/IndirectResponseQueryStringStrategy.php b/app/Strategies/IndirectResponseQueryStringStrategy.php index 57390ea3..5e1f88c2 100644 --- a/app/Strategies/IndirectResponseQueryStringStrategy.php +++ b/app/Strategies/IndirectResponseQueryStringStrategy.php @@ -11,17 +11,13 @@ * See the License for the specific language governing permissions and * limitations under the License. **/ -use Utils\IHttpResponseStrategy; -use Illuminate\Http\RedirectResponse; -use Illuminate\Support\Facades\Redirect; use Illuminate\Support\Facades\Response; -use Illuminate\Support\Facades\URL; /** * Class IndirectResponseQueryStringStrategy * Redirect and http response using a 302 adding params on query string * @package Strategies */ -class IndirectResponseQueryStringStrategy implements IHttpResponseStrategy +class IndirectResponseQueryStringStrategy extends AbstractIndirectResponseStrategy { /** @@ -38,17 +34,6 @@ public function handle($response) } $return_to = (strpos($return_to, "?") == false) ? $return_to . "?" . $query_string : $return_to . "&" . $query_string; - // RFC 8252 SS7.1 authority-less URIs (com.example.app:/cb?code=...) fail Laravel's - // UrlGenerator::isValidUrl(), so Redirect::to() would treat the already-validated redirect - // target as a RELATIVE path and prefix the site URL, corrupting the redirect. For an absolute - // URI (leading scheme) Laravel does not recognize, emit the Location verbatim - Symfony still - // rejects CR/LF in the header value, so no header-injection surface is opened. - $redirect = (!URL::isValidUrl($return_to) && preg_match('~^[A-Za-z][A-Za-z0-9+.\-]*:~', $return_to) === 1) - ? new RedirectResponse($return_to) - : Redirect::to($return_to); - - return $redirect - ->header('Cache-Control', 'no-cache, no-store, max-age=0, must-revalidate') - ->header('Pragma','no-cache'); + return $this->redirectTo($return_to); } } \ No newline at end of file diff --git a/app/Strategies/IndirectResponseUrlFragmentStrategy.php b/app/Strategies/IndirectResponseUrlFragmentStrategy.php index 2333facd..1665d207 100644 --- a/app/Strategies/IndirectResponseUrlFragmentStrategy.php +++ b/app/Strategies/IndirectResponseUrlFragmentStrategy.php @@ -11,17 +11,13 @@ * See the License for the specific language governing permissions and * limitations under the License. **/ -use Utils\IHttpResponseStrategy; -use Illuminate\Http\RedirectResponse; -use Illuminate\Support\Facades\Redirect; use Illuminate\Support\Facades\Response; -use Illuminate\Support\Facades\URL; /** * Class IndirectResponseUrlFragmentStrategy * Redirect and http response using a 302 adding params on url fragment * @package Strategies */ -class IndirectResponseUrlFragmentStrategy implements IHttpResponseStrategy +class IndirectResponseUrlFragmentStrategy extends AbstractIndirectResponseStrategy { /** @@ -39,15 +35,6 @@ public function handle($response) $return_to = (strpos($return_to, "#") == false) ? $return_to . "#" . $fragment : $return_to . "&" . $fragment; - // same RFC 8252 SS7.1 authority-less guard as IndirectResponseQueryStringStrategy: an absolute - // URI Laravel's UrlGenerator does not recognize must be emitted verbatim, or Redirect::to() - // prefixes the site URL and corrupts the already-validated redirect target. - $redirect = (!URL::isValidUrl($return_to) && preg_match('~^[A-Za-z][A-Za-z0-9+.\-]*:~', $return_to) === 1) - ? new RedirectResponse($return_to) - : Redirect::to($return_to); - - return $redirect - ->header('Cache-Control', 'no-cache, no-store, max-age=0, must-revalidate') - ->header('Pragma','no-cache'); + return $this->redirectTo($return_to); } } \ No newline at end of file diff --git a/app/libs/Utils/URLUtils.php b/app/libs/Utils/URLUtils.php index 459473ab..dac628f5 100644 --- a/app/libs/Utils/URLUtils.php +++ b/app/libs/Utils/URLUtils.php @@ -12,7 +12,7 @@ * limitations under the License. **/ -use AWS\CRT\Log; +use Illuminate\Support\Facades\Log; use URL\Normalizer; /** * Class URLUtils @@ -62,6 +62,55 @@ public static function canonicalUrl(string $url, bool $usePort = true):?string{ return rtrim($canonical_url, '/'); } + /** + * The single canonicalization pipeline every runtime URI-matching gate feeds BOTH sides through: + * canonicalUrl() (validity, scheme://host[:port]/path or RFC 8252 SS7.1 authority-less scheme:/path, + * query/fragment dropped, host/path lowercased) followed by normalizeUrl() (RFC 3986 normalization - + * scheme lowercased, default ports removed). Returns null when the URI cannot be canonicalized; + * callers treat that as "cannot match anything". + * + * @param string $uri + * @param bool $usePort + * @return string|null + */ + public static function canonicalizeForMatch(string $uri, bool $usePort = true):?string{ + $canonical = self::canonicalUrl($uri, $usePort); + if(empty($canonical)) return null; + $normalized = self::normalizeUrl($canonical); + return empty($normalized) ? null : $normalized; + } + + /** + * The single registered-URI-list matcher behind Client::isUriAllowed()/isPostLogoutUriAllowed()/ + * isOriginAllowed(): exact match of ANY of the requested canonical forms against EACH item of the + * stored comma-separated registration list, the registered side canonicalized through the same + * canonicalizeForMatch() pipeline the caller used for the requested side. Per-field matching + * differences (loopback port-agnostic redirects, the origin with/without-port dual form) are + * expressed by the CALLERS via $requested_canonicals/$registered_use_port - the matching + * algorithm itself exists only here. + * + * @param string[] $requested_canonicals already-canonicalized acceptable forms of the requested URI + * @param string|null $registered_csv the stored comma-separated registration list + * @param bool $registered_use_port whether registered items keep their explicit port when canonicalized + * @param string $log_context caller tag for debug traceability + * @return bool + */ + public static function anyCanonicalMatchesList(array $requested_canonicals, ?string $registered_csv, bool $registered_use_port, string $log_context):bool{ + if(empty($registered_csv)) return false; + foreach(explode(',', $registered_csv) as $registered_uri){ + $registered_uri = trim($registered_uri); + if(empty($registered_uri)) continue; + + $canonical_registered_uri = self::canonicalizeForMatch($registered_uri, $registered_use_port); + if(is_null($canonical_registered_uri)) continue; + + Log::debug(sprintf("%s comparing requested (%s) against registered %s", $log_context, implode('|', $requested_canonicals), $canonical_registered_uri)); + if(in_array($canonical_registered_uri, $requested_canonicals, true)) + return true; + } + return false; + } + /** * @param string $uri * @return bool diff --git a/docs/adr/0001-native-client-custom-uri-schemes.md b/docs/adr/0001-native-client-custom-uri-schemes.md index 40fbc5f1..71a3fdc9 100644 --- a/docs/adr/0001-native-client-custom-uri-schemes.md +++ b/docs/adr/0001-native-client-custom-uri-schemes.md @@ -32,7 +32,7 @@ Four consecutive adversarial code-review passes (xhigh-effort, multi-agent) surf 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`. Both custom-scheme URI shapes are supported end-to-end: the authority form (`myapp://callback`) and the **RFC 8252 §7.1 authority-less form** (`com.example.app:/oauth2redirect` — the RFC's own example, and the default shape AppAuth-based apps register). `URLUtils::canonicalUrl()` canonicalizes the authority-less form as scheme + rooted, lowercased path (query/fragment dropped, same as the authority form); the two forms are distinct URIs and never cross-match. Opaque URIs (`mailto:foo@bar` — no authority *and* no rooted path) remain rejected: there is no location to match a redirect against. The redirect *emitters* (`IndirectResponseQueryStringStrategy`/`IndirectResponseUrlFragmentStrategy`) also special-case this form: Laravel's `Redirect::to()` relies on the same `FILTER_VALIDATE_URL` check internally (`UrlGenerator::isValidUrl()`) and would otherwise treat the already-validated target as a relative path, prefixing the site URL — an absolute URI Laravel does not recognize is emitted as a verbatim `Location` header instead (Symfony still rejects CR/LF in header values, so no header-injection surface opens). 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`). At match time (`Client::isUriAllowed()`), a Native client's http-loopback request is additionally compared **port-agnostically**: RFC 8252 §7.3 requires the AS to allow any port specified at request time, because native apps bind an ephemeral loopback port per run. Only the port is ignored — scheme, host, and path still require an exact match, and the loopback hosts are not cross-matched against each other (registering `127.0.0.1` does not allow `localhost`). +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`). At match time (`Client::isUriAllowed()`), a Native client's http-loopback request is additionally compared **port-agnostically**: RFC 8252 §7.3 requires the AS to allow any port specified at request time, because native apps bind an ephemeral loopback port per run. Only the port is ignored — scheme, host, and path still require an exact match, and the loopback hosts are not cross-matched against each other (registering `127.0.0.1` does not allow `localhost`). The port-agnostic comparison deliberately applies to `isUriAllowed()` only: `isPostLogoutUriAllowed()` matches a registered loopback port exactly, since no spec extends the RFC 8252 §7.3 carve-out to RP-initiated logout. The rule is decided in one predicate (`Client::isRfc8252LoopbackRedirect()`) feeding one shared matcher (`URLUtils::anyCanonicalMatchesList()`, the single canonicalize-both-sides-then-exact-match implementation behind all three runtime gates), so revisiting that choice is a one-line change, not a re-implementation. 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`, plus a comma-space variant tolerating the legacy `", scheme://"` single-space artifact that the pre-hardening `create()` could persist; `ClientFactory::populate()` now trims each list item before normalizing, so new rows are always canonical regardless of write path. The item boundary is `scheme:/` rather than `scheme://`, so a scheme claimed via the authority-less RFC 8252 §7.1 form (`scheme:/path`) collides with one claimed via the authority form (`scheme://host`) and vice versa — the OS-level interception risk is about the scheme, not the URI shape it was registered in. 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 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. This closed gap is itself a deliberate API contract change beyond Native clients: `getCreatePayloadValidationRules()` previously declared none of the three URI fields, so `create()` accepted *any* value for them for every application type; the new `custom_url_set` rule enforces per-item https for non-Native types at create time. Automation that registered Web_App/JS clients with `http://` URIs (e.g. localhost dev tooling) or malformed lists now receives `412` where it previously got `201` — matching what `update()` always enforced for those types. From 9a800bf164281d2447521a4ac7db6d98c6afeeaa Mon Sep 17 00:00:00 2001 From: smarcet Date: Thu, 6 Aug 2026 15:14:53 -0300 Subject: [PATCH 23/25] fix(oauth2): ignore port when matching Native http-loopback post_logout_redirect_uris Completes the RFC 8252 SS7.3 story started in 4cc36118: the ephemeral-port reality is identical for a native app receiving its LOGOUT redirect on the loopback interface - it binds the port at request time, so the registered loopback post-logout URI cannot know it in advance. isPostLogoutUriAllowed() now feeds the same isRfc8252LoopbackRedirect() predicate into the shared matcher that isUriAllowed() uses (the one-line change the consolidation in 37c73d05 was built to make possible). Only the port is ignored: scheme, host and path stay exact, non-loopback http stays deny-listed, loopback hosts are not cross-matched. No spec mandates the carve-out for RP-initiated logout - the ADR (decision 3) records it as a deliberate extension. TDD: regression test confirmed failing first. Full Application suite: 197 tests / 1010 assertions, 0 failures; OTEL 23+12, 0 failures. Live-verified: registered http://127.0.0.1/logout, requested :49152 -> 302 with the ephemeral port preserved in Location; wrong path -> 400; localhost cross-match -> 400. --- app/Models/OAuth2/Client.php | 21 +++++++++------- .../0001-native-client-custom-uri-schemes.md | 2 +- tests/unit/ClientMappingTest.php | 25 +++++++++++++++++++ 3 files changed, 38 insertions(+), 10 deletions(-) diff --git a/app/Models/OAuth2/Client.php b/app/Models/OAuth2/Client.php index 3a17f1a5..a2b2f6c6 100644 --- a/app/Models/OAuth2/Client.php +++ b/app/Models/OAuth2/Client.php @@ -661,10 +661,10 @@ private function isNativeDangerousScheme(string $scheme, ?string $host = null): /** * RFC 8252 SS7.3: a Native client's http-loopback redirect binds an EPHEMERAL port at request * time - "the authorization server MUST allow any port to be specified at the time of the - * request for loopback IP redirect URIs". Single place this rule is decided; isUriAllowed() - * feeds it into the matching pipeline as "ignore the port on both sides". Deliberately NOT - * consulted by isPostLogoutUriAllowed() - no spec extends the carve-out to RP-initiated - * logout (see ADR-0001, decision 3). + * request for loopback IP redirect URIs". Single place this rule is decided; both redirect + * gates (isUriAllowed() per the RFC's mandate, isPostLogoutUriAllowed() by extension - the + * ephemeral-port reality is identical for a loopback logout redirect) feed it into the + * matching pipeline as "ignore the port on both sides" (see ADR-0001, decision 3). * * @param array|false $parts result of parse_url() on the requested URI * @return bool @@ -1197,16 +1197,19 @@ public function isPostLogoutUriAllowed($post_logout_uri) // exact match against each registered value, both sides through the same canonicalize+normalize // pipeline (URLUtils::anyCanonicalMatchesList): the full path is part of the comparison, // scheme/host stay case-insensitive, and query strings remain tolerated (dropped from both - // sides), so a client's dynamic ?state=.../?session=... params never break the match. The - // registered port is matched exactly - the RFC 8252 SS7.3 port carve-out deliberately applies - // to isUriAllowed() only (see isRfc8252LoopbackRedirect / ADR-0001 decision 3). - $requested_uri = URLUtils::canonicalizeForMatch($post_logout_uri); + // sides), so a client's dynamic ?state=.../?session=... params never break the match. Native + // http-loopback logout redirects are compared port-agnostically, same as isUriAllowed() - the + // app binds its loopback port at request time (see isRfc8252LoopbackRedirect / ADR-0001 + // decision 3); only the port is ignored, scheme/host/path stay exact. + $use_port = !$this->isRfc8252LoopbackRedirect($parts); + + $requested_uri = URLUtils::canonicalizeForMatch($post_logout_uri, $use_port); if(is_null($requested_uri)) return false; return URLUtils::anyCanonicalMatchesList( [$requested_uri], $this->post_logout_redirect_uris, - true, + $use_port, sprintf("Client::isPostLogoutUriAllowed client %s", $this->client_id)); } diff --git a/docs/adr/0001-native-client-custom-uri-schemes.md b/docs/adr/0001-native-client-custom-uri-schemes.md index 71a3fdc9..c437c5f6 100644 --- a/docs/adr/0001-native-client-custom-uri-schemes.md +++ b/docs/adr/0001-native-client-custom-uri-schemes.md @@ -32,7 +32,7 @@ Four consecutive adversarial code-review passes (xhigh-effort, multi-agent) surf 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`. Both custom-scheme URI shapes are supported end-to-end: the authority form (`myapp://callback`) and the **RFC 8252 §7.1 authority-less form** (`com.example.app:/oauth2redirect` — the RFC's own example, and the default shape AppAuth-based apps register). `URLUtils::canonicalUrl()` canonicalizes the authority-less form as scheme + rooted, lowercased path (query/fragment dropped, same as the authority form); the two forms are distinct URIs and never cross-match. Opaque URIs (`mailto:foo@bar` — no authority *and* no rooted path) remain rejected: there is no location to match a redirect against. The redirect *emitters* (`IndirectResponseQueryStringStrategy`/`IndirectResponseUrlFragmentStrategy`) also special-case this form: Laravel's `Redirect::to()` relies on the same `FILTER_VALIDATE_URL` check internally (`UrlGenerator::isValidUrl()`) and would otherwise treat the already-validated target as a relative path, prefixing the site URL — an absolute URI Laravel does not recognize is emitted as a verbatim `Location` header instead (Symfony still rejects CR/LF in header values, so no header-injection surface opens). 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`). At match time (`Client::isUriAllowed()`), a Native client's http-loopback request is additionally compared **port-agnostically**: RFC 8252 §7.3 requires the AS to allow any port specified at request time, because native apps bind an ephemeral loopback port per run. Only the port is ignored — scheme, host, and path still require an exact match, and the loopback hosts are not cross-matched against each other (registering `127.0.0.1` does not allow `localhost`). The port-agnostic comparison deliberately applies to `isUriAllowed()` only: `isPostLogoutUriAllowed()` matches a registered loopback port exactly, since no spec extends the RFC 8252 §7.3 carve-out to RP-initiated logout. The rule is decided in one predicate (`Client::isRfc8252LoopbackRedirect()`) feeding one shared matcher (`URLUtils::anyCanonicalMatchesList()`, the single canonicalize-both-sides-then-exact-match implementation behind all three runtime gates), so revisiting that choice is a one-line change, not a re-implementation. +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`). At match time (`Client::isUriAllowed()`), a Native client's http-loopback request is additionally compared **port-agnostically**: RFC 8252 §7.3 requires the AS to allow any port specified at request time, because native apps bind an ephemeral loopback port per run. Only the port is ignored — scheme, host, and path still require an exact match, and the loopback hosts are not cross-matched against each other (registering `127.0.0.1` does not allow `localhost`). The port-agnostic comparison applies to **both** redirect gates: `isUriAllowed()` per the RFC's mandate, and `isPostLogoutUriAllowed()` by extension — no spec covers RP-initiated-logout loopback redirects, but the ephemeral-port reality motivating the carve-out is identical for a native app receiving its logout redirect on the loopback interface. The rule is decided in one predicate (`Client::isRfc8252LoopbackRedirect()`) feeding one shared matcher (`URLUtils::anyCanonicalMatchesList()`, the single canonicalize-both-sides-then-exact-match implementation behind all three runtime gates). `isOriginAllowed()` keeps its own port semantics (a registered origin without an explicit port matches any requested port; one with a port requires that exact port) — origins are not redirect targets. 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`, plus a comma-space variant tolerating the legacy `", scheme://"` single-space artifact that the pre-hardening `create()` could persist; `ClientFactory::populate()` now trims each list item before normalizing, so new rows are always canonical regardless of write path. The item boundary is `scheme:/` rather than `scheme://`, so a scheme claimed via the authority-less RFC 8252 §7.1 form (`scheme:/path`) collides with one claimed via the authority form (`scheme://host`) and vice versa — the OS-level interception risk is about the scheme, not the URI shape it was registered in. 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 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. This closed gap is itself a deliberate API contract change beyond Native clients: `getCreatePayloadValidationRules()` previously declared none of the three URI fields, so `create()` accepted *any* value for them for every application type; the new `custom_url_set` rule enforces per-item https for non-Native types at create time. Automation that registered Web_App/JS clients with `http://` URIs (e.g. localhost dev tooling) or malformed lists now receives `412` where it previously got `201` — matching what `update()` always enforced for those types. diff --git a/tests/unit/ClientMappingTest.php b/tests/unit/ClientMappingTest.php index 9d6194d7..6f43998b 100644 --- a/tests/unit/ClientMappingTest.php +++ b/tests/unit/ClientMappingTest.php @@ -413,6 +413,31 @@ public function testIsUriAllowedNativeClientAcceptsAuthorityLessRfc8252Uri() $this->assertFalse($client->isUriAllowed('com.example.app:oauth2redirect')); } + public function testIsPostLogoutUriAllowedNativeClientMatchesHttpLoopbackRegardlessOfPort() + { + // same ephemeral-port reality as the authorization redirect (RFC 8252 SS7.3): a native app + // receiving its logout redirect on the loopback interface binds its port at request time, so + // the registered loopback post-logout URI cannot know it in advance. Port-agnostic matching + // applies to BOTH redirect gates via the same predicate (isRfc8252LoopbackRedirect): only the + // port is ignored - scheme, host and path stay exact, non-loopback http stays deny-listed, + // and loopback hosts are not cross-matched. + $client = new Client(); + $client->setApplicationType(IClient::ApplicationType_Native); + $client->setPostLogoutRedirectUris('http://127.0.0.1/logout,http://[::1]:8080/logout'); + + // registered without a port matches any requested port + $this->assertTrue($client->isPostLogoutUriAllowed('http://127.0.0.1:49152/logout')); + $this->assertTrue($client->isPostLogoutUriAllowed('http://127.0.0.1/logout')); + // registered WITH a port still matches any requested port (the port is ignored entirely) + $this->assertTrue($client->isPostLogoutUriAllowed('http://[::1]:51204/logout')); + // path stays exact + $this->assertFalse($client->isPostLogoutUriAllowed('http://127.0.0.1:49152/other')); + // non-loopback http stays rejected by the deny-list carve-out + $this->assertFalse($client->isPostLogoutUriAllowed('http://insecure.example.com:49152/logout')); + // loopback hosts are distinct - no cross-match + $this->assertFalse($client->isPostLogoutUriAllowed('http://localhost:49152/logout')); + } + public function testIsPostLogoutUriAllowedNativeClientAcceptsAuthorityLessUri() { // same RFC 8252 SS7.1 authority-less form, at the end-session gate: this one was additionally From 0205b197ff2e7f3fc58ad38aeda4782c739d20bb Mon Sep 17 00:00:00 2001 From: smarcet Date: Thu, 6 Aug 2026 16:05:15 -0300 Subject: [PATCH 24/25] fix(oauth2): enforce native scheme deny-list on redirect_uris UI, stop blocking legit custom schemes The Allowed Redirection Uris tag input had two defects for Native clients: - TagsInput's type="url" ran its own http(s)-only check on every entry, so a legitimate custom-scheme redirect URI (myapp://callback) could never be added through the UI at all - the reason custom-scheme redirect URIs historically had to be set via SQL. Dropped, exactly as the post-logout input already was; behavior-safe for non-Native types since their validator (https, no query) is stricter than the removed check. - validateRedirectURI() returned true for ANY parseable URL when Native - no deny-list - so javascript://x passed inline validation and only failed at save time with a backend 412, while the sibling post-logout field validated against window.DISALLOWED_NATIVE_URI_SCHEMES. Both fields now validate through one shared module (native_uri_schemes.js: isDisallowedNativeUriScheme + isValidNativeUri) reading the backend-injected policy - same single-source-of-truth consolidation the backend matcher got. First jest test in the repo covers the module (5 assertions, RED-first); enabling it surfaced that babel.config.js (consumed by babel-jest only - webpack carries its own inline presets) pinned corejs 3 while core-js@2 is installed, emitting unresolvable polyfill imports - dropped useBuiltIns there. Browser-verified against the local IDP admin (webpack build + edit-client): otherapp://cb and http://127.0.0.1:8080/cb now ADD as tags, javascript://x and http://evil.example.com/cb are rejected inline, save persists (myapp://callback/,otherapp://cb/,http://127.0.0.1:8080/cb in DB), and the post-logout field still validates through the shared module (myapp2://logout accepted, javascript://logout rejected). --- babel.config.js | 8 ++-- .../0001-native-client-custom-uri-schemes.md | 2 +- .../edit_client/components/logout_options.js | 20 ++------- .../components/native_uri_schemes.js | 30 +++++++++++++ .../components/native_uri_schemes.test.js | 44 +++++++++++++++++++ .../edit_client/components/oauth_panel.js | 10 +++-- 6 files changed, 90 insertions(+), 24 deletions(-) create mode 100644 resources/js/oauth2/profile/edit_client/components/native_uri_schemes.js create mode 100644 resources/js/oauth2/profile/edit_client/components/native_uri_schemes.test.js diff --git a/babel.config.js b/babel.config.js index bc853ec6..62d2842e 100644 --- a/babel.config.js +++ b/babel.config.js @@ -10,9 +10,11 @@ module.exports = { "chrome": "67", "safari": "11.1", "node":"current" - }, - "useBuiltIns": "usage", - "corejs": "3.9.1" + } + // no useBuiltIns/corejs: this config is consumed by babel-jest only (webpack.common.js + // carries its own inline babel options, no polyfills) and the corejs pin pointed at + // core-js@3 while the installed dependency is core-js@2 - polyfill imports emitted for + // jest could never resolve. Tests run under current node; no polyfills needed. } ], "@babel/preset-react", diff --git a/docs/adr/0001-native-client-custom-uri-schemes.md b/docs/adr/0001-native-client-custom-uri-schemes.md index c437c5f6..f157862a 100644 --- a/docs/adr/0001-native-client-custom-uri-schemes.md +++ b/docs/adr/0001-native-client-custom-uri-schemes.md @@ -31,7 +31,7 @@ Four consecutive adversarial code-review passes (xhigh-effort, multi-agent) surf ## 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`. Both custom-scheme URI shapes are supported end-to-end: the authority form (`myapp://callback`) and the **RFC 8252 §7.1 authority-less form** (`com.example.app:/oauth2redirect` — the RFC's own example, and the default shape AppAuth-based apps register). `URLUtils::canonicalUrl()` canonicalizes the authority-less form as scheme + rooted, lowercased path (query/fragment dropped, same as the authority form); the two forms are distinct URIs and never cross-match. Opaque URIs (`mailto:foo@bar` — no authority *and* no rooted path) remain rejected: there is no location to match a redirect against. The redirect *emitters* (`IndirectResponseQueryStringStrategy`/`IndirectResponseUrlFragmentStrategy`) also special-case this form: Laravel's `Redirect::to()` relies on the same `FILTER_VALIDATE_URL` check internally (`UrlGenerator::isValidUrl()`) and would otherwise treat the already-validated target as a relative path, prefixing the site URL — an absolute URI Laravel does not recognize is emitted as a verbatim `Location` header instead (Symfony still rejects CR/LF in header values, so no header-injection surface opens). -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.)* +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`); the inline validators for **both** URI fields (`logout_options.js` and `oauth_panel.js`'s `redirect_uris`) read them through one shared module (`native_uri_schemes.js`) rather than maintaining copies. The `redirect_uris` tag input also dropped its `type="url"` attribute — the tag component's own http(s)-only check silently blocked every custom-scheme entry regardless of the validator, which is why custom-scheme redirect URIs historically had to be set via SQL. *(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`). At match time (`Client::isUriAllowed()`), a Native client's http-loopback request is additionally compared **port-agnostically**: RFC 8252 §7.3 requires the AS to allow any port specified at request time, because native apps bind an ephemeral loopback port per run. Only the port is ignored — scheme, host, and path still require an exact match, and the loopback hosts are not cross-matched against each other (registering `127.0.0.1` does not allow `localhost`). The port-agnostic comparison applies to **both** redirect gates: `isUriAllowed()` per the RFC's mandate, and `isPostLogoutUriAllowed()` by extension — no spec covers RP-initiated-logout loopback redirects, but the ephemeral-port reality motivating the carve-out is identical for a native app receiving its logout redirect on the loopback interface. The rule is decided in one predicate (`Client::isRfc8252LoopbackRedirect()`) feeding one shared matcher (`URLUtils::anyCanonicalMatchesList()`, the single canonicalize-both-sides-then-exact-match implementation behind all three runtime gates). `isOriginAllowed()` keeps its own port semantics (a registered origin without an explicit port matches any requested port; one with a port requires that exact port) — origins are not redirect targets. 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`, plus a comma-space variant tolerating the legacy `", scheme://"` single-space artifact that the pre-hardening `create()` could persist; `ClientFactory::populate()` now trims each list item before normalizing, so new rows are always canonical regardless of write path. The item boundary is `scheme:/` rather than `scheme://`, so a scheme claimed via the authority-less RFC 8252 §7.1 form (`scheme:/path`) collides with one claimed via the authority form (`scheme://host`) and vice versa — the OS-level interception risk is about the scheme, not the URI shape it was registered in. 5. **Defense-in-depth**: the runtime allow-gates independently re-check the scheme deny-list; write-time validation is not the sole enforcement point. 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 02eece45..113a2173 100644 --- a/resources/js/oauth2/profile/edit_client/components/logout_options.js +++ b/resources/js/oauth2/profile/edit_client/components/logout_options.js @@ -10,16 +10,7 @@ import TagsInput, {getTags} from "../../../../components/tags_input"; import styles from "./common.module.scss"; -// 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); -} +import {isValidNativeUri} from "./native_uri_schemes"; const LogoutOptions = ({appTypes, initialValues, onSavePromise}) => { const [loading, setLoading] = useState(false); @@ -27,14 +18,9 @@ const LogoutOptions = ({appTypes, initialValues, onSavePromise}) => { const validatePostLogoutRedirectURI = (value) => { // 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. + // (javascript:, data:, intent:, ...): matches the backend deny-list (see native_uri_schemes.js). if (initialValues.application_type === appTypes.Native) { - try { - const url = new URL(value); - return url.protocol === 'https:' || !isDisallowedNativeUriScheme(url.protocol, url.hostname); - } catch (err) { - return false; - } + return isValidNativeUri(value); } const regex = /^https:\/\/([\w@][\w.:@]+)\/?[\w\.?=%&=\-@/$,]*$/ig; return regex.test(value); diff --git a/resources/js/oauth2/profile/edit_client/components/native_uri_schemes.js b/resources/js/oauth2/profile/edit_client/components/native_uri_schemes.js new file mode 100644 index 00000000..8c977ef1 --- /dev/null +++ b/resources/js/oauth2/profile/edit_client/components/native_uri_schemes.js @@ -0,0 +1,30 @@ +/** + * Native-client URI scheme policy, client side. 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. Single module so every URI field's + * inline validator (redirect_uris, post_logout_redirect_uris) shares one implementation instead of + * hand-rolling copies that drift. + */ + +export 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); +} + +/** + * Inline validity of a single URI for a Native client's URI-bearing fields: https always passes; + * anything else passes unless its scheme is deny-listed (with the RFC 8252 http-loopback carve-out). + * Matches the backend write-time rule (ClientService::assertNativeCustomSchemesAllowed). + */ +export const isValidNativeUri = (value) => { + try { + const url = new URL(value); + return url.protocol === 'https:' || !isDisallowedNativeUriScheme(url.protocol, url.hostname); + } catch (err) { + return false; + } +} diff --git a/resources/js/oauth2/profile/edit_client/components/native_uri_schemes.test.js b/resources/js/oauth2/profile/edit_client/components/native_uri_schemes.test.js new file mode 100644 index 00000000..c77159a2 --- /dev/null +++ b/resources/js/oauth2/profile/edit_client/components/native_uri_schemes.test.js @@ -0,0 +1,44 @@ +import {isDisallowedNativeUriScheme, isValidNativeUri} from "./native_uri_schemes"; + +// the module reads the backend-injected policy at call time (see edit-client.blade.php); +// mirror a representative subset of IClient::DISALLOWED_NATIVE_URI_SCHEMES / NATIVE_LOOPBACK_HOSTS +beforeEach(() => { + window.DISALLOWED_NATIVE_URI_SCHEMES = ['javascript', 'data', 'intent', 'file', 'itms-services']; + window.NATIVE_LOOPBACK_HOSTS = ['127.0.0.1', '::1', '[::1]', 'localhost']; +}); + +describe('isDisallowedNativeUriScheme', () => { + it('rejects deny-listed schemes, case-insensitively', () => { + expect(isDisallowedNativeUriScheme('javascript:', '')).toBe(true); + expect(isDisallowedNativeUriScheme('JAVASCRIPT:', '')).toBe(true); + expect(isDisallowedNativeUriScheme('intent:', 'scan')).toBe(true); + }); + + it('allows genuine custom app schemes', () => { + expect(isDisallowedNativeUriScheme('myapp:', 'callback')).toBe(false); + expect(isDisallowedNativeUriScheme('com.example.app:', '')).toBe(false); + }); + + it('applies the RFC 8252 loopback carve-out to http', () => { + expect(isDisallowedNativeUriScheme('http:', '127.0.0.1')).toBe(false); + expect(isDisallowedNativeUriScheme('http:', 'localhost')).toBe(false); + expect(isDisallowedNativeUriScheme('http:', 'evil.example.com')).toBe(true); + expect(isDisallowedNativeUriScheme('http:', '')).toBe(true); + }); +}); + +describe('isValidNativeUri', () => { + it('accepts https, custom schemes, the RFC 8252 authority-less form and http loopback', () => { + expect(isValidNativeUri('https://web.example.com/cb')).toBe(true); + expect(isValidNativeUri('myapp://callback')).toBe(true); + expect(isValidNativeUri('com.example.app:/oauth2redirect')).toBe(true); + expect(isValidNativeUri('http://127.0.0.1:8080/cb')).toBe(true); + }); + + it('rejects deny-listed schemes, non-loopback http and unparseable values', () => { + expect(isValidNativeUri('javascript://x%0aalert(1)')).toBe(false); + expect(isValidNativeUri('itms-services://x/?action=download-manifest')).toBe(false); + expect(isValidNativeUri('http://evil.example.com/cb')).toBe(false); + expect(isValidNativeUri('not a uri')).toBe(false); + }); +}); diff --git a/resources/js/oauth2/profile/edit_client/components/oauth_panel.js b/resources/js/oauth2/profile/edit_client/components/oauth_panel.js index 24d35131..4d206af3 100644 --- a/resources/js/oauth2/profile/edit_client/components/oauth_panel.js +++ b/resources/js/oauth2/profile/edit_client/components/oauth_panel.js @@ -11,6 +11,7 @@ import CheckCircleIcon from '@material-ui/icons/CheckCircle'; import InfoOutlinedIcon from "@material-ui/icons/InfoOutlined"; import RefreshIcon from "@material-ui/icons/Refresh"; import Swal from "sweetalert2"; +import {isValidNativeUri} from "./native_uri_schemes"; import { Box, Button, @@ -56,10 +57,14 @@ const OauthPanel = ({ } const validateRedirectURI = (value) => { + // native clients: custom app schemes / https / RFC 8252 http loopback, minus the backend + // deny-list - one shared implementation with the post-logout validator (native_uri_schemes.js). + if (application_type === appTypes.Native) { + return isValidNativeUri(value); + } try { const url = new URL(value); - return application_type === appTypes.Native ? true : url.protocol === 'https:' - && url.search === ''; + return url.protocol === 'https:' && url.search === ''; } catch (err) { return false; } @@ -362,7 +367,6 @@ const OauthPanel = ({ fullWidth size="small" variant="outlined" - type="url" onChange={formik.handleChange} tags={getTags(formik.values.redirect_uris)} isValid={validateRedirectURI} From 01ecbf5ca3256308390853e6a8ddcf6f20e748f0 Mon Sep 17 00:00:00 2001 From: smarcet Date: Thu, 6 Aug 2026 16:38:20 -0300 Subject: [PATCH 25/25] fix(oauth2): reject opaque URIs at write time - the runtime can never match them com.example.app:oauth2redirect (opaque - no authority AND no rooted path, the one-character typo of the RFC 8252 SS7.1 form) passed every write-time check: validateCustomUrl only requires a scheme, the deny-list doesn't list it, and the ':/'-anchored uniqueness LIKE can't even see it. It got stored (with URL\Normalizer deprecation noise from populate()) and then URLUtils::canonicalizeForMatch() returned null forever - a silently dead registration and an undiagnosable support case: the registered value and the value the app sends are IDENTICAL, and the login still fails. Unlike the round-7 authority-less fix (where the runtime learned to match the RFC form), opaque URIs cannot be supported - they name no location to redirect to - so the coherent close is the mirror image: write-time now applies the SAME canonicalizable rule as the runtime matcher. assertNativeCustomSchemes- Allowed() rejects any URI canonicalizeForMatch() returns null for, with a 412 naming the accepted shapes (scheme://host/... or scheme:/path). One source of truth on both ends - they cannot drift again. The inline validator agrees (isValidNativeUri requires an authority or a rooted path), so the typo is caught while typing, not at save. Contract note (Native clients only): an opaque URI value that previously returned 201 and stored a dead row now returns 412. No working client can break - the opaque form never matched at runtime. Legacy opaque rows are inert (every gate already ignores them); no migration or audit needed. TDD both ends (API test 201->412 and jest case confirmed failing first). Application suite: 198 tests / 1011 assertions, 0 failures; OTEL 23+12. Browser-verified on the local IDP admin after webpack rebuild: the opaque form is rejected inline in BOTH URI fields, the rooted RFC form still adds fine (com.example.app:/oauth2redirect, com.example.app:/logout). --- app/Services/OAuth2/ClientService.php | 9 ++++++++ .../0001-native-client-custom-uri-schemes.md | 2 +- .../components/native_uri_schemes.js | 7 +++++- .../components/native_uri_schemes.test.js | 6 +++++ tests/ClientApiTest.php | 23 +++++++++++++++++++ 5 files changed, 45 insertions(+), 2 deletions(-) diff --git a/app/Services/OAuth2/ClientService.php b/app/Services/OAuth2/ClientService.php index 446cc28e..89eba221 100644 --- a/app/Services/OAuth2/ClientService.php +++ b/app/Services/OAuth2/ClientService.php @@ -13,6 +13,7 @@ **/ use App\Events\OAuth2ClientLocked; +use App\libs\Utils\URLUtils; use App\Models\OAuth2\Factories\ClientFactory; use App\Services\AbstractService; use Auth\Repositories\IUserRepository; @@ -322,6 +323,14 @@ private function assertNativeCustomSchemesAllowed(array $payload, int $exclude_c if (Client::isDisallowedNativeUriScheme($scheme, $parts['host'] ?? null)) { throw new ValidationException(sprintf('scheme %s:// is not allowed.', $scheme)); } + // write/runtime symmetry: if the runtime matcher cannot canonicalize the value + // (URLUtils::canonicalizeForMatch - requires an authority or a rooted path), it can + // never match a redirect, so storing it would create a silently dead registration. + // Rejects opaque URIs like "com.example.app:oauth2redirect" (the one-character typo + // of the RFC 8252 SS7.1 form) with a clear 412 at registration time instead. + if (is_null(URLUtils::canonicalizeForMatch(trim($uri)))) { + throw new ValidationException(sprintf('uri %s on %s is not an acceptable redirect location - use scheme://host/... or scheme:/path.', trim($uri), $field)); + } if (HttpUtils::isCustomSchema($scheme) && $this->client_repository->hasCustomSchemeRegisteredOnAnotherClientThan($exclude_client_id, $scheme)) { throw new ValidationException(sprintf('schema %s:// already registered for another client.', $scheme)); diff --git a/docs/adr/0001-native-client-custom-uri-schemes.md b/docs/adr/0001-native-client-custom-uri-schemes.md index f157862a..12a774bd 100644 --- a/docs/adr/0001-native-client-custom-uri-schemes.md +++ b/docs/adr/0001-native-client-custom-uri-schemes.md @@ -30,7 +30,7 @@ Four consecutive adversarial code-review passes (xhigh-effort, multi-agent) surf ## 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`. Both custom-scheme URI shapes are supported end-to-end: the authority form (`myapp://callback`) and the **RFC 8252 §7.1 authority-less form** (`com.example.app:/oauth2redirect` — the RFC's own example, and the default shape AppAuth-based apps register). `URLUtils::canonicalUrl()` canonicalizes the authority-less form as scheme + rooted, lowercased path (query/fragment dropped, same as the authority form); the two forms are distinct URIs and never cross-match. Opaque URIs (`mailto:foo@bar` — no authority *and* no rooted path) remain rejected: there is no location to match a redirect against. The redirect *emitters* (`IndirectResponseQueryStringStrategy`/`IndirectResponseUrlFragmentStrategy`) also special-case this form: Laravel's `Redirect::to()` relies on the same `FILTER_VALIDATE_URL` check internally (`UrlGenerator::isValidUrl()`) and would otherwise treat the already-validated target as a relative path, prefixing the site URL — an absolute URI Laravel does not recognize is emitted as a verbatim `Location` header instead (Symfony still rejects CR/LF in header values, so no header-injection surface opens). +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`. Both custom-scheme URI shapes are supported end-to-end: the authority form (`myapp://callback`) and the **RFC 8252 §7.1 authority-less form** (`com.example.app:/oauth2redirect` — the RFC's own example, and the default shape AppAuth-based apps register). `URLUtils::canonicalUrl()` canonicalizes the authority-less form as scheme + rooted, lowercased path (query/fragment dropped, same as the authority form); the two forms are distinct URIs and never cross-match. Opaque URIs (`mailto:foo@bar`, or the one-character typo `com.example.app:oauth2redirect` — no authority *and* no rooted path) are rejected at **both ends**: the runtime matcher cannot canonicalize them (there is no location to match a redirect against), and write-time validation (`assertNativeCustomSchemesAllowed()`) applies the same `canonicalizeForMatch()` rule, returning a clear `412` instead of storing a registration the runtime would silently never match. The redirect *emitters* (`IndirectResponseQueryStringStrategy`/`IndirectResponseUrlFragmentStrategy`) also special-case this form: Laravel's `Redirect::to()` relies on the same `FILTER_VALIDATE_URL` check internally (`UrlGenerator::isValidUrl()`) and would otherwise treat the already-validated target as a relative path, prefixing the site URL — an absolute URI Laravel does not recognize is emitted as a verbatim `Location` header instead (Symfony still rejects CR/LF in header values, so no header-injection surface opens). 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`); the inline validators for **both** URI fields (`logout_options.js` and `oauth_panel.js`'s `redirect_uris`) read them through one shared module (`native_uri_schemes.js`) rather than maintaining copies. The `redirect_uris` tag input also dropped its `type="url"` attribute — the tag component's own http(s)-only check silently blocked every custom-scheme entry regardless of the validator, which is why custom-scheme redirect URIs historically had to be set via SQL. *(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`). At match time (`Client::isUriAllowed()`), a Native client's http-loopback request is additionally compared **port-agnostically**: RFC 8252 §7.3 requires the AS to allow any port specified at request time, because native apps bind an ephemeral loopback port per run. Only the port is ignored — scheme, host, and path still require an exact match, and the loopback hosts are not cross-matched against each other (registering `127.0.0.1` does not allow `localhost`). The port-agnostic comparison applies to **both** redirect gates: `isUriAllowed()` per the RFC's mandate, and `isPostLogoutUriAllowed()` by extension — no spec covers RP-initiated-logout loopback redirects, but the ephemeral-port reality motivating the carve-out is identical for a native app receiving its logout redirect on the loopback interface. The rule is decided in one predicate (`Client::isRfc8252LoopbackRedirect()`) feeding one shared matcher (`URLUtils::anyCanonicalMatchesList()`, the single canonicalize-both-sides-then-exact-match implementation behind all three runtime gates). `isOriginAllowed()` keeps its own port semantics (a registered origin without an explicit port matches any requested port; one with a port requires that exact port) — origins are not redirect targets. 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`, plus a comma-space variant tolerating the legacy `", scheme://"` single-space artifact that the pre-hardening `create()` could persist; `ClientFactory::populate()` now trims each list item before normalizing, so new rows are always canonical regardless of write path. The item boundary is `scheme:/` rather than `scheme://`, so a scheme claimed via the authority-less RFC 8252 §7.1 form (`scheme:/path`) collides with one claimed via the authority form (`scheme://host`) and vice versa — the OS-level interception risk is about the scheme, not the URI shape it was registered in. diff --git a/resources/js/oauth2/profile/edit_client/components/native_uri_schemes.js b/resources/js/oauth2/profile/edit_client/components/native_uri_schemes.js index 8c977ef1..753b1942 100644 --- a/resources/js/oauth2/profile/edit_client/components/native_uri_schemes.js +++ b/resources/js/oauth2/profile/edit_client/components/native_uri_schemes.js @@ -23,7 +23,12 @@ export const isDisallowedNativeUriScheme = (protocol, host) => { export const isValidNativeUri = (value) => { try { const url = new URL(value); - return url.protocol === 'https:' || !isDisallowedNativeUriScheme(url.protocol, url.hostname); + if (url.protocol === 'https:') return true; + if (isDisallowedNativeUriScheme(url.protocol, url.hostname)) return false; + // opaque URIs (scheme:data - no authority AND no rooted path, e.g. the one-character typo + // "com.example.app:oauth2redirect") have no location to redirect to; the runtime matcher can + // never canonicalize them, and the backend rejects them at write time with a 412 - agree inline. + return url.hostname !== '' || url.pathname.startsWith('/'); } catch (err) { return false; } diff --git a/resources/js/oauth2/profile/edit_client/components/native_uri_schemes.test.js b/resources/js/oauth2/profile/edit_client/components/native_uri_schemes.test.js index c77159a2..d1da717b 100644 --- a/resources/js/oauth2/profile/edit_client/components/native_uri_schemes.test.js +++ b/resources/js/oauth2/profile/edit_client/components/native_uri_schemes.test.js @@ -41,4 +41,10 @@ describe('isValidNativeUri', () => { expect(isValidNativeUri('http://evil.example.com/cb')).toBe(false); expect(isValidNativeUri('not a uri')).toBe(false); }); + + it('rejects opaque URIs (no authority and no rooted path) that the runtime can never match', () => { + // the one-character typo of the RFC 8252 SS7.1 form - backend write-time validation rejects + // it with a 412 (assertNativeCustomSchemesAllowed), the inline validator must agree + expect(isValidNativeUri('com.example.app:oauth2redirect')).toBe(false); + }); }); diff --git a/tests/ClientApiTest.php b/tests/ClientApiTest.php index c3933ee9..7f552af1 100644 --- a/tests/ClientApiTest.php +++ b/tests/ClientApiTest.php @@ -503,6 +503,29 @@ public function testUpdateNativeClientNotTouchingUriFieldsIgnoresLockContention( } } + public function testUpdateNativeClientRejectsOpaqueUriThatRuntimeCanNeverMatch(){ + + // "com.example.app:oauth2redirect" (opaque - no authority AND no rooted path, the one-character + // typo of the RFC 8252 SS7.1 form) used to pass every write-time check (scheme present, not + // deny-listed, invisible to the ":/"-anchored uniqueness LIKE) and get stored - but + // URLUtils::canonicalizeForMatch() returns null for it, so every runtime gate rejects it + // forever: a silently dead registration. Write-time now applies the SAME canonicalizable rule + // as the runtime matcher, turning the dead registration into a clean 412. + $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' => 'com.example.app:oauth2redirect', + ), + [], + [], + []); + + $this->assertResponseStatus(412); + } + public function testUpdateNativeClientRejectsSchemeAlreadyRegisteredInAuthorityLessForm(){ // RFC 8252 SS7.1 authority-less registrations (com.example.app:/oauth2redirect) store the scheme