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): replace changeOrigin changes in 5.3.0 with new rewriteWsOrigin option #17563

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

johnhunter
Copy link
Contributor

@johnhunter johnhunter commented Jun 25, 2024

Description

fixes #17562
fixes #17520

This PR reverts the change to the changeOrigin option released in 5.3.0 and adds an alternative solution:

  • A new rewriteWsOrigin option which rewrites the Origin header for WebSocket requests only
  • Check for headersSent prior to rewriting, logging a warning if it does not rewrite
  • Update docs for new option and warns about the security implications

- Revert changeOrigin behaviour
- Introduce rewriteWsOrigin option
- Only use rewriteWsOrigin in ws requests
- Update docs
- Check for headersSent before rewriting Origin
- Log warning if fails
Copy link

stackblitz bot commented Jun 25, 2024

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

@johnhunter johnhunter changed the title fix(proxy): Replace changeOrigin changes in 5.3.0 with new rewriteWsOrigin option fix(proxy): replace changeOrigin changes in 5.3.0 with new rewriteWsOrigin option Jun 25, 2024
@pfdgithub
Copy link

Maybe a rewriteHttpOrigin option should be added as well?

packages/vite/src/types/http-proxy.d.ts Outdated Show resolved Hide resolved
docs/config/server-options.md Outdated Show resolved Hide resolved
@bluwy bluwy added p4-important Violate documented behavior or significantly improves performance (priority) regression The issue only appears after a new release labels Jun 27, 2024
@johnhunter
Copy link
Contributor Author

Maybe a rewriteHttpOrigin option should be added as well?

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.

@pfdgithub
Copy link

pfdgithub commented Jun 28, 2024

Maybe a rewriteHttpOrigin option should be added as well?

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:
Proxying to grafana server, i need to specify the Origin option in the headers.

Due to the diversity in HTTP implementation, compared to WebSocket, HTTP has more use cases where Origin checking is necessary.

Copy link
Member

@bluwy bluwy left a 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.

@pfdgithub
Copy link

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.
Thank you for your work and all the best.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p4-important Violate documented behavior or significantly improves performance (priority) regression The issue only appears after a new release
Projects
None yet
4 participants