fix(form): match Next.js navigation parity#2245
Conversation
|
/bigbonk review for issues |
commit: |
|
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
/bigbonk review — fix(form): match Next.js navigation parity
Reviewed the form-shim rework against Next.js v16.2.6 source (packages/next/src/client/app-dir/form.tsx). The action-URL classification, external/protocol-relative hard-navigation fallthrough, and verbatim submitter formAction handling all match upstream, and the production prefetch loading-shell change looks consistent with how <Link> seeds optimistic shells. One blocking issue.
Blocking: dangerous-scheme (javascript:/data:/vbscript:) form actions are no longer blocked (parity + security regression)
This PR removes isSafeAction (which delegated to isDangerousScheme) from both the form-level action gate and the submitter effectiveAction gate in packages/vinext/src/shims/form.tsx. Removing the external-origin and protocol-relative blocking is correct — Next.js intentionally hard-navigates to those. But removing the dangerous-scheme block diverges from Next.js and from the rest of this repo.
In Next.js, onFormSubmit ends with router[method](targetHref, { scroll }) (form.tsx:230). The App Router router.push/replace block javascript: URLs and throw "Next.js has blocked a javascript: URL as a security precaution." (app-router-instance.ts:332,389). So a javascript: form action is blocked by Next.js via the router guard.
vinext's form does not route through the guarded router — it calls navigateClientSide(url, ...) directly (form.tsx:429). navigateClientSide has no assertSafeNavigationUrl/isDangerousScheme check (only _appRouter.push/replace do, navigation.ts:1789/1807). Trace for action="javascript:alert(1)" after this PR:
e.preventDefault()runs (form.tsx:422)createFormSubmitDestinationUrl→new URL("javascript:alert(1)", href)preserves thejavascript:schemenavigateClientSide→isExternalUrltrue →toSameOriginAppPathreturns null →hardNavigateTo→window.location.assign("javascript:...")
So vinext silently hard-navigates instead of blocking + emitting the standard console.error. Same path applies to data: and vbscript: (the repo intentionally extends the block to those — see shims/url-safety.ts).
This contradicts the repo's existing parity, which is explicitly tested in tests/router-javascript-urls.test.ts (push/replace/prefetch + Pages Router all throw BLOCK_MESSAGE) and tests/url-safety.test.ts. The form shim on main upheld this via isSafeAction; this PR drops it with no replacement on the navigation path.
Fix options:
- Have the form submit through the guarded router methods (matches Next.js exactly, which calls
router[method]), or - Re-add a dangerous-scheme guard in the submit path (keep
isDangerousScheme(effectiveAction)→reportBlockedDangerousNavigation()+ bail), while still allowing external/protocol-relative to hard-navigate.
Either way, please add a test covering a javascript: (and ideally data:) form action and submitter formAction, asserting it does not hard-navigate and emits the block message. The new tests/e2e/next-form-parity/form-actions.spec.ts covers external/protocol-relative but has no dangerous-scheme case, so this regression is currently untested.
Non-blocking notes
isPrefetchableActionnow resolvesactionHrefagainstwindow.location.hrefand gates prefetch to same-origin http(s). Good — but it referenceswindowunguarded; it's only called inside the productionuseEffectthat already returns in non-browser/dev, so it's fine in practice. Worth a one-line comment that it's browser-only.- Prefetch options flip to
cacheForNavigation: false+optimisticRouteShell: truewithrenderMode: APP_RSC_RENDER_MODE_PREFETCH_LOADING_SHELL. This matches the loading-shell seeding intent; the new shims.test.ts assertion only string-matches the source, so consider a behavioral test that the prefetched shell is actually consumed on the blocked-RSC navigation (the prod-only E2E may already cover this).
|
Review posted to PR #2245. SummaryI reviewed the One blocking issue found: The PR removes
I recommended either routing the form submit through the guarded router methods (exact Next.js match) or re-adding a dangerous-scheme guard in the submit path, plus a test for |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk re-review — fix(form): match Next.js navigation parity
Re-reviewed after b573f1b ("fix(form): block dangerous navigation schemes"), which addresses my prior blocking note.
Resolved: dangerous-scheme blocking restored ✅
b573f1b re-adds the guard via assertSafeNavigationUrl(url) in the App Router submit path (form.tsx:431), running it before navigateClientSide. This correctly blocks javascript:/data:/vbscript: (isDangerousScheme) while still letting external/protocol-relative URLs fall through to hard navigation — both parity goals preserved. The synchronous handleSubmit now lets the throw bubble into React's event runtime → console.error, matching the Next.js app-dir/javascript-urls assertion. Unit coverage for javascript:/data:/vbscript: form-level actions and data:/vbscript: submitter overrides is good.
Blocking: the new javascript-submitter-override E2E test contradicts the code path
tests/e2e/next-form-parity/form-actions.spec.ts:128-154 adds a javascript submitter formAction overrides case asserting (a) the block message appears in console.error, (b) the URL is unchanged, and (c) __VINEXT_FORM_JAVASCRIPT_SUBMITTER__ stays undefined. The fixture (app/forms/actions/page.tsx:69-76) uses formAction="javascript:globalThis.__VINEXT_FORM_JAVASCRIPT_SUBMITTER__=true".
But a javascript: submitter formAction never reaches assertSafeNavigationUrl. It is caught earlier by hasReactClientActionAttributes (form.tsx:406, regex /\s*javascript:/i) and bails silently at line 407 — before e.preventDefault() at line 423. So:
- No block message is emitted (
assertSafeNavigationUrlnever runs) → assertion (a) fails. preventDefault()is never called, so in a real browser the native form submission proceeds toformAction="javascript:..."and executes the script, setting the global → assertion (c) fails too.
This is internally inconsistent with the unit test the same commit keeps green: form.test.ts:549 (bails for React client-action submitters) explicitly asserts that a javascript: submitter formAction produces no error, no preventDefault(), and no navigation. And the new unit loop at form.test.ts:323 deliberately tests only ["data", "vbscript"] submitter overrides — javascript: is excluded precisely because it takes the silent client-action path. The E2E test asserts the opposite of the unit tests for the identical input.
Two things to reconcile:
-
The E2E test as written should fail. Please run
PLAYWRIGHT_PROJECT=next-form-parity-devand...-prodagainsttests/e2e/next-form-parity/form-actions.spec.ts— the PR's validation numbers predateb573f1b, so these new dangerous-scheme E2E cases (added in that commit) appear not to have been exercised. If they were green, I'd want to understand why, because the trace above says the marker should be set and no block message emitted. -
Decide the intended behavior and make unit + E2E agree. In real Next.js,
formAction="javascript:..."is a React-generated client action reference that React intercepts (thehasReactClientActionAttributesbail mirrorsform-shared.tsx). A raw author-writtenjavascript:formAction like this fixture is not a React action, so the native browser submit will execute it. If the goal is to actually block a rawjavascript:submitter override (defense-in-depth), the silentreturnatform.tsx:407needs toe.preventDefault()+ emit the block message instead of relying onassertSafeNavigationUrl(which is unreachable for this input). If the intent is to keep matching Next.js's client-action bail, then thejavascript-submitter-overrideE2E case should be dropped/rewritten to assert the silent-bail behavior the unit test already encodes — not the block message.
Non-blocking
isPrefetchableAction(form.tsx:68-78) referenceswindowunguarded; it's only invoked inside the produseEffectthat already returns in dev/non-browser, so it's safe in practice. A one-line "browser-only" comment would prevent future misuse.
|
Review posted to PR #2245. SummaryI re-reviewed the form-shim parity work after commit Resolved: The dangerous-scheme regression is fixed. One new blocking issue found: The new
This is internally inconsistent with the unit tests the same commit keeps green: I recommended running the |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk re-review — fix(form): match Next.js navigation parity
Re-reviewed after 472dd7a ("fix(form): address Pages navigation review"), which lands on top of the dangerous-scheme fix from b573f1b.
Both prior blocking issues are resolved ✅
-
Dangerous-scheme blocking restored.
assertSafeNavigationUrl(url)now runs in both submit paths before navigation: App Router atform.tsx:432(beforenavigateClientSide) and Pages Router atform.tsx:440(before the asyncimport("./router.js")IIFE). Traced the fixture cases end to end:action="javascript:..."→withBasePathleaves the scheme intact (isAbsoluteOrProtocolRelativeUrltrue) →createFormSubmitDestinationUrl→new URL("javascript:...", href)preservesjavascript:→assertSafeNavigationUrlthrows +console.error. ✓formAction="data:..."/vbscript:submitter overrides: opaque URLs keep their scheme even aftersearchParams.append(verifiedvbscript:dangerous?q=reactstill matchesisDangerousScheme's prefix regex). ✓- The throw is synchronous inside
handleSubmit, so it bubbles into React's event runtime →console.error, matching the Next.jsapp-dir/javascript-urlsassertion.
-
The contradictory
javascript-submitter-overrideE2E case is gone.472dd7aremoved both the E2E case (form-actions.spec.ts) and the fixture<Form>(actions/page.tsx) that asserted a block message for a rawjavascript:submitterformAction— which would actually hit the silenthasReactClientActionAttributesclient-action bail atform.tsx:407(nopreventDefault, native submit executes the URL). The remaining dangerous E2E cases (javascript:form action,data:submitter override) take paths that do reachassertSafeNavigationUrl, so unit (form.test.ts:704-730) and E2E now agree.
Verified locally
vp test run tests/form.test.ts— 38 passedvp test run tests/shims.test.ts -t "next/form shim"— 8 passedvp checkon all changed source/test/config/fixture files — clean (format, lint, types)- External / protocol-relative actions correctly hard-navigate:
navigateClientSide→isExternalUrl→toSameOriginAppPathreturns null →hardNavigateTo(navigation.ts:1608-1621), and the dangerous guard runs first so it can't be bypassed. - Pages Router refactor is sound:
assertSafeNavigationUrlis hoisted out of the async IIFE, and the import-failure fallback nowreturns before the Router push/replace branch, so a failed import can't double-navigate. The new unit loop asserts nopushState/replaceState/dispatch on a blocked scheme. - Playwright ports de-duped (
catch-error→ 4193,next-form-parity-dev→ 4192); no port collisions, and everybaseURLmatches its serverport. CI matrix adds bothnext-form-parity-dev/-prodprojects.
Non-blocking notes
-
next-form-parityfixture has nopackage.json(the only prod fixture without one). This forces the bespokestart-prod.shworkaround — symlinkingnode_modulesfromapp-basic, writingdist/package.jsonwith{"type":"module"}, andcp-ingindex.mjs→index.js/ssr/index.mjs→ssr/index.jsbecauseprod-server.tsresolvesdist/server/index.js(prod-server.ts:962) but the build emits.mjswithout"type":"module". Every other prod fixture (e.g.global-not-found-basic) ships its ownpackage.jsonwith"type":"module"and just runscli.js startdirectly. Adding a minimalpackage.json({"name":"next-form-parity-fixture","private":true,"type":"module"}) would let you delete the.mjs→.jscopy hack and thedist/package.jsonwrite, making the fixture consistent with the rest of the suite and less likely to break if the build output naming changes. -
shims.test.tsprefetch assertion is still source-string-only (expect(source).toContain("renderMode: APP_RSC_RENDER_MODE_PREFETCH_LOADING_SHELL")etc.). The behavioral coverage that the prefetched loading shell is actually consumed lives in the prod-onlyform-prefetch.spec.ts, which is the right place — just noting the unit assertion is brittle to refactors (renaming the option or reformatting the call would fail it without a behavior change). Fine as-is. -
isPrefetchableAction(form.tsx:68-79) referenceswindowunguarded; now documented with a "Browser-only" comment and only invoked inside the produseEffectthat returns early in dev/non-browser. Good.
LGTM once the two prior blocking issues are confirmed green in CI (the next-form-parity-dev/-prod E2E projects, which now run there). The fixture-package.json cleanup is optional but recommended.
|
Review posted to PR #2245. SummaryI re-reviewed the form-shim parity work after the latest commit Both prior blocking issues are now resolved:
Verified locally: form unit tests (38 passed), form shim tests (8 passed), Non-blocking findings:
No new blocking issues. LGTM pending the |
|
/bigbonk review for issues |
There was a problem hiding this comment.
/bigbonk re-review — fix(form): match Next.js navigation parity
Re-reviewed after the new commit e48f6fc ("fix(form): preserve native and hard navigation fallbacks"), which landed after the prior LGTM. It makes two behavioral changes; I verified both against Next.js v16.2.6 source and the test suite.
e48f6fc changes are correct ✅
-
e.preventDefault()moved to aftercreateFormSubmitDestinationUrl()(form.tsx:424-425). This matches upstreamapp-dir/form.tsxexactly: Next.js callscreateFormSubmitDestinationUrl(action, formElement)first, thenevent.preventDefault()under the comment "Finally, no more reasons for bailing out." So if URL construction throws (e.g.action="http://["),preventDefaultis never reached and the browser performs its native submission — correct progressive-enhancement parity. The new Pages Router unit cases (form.test.ts:743-763) assert exactly this: a malformed formaction/ submitterformActionthrows,preventDefaultis not called, and noassign/replace/pushState/replaceStatefires. The dangerous-scheme ordering is unaffected: forjavascript:/data:/vbscript:the URL constructs fine, sopreventDefaultruns and thenassertSafeNavigationUrlthrows — native submit is correctly suppressed (verifiednew URL("data:…"/"javascript:…", base)preserves the dangerous protocol). -
Pages Router import-failure fallback now hard-navigates instead of faking history (
form.tsx:441-461). The oldcatchdidhistory.pushState+ a syntheticpopstate+scrollTo; the new one doeswindow.location.assign/replace(url). This is the safer behavior — publishing a URL viahistoryalone without actually loading the destination leaves the page stale. I confirmed thiscatchwon't cause spurious hard reloads on normal navigation failures:Router.push/replaceresolve to"failed"/"cancelled"internally (runNavigateClientatrouter.ts:2178-2194catches and returns rather than rejecting, falling back towindow.location.hrefitself), so the form'scatchonly fires on genuinely thrown setup/import failures. The new parameterized test (form.test.ts:697-725) forcesRouter.push/replaceto throw and assertslocation.assign/location.replaceis called with the resolved URL and that nopushState/replaceStateleaks.
Both prior blocking issues remain resolved ✅
- Dangerous-scheme blocking still runs before navigation in both paths (
form.tsx:432App Router,form.tsx:440Pages Router). Thejavascript:form action anddata:submitter override in the fixture (actions/page.tsx:55-68) both reachassertSafeNavigationUrl(thedata:override is not caught by thejavascript:-onlyhasReactClientActionAttributesregex), matching the unit tests. - The contradictory
javascript:submitter-override E2E case stays removed, so unit + E2E agree.
Verified locally
vp test run tests/form.test.ts— 42 passed (was 38; +4 frome48f6fc)vp test run tests/shims.test.ts -t "next/form shim"— 8 passedvp check packages/vinext/src/shims/form.tsx tests/form.test.ts— clean (format, lint, types)- Playwright ports: no collisions —
catch-error→ 4193,pages-scroll-restorationowns the freed 4185,next-form-parity-dev→ 4192,next-form-parity-prod→ 4191; everybaseURLmatches its serverport. CI matrix adds bothnext-form-parity-{dev,prod}projects. form-prefetch.spec.tsheader constants match source:x-vinext-rsc-render-mode(Playwright lowercases theX-Vinext-Rsc-Render-Modeheader) and valueprefetch-loading-shell(APP_RSC_RENDER_MODE_PREFETCH_LOADING_SHELL).
Non-blocking notes (carried forward / new)
-
App Router malformed-URL native fallback is untested. The new
e48f6fcunit cases for the malformed-URL → no-preventDefaultfallback only cover the Pages Router branch (form.test.ts:743-763). The App Router branch shares the samecreateFormSubmitDestinationUrl→preventDefaultordering so it's structurally covered, but a symmetric App Router unit case (throw beforepreventDefault, nonavigateClientSide) would lock in the parity and guard against a future refactor that special-cases one branch. Minor. -
next-form-parityfixture still has nopackage.json(the only prod fixture without one), forcing the bespokestart-prod.shdist/package.jsonwrite +.mjs→.jscopy hack. Confirmed every other prod fixture (e.g.global-not-found-basic) ships apackage.jsonwith"type":"module"and runscli.js startdirectly. Adding{"name":"next-form-parity-fixture","private":true,"type":"module"}would delete the workaround and make the fixture consistent. Recommended cleanup, not blocking. -
shims.test.tsprefetch assertion is source-string-only — brittle to refactors but fine, since the behavior is covered by the prod-onlyform-prefetch.spec.ts.
No new blocking issues. The post-LGTM commit e48f6fc is a correct parity improvement. LGTM pending the next-form-parity-dev/-prod E2E projects going green in CI.
|
Review posted to PR #2245. SummaryI re-reviewed the form-shim parity work after the new commit
Both prior blocking issues remain resolved, and I verified locally:
No new blocking issues. Non-blocking notes: App Router malformed-URL fallback lacks a symmetric unit test, the fixture still lacks a |
1 participant
Summary
Match
next/formbehavior from Next.js v16.2.6 while preserving the existing basePath and viewport-prefetch work.Fixes three Next.js E2E parity failure themes:
formActionoverrides fall through to native hard navigation instead of being blocked; same-origin and relative actions continue to soft-navigate.loading.tsx;prefetch={false}and development do not prefetch.Also verifies basePath is applied exactly once and submitter
formActionis used verbatim.Validation
Targeted only, one worker where applicable:
vp test run tests/shims.test.ts -t "next/form shim" --maxWorkers=1— 8 passedvp check packages/vinext/src/shims/form.tsx tests/shims.test.ts playwright.config.ts tests/e2e/next-form-parity tests/fixtures/next-form-parity— passedPLAYWRIGHT_PROJECT=next-form-parity-dev vp exec playwright test tests/e2e/next-form-parity --workers=1— 10 passed, 1 production-only skippedPLAYWRIGHT_PROJECT=next-form-parity-prod vp exec playwright test tests/e2e/next-form-parity --workers=1— 10 passed, 1 development-only skippedNo broad test suites were run.