Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incomplete URL in next/link crashes the app in production #66650

Closed
agurtovoy opened this issue Jun 7, 2024 · 2 comments · Fixed by #66755
Closed

Incomplete URL in next/link crashes the app in production #66650

agurtovoy opened this issue Jun 7, 2024 · 2 comments · Fixed by #66755
Labels
area: app App directory (appDir: true) bug Issue was opened via the bug report template. locked Navigation Related to Next.js linking (e.g., <Link>) and navigation. Runtime Related to Node.js or Edge Runtime with Next.js.

Comments

@agurtovoy
Copy link

agurtovoy commented Jun 7, 2024

Link to the code that reproduces this issue

https://github.com/agurtovoy/nextjs-incomplete-link-bug

To Reproduce

  1. Deploy the repo above to Vercel
  2. Open the deployed site, get Application error: a client-side exception has occurred (see the browser console for more information). (sample deployment)

Current vs. Expected behavior

Current: passing incomplete/invalid URL to next/link throws an exception on render (in production) even if the user never clicks on the link.

Expected: providing incomplete/invalid URL to next/link should never lead to an exception during rendering.

Provide environment information

Operating System:
  Platform: darwin
  Arch: arm64
  Version: Darwin Kernel Version 23.5.0: Wed May  1 20:12:58 PDT 2024; root:xnu-10063.121.3~5/RELEASE_ARM64_T6000
  Available memory (MB): 16384
  Available CPU cores: 10
Binaries:
  Node: 18.20.3
  npm: 10.7.0
  Yarn: 1.22.21
  pnpm: 7.26.3
Relevant Packages:
  next: 14.2.3 // Latest available version is detected (14.2.3).
  eslint-config-next: 14.2.3
  react: 18.3.1
  react-dom: 18.3.1
  typescript: 5.4.5
Next.js Config:
  output: N/A

Which area(s) are affected? (Select all that apply)

Navigation, Runtime

Which stage(s) are affected? (Select all that apply)

Vercel (Deployed), Other (Deployed)

Additional context

The offending exception is thrown by the URL constructor here (while preparing to prefetch the URL):

const url = new URL(addBasePath(href), window.location.href)

While the URL constructor is pretty tolerant and in most cases will obediently return some sort of URL, even if you pass it arguably nonsensical args, it does throw in a couple of circumstances. One of these circumstances is an attempt to mix protocols, e.g.: new URL('http:', 'https://vercel.com').

That means something as innocent as <Link href="http:">Home</Link> will currently throw on render in production.

Note that because prefetching is unconditionally disabled in dev, it's easy to ship a broken site w/o knowing it.

Also note that there is other prefetching code elsewhere in the codebase (e.g. page router) where the assumption seems to be that the URL constructor doesn't throw.

@agurtovoy agurtovoy added the bug Issue was opened via the bug report template. label Jun 7, 2024
@github-actions github-actions bot added Navigation Related to Next.js linking (e.g., <Link>) and navigation. Runtime Related to Node.js or Edge Runtime with Next.js. labels Jun 7, 2024
@lubieowoce lubieowoce added the area: app App directory (appDir: true) label Jun 7, 2024
@prabal-goyal

This comment has been minimized.

lubieowoce added a commit that referenced this issue Jun 11, 2024
…66755)

Closes [#66650](#66650)
Closes NEXT-3520

### What?

- Make Link not throw during prefetch if it received an invalid `href`
(see [#66650](#66650))
- Throw in dev mode if an invalid link was passed to `router.prefetch`
-- this matches current prod behavior
- (previously, we'd immediately exit out of `router.prefetch`, so the
user would see no indication that this'd fail in prod)

### Why?

If an invalid URL was passed to `<Link>`, the whole app would crash and
show "A client-side exception occurred". A failed prefetch should not
bring down the whole app.

Note that This preserves the current behavior of explicit
`router.prefetch(url)` throwing an error if `url` is invalid. We may
want to adjust this in the future, but that could be considered a
breaking change, so I'm leaving it out for now. This only affects
`Link`, which was intended to catch any errors thrown from
`router.prefetch`, but that bit was accidentally broken.
lubieowoce added a commit that referenced this issue Jun 11, 2024
…66755)

Closes [#66650](#66650)
Closes NEXT-3520

- Make Link not throw during prefetch if it received an invalid `href`
(see [#66650](#66650))
- Throw in dev mode if an invalid link was passed to `router.prefetch`
-- this matches current prod behavior
- (previously, we'd immediately exit out of `router.prefetch`, so the
user would see no indication that this'd fail in prod)

If an invalid URL was passed to `<Link>`, the whole app would crash and
show "A client-side exception occurred". A failed prefetch should not
bring down the whole app.

Note that This preserves the current behavior of explicit
`router.prefetch(url)` throwing an error if `url` is invalid. We may
want to adjust this in the future, but that could be considered a
breaking change, so I'm leaving it out for now. This only affects
`Link`, which was intended to catch any errors thrown from
`router.prefetch`, but that bit was accidentally broken.
Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for 2 weeks. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: app App directory (appDir: true) bug Issue was opened via the bug report template. locked Navigation Related to Next.js linking (e.g., <Link>) and navigation. Runtime Related to Node.js or Edge Runtime with Next.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants