OpenStackweb · smarcet · Jul 14, 2026 · Jul 15, 2026 · Jul 15, 2026 · Jul 15, 2026
diff --git 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
@@ -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
@@ -629,14 +629,60 @@ 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
      */
     public function isUriAllowed(string $uri):bool
     {
         Log::debug(sprintf("Client::isUriAllowed client %s original uri %s", $this->client_id, $uri));
-        $uri = URLUtils::canonicalUrl($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;
+        }
+
+        // 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;
@@ -651,13 +697,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/<anything>").
+            $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;
         }
 
@@ -809,9 +865,35 @@ 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);
+        // 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);
-        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(empty($canonical_allowed_origin)) continue;
+
+            if($originWithoutPort === $canonical_allowed_origin) return true;
+            if($originWithPort !== null && $originWithPort === $canonical_allowed_origin) return true;
+        }
+
+        return false;
     }
 
     public function getWebsite()
@@ -1097,18 +1179,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/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
@@ -163,19 +163,51 @@ 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 . '://%';
+        // 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_space')
+            );
+        };
+
+        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("after_comma_space", $after_comma_space)
             ->setParameter("id", $id)
             ->setMaxResults(1)
             ->getQuery()