fix(oauth2): validate redirect_uris scheme/uniqueness on client create, not just update

CodeRabbit PR review (#147): ClientService::create() validated
allowed_origins/post_logout_redirect_uris for Native clients but never
redirect_uris - a create payload could register a dangerous scheme
(javascript://, etc.) or a scheme already claimed by another client
through redirect_uris, only ever caught later by the runtime
isUriAllowed() gate rather than at write time.

assertNativeCustomSchemesAllowed() already ran both the deny-list
check and the cross-client uniqueness check generically per field
(since the earlier hardening pass); extending its field list to
include redirect_uris closes the gap for both create() and update()
via one shared method. This also let update()'s separate, by-then
fully-duplicate inline redirect_uris validation loop be deleted.

Verified: 156 tests / 0 failures.

Plan: docs/plans/2026-07-14-native-clients-custom-schemes.md (Task 9)
ADR: docs/adr/0001-native-client-custom-uri-schemes.md

Author: smarcet

app/Services/OAuth2/ClientService.php

@@ -222,20 +222,21 @@ public function getCurrentClientAuthInfo()
 
     /**
      * Native clients may register genuine custom app URI schemes (e.g. myapp://, com.example.app://) in
-     * allowed_origins and post_logout_redirect_uris. They may NOT register plain http:// outside the RFC 8252
-     * loopback carve-out, nor dangerous/launch pseudo-schemes (javascript:, data:, intent:, ...): at
-     * end-session these fields become live 302 redirect targets. See IClient::DISALLOWED_NATIVE_URI_SCHEMES
-     * for the deny-list (shared with the runtime allow-gates in Client::isUriAllowed/isPostLogoutUriAllowed).
-     * Also enforces the same cross-client scheme-uniqueness rule redirect_uris already has, since a scheme
-     * claimed by another client here creates the identical OS-level interception risk.
+     * redirect_uris, allowed_origins, and post_logout_redirect_uris. They may NOT register plain http://
+     * outside the RFC 8252 loopback carve-out, nor dangerous/launch pseudo-schemes (javascript:, data:,
+     * intent:, ...): redirect_uris carries the authorization code, and the other two fields become live
+     * 302 redirect targets at end-session. See IClient::DISALLOWED_NATIVE_URI_SCHEMES for the deny-list
+     * (shared with the runtime allow-gates in Client::isUriAllowed/isPostLogoutUriAllowed). Also enforces
+     * cross-client scheme uniqueness: a scheme claimed by another client in any of these fields creates the
+     * same OS-level interception risk regardless of which field either client used it in.
      *
      * @param array $payload
      * @param int $exclude_client_id the client being written; -1 (never a real id) when creating a new one
      * @throws ValidationException
      */
     private function assertNativeCustomSchemesAllowed(array $payload, int $exclude_client_id = -1): void
     {
-        foreach (['allowed_origins', 'post_logout_redirect_uris'] as $field) {
+        foreach (['redirect_uris', 'allowed_origins', 'post_logout_redirect_uris'] as $field) {
             if (empty($payload[$field])) continue;
             foreach (explode(',', $payload[$field]) as $uri) {
                 $parts = @parse_url(trim($uri));
@@ -272,8 +273,8 @@ public function create(array $payload):IEntity
                 throw new ValidationException('there is already another application with that name, please choose another one.');
             }
 
-            // close the create-path bypass: the same scheme allow-list update() enforces (only reachable
-            // for native clients, where the runtime https gate is relaxed).
+            // same scheme deny-list + cross-client uniqueness rule update() enforces (only reachable
+            // for native clients, where the runtime https gate is relaxed) - now covers redirect_uris too.
             if (($payload['application_type'] ?? null) === IClient::ApplicationType_Native) {
                 $this->assertNativeCustomSchemesAllowed($payload);
             }
@@ -337,31 +338,9 @@ public function update(int $id, array $payload):IEntity
             // validate uris
             switch($client->getApplicationType()) {
                 case IClient::ApplicationType_Native: {
-
-                    if (isset($payload['redirect_uris'])) {
-                        $redirect_uris = explode(',', $payload['redirect_uris']);
-                        //check that custom schema does not already exists for another registerd app
-                        if (!empty($payload['redirect_uris'])) {
-                            foreach ($redirect_uris as $uri) {
-                                $uri = @parse_url($uri);
-                                if (!isset($uri['scheme'])) {
-                                    throw new ValidationException('invalid scheme on redirect uri.');
-                                }
-                                if (Client::isDisallowedNativeUriScheme($uri['scheme'], $uri['host'] ?? null)) {
-                                    throw new ValidationException(sprintf('scheme %s:// is not allowed.', $uri['scheme']));
-                                }
-                                // the else branch previously here (rejecting non-http(s) "non-custom" schemes)
-                                // is unreachable now: ftp/file and non-loopback http are already rejected by
-                                // the deny-list check above, and https/loopback-http both satisfy isHttpSchema.
-                                if (HttpUtils::isCustomSchema($uri['scheme'])
-                                    && $this->client_repository->hasCustomSchemeRegisteredOnAnotherClientThan($id, $uri['scheme'])) {
-                                    throw new ValidationException(sprintf('schema %s:// already registered for another client.',
-                                        $uri['scheme']));
-                                }
-                            }
-                        }
-                    }
-
+                    // redirect_uris, allowed_origins, and post_logout_redirect_uris all share the same
+                    // scheme deny-list + cross-client uniqueness rule; assertNativeCustomSchemesAllowed
+                    // validates whichever of the three are present in the payload.
                     $this->assertNativeCustomSchemesAllowed($payload, $id);
                 }
                 break;

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

@@ -54,9 +54,9 @@ Four consecutive adversarial code-review passes (xhigh-effort, multi-agent) surf
 
 **Corrected during review (not a trade-off — fixed before merge):**
 - The deny-list/loopback-host constants were initially placed on `Utils\Http\HttpUtils` (a generic scheme-classification helper) and hand-duplicated as a second literal array in `logout_options.js`. Both were flagged in PR review: the frontend duplication as an unmaintained "keep in sync" liability, and the `HttpUtils` placement as the wrong dependency direction (a generic utility class encoding OAuth2-Native-client domain policy). Relocated to `IClient` (data) + `Client` (predicate logic) and wired the admin UI to read the values from the backend at render time instead of maintaining its own copy.
+- `ClientService::create()` initially never validated `redirect_uris` scheme or cross-client uniqueness at all — originally accepted as a trade-off (mitigated by the runtime allow-gate). CodeRabbit's automated PR review re-flagged it; by that point `assertNativeCustomSchemesAllowed()` already did both checks generically per field, so closing the gap was a two-line change (add `redirect_uris` to its field list) that additionally let `update()`'s separate, now-fully-duplicate inline `redirect_uris` loop be deleted (~25 lines). No longer a trade-off — `create()` and `update()` now enforce identically for all three fields via one shared method.
 
 **Accepted trade-offs (not fixed in this change, documented for a future pass if warranted):**
-- `ClientService::create()` still never validates `redirect_uris` scheme or cross-client uniqueness at all (a pre-existing gap, unrelated to the deny-list itself) — mitigated by the runtime allow-gate rejecting a dangerous scheme regardless of how it reached storage, but write-time hygiene on that one path remains weaker than `update()`.
 - The deny-list can never be exhaustive against every OS/browser/app-launcher scheme that might exist now or in the future (a structural property of any blocklist). A `search-ms://`-style scheme not yet on the list would be accepted. Closing this fully requires an allow-list architecture, a larger change than this ADR's scope.
 
 ## References

tests/ClientApiTest.php

@@ -272,6 +272,62 @@ public function testCreateNativeClientRejectsDangerousSchemeOnPostLogout(){
         $this->assertResponseStatus(412);
     }
 
+    public function testCreateNativeClientRejectsDangerousSchemeOnRedirectUris(){
+
+        // CodeRabbit PR #147 finding: create() validated allowed_origins/post_logout_redirect_uris for
+        // dangerous schemes but never redirect_uris - a create payload could register javascript:// etc.
+        // there and it would only ever be caught later by the runtime isUriAllowed() gate, not at write time.
+        $user = EntityManager::getRepository(User::class)->findOneBy(['identifier' => 'sebastian.marcet']);
+
+        $data = array(
+            'user_id'          => $user->id,
+            'app_name'         => 'native_dangerous_redirect_uri_app',
+            'app_description'  => 'native app with dangerous scheme on redirect_uris',
+            'application_type' => IClient::ApplicationType_Native,
+            'redirect_uris'    => 'javascript://x%0aalert(1)',
+        );
+
+        $response = $this->action("POST", "Api\\ClientApiController@create",
+            $data,
+            [],
+            [],
+            []);
+
+        $this->assertResponseStatus(412);
+    }
+
+    public function testCreateNativeClientRejectsRedirectUriSchemeAlreadyRegisteredByAnotherClient(){
+
+        // CodeRabbit PR #147 finding, cross-client uniqueness half: create() never checked redirect_uris
+        // scheme collisions either (only update() did, via a separate now-removed inline loop).
+        $existing = EntityManager::getRepository(Client::class)->findOneBy(['app_name' => 'oauth2_native_app']);
+        $response = $this->action("PUT", "Api\\ClientApiController@update",
+            array(
+                'id'               => $existing->id,
+                'application_type' => IClient::ApplicationType_Native,
+                'redirect_uris'    => 'createuniqueness://callback',
+            ),
+            [],
+            [],
+            []);
+        $this->assertResponseStatus(201);
+
+        $user = EntityManager::getRepository(User::class)->findOneBy(['identifier' => 'sebastian.marcet']);
+        $response = $this->action("POST", "Api\\ClientApiController@create",
+            array(
+                'user_id'          => $user->id,
+                'app_name'         => 'native_duplicate_redirect_scheme_app',
+                'app_description'  => 'native app registering an already-claimed redirect_uris scheme',
+                'application_type' => IClient::ApplicationType_Native,
+                'redirect_uris'    => 'createuniqueness://other',
+            ),
+            [],
+            [],
+            []);
+
+        $this->assertResponseStatus(412);
+    }
+
     public function testUpdateJsClientRejectsCustomSchemeOnPostLogoutUrisAndAllowedOrigins(){
 
         $client = EntityManager::getRepository(Client::class)->findOneBy(['app_name' => 'oauth2_test_app_public_2']);