fix(oauth2): exact-match redirect_uris, declare scheme predicate on IClient

isUriAllowed() matched a registered redirect_uri via str_contains() against
the requested URI - a prefix/substring check, not an exact match. Registering
"myapp://callback" therefore also permitted "myapp://callback/<anything>":
any path appended after the registered value passed, on the field that
carries the OAuth2 authorization code. Both sides now go through the same
canonicalUrl()+normalizeUrl() pipeline and are compared with strict equality;
query strings remain tolerated exactly as before (canonicalUrl already drops
them from both sides).

Also:
- Declare isDisallowedNativeUriScheme() on IClient alongside the constants it
  interprets, matching the interface-first convention every other predicate
  on Client already follows.
- Add a regression test for the path-suffix bypass.
- Correct ADR 0001 decision item 6, which still claimed create() never
  validates redirect_uris - stale relative to its own Consequences section
  and the actual assertNativeCustomSchemesAllowed() field list.

Author: smarcet

app/Models/OAuth2/Client.php

@@ -687,13 +687,23 @@ public function isUriAllowed(string $uri):bool
             return false;
         }
 
-        $redirect_uris = explode(',',strtolower($this->redirect_uris));
+        $redirect_uris = explode(',', $this->redirect_uris);
         $uri = URLUtils::normalizeUrl($uri);
         if(empty($uri)) return false;
         foreach($redirect_uris as $redirect_uri){
+            $redirect_uri = trim($redirect_uri);
             if(empty($redirect_uri)) continue;
-            Log::debug(sprintf("Client::isUriAllowed url %s client %s redirect_uri %s", $uri, $this->client_id, $redirect_uri));
-            if(str_contains($uri, $redirect_uri))
+
+            // symmetric normalization: compare both sides through the same canonicalize+normalize
+            // pipeline, then require an exact match - a registered value must no longer be accepted
+            // merely as a *prefix* of the requested URI (e.g. "myapp://callback" matching any
+            // "myapp://callback/<anything>").
+            $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;
         }
 

app/libs/OAuth2/Models/IClient.php

@@ -61,6 +61,18 @@ interface IClient extends IEntity
      */
     const array NATIVE_LOOPBACK_HOSTS = ['127.0.0.1', '::1', '[::1]', 'localhost'];
 
+    /**
+     * Single source of truth for "is this scheme disallowed for a Native client's URI fields", per
+     * DISALLOWED_NATIVE_URI_SCHEMES / NATIVE_LOOPBACK_HOSTS above. Declared here (contract) and
+     * implemented on Client (body) like every other predicate on this interface; kept static because
+     * ClientService::create() must validate a scheme before a Client entity exists to call it on.
+     *
+     * @param string $scheme
+     * @param string|null $host enables the RFC 8252 http-loopback carve-out (see NATIVE_LOOPBACK_HOSTS)
+     * @return bool
+     */
+    public static function isDisallowedNativeUriScheme(string $scheme, ?string $host = null): bool;
+
     /**
      * @return int
      */

docs/adr/0001-native-client-custom-uri-schemes.md

@@ -35,7 +35,7 @@ Four consecutive adversarial code-review passes (xhigh-effort, multi-agent) surf
 3. **`http` is a special case with an RFC 8252 loopback carve-out**: disallowed everywhere except `127.0.0.1` / `::1` / `localhost` (`IClient::NATIVE_LOOPBACK_HOSTS`).
 4. **Cross-client scheme uniqueness** (`IClientRepository::hasCustomSchemeRegisteredOnAnotherClientThan`) checks all three URI columns together — a scheme claimed by another client in *any* of the three fields blocks re-registration in any of the three, since the OS-level interception risk is identical regardless of which field either client used. The query anchors matches to real list-item boundaries (start-of-field or immediately after a comma) rather than an unanchored substring `LIKE`.
 5. **Defense-in-depth**: the runtime allow-gates independently re-check the scheme deny-list; write-time validation is not the sole enforcement point.
-6. **Enforced on both write paths** (`create()` and `update()`) for `allowed_origins`/`post_logout_redirect_uris`. `redirect_uris` scheme validation remains `update()`-only, matching its pre-existing (unchanged) behavior — `create()` never validated `redirect_uris` at all, before or after this change (see Consequences).
+6. **Enforced on both write paths** (`create()` and `update()`) for all three fields, including `redirect_uris`. `redirect_uris` initially had no request-level validation in `create()` at all — closed during review (see Consequences) by adding it to the same `assertNativeCustomSchemesAllowed()` field loop already used for the other two fields.
 7. **The `allowed_origins` admin UI input stays hidden for Native clients.** No runtime path enforces `allowed_origins` for Native today — both the IDP's own `OAuth2BearerAccessTokenRequestValidator` middleware and summit-api's equivalent gate the origin check to `application_type === JS_Client`. The field remains settable via the admin API only (the value ships in token-introspection responses and may be enforced by a resource server in the future), but exposing a UI control for a value nothing currently checks was judged not worth the surface.
 
 ### Alternatives considered

tests/unit/ClientMappingTest.php

@@ -252,4 +252,21 @@ public function testIsUriAllowedNativeClientRejectsHostlessUriWithoutError()
         $this->assertFalse($client->isUriAllowed('mailto:foo@bar.com'));
         $this->assertFalse($client->isUriAllowed('file:///etc/passwd'));
     }
+
+    public function testIsUriAllowedNativeClientRejectsPathSuffixOnRegisteredRedirectUri()
+    {
+        // isUriAllowed() previously matched via str_contains($uri, $redirect_uri) - a substring/prefix
+        // check, not an exact match. Registering "myapp://callback/safe" therefore also permitted
+        // "myapp://callback/other" and "myapp://callback/safe/extra": any path appended after the
+        // registered value passed. redirect_uris carries the OAuth2 authorization code, so an exact
+        // match is required here (query strings remain tolerated - canonicalUrl() strips them from
+        // both sides before comparison).
+        $client = new Client();
+        $client->setApplicationType(IClient::ApplicationType_Native);
+        $client->setRedirectUris('myapp://callback/safe');
+
+        $this->assertTrue($client->isUriAllowed('myapp://callback/safe'));
+        $this->assertFalse($client->isUriAllowed('myapp://callback/other'));
+        $this->assertFalse($client->isUriAllowed('myapp://callback/safe/extra'));
+    }
 }