#2101 Wildcard in redirection URI support#2154
Open
anushasunkada wants to merge 4 commits intoasgardeo:mainfrom
Open
#2101 Wildcard in redirection URI support#2154anushasunkada wants to merge 4 commits intoasgardeo:mainfrom
anushasunkada wants to merge 4 commits intoasgardeo:mainfrom
Conversation
Signed-off-by: anushasunkada <anushasunkada@gmail.com>
Signed-off-by: anushasunkada <anushasunkada@gmail.com>
📝 WalkthroughWalkthroughThis PR implements wildcard redirect URI support for OAuth2 authorization flows. It adds pattern matching logic to validate redirect URIs against registered patterns using Changes
Sequence Diagram(s)sequenceDiagram
participant Client as OAuth Client
participant AuthServer as Authorization Server
participant AppRegistry as App Registry/Database
participant PatternMatcher as Pattern Matcher Utility
rect rgba(220, 100, 100, 0.5)
Note over Client,AppRegistry: OAuth App Registration Flow
Client->>AuthServer: POST /applications (with redirectUris: ["https://app.com/callback/*"])
AuthServer->>AuthServer: validateRedirectURIs()
AuthServer->>AuthServer: Check: wildcard not in scheme/host/query
AuthServer->>AuthServer: Check: only * and ** in path (no regex chars)
AuthServer->>AppRegistry: Store app with registered redirectUris
AppRegistry-->>AuthServer: App created
AuthServer-->>Client: 201 Created
end
rect rgba(100, 220, 100, 0.5)
Note over Client,AppRegistry: OAuth Authorization Request Flow
Client->>AuthServer: GET /oauth2/authorize?redirect_uri=https://app.com/callback/success
AuthServer->>AppRegistry: Fetch app's registered redirectUris
AppRegistry-->>AuthServer: Returns: ["https://app.com/callback/*"]
AuthServer->>PatternMatcher: MatchRedirectURIPattern(pattern, incoming)
PatternMatcher->>PatternMatcher: Parse URIs (scheme, host, path, query)
PatternMatcher->>PatternMatcher: Compare scheme/host exactly
PatternMatcher->>PatternMatcher: matchPathPattern("callback/*", "callback/success")
PatternMatcher-->>AuthServer: true (match)
AuthServer->>AuthServer: validateInitialAuthorizationRequest() passes
AuthServer-->>Client: Redirect to login/consent
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/internal/application/service.go (2)
829-875:⚠️ Potential issue | 🟠 MajorAdd end-user docs under
docs/content/guides/for wildcard redirect URIs.This changes the OAuth client-registration and authorization-flow contract, but the PR only adds an internal design note under
feature-docs/. Please update the OAuth/OIDC application guide indocs/content/guides/—or add a dedicated wildcard redirect URI guide there—to cover allowed wildcard syntax, deep-link limitations, and when callers must send a concreteredirect_uri.
As per coding guidelines, "This PR introduces a user-facing change (e.g., new/modified API endpoint, configuration option, authentication flow, or SDK behavior) but does not include corresponding documentation updates underdocs/."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/application/service.go` around lines 829 - 875, Add a user-facing guide under docs/content/guides that documents the new wildcard redirect URI behavior introduced around redirectURI validation (the code paths referencing sysutils.ParseURL, ErrorInvalidRedirectURI, containsRegexMetacharacter and the checks for scheme/host/path/query/fragment). The guide should state the exact allowed wildcard syntax in the path (* and **), explain that wildcards are disallowed in scheme/host/query/fragment and that fragments are forbidden, describe deep-linking limitations and examples of when callers must supply a concrete redirect_uri during authorization/registration, and include migration/compatibility notes and examples for client implementers.
869-892:⚠️ Potential issue | 🟠 MajorAvoid blanket metacharacter rejection on redirect paths.
containsRegexMetacharacter(parsedURI.Path)will reject legitimate redirect URIs like/callback.html, because.is a normal literal path character. That turns valid exact redirect URIs into registration failures. If the goal is to constrain wildcard usage, validate*/**placement separately from URI path validity instead of rejecting every regex-like character up front. Based on learnings, ensure all identity-related code aligns with relevant RFC specifications.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/application/service.go` around lines 869 - 892, The current containsRegexMetacharacter(parsedURI.Path) used in the redirect URI validation (see containsRegexMetacharacter and its call in the redirect URI loop) incorrectly rejects valid path characters like '.'; change the validation so it no longer treats normal literal path characters as invalid: remove '.' from the banned metacharacter set and/or eliminate this blanket rejection, and instead implement a focused check that only enforces allowed wildcard usage (validate '*' and '**' placement and that no other regex-exclusive tokens like [](){}+?^$|\ are present) while leaving ordinary path punctuation (e.g., '.') permitted; update containsRegexMetacharacter (or replace it with a more specific function) and adjust the redirect URI validation logic to call the new check so exact URIs pass but improper regex tokens or misplaced wildcards are still rejected.backend/internal/application/model/oauth_app.go (1)
216-274:⚠️ Potential issue | 🟠 MajorAdd published docs for wildcard
redirectUrisbefore merge.This changes the registration contract for
redirectUrisand the authorization-timeredirect_urivalidation rules, but the PR only references internal feature docs. Please add customer-facing docs underdocs/—at minimum an API note indocs/content/apis.mdxand a guide/update underdocs/content/guides/covering*/**path semantics, invalid placements in scheme/host/query, and the requirement to send an explicitredirect_uriwhen a wildcard URI is registered.As per coding guidelines, "This PR introduces a user-facing change (e.g., new/modified API endpoint, configuration option, authentication flow, or SDK behavior) but does not include corresponding documentation updates under
docs/."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/application/model/oauth_app.go` around lines 216 - 274, The PR adds wildcard support for redirectUris (see matchAnyRedirectURIPattern and utils.MatchRedirectURIPattern) and changes validation rules but lacks user-facing docs; add an API note in docs/content/apis.mdx describing the new redirectUris registration contract and authorization-time behavior (explicit redirect_uri required when a wildcard is registered), and add a guide under docs/content/guides/ explaining the wildcard path semantics for `*` vs `**`, where wildcards are invalid (scheme, host, query, fragment), examples of valid/invalid registrations and authorization requests, and a note about fragment prohibition and error messages returned by validation functions (e.g., the errors returned by matchAnyRedirectURIPattern and the redirect URI parsing/validation paths).
🧹 Nitpick comments (3)
backend/internal/system/utils/http_util_test.go (1)
891-1055: Add one mixed-case scheme/host case to lock the matcher contract.The table says a lot about path semantics, but every URI here uses lowercase scheme and host. One case like
HTTPS://Example.com/callbackvshttps://example.com/callbackwould prevent a regression to raw string comparison. Based on learnings, ensure all identity-related code aligns with relevant RFC specifications.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/system/utils/http_util_test.go` around lines 891 - 1055, Add a test case in the TestMatchRedirectURIPattern table to verify scheme/host comparison is case-insensitive (per RFC): call out the MatchRedirectURIPattern function and add an entry where the pattern is "https://example.com/callback" and the incoming URI uses mixed-case scheme/host like "HTTPS://Example.com/callback" (wantMatch: true), so the matcher is exercised for case-normalization rather than raw string equality; place this new case alongside the other positive matches in the tests slice.backend/internal/application/model/oauth_app.go (1)
230-235: Make the matcher error contract explicit.
matchAnyRedirectURIPattern()always swallowsutils.MatchRedirectURIPattern()errors withcontinue, so the error branch at Lines 232-235 is currently unreachable. Either propagate matcher errors or collapse the helper to a plainboolreturn so malformed-pattern handling is explicit.Also applies to: 257-273
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/application/model/oauth_app.go` around lines 230 - 235, The matcher helper matchAnyRedirectURIPattern currently swallows errors from utils.MatchRedirectURIPattern (making the caller error branch unreachable); update the contract so malformed pattern errors are explicit: either change matchAnyRedirectURIPattern to return (bool, error) and propagate any utils.MatchRedirectURIPattern errors up to callers (and update both call sites used around the other block at the second usage) or collapse it into a plain bool and validate/parse patterns separately before matching so callers never expect an error; adjust the callers (the two places invoking matchAnyRedirectURIPattern) to handle the new return type accordingly.tests/e2e/tests/oauth-app/wildcard-redirect-uri.spec.ts (1)
228-386: Use unique app names in the registration suite too.This block still uses fixed names like
e2e-wc-ac04, while the authz suite already adds aDate.now()suffix. If a previous run leaks an app or the suite runs against a reused tenant, these cases can fail for reasons unrelated to wildcard validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/tests/oauth-app/wildcard-redirect-uri.spec.ts` around lines 228 - 386, The tests use fixed application names (e.g., the "name" field values like "e2e-wc-ac01", "e2e-wc-ac04", "e2e-wc-ac13", etc.) which can collide across runs; update each POST payload in these tests to generate a unique name (append a timestamp or random suffix such as Date.now() or a UUID) when setting the name before calling the API (keep createdAppIds.push(body.id) unchanged). Ensure all tests in this file that set name values (AC-01 through AC-05, AC-13, AC-14) use the same uniqueness pattern so runs against reused tenants won’t conflict.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/internal/system/utils/http_util.go`:
- Around line 103-159: The recursive matcher currently uses a global depth cap
which rejects valid deep paths; replace this with memoization of (i,j) to bound
work. In matchPathPattern, create a memo map (e.g., map[[2]int]bool or
map[string]bool) and pass it into matchSegs; in matchSegs (and remove the depth
parameter), check the memo for the current (i,j) and return false if already
visited, otherwise mark (i,j) visited before exploring recursion; keep all
existing segment logic (*, **) but eliminate the depth > 32 checks so
deep-but-valid patterns like /** succeed. Ensure you update signatures:
matchPathPattern -> matchSegs to accept the memo and remove depth usage.
- Around line 70-159: The code introduces user-visible wildcard redirect_uri
semantics via MatchRedirectURIPattern and helpers matchPathPattern/matchSegs;
add public documentation before merging by updating the OAuth/application API
reference and guides to describe the new '*' (single segment) and '**'
(zero-or-more segments) wildcards, the restriction that wildcards are only
allowed in the path, and the exact matching rules for scheme, host
(case-insensitive per RFC 3986), and query; include examples of valid/invalid
pattern+incoming URIs, note the fragment is disallowed, and link to the internal
feature-docs for implementation details so users can understand behavior and
migrate safely.
- Around line 97-109: The matcher is using percent-decoded URL.Path which allows
encoded slashes to be treated as real separators; update the call that invokes
matchPathPattern to pass the escaped paths instead (use EscapedPath() for both
pattern and incoming URLs) so matchPathPattern and its matchSegs logic operate
on percent-encoded segments and `%2F` cannot be interpreted as a path separator.
In `@feature-docs/wildcard-redirect-uri.md`:
- Line 16: The current bullet "At least one redirection uri MUST be registrated
per client." is too broad; change it to require redirect URIs only for
authorization-based clients by rewording it to something like "Clients that use
the authorization_code grant MUST register at least one redirection URI." Ensure
the phrasing references the authorization_code grant (or "authorization-based
clients") so the documented requirement matches the validator logic.
In `@tests/e2e/tests/oauth-app/wildcard-redirect-uri.spec.ts`:
- Around line 406-430: Initialize the created app variables (singleStarApp,
doubleStarApp, exactApp, queryApp, multiApp) to null before test.beforeAll and
make test.afterAll resilient: check each variable for truthiness before calling
deleteApp (e.g. if (singleStarApp) await deleteApp(ctx, adminToken,
singleStarApp.appId)), wrap each deleteApp call in its own try/catch so one
failing delete doesn't stop the rest, and ensure ctx.dispose() is called in a
finally-like manner (guarding if ctx exists) so resources are always released
even if beforeAll partially fails; update references to createApp/deleteApp
accordingly.
---
Outside diff comments:
In `@backend/internal/application/model/oauth_app.go`:
- Around line 216-274: The PR adds wildcard support for redirectUris (see
matchAnyRedirectURIPattern and utils.MatchRedirectURIPattern) and changes
validation rules but lacks user-facing docs; add an API note in
docs/content/apis.mdx describing the new redirectUris registration contract and
authorization-time behavior (explicit redirect_uri required when a wildcard is
registered), and add a guide under docs/content/guides/ explaining the wildcard
path semantics for `*` vs `**`, where wildcards are invalid (scheme, host,
query, fragment), examples of valid/invalid registrations and authorization
requests, and a note about fragment prohibition and error messages returned by
validation functions (e.g., the errors returned by matchAnyRedirectURIPattern
and the redirect URI parsing/validation paths).
In `@backend/internal/application/service.go`:
- Around line 829-875: Add a user-facing guide under docs/content/guides that
documents the new wildcard redirect URI behavior introduced around redirectURI
validation (the code paths referencing sysutils.ParseURL,
ErrorInvalidRedirectURI, containsRegexMetacharacter and the checks for
scheme/host/path/query/fragment). The guide should state the exact allowed
wildcard syntax in the path (* and **), explain that wildcards are disallowed in
scheme/host/query/fragment and that fragments are forbidden, describe
deep-linking limitations and examples of when callers must supply a concrete
redirect_uri during authorization/registration, and include
migration/compatibility notes and examples for client implementers.
- Around line 869-892: The current containsRegexMetacharacter(parsedURI.Path)
used in the redirect URI validation (see containsRegexMetacharacter and its call
in the redirect URI loop) incorrectly rejects valid path characters like '.';
change the validation so it no longer treats normal literal path characters as
invalid: remove '.' from the banned metacharacter set and/or eliminate this
blanket rejection, and instead implement a focused check that only enforces
allowed wildcard usage (validate '*' and '**' placement and that no other
regex-exclusive tokens like [](){}+?^$|\ are present) while leaving ordinary
path punctuation (e.g., '.') permitted; update containsRegexMetacharacter (or
replace it with a more specific function) and adjust the redirect URI validation
logic to call the new check so exact URIs pass but improper regex tokens or
misplaced wildcards are still rejected.
---
Nitpick comments:
In `@backend/internal/application/model/oauth_app.go`:
- Around line 230-235: The matcher helper matchAnyRedirectURIPattern currently
swallows errors from utils.MatchRedirectURIPattern (making the caller error
branch unreachable); update the contract so malformed pattern errors are
explicit: either change matchAnyRedirectURIPattern to return (bool, error) and
propagate any utils.MatchRedirectURIPattern errors up to callers (and update
both call sites used around the other block at the second usage) or collapse it
into a plain bool and validate/parse patterns separately before matching so
callers never expect an error; adjust the callers (the two places invoking
matchAnyRedirectURIPattern) to handle the new return type accordingly.
In `@backend/internal/system/utils/http_util_test.go`:
- Around line 891-1055: Add a test case in the TestMatchRedirectURIPattern table
to verify scheme/host comparison is case-insensitive (per RFC): call out the
MatchRedirectURIPattern function and add an entry where the pattern is
"https://example.com/callback" and the incoming URI uses mixed-case scheme/host
like "HTTPS://Example.com/callback" (wantMatch: true), so the matcher is
exercised for case-normalization rather than raw string equality; place this new
case alongside the other positive matches in the tests slice.
In `@tests/e2e/tests/oauth-app/wildcard-redirect-uri.spec.ts`:
- Around line 228-386: The tests use fixed application names (e.g., the "name"
field values like "e2e-wc-ac01", "e2e-wc-ac04", "e2e-wc-ac13", etc.) which can
collide across runs; update each POST payload in these tests to generate a
unique name (append a timestamp or random suffix such as Date.now() or a UUID)
when setting the name before calling the API (keep createdAppIds.push(body.id)
unchanged). Ensure all tests in this file that set name values (AC-01 through
AC-05, AC-13, AC-14) use the same uniqueness pattern so runs against reused
tenants won’t conflict.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f1218cdb-42c1-41de-a08e-2be3e27e3d1e
📒 Files selected for processing (10)
AGENTS.mdbackend/internal/application/model/oauth_app.gobackend/internal/application/model/oauth_app_test.gobackend/internal/application/service.gobackend/internal/application/service_test.gobackend/internal/oauth/oauth2/authz/validator_test.gobackend/internal/system/utils/http_util.gobackend/internal/system/utils/http_util_test.gofeature-docs/wildcard-redirect-uri.mdtests/e2e/tests/oauth-app/wildcard-redirect-uri.spec.ts
Comment on lines
+70
to
+159
| // MatchRedirectURIPattern reports whether incoming matches pattern. | ||
| // pattern may contain * (exactly one path segment) or ** (zero or more path segments) | ||
| // only in the path component. Scheme, host, and query must match exactly (case-insensitive | ||
| // scheme and host per RFC 3986). Returns (false, error) for malformed inputs, | ||
| // (false, nil) for no match, (true, nil) for a match. | ||
| func MatchRedirectURIPattern(pattern, incoming string) (bool, error) { | ||
| patternURL, err := url.Parse(pattern) | ||
| if err != nil || patternURL.Scheme == "" || patternURL.Host == "" { | ||
| return false, errors.New("invalid pattern URI: missing scheme or host") | ||
| } | ||
| incomingURL, err := url.Parse(incoming) | ||
| if err != nil || incomingURL.Scheme == "" || incomingURL.Host == "" { | ||
| return false, errors.New("invalid incoming URI: missing scheme or host") | ||
| } | ||
|
|
||
| if !strings.EqualFold(patternURL.Scheme, incomingURL.Scheme) { | ||
| return false, nil | ||
| } | ||
| if !strings.EqualFold(patternURL.Host, incomingURL.Host) { | ||
| return false, nil | ||
| } | ||
| if patternURL.RawQuery != incomingURL.RawQuery { | ||
| return false, nil | ||
| } | ||
| if incomingURL.Fragment != "" { | ||
| return false, nil | ||
| } | ||
| return matchPathPattern(patternURL.Path, incomingURL.Path, 0), nil | ||
| } | ||
|
|
||
| // matchPathPattern reports whether incomingPath matches patternPath. | ||
| // Wildcards * (one segment) and ** (zero or more segments) are supported in patternPath. | ||
| // depth guards against excessive recursion. | ||
| func matchPathPattern(patternPath, incomingPath string, depth int) bool { | ||
| if depth > 32 { | ||
| return false | ||
| } | ||
| patSegs := strings.Split(patternPath, "/") | ||
| incSegs := strings.Split(incomingPath, "/") | ||
| return matchSegs(patSegs, incSegs, 0, 0, depth) | ||
| } | ||
|
|
||
| func matchSegs(patSegs, incSegs []string, i, j, depth int) bool { | ||
| if depth > 32 { | ||
| return false | ||
| } | ||
| // Both exhausted. | ||
| if i == len(patSegs) && j == len(incSegs) { | ||
| return true | ||
| } | ||
| // Pattern exhausted but incoming still has segments. | ||
| if i == len(patSegs) { | ||
| return false | ||
| } | ||
| // Incoming exhausted but pattern still has segments: | ||
| // only true if all remaining pattern segments are "**". | ||
| if j == len(incSegs) { | ||
| for k := i; k < len(patSegs); k++ { | ||
| if patSegs[k] != "**" { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| pSeg := patSegs[i] | ||
|
|
||
| if pSeg == "**" { | ||
| // Try consuming zero incoming segments (advance pattern only). | ||
| if matchSegs(patSegs, incSegs, i+1, j, depth+1) { | ||
| return true | ||
| } | ||
| // Try consuming one incoming segment (keep pattern position). | ||
| return matchSegs(patSegs, incSegs, i, j+1, depth+1) | ||
| } | ||
|
|
||
| if pSeg == "*" { | ||
| // Must match exactly one non-empty segment. | ||
| if incSegs[j] == "" { | ||
| return false | ||
| } | ||
| return matchSegs(patSegs, incSegs, i+1, j+1, depth+1) | ||
| } | ||
|
|
||
| // Literal segment: must match exactly. | ||
| if pSeg != incSegs[j] { | ||
| return false | ||
| } | ||
| return matchSegs(patSegs, incSegs, i+1, j+1, depth+1) | ||
| } |
There was a problem hiding this comment.
🔴 Documentation Required.
This adds a user-visible OAuth behavior change: clients can now register and authorize with wildcard redirect_uri patterns. I only see internal notes under feature-docs/; please also update public docs under docs/—at minimum the OAuth/application API reference in docs/content/apis.mdx and the relevant guide in docs/content/guides/ (or a new wildcard redirect URI guide) to document */**, the path-only restriction, and the scheme/host/query rules before merge.
As per coding guidelines, "🔴 Documentation Required: This PR introduces a user-facing change (e.g., new/modified API endpoint, configuration option, authentication flow, or SDK behavior) but does not include corresponding documentation updates under docs/. Please update the relevant documentation before merging."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/internal/system/utils/http_util.go` around lines 70 - 159, The code
introduces user-visible wildcard redirect_uri semantics via
MatchRedirectURIPattern and helpers matchPathPattern/matchSegs; add public
documentation before merging by updating the OAuth/application API reference and
guides to describe the new '*' (single segment) and '**' (zero-or-more segments)
wildcards, the restriction that wildcards are only allowed in the path, and the
exact matching rules for scheme, host (case-insensitive per RFC 3986), and
query; include examples of valid/invalid pattern+incoming URIs, note the
fragment is disallowed, and link to the internal feature-docs for implementation
details so users can understand behavior and migrate safely.
Comment on lines
+103
to
+159
| func matchPathPattern(patternPath, incomingPath string, depth int) bool { | ||
| if depth > 32 { | ||
| return false | ||
| } | ||
| patSegs := strings.Split(patternPath, "/") | ||
| incSegs := strings.Split(incomingPath, "/") | ||
| return matchSegs(patSegs, incSegs, 0, 0, depth) | ||
| } | ||
|
|
||
| func matchSegs(patSegs, incSegs []string, i, j, depth int) bool { | ||
| if depth > 32 { | ||
| return false | ||
| } | ||
| // Both exhausted. | ||
| if i == len(patSegs) && j == len(incSegs) { | ||
| return true | ||
| } | ||
| // Pattern exhausted but incoming still has segments. | ||
| if i == len(patSegs) { | ||
| return false | ||
| } | ||
| // Incoming exhausted but pattern still has segments: | ||
| // only true if all remaining pattern segments are "**". | ||
| if j == len(incSegs) { | ||
| for k := i; k < len(patSegs); k++ { | ||
| if patSegs[k] != "**" { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| pSeg := patSegs[i] | ||
|
|
||
| if pSeg == "**" { | ||
| // Try consuming zero incoming segments (advance pattern only). | ||
| if matchSegs(patSegs, incSegs, i+1, j, depth+1) { | ||
| return true | ||
| } | ||
| // Try consuming one incoming segment (keep pattern position). | ||
| return matchSegs(patSegs, incSegs, i, j+1, depth+1) | ||
| } | ||
|
|
||
| if pSeg == "*" { | ||
| // Must match exactly one non-empty segment. | ||
| if incSegs[j] == "" { | ||
| return false | ||
| } | ||
| return matchSegs(patSegs, incSegs, i+1, j+1, depth+1) | ||
| } | ||
|
|
||
| // Literal segment: must match exactly. | ||
| if pSeg != incSegs[j] { | ||
| return false | ||
| } | ||
| return matchSegs(patSegs, incSegs, i+1, j+1, depth+1) | ||
| } |
There was a problem hiding this comment.
The 32-step cap rejects valid deep paths.
Line 104 and Line 113 increment depth on every recursive step, not just on wildcard expansion. That means a legitimate match with more than 32 path steps can fail even when the pattern is compatible, including simple /** cases. A memoized (i, j) matcher would keep the work bounded without turning deep-but-valid redirect URIs into false negatives.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/internal/system/utils/http_util.go` around lines 103 - 159, The
recursive matcher currently uses a global depth cap which rejects valid deep
paths; replace this with memoization of (i,j) to bound work. In
matchPathPattern, create a memo map (e.g., map[[2]int]bool or map[string]bool)
and pass it into matchSegs; in matchSegs (and remove the depth parameter), check
the memo for the current (i,j) and return false if already visited, otherwise
mark (i,j) visited before exploring recursion; keep all existing segment logic
(*, **) but eliminate the depth > 32 checks so deep-but-valid patterns like /**
succeed. Ensure you update signatures: matchPathPattern -> matchSegs to accept
the memo and remove depth usage.
Author
There was a problem hiding this comment.
We have intentionally capped it to 32-step depth during the path match.
| - The redirect_uri in token requests is still validated with exact match logic per RFC 6749 §4.1.3; pattern matching is not applied there. | ||
| - Invalid redirect_uri should inform the resource owner of the error and MUST NOT automatically redirect the user-agent to the invalid redirection URI | ||
| - Support deeplinks for the mobile app redirections. wildcards are not allowed in the scheme part of the deeplink. | ||
| - At least one redirection uri MUST be registrated per client. |
There was a problem hiding this comment.
Don't require redirect URIs for every OAuth client.
Line 16 says every client must register at least one redirect URI, but the validator only enforces that when authorization_code is enabled. Please scope this bullet to authorization-based clients so the feature contract matches the implementation.
📝 Suggested edit
-- At least one redirection uri MUST be registrated per client.
+- Clients using the `authorization_code` grant MUST register at least one redirect URI.📝 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.
Suggested change
| - At least one redirection uri MUST be registrated per client. | |
| - Clients using the `authorization_code` grant MUST register at least one redirect URI. |
🧰 Tools
🪛 LanguageTool
[grammar] ~16-~16: Ensure spelling is correct
Context: ... - At least one redirection uri MUST be registrated per client. ## Non-Goals - Query para...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@feature-docs/wildcard-redirect-uri.md` at line 16, The current bullet "At
least one redirection uri MUST be registrated per client." is too broad; change
it to require redirect URIs only for authorization-based clients by rewording it
to something like "Clients that use the authorization_code grant MUST register
at least one redirection URI." Ensure the phrasing references the
authorization_code grant (or "authorization-based clients") so the documented
requirement matches the validator logic.
Comment on lines
+406
to
+430
| test.beforeAll(async () => { | ||
| ctx = await playwrightRequest.newContext({ ignoreHTTPSErrors: true }); | ||
| adminToken = getAdminTokenFromStoredSession(); | ||
|
|
||
| const currentTimestamp = Date.now(); | ||
| // Create apps sequentially to avoid SQLite database lock contention | ||
| singleStarApp = await createApp(ctx, adminToken, `e2e-wc-authz-single-${currentTimestamp}`, ["https://client.example.com/cb/*"]); | ||
| doubleStarApp = await createApp(ctx, adminToken, `e2e-wc-authz-double-${currentTimestamp}`, ["https://client.example.com/app/**/cb"]); | ||
| exactApp = await createApp(ctx, adminToken, `e2e-wc-authz-exact-${currentTimestamp}`, ["https://client.example.com/callback"]); | ||
| queryApp = await createApp(ctx, adminToken, `e2e-wc-authz-query-${currentTimestamp}`, ["https://client.example.com/cb?foo=bar"]); | ||
| multiApp = await createApp(ctx, adminToken, `e2e-wc-authz-multi-${currentTimestamp}`, [ | ||
| "https://client.example.com/a/*", | ||
| "https://client.example.com/b/*", | ||
| ]); | ||
| }); | ||
|
|
||
| test.afterAll(async () => { | ||
| // Delete apps sequentially to avoid SQLite database lock contention | ||
| await deleteApp(ctx, adminToken, singleStarApp.appId); | ||
| await deleteApp(ctx, adminToken, doubleStarApp.appId); | ||
| await deleteApp(ctx, adminToken, exactApp.appId); | ||
| await deleteApp(ctx, adminToken, queryApp.appId); | ||
| await deleteApp(ctx, adminToken, multiApp.appId); | ||
| await ctx.dispose(); | ||
| }); |
There was a problem hiding this comment.
Guard cleanup against partial beforeAll failures.
If one createApp() call fails, the remaining singleStarApp / doubleStarApp / ... vars stay uninitialized and Lines 424-428 will throw before cleanup finishes. That can mask the original failure and leak the apps that were already created.
Suggested hardening
test.describe("Wildcard Redirect URI — Authorization Request Matching", () => {
let ctx: APIRequestContext;
let adminToken: string;
+ const createdApps: CreatedApp[] = [];
...
- singleStarApp = await createApp(ctx, adminToken, `e2e-wc-authz-single-${currentTimestamp}`, ["https://client.example.com/cb/*"]);
- doubleStarApp = await createApp(ctx, adminToken, `e2e-wc-authz-double-${currentTimestamp}`, ["https://client.example.com/app/**/cb"]);
- exactApp = await createApp(ctx, adminToken, `e2e-wc-authz-exact-${currentTimestamp}`, ["https://client.example.com/callback"]);
- queryApp = await createApp(ctx, adminToken, `e2e-wc-authz-query-${currentTimestamp}`, ["https://client.example.com/cb?foo=bar"]);
- multiApp = await createApp(ctx, adminToken, `e2e-wc-authz-multi-${currentTimestamp}`, [
+ singleStarApp = await createApp(ctx, adminToken, `e2e-wc-authz-single-${currentTimestamp}`, ["https://client.example.com/cb/*"]);
+ createdApps.push(singleStarApp);
+ doubleStarApp = await createApp(ctx, adminToken, `e2e-wc-authz-double-${currentTimestamp}`, ["https://client.example.com/app/**/cb"]);
+ createdApps.push(doubleStarApp);
+ exactApp = await createApp(ctx, adminToken, `e2e-wc-authz-exact-${currentTimestamp}`, ["https://client.example.com/callback"]);
+ createdApps.push(exactApp);
+ queryApp = await createApp(ctx, adminToken, `e2e-wc-authz-query-${currentTimestamp}`, ["https://client.example.com/cb?foo=bar"]);
+ createdApps.push(queryApp);
+ multiApp = await createApp(ctx, adminToken, `e2e-wc-authz-multi-${currentTimestamp}`, [
"https://client.example.com/a/*",
"https://client.example.com/b/*",
]);
+ createdApps.push(multiApp);
});
test.afterAll(async () => {
- await deleteApp(ctx, adminToken, singleStarApp.appId);
- await deleteApp(ctx, adminToken, doubleStarApp.appId);
- await deleteApp(ctx, adminToken, exactApp.appId);
- await deleteApp(ctx, adminToken, queryApp.appId);
- await deleteApp(ctx, adminToken, multiApp.appId);
+ for (const app of createdApps) {
+ await deleteApp(ctx, adminToken, app.appId);
+ }
await ctx.dispose();
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/tests/oauth-app/wildcard-redirect-uri.spec.ts` around lines 406 -
430, Initialize the created app variables (singleStarApp, doubleStarApp,
exactApp, queryApp, multiApp) to null before test.beforeAll and make
test.afterAll resilient: check each variable for truthiness before calling
deleteApp (e.g. if (singleStarApp) await deleteApp(ctx, adminToken,
singleStarApp.appId)), wrap each deleteApp call in its own try/catch so one
failing delete doesn't stop the rest, and ensure ctx.dispose() is called in a
finally-like manner (guarding if ctx exists) so resources are always released
even if beforeAll partially fails; update references to createApp/deleteApp
accordingly.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2154 +/- ##
==========================================
- Coverage 89.84% 89.84% -0.01%
==========================================
Files 898 898
Lines 58951 59038 +87
==========================================
+ Hits 52967 53044 +77
- Misses 4418 4425 +7
- Partials 1566 1569 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Wildcard support in the redirect_uri's both during registration and authorization calls.
Approach
Approch is documented here - feature-docs/wildcard-redirect-uri.md
Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
Release Notes
New Features
*(single segment) and**(zero or more segments) wildcards, while scheme, domain, and query parameters require exact matching.Documentation