Node: Invalid HTTP method incorrectly returns a 400 instead of a 405

[Qix-]

const http = require('http');

const srv = http.createServer((req, res) => {
    res.writeHead(200, { 'Content-Type': 'text/plain' });
    res.end('okay');
});

srv.listen(3210);

$ curl -vv localhost:3210 -XOHAI
* Rebuilt URL to: localhost:3210/
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 3210 (#0)
> OHAI / HTTP/1.1
> Host: localhost:3210
> User-Agent: curl/7.54.0
> Accept: */*
>
< HTTP/1.1 400 Bad Request
* no chunk, no close, no size. Assume close to signal end
<
* Closing connection 0

This should be sending a 405 Method Not Allowed, not a 400.

Also, preferably, Node shouldn't care about the method and should happily forward it along in the req.method field. But I know that's asking for too much.

[devsnek]

Also, preferably, Node shouldn't care about the method and should happily forward it along in the req.method field.

node's http parser cannot actually use custom methods, it has them hardcoded

[aqrln]

Hi @Qix- and thanks for your suggestion, but I would argue that the current behavior is, in fact, correct and conforms to the RFC.

https://tools.ietf.org/html/rfc7231#section-6.5.1:

The 400 (Bad Request) status code indicates that the server cannot or
will not process the request due to something that is perceived to be
a client error (e.g., malformed request syntax, invalid request
message framing, or deceptive request routing).

https://tools.ietf.org/html/rfc7231#section-6.5.5:

The 405 (Method Not Allowed) status code indicates that the method
received in the request-line is known by the origin server but not
supported by the target resource.

In other words, if Node's HTTP parser cannot parse the request because it can't recognize the method, it should send a 400. A 405 would imply that the server supports the method in general, but it isn't supported for a specific resource a client requests.

[Qix-]

node's http parser cannot actually use custom methods, it has them hardcoded

I know - I don't think it should do that. Or, it should be opt-out at the very least. I understand when I'm breaking RFC (see below) to do some proprietary operations on my HTTP server and now Node refuses to let me do that. Node is forcing its opinion on my applications - something that's a bit out of place for Node, IMO.

Hi @Qix- and thanks for your suggestion, but I would argue that the current behavior is, in fact, correct and conforms to the RFC.

Which RFC? According to RFC 7231 §4.1 (the same RFC you reference), there is no limitation on the methods - instead, there are a set of _standard_ methods with well-defined semantics, all of which are optional except for GET and HEAD.

I'll cede that 400 is the correct status code in this case but Node shouldn't be making that decision for me. My application logic should be the one determining what is/isn't "known" to my application. Not Node.

[devsnek]

@Qix- you can override process.binding('http_parser').HTTPParser for the time being, which i think counts as an "opt out", although its pretty messy. perhaps someone can pr a more standardized format for the parser and an option to override it in createServer

[Qix-]

@devsnek I'd rather not re-implement Node internals, but thanks for the hint. If it really comes to it, I can do that. I'd imagine it'd be a huge performance penalty if it were done in pure Javascript, though.

[devsnek]

related: #1081, #1457

[aqrln]

@Qix-:

According to RFC 7231 §4.1 (the same RFC you reference), there is no limitation on the methods - instead, there are a set of standard methods with well-defined semantics, all of which are optional except for GET and HEAD.

Yes, that's right. But it's a completely different issue and a limitation of Node's HTTP parser. If you'd like to open a feature request, go for it. However, realistically, I wouldn't make any assumptions about how fast such a feature could land in core if it were implemented, and would it eventually land at all given the amount of churn in critical internal code it would most probably cause :(

[jasnell]

The http1 http_parser is written to support a limited range of methods primarily for performance reasons. I've tried changing that before and it ended up with too great of a performance penalty and went no where. At this point, it is exceedingly unlikely to change. Fortunately, the new http2 implementation does not have that problem and supports any HTTP method.

As for the response code, returning a 400 response is technically correct as 400 generically covers all 4xx codes -- it's just, admittedly, not as useful.

[Qix-]

Thanks for the answer @jasnell, that makes sense. I'll look into the http2 module as, luckily, we're not constrained by Node version 👍