Skip to content

Fixed invalid path in CONNECT method requests. #34412

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.

Sign up for GitHub

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

Closed
preyunk wants to merge 9 commits into from
Closed

Fixed invalid path in CONNECT method requests. #34412

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
edd8f6e
http: fixed invalid path with connect method
preyunk Jul 17, 2020
0825564
http: fixed invalid path with connect method
preyunk Jul 17, 2020
247401a
Fixed linting
preyunk Jul 17, 2020
989cecd
checked for undefined options.path
preyunk Jul 17, 2020
e1283be
Added check for valid host:port path
preyunk Jul 21, 2020
669ee65
Fixes
preyunk Jul 22, 2020
c9f1597
fixes
preyunk Aug 5, 2020
d8039e3
minor fixes
preyunk Aug 6, 2020
9426cbb
removed regex
preyunk Sep 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fixes
  • Loading branch information
@preyunk
preyunk committed Aug 5, 2020
commit c9f15978da4dbb406be72c8379a4c76626f149a0
33 changes: 25 additions & 8 deletions lib/_http_client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,18 @@ const Agent = require('_http_agent');
const { Buffer } = require('buffer');
const { defaultTriggerAsyncIdScope } = require('internal/async_hooks');
const { URL, urlToOptions, searchParamsSymbol } = require('internal/url');
const { kOutHeaders, kNeedDrain, validatePath } = require('internal/http');
const { kOutHeaders,
kNeedDrain,
isValidCONNECTPath
} = require('internal/http');

@bnoordhuis bnoordhuis Aug 6, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const { kOutHeaders,
kNeedDrain,
isValidCONNECTPath
} = require('internal/http');
const {
kOutHeaders,
kNeedDrain,
isValidCONNECTPath,
} = require('internal/http');

const { connResetException, codes } = require('internal/errors');
const {
ERR_HTTP_HEADERS_SENT,
ERR_INVALID_ARG_TYPE,
ERR_INVALID_HTTP_TOKEN,
ERR_INVALID_PROTOCOL,
ERR_UNESCAPED_CHARACTERS
ERR_UNESCAPED_CHARACTERS,
ERR_INVALID_ARG_VALUE
} = codes;
const { validateInteger } = require('internal/validators');
const { getTimerDuration } = require('internal/timers');
Expand Down Expand Up @@ -197,12 +201,25 @@ function ClientRequest(input, options, cb) {
}
this.insecureHTTPParser = insecureHTTPParser;

this.path = (options.path && method === 'CONNECT' &&
options.path.charAt(0) === '/' ?
options.path.slice(1) : options.path) || '/';

if (method === 'CONNECT' && this.path !== '/') {
validatePath(this.path);
path = options.path;
if (path) {
if (method === 'CONNECT') {
if (path.charAt(0) === '/') {

@bnoordhuis bnoordhuis Aug 6, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (path.charAt(0) === '/') {
if (path.startsWith('/')) {

Arguably a little more idiomatic for core code.

path = path.slice(1) || '/';
if (!isValidCONNECTPath(path)) {
throw new ERR_INVALID_ARG_VALUE('options.path',
path,
'must be a valid host:port combo');
}
this.path = path;
} else {
this.path = path;
}
} else {
this.path = path;
}
} else {
this.path = '/';
}

if (cb) {
Expand Down
18 changes: 6 additions & 12 deletions lib/internal/http.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,8 @@ const {

const { setUnrefTimeout } = require('internal/timers');
const { PerformanceEntry, notify } = internalBinding('performance');
const {
codes: {
ERR_INVALID_ARG_VALUE
}
} = require('internal/errors');

const VALID_PATH_REGEX = /^[_0-9A-Za-z]+(?:\.[_0-9A-Za-z]+)*\.?:(?:[1-9]|[1-9][0-9]{1,2}|[1-5][0-9]{3}|6[0-4][0-9]{3}|65[0-4][0-9]{2}|655[0-2][0-9]|6553[0-5])$/;

@rickyes rickyes Aug 18, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Benchmarks tests may be required to assess whether performance has been affected.

@preyunk preyunk Aug 19, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rickyes Could you guide me more about these benchmark tests, as how to write one?

@rickyes rickyes Aug 19, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@preyunk Usually there is no need to write additional benchmarks code, you can use existing benchmarks for testing, refer to the document https://github.com/nodejs/node/blob/master/doc/guides/writing-and-running-benchmarks.md

@jasnell jasnell Aug 19, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex here does not properly account for the possibility of IPv6 addresses. For instance, [2001:db8::1]:111 should be a valid path but fails this check. A likely better approach for the validation check would be to attempt to create a URL from the path:

new URL(`http://${path}`)

If creating the URL is successful, check that the hostname and port are appropriately set, then continue.

@szmarczak szmarczak Aug 19, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also you need to check that http://${url.host} is url.href to disallow asdf.com:1234/asdf etc.

let nowCache;
let utcCache;

Expand All @@ -37,13 +33,11 @@ function resetCache() {
utcCache = undefined;
}

function validatePath(path) {
const pathRegex = /^[_0-9A-Za-z]+(\.[_0-9A-Za-z]+)*\.?:([1-9]|[1-9][0-9]{1,2}|[1-5][0-9]{3}|6[0-4][0-9]{3}|65[0-4][0-9]{2}|655[0-2][0-9]|6553[0-5])$/;
if (!pathRegex.test(path)) {
throw new ERR_INVALID_ARG_VALUE('options.path',
path, 'must be a valid host:port combo');
function isValidCONNECTPath(path) {

@rickyes rickyes Aug 18, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better to use the Camel-Case, e.g. isValidConnectPath.

if (!VALID_PATH_REGEX.test(path)) {
return false;
}
return path;
return true;
}

class HttpRequestTiming extends PerformanceEntry {
Expand All @@ -68,5 +62,5 @@ module.exports = {
nowDate,
utcDate,
emitStatistics,
validatePath
isValidCONNECTPath
};
11 changes: 2 additions & 9 deletions test/parallel/test-http-request-connect-method.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
const common = require('../common');
const assert = require('assert');
const http = require('http');
const { validatePath } = require('internal/http');
const { isValidCONNECTPath } = require('internal/http');

const server = http.createServer();

Expand Down Expand Up @@ -38,12 +38,5 @@ server.on('listening', common.mustCall(() => {
}));

['example.com', 'example.com:0', 'example.com:65536'].forEach((path) => {
Comment thread
jasnell marked this conversation as resolved.
Outdated
assert.throws(
() => validatePath(path),
{
code: 'ERR_INVALID_ARG_VALUE',
name: 'TypeError',
message: /^The argument 'options\.path' must be a valid host:port combo\. Received .+$/
}
);
assert.strictEqual(isValidCONNECTPath(path), false);
});