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

Implement --limit-request-header-count flag #1683

Closed
wants to merge 2 commits into from

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented Oct 1, 2022

Please check #1682 before reviewing this.

Checklist

  • h11 implementation. See Limit number of headers python-hyper/h11#155 for more information.
  • Check if the flag limit_request_fields on gunicorn is correctly passed to uvicorn.
  • Add an entry to "Resource Limits" section in the documentation.

@njsmith
Copy link

njsmith commented Oct 1, 2022

I'm not sure this approach actually does anything useful -- presumably the reason people want to limit header count is to avoid people sending huge numbers of headers and overwhelming the server. But this PR checks the number of headers after the server has already accepted and parsed the entire header block, so if the header block was going to overwhelm the server, it's already happened before the check runs.

In practice though h11 already puts a limit on the total size of the header block, so this option probably isn't doing anything useful anyway. python-hyper/h11#155 (comment) has some more details.

@Kludex
Copy link
Member Author

Kludex commented Oct 2, 2022

I see... Thanks @njsmith 🙏

I'll look for just limiting the size then. For h11, we already support h11-max-incomplete-event-size, but for httptools can I limit this on uvicorn itself? If I do that check on data_received, it would slow down uvicorn... 🤔

@Kludex Kludex closed this Oct 2, 2022
@Kludex Kludex deleted the feat/too-many-headers-http branch October 2, 2022 07:15
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