fix: return 431 status code on HTTP header overflow error

[nflaig]

This PR improves error response if URI is too long. The status code should ideally be 414 to better align response with other http servers and return a more meaningful error message to the client.

EDIT: Since it is not possible to determine if error was caused by too long URI or too large headers we can't change the status code as it could be rather misleading. This PR now just improves the error message if maximum allowed header size was exceeded.

EDIT2: As discussed it might be better to just return a 431 for now in both cases (request header fields are too large and URI is too long) to be more concise about the error (400 is too generic) and to be in line with default Node's default response (nodejs/node#25605).

The error code is set by http parser : HPE_HEADER_OVERFLOW

Checklist

• run npm run test and npm run benchmark
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[mcollina]

lgtm

[Uzlopak]

Somehow this makes no sense for me.

If the header is too long, it should be 431. If the uri is too long, than it should be 414.

Ok, node is handling max size of uri with headers in the same maxHeaderSize option, but this PR will send 414 in case the uri is small but the header is very big.
The test is sending a request with a very long url. But sending a request with a short url and a big header should also throw HPE_HEADER_OVERFLOW

So I am kind of against it, as long as we can not determine if the uri or the headers is too big.

[nflaig]

So I am kind of against it, as long as we can not determine if the uri or the headers is too big

I am not sure this will be possible but I kinda agree here, I was not aware of 431 to be honest, never seen this in the wild.

However, I think it would still be useful to give the client a better indication of what the issue is, maybe could communicate that via the error message and use status code 400.

[Uzlopak]

I also think it is not possible to determine, as the HPE_HEADER_OVERFLOW is thrown in node

[Uzlopak]

LGTM
But I prefer that still one or two other maintainers review this. I have not that much insights into nodejs as some of our maintainers, who are also nodejs core maintainers. So maybe we miss something or so, and others can give proper feedback. ;)

[Fdawgs]

Ok, node is handling max size of uri with headers in the same maxHeaderSize option, but this PR will send 414 in case the uri is small but the header is very big. The test is sending a request with a very long url. But sending a request with a short url and a big header should also throw HPE_HEADER_OVERFLOW

Yeah, bit weird: nodejs/node#26296 (comment)
Should this not be 431 to be in line with Node's default response?

[nflaig]

Should this not be 431 to be in line with Node's default response?

Isn't that inaccurate due to same reason why 414 might be inaccurate?

This PR never got merged but looks like the most reasonable approach to differentiate those two cases

• http: create an option for setting a maximum size for uri parsing nodejs/node#26553

[metcoder95]

I believe that sadly we are limited by Node.js settings on this, as it treats both the request target and headers size errors in a similar manner.

I agree with @Uzlopak that does not feel right, but if looking to be more concise with the errors, I would prefer to move forward with 431 instead of 400, as at least is slightly more spec compliant.

[nflaig]

Updated to 431 status code to be inline with node default

• when a HTTP header overflow error occurs, instead of a generic 400 response, return 431 Request Header Fields Too Large nodejs/node#25605

Until further updates to node itself (like nodejs/node#26553) it won't be possible to differentiate the error cases.

The test cases would catch if the error code used by node changes at any point in the future, and worst case would just default to "400 - Client Error".