-
-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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): replace changeOrigin changes in 5.3.0 with new rewriteWsOrigin option #17563
base: main
Are you sure you want to change the base?
Conversation
- Revert changeOrigin behaviour - Introduce rewriteWsOrigin option - Only use rewriteWsOrigin in ws requests - Update docs
- Check for headersSent before rewriting Origin - Log warning if fails
|
Maybe a |
I haven't seen a use case for that yet, and given rewriting the Origin has security implications I'd prefer to not implement. If a need arises later we could revisit it. |
I have seen this use case: Due to the diversity in HTTP implementation, compared to WebSocket, HTTP has more use cases where Origin checking is necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Would also like @sapphi-red's eye on this in case I miss something
@pfdgithub I'm leaning towards fixing the regression first and we could revisit if we need rewriteHttpOrigin
later. Since this introduces a new option, we'd need the team's opinion on this first, and introducing 2 new options may be more complicated.
The 'rewriteHttpOrigin' option is not required, there are other ways to implement it. |
Description
fixes #17562
fixes #17520
This PR reverts the change to the changeOrigin option released in 5.3.0 and adds an alternative solution:
rewriteWsOrigin
option which rewrites the Origin header for WebSocket requests onlyheadersSent
prior to rewriting, logging a warning if it does not rewrite