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

Validating host header #1858

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

lieryan
Copy link

@lieryan lieryan commented Sep 12, 2022

This is one way this issue could be addressed in Starlette. This basically change the type of the headers value for host to a marker class that inherits from bytes (TrustedHost) once the header is validated by TrustedHostMiddleware. This may cause compatibility issues with middlewares/applications that expects host to be exactly bytes.

There are other approaches this could have been addressed, such as:

  • adding the validation marker into the scope, e.g. scope["host_validated"] = "blah.com"
  • simply validating host against a regex pattern (must only contain characters that are allowed for a domain name), you can still be redirected to an unexpected malicious domain, but at least it would prevent most of the funkier attacks
  • since raising a hard error here is necessarily backwards incompatible, maybe this should just print a warning instead for a few versions

Let me know if you think that another approach should be taken instead.

@Kludex Kludex mentioned this pull request Oct 3, 2022
11 tasks
@Kludex Kludex requested a review from a team February 4, 2023 20:22
@Kludex Kludex mentioned this pull request Feb 14, 2023
8 tasks
@Kludex Kludex added the need confirmation This issue needs confirmation. label Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need confirmation This issue needs confirmation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants