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

Add Host header #95

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ZanyMonk
Copy link

@ZanyMonk ZanyMonk commented Mar 5, 2024

Add the Host header back into the request headers.

@darklynx
Copy link
Owner

darklynx commented Mar 6, 2024

Hi,

This PR is trying to address the issue reported here: #61

See my comment on the issue: unfortunately, Go developers decided to swallow the Host header in their HTTP handling implementation :(

Your PR solves the issue for a subset of the cases and can mislead a client about true value of Host header, which might not be present in the original request at all, or have a different value.

Given the fact that in most of the cases the Request Baskets service would run behind the proxy in order to gain HTTPS capabilities, the suggested logic for completing the Host header might be entirely incorrect.

I'll try to run some tests to see how the logic behaves when a service is behind proxy, and will report my findings.

Cheers,
darklynx

@darklynx
Copy link
Owner

darklynx commented Mar 6, 2024

Another observation: the current implementation may actually break the request forwarding logic:
https://github.com/darklynx/request-baskets/blob/master/baskets.go#L178

I'm not really sure how the Host header might be interpreted by a 3rd party service when a request is forwarded to a totally different domain, such forwarding might be rejected by a recipient or mess up the handling logic 🤔

I would at least extend the headers cleanup logic and remove the Host header.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants