Skip to content

Conversation

@TheBlueMatt
Copy link
Member

I realize we also don't have default size limits for the total response headers or status line, which is quite a large footgun. Instead, here we adopt the default response header limit from chrome and set a status line limit of 1/4 of that. Also adds a 1GiB default body response just to have some limit even if its incredibly high and we still document that folks should really set it.

IMO we should cut this as 0.3.1.

While its hard to say what size body a client wants, HTTP headers
and status line should be quite limited and its very easy for a dev
to forget to set a reasonable limit.

Here we limit the total header size to the same value used in
Chrome, which seems like a pretty safe limit, and the status line
limit to a quarter of that (which is really absurd for a status
line).
Because its very easy to (mis-)use the API and forget to set a
response size limit, we really should have a default one. Sadly,
its also quite hard to pick a reasonable default here. Rather than
being conservative, we pick 1 GiB and hope that this is enough to
avoid an OOM, even though its still quite huge, and almost
certainly is larger than what people will use `bitreq` for.
Copy link
Collaborator

@jamillambert jamillambert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK dfa0ce9

nit: Could be defined as consts e.g. DEFAULT_MAX_HEADER_SIZE to make it easier to understand what the numbers are when scanning over the code.

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