feat: add support for limit header size #1684
Conversation
|
Some tests are failing because of huge headers. Should we not default to 8190, and use only if the argument is passed? |
…preserve backwards compatibility.
| @@ -237,6 +241,12 @@ def on_header(self, name: bytes, value: bytes) -> None: | |||
| if name == b"expect" and value.lower() == b"100-continue": | |||
| self.expect_100_continue = True | |||
| self.headers.append((name, value)) | |||
| if self.config.limit_request_header_size and ( | |||
There was a problem hiding this comment.
I wanted to check lengths of name and values individually. Is this the right approach? Or should we combine? len(name+value)?
| type=int, | ||
| default=0, | ||
| is_flag=False, | ||
| flag_value=8190, |
There was a problem hiding this comment.
8190 is standard value used by Gunicorn, IBM server, etc.
|
Thanks @Kludex for pointing out an example test :) |
Check if headers are legal before appending. This is more secure.
| @@ -154,7 +155,10 @@ def data_received(self, data: bytes) -> None: | |||
| try: | |||
| self.parser.feed_data(data) | |||
| except httptools.HttpParserError: | |||
| msg = "Invalid HTTP request received." | |||
| if self.header_too_big: | |||
| msg = "Header size exceeds limit" | |||
There was a problem hiding this comment.
it sould be return http 431 Request Header Fields Too Large
https://www.rfc-editor.org/rfc/rfc6585#section-5
There was a problem hiding this comment.
Makes sense, thanks!
But I'm not sure if there's a straightforward way to raise or respond with status code other than 400.
May be we need a helper function similar to send400response for other status codes.
|
We should be able to stop when we are about to reach the limit, not from the chunk itself. When we have the chunk of data, it's already too late. I'm not sure if this is possible without changing httptools itself. Let's follow MagicStack/httptools#87 |
|
So shall we close this PR now? |
Implemented a feature discussed in #157
Note:
I am still figuring out how to add this in
h11implementation. But as per #157 (comment) we do not need there.