http: add strictMode option and checkIsHttpToken check #2526
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fishrock123
added
http
|
Would the goal be to eventually have this as a default setting? |
|
Perhaps eventually.
|
|
LGTM if CI is happy. |
|
Just for reference, I ran the http benchmarks with the strict check gating the token check and without... the numbers follow: With the token check behind the strictMode switch: Always using the token check (no strictMode switch): As you can tell, there is a small bump in the numbers (as should be expected). The question is: is the bump enough to justify the addition of the strictMode switch? Or should we just go ahead and always perform the token check? |
|
Would it be semver-major if it was turned on by default? |
| @@ -493,6 +504,9 @@ Options: | |||
| - `Agent` object: explicitly use the passed in `Agent`. | |||
| - `false`: opts out of connection pooling with an Agent, defaults request to | |||
| `Connection: close`. | |||
| - `strictMode`: When `true`, enables additional strict standards conformance | |||
There was a problem hiding this comment.
Is the bullet aligned with the previous bullets?
|
I'm a bit concerned about the impending beating we may be in for wrt performance in v4. We don't have good numbers comparing the current codebase to 0.10, 0.12 or any version of io.js. We're good at microbenchmarking between increments but not as a broad view. So, without us providing numbers it's going to fall on others to do their own benchmarking, likely in ways that we don't think are necessarily valid reflections of true performance—and even where it is we may be in for some negative press with lower numbers particularly because of the So my inclination for now would be that if this lands it should stay behind a flag for v4 and when we have a chance to investigate the larger perf story (or hear it from others..) we can consider turning it on by default for v5 or later. |
| @@ -208,7 +210,6 @@ OutgoingMessage.prototype._storeHeader = function(firstLine, headers) { | |||
| field = key; | |||
| value = headers[key]; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Invalid whitespace change.
jasnell
force-pushed
the
sanitize-headers
branch
from
f0c144f
to
9c40815
Compare
|
Yes, keeping this behind a flag for now is likely the best option. However, @rvagg, how do you feel about that flag being part of the API itself as opposed to a command line switch? The advantage of having the switch in code is that it is only enabled per individual request or response, it's not set globally, minimizing the impact, but it does mean an additional API we're going to have to track. |
|
I'm not sure this needs to be behind a flag. It's already an opt-in with |
|
@trevnorris I believe by "flag" he was referring to However, if this does become default behaviour then that could be easily removed/ignored. If it doesn't become default behaviour then we're stuck maintaining this functionality, in a Domains kind of way. |
|
Oh. Missed that the hope was for this to be the default behavior. |
jasnell
force-pushed
the
sanitize-headers
branch
from
9c40815
to
a5382de
Compare
|
@brendanashworth @trevnorris @rvagg @indutny @ChALkeR ... I updated the commit to remove the |
|
LGTM if CI is green. Good job @jasnell ! |
| @@ -0,0 +1,56 @@ | |||
| 'use strict'; | |||
There was a problem hiding this comment.
Is it possible to have this test go in /parallel/?
There was a problem hiding this comment.
Certainly. I also just realized I didn't rename it to reflect the fact that strictMode isn't actually there. I'll make both changes now.
|
pretty sure this needs to be upgraded to |
| function checkIsHttpToken(val) { | ||
| return typeof val === 'string' && token.test(val); | ||
| } | ||
| exports._checkIsHttpToken = checkIsHttpToken; |
There was a problem hiding this comment.
Why not just exports.checkIsHttpToken?
There was a problem hiding this comment.
Or if you wanted to introduce a new internal module (internal/http/util?) with this as the first one inside... that'd be great too.
There was a problem hiding this comment.
I'm not entirely convinced that's needed at this point. We already have _http_common.js, which is supposed to be (at least in theory) an internal module. Adding this method to that makes sense and if we want to pull bits of _http_common.js into an internal module later on, then so be it.
|
CI Run going here: https://ci.nodejs.org/job/node-test-pull-request/375/ |
|
Two CI failures that look completely unrelated to this PR |
|
LGTM - Trevor's comment about the commit log. |
jasnell
added a commit
that referenced
this pull request
Sep 25, 2015
Ref: nodejs/node-convergence-archive#13 This adds a new check for header and trailer fields names and method names to ensure that they conform to the HTTP token rule. If they do not, a `TypeError` is thrown. Previously this had an additional `strictMode` option that has been removed in favor of making the strict check the default (and only) behavior. Doc and test case are included. On the client-side ```javascript var http = require('http'); var url = require('url'); var p = url.parse('http://localhost:8888'); p.headers = {'testing 123': 123}; http.client(p, function(res) { }); // throws ``` On the server-side ```javascript var http = require('http'); var server = http.createServer(function(req,res) { res.setHeader('testing 123', 123); // throws res.end('...'); }); ``` Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Trevor Norris <trevnorris@nodejs.org> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org> PR-URL: #2526
|
Landed in 6192c98. Commit log nit fixed. |
rvagg
added a commit
to rvagg/io.js
that referenced
this pull request
Oct 29, 2015
Notable changes: * buffer: (Breaking) Removed both 'raw' and 'raws' encoding types from Buffer, these have been deprecated for a long time (Sakthipriyan Vairamani) nodejs#2859. * console: (Breaking) Values reported by console.time() now have 3 decimals of accuracy added (Michaël Zasso) nodejs#3166. * fs: - fs.readFile*(), fs.writeFile*(), and fs.appendFile*() now also accept a file descriptor as their first argument (Johannes Wüller) nodejs#3163. - (Breaking) In fs.readFile(), if an encoding is specified and the internal toString() fails the error is no longer thrown but is passed to the callback (Evan Lucas) nodejs#3485. - (Breaking) In fs.read() (using the fs.read(fd, length, position, encoding, callback) form), if the internal toString() fails the error is no longer thrown but is passed to the callback (Evan Lucas) nodejs#3503. * http: - Fixed a bug where pipelined http requests would stall (Fedor Indutny) nodejs#3342. - (Breaking) When parsing HTTP, don't add duplicates of the following headers: Retry-After, ETag, Last-Modified, Server, Age, Expires. This is in addition to the following headers which already block duplicates: Content-Type, Content-Length, User-Agent, Referer, Host, Authorization, Proxy-Authorization, If-Modified-Since, If-Unmodified-Since, From, Location, Max-Forwards (James M Snell) nodejs#3090. - (Breaking) The callback argument to OutgoingMessage#setTimeout() must be a function or a TypeError is thrown (James M Snell) nodejs#3090. - (Breaking) HTTP methods and header names must now conform to the RFC 2616 "token" rule, a list of allowed characters that excludes control characters and a number of separator characters. Specifically, methods and header names must now match /^[a-zA-Z0-9_!#$%&'*+.^`|~-]+$/ or a TypeError will be thrown (James M Snell) nodejs#2526. * node: - (Breaking) Deprecated the _linklist module (Rich Trott) nodejs#3078. - (Breaking) Removed require.paths and require.registerExtension(), both had been previously set to throw Error when accessed (Sakthipriyan Vairamani) nodejs#2922. * npm: Upgraded to version 3.3.6 from 2.14.7, see https://github.com/npm/npm/releases/tag/v3.3.6 for more details. This is a major version bump for npm and it has seen a significant amount of change. Please see the original npm v3.0.0 release notes for a list of major changes (Rebecca Turner) nodejs#3310. * src: (Breaking) Bumped NODE_MODULE_VERSION to 47 from 46, this is necessary due to the V8 upgrade. Native add-ons will need to be recompiled (Rod Vagg) nodejs#3400. * timers: Attempt to reuse the timer handle for setTimeout().unref(). This fixes a long-standing known issue where unrefed timers would perviously hold beforeExit open (Fedor Indutny) nodejs#3407. * tls: - Added ALPN Support (Shigeki Ohtsu) nodejs#2564. - TLS options can now be passed in an object to createSecurePair() (Коренберг Марк) nodejs#2441. - (Breaking) The default minimum DH key size for tls.connect() is now 1024 bits and a warning is shown when DH key size is less than 2048 bits. This a security consideration to prevent "logjam" attacks. A new minDHSize TLS option can be used to override the default. (Shigeki Ohtsu) nodejs#1831. * util: - (Breaking) util.p() was deprecated for years, and has now been removed (Wyatt Preul) nodejs#3432. - (Breaking) util.inherits() can now work with ES6 classes. This is considered a breaking change because of potential subtle side-effects caused by a change from directly reassigning the prototype of the constructor using `ctor.prototype = Object.create(superCtor.prototype, { constructor: { ... } })` to using `Object.setPrototypeOf(ctor.prototype, superCtor.prototype)` (Michaël Zasso) nodejs#3455. * v8: (Breaking) Upgraded to 4.6.85.25 from 4.5.103.35 (Ali Ijaz Sheikh) nodejs#3351. - Implements the spread operator, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_operator for further information. - Implements new.target, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/new.target for further information. * zlib: Decompression now throws on truncated input (e.g. unexpected end of file) (Yuval Brik) nodejs#2595. PR-URL: nodejs#3466
rvagg
added a commit
that referenced
this pull request
Oct 29, 2015
Notable changes: * buffer: (Breaking) Removed both 'raw' and 'raws' encoding types from Buffer, these have been deprecated for a long time (Sakthipriyan Vairamani) #2859. * console: (Breaking) Values reported by console.time() now have 3 decimals of accuracy added (Michaël Zasso) #3166. * fs: - fs.readFile*(), fs.writeFile*(), and fs.appendFile*() now also accept a file descriptor as their first argument (Johannes Wüller) #3163. - (Breaking) In fs.readFile(), if an encoding is specified and the internal toString() fails the error is no longer thrown but is passed to the callback (Evan Lucas) #3485. - (Breaking) In fs.read() (using the fs.read(fd, length, position, encoding, callback) form), if the internal toString() fails the error is no longer thrown but is passed to the callback (Evan Lucas) #3503. * http: - Fixed a bug where pipelined http requests would stall (Fedor Indutny) #3342. - (Breaking) When parsing HTTP, don't add duplicates of the following headers: Retry-After, ETag, Last-Modified, Server, Age, Expires. This is in addition to the following headers which already block duplicates: Content-Type, Content-Length, User-Agent, Referer, Host, Authorization, Proxy-Authorization, If-Modified-Since, If-Unmodified-Since, From, Location, Max-Forwards (James M Snell) #3090. - (Breaking) The callback argument to OutgoingMessage#setTimeout() must be a function or a TypeError is thrown (James M Snell) #3090. - (Breaking) HTTP methods and header names must now conform to the RFC 2616 "token" rule, a list of allowed characters that excludes control characters and a number of separator characters. Specifically, methods and header names must now match /^[a-zA-Z0-9_!#$%&'*+.^`|~-]+$/ or a TypeError will be thrown (James M Snell) #2526. * node: - (Breaking) Deprecated the _linklist module (Rich Trott) #3078. - (Breaking) Removed require.paths and require.registerExtension(), both had been previously set to throw Error when accessed (Sakthipriyan Vairamani) #2922. * npm: Upgraded to version 3.3.6 from 2.14.7, see https://github.com/npm/npm/releases/tag/v3.3.6 for more details. This is a major version bump for npm and it has seen a significant amount of change. Please see the original npm v3.0.0 release notes for a list of major changes (Rebecca Turner) #3310. * src: (Breaking) Bumped NODE_MODULE_VERSION to 47 from 46, this is necessary due to the V8 upgrade. Native add-ons will need to be recompiled (Rod Vagg) #3400. * timers: Attempt to reuse the timer handle for setTimeout().unref(). This fixes a long-standing known issue where unrefed timers would perviously hold beforeExit open (Fedor Indutny) #3407. * tls: - Added ALPN Support (Shigeki Ohtsu) #2564. - TLS options can now be passed in an object to createSecurePair() (Коренберг Марк) #2441. - (Breaking) The default minimum DH key size for tls.connect() is now 1024 bits and a warning is shown when DH key size is less than 2048 bits. This a security consideration to prevent "logjam" attacks. A new minDHSize TLS option can be used to override the default. (Shigeki Ohtsu) #1831. * util: - (Breaking) util.p() was deprecated for years, and has now been removed (Wyatt Preul) #3432. - (Breaking) util.inherits() can now work with ES6 classes. This is considered a breaking change because of potential subtle side-effects caused by a change from directly reassigning the prototype of the constructor using `ctor.prototype = Object.create(superCtor.prototype, { constructor: { ... } })` to using `Object.setPrototypeOf(ctor.prototype, superCtor.prototype)` (Michaël Zasso) #3455. * v8: (Breaking) Upgraded to 4.6.85.25 from 4.5.103.35 (Ali Ijaz Sheikh) #3351. - Implements the spread operator, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_operator for further information. - Implements new.target, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/new.target for further information. * zlib: Decompression now throws on truncated input (e.g. unexpected end of file) (Yuval Brik) #2595. PR-URL: #3466
This was referenced Dec 20, 2023
This was referenced Mar 29, 2024
This was referenced Mar 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Per: nodejs/node-convergence-archive#13
Adds a check that will cause a TypeError to be thrown if the HTTP
method or header field name does not conform to the Token rule.
This adds a newstrictModeflag to OutgoingMessage that, whenenabled, will cause a TypeError to be thrown if the HTTP Methodor header field name does not conform to the Token rule.ThestrictModeflag ensures that, in the common case, thereis minimal performance hit and that existing code should continueto work. The Token check is only performed whenstrictMode = true.Doc and test case are included.
On the client-side
On the server-side