fix(pages-router): throw on javascript: URL synchronously in router.push/replace

packages/vinext/src/shims/router.ts

@@ -891,14 +891,13 @@ async function performNavigation(
     throwNoRouterInstance();
   }
 
-  // Block dangerous URI schemes (javascript:, data:, vbscript:) before any
-  // navigation work happens. Mirrors Next.js's Pages Router guard at
-  // packages/next/src/shared/lib/router/router.ts:1020-1028,1052-1060, which
-  // throws and (via React's event-handler runtime) surfaces a console.error
-  // that the `test/e2e/app-dir/javascript-urls/javascript-urls.test.ts` suite
-  // asserts on. `assertSafeNavigationUrl` emits the matching console.error
-  // before throwing so the same observable behaviour holds when the throw is
-  // swallowed by an async event handler (e.g. Link's click delegation).
+  // Defence-in-depth dangerous-scheme guard. The synchronous guard inside
+  // `Router.push` / `Router.replace` (see RouterMethods below) is the primary
+  // line of defence and is what surfaces the matching console.error to React's
+  // event-handler runtime. This inner guard catches any future call sites
+  // that bypass the public Router methods and call `performNavigation`
+  // directly. Mirrors Next.js's Pages Router check at
+  // packages/next/src/shared/lib/router/router.ts:1025-1033,1057-1065.
   assertSafeNavigationUrl(resolveUrl(url));
   if (as !== undefined) {
     assertSafeNavigationUrl(String(as));
@@ -1200,10 +1199,28 @@ export function withRouter<P extends WithRouterProps>(
 const RouterMethods = {
   push: (url: string | UrlObject, as?: string, options?: TransitionOptions) => {
     if (typeof window === "undefined") throwNoRouterInstance();
+    // Synchronously guard dangerous URI schemes (javascript:, data:, vbscript:)
+    // before the async performNavigation kicks off. Mirrors Next.js's
+    // Pages Router `push` at packages/next/src/shared/lib/router/router.ts:1025-1033,
+    // where the check runs synchronously inside push() so the throw bubbles up
+    // through React's event-handler error reporter (surfacing console.error).
+    // Without this synchronous hoist, the throw inside `performNavigation`
+    // (an async function) becomes a rejected Promise that React does not
+    // observe from an event handler that does not await it (e.g.
+    // `<button onClick={() => router.push(...)}>`).
+    assertSafeNavigationUrl(resolveUrl(url));
+    if (as !== undefined) {
+      assertSafeNavigationUrl(String(as));
+    }
     return performNavigation(url, as, options, "push");
   },
   replace: (url: string | UrlObject, as?: string, options?: TransitionOptions) => {
     if (typeof window === "undefined") throwNoRouterInstance();
+    // See `push` above for the rationale on the synchronous guard.
+    assertSafeNavigationUrl(resolveUrl(url));
+    if (as !== undefined) {
+      assertSafeNavigationUrl(String(as));
+    }
     return performNavigation(url, as, options, "replace");
   },
   back: () => {

tests/router-javascript-urls.test.ts

@@ -142,7 +142,9 @@ describe("App Router appRouterInstance emits console.error on dangerous URI sche
 // Pages Router parity: next/router push/replace must also block dangerous
 // URI schemes with both a throw and a matching console.error. Mirrors
 // Next.js's `packages/next/src/shared/lib/router/router.ts:1020,1052`, where
-// push/replace throw the same error message.
+// push/replace throw the same error message *synchronously* (before any
+// async work) so React's event-handler error reporter can surface the
+// console.error to the user.
 describe("Pages Router next/router blocks dangerous URI schemes", () => {
   // Minimal fake window/document so importing shims/router.ts (which touches
   // window at module load to attach popstate) does not crash. Installed by
@@ -175,91 +177,123 @@ describe("Pages Router next/router blocks dangerous URI schemes", () => {
     vi.resetModules();
   });
 
-  it("Router.push rejects on javascript: URL", async () => {
+  it("Router.push throws synchronously on javascript: URL", async () => {
     const restoreBrowserGlobals = installFakeBrowserGlobals();
     const consoleError = vi.spyOn(console, "error").mockImplementation(() => {});
     try {
       vi.resetModules();
       const routerModule = await import("../packages/vinext/src/shims/router.js");
       const Router = routerModule.default;
-      await expect(Router.push("javascript:alert(1)")).rejects.toThrow(BLOCK_MESSAGE);
+      // Next.js's Pages Router `push` throws synchronously inside the push()
+      // call so React's event-handler error reporter can surface
+      // `console.error`. Mirror that here (do not await the call).
+      expect(() => Router.push("javascript:alert(1)")).toThrow(BLOCK_MESSAGE);
       expect(consoleError).toHaveBeenCalledWith(BLOCK_MESSAGE);
     } finally {
       consoleError.mockRestore();
       restoreBrowserGlobals();
     }
   });
 
-  it("Router.replace rejects on javascript: URL", async () => {
+  it("Router.replace throws synchronously on javascript: URL", async () => {
     const restoreBrowserGlobals = installFakeBrowserGlobals();
     const consoleError = vi.spyOn(console, "error").mockImplementation(() => {});
     try {
       vi.resetModules();
       const routerModule = await import("../packages/vinext/src/shims/router.js");
       const Router = routerModule.default;
-      await expect(Router.replace("javascript:alert(1)")).rejects.toThrow(BLOCK_MESSAGE);
+      expect(() => Router.replace("javascript:alert(1)")).toThrow(BLOCK_MESSAGE);
       expect(consoleError).toHaveBeenCalledWith(BLOCK_MESSAGE);
     } finally {
       consoleError.mockRestore();
       restoreBrowserGlobals();
     }
   });
 
-  it("Router.push rejects when only `as` is a javascript: URL", async () => {
+  it("Router.push throws synchronously when only `as` is a javascript: URL", async () => {
     const restoreBrowserGlobals = installFakeBrowserGlobals();
     const consoleError = vi.spyOn(console, "error").mockImplementation(() => {});
     try {
       vi.resetModules();
       const routerModule = await import("../packages/vinext/src/shims/router.js");
       const Router = routerModule.default;
-      await expect(Router.push("/safe", "javascript:alert(1)")).rejects.toThrow(BLOCK_MESSAGE);
+      expect(() => Router.push("/safe", "javascript:alert(1)")).toThrow(BLOCK_MESSAGE);
       expect(consoleError).toHaveBeenCalledWith(BLOCK_MESSAGE);
     } finally {
       consoleError.mockRestore();
       restoreBrowserGlobals();
     }
   });
 
-  it("Router.replace rejects when only `as` is a javascript: URL", async () => {
+  it("Router.replace throws synchronously when only `as` is a javascript: URL", async () => {
     const restoreBrowserGlobals = installFakeBrowserGlobals();
     const consoleError = vi.spyOn(console, "error").mockImplementation(() => {});
     try {
       vi.resetModules();
       const routerModule = await import("../packages/vinext/src/shims/router.js");
       const Router = routerModule.default;
-      await expect(Router.replace("/safe", "javascript:alert(1)")).rejects.toThrow(BLOCK_MESSAGE);
+      expect(() => Router.replace("/safe", "javascript:alert(1)")).toThrow(BLOCK_MESSAGE);
       expect(consoleError).toHaveBeenCalledWith(BLOCK_MESSAGE);
     } finally {
       consoleError.mockRestore();
       restoreBrowserGlobals();
     }
   });
 
-  it("Router.push rejects on uppercase JAVASCRIPT: URL", async () => {
+  it("Router.push throws synchronously on uppercase JAVASCRIPT: URL", async () => {
     const restoreBrowserGlobals = installFakeBrowserGlobals();
     const consoleError = vi.spyOn(console, "error").mockImplementation(() => {});
     try {
       vi.resetModules();
       const routerModule = await import("../packages/vinext/src/shims/router.js");
       const Router = routerModule.default;
-      await expect(Router.push("JAVASCRIPT:alert(1)")).rejects.toThrow(BLOCK_MESSAGE);
+      expect(() => Router.push("JAVASCRIPT:alert(1)")).toThrow(BLOCK_MESSAGE);
       expect(consoleError).toHaveBeenCalledWith(BLOCK_MESSAGE);
     } finally {
       consoleError.mockRestore();
       restoreBrowserGlobals();
     }
   });
 
-  it("Router.push rejects on data: URL", async () => {
+  it("Router.push throws synchronously on data: URL", async () => {
     const restoreBrowserGlobals = installFakeBrowserGlobals();
     const consoleError = vi.spyOn(console, "error").mockImplementation(() => {});
     try {
       vi.resetModules();
       const routerModule = await import("../packages/vinext/src/shims/router.js");
       const Router = routerModule.default;
-      await expect(Router.push("data:text/html,<script>alert(1)</script>")).rejects.toThrow(
-        BLOCK_MESSAGE,
-      );
+      expect(() => Router.push("data:text/html,<script>alert(1)</script>")).toThrow(BLOCK_MESSAGE);
+      expect(consoleError).toHaveBeenCalledWith(BLOCK_MESSAGE);
+    } finally {
+      consoleError.mockRestore();
+      restoreBrowserGlobals();
+    }
+  });
+
+  // Regression coverage: `Router.push(...)` must throw *before* returning the
+  // Promise. If it ever regresses to throwing inside the async
+  // `performNavigation` body, the throw would be wrapped in a rejected
+  // Promise that React's event-handler runtime does not observe, and the
+  // matching `console.error` would not be reported through page logs (the
+  // failure mode covered by Next.js's
+  // `test/e2e/app-dir/javascript-urls/javascript-urls.test.ts:341,376`).
+  it("Router.push throws synchronously (does not return a rejected Promise)", async () => {
+    const restoreBrowserGlobals = installFakeBrowserGlobals();
+    const consoleError = vi.spyOn(console, "error").mockImplementation(() => {});
+    try {
+      vi.resetModules();
+      const routerModule = await import("../packages/vinext/src/shims/router.js");
+      const Router = routerModule.default;
+      let threw = false;
+      let returned: unknown = "not-called";
+      try {
+        returned = Router.push("javascript:alert(1)");
+      } catch {
+        threw = true;
+      }
+      expect(threw).toBe(true);
+      // The synchronous throw means no value was ever returned.
+      expect(returned).toBe("not-called");
       expect(consoleError).toHaveBeenCalledWith(BLOCK_MESSAGE);
     } finally {
       consoleError.mockRestore();