python-hyper / h11

A pure-Python, bring-your-own-I/O implementation of HTTP/1.1
https://h11.readthedocs.io/
MIT License
490 stars 62 forks source link

Limit number of headers #155

Closed Kludex closed 2 years ago

Kludex commented 2 years ago

Hi there :wave:

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 commented 2 years ago

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 commented 2 years ago

I'm not sure why to limit by number?

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

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

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

njsmith commented 2 years ago

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 commented 2 years ago

Perfect. Thanks for the insight @njsmith 🙏