fix(pages-router): throw on javascript: URL synchronously in router.push/replace#1575
fix(pages-router): throw on javascript: URL synchronously in router.push/replace#1575james-elicx wants to merge 1 commit into
Conversation
Hoist the `javascript:` / `data:` / `vbscript:` block from inside the
async `performNavigation` body up into `Router.push` / `Router.replace`
themselves so the throw is fully synchronous. Mirrors Next.js's Pages
Router `push`/`replace` at
packages/next/src/shared/lib/router/router.ts:1025-1033,1057-1065.
Why: `performNavigation` is an `async` function, so throwing from inside
it wraps the error in a rejected Promise. `<button onClick={() =>
router.push("javascript:...")}>` does not await that Promise, so React's
event-handler error reporter never observes the throw and the matching
`console.error` is not surfaced through page logs. That is the failure
shape asserted by Next.js's
`test/e2e/app-dir/javascript-urls/javascript-urls.test.ts:341,376`.
Behaviour preserved: navigation is still blocked, and the matching
`console.error("Next.js has blocked a javascript: URL as a security
precaution.")` is still emitted by `assertSafeNavigationUrl`. The inner
guard inside `performNavigation` is kept as defence-in-depth.
Refs #1349
|
/bigbonk review |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-scoped fix. The analysis is correct: an async function always wraps thrown exceptions in a rejected Promise, so the previous guard inside performNavigation could never surface the error synchronously through React's event-handler error reporter. Hoisting the guard into the synchronous push/replace wrappers matches Next.js's Pages Router behavior.
A few observations:
Defence-in-depth is preserved correctly. The inner assertSafeNavigationUrl call inside performNavigation is kept, and the comment is updated to explain its role as a catch-all for any hypothetical future caller that bypasses RouterMethods. This is the right layering.
useRouter() consumers also get the fix. PagesRouterProvider wires Router.push / Router.replace directly into the context value (router.ts:1009-1010), so hook-based callers inherit the synchronous guard without any additional changes.
The regression test ("does not return a rejected Promise") is a valuable addition — it directly asserts the invariant that caused the original bug (the return value is never assigned because the throw fires first).
performNavigation is module-private. Confirmed there are no other call sites — only RouterMethods.push and RouterMethods.replace call it. The defence-in-depth guard is still justified as forward-looking protection, but the surface area is well-contained.
Type signature note (non-blocking): NextRouter.push/replace are typed as returning Promise<boolean>, which is accurate for the happy path (the return performNavigation(...) still returns the async function's Promise). On the dangerous-URL path, the synchronous throw prevents a return value entirely. This is the same contract as Next.js's Pages Router — their push/replace are also typed as returning Promise<boolean> but throw synchronously before reaching the async body. No change needed, just noting the type-level contract is sound.
Looks good to ship once CI is green.
|
Review posted. Approved PR #1575 with a detailed analysis covering:
CI is still running (Check passed, several E2E jobs passed, integration tests pending). Good to merge once green. |
1 participant
Refs #1349 — Pages Router
router.push/router.replaceonly. Follow-up issue: #1576.Summary
performNavigationbody and intoRouter.push/Router.replacethemselves so the throw is fully synchronous. Matches Next.js's Pages Routerpush/replaceatpackages/next/src/shared/lib/router/router.ts:1025-1033,1057-1065.performNavigationisasync). When a typical Pages Router fixture does<button onClick={() => router.push("javascript:...")}>the Promise is not awaited, React's event-handler error reporter never observes it, and the matchingconsole.erroris never surfaced through page logs — the failure mode asserted bytest/e2e/app-dir/javascript-urls/javascript-urls.test.ts:341,376.console.error("Next.js has blocked a javascript: URL as a security precaution.")fromassertSafeNavigationUrlis unchanged; navigation continues to be blocked. The inner guard insideperformNavigationis kept as defence-in-depth.Out of scope (tracked in #1576)
<Link>click warning<Link>click warningrouter.push/router.replacewarningDo not close #1349 when this PR merges — close it when #1576 also lands.
Test plan
tests/router-javascript-urls.test.tsPages Router cases to assert the synchronous throw, plus a regression test thatRouter.pushnever returns a Promise on a dangerous URL.vp test run tests/router-javascript-urls.test.ts(22 tests passing).vp test run tests/link-navigation.test.ts tests/link.test.ts tests/navigation-runtime.test.ts tests/url-safety.test.ts(178 tests passing).vp checkclean.