fix(auth): replace bespoke wildcard matching with glob-based redirect…#1235
Conversation
WalkthroughImplements Supabase-style glob pattern matching for redirect URL allowlisting: schema regex expanded, picomatch dependency added, core validation rewritten with pattern normalization and picomatch matching, extensive unit tests added, and documentation navigation updated. ChangesGlob Pattern Support for Redirect URL Allowlisting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
backend/src/services/auth/auth-config.service.ts (2)
304-312: 💤 Low valueFallback branch in
normalizePatternis misleading.When
normalizeUrl(safe)returns null, you fall back topattern.toLowerCase().replace(/\/$/, '')and then iteratereplacementscallingfallback.replace(placeholder.toLowerCase(), original). Butfallbackis derived from the originalpatternand contains the actual glob meta-chars (*,?,[…]), not the__GLOBn__placeholders — so the loop never matches anything and is effectively dead code. The end result is correct (original glob chars are preserved), but the intent reads as if restoration is happening. Consider dropping the loop and just returning the lowercased/trimmed original pattern, with a comment explaining why no restoration is needed in this branch.♻️ Suggested simplification
const normalized = this.normalizeUrl(safe); if (!normalized) { - // Best-effort: lowercase and strip trailing slash. - let fallback = pattern.toLowerCase().replace(/\/$/, ''); - for (const { placeholder, original } of replacements) { - fallback = fallback.replace(placeholder.toLowerCase(), original); - } - return fallback; + // Best-effort: original pattern already contains the glob meta-chars, + // so just lowercase + strip trailing slash. picomatch will match against + // the (lower-cased) target produced by normalizeUrl(). + return pattern.toLowerCase().replace(/\/$/, ''); }🤖 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 `@backend/src/services/auth/auth-config.service.ts` around lines 304 - 312, In normalizePattern, the fallback branch (when normalizeUrl(safe) returns null) should not attempt to restore placeholders because fallback is based on the original pattern (with glob meta-chars) and will never contain the __GLOBn__ placeholders; remove the for-loop that iterates over replacements and simply return the lowercased/trimmed fallback, and add a short comment in the normalizePattern function explaining that restoration is unnecessary here because replacements were only used when normalizeUrl succeeded and the original pattern still contains glob characters (reference: normalizePattern, normalizeUrl, replacements, placeholder).
342-357: ⚡ Quick winConsider caching compiled picomatch matchers to avoid recompilation on every auth validation request.
validateRedirectUrl()is called frequently in hot auth paths (sign-in, OAuth flows, password reset, email verification). Each call iterates the entireallowedRedirectUrlslist, and for each pattern,picomatch.isMatch(target, pattern, opts)recompiles the pattern into a regex. Since patterns only change when the admin updates config, precompile each normalized pattern once usingpicomatch(normalizedPattern, { dot: true })to create a reusable matcher function, then cache the matcher keyed by the raw pattern. This avoids regex recompilation and reduces overhead per auth request.Additionally,
getAuthConfig()queries the database on every call. Consider cachingallowedRedirectUrlswith cache invalidation on config updates, or add memoization with a short TTL if config changes are infrequent.🤖 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 `@backend/src/services/auth/auth-config.service.ts` around lines 342 - 357, The matchesGlobPattern function is recompiling picomatch on every call; change AuthConfigService to cache compiled matchers and avoid per-request regex creation: add a Map<string, Matcher> (e.g., this._picomatchCache) keyed by the raw pattern and, in matchesGlobPattern, after normalizePattern look up or create the matcher via picomatch(normalizedPattern, { dot: true }) and call the cached matcher(normalizedTarget) instead of picomatch.isMatch; also provide a cache invalidation path whenever allowedRedirectUrls are updated (or clear the cache when config changes). Separately, reduce repeated DB hits from getAuthConfig by memoizing allowedRedirectUrls (or adding a short TTL cache) and ensure the memo/TTL is invalidated when auth config is updated so validateRedirectUrl iterates the cached list instead of querying every request.backend/tests/unit/auth-config.service.test.ts (2)
122-249: ⚡ Quick winAdd coverage for
?single-char wildcard and[…]character classes.The service docs and
redirect-urls.mdxadvertise?(single non-/character) and[abc]/[!abc]character classes as first-class supported patterns, but the test suite never exercises them. These are exactly the patterns most likely to regress on a picomatch upgrade or anormalizePatternchange. A handful of small tests would lock in the documented contract.🧪 Suggested tests to add
+ // -------------------------------------------------------------------------- + // Single-character wildcard (?) + // -------------------------------------------------------------------------- + describe('validateRedirectUrl — single-char wildcard (?)', () => { + it('matches exactly one non-slash character', async () => { + stubAllowedUrls(['https://example.com/v?']); + const service = AuthConfigService.getInstance(); + expect(await service.validateRedirectUrl('https://example.com/v1')).toBe(true); + expect(await service.validateRedirectUrl('https://example.com/v12')).toBe(false); + expect(await service.validateRedirectUrl('https://example.com/v')).toBe(false); + }); + }); + + // -------------------------------------------------------------------------- + // Character classes ([abc], [!abc], ranges) + // -------------------------------------------------------------------------- + describe('validateRedirectUrl — character classes', () => { + it('matches a character in the set', async () => { + stubAllowedUrls(['https://example.com/[abc]']); + const service = AuthConfigService.getInstance(); + expect(await service.validateRedirectUrl('https://example.com/a')).toBe(true); + expect(await service.validateRedirectUrl('https://example.com/d')).toBe(false); + }); + + it('matches a character range', async () => { + stubAllowedUrls(['https://example.com/[a-z]*']); + const service = AuthConfigService.getInstance(); + expect(await service.validateRedirectUrl('https://example.com/abc')).toBe(true); + expect(await service.validateRedirectUrl('https://example.com/Abc')).toBe(false); + }); + + it('negated character class', async () => { + stubAllowedUrls(['https://example.com/[!0-9]']); + const service = AuthConfigService.getInstance(); + expect(await service.validateRedirectUrl('https://example.com/a')).toBe(true); + expect(await service.validateRedirectUrl('https://example.com/1')).toBe(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 `@backend/tests/unit/auth-config.service.test.ts` around lines 122 - 249, Add unit tests that cover the single-character wildcard (?) and character-class patterns ([abc] and [!abc]) so the documented behavior can't regress: use stubAllowedUrls to register patterns like 'https://example.com/fo?' (expect 'https://example.com/foo' true, 'https://example.com/fooo' false), 'https://example.com/file[0-9]' (expect 'https://example.com/file3' true, 'https://example.com/filea' false), and a negated class like 'https://example.com/file[!a]' (expect 'https://example.com/fileb' true, 'https://example.com/filea' false); call AuthConfigService.getInstance().validateRedirectUrl for each case and assert true/false accordingly.
182-204: 💤 Low valueAdd an explicit test for
**matching the apex with no path.A pattern like
https://example.com/**is commonly intended to mean "any path on this host, including the host root". SincenormalizeUrlremoves trailing slashes, bothhttps://example.comandhttps://example.com/normalize tohttps://example.combefore picomatch matching. The**token should match zero segments per picomatch semantics, but this behavior is worth pinning with an explicit assertion to guard against future picomatch version changes or normalization tweaks.🧪 Suggested addition
it('matches deeply nested paths', async () => { stubAllowedUrls(['https://example.com/**']); const service = AuthConfigService.getInstance(); expect(await service.validateRedirectUrl('https://example.com/foo/bar')).toBe(true); expect(await service.validateRedirectUrl('https://example.com/foo/bar/baz')).toBe(true); }); + + it('matches the host root with no path', async () => { + stubAllowedUrls(['https://example.com/**']); + + const service = AuthConfigService.getInstance(); + expect(await service.validateRedirectUrl('https://example.com')).toBe(true); + expect(await service.validateRedirectUrl('https://example.com/')).toBe(true); + });🤖 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 `@backend/tests/unit/auth-config.service.test.ts` around lines 182 - 204, Add a unit test asserting that the recursive wildcard pattern matches the apex (no-path) host: when stubAllowedUrls(['https://example.com/**']) call AuthConfigService.getInstance().validateRedirectUrl('https://example.com') and expect true. This pins behavior of the '**' token (and normalizeUrl/picomatch interaction) so the pattern matches zero path segments; add the assertion alongside the existing tests that exercise single/deep/query paths.packages/shared-schemas/src/auth.schema.ts (1)
106-107: ⚖️ Poor tradeoffRegex is correct but hard to maintain — consider naming sub-parts.
The expression compresses several distinct concerns (http(s) host with optional
*.prefix, IPv6 literal, custom-scheme branch with javascript/data/file/vbscript guard, port, path) into one line, with two near-duplicate hostname alternates. Tests cover the main shapes, but anyone editing this in the future will have a hard time reasoning about which branch handles which input. Consider building it from clearly-named sub-RegExp source strings (e.g.HOST_GLOB,IPV6_LITERAL,PATH,CUSTOM_SCHEME) and composing them withnew RegExp([...].join(''), 'i'), with comments per part. Functionally no change; purely maintainability.🤖 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 `@packages/shared-schemas/src/auth.schema.ts` around lines 106 - 107, The allowedRedirectUrlsRegex is correct but dense and hard to maintain; refactor by extracting named sub-pattern strings (e.g., HOST_GLOB, IPV6_LITERAL, PORT, PATH, CUSTOM_SCHEME) and compose the full pattern via new RegExp([...parts...].join(''), 'i') in the same file so each part can be commented and tested separately; replace the single-line literal const allowedRedirectUrlsRegex with the composed RegExp while preserving exact matching behavior and test coverage.docs/core-concepts/authentication/redirect-urls.mdx (1)
79-85: 💤 Low valueConsider clarifying the
?wildcard behavior in the query-string example.The pattern
https://example.com/?session=*will match bothhttps://example.com/?session=abc123and unintended targets likehttps://example.com/Xsession=foo, because in picomatch,?matches any single character (except/)—it does not function as a literal URL query delimiter. Adding a brief note that?is a glob wildcard (not literal syntax) will help users avoid surprise matches when constructing their own patterns.🤖 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 `@docs/core-concepts/authentication/redirect-urls.mdx` around lines 79 - 85, Update the "Query string wildcards" section to clarify that the leading `?` in the example pattern is not a literal URL delimiter but a picomatch single-character wildcard, and advise users to escape the `?` (e.g., `\?`) or percent-encode it if they mean a literal query delimiter; update the example `https://example.com/?session=*` and add a brief note explaining the wildcard behavior so readers know how to match only real query strings.
🤖 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 `@backend/src/services/auth/auth-config.service.ts`:
- Around line 304-312: In normalizePattern, the fallback branch (when
normalizeUrl(safe) returns null) should not attempt to restore placeholders
because fallback is based on the original pattern (with glob meta-chars) and
will never contain the __GLOBn__ placeholders; remove the for-loop that iterates
over replacements and simply return the lowercased/trimmed fallback, and add a
short comment in the normalizePattern function explaining that restoration is
unnecessary here because replacements were only used when normalizeUrl succeeded
and the original pattern still contains glob characters (reference:
normalizePattern, normalizeUrl, replacements, placeholder).
- Around line 342-357: The matchesGlobPattern function is recompiling picomatch
on every call; change AuthConfigService to cache compiled matchers and avoid
per-request regex creation: add a Map<string, Matcher> (e.g.,
this._picomatchCache) keyed by the raw pattern and, in matchesGlobPattern, after
normalizePattern look up or create the matcher via picomatch(normalizedPattern,
{ dot: true }) and call the cached matcher(normalizedTarget) instead of
picomatch.isMatch; also provide a cache invalidation path whenever
allowedRedirectUrls are updated (or clear the cache when config changes).
Separately, reduce repeated DB hits from getAuthConfig by memoizing
allowedRedirectUrls (or adding a short TTL cache) and ensure the memo/TTL is
invalidated when auth config is updated so validateRedirectUrl iterates the
cached list instead of querying every request.
In `@backend/tests/unit/auth-config.service.test.ts`:
- Around line 122-249: Add unit tests that cover the single-character wildcard
(?) and character-class patterns ([abc] and [!abc]) so the documented behavior
can't regress: use stubAllowedUrls to register patterns like
'https://example.com/fo?' (expect 'https://example.com/foo' true,
'https://example.com/fooo' false), 'https://example.com/file[0-9]' (expect
'https://example.com/file3' true, 'https://example.com/filea' false), and a
negated class like 'https://example.com/file[!a]' (expect
'https://example.com/fileb' true, 'https://example.com/filea' false); call
AuthConfigService.getInstance().validateRedirectUrl for each case and assert
true/false accordingly.
- Around line 182-204: Add a unit test asserting that the recursive wildcard
pattern matches the apex (no-path) host: when
stubAllowedUrls(['https://example.com/**']) call
AuthConfigService.getInstance().validateRedirectUrl('https://example.com') and
expect true. This pins behavior of the '**' token (and normalizeUrl/picomatch
interaction) so the pattern matches zero path segments; add the assertion
alongside the existing tests that exercise single/deep/query paths.
In `@docs/core-concepts/authentication/redirect-urls.mdx`:
- Around line 79-85: Update the "Query string wildcards" section to clarify that
the leading `?` in the example pattern is not a literal URL delimiter but a
picomatch single-character wildcard, and advise users to escape the `?` (e.g.,
`\?`) or percent-encode it if they mean a literal query delimiter; update the
example `https://example.com/?session=*` and add a brief note explaining the
wildcard behavior so readers know how to match only real query strings.
In `@packages/shared-schemas/src/auth.schema.ts`:
- Around line 106-107: The allowedRedirectUrlsRegex is correct but dense and
hard to maintain; refactor by extracting named sub-pattern strings (e.g.,
HOST_GLOB, IPV6_LITERAL, PORT, PATH, CUSTOM_SCHEME) and compose the full pattern
via new RegExp([...parts...].join(''), 'i') in the same file so each part can be
commented and tested separately; replace the single-line literal const
allowedRedirectUrlsRegex with the composed RegExp while preserving exact
matching behavior and test coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fbfc97e1-f359-4805-acee-d7f0b505a7a8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
backend/package.jsonbackend/src/services/auth/auth-config.service.tsbackend/tests/unit/auth-config.service.test.tsdocs/core-concepts/authentication/redirect-urls.mdxdocs/docs.jsonpackages/shared-schemas/src/auth.schema.ts
|
@tonychang04 kindly review |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 7 files
Confidence score: 3/5
- There is a concrete validation regression risk in
packages/shared-schemas/src/auth.schema.ts: the new hostname alternative can match an empty string, which may incorrectly accept invalid origins likehttps://orhttps://:8080. - Given the issue’s medium-high severity (6/10) and high confidence (9/10), this introduces user-impacting behavior in auth-related schema checks, so merge risk is moderate rather than minimal.
- Pay close attention to
packages/shared-schemas/src/auth.schema.ts- tighten the hostname pattern (after optional*.) so at least one hostname character is required.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/shared-schemas/src/auth.schema.ts">
<violation number="1" location="packages/shared-schemas/src/auth.schema.ts:107">
P2: The new hostname alternative can match an empty string, allowing patterns like `https://` or `https://:8080` to pass validation. Add a lookahead after the optional `*.` prefix to ensure at least one hostname character is present.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
… URL validation (InsForge#1220) The previous validateRedirectUrl only supported *.host.com subdomain wildcards via a narrow *. substring check. All other patterns silently fell through to exact-match, making path wildcards, query-string wildcards, and combined patterns impossible to express. Replace the bespoke logic with picomatch glob matching (Supabase-compatible): - * matches a single segment (subdomain or path) - ** matches any depth including separators - ? matches a single non-separator character - [...] character class support Changes: - backend: rewrite validateRedirectUrl with picomatch + normalizePattern - shared-schemas: update allowedRedirectUrlsRegex to accept glob chars - tests: 32 comprehensive test cases covering all acceptance criteria - docs: new redirect-urls.mdx with pattern syntax, examples, security notes Protocol and port matching remain strict. *.example.com does not match the apex domain (add it separately). No breaking changes to existing *.host.com patterns. Closes InsForge#1220
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/tests/unit/auth-config.service.test.ts (1)
362-379: ⚡ Quick winAdd
validateRedirectUrlbehavior tests for?and[...]globs.
?and character classes are currently asserted at regex-acceptance level, but not verified end-to-end invalidateRedirectUrlmatching semantics. Adding these closes a regression gap for the new glob feature set.Proposed test additions
+ // -------------------------------------------------------------------------- + // Additional glob operators (?, [...]) + // -------------------------------------------------------------------------- + describe('validateRedirectUrl — additional glob operators', () => { + it('matches single-character wildcard (?) in host', async () => { + stubAllowedUrls(['https://app?.example.com/auth/callback']); + + const service = AuthConfigService.getInstance(); + expect(await service.validateRedirectUrl('https://app1.example.com/auth/callback')).toBe(true); + expect(await service.validateRedirectUrl('https://app12.example.com/auth/callback')).toBe(false); + }); + + it('matches character class wildcard ([...]) in path', async () => { + stubAllowedUrls(['https://example.com/auth/[a-c]allback']); + + const service = AuthConfigService.getInstance(); + expect(await service.validateRedirectUrl('https://example.com/auth/ballback')).toBe(true); + expect(await service.validateRedirectUrl('https://example.com/auth/zallback')).toBe(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 `@backend/tests/unit/auth-config.service.test.ts` around lines 362 - 379, Add end-to-end tests that exercise validateRedirectUrl (not just allowedRedirectUrlsRegex) for patterns using the single-character wildcard '?' and character classes '[...]'; specifically, add it.each cases that call validateRedirectUrl with patterns like 'https://example.com/?session=*' and 'https://example.com/[a-z]*' (and matching candidate URLs) and assert true/false according to expected matching semantics. Ensure the new tests reference validateRedirectUrl and allowedRedirectUrlsRegex so both the regex and the higher-level validation logic are covered and verify acceptance and rejection of edge cases for the new glob feature set.
🤖 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 `@backend/tests/unit/auth-config.service.test.ts`:
- Around line 362-379: Add end-to-end tests that exercise validateRedirectUrl
(not just allowedRedirectUrlsRegex) for patterns using the single-character
wildcard '?' and character classes '[...]'; specifically, add it.each cases that
call validateRedirectUrl with patterns like 'https://example.com/?session=*' and
'https://example.com/[a-z]*' (and matching candidate URLs) and assert true/false
according to expected matching semantics. Ensure the new tests reference
validateRedirectUrl and allowedRedirectUrlsRegex so both the regex and the
higher-level validation logic are covered and verify acceptance and rejection of
edge cases for the new glob feature set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e489411-d6df-40bc-8302-d4b0e84b0348
📒 Files selected for processing (2)
backend/tests/unit/auth-config.service.test.tspackages/shared-schemas/src/auth.schema.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/shared-schemas/src/auth.schema.ts
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Summary
Solid, well-scoped bug fix that replaces a fragile substring-based wildcard with a proper glob engine (picomatch). The implementation is correct, backward-compatible, and thoroughly tested. No blocking issues found.
Requirements context
No matching spec or plan was found under /docs/superpowers/specs/ or /docs/superpowers/plans/ for redirect-URL validation. Assessment is based on PR description, issue #1220, and the existing validateRedirectUrl contract.
Findings
Critical
(none)
Suggestion
[Functionality] normalizePattern fallback has a latent case-sensitivity mismatch — auth-config.service.ts:305-311
The fallback branch (reached when normalizeUrl(safe) returns null) lowercases the entire pattern string before restoring glob tokens:
let fallback = pattern.toLowerCase().replace(/\/$/, '');But the normalizedTarget passed in from matchesGlobPattern is produced by new URL(urlStr).href, which lowercases only the hostname — the path preserves its original case (per the URL spec). If a path component differs in case between pattern and target, the subsequent picomatch.isMatch call can fail to match even though it should.
Blast radius is low — this fallback is dead code for any pattern that passes allowedRedirectUrlsRegex (all valid inputs), because schema validation guarantees the pattern is a parseable URL. But it is silent incorrect behavior for any edge cases that somehow reach it (e.g. a code path that sets allowedRedirectUrls without going through the schema). Options:
- Replace the lowercased fallback with a simple
return pattern.replace(/\/$/, '')(keep original case, sincepicomatch.isMatchwithdot: truerespects case by default, matching the URL spec behavior). - Or remove the fallback entirely and return an unmatchable sentinel string (e.g.
'\0') — this makes the rejection explicit and removes a dead branch.
[Software Engineering] Fallback path has no direct test coverage — auth-config.service.ts:305-311
The fallback in normalizePattern is an instance-private method, so it's only tested indirectly through validateRedirectUrl. No test currently exercises the fallback branch (a pattern that remains unparseable after placeholder substitution). Consider:
- Adding an integration-style test where the allowlist entry is a deliberately malformed pattern (bypassing schema validation via the mock), to pin the fallback's behavior.
- Or, as above, eliminating the fallback so there is no branch to miss.
Information
[Software Engineering] normalizeUrl and normalizePattern could be private static — auth-config.service.ts:263-321
Neither method uses instance state — normalizeUrl only calls new URL() and normalizePattern only calls this.normalizeUrl. Making both private static would make the absence of side-effects explicit and allows calling them without a service instance in future tests. Minor style note only.
[Software Engineering] Test gap: ** vs root path — auth-config.service.test.ts (recursive path wildcard block)
The ** tests cover /foo and /foo/bar/baz, but not https://example.com/** matched against https://example.com/ (a trailing-slash root). After normalization, normalizeUrl('https://example.com/') strips the trailing slash to https://example.com, while the pattern becomes https://example.com/**. Whether picomatch treats https://example.com as matching https://example.com/** (i.e., is an empty segment after the literal / accepted by **?) is worth pinning with an explicit test.
[Functionality / Docs] Permissive-default callout could use <Warning> — docs/core-concepts/authentication/redirect-urls.mdx:15-19
The current <Info> block describes the empty-allowlist behavior:
If no redirect URLs are configured (empty list), InsForge allows all redirect URLs. This is intentional…
This is a high-impact footgun in production — any redirectTo value, including crafted ones, will pass validation until the allowlist is populated. Surfacing it as <Warning> would better signal to operators that this is a configuration step they must not skip before going live.
Verdict
approved (informational — no Critical findings; human approval required via the approve flow)
All four review dimensions checked:
- Software engineering — 32 tests, excellent coverage across all documented pattern types; test structure is clean and readable.
- Functionality — back-compat for
*.host.comis preserved and verified; protocol and port strictness are correctly enforced by keeping them as literal characters in the normalized string before glob matching; apex-domain exclusion is intentional and tested. - Security —
picomatchis a widely-used, reputable library (part of the micromatch/chokidar ecosystem) with no known CVEs at this version; no secrets logged or returned; no auth checks weakened; placeholder restoration does not introduce regex-replacement injection since no glob token begins with$. - Performance —
picomatch.isMatchuses an internal compiled-pattern cache keyed by pattern string, so repeated calls with the same allowlist patterns incur no re-compilation overhead.allowedRedirectUrlslists are expected to be small; no N+1 concern.
| @@ -0,0 +1,138 @@ | |||
| --- | |||
There was a problem hiding this comment.
can you not change our doc right now? i think this is good but docs maybe we can add it ourselves
…e#1220) - Make normalizeUrl and normalizePattern static methods - Add test: ** matches root URL (with and without trailing slash) - Add test: unparseable patterns that bypass schema validation are rejected - Remove redirect-urls.mdx (docs to be handled separately)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@backend/src/services/auth/auth-config.service.ts`:
- Around line 276-355: The restore step in normalizePattern currently reinserts
raw tokens (e.g., "?", "[" , "]") which picomatch then treats as glob syntax;
change the restoration loop in AuthConfigService.normalizePattern to replace
each placeholder with an escaped version of the original token (escape
characters matching /([*?[\]\\])/g → '\\$1') before doing result.replace(new
RegExp(placeholder, 'i'), escaped) so literal URL syntax is treated literally;
keep using AuthConfigService.GLOB_CHARS, UNMATCHABLE_PATTERN, normalizeUrl and
picomatch.isMatch as-is and ensure matchesGlobPattern still routes to picomatch
only when pattern truly contains glob semantics.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63fa2130-5ff7-4528-ab1a-f4a5c81ffc6a
📒 Files selected for processing (2)
backend/src/services/auth/auth-config.service.tsbackend/tests/unit/auth-config.service.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/unit/auth-config.service.test.ts
There was a problem hiding this comment.
1 issue found across 6 files
Confidence score: 3/5
- There is a concrete user-impacting risk in
backend/src/services/auth/auth-config.service.ts: IPv6 redirect URLs likehttps://[::1]/cbcan be misinterpreted due to glob character-class handling, which may cause valid auth callbacks to fail or match the wrong pattern. - Given the severity (7/10) and strong confidence (8/10), this looks more than a minor edge case and could affect environments that rely on bracketed IPv6 hosts during login/redirect flows.
- This is likely fixable and localized, but it introduces meaningful regression risk in redirect validation behavior, so merge confidence is moderate rather than high.
- Pay close attention to
backend/src/services/auth/auth-config.service.ts- redirect pattern parsing around bracketed IPv6 hosts may behave incorrectly.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/src/services/auth/auth-config.service.ts">
<violation number="1" location="backend/src/services/auth/auth-config.service.ts:354">
P1: IPv6 redirect patterns are interpreted as glob character classes, so valid bracketed hosts (e.g. `https://[::1]/cb`) may fail or match incorrectly.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
Distinguish IPv6 host brackets (e.g. [::1], [2001:db8::1]) from glob character classes during pattern normalization. Without this, picomatch treats [::1] as a char class and incorrectly matches single characters like https://1/cb against https://[::1]/cb. - Detect IPv6 bracket content (hex/colons with ≥2 colons) and escape brackets on restoration to force literal matching - Lowercase IPv6 host to align with URL.href hostname normalization - Add 5 test cases covering IPv6 matching and regression prevention
|
@cubic-dev-ai review |
@tonychang04 I have started the AI code review. It will take a few minutes to complete. |
Per review feedback on PR InsForge#1220 — docs to be added separately.
|
@tonychang04 kindly review |
|
🚀merge! |
3 participants
Summary
Closes #1220. Replaces the bespoke
*.substring check invalidateRedirectUrlwith a real glob matcher (
picomatch) soallowedRedirectUrlsacceptsSupabase-style patterns instead of silently failing on anything that's not
https://*.host.com.validateRedirectUrl(in auth-config.service.ts) now matches viapicomatchafter normalising both pattern and target through theURLclass. Glob meta-chars (*,**,?,[…]) are placeholder-swapped before normalisation sonew URL()doesn't reject or percent-encode them.allowedRedirectUrlsRegex(in auth.schema.ts) widened to accept*/**/?/[…]in the host and path; protocol stays strict (no globbing the scheme;javascript:/data:/ etc. still rejected via the existing negative lookahead).*.host.compatterns continue to match arbitrarily deep subdomains (the legacy matcher usedendsWith('.' + base)which did the same).*.example.comis intentionally not silently included — you must listexample.comseparately. Protocol and port remain strict — wildcards never cross those boundaries.How did you test this change?
cd backend && npx vitest run tests/unit/auth-config.service.test.ts— 32/32 passingnpx tsc --noEmitinbackendandpackages/shared-schemas— cleannpx prettier --checkon all touched files — cleanhttps://*.example.com/auth/*andhttps://example.com/?session=*in Auth → Settings, confirm a real OAuth/email-verify redirect against eachhttps://*.host.compatterns still pass (regression check)Summary by cubic
Replaced bespoke wildcard checks with glob-based redirect URL validation using
picomatch, enabling Supabase-style patterns across host, path, and query. Keeps protocol/port strict, preserves deep subdomain behavior, and adds comprehensive tests.Bug Fixes
picomatchover normalized URLs; supports*,**,?,[…];**also matches the root URL.*.example.comdeep-subdomain behavior; apex must be added separately.[::1]-style brackets as literals to prevent over-matching.Dependencies
picomatchand@types/picomatch.Written for commit 14e01cf. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Tests