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

fix: Do not include user information in Host header #3621

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

Conversation

tindzk
Copy link

@tindzk tindzk commented Apr 5, 2024

According to RFC 9110, section 7.2, the Host header should only comprise the URI host and an optional port.

Currently, the examples set the Host header to the URI's authority which may also contain user information (see RFC 3986, section 3.2).

Update the examples to construct the Host header manually to avoid sensitive information from showing up in server logs and to ensure that the server's routing logic works correctly when a username and password are supplied.

@seanmonstar
Copy link
Member

Thanks for the PR! I agree with your assessment, this is more correct. I do wish, however, that the examples could remain as simple as possible...

hyper-util's legacy Client does it automatically. That should get extracted to a helper middleware, like SetHost. We can't reference it now, but if we eventually could, would a comment like // consider using tower_http::SetHost help people? This isn't very actionable in this PR...

According to RFC 9110, section 7.2, the Host header should only comprise
the URI host and an optional port.

Currently, the examples set the Host header to the URI's authority which
may also contain user information (see RFC 3986, section 3.2).

Update the examples to construct the Host header manually to avoid
sensitive information from showing up in server logs and to ensure that
the server's routing logic works correctly when a username and password
are supplied.
@tindzk
Copy link
Author

tindzk commented Apr 5, 2024

I agree with the sentiment of keeping the examples as minimal as possible. I have shortened the fix to always include the port number. This should be compatible with the majority of servers out there.

The same issue is also present on the website: https://hyper.rs/guides/1/client/basic/

@sfackler
Copy link
Contributor

@seanmonstar I think this was closed by accident?

@seanmonstar seanmonstar reopened this Apr 10, 2024
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.

None yet

3 participants