-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
Conversation
Run & review this pull request in StackBlitz Codeflow. |
Moving to draft while I investigate why http-proxy isn't doing the chnage. |
This is misleadingly named in http-proxy
Thank you for the PR and digging down what So first, I'd like to ask some questions about that.
|
Thanks @sapphi-red I'll look into it. |
@sapphi-red excellent points, answers below:
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
I looked in to this - they originally set both 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
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
|
Rewrites the header for both ws and http requests
Thanks for checking! We can introduce a different option like |
Thats a great idea! Happy to revisit if we get issues. |
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.
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... |
Thanks for the response and explanation @tryforceful. I think the best approach is to:
I'll raise an issue to track |
Issue and PR raised #17563 |
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 assumingchangeOrigin:true
means change the origin header.