Skip to content

Commit fc403be

Browse files
committed
feat(oauth2): support RFC 8252 §7.1 authority-less custom-scheme URIs end-to-end
com.example.app:/oauth2redirect (the RFC's own example form, and the default shape AppAuth-based apps register) passed every write-time validator but was silently rejected by every runtime gate: URLUtils::canonicalUrl() opens with FILTER_VALIDATE_URL, which rejects the authority-less shape, so isUriAllowed()/isPostLogoutUriAllowed() could never match it - a registration that validates cleanly and never authenticates. - URLUtils::canonicalUrl(): canonicalize authority-less URIs as scheme + rooted lowercased path (query/fragment dropped, same as the authority form); opaque URIs (mailto:foo@bar - no authority AND no rooted path) keep returning null. - Client::isPostLogoutUriAllowed(): drop its own FILTER_VALIDATE_URL and isset(host) guards - validity is canonicalUrl()'s job now, and the host-less crash those guards prevented cannot recur. - DoctrineOAuth2ClientRepository: anchor the cross-client scheme-uniqueness LIKE on ':/' instead of '://' so a scheme claimed via either URI form collides with the other - the OS-level interception risk is about the scheme, not the shape it was registered in. - IndirectResponseQueryStringStrategy/IndirectResponseUrlFragmentStrategy: Laravel's Redirect::to() relies on the same FILTER_VALIDATE_URL check (UrlGenerator::isValidUrl) and rewrote the already-validated redirect target as a RELATIVE path (Location: https://<idp>/com.example.app:/logout) - caught by live verification only, invisible to the unit layer. Absolute URIs Laravel does not recognize are now emitted as a verbatim Location header; Symfony still rejects CR/LF in header values. All four regression tests written and confirmed failing first (TDD). The new end-session feature test lives in tests/OAuth2EndSessionTest.php because tests/OAuth2ProtocolTestCase.php's *TestCase.php suffix keeps that whole file out of PHPUnit's *Test.php discovery - the Application suite never runs it. Full Application suite: 173 tests / 562 assertions, 0 failures. Live-verified against the local docker IDP: registered com.example.app:/logout -> 302 Location: com.example.app:/logout?state=xyz (verbatim); unregistered path -> 400.
1 parent 570ff45 commit fc403be

9 files changed

Lines changed: 169 additions & 21 deletions

app/Models/OAuth2/Client.php

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1170,13 +1170,13 @@ public function isPostLogoutUriAllowed($post_logout_uri)
11701170
if(empty($this->post_logout_redirect_uris)) return false;
11711171
if(empty($post_logout_uri)) return false;
11721172

1173-
if(!filter_var($post_logout_uri, FILTER_VALIDATE_URL)) return false;
1174-
if(is_null($this->post_logout_redirect_uris)) return false;
1175-
if(empty($this->post_logout_redirect_uris)) return false;
1176-
1173+
// no FILTER_VALIDATE_URL gate here: it rejects the RFC 8252 SS7.1 authority-less form
1174+
// (com.example.app:/logout) that native clients may register. Validity is enforced by the
1175+
// scheme checks below plus canonicalUrl() (which still applies FILTER_VALIDATE_URL to
1176+
// authority-bearing URIs and requires a rooted path for authority-less ones).
11771177
$parts = @parse_url($post_logout_uri);
11781178

1179-
if ($parts == false) {
1179+
if ($parts == false || !isset($parts['scheme'])) {
11801180
return false;
11811181
}
11821182
// native clients may register custom schemes (myapp://...); every other app type requires https
@@ -1190,10 +1190,9 @@ public function isPostLogoutUriAllowed($post_logout_uri)
11901190
if($this->isNativeDangerousScheme($parts['scheme'], $parts['host'] ?? null))
11911191
return false;
11921192

1193-
// host-less URIs (e.g. mailto:, file:///x, myapp:///cb) pass FILTER_VALIDATE_URL but have no
1194-
// authority to match against; without this guard the concatenation below raises an
1195-
// "Undefined array key host" warning (converted to ErrorException) on the public end-session endpoint.
1196-
if(!isset($parts['host'])) return false;
1193+
// NOTE: no isset($parts['host']) guard here - authority-less URIs go through canonicalUrl(),
1194+
// which either canonicalizes them (RFC 8252 SS7.1 rooted-path form) or returns null (opaque
1195+
// forms like mailto:foo@bar), so the host-less crash this gate used to have cannot recur.
11971196

11981197
// exact match against each registered value, through the same canonicalize+normalize pipeline on
11991198
// both sides (mirrors isUriAllowed()): a registered value's scheme+host[:port] must no longer match

app/Repositories/DoctrineOAuth2ClientRepository.php

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -179,13 +179,16 @@ public function hasCustomSchemeRegisteredOnAnotherClientThan(int $id, string $cu
179179
// fields are comma-separated URI lists; a plain '%scheme://%' substring match false-positives on any
180180
// longer scheme ending in this one (e.g. 'roipapp' matching inside 'androipapp://...'). Anchor the
181181
// match to a real list-item boundary: the scheme starts the field, or immediately follows a comma.
182-
$starts_with = $scheme . '://%';
183-
$after_comma = '%,' . $scheme . '://%';
182+
// The boundary is ':/' rather than '://' so BOTH registered forms are seen - the authority form
183+
// (scheme://host/...) and the RFC 8252 SS7.1 authority-less form (scheme:/path): the OS-level
184+
// interception risk is about the scheme, regardless of which URI form either client registered.
185+
$starts_with = $scheme . ':/%';
186+
$after_comma = '%,' . $scheme . ':/%';
184187
// legacy rows: before the create()-validation hardening, POST create persisted lists verbatim,
185188
// so an item can still sit after ", " (comma + single space - the JSON/forms list artifact).
186189
// ClientFactory::populate now trims per item, so no NEW rows take this shape; N-space/other
187190
// whitespace leftovers are for the pre-deploy audit (... LIKE '%, %'), not this query.
188-
$after_comma_space = '%, ' . $scheme . '://%';
191+
$after_comma_space = '%, ' . $scheme . ':/%';
189192

190193
$qb = $this->getEntityManager()->createQueryBuilder();
191194
$matches_field = function (string $field) use ($qb) {

app/Strategies/IndirectResponseQueryStringStrategy.php

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,10 @@
1212
* limitations under the License.
1313
**/
1414
use Utils\IHttpResponseStrategy;
15+
use Illuminate\Http\RedirectResponse;
1516
use Illuminate\Support\Facades\Redirect;
1617
use Illuminate\Support\Facades\Response;
18+
use Illuminate\Support\Facades\URL;
1719
/**
1820
* Class IndirectResponseQueryStringStrategy
1921
* Redirect and http response using a 302 adding params on query string
@@ -36,7 +38,16 @@ public function handle($response)
3638
}
3739
$return_to = (strpos($return_to, "?") == false) ? $return_to . "?" . $query_string : $return_to . "&" . $query_string;
3840

39-
return Redirect::to($return_to)
41+
// RFC 8252 SS7.1 authority-less URIs (com.example.app:/cb?code=...) fail Laravel's
42+
// UrlGenerator::isValidUrl(), so Redirect::to() would treat the already-validated redirect
43+
// target as a RELATIVE path and prefix the site URL, corrupting the redirect. For an absolute
44+
// URI (leading scheme) Laravel does not recognize, emit the Location verbatim - Symfony still
45+
// rejects CR/LF in the header value, so no header-injection surface is opened.
46+
$redirect = (!URL::isValidUrl($return_to) && preg_match('~^[A-Za-z][A-Za-z0-9+.\-]*:~', $return_to) === 1)
47+
? new RedirectResponse($return_to)
48+
: Redirect::to($return_to);
49+
50+
return $redirect
4051
->header('Cache-Control', 'no-cache, no-store, max-age=0, must-revalidate')
4152
->header('Pragma','no-cache');
4253
}

app/Strategies/IndirectResponseUrlFragmentStrategy.php

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,10 @@
1212
* limitations under the License.
1313
**/
1414
use Utils\IHttpResponseStrategy;
15+
use Illuminate\Http\RedirectResponse;
1516
use Illuminate\Support\Facades\Redirect;
1617
use Illuminate\Support\Facades\Response;
18+
use Illuminate\Support\Facades\URL;
1719
/**
1820
* Class IndirectResponseUrlFragmentStrategy
1921
* Redirect and http response using a 302 adding params on url fragment
@@ -37,7 +39,14 @@ public function handle($response)
3739

3840
$return_to = (strpos($return_to, "#") == false) ? $return_to . "#" . $fragment : $return_to . "&" . $fragment;
3941

40-
return Redirect::to($return_to)
42+
// same RFC 8252 SS7.1 authority-less guard as IndirectResponseQueryStringStrategy: an absolute
43+
// URI Laravel's UrlGenerator does not recognize must be emitted verbatim, or Redirect::to()
44+
// prefixes the site URL and corrupts the already-validated redirect target.
45+
$redirect = (!URL::isValidUrl($return_to) && preg_match('~^[A-Za-z][A-Za-z0-9+.\-]*:~', $return_to) === 1)
46+
? new RedirectResponse($return_to)
47+
: Redirect::to($return_to);
48+
49+
return $redirect
4150
->header('Cache-Control', 'no-cache, no-store, max-age=0, must-revalidate')
4251
->header('Pragma','no-cache');
4352
}

app/libs/Utils/URLUtils.php

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,17 +34,24 @@ public static function normalizeUrl(string $url):?string{
3434
* @return string|null
3535
*/
3636
public static function canonicalUrl(string $url, bool $usePort = true):?string{
37-
if(!filter_var($url, FILTER_VALIDATE_URL)) return null;
3837
$parts = @parse_url($url);
39-
if ($parts == false)
38+
if ($parts == false || !isset($parts['scheme']))
4039
{
4140
return null;
4241
}
43-
// host-less URIs (e.g. mailto:, file:///x) pass FILTER_VALIDATE_URL but have no authority to
44-
// canonicalize; without this guard the concatenation below raises an "Undefined array key host" warning.
4542
if (!isset($parts['host'])) {
46-
return null;
43+
// RFC 8252 SS7.1: private-use scheme redirect URIs may omit the authority entirely -
44+
// "com.example.app:/oauth2redirect/example-provider" is the RFC's own example form.
45+
// FILTER_VALIDATE_URL rejects that shape, so it is canonicalized here from parse_url parts:
46+
// scheme + rooted path (query/fragment dropped, path lowercased, same as the authority form).
47+
// Opaque URIs (mailto:foo@bar - no authority AND no rooted path) keep returning null: there
48+
// is no location to match a redirect against.
49+
if (!isset($parts['path']) || !str_starts_with($parts['path'], '/') || isset($parts['user']) || isset($parts['port'])) {
50+
return null;
51+
}
52+
return rtrim($parts['scheme'].':'.strtolower($parts['path']), '/');
4753
}
54+
if(!filter_var($url, FILTER_VALIDATE_URL)) return null;
4855
$canonical_url = $parts['scheme'].'://'.strtolower($parts['host']);
4956
if(isset($parts['port']) && $usePort) {
5057
$canonical_url .= ':'.strtolower($parts['port']);

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,10 @@ Four consecutive adversarial code-review passes (xhigh-effort, multi-agent) surf
3030

3131
## Decision
3232

33-
1. **Allow custom app URI schemes in all three URI-bearing Native-client fields** (`redirect_uris`, `allowed_origins`, `post_logout_redirect_uris`), gated by a **deny-list**, not an allow-list — any scheme is treated as a legitimate custom app scheme unless it appears on `IClient::DISALLOWED_NATIVE_URI_SCHEMES`.
33+
1. **Allow custom app URI schemes in all three URI-bearing Native-client fields** (`redirect_uris`, `allowed_origins`, `post_logout_redirect_uris`), gated by a **deny-list**, not an allow-list — any scheme is treated as a legitimate custom app scheme unless it appears on `IClient::DISALLOWED_NATIVE_URI_SCHEMES`. Both custom-scheme URI shapes are supported end-to-end: the authority form (`myapp://callback`) and the **RFC 8252 §7.1 authority-less form** (`com.example.app:/oauth2redirect` — the RFC's own example, and the default shape AppAuth-based apps register). `URLUtils::canonicalUrl()` canonicalizes the authority-less form as scheme + rooted, lowercased path (query/fragment dropped, same as the authority form); the two forms are distinct URIs and never cross-match. Opaque URIs (`mailto:foo@bar` — no authority *and* no rooted path) remain rejected: there is no location to match a redirect against. The redirect *emitters* (`IndirectResponseQueryStringStrategy`/`IndirectResponseUrlFragmentStrategy`) also special-case this form: Laravel's `Redirect::to()` relies on the same `FILTER_VALIDATE_URL` check internally (`UrlGenerator::isValidUrl()`) and would otherwise treat the already-validated target as a relative path, prefixing the site URL — an absolute URI Laravel does not recognize is emitted as a verbatim `Location` header instead (Symfony still rejects CR/LF in header values, so no header-injection surface opens).
3434
2. **Single source of truth for the deny-list policy, owned by the OAuth2 domain layer, not a generic HTTP helper.** The deny-list and loopback-host list are `const` arrays on `IClient` (domain policy for Native OAuth2 clients — the same interface already holding `ApplicationType_Native`, `ClientType_Confidential`, etc.). Since PHP interfaces can't hold method bodies, the predicate that interprets them (`isDisallowedNativeUriScheme(string $scheme, ?string $host = null): bool`) is a `public static` method on `Client`, the concrete entity. Both the write-time validator (`ClientService::assertNativeCustomSchemesAllowed()`, and the `redirect_uris` validation branch in `ClientService::update()`) and the runtime allow-gates (`Client::isUriAllowed()`, `Client::isPostLogoutUriAllowed()`, via a shared `Client::isNativeDangerousScheme()` helper) call this one method. The admin UI reads the same two lists at runtime instead of hand-duplicating them in JavaScript: `AdminController` passes `IClient::DISALLOWED_NATIVE_URI_SCHEMES`/`IClient::NATIVE_LOOPBACK_HOSTS` to the edit-client view, which injects them as `window.DISALLOWED_NATIVE_URI_SCHEMES`/`window.NATIVE_LOOPBACK_HOSTS` (the same mechanism already used for `window.APP_TYPES`); `logout_options.js`'s inline validator reads from `window.*` rather than maintaining its own copy. *(This constant/method placement was revised once, after initial review placed the deny-list on the generic `Utils\Http\HttpUtils` class — see Consequences.)*
3535
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`). At match time (`Client::isUriAllowed()`), a Native client's http-loopback request is additionally compared **port-agnostically**: RFC 8252 §7.3 requires the AS to allow any port specified at request time, because native apps bind an ephemeral loopback port per run. Only the port is ignored — scheme, host, and path still require an exact match, and the loopback hosts are not cross-matched against each other (registering `127.0.0.1` does not allow `localhost`).
36-
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`, plus a comma-space variant tolerating the legacy `", scheme://"` single-space artifact that the pre-hardening `create()` could persist; `ClientFactory::populate()` now trims each list item before normalizing, so new rows are always canonical regardless of write path.
36+
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`, plus a comma-space variant tolerating the legacy `", scheme://"` single-space artifact that the pre-hardening `create()` could persist; `ClientFactory::populate()` now trims each list item before normalizing, so new rows are always canonical regardless of write path. The item boundary is `scheme:/` rather than `scheme://`, so a scheme claimed via the authority-less RFC 8252 §7.1 form (`scheme:/path`) collides with one claimed via the authority form (`scheme://host`) and vice versa — the OS-level interception risk is about the scheme, not the URI shape it was registered in.
3737
5. **Defense-in-depth**: the runtime allow-gates independently re-check the scheme deny-list; write-time validation is not the sole enforcement point.
3838
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. This closed gap is itself a deliberate API contract change beyond Native clients: `getCreatePayloadValidationRules()` previously declared none of the three URI fields, so `create()` accepted *any* value for them for every application type; the new `custom_url_set` rule enforces per-item https for non-Native types at create time. Automation that registered Web_App/JS clients with `http://` URIs (e.g. localhost dev tooling) or malformed lists now receives `412` where it previously got `201` — matching what `update()` always enforced for those types.
3939
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.

tests/ClientApiTest.php

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -503,4 +503,38 @@ public function testUpdateNativeClientNotTouchingUriFieldsIgnoresLockContention(
503503
}
504504
}
505505

506+
public function testUpdateNativeClientRejectsSchemeAlreadyRegisteredInAuthorityLessForm(){
507+
508+
// RFC 8252 SS7.1 authority-less registrations (com.example.app:/oauth2redirect) store the scheme
509+
// followed by ":/" instead of "://". The cross-client scheme-uniqueness LIKE must see that shape
510+
// too: the OS-level interception risk is about the SCHEME, regardless of which URI form either
511+
// client registered it in - so claiming "authlessscheme" via the authority-less form must block
512+
// another client from claiming it via the authority form (and vice versa).
513+
$client1 = EntityManager::getRepository(Client::class)->findOneBy(['app_name' => 'oauth2_native_app']);
514+
515+
$response = $this->action("PUT", "Api\\ClientApiController@update",
516+
array(
517+
'id' => $client1->id,
518+
'application_type' => IClient::ApplicationType_Native,
519+
'redirect_uris' => 'authlessscheme:/callback',
520+
),
521+
[],
522+
[],
523+
[]);
524+
$this->assertResponseStatus(201);
525+
526+
$client2 = EntityManager::getRepository(Client::class)->findOneBy(['app_name' => 'oauth2_native_app2']);
527+
528+
$response = $this->action("PUT", "Api\\ClientApiController@update",
529+
array(
530+
'id' => $client2->id,
531+
'application_type' => IClient::ApplicationType_Native,
532+
'redirect_uris' => 'authlessscheme://other',
533+
),
534+
[],
535+
[],
536+
[]);
537+
$this->assertResponseStatus(412);
538+
}
539+
506540
}

tests/OAuth2EndSessionTest.php

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
<?php namespace Tests;
2+
/**
3+
* Copyright 2026 OpenStack Foundation
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
* http://www.apache.org/licenses/LICENSE-2.0
8+
* Unless required by applicable law or agreed to in writing, software
9+
* distributed under the License is distributed on an "AS IS" BASIS,
10+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11+
* See the License for the specific language governing permissions and
12+
* limitations under the License.
13+
**/
14+
15+
use LaravelDoctrine\ORM\Facades\EntityManager;
16+
use Models\OAuth2\Client;
17+
18+
/**
19+
* Class OAuth2EndSessionTest
20+
* NOTE: deliberately NOT placed in OAuth2ProtocolTestCase.php - that file's *TestCase.php suffix
21+
* keeps it OUT of the Application Test Suite (PHPUnit only auto-discovers *Test.php), so a test
22+
* added there would never run in CI.
23+
* @package Tests
24+
*/
25+
final class OAuth2EndSessionTest extends OpenStackIDBaseTestCase
26+
{
27+
public function testEndSessionRedirectsVerbatimToAuthorityLessPostLogoutUri()
28+
{
29+
// RFC 8252 SS7.1 authority-less URIs (com.example.app:/logout) fail Laravel's
30+
// UrlGenerator::isValidUrl() (FILTER_VALIDATE_URL-based), so Redirect::to() inside
31+
// IndirectResponseQueryStringStrategy used to treat the approved post-logout target as a
32+
// RELATIVE path and prefix the site URL (Location: http://<idp>/com.example.app:/logout) -
33+
// corrupting the redirect at the emitter even once the runtime allow-gates accept the
34+
// authority-less form. The Location header must carry the registered URI verbatim, with the
35+
// state round-tripped on the query string.
36+
$client = EntityManager::getRepository(Client::class)->findOneBy(['app_name' => 'oauth2_native_app']);
37+
$client->setPostLogoutRedirectUris('com.example.app:/logout');
38+
EntityManager::persist($client);
39+
EntityManager::flush();
40+
41+
$this->call('GET', '/oauth2/end-session', [
42+
'client_id' => $client->getClientId(),
43+
'post_logout_redirect_uri' => 'com.example.app:/logout',
44+
'state' => 'xyz',
45+
]);
46+
47+
$this->assertResponseStatus(302);
48+
$this->assertEquals('com.example.app:/logout?state=xyz', $this->response->headers->get('Location'));
49+
}
50+
}

0 commit comments

Comments
 (0)