-
Notifications
You must be signed in to change notification settings - Fork 516
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(cloud-function): strip X-Forwarded-Host + Forwarded headers #8894
Conversation
`http-proxy` with `xfwd: true` only sets x-forwarded-{for,port,proto}, so other X-Forwarded headers would stay in place, causing side-effects.
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
Stripping |
@fiji-flo as I understand it, |
Co-authored-by: Leo McArdle <[email protected]>
0530ac9
to
c169410
Compare
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.
Looks good, let's put on staging to test it fixes the problem (without causing any others).
Edit: I see you've already kicked off an xyz build
as I understand it, http-proxy won't overwrite an existing x-forwarded-for header, which is why we strip it, then it adds the correct value back in
I misread, it does do the proper thing with x-forwarded-for
.
Summary
(MP-427)
Problem
http-proxy
withxfwd: true
only setsx-forwarded-{for,port,proto}
and doesn't overridex-forwarded-host
(see here), which may cause unwanted side-effects.Solution
Remove
x-forwarded-host
header (plusforwarded
, which may contain ahost=
directive) in our Cloud Function.How did you test this change?
Ran
npm start
incloud-function/
and verified thatcurl -H 'x-forwarded-host: bar http://localhost:5100/fr/?example
does not forward the header.