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

Next.js should not start on "bad ports" #55050

Closed
1 task done
dandrei opened this issue Sep 6, 2023 · 7 comments · Fixed by #55237
Closed
1 task done

Next.js should not start on "bad ports" #55050

dandrei opened this issue Sep 6, 2023 · 7 comments · Fixed by #55237
Labels
bug Issue was opened via the bug report template. good first issue Easy to fix issues, good for newcomers Image (next/image) Related to Next.js Image Optimization. linear: next Confirmed issue that is tracked by the Next.js team. locked

Comments

@dandrei
Copy link

dandrei commented Sep 6, 2023

Link to the code that reproduces this issue or a replay of the bug

https://codesandbox.io/p/sandbox/stupefied-parm-nkr2wn

To Reproduce

  1. A Next.js application in production (npm run start) on a "bad port" (as defined by the fetch standard, e.g. 4045).
  2. Access a page that uses a "next/image" component.

Current vs. Expected behavior

The demonstrative example showcases the loading of a "next/image" component .
It loads fine in development mode, but fails in production when running on a "bad port":

  • the page displays a broken image
  • the output from next start will display a stacktrace like:
upstream image response failed for /_next/static/media/test.37141d1a.png TypeError: fetch failed
    at Object.fetch (node_modules/next/dist/compiled/undici/index.js:1:26669)
    at async invokeRequest (node_modules/next/dist/server/lib/server-ipc/invoke-request.js:17:12)
    at async node_modules/next/dist/server/next-server.js:359:35
    at async imageOptimizer (node_modules/next/dist/server/image-optimizer.js:537:13)
    at async cacheEntry.imageResponseCache.get.incrementalCache (node_modules/next/dist/server/next-server.js:598:60)
    at async node_modules/next/dist/server/response-cache/index.js:99:36 {
  cause: Error: bad port
      at makeNetworkError (node_modules/next/dist/compiled/undici/index.js:2:55255)
      at mainFetch (node_modules/next/dist/compiled/undici/index.js:2:29022)
      at fetching (node_modules/next/dist/compiled/undici/index.js:2:28803)
      at fetch (node_modules/next/dist/compiled/undici/index.js:2:26721)
      at Object.fetch (node_modules/next/dist/compiled/undici/index.js:1:26638)
      at globalThis.fetch (node_modules/next/dist/server/node-polyfill-fetch.js:30:26)
      at invokeRequest (node_modules/next/dist/server/lib/server-ipc/invoke-request.js:17:18)
      at node_modules/next/dist/server/next-server.js:359:74
      at imageOptimizer (node_modules/next/dist/server/image-optimizer.js:537:19)
      at NextNodeServer.imageOptimizer (node_modules/next/dist/server/next-server.js:354:16)

The stacktrace explains what's going on:

  • The image optimizer attempts to fetch the image for optimization, using the undici implementation of fetch.
  • Because the server is running on a "bad port", undici throws an error.

The result is a production deployment that should work fine, but none of the images load, because the optimizer crashes when attempting to fetch them.

Here's how the fetch standard defines a "bad port":
https://fetch.spec.whatwg.org/#bad-port

One possible solution could be to mirror Undici's list of "bad ports", and raise it as an issue when running Next.js in production.

Verify canary release

  • I verified that the issue exists in the latest Next.js canary release

Provide environment information

Operating System:
      Platform: linux
      Arch: x64
      Version: #1 SMP PREEMPT_DYNAMIC Sun Aug  6 20:05:33 UTC 2023
    Binaries:
      Node: 16.17.0
      npm: 8.15.0
      Yarn: 1.22.19
      pnpm: 7.1.0
    Relevant Packages:
      next: 13.4.20-canary.18
      eslint-config-next: N/A
      react: 18.2.0
      react-dom: 18.2.0
      typescript: 5.2.2
    Next.js Config:
      output: N/A

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

Image optimization (next/image, next/legacy/image)

Additional context

No response

NEXT-1608

@dandrei dandrei added the bug Issue was opened via the bug report template. label Sep 6, 2023
@github-actions github-actions bot added the Image (next/image) Related to Next.js Image Optimization. label Sep 6, 2023
@balazsorban44
Copy link
Member

One possible solution could be to mirror Undici's list of "bad ports", and raise it as an issue when running Next.js in production.

Isn't this what's happening already? You are showing the stack trace that Next.js is outputting.

On the client, we don't want to expose more information than necessary. Currently, you should be seeing something like "url" parameter is valid but upstream response is invalid as the response.

@dandrei
Copy link
Author

dandrei commented Sep 6, 2023

In this particular case, I think it would be justified to have Next.js warn users about running on certain ports that are certain to crash in production, otherwise the user has to debug an issue that's not even technical, but originates in an arbitrary constraint imposed by a dependency, which is itself an implementation detail.

@styfle
Copy link
Member

styfle commented Sep 7, 2023

This isn't just an implementation detail of undici. All browsers block these ports too (I tested Chrome, Firefox, Safari).

Next.js should probably just hard error as soon as you try to start with one of those ports since no one will be able to visit your web app.

Do you want to create a PR?

@dandrei
Copy link
Author

dandrei commented Sep 7, 2023

In production you wouldn't usually run the website on these unusual ports directly. Instead you'd have it run on localhost[:port], and then get Apache or nginx to proxy it. So in theory, it wouldn't necessarily be an issue, as visitors to the website would access it on the usual ports (80, 443).

But since it appears to be a wider web standard that these ports shouldn't be accessed by web clients, it adds more weight to having Next.js refuse to serve on them.

I am not familiar enough with the codebase to create a PR for warning users against starting Next.js on a "bad port".

@balazsorban44 balazsorban44 added good first issue Easy to fix issues, good for newcomers linear: next Confirmed issue that is tracked by the Next.js team. labels Sep 11, 2023
@balazsorban44
Copy link
Member

balazsorban44 commented Sep 11, 2023

You could add a similar check to exit the next start process like it's done here:

if (
typeof keepAliveTimeoutArg !== 'undefined' &&
(Number.isNaN(keepAliveTimeoutArg) ||
!Number.isFinite(keepAliveTimeoutArg) ||
keepAliveTimeoutArg < 0)
) {
printAndExit(
`Invalid --keepAliveTimeout, expected a non negative number but received "${keepAliveTimeoutArg}"`,
1
)
}

Same for next dev here:

// Check if pages dir exists and warn if not
if (!(await fileExists(dir, FileType.Directory))) {
printAndExit(`> No such directory exists as the project root: ${dir}`)
}

@balazsorban44 balazsorban44 changed the title Next.js image optimizer bug (caused by undici) when running on a "bad port" in production Next.js should not start on "bad ports" Sep 11, 2023
@olingern
Copy link
Contributor

👋 I opened a PR here for just the next start command and wanted to get feedback on approach / testing before moving on to next dev

olingern added a commit to olingern/next.js that referenced this issue Sep 11, 2023
olingern added a commit to olingern/next.js that referenced this issue Sep 11, 2023
olingern added a commit to olingern/next.js that referenced this issue Sep 12, 2023
olingern added a commit to olingern/next.js that referenced this issue Sep 12, 2023
olingern added a commit to olingern/next.js that referenced this issue Sep 12, 2023
olingern added a commit to olingern/next.js that referenced this issue Sep 12, 2023
@kodiakhq kodiakhq bot closed this as completed in #55237 Sep 12, 2023
kodiakhq bot pushed a commit that referenced this issue Sep 12, 2023
### Fixing a bug

- [x] Related issues linked using `fixes #number`
- [x] Tests added. See: https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs
- [x] Errors have a helpful link attached, see https://github.com/vercel/next.js/blob/canary/contributing.md

Closes NEXT-
Fixes #55050 


Co-authored-by: Steven <[email protected]>
Co-authored-by: Tim Neutkens <[email protected]>
@github-actions
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 Sep 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue was opened via the bug report template. good first issue Easy to fix issues, good for newcomers Image (next/image) Related to Next.js Image Optimization. linear: next Confirmed issue that is tracked by the Next.js team. locked
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants