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

Make URI and Host header semantics more explicit #90

Merged
merged 1 commit into from
Nov 11, 2021

Conversation

aturon
Copy link
Contributor

@aturon aturon commented Nov 10, 2021

Previously, we were relying on subtleties of Hyper's behavior to determine the final URI and Host header for outbound requests. Unfortunately, that behavior is sensitive to a number of factors, e.g. whether we happen to be sending a request via an HTTP2 connection.

This commit changes Viceroy to canonicalize the request URI and Host header before it ever reaches Hyper. In most cases this behavior will match what Hyper would do anyway.

Fixes #89

Previously, we were relying on subtleties of Hyper's behavior to determine the final URI and Host header for outbound requests. Unfortunately, that behavior is sensitive to a number of factors, e.g. whether we happen to be sending a request via an HTTP2 connection.

This commit changes Viceroy to canonicalize the request URI and Host header before it ever reaches Hyper. In most cases this behavior will match what Hyper would do anyway.

Fixes #89
@aturon aturon requested a review from cratelyn November 10, 2021 19:59
Copy link
Contributor

@cratelyn cratelyn left a comment

Choose a reason for hiding this comment

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

this was a remarkably tricky bug, and i want to commend you again for tracking down the cause of this @aturon. wonderful work ✔️ ✨

@aturon aturon merged commit b85d0f5 into main Nov 11, 2021
@aturon aturon deleted the aturon/uri-and-host-semantics branch November 11, 2021 18:27
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.

Backend requests to TLS origins don't seem to set Host overrides properly
2 participants