feat(oauth2): support custom URI schemes for Native clients across redirect_uris, allowed_origins, post_logout_redirect_uris - #147
Conversation
…direct_uris, allowed_origins, post_logout_redirect_uris Native (mobile/desktop) OAuth2 clients can now register custom app schemes (myapp://callback) in allowed_origins and post_logout_redirect_uris via the admin API and React UI, matching the support redirect_uris already had. The OIDC end-session flow honors a registered custom-scheme post-logout URI at runtime. Security hardening (found via adversarial code review): - Deny-list for dangerous/launch pseudo-schemes (javascript:, data:, intent:, etc.) and plain http, centralized in HttpUtils and shared by write-time validation (ClientService) and runtime allow-gates (Client::isUriAllowed/isPostLogoutUriAllowed). - RFC 8252 loopback carve-out: http://127.0.0.1|localhost redirect URIs remain allowed for Native clients (the standard native-app pattern), only non-loopback http is blocked. - Cross-client custom-scheme uniqueness check extended to all three URI fields (was redirect_uris only), preventing OS-level scheme interception between clients. - Defense-in-depth: runtime gates independently re-check the scheme deny-list rather than relying solely on write-time validation. - Fixed a pre-existing crash (missing array key "host") in URLUtils::canonicalUrl/Client::isPostLogoutUriAllowed for host-less custom-scheme URIs (mailto:, file:///x). - Fixed a pre-existing substring false-positive in the cross-client scheme collision check (e.g. "roipapp" matching inside "androipapp://..."). Also fixes an unrelated pre-existing bug in UserLoginTurnstileTest where assigning null (from an unset env var) to a typed string property threw a TypeError before the intended skip-guard could run. Plan: docs/plans/2026-07-14-native-clients-custom-schemes.md
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughNative OAuth2 URI validation now uses shared deny-list and loopback policies across API writes, runtime URI checks, repository collision detection, and the admin editor. URL-set validation, host-less URI handling, locking, and regression coverage are also updated. ChangesNative URI validation
Test environment guard
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AdminEditor
participant ClientApiController
participant ClientService
participant ILockManagerService
participant Client
participant ClientRepository
AdminEditor->>ClientApiController: submit URI fields
ClientApiController->>ClientService: validate create or update payload
ClientService->>ILockManagerService: acquire native scheme lock
ClientService->>Client: evaluate scheme and host policy
ClientService->>ClientRepository: check custom scheme ownership
ClientRepository-->>ClientService: collision result
ClientService-->>ClientApiController: validation response
ClientApiController-->>AdminEditor: success or validation error
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-147/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Repositories/DoctrineOAuth2ClientRepository.php (1)
193-207: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftEnforce scheme claims atomically
This count-then-write check has a TOCTOU race: concurrent registrations can both pass before either write lands. Enforce the claim at the database layer with a unique constraint or equivalent locking.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Repositories/DoctrineOAuth2ClientRepository.php` around lines 193 - 207, Replace the count-based duplicate check in the repository method containing the shown query with database-enforced atomic uniqueness for redirect URI, post-logout URI, and allowed-origin scheme claims. Add or use a unique constraint or equivalent locking mechanism, and make the registration write handle constraint conflicts so concurrent registrations cannot both succeed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Models/OAuth2/Client.php`:
- Around line 1138-1148: Update the post-logout URI matching in the surrounding
OAuth2 client method to split the registered values on comma boundaries and
compare each complete URI against the requested URI, preserving path, query, and
other case-sensitive components while normalizing only scheme, host, and port as
appropriate. Remove the substring-based checks using stored_post_logout_uris,
logout_without_port, and logout_with_port, and add a regression test covering
identical scheme/host values with different paths.
In `@app/Services/OAuth2/ClientService.php`:
- Around line 275-279: Extend assertNativeCustomSchemesAllowed to validate
redirect_uris in addition to allowed_origins and post_logout_redirect_uris, and
invoke this shared helper during native client creation and update. Remove or
replace the duplicated redirect validation in update() so both paths enforce the
same scheme allow-list and duplicate checks.
In `@resources/js/oauth2/profile/edit_client/components/logout_options.js`:
- Around line 14-30: Update validatePostLogoutRedirectURI in the native-client
branch to allow http: URLs only when their host is a permitted loopback address,
matching HttpUtils and the backend’s RFC 8252 exception; continue rejecting
non-loopback HTTP and all disallowed schemes while preserving HTTPS and valid
custom schemes.
---
Outside diff comments:
In `@app/Repositories/DoctrineOAuth2ClientRepository.php`:
- Around line 193-207: Replace the count-based duplicate check in the repository
method containing the shown query with database-enforced atomic uniqueness for
redirect URI, post-logout URI, and allowed-origin scheme claims. Add or use a
unique constraint or equivalent locking mechanism, and make the registration
write handle constraint conflicts so concurrent registrations cannot both
succeed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 644ca4c9-a306-4e85-988a-fbc8613df5c7
📒 Files selected for processing (12)
app/Http/Controllers/Api/ClientApiController.phpapp/Models/OAuth2/Client.phpapp/Repositories/DoctrineOAuth2ClientRepository.phpapp/Services/OAuth2/ClientService.phpapp/libs/OAuth2/Repositories/IClientRepository.phpapp/libs/Utils/Http/HttpUtils.phpapp/libs/Utils/URLUtils.phpresources/js/oauth2/profile/edit_client/components/logout_options.jsresources/js/oauth2/profile/edit_client/components/security_settings_panel.jstests/ClientApiTest.phptests/UserLoginTurnstileTest.phptests/unit/ClientMappingTest.php
…ils to IClient/Client; stop duplicating it in the frontend PR review feedback: the scheme deny-list (DISALLOWED_NATIVE_URI_SCHEMES) belonged on IClient (OAuth2 domain policy for Native clients), not HttpUtils (a generic HTTP scheme classifier unrelated to any particular client type). And logout_options.js was hand-duplicating the list in JS with a "keep in sync" comment - an unenforced, easily-drifting contract. - IClient: new DISALLOWED_NATIVE_URI_SCHEMES / NATIVE_LOOPBACK_HOSTS consts (interfaces can't hold method bodies, so the data lives here). - Client: new public static isDisallowedNativeUriScheme() - the predicate that interprets those consts, callable from ClientService without an instantiated entity. isNativeDangerousScheme() now delegates to it. - HttpUtils: reverted to a pure generic scheme classifier (isCustomSchema/isHttpSchema/isHttpsSchema only) - no OAuth2/Native domain knowledge. - AdminController -> edit-client.blade.php -> window.*: the admin UI now reads window.DISALLOWED_NATIVE_URI_SCHEMES / window.NATIVE_LOOPBACK_HOSTS, injected server-side from the IClient constants (same mechanism already used for window.APP_TYPES). logout_options.js has zero hardcoded scheme knowledge left. - Side effect: the frontend never had an RFC 8252 http-loopback carve-out before (the old hardcoded list unconditionally rejected http:); now correctly mirrors the backend's loopback exception. Verified: 154 tests / 0 failures; live browser check confirms window.DISALLOWED_NATIVE_URI_SCHEMES / window.NATIVE_LOOPBACK_HOSTS are populated from IClient and the validator behaves identically (dangerous scheme rejected, loopback http accepted, custom scheme accepted). Plan: docs/plans/2026-07-14-native-clients-custom-schemes.md (Task 8) ADR: docs/adr/0001-native-client-custom-uri-schemes.md
Documents the decision, the security findings from 4 review passes (RFC 8252 loopback carve-out, cross-client scheme uniqueness, defense-in-depth, deny-list vs allow-list trade-off), and the post-review correction to deny-list ownership (IClient/Client instead of HttpUtils; backend-served to the frontend instead of duplicated).
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-147/ This page is automatically updated on each push to this PR. |
1 similar comment
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-147/ This page is automatically updated on each push to this PR. |
…e, 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
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-147/ This page is automatically updated on each push to this PR. |
getCreatePayloadValidationRules() did not declare redirect_uris, allowed_origins, or post_logout_redirect_uris, so create() applied zero request-level validation to them. Two consequences: - A list with a space after the comma (e.g. "https://a.com, myapp://cb") reached storage verbatim (ClientFactory::populate only trims the whole payload string, not each item), silently defeating hasCustomSchemeRegisteredOnAnotherClientThan()'s comma-boundary anchored match and letting a second client register the same custom scheme unopposed. - A non-string value (e.g. a JSON array) reached assertNativeCustomSchemesAllowed() untouched, where explode() threw an uncaught TypeError (not an Exception) instead of a clean 412. Add the same custom_url_set:application_type rules update() already uses to getCreatePayloadValidationRules(), closing both gaps at the shared root cause. Also harden CustomValidator::validateCustomUrlSet() with an is_string() guard, since Laravel invokes the rule callback with the raw value regardless of rule order, so declaring the rule alone doesn't stop the TypeError. Regression tests added first (confirmed red before the fix): testCreateNativeClientRejectsCustomSchemeWithLeadingSpaceInList and testCreateNativeClientRejectsArrayValueForRedirectUrisCleanly. Full suite: 158 tests, 527 assertions, 0 failures.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-147/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Http/Controllers/Api/ClientApiController.php (1)
702-703: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRequire
application_typeto prevent 500 errors and ensure URL validation.The
custom_url_set:application_typevalidation rule relies onapplication_typebeing present in the request data. Ifapplication_typeis omitted from thePUTpayload, the rule silently returnstrue(skipping the URI validation entirely). The request then reachesClientService::update(), which throws an HTTP 500ErrorExceptiondue to an undefined array key when unconditionally checking$payload['application_type'](line 331).To fix this and ensure a clean HTTP 412 response, uncomment and enforce the
application_typevalidation rule. For consistency with thecreatevalidation rules, consider also explicitly adding thestringconstraint to the URI fields.🛠️ Proposed changes to `getUpdatePayloadValidationRules`
First, uncomment
application_typenear the top of the array:- //'application_type' => 'required|application_type', + 'application_type' => 'required|applicationtype',Then, update the URI fields:
- 'redirect_uris' => 'nullable|custom_url_set:application_type', 'policy_uri' => 'nullable|url', - 'post_logout_redirect_uris' => 'nullable|custom_url_set:application_type', - 'allowed_origins' => 'nullable|custom_url_set:application_type', + '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',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Http/Controllers/Api/ClientApiController.php` around lines 702 - 703, Update getUpdatePayloadValidationRules to require application_type using the existing application type validation rule, preventing ClientService::update from receiving an incomplete payload. Add the string constraint to post_logout_redirect_uris and allowed_origins to match the create validation rules while preserving their custom_url_set:application_type validation.
🧹 Nitpick comments (1)
tests/ClientApiTest.php (1)
290-295: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused local variables.
The
$responsevariable is assigned but never used, as the internal response state is verified separately via$this->assertResponseStatus(). Removing the assignment cleans up static analysis warnings.
tests/ClientApiTest.php#L290-L295: remove the unused$response =assignment.tests/ClientApiTest.php#L304-L312: remove the unused$response =assignment.tests/ClientApiTest.php#L316-L326: remove the unused$response =assignment.tests/ClientApiTest.php#L376-L386: remove the unused$response =assignment.tests/ClientApiTest.php#L401-L411: remove the unused$response =assignment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/ClientApiTest.php` around lines 290 - 295, Remove the unused $response assignment from each $this->action("POST", "Api\\ClientApiController@create", ...) call in tests/ClientApiTest.php at lines 290-295, 304-312, 316-326, 376-386, and 401-411, while preserving the existing calls and separate $this->assertResponseStatus() verification.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/Http/Controllers/Api/ClientApiController.php`:
- Around line 702-703: Update getUpdatePayloadValidationRules to require
application_type using the existing application type validation rule, preventing
ClientService::update from receiving an incomplete payload. Add the string
constraint to post_logout_redirect_uris and allowed_origins to match the create
validation rules while preserving their custom_url_set:application_type
validation.
---
Nitpick comments:
In `@tests/ClientApiTest.php`:
- Around line 290-295: Remove the unused $response assignment from each
$this->action("POST", "Api\\ClientApiController@create", ...) call in
tests/ClientApiTest.php at lines 290-295, 304-312, 316-326, 376-386, and
401-411, while preserving the existing calls and separate
$this->assertResponseStatus() verification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46153607-d804-48fa-85da-1164176fa8ee
📒 Files selected for processing (10)
app/Http/Controllers/AdminController.phpapp/Http/Controllers/Api/ClientApiController.phpapp/Models/OAuth2/Client.phpapp/Services/OAuth2/ClientService.phpapp/Validators/CustomValidator.phpapp/libs/OAuth2/Models/IClient.phpdocs/adr/0001-native-client-custom-uri-schemes.mdresources/js/oauth2/profile/edit_client/components/logout_options.jsresources/views/oauth2/profile/edit-client.blade.phptests/ClientApiTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- resources/js/oauth2/profile/edit_client/components/logout_options.js
- app/Models/OAuth2/Client.php
…Client 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.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-147/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Models/OAuth2/Client.php (1)
854-861: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winExact match required for allowed origins.
The origin verification uses
str_containsagainst the comma-separatedallowed_originslist. This permits dangerous substring bypasses. For example, if a registered origin ishttps://my-app.example.com, an attacker who registers the domainhttps://my-app.example.co(orhttps://my-app) will be granted CORS access because the registered string contains the attacker's requested origin.Split the
allowed_originslist and perform an exact match against each registered origin to prevent this authorization bypass.🔒️ Proposed fix to require exact origin matches
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; $originWithPort = URLUtils::canonicalUrl($origin); - return str_contains($this->allowed_origins, URLUtils::normalizeUrl($originWithPort)); + + $normalizedWithoutPort = URLUtils::normalizeUrl($originWithoutPort); + $normalizedWithPort = $originWithPort ? URLUtils::normalizeUrl($originWithPort) : null; + + $allowed = explode(',', $this->allowed_origins); + foreach ($allowed as $allowed_origin) { + $allowed_origin = trim($allowed_origin); + if (empty($allowed_origin)) continue; + + $canonical_allowed = URLUtils::canonicalUrl($allowed_origin); + if (empty($canonical_allowed)) continue; + $canonical_allowed = URLUtils::normalizeUrl($canonical_allowed); + + if ($normalizedWithoutPort === $canonical_allowed || ($normalizedWithPort && $normalizedWithPort === $canonical_allowed)) { + return true; + } + } + return false; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Models/OAuth2/Client.php` around lines 854 - 861, Update OAuth2\Client::isOriginAllowed to split the comma-separated allowed_origins value and compare the normalized requested origin against each registered origin using exact equality, rather than str_contains. Apply this exact-match behavior for both canonical origin variants while preserving the existing empty-origin rejection.
♻️ Duplicate comments (1)
app/Models/OAuth2/Client.php (1)
1162-1174: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winMatch the complete registered post-logout URI.
The check authorizes only by substring-matching
scheme://host[:port], ignoring the path, query, and list-item boundaries. For example, registeringmyapp://callback/safealso permitsmyapp://callback/other.Parse the comma-separated registrations and compare each complete URI, normalizing only case-insensitive components. Add a regression test with identical scheme/host but a different path.
🔒️ Proposed fix to securely match the complete post-logout URI
- // scheme/host are case-insensitive (RFC 3986); the write path normally lowercases the stored value, - // but match case-insensitively regardless so a bypassing write path can't silently break matching. - $stored_post_logout_uris = strtolower($this->post_logout_redirect_uris); - $logout_without_port = strtolower($parts['scheme'].'://'.$parts['host']); - - if(str_contains($stored_post_logout_uris, $logout_without_port )) return true; - - if(isset($parts['port'])) - { - $logout_with_port = $logout_without_port.':'.$parts['port']; - return str_contains($stored_post_logout_uris, $logout_with_port ); - } - return false; + // Parse the comma-separated registrations and require an exact match using the same + // canonicalize+normalize pipeline used for redirect_uris. + $canonical_requested = URLUtils::canonicalUrl($post_logout_uri); + if (empty($canonical_requested)) return false; + $canonical_requested = URLUtils::normalizeUrl($canonical_requested); + if (empty($canonical_requested)) return false; + + $stored_uris = explode(',', $this->post_logout_redirect_uris); + foreach ($stored_uris as $stored_uri) { + $stored_uri = trim($stored_uri); + if (empty($stored_uri)) continue; + + $canonical_stored = URLUtils::canonicalUrl($stored_uri); + if (empty($canonical_stored)) continue; + $canonical_stored = URLUtils::normalizeUrl($canonical_stored); + + if ($canonical_requested === $canonical_stored) { + return true; + } + } + return false;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Models/OAuth2/Client.php` around lines 1162 - 1174, Replace the substring checks in the post-logout matching logic with parsing of each comma-separated registered URI and comparison against the complete requested URI. Normalize only scheme and host (and port as appropriate), while preserving path, query, and list-item boundaries; update the relevant OAuth2 client test with a same-scheme/host, different-path regression case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/Models/OAuth2/Client.php`:
- Around line 854-861: Update OAuth2\Client::isOriginAllowed to split the
comma-separated allowed_origins value and compare the normalized requested
origin against each registered origin using exact equality, rather than
str_contains. Apply this exact-match behavior for both canonical origin variants
while preserving the existing empty-origin rejection.
---
Duplicate comments:
In `@app/Models/OAuth2/Client.php`:
- Around line 1162-1174: Replace the substring checks in the post-logout
matching logic with parsing of each comma-separated registered URI and
comparison against the complete requested URI. Normalize only scheme and host
(and port as appropriate), while preserving path, query, and list-item
boundaries; update the relevant OAuth2 client test with a same-scheme/host,
different-path regression case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb0b1850-8dea-4c4f-b61d-70ccca90ccac
📒 Files selected for processing (4)
app/Models/OAuth2/Client.phpapp/libs/OAuth2/Models/IClient.phpdocs/adr/0001-native-client-custom-uri-schemes.mdtests/unit/ClientMappingTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/adr/0001-native-client-custom-uri-schemes.md
- tests/unit/ClientMappingTest.php
…abbit-flagged gap isPostLogoutUriAllowed() matched a registered value via str_contains() of only scheme://host[:port] against the whole registered CSV string - the path was never part of the comparison at all. Registering "myapp://callback/safe" therefore also permitted "myapp://callback/other" or "myapp://callback/safe/ extra". This mirrors the fix already applied to isUriAllowed() (bc5d118): both sides now go through the same canonicalUrl()+normalizeUrl() pipeline and are compared with strict equality per registered entry. Query strings remain tolerated - canonicalUrl() drops them from both sides, so dynamic per-request ?session=/?state=... params still match. Adds regression tests for the path-suffix bypass and for continued dynamic query-string acceptance.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-147/ This page is automatically updated on each push to this PR. |
…g-bypass gap isOriginAllowed() (CORS origin check for JS_Client, gated by OAuth2BearerAccessTokenRequestValidator) still compared via str_contains($this->allowed_origins, $normalizedOrigin) - a registered origin like https://my-app.example.com incorrectly matched a requested https://my-app.example.co, since the latter is a literal string prefix of the former. This is the same bypass class already fixed on isUriAllowed()/isPostLogoutUriAllowed() earlier in this PR, flagged Critical by CodeRabbit but left unaddressed on this sibling method (surfaced by /review-pr-deep on PR #147). Rewritten to the same explode+trim+canonicalize+normalize+exact-match pipeline as the two sibling methods. As a side effect this also fixes an asymmetry bug where the registered side was never normalized, so an exact-value registration could fail to match itself once normalizeUrl() appended a trailing slash to the request side only. Regression tests added to ClientMappingTest: the substring-prefix bypass (RED before this fix), and the pre-existing with/without-port matching semantics (a registered origin with no port matches any request port; one with an explicit port only matches that port). Verified: ClientMappingTest 14/14, and full "Application Test Suite" 163/163 (0 failures/errors) after a clean doctrine:migrations rebuild. Also documents the accepted trade-off (docs/adr/0001): the exact-match rewrite on isUriAllowed()/isPostLogoutUriAllowed() changes matching behavior for every application type, not only Native, with no pre-deploy audit of existing registrations - accepted given the small client base.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-147/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Models/OAuth2/Client.php (1)
690-706: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftCritical authorization bypass: Ignoring query strings and path casing in URI validation.
Both
isUriAllowedandisPostLogoutUriAllowedrely onURLUtils::canonicalUrl()for their exact equality checks. Because this helper explicitly drops query strings and lowercases paths, the URI validation ignores these components.While the inline comments and test (
testIsPostLogoutUriAllowedNativeClientAcceptsDynamicQueryString) indicate this was intentionally done to tolerate dynamic?state=or?session=parameters in the redirect URI, this fundamentally violates OAuth 2.0 exact-matching requirements (RFC 6749 Section 3.1.2.2). Allowing clients to dynamically append query strings introduces a severe open-redirect and token-leakage vulnerability. If multiple applications share a callback domain but isolate tenants via query parameters (e.g.,https://shared.com/callback?tenant=victimvstenant=attacker), an attacker can supply malicious query parameters that the server ignores during validation but blindly follows during redirection. Dynamic parameters likestatemust always be sent as separate, top-level authorization request parameters, never dynamically embedded by the client into theredirect_uri.To fix this securely, enforce exact string matching on the path and query string:
app/Models/OAuth2/Client.php#L690-L706: Retain the original requested$uri(before it is overwritten bycanonicalUrl()at line 675). Update the loop to require that($requested_parts['path'] ?? '') === ($registered_parts['path'] ?? '')and($requested_parts['query'] ?? '') === ($registered_parts['query'] ?? ''), alongside the base canonical URL equality. Also, ensureClientMappingTest.phpis updated to reject dynamic query strings.app/Models/OAuth2/Client.php#L1182-L1203: Apply the exact same strict, case-sensitive path and query string equality requirement when matching the original$post_logout_uriagainst each$registered_uri.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Models/OAuth2/Client.php` around lines 690 - 706, The URI validation in Client::isUriAllowed and Client::isPostLogoutUriAllowed must preserve exact, case-sensitive path and query matching instead of relying solely on URLUtils::canonicalUrl(). Retain each original requested URI before canonicalization, parse requested and registered URIs, and require matching base canonical URLs plus identical path and query components; update ClientMappingTest.php to reject dynamic query-string variants.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Models/OAuth2/Client.php`:
- Around line 858-880: Add empty-value guards after each
URLUtils::normalizeUrl() call in the origin-matching flow, including the
requested origin values and canonical_allowed_origin, before performing strict
equality comparisons. Ensure malformed origins with null normalization results
are skipped or rejected, matching the established behavior in
isPostLogoutUriAllowed.
---
Outside diff comments:
In `@app/Models/OAuth2/Client.php`:
- Around line 690-706: The URI validation in Client::isUriAllowed and
Client::isPostLogoutUriAllowed must preserve exact, case-sensitive path and
query matching instead of relying solely on URLUtils::canonicalUrl(). Retain
each original requested URI before canonicalization, parse requested and
registered URIs, and require matching base canonical URLs plus identical path
and query components; update ClientMappingTest.php to reject dynamic
query-string variants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2dc1414d-e523-48f6-8e63-21338f08355a
📒 Files selected for processing (3)
app/Models/OAuth2/Client.phpdocs/adr/0001-native-client-custom-uri-schemes.mdtests/unit/ClientMappingTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/adr/0001-native-client-custom-uri-schemes.md
- tests/unit/ClientMappingTest.php
| $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; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Missing null checks after normalization can cause unintended matches.
If URLUtils::normalizeUrl() fails and returns null for a malformed requested origin, $originWithoutPort evaluates to null. If a registered origin also causes normalizeUrl() to fail, the strict equality check ($originWithoutPort === $canonical_allowed_origin) evaluates to null === null and incorrectly returns true.
Add empty() checks after normalization to ensure null values are safely rejected, consistent with the pattern used in isPostLogoutUriAllowed.
🛡️ Proposed fix
- $originWithoutPort = URLUtils::normalizeUrl($originWithoutPort);
+ $originWithoutPort = URLUtils::normalizeUrl($originWithoutPort);
+ if(empty($originWithoutPort)) return false;
$originWithPort = URLUtils::canonicalUrl($origin);
$originWithPort = empty($originWithPort) ? null : URLUtils::normalizeUrl($originWithPort);
// exact match against each registered value, through the same canonicalize+normalize pipeline on
// both sides (mirrors isUriAllowed()/isPostLogoutUriAllowed()) - a registered origin must no longer
// match merely because the requested origin is a string prefix of it (e.g. registered
// "https://my-app.example.com" incorrectly matching a requested "https://my-app.example.co" under
// the old str_contains($this->allowed_origins, $origin) check).
foreach(explode(',', $this->allowed_origins) as $allowed_origin){
$allowed_origin = trim($allowed_origin);
if(empty($allowed_origin)) continue;
$canonical_allowed_origin = URLUtils::canonicalUrl($allowed_origin);
if(empty($canonical_allowed_origin)) continue;
$canonical_allowed_origin = URLUtils::normalizeUrl($canonical_allowed_origin);
+ if(empty($canonical_allowed_origin)) continue;
if($originWithoutPort === $canonical_allowed_origin) return true;
if($originWithPort !== null && $originWithPort === $canonical_allowed_origin) return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $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; | |
| $originWithoutPort = URLUtils::normalizeUrl($originWithoutPort); | |
| if(empty($originWithoutPort)) return false; | |
| $originWithPort = URLUtils::canonicalUrl($origin); | |
| $originWithPort = empty($originWithPort) ? null : URLUtils::normalizeUrl($originWithPort); | |
| // exact match against each registered value, through the same canonicalize+normalize pipeline on | |
| // both sides (mirrors isUriAllowed()/isPostLogoutUriAllowed()) - a registered origin must no longer | |
| // match merely because the requested origin is a string prefix of it (e.g. registered | |
| // "https://my-app.example.com" incorrectly matching a requested "https://my-app.example.co" under | |
| // the old str_contains($this->allowed_origins, $origin) check). | |
| foreach(explode(',', $this->allowed_origins) as $allowed_origin){ | |
| $allowed_origin = trim($allowed_origin); | |
| if(empty($allowed_origin)) continue; | |
| $canonical_allowed_origin = URLUtils::canonicalUrl($allowed_origin); | |
| if(empty($canonical_allowed_origin)) continue; | |
| $canonical_allowed_origin = URLUtils::normalizeUrl($canonical_allowed_origin); | |
| if(empty($canonical_allowed_origin)) continue; | |
| if($originWithoutPort === $canonical_allowed_origin) return true; | |
| if($originWithPort !== null && $originWithPort === $canonical_allowed_origin) return true; | |
| } | |
| return false; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/Models/OAuth2/Client.php` around lines 858 - 880, Add empty-value guards
after each URLUtils::normalizeUrl() call in the origin-matching flow, including
the requested origin values and canonical_allowed_origin, before performing
strict equality comparisons. Ensure malformed origins with null normalization
results are skipped or rejected, matching the established behavior in
isPostLogoutUriAllowed.
There was a problem hiding this comment.
@smarcet Following up on both points raised in this review round for Client.php:
Missing null checks after normalizeUrl() (this thread, isOriginAllowed() line 880): Investigated by fuzzing URLUtils::normalizeUrl() against a wide range of malformed inputs (invalid UTF-8, malformed IPv6/ports, control characters, bracket/backslash paths, overlong percent-encoding, etc.). Every malformed case already fails earlier, at canonicalUrl()'s own filter_var(FILTER_VALIDATE_URL)/parse_url() guard, which is already checked. Could not find an input where canonicalUrl() succeeds but the subsequent normalizeUrl() call on that output returns null/empty. The gap is real on inspection (the underlying glenscott/url-normalizer library's mbParseUrl() can diverge from PHP's native parse_url() and reset to an empty state - see Normalizer::setUrl()), and adding the guard would match the pattern already used on the requested side in isPostLogoutUriAllowed(). Leaving open as a low-priority defensive-hardening item rather than a confirmed exploitable bug, since no reproduction was found despite the attempt.
Query string / path casing dropped by canonicalUrl() (outside-diff comment, lines 690-706): Not acting on this. canonicalUrl() never reads the query component and lowercases the path; both were true before this PR's changes and remain true after. The exact-match rewrite in this PR exists to close a prefix/substring bypass (a registered value matching as a substring of a different, longer URI) - that's the property RFC 6749 SS3.1.2.2 exact-matching is protecting here, and it's fully closed. Query-string tolerance is a pre-existing, deliberate choice already documented and tested for isPostLogoutUriAllowed() (testIsPostLogoutUriAllowedNativeClientAcceptsDynamicQueryString - dynamic state/session params appended by the client), and doesn't reopen that bypass: it doesn't change the destination authority or path, only the client's own trailing query params on their own already-validated endpoint. Path-case-insensitivity is similarly self-referential - it can only make a registered value match a differently-cased variant of itself, never a different client's URI or a different authority/path, so it doesn't reintroduce cross-client or cross-origin risk either. Both behaviors are now additionally covered by regression tests (testIsUriAllowedIgnoresPathCasingDifferences, testIsPostLogoutUriAllowedIgnoresPathCasingDifferences, testIsUriAllowedAcceptsAnyQueryStringNotJustDynamicOnes) so this stays a deliberate, pinned behavior rather than an unreviewed implicit one.
…nd a Redis lock Closes the TOCTOU race in IClientRepository::hasCustomSchemeRegisteredOnAnotherClientThan(): it's a count-then-write check with no DB-level uniqueness constraint or locking. Two concurrent create()/update() requests registering the same custom scheme (e.g. two different developers self-registering clients on this IDP) could both pass the "is this scheme already claimed" check before either transaction commits, letting both end up with the same scheme - defeating the OS-level interception-prevention guarantee the check exists for (flagged Major by CodeRabbit on PR #147, left unaddressed; raised again by /review-pr-deep as Finding 3). Fix: ClientService::create()/update() now acquire a single global lock (ILockManagerService, Redis-backed via LockManagerService - the same distributed-lock abstraction TokenService already uses) around the entire transaction for Native-client payloads, so the uniqueness check and the write it gates are no longer split across two unsynchronized transactions. Non-Native payloads are unaffected (no lock, no behavior change). SHORTCUT: one lock name serializes ALL Native client scheme create/update calls, not just ones that would actually collide on the same scheme. Acceptable given self-service client registration is low-throughput. Upgrade trigger: move to per-scheme locks (sorted acquisition order to avoid deadlock) if registration volume makes this a bottleneck. Residual gap (pre-existing, not introduced or worsened here): a row reaching storage via ClientFactory::build() directly (e.g. a seeder) still bypasses this lock, same as it already bypasses assertNativeCustomSchemesAllowed() - unchanged by this fix. Regression test added to ClientApiTest: pre-acquires the same lock name the service uses to simulate contention, asserts a clean 412 instead of a successful registration. True concurrent-transaction races aren't reproducible in this synchronous test harness, so this proves the locking mechanism is wired in and fails closed rather than proving the race itself is gone. Verified: ClientApiTest 17/17, full "Application Test Suite" 164/164 (0 failures/errors).
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-147/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/ClientApiTest.php (1)
432-440: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused
$responseassignment.The
$responsevariable is assigned but never used here, as$this->assertResponseStatus(412)checks the internal state from the request directly. Removing the assignment resolves static analysis warnings.Note: This same pattern appears in several other tests added in this file (e.g., lines 137, 157, 172, 202, 269, 307, 344, 379, 404). You can safely remove the
$response =assignment in those locations as well.♻️ Proposed fix
- $response = $this->action("PUT", "Api\\ClientApiController@update", + $this->action("PUT", "Api\\ClientApiController@update", array( 'id' => $client->id, 'application_type' => IClient::ApplicationType_Native, 'redirect_uris' => 'lockcontention://callback', ), [], [], []);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/ClientApiTest.php` around lines 432 - 440, Remove the unused $response assignment from this update test and the other matching test cases in the file, while preserving the existing $this->action(...) calls and subsequent assertions.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/ClientApiTest.php`:
- Around line 432-440: Remove the unused $response assignment from this update
test and the other matching test cases in the file, while preserving the
existing $this->action(...) calls and subsequent assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e032c63-c8bc-4a10-be7a-ed229252c08b
📒 Files selected for processing (2)
app/Services/OAuth2/ClientService.phptests/ClientApiTest.php
…review Finding 2) Adds coverage for a gap identified by /review-pr-deep and independently by CodeRabbit: URLUtils::canonicalUrl() drops the query string and lowercases the full path before comparison, so the "exact match" isUriAllowed()/ isPostLogoutUriAllowed() advertise is weaker than RFC 6749 SS3.1.2.2's byte-for-byte requirement on path/query. These tests pin today's actual (permissive) behavior - they pass now, not RED tests for a fix - so a future change to this area can't silently alter the behavior without a test noticing, and so the gap is visible in the suite rather than only in a review comment. testIsUriAllowedIgnoresPathCasingDifferences / testIsPostLogoutUriAllowedIgnoresPathCasingDifferences: registered path casing doesn't have to match the requested URI's casing. testIsUriAllowedAcceptsAnyQueryStringNotJustDynamicOnes: fills the same gap already covered for isPostLogoutUriAllowed by the pre-existing testIsPostLogoutUriAllowedNativeClientAcceptsDynamicQueryString (which documents a deliberate choice for end-session's session/state params) - isUriAllowed had no equivalent test, and unlike post-logout, redirect_uri at /oauth2/authorize isn't expected to carry a client-appended dynamic query string, so this coverage gap looks closer to unintended than deliberate. No test added for review Finding 1 (isOriginAllowed() missing an empty-check after normalizeUrl(), independently flagged by CodeRabbit): extensive fuzzing of URLUtils::normalizeUrl() (invalid UTF-8, malformed IPv6/ports, control characters, bracket/backslash paths, etc.) found no input where canonicalUrl() succeeds but the subsequent normalizeUrl() call returns null - every malformed case already fails at the canonicalUrl() stage, which is already guarded. Not writing a test for an unconfirmed reproduction.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-147/ This page is automatically updated on each push to this PR. |
…touch URI fields ClientService::create()/update() acquired the TOCTOU lock (NATIVE_CUSTOM_SCHEME_REGISTRATION_LOCK) whenever application_type was Native, regardless of whether the payload touched redirect_uris, allowed_origins, or post_logout_redirect_uris. A plain app_name rename on one Native client therefore contended with an unrelated tenant's scheme registration, producing spurious "please retry" 412s. Extract NATIVE_CUSTOM_SCHEME_URI_FIELDS as the single source of truth for which fields matter, shared by assertNativeCustomSchemesAllowed() and the new payloadTouchesNativeCustomSchemeFields() gate. The lock is now taken only when the payload actually registers a custom scheme - matching what the SHORTCUT comment on the lock constant already claimed. Found during /review-pr-deep review of PR #147.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-147/ This page is automatically updated on each push to this PR. |
…y Throwable LockManagerService::acquireLock() passed the absolute expiry timestamp (time()+lifetime+1) to Redis EXPIRE, which expects relative seconds: an unreleased lock (owner crashed or killed mid-callback) stayed held for ~55 years instead of auto-recovering after lifetime. For the NATIVE_CUSTOM_SCHEME_REGISTRATION_LOCK this meant every Native client create()/update() touching URI fields returned 412 "please retry" forever, until the Redis key was deleted by hand - contradicting the 30s recovery the lock's own doc comment promises. lock() also caught only Exception, so a Throwable that does not extend it (TypeError, Error) skipped both release paths and leaked the lock until that TTL expired. Release now happens in a finally block; acquisition stays outside the try so a failed acquireLock() still propagates without releasing a lock we don't own. Callers using acquireLock() as a single-use replay marker (nonce, auth code, private association) are unaffected: their artifacts' own lifetimes (360s/240s defaults) are shorter than the lock lifetimes, so the now-honored relative TTL opens no replay window. Found during /review-pr-deep review of PR #147.
… setUp The test swaps in a minimal Container as facade root but never cleared Facade::$resolvedInstance, so any previously-run BrowserKitTestCase (which boots the full Laravel app) left resolved facades that shadowed the mocks bound on the minimal container - Redirect::action() hit the real redirector and the once() expectations failed at Mockery::close(). And since Mockery::close() is tearDown's first line, the facade cleanup below it never ran, cascading the failure across all three tests. Only surfaced now because the new tests/unit/LockManagerServiceTest.php changed which test precedes it in the suite; running the unchanged ClientMappingTest before it reproduces the same 3 errors, proving the isolation gap predates this branch's changes.
…) throws tearDown() called Mockery::close() first with no try/finally, so an unmet mock expectation there threw before Facade::clearResolvedInstances()/ setFacadeApplication(null) could run - leaking this test's facade root (the minimal Container with mocked bindings) to whatever test runs next in the same PHP process. Same bug class the recent setUp() fix guards against, but in the opposite direction: leaking outward instead of receiving a leak from a prior test. Found during adversarial re-review of the setUp() fix.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-147/ This page is automatically updated on each push to this PR. |
Summary
ApplicationType_Native(mobile/desktop) OAuth2 clients can now register custom app URI schemes (myapp://callback) inallowed_originsandpost_logout_redirect_urisvia the admin API and React UI, matching the supportredirect_urisalready had. The OIDC end-session flow honors a registered custom-scheme post-logout URI at runtime.allowed_originsremains admin-API-only for Native clients (the UI input stays hidden) — no runtime path enforces it for Native today, so exposing a control for a value nothing currently checks wasn't worth the surface.See ADR:
docs/adr/0001-native-client-custom-uri-schemes.md· Plan:docs/plans/2026-07-14-native-clients-custom-schemes.mdSecurity hardening
Five rounds of adversarial code review surfaced that naively allowing "any scheme" was unsafe once these fields become live
302redirect targets — and that the gap actually predated this PR inredirect_uristoo (the field that carries the authorization code, more critical than the two new ones):javascript:,data:,intent:,itms-services:, etc.) and plainhttp, applied consistently to all three URI fields.http://127.0.0.1|::1|localhostremains allowed for Native clients (the standard native-app redirect pattern) — only non-loopbackhttpis blocked. Without this carve-out, the initial hardening pass would have broken every existing Native client using the standard pattern.redirect_uris-only before), preventing OS-level scheme interception between two clients registered with this IDP.Client::isUriAllowed/isPostLogoutUriAllowed) independently re-check the scheme deny-list rather than relying solely on write-time validation — a row reaching storage via a path other thanClientService(e.g. a seeder callingClientFactory::build()directly) still can't produce a live redirect to a dangerous scheme."host") inURLUtils::canonicalUrl/Client::isPostLogoutUriAllowedfor host-less custom-scheme URIs (mailto:,file:///x).roipappmatching insideandroipapp://...) — an unanchoredLIKE '%scheme://%', now anchored to real list-item boundaries.create()had zero request-level validation for the three URI fields, independent of the deny-list/uniqueness logic above:getCreatePayloadValidationRules()never declaredredirect_uris/allowed_origins/post_logout_redirect_uris, socreate()relied solely on the service-layerassertNativeCustomSchemesAllowed(). Two consequences: (1) a list submitted with a space after the comma (e.g."https://a.com, myapp://cb") reached storage verbatim —ClientFactory::populate()only trims the whole payload string, not each item — silently defeating the anchored cross-client uniqueness match above for that entry; (2) a non-string value (e.g. a JSON array) reachedexplode()insideassertNativeCustomSchemesAllowed()untouched, throwing an uncaughtTypeError(not caught by anyExceptionhandler inAPICRUDController::create()) instead of a clean412. Fixed by adding the samecustom_url_set:application_typerulesupdate()already used togetCreatePayloadValidationRules(), plus anis_string()guard inCustomValidator::validateCustomUrlSet()— declaring the rule alone doesn't stop theTypeError, since Laravel invokes the rule callback with the raw value regardless of rule order.testCreateNativeClientRejectsCustomSchemeWithLeadingSpaceInList,testCreateNativeClientRejectsArrayValueForRedirectUrisCleanly) before the fix, per TDD. One self-correction along the way: the space-after-comma bug was initially suspected reachable viaupdate()too, but that path was already protected byvalidateCustomUrl's pre-existing untrimmed per-item scheme check — the real gap wascreate()-only, sharing one root cause with theTypeErrorbug, so one fix closes both.Design: deny-list ownership
Per review, the deny-list (
DISALLOWED_NATIVE_URI_SCHEMES) moved fromUtils\Http\HttpUtils(a generic HTTP helper — wrong dependency direction for OAuth2-Native-client domain policy) toIClient(data, alongsideApplicationType_Nativeand friends) +Client::isDisallowedNativeUriScheme()(the predicate, since interfaces can't hold method bodies). The admin UI no longer hand-duplicates the list in JS —AdminControllernow injectswindow.DISALLOWED_NATIVE_URI_SCHEMES/window.NATIVE_LOOPBACK_HOSTSfrom theIClientconstants (same mechanism already used forwindow.APP_TYPES), so the backend is the single source of truth.Also fixes
An unrelated pre-existing bug in
UserLoginTurnstileTestwhere assigningnull(from an unset env var) to a typedstringproperty threw aTypeErrorbefore the intended skip-guard could run.Testing
Application Test Suitere-verified afterward at 158 tests / 527 assertions, 0 failures.302redirect at/oauth2/end-session; unregistered/dangerous scheme → clean400; loopbackhttp://127.0.0.1allowed, non-loopbackhttp://rejected; browser-driven admin UI save/reload round-trip for a Native client's Post Logout Uris.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation