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/Http/Controllers/Api/ClientApiController.php b/app/Http/Controllers/Api/ClientApiController.php index 961e3e96..a507143a 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', @@ -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/Models/OAuth2/Client.php b/app/Models/OAuth2/Client.php index c01be875..8341c5c9 100644 --- a/app/Models/OAuth2/Client.php +++ b/app/Models/OAuth2/Client.php @@ -629,6 +629,35 @@ public function isScopeAllowed(string $scope):bool return $res; } + /** + * 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 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 && self::isDisallowedNativeUriScheme($scheme, $host); + } + /** * @param string $uri * @return bool @@ -636,6 +665,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)); @@ -651,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; } @@ -809,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() @@ -1097,18 +1163,44 @@ 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']; - - if(str_contains($this->post_logout_redirect_uris, $logout_without_port )) return true; + // 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; - if(isset($parts['port'])) - { - $logout_with_port = $parts['scheme'].'://'.$parts['host'].':'.$parts['port']; - return str_contains($this->post_logout_redirect_uris, $logout_with_port ); + // 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; + + // 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/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..446cc28e 100644 --- a/app/Services/OAuth2/ClientService.php +++ b/app/Services/OAuth2/ClientService.php @@ -39,14 +39,40 @@ 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; + + /** + * 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 */ @@ -74,6 +100,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 +114,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 +124,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 +135,48 @@ 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.'); + } + } + + /** + * 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; } @@ -220,6 +295,41 @@ public function getCurrentClientAuthInfo() throw new InvalidClientAuthMethodException; } + /** + * Native clients may register genuine custom app URI schemes (e.g. myapp://, com.example.app://) in + * 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 + * @throws ValidationException + */ + private function assertNativeCustomSchemesAllowed(array $payload, int $exclude_client_id = -1): void + { + 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)); + if (!isset($parts['scheme'])) { + throw new ValidationException(sprintf('invalid scheme on %s uri.', $field)); + } + $scheme = strtolower($parts['scheme']); + if (Client::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 @@ -227,37 +337,51 @@ public function getCurrentClientAuthInfo() */ 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); + } - $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 + && $this->payloadTouchesNativeCustomSchemeFields($payload)) { + return $this->withNativeCustomSchemeLock($do_create); + } + + return $do_create(); } @@ -270,68 +394,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.'); - } - - ClientFactory::populate($client, $payload); + 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.'); + } - // validate uris - switch($client->getApplicationType()) { - case IClient::ApplicationType_Native: { + ClientFactory::populate($client, $payload); - 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 (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'])); + // 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.'); } - } else { - if (!HttpUtils::isHttpSchema($uri['scheme'])) { - throw new ValidationException(sprintf('scheme %s:// is invalid.', - $uri['scheme'])); + if (!HttpUtils::isHttpsSchema($uri['scheme'])) { + throw new ValidationException(sprintf('scheme %s:// is invalid.', $uri['scheme'])); } } } } - } - } - 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) { + 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'])); @@ -339,37 +456,32 @@ 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 + && $this->payloadTouchesNativeCustomSchemeFields($payload)) { + return $this->withNativeCustomSchemeLock($do_update); + } + + return $do_update(); } /** 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/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/app/libs/OAuth2/Models/IClient.php b/app/libs/OAuth2/Models/IClient.php index fd69a40f..0fdf991c 100644 --- a/app/libs/OAuth2/Models/IClient.php +++ b/app/libs/OAuth2/Models/IClient.php @@ -37,6 +37,42 @@ 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']; + + /** + * 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/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/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/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..05670508 --- /dev/null +++ b/docs/adr/0001-native-client-custom-uri-schemes.md @@ -0,0 +1,67 @@ +# 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 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 + +- **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. +- `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):** +- 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 + +- 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) 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..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,10 +10,32 @@ import TagsInput, {getTags} from "../../../../components/tags_input"; import styles from "./common.module.scss"; -const LogoutOptions = ({initialValues, onSavePromise}) => { +// 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://...), 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 url = new URL(value); + return url.protocol === 'https:' || !isDisallowedNativeUriScheme(url.protocol, url.hostname); + } catch (err) { + return false; + } + } const regex = /^https:\/\/([\w@][\w.:@]+)\/?[\w\.?=%&=\-@/$,]*$/ig; return regex.test(value); } @@ -72,7 +94,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/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"))!!}'; diff --git a/tests/ClientApiTest.php b/tests/ClientApiTest.php index 3b08ba9d..218d8aba 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 */ @@ -96,4 +99,379 @@ 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 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']); + + $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); + } + + 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); + } + + 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); + } + } + + 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 diff --git a/tests/UserLoginTurnstileTest.php b/tests/UserLoginTurnstileTest.php index f13b3db8..e525db98 100644 --- a/tests/UserLoginTurnstileTest.php +++ b/tests/UserLoginTurnstileTest.php @@ -41,11 +41,15 @@ final class UserLoginTurnstileTest extends BrowserKitTestCase protected function prepareForTests(): void { parent::prepareForTests(); - $this->testEmail = env('TEST_USER_EMAIL'); - $this->testPassword = env('TEST_USER_PASSWORD'); - if (empty($this->testEmail) || empty($this->testPassword)) { + // read into locals first: assigning null to the typed string properties would + // throw a TypeError before the skip guard below can run + $testEmail = env('TEST_USER_EMAIL'); + $testPassword = env('TEST_USER_PASSWORD'); + if (empty($testEmail) || empty($testPassword)) { $this->markTestSkipped('TEST_USER_EMAIL and TEST_USER_PASSWORD env vars are required.'); } + $this->testEmail = $testEmail; + $this->testPassword = $testPassword; Session::start(); } diff --git a/tests/unit/ClientMappingTest.php b/tests/unit/ClientMappingTest.php index aa827dc5..da9d2853 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,217 @@ 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 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 + // 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')); + } + + 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')); + } + + 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')); + } + + 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')); + } } 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); + } + } +} diff --git a/tests/unit/OAuth2LoginStrategyTest.php b/tests/unit/OAuth2LoginStrategyTest.php index 701b9d11..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; @@ -54,7 +55,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); @@ -88,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(); + } } /** @@ -213,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'); + } }