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/server proxies middleware rewrites if not served from localhost #31179

Closed
ascorbic opened this issue Nov 8, 2021 · 1 comment · Fixed by #31180
Closed

next/server proxies middleware rewrites if not served from localhost #31179

ascorbic opened this issue Nov 8, 2021 · 1 comment · Fixed by #31180
Assignees
Labels
bug Issue was opened via the bug report template. Middleware Related to Next.js Middleware.

Comments

@ascorbic
Copy link
Contributor

ascorbic commented Nov 8, 2021

What version of Next.js are you using?

12.0.4-canary.0

What version of Node.js are you using?

16.11.0

What browser are you using?

Firefox

What operating system are you using?

macOS

How are you deploying your application?

next start

Describe the Bug

When returning NextResponse.rewrite from middleware, next/server determines whether to proxy the request by checking if the parsed rewrite URL has a protocol. When running on localhost this will be false, so it will handle the rewrite as expected. However if running with a hostname, such as if using mycomputername.local:3000, using any other value for localhost (such as 127.0.0.1), or running a live server, then the parsed URL will have a protocol, so the server will attempt to proxy to the address. This will result in either an ssl error or a proxy loop.

I am happy to submit a PR to fix this. It can be fixed by also checking if the rewrite host matches the request host.

Expected Behavior

Server does not attempt to proxy local rewrites, even when running with a hostname.

To Reproduce

  • Clone the geolocation demo
  • Run locally
  • Load using your computer's hostname, or localtest.me (which resolves to localhost)
@ascorbic ascorbic added the bug Issue was opened via the bug report template. label Nov 8, 2021
@timneutkens timneutkens added the Middleware Related to Next.js Middleware. label Nov 9, 2021
@Kikobeats Kikobeats self-assigned this Nov 12, 2021
@kodiakhq kodiakhq bot closed this as completed in #31180 Nov 13, 2021
kodiakhq bot pushed a commit that referenced this issue Nov 13, 2021
This changes the check for whether a rewrite is internal or if it should be proxied. Currently it checks if `protocol` is unset, which is only the case for relative URLs or localhost. This means that requests where there is a hostname set, or if localhost is specified in another way such as `127.0.0.1`, then it will be proxied, which potentially causes a proxy loop or ssl error. This PR changes the test so that it also checks if the hosts match, and only proxies if they are different.

Fixes #31179 

## Bug

- [x] Related issues linked using `fixes #number`
- [x] Integration tests added
- [ ] Errors have helpful link attached, see `contributing.md`
@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 27, 2022
natew pushed a commit to natew/next.js that referenced this issue Feb 16, 2022
This changes the check for whether a rewrite is internal or if it should be proxied. Currently it checks if `protocol` is unset, which is only the case for relative URLs or localhost. This means that requests where there is a hostname set, or if localhost is specified in another way such as `127.0.0.1`, then it will be proxied, which potentially causes a proxy loop or ssl error. This PR changes the test so that it also checks if the hosts match, and only proxies if they are different.

Fixes vercel#31179 

## Bug

- [x] Related issues linked using `fixes #number`
- [x] Integration tests added
- [ ] Errors have helpful link attached, see `contributing.md`
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. Middleware Related to Next.js Middleware.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants