Skip to content

unset host, if set, before sending proxy cloud run/functions request #3025

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

Merged
merged 7 commits into from
Jan 13, 2021

Conversation

bkendall
Copy link
Contributor

@bkendall bkendall commented Jan 12, 2021

Description

I cannot keep host set while proxying as it breaks the outbound request since it won't match the URL we're going for.

Fixes #3012

Scenarios Tested

  • Proxying to a Cloud Run container.
  • Proxying to a Cloud Function.

@bkendall bkendall requested a review from samtstern January 12, 2021 21:30
@google-cla google-cla bot added the cla: yes Manual indication that this has passed CLA. label Jan 12, 2021
@coveralls
Copy link

coveralls commented Jan 12, 2021

Coverage Status

Coverage increased (+0.01%) to 65.548% when pulling f3ef28f on bk-3012 into 87434cc on master.

Copy link
Contributor

@samtstern samtstern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved but please address one comment.

@@ -68,6 +68,11 @@ export function proxyRequestHandler(url: string, rewriteIdentifier: string): Req
Cookie: sessionCookie || "",
});
for (const key of Object.keys(req.headers)) {
// Skip particular header keys:
// - using x-forwarded-host, don't need to keep `host` in the headers.
if (["host"].includes(key)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why ["host"].includes(key) and not key === "host"? Was this maybe a longer list at first?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a headers.delete("host") below before but didn't want that as a pattern. I figured being able to have a list here would be great just in case there were other headers a proxy shouldn't actually be sending.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then maybe do two lines:

const headersToDelete = ["host"];

if (headersToDelete.includes(key)) {

@bkendall bkendall merged commit b26aa46 into master Jan 13, 2021
@bkendall bkendall deleted the bk-3012 branch January 13, 2021 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Manual indication that this has passed CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Firebase serve fails to rewrite URL to containerized app after 8.18.0
3 participants