Skip to content
Closed
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
26 changes: 23 additions & 3 deletions lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,19 @@ 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 } = 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,7 +202,22 @@ function ClientRequest(input, options, cb) {
}
this.insecureHTTPParser = insecureHTTPParser;

this.path = options.path || '/';
path = options.path;
if (path) {
if (method === 'CONNECT') {
if (path.startsWith('/')) {

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.

path[0] === '/' is much faster

// if path is '/'
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 || '/';

if (cb) {
this.once('response', cb);
}
Expand Down
8 changes: 7 additions & 1 deletion lib/internal/http.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const {
const { setUnrefTimeout } = require('internal/timers');
const { PerformanceEntry, notify } = internalBinding('performance');

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])$/;

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 •

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?

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

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.

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.

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 @@ -32,6 +33,10 @@ function resetCache() {
utcCache = undefined;
}

function isValidCONNECTPath(path) {

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.

return VALID_PATH_REGEX.test(path);
}

class HttpRequestTiming extends PerformanceEntry {
constructor(statistics) {
super();
Expand All @@ -53,5 +58,6 @@ module.exports = {
kNeedDrain: Symbol('kNeedDrain'),
nowDate,
utcDate,
emitStatistics
emitStatistics,
isValidCONNECTPath
};
42 changes: 42 additions & 0 deletions test/parallel/test-http-request-connect-method.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// Flags: --expose-internals

'use strict';

const common = require('../common');
const assert = require('assert');
const http = require('http');
const { isValidCONNECTPath } = require('internal/http');

const server = http.createServer();

server.on('connect', common.mustCall((req, stream) => {
assert.strictEqual(req.url, 'example.com:80');
stream.end('HTTP/1.1 501 Not Implemented\r\n\r\n');
}));

server.listen(0);

server.on('listening', common.mustCall(() => {
const url = new URL(`http://localhost:${server.address().port}/example.com:80`);
let req = http.request(url, { method: 'CONNECT' }).end();
// invalid path
const invalidPathURL = new URL(`http://localhost:${server.address().port}/example.com`);
assert.throws(
() => {
req = http.request(invalidPathURL, { method: 'CONNECT' }).end();
},
{
code: 'ERR_INVALID_ARG_VALUE',
name: 'TypeError',
message: /^The argument 'options\.path' must be a valid host:port combo\. Received .+$/
}
);
req.once('connect', common.mustCall((res) => {
res.destroy();
server.close();
}));
}));

['example.com', 'example.com:0', 'example.com:65536'].forEach((path) => {
Comment thread
jasnell marked this conversation as resolved.
Outdated
assert.strictEqual(isValidCONNECTPath(path), false);
});