New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: verify method is a string #10111

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@lucamaraschi
Contributor

lucamaraschi commented Dec 4, 2016

Checklist
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

http

Description of change

Added strict check for http request method to be a string. If method is not a string (or undefined or null) a TypeError exceptions is thrown.
The method value is still defaulted to GET in the following cases:

  • method is undefined
  • method is null
    This fixes the http-client to crash if options.method in http.request was set to a number not equal to 0.
Show outdated Hide outdated lib/_http_client.js
if (_method !== undefined && _method !== null
&& typeof _method !== 'string') {
throw new TypeError('Method must be a string');
}

This comment has been minimized.

@mscdex

mscdex Dec 4, 2016

Contributor

For the best backwards compatibility (and to emulate the method value selection behavior below the typeof check), I think the undefined/null check should be removed and just test for truthiness:

if (options.method && typeof options.method !== 'string') {
  throw new TypeError('Method must be a string');
}

The reason being that options.method could be one of several non-truthy values, such as 0 and false.

@mscdex

mscdex Dec 4, 2016

Contributor

For the best backwards compatibility (and to emulate the method value selection behavior below the typeof check), I think the undefined/null check should be removed and just test for truthiness:

if (options.method && typeof options.method !== 'string') {
  throw new TypeError('Method must be a string');
}

The reason being that options.method could be one of several non-truthy values, such as 0 and false.

This comment has been minimized.

@lucamaraschi

lucamaraschi Dec 4, 2016

Contributor

In this case if options.method is set to 0 results in a falsy value which is going to default the method to GET in the following check.

@lucamaraschi

lucamaraschi Dec 4, 2016

Contributor

In this case if options.method is set to 0 results in a falsy value which is going to default the method to GET in the following check.

This comment has been minimized.

@lucamaraschi

lucamaraschi Dec 4, 2016

Contributor

@mscdex that would not fix the non-truthy values, confusing the defaulting of 0 values to GET.

@lucamaraschi

lucamaraschi Dec 4, 2016

Contributor

@mscdex that would not fix the non-truthy values, confusing the defaulting of 0 values to GET.

This comment has been minimized.

@mscdex

mscdex Dec 4, 2016

Contributor

Well, it's a difference between semver-ness. This change is definitely going to be semver-major as-is, but it could be semver-patch with the check I suggested. It's up to you.

@mscdex

mscdex Dec 4, 2016

Contributor

Well, it's a difference between semver-ness. This change is definitely going to be semver-major as-is, but it could be semver-patch with the check I suggested. It's up to you.

@mscdex mscdex added the semver-major label Dec 4, 2016

Show outdated Hide outdated test/parallel/test-http-client-check-http-token.js
const assert = require('assert');
const http = require('http');
const expectedSuccesses = {

This comment has been minimized.

@cjihrig

cjihrig Dec 4, 2016

Contributor

Only the keys seem to be used. This could probably be an array...

@cjihrig

cjihrig Dec 4, 2016

Contributor

Only the keys seem to be used. This could probably be an array...

Show outdated Hide outdated test/parallel/test-http-client-check-http-token.js
http.request({ method: method, port: server.address().port }).end();
}
ok(undefined);

This comment has been minimized.

@cjihrig

cjihrig Dec 4, 2016

Contributor

... and then you can loop over the array here. Right now, the number of times ok() is called is independent of methods.

@cjihrig

cjihrig Dec 4, 2016

Contributor

... and then you can loop over the array here. Right now, the number of times ok() is called is independent of methods.

Show outdated Hide outdated test/parallel/test-http-client-check-http-token.js
function fail(input) {
assert.throws(() => {
http.request({ method: input, path: '/' }, common.fail);
}, /Method must be a string/);

This comment has been minimized.

@cjihrig

cjihrig Dec 4, 2016

Contributor

If you want to be really strict, this could be /^TypeError: Method must be a string$/.

@cjihrig

cjihrig Dec 4, 2016

Contributor

If you want to be really strict, this could be /^TypeError: Method must be a string$/.

Show outdated Hide outdated lib/_http_client.js
@@ -68,6 +68,11 @@ function ClientRequest(options, cb) {
self.socketPath = options.socketPath;
self.timeout = options.timeout;
const _method = options.method;
if (_method !== undefined && _method !== null

This comment has been minimized.

@cjihrig

cjihrig Dec 4, 2016

Contributor

This can be written as:

if (_method != null && typeof _method !== 'string')

If you don't choose to rewrite like that, the && should be moved to the previous line, and the second line should be lined up. I'm surprised the linter doesn't complain.

@cjihrig

cjihrig Dec 4, 2016

Contributor

This can be written as:

if (_method != null && typeof _method !== 'string')

If you don't choose to rewrite like that, the && should be moved to the previous line, and the second line should be lined up. I'm surprised the linter doesn't complain.

@lucamaraschi

This comment has been minimized.

Show comment
Hide comment
@lucamaraschi

lucamaraschi Dec 6, 2016

Contributor

@cjihrig and @mscdex I amended the PR adding your feedbacks.

Contributor

lucamaraschi commented Dec 6, 2016

@cjihrig and @mscdex I amended the PR adding your feedbacks.

@cjihrig

This comment has been minimized.

Show comment
Hide comment
Contributor

cjihrig commented Dec 6, 2016

Show outdated Hide outdated lib/_http_client.js
const _method = options.method;
if (_method != null && typeof _method !== 'string') {
throw new TypeError('Method must be a string');
}
var method = self.method = (options.method || 'GET').toUpperCase();

This comment has been minimized.

@cjihrig

cjihrig Dec 6, 2016

Contributor

We should probably use options.method or _method consistently. Right now it's a mix.

@cjihrig

cjihrig Dec 6, 2016

Contributor

We should probably use options.method or _method consistently. Right now it's a mix.

This comment has been minimized.

@jasnell

jasnell Dec 6, 2016

Member

The common._checkIsHttpToken() check on line 77 already includes a test to ensure method is a string.

@jasnell

jasnell Dec 6, 2016

Member

The common._checkIsHttpToken() check on line 77 already includes a test to ensure method is a string.

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Dec 6, 2016

Contributor

One CI failure looks unrelated.

Contributor

cjihrig commented Dec 6, 2016

One CI failure looks unrelated.

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Dec 6, 2016

Member

Is this really semver-major? Wouldn't node have thrown an unexpected internal type error when it tried to use the method as a String, even though it wasn't? I understand when text of warning messages are changed it can break user-space, but user-space depending on something like undefined is not a function instead of a proper type error seems a bit more far fetched. Generally, people test for operation errors, not syntax errors.

Just asking for clarification here.

Member

sam-github commented Dec 6, 2016

Is this really semver-major? Wouldn't node have thrown an unexpected internal type error when it tried to use the method as a String, even though it wasn't? I understand when text of warning messages are changed it can break user-space, but user-space depending on something like undefined is not a function instead of a proper type error seems a bit more far fetched. Generally, people test for operation errors, not syntax errors.

Just asking for clarification here.

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Dec 6, 2016

Member

@sam-github See mscdex's comment here: #10111 (comment)
It's semver-major because before the change, if options.method was any falsy value, it would default to 'GET'. Now it throws (unless it's the empty string).
IMO the check should be changed so that we can remove the semver-major label.

Member

targos commented Dec 6, 2016

@sam-github See mscdex's comment here: #10111 (comment)
It's semver-major because before the change, if options.method was any falsy value, it would default to 'GET'. Now it throws (unless it's the empty string).
IMO the check should be changed so that we can remove the semver-major label.

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Dec 6, 2016

Contributor

I think this is the correct thing to do moving forward. A second PR that is more backwards compatible might be good for Node 7 and earlier.

Contributor

cjihrig commented Dec 6, 2016

I think this is the correct thing to do moving forward. A second PR that is more backwards compatible might be good for Node 7 and earlier.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Dec 6, 2016

Member

While I appreciate the new test, the common._checkIsHttpToken() check that is already there already validates that method is a string and already throws a TypeError if it is not.

Member

jasnell commented Dec 6, 2016

While I appreciate the new test, the common._checkIsHttpToken() check that is already there already validates that method is a string and already throws a TypeError if it is not.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Dec 6, 2016

Contributor

@jasnell The issue though is that that common._checkIsHttpToken() check is happening after options.method || 'GET'. So if someone passes say false for options.method, it will be implicitly changed to 'GET'. This PR is just being more strict, presumably to catch programmer errors.

Contributor

mscdex commented Dec 6, 2016

@jasnell The issue though is that that common._checkIsHttpToken() check is happening after options.method || 'GET'. So if someone passes say false for options.method, it will be implicitly changed to 'GET'. This PR is just being more strict, presumably to catch programmer errors.

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Dec 6, 2016

Contributor

In addition to what @mscdex said, if the method is not a string and is truthy, then an exception is thrown: TypeError: (options.method || "GET").toUpperCase is not a function

Contributor

cjihrig commented Dec 6, 2016

In addition to what @mscdex said, if the method is not a string and is truthy, then an exception is thrown: TypeError: (options.method || "GET").toUpperCase is not a function

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Dec 6, 2016

Member

ok.. works for me :-)

Member

jasnell commented Dec 6, 2016

ok.. works for me :-)

Show outdated Hide outdated lib/_http_client.js
@@ -68,7 +68,11 @@ function ClientRequest(options, cb) {
self.socketPath = options.socketPath;
self.timeout = options.timeout;
var method = self.method = (options.method || 'GET').toUpperCase();
const _method = options.method;

This comment has been minimized.

@mscdex

mscdex Dec 7, 2016

Contributor

Maybe we can just move the var method declaration up here and re-assign on line 75, that way we don't have a second variable for the same thing?

@mscdex

mscdex Dec 7, 2016

Contributor

Maybe we can just move the var method declaration up here and re-assign on line 75, that way we don't have a second variable for the same thing?

Show outdated Hide outdated lib/_http_client.js
@@ -68,7 +68,11 @@ function ClientRequest(options, cb) {
self.socketPath = options.socketPath;
self.timeout = options.timeout;
var method = self.method = (options.method || 'GET').toUpperCase();
let method = options.method;

This comment has been minimized.

@cjihrig

cjihrig Dec 7, 2016

Contributor

Can you keep var. We haven't really made the switch to let in lib/ code for performance reasons.

@cjihrig

cjihrig Dec 7, 2016

Contributor

Can you keep var. We haven't really made the switch to let in lib/ code for performance reasons.

@jasnell

jasnell approved these changes Dec 7, 2016

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Dec 8, 2016

Contributor

Landed in df39784. Thanks!

Contributor

cjihrig commented Dec 8, 2016

Landed in df39784. Thanks!

@cjihrig cjihrig closed this Dec 8, 2016

cjihrig added a commit that referenced this pull request Dec 8, 2016

http: verify client method is a string
Prior to this commit, it was possible to pass a truthy non-string
value as the HTTP method to the HTTP client, resulting in an
exception being thrown. This commit adds validation to the method.

PR-URL: #10111
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

italoacasas added a commit to italoacasas/node that referenced this pull request Jan 18, 2017

http: verify client method is a string
Prior to this commit, it was possible to pass a truthy non-string
value as the HTTP method to the HTTP client, resulting in an
exception being thrown. This commit adds validation to the method.

PR-URL: nodejs#10111
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment