bodyLimit middleware trusts Content-Length

[sharifmacky]

Why does the bodyLimit middleware trust the Content-Length header? I can manually set its value to any small value and the middleware skips checking the limit and happily parses the entire body.

if (c.req.raw.headers.has("content-length")) {
    const contentLength = parseInt(c.req.raw.headers.get("content-length") || "0", 10);
    return contentLength > maxSize ? onError(c) : next(); // <-- if content length is set, trust it
}

[usualoma]

Hi @sharifmacky, thank you for your feedback.

When we implemented this, we confirmed that regardless of the runtime environment (deno, bun, node-server, etc.), specifying a content-length smaller than the actual body size resulted in a 400 error (or similar). Based on this, we determined that this behavior is reliable and have implemented it accordingly.

Is there a runtime that cannot be trusted?

[sharifmacky]

Hi thanks for your quick reply. Yes I see this is only an issue when running in a test environment, and running live with node/undici does indeed truncate the stream. It's a great optimization but feel concerned that it relies on the underlying environment. I'm not sure how other libs like Got and Superagent etc are implemented, but potentially that would leave a gap in your security? Perhaps, this should be an opt-in optimisation and more clearly documented. I know you mention using the value of the Content-Length in your documentation, but it's a little vague and doesn't make it clear it relies on the underlying fetch implementation.

[usualoma]

Hi @sharifmacky, thank you for your response.

Yes, as you pointed out, I think it depends on the environment.

If there is an app running within some internal environment (such as a service worker), and you create a Request object yourself, it is possible that an incorrect value will be entered in Content-Length.

However, in the usage environments we are aware of where bodyLimit is likely to be used, Content-Length is considered reliable. The documentation explicitly states, "This middleware first uses the value of the Content-Length header in the request, if present." Therefore, I believe this is not an issue.

https://hono.dev/docs/middleware/builtin/body-limit

Hi @yusukebe, what do you think?

[yusukebe]

Hi. I think the current implementation is okay for the same reason as @usualoma .

We may be able to add an option like checkContentLengthHeader (the default value is true) and users can disable it by setting it to false. But I'm not so eager to do so.

[sharifmacky]

Hi, I was more concerned about a bad actor trying to spoof the header. I guess it becomes a bit of an edge case since as you pointed out, undici fetch (node) will only read up to the content-length, so would only affect someone using another fetch implementation (which likely does the same anyway). Perhaps a comment in doc just to clarify it relies on the fetch implementation and that node etc, already truncates the stream. I found it because in a unit test env there's no network, so the stream isn't read and not truncated - leading me to think the middleware was broken.