http: Check for malformed HTTP method for request

[jasnell]

Add a check to make sure that the specified HTTP method is a valid
token per the spec. To do so, we look at the method string and check
that each character is valid per the token rule. The first violation
aborts the check and throws.

This check is necessary to avoid malicious http request header
injection. per #8947

[trevnorris]

@misterdjules @cjihrig Thoughts?

[cjihrig]

I'm +1 on fixing the problem. Does anyone know how much of a security risk do we face by not ensuring that the request method matches the token description precisely? In other words, could we safely get away with cheating and just detecting newlines?

[misterdjules]

check_is_token sounds a bit generic, even though this is in lib/http.js. Maybe check_is_http_token is more explicit?

[misterdjules]

Also, I think it would help to mention that this code is written to conform to the HTTP/1.1 RFC, and from which section comes the set of forbidden characters. Otherwise, it might look like it was chosen arbitrarily.

[tjfontaine]

I agree we want to change the name, and I think general we're more camelCase style -- perhaps checkValidToken

[misterdjules]

Commented inline, I have a few additional comments:

• The code does not pass make jslint.
• I think the test provided could be more thorough. Among other things, it could:
  • Test that valid method options do not throw (using assert.doesNotThrow).
  • Test more invalid characters. If we're making this part of the API more RFC compliant and not just fixing the header injection issue, I think we should test what the RFC describes more thoroughly.

What do you think?

[misterdjules]

Generally, I like to mention using comments what the goal of a given test is. We could mention the link to the given issue, and the rationale for this test (not only fixing the "header injection" issue, but also making the API more compliant with the HTTP RFCs). It helps when revisiting tests much later and trying to understand the intent of the author(s).

[jasnell]

Agreed that a more thorough review of the RFC requirements is going to be necessary overall. I've marked that on my short list to do for v0.14. If folks think it's important enough to do sooner, let me know.

[misterdjules]

My opinion is that without more thorough tests, we won't be able to be confident that this set of changes does what it intends to do, so I would advocate for including them from the start.

[jasnell]

I'm expanding the test case to check a wider range of valid and invalid characters in the method.

[jasnell]

Ok, will refactor this one a bit based on the discussion and get it reworked. Target v0.12?

[misterdjules]

Targeting v0.10 still makes sense to me, as the preferred way to land changes is to do that in the oldest supported & affected branch.

[jasnell]

Replaced by #9076