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

Limit number of headers #155

Closed
Kludex opened this issue Oct 1, 2022 · 4 comments
Closed

Limit number of headers #155

Kludex opened this issue Oct 1, 2022 · 4 comments

Comments

@Kludex
Copy link
Contributor

Kludex commented Oct 1, 2022

Hi there 👋

I'm trying to implement a logic to limit the number of header fields in uvicorn, and I was wondering: is it even possible?

I can check the size of the Request.headers, but my intention is to send a 400 when I pass a threshold when reading the data, not when I already have the headers. Pretty much how gunicorn does: https://docs.gunicorn.org/en/stable/settings.html?highlight=limit_request_fields#limit-request-fields .

Tips for me here?

@pgjones
Copy link
Member

pgjones commented Oct 1, 2022

I'm not sure why to limit by number? I think DEFAULT_MAX_INCOMPLETE_EVENT_SIZE acts in a similar way, but limits by memory usage.

@Kludex
Copy link
Contributor Author

Kludex commented Oct 1, 2022

I'm not sure why to limit by number?

Well... To be honest... Neither do I. I was going to ask you later. 🤣

I thought it would make sense as gunicorn has it, and @tomchristie listed on encode/uvicorn#157 (it's from 4 years ago tho).

Also, we have h11-max-incomplete-event-size setting on uvicorn already. Thanks Phil. 🙏

@njsmith
Copy link
Member

njsmith commented Oct 1, 2022

Yeah, if your goal is to prevent DoS attacks, then using a limit on the number of headers means you have to incrementally parse each header as it arrives and compare it against the limit, which is substantially slower than just checking the size of header block as it streams in and parsing it afterwards in one go. And then you'd still need a size limit anyway, to stop someone sending a single gigabyte-sized header. So that's why I went with one overall limit on all the headers, instead of doing something per-header.

You could reasonably rename that h11-max-incomplete-event-size option to something more user-friendly. In practice the limit pretty much only applies to headers.

@Kludex
Copy link
Contributor Author

Kludex commented Oct 2, 2022

Perfect. Thanks for the insight @njsmith 🙏

@Kludex Kludex closed this as completed Oct 2, 2022
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

No branches or pull requests

3 participants