-
Notifications
You must be signed in to change notification settings - Fork 28.4k
fix(core): Add support for proxy using forward headers #15006
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
Conversation
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.
mrge found 3 issues across 1 file. View them in mrge.io
Hey @Phiph, Thanks for the PR, We have created "GHC-1804" as the internal reference to get this reviewed. One of us will be in touch if there are any changes needed, in most cases this is normally within a couple of weeks but it depends on the current workload of the team. |
hey @despairblue thanks for adding the tests and cleaning up the implementation. :) I was going to look at the tests tomorrow! I appreciate it! Is there anything you'd like me to do? |
No problem. Thanks for the initial work ❤️ |
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.
cubic reviewed 2 files and found no issues. Review PR in cubic.dev.
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.
Left some comments, but overall this looks like a good solution to solve the issue we have 👍
* precedence over `host`. | ||
* If they are not both defined then `host` is used and the protocol is | ||
* inferred from `origin`. | ||
*/ |
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.
There's also Forwarded
header with an optional host
field that some people might use, but we can discuss adding support for that on another ticket too.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Forwarded
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.
🚀
/test-workflows |
Co-authored-by: Danny Martini <[email protected]>
Co-authored-by: Danny Martini <[email protected]>
Got released with |
Tested with my setup on AWS + Ubuntu + Litespeed Web Server + CyperPanel, and unfortunately it doesn't fix the "lost connection/websocket" issue. It seems that the culprit is the Litespeed Web Server. |
Co-authored-by: Danny Martini <[email protected]>
Summary
This pull request introduces support for proxies in the
Push
class by utilizingX-Forwarded
headers to determine the expected origin, along with enhanced logging to debug origin-related issues.Support for proxies:
X-Forwarded-Host
andX-Forwarded-Proto
headers over the standardHost
andOrigin
headers to compute theexpectedOrigin
. This ensures compatibility when the application is behind a proxy.Enhanced logging:
Host
,Origin
,X-Forwarded-Host
,X-Forwarded-Proto
, and the computedexpectedOrigin
. This helps in identifying mismatches between theOrigin
header and theexpectedOrigin
.Related Linear tickets, Github issues, and Community forum posts
fixes: #14619, #14653
Review / Merge checklist
release/backport
(if the PR is an urgent fix that needs to be backported)