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

fix(proxy): rewrite the origin header to match the target for ws proxy #16558

Merged
merged 4 commits into from
May 29, 2024

Conversation

johnhunter
Copy link
Contributor

@johnhunter johnhunter commented Apr 29, 2024

Description

fixes #16557

For ws proxying, this PR updates the request Origin header to match the target Host value. This resolves an issue where proxying to an RFC 6455 compliant server causes a 403 to be returned when request Origin and Host do not match.

I didn't see any existing tests for the proxy module but can look at adding coverage if its a blocker to merging.

Update:

The PR also updates docs to clarify the actual effect of the changeOrigin option. See http-party/node-http-proxy#1130 which was raised in 2017 and has not been merged due to ongoing discussion about whether the option should be removed altogether. I believe clarifying this for Vite users will help them avoid assuming changeOrigin:true means change the origin header.

Copy link

stackblitz bot commented Apr 29, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@johnhunter johnhunter changed the title Rewrite the origin header to match the target for ws proxy fix: Rewrite the origin header to match the target for ws proxy Apr 29, 2024
@johnhunter johnhunter changed the title fix: Rewrite the origin header to match the target for ws proxy fix: rewrite the origin header to match the target for ws proxy Apr 29, 2024
@johnhunter johnhunter changed the title fix: rewrite the origin header to match the target for ws proxy fix(proxy): rewrite the origin header to match the target for ws proxy Apr 29, 2024
@johnhunter
Copy link
Contributor Author

Moving to draft while I investigate why http-proxy isn't doing the chnage.

@johnhunter johnhunter marked this pull request as draft April 30, 2024 10:02
This is misleadingly named in http-proxy
@johnhunter johnhunter marked this pull request as ready for review April 30, 2024 12:57
@sapphi-red
Copy link
Member

Thank you for the PR and digging down what changeOrigin option does! I agree it'd great if Vite sets the origin header OOTB. But I'm afraid of breaking existing setups by setting it by default.

So first, I'd like to ask some questions about that.

  1. Do you think there's a case where users want to skip setting the origin header?
  2. Do you know why http-proxy sets host header instead of origin header?
  3. I wonder if we should set origin header for non-ws requests as well. I guess it's useful for servers with origin header checks for CSRF prevention. Do you think there's any concerns about doing it?

@johnhunter
Copy link
Contributor Author

Thanks @sapphi-red I'll look into it.

@johnhunter
Copy link
Contributor Author

johnhunter commented May 14, 2024

@sapphi-red excellent points, answers below:

  1. Do you think there's a case where users want to skip setting the origin header?

I can't think of a case for skipping - certainly not with vite proxy as it is only used in local dev / preview environments. There may be other proxy/reverse-proxy use cases that would be relevant to the http-proxy project. I think it should be switched with the existing changeOrigin option, which will hopefully become clear...

  1. Do you know why http-proxy sets host header instead of origin header?

I looked in to this - they originally set both Host and Origin headers when the changeOrigin option was true. The option was removed entirely in a refactor and then reimplemented but with only the Host change. I've asked if that was an intentional change node-http-proxy#1669 but I suspect it was an unintended regression.

I'll look at raising a PR on http-proxy to re-instate the original (and preferred) behaviour. However I am not confident that maintainers have the bandwidth to review, merge and release such a change.

Therefore what I will do is update this vite PR to honour the original http-proxy behaviour and change the Origin header when changeOrigin is true.

  1. I wonder if we should set origin header for non-ws requests as well. I guess it's useful for servers with origin header checks for CSRF prevention. Do you think there's any concerns about doing it?

Given that http-proxy originally applied the change to http requests as well I think it would make sense to do the same.

While updating the changeOrigin behaviour to change Origin as well as Host is a change to vite proxy, I think it is more in line with vite users expectations than the current situation. If changing the Origin header is an issue for vite users then I would expect they already had issues with the current Host header change and so they would have set changeOrigin:false.

I'll set the PR back to draft until I've made the outlined changes Ready for review.

@johnhunter johnhunter marked this pull request as draft May 14, 2024 16:24
@johnhunter johnhunter marked this pull request as draft May 14, 2024 16:24
Rewrites the header for both ws and http requests
@johnhunter johnhunter marked this pull request as ready for review May 14, 2024 17:18
@sapphi-red
Copy link
Member

Thanks for checking!

We can introduce a different option like changeOrigin: 'both' to be really safe. But I think we can try this out without that in the next minor beta version. If it breaks, let's try adding an option.

@sapphi-red sapphi-red added has workaround p2-edge-case Bug, but has workaround or limited in scope (priority) labels May 16, 2024
@sapphi-red sapphi-red added this to the 5.3 milestone May 16, 2024
@johnhunter
Copy link
Contributor Author

We can introduce a different option like changeOrigin: 'both' to be really safe. But I think we can try this out without that in the next minor beta version. If it breaks, let's try adding an option.

Thats a great idea! Happy to revisit if we get issues.

@bluwy bluwy merged commit 7b0a65e into vitejs:main May 29, 2024
11 checks passed
@tryforceful
Copy link

This has broken my app's proxying behavior for the reasons you suspected above.

I am not using WS but I need my host header rewritten without my origin header rewritten, or else my CI fails.

Origin vs. Host Headers: The Key Difference

Host:

  • Tells the server which website/application is being requested.
  • Used for virtual hosting (multiple sites on one IP address).
  • Modified by the proxy when changeOrigin: true to match the target backend.

Origin:

  • Indicates the origin (protocol, domain, port) where the request initiated.
  • Primarily used for Cross-Origin Resource Sharing (CORS) security.
  • Not typically modified by proxies, even with changeOrigin: true. This is a browser security mechanism.

This change seems to leave the proxying open to CSRF attacks.

Perhaps it is wanted in a WS context, but I need a way to turn off the origin reassignment.

I'd prefer to have two options (as mentioned above) if you really need to maintain this behavior... rewriteHost and rewriteOrigin, with a warning on the latter

@johnhunter
Copy link
Contributor Author

Thanks for the response and explanation @tryforceful. I think the best approach is to:

  • revert the changeOrigin behaviour to the original
  • add a new option rewriteWsOrigin that rewrites the Origin header for ws requests only

I'll raise an issue to track

@johnhunter
Copy link
Contributor Author

Issue and PR raised #17563

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has workaround p2-edge-case Bug, but has workaround or limited in scope (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Forwarded Origin header not updated on proxied WebSocket requests
5 participants