Fixed invalid path in CONNECT method requests. by preyunk · Pull Request #34412 · nodejs/node

preyunk · 2020-07-17T15:47:15Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

preyunk · 2020-07-17T17:12:06Z

@szmarczak could you have a look at this?

szmarczak · 2020-07-18T01:59:01Z

Should options.path be required when the method is CONNECT?

Two things:

Don't modify options.path - the options object can be owned by the caller, not by this method; mutations will be visible.

CONNECT requires a <host>:<port> combo. Chopping off the leading slash is the first step but it should also check it's a valid combo.

I don't think we have a utility function for that right now and checking if new URL(`http://${options.path}`) throws isn't robust (accepts e.g. evil.com:666/good.org:777.)

I think the regex would look something like this...

^[\-_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])$

(I hope I'm not introducing catastrophic backtracing here...)

It should match inputs like example.com:80 but also example.com.:80 (FQDN), example:80, example.:80, etc., but not e.g. example.com:0 or example.com:65536.

Should options.path be required when the method is CONNECT?

@szmarczak
Apparently yes, there was a test in which options.path is undefined so charAt throws an error.

@bnoordhuis

Don't modify options.path - the options object can be owned by the caller, not by this method; mutations will be visible.

Okay, I'll try avoiding that.

I don't think we have a utility function for that right now and checking if new URL(http://${options.path}) throws isn't robust (accepts e.g. evil.com:666/good.org:777.)

Should we include this check as a utility function in url.js or must check for a valid <host>:<port> here in _http_client.js?
Also it would require some additional tests right?

It certainly shouldn't be exposed as a public function, if that is what you're asking.

An internal function in _http_client.js is okay but if you add it to lib/internal/http.js, you can unit-test it if you add a // Flags: --expose-internals magic comment to the test file.

preyunk · 2020-07-25T22:03:38Z

Gentle ping @bnoordhuis

szmarczak · 2020-07-26T08:03:02Z

Does this sound good? Maybe validateConnectPath? Or validateAuthority?

szmarczak · 2020-07-26T08:04:19Z

Why not

Suggested change

if (method === 'CONNECT' && this.path !== '/') {

if (method === 'CONNECT') {

szmarczak · 2020-07-26T08:10:04Z

This is much cleaner:

if (options.path) { if (method === 'CONNECT') { if (options.path.charAt(0) === '/') { this.path = options.path.slice(1); } else { this.path = options.path; } } else { this.path = options.path; } } else { this.path = '/'; }

Agreed, and as a micro-optimization: assign to a let path first and then do this.path = path; outside the if block.

(Ask me why if you want details but it has to do with how V8 stores object properties.)

@bnoordhuis Do you mean this?

const {path} = options; if (path) { if (method === 'CONNECT') { if (path.charAt(0) === '/') { this.path = path.slice(1); } else { this.path = path; } } else { this.path = path; } } else { this.path = '/'; }

Agreed, and as a micro-optimization: assign to a let path first and then do this.path = path; outside the if block.

(Ask me why if you want details but it has to do with how V8 stores object properties.)

Yes, I was about to ask the reason for this, could you elaborate as why we are avoiding the direct use of options.path?

const {path} = options;

There is already a let path variable declared in _http_client.js. I think it must be reused.

bnoordhuis · 2020-07-27T08:57:30Z

Agreed, and as a micro-optimization: assign to a let path first and then do this.path = path; outside the if block.

(Ask me why if you want details but it has to do with how V8 stores object properties.)

bnoordhuis · 2020-07-27T08:59:19Z

It might be worth checking if it matters performance-wise when you move the regex outside the function.

The regex should use (?:...) non-capturing groups, otherwise it pollutes RegExp.$1, etc.

bnoordhuis · 2020-07-27T09:04:52Z

I'd change this to simply return true/false and have the caller throw the exception (and rename to e.g. isValidCONNECTPath().)

Rule of thumb: exceptions should be thrown close to the edge, where "edge" means the cross-over point between user code and core code.

Fixes: nodejs#34347

Added tests Fixes: nodejs#34347

bnoordhuis

Left some comments but mostly LGTM.

@jasnell I'm going to dismiss your review because this PR has gone through substantial changes and I think it needs at least one other review before it can land.

bnoordhuis · 2020-08-06T08:25:48Z

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


Suggested change

const { kOutHeaders,

kNeedDrain,

isValidCONNECTPath

} = require('internal/http');

const {

kOutHeaders,

kNeedDrain,

isValidCONNECTPath,

} = require('internal/http');

bnoordhuis · 2020-08-06T08:29:19Z

+  path = options.path;
+  if (path) {
+    if (method === 'CONNECT') {
+      if (path.charAt(0) === '/') {


Suggested change

if (path.charAt(0) === '/') {

if (path.startsWith('/')) {

Arguably a little more idiomatic for core code.

bnoordhuis · 2020-08-06T08:31:37Z

+        this.path = path;
+      } else {
+        this.path = path;
+      }
+    } else {
+      this.path = path;
+    }
+  } else {
+    this.path = '/';
+  }


Can you assign to this.path just once outside the if block? Every this.path = ... forces V8 to emit a type check that this is an object of the expected shape.

V8 can hoist and coalesce such checks but... it's complicated. Just assign once, please. :-)

bnoordhuis · 2020-08-06T08:32:27Z

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


Suggested change

if (!VALID_PATH_REGEX.test(path)) {

return false;

}

return true;

return VALID_PATH_REGEX.test(path));

for old version of pr

szmarczak · 2020-08-06T11:08:01Z

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


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

preyunk · 2020-08-16T10:11:09Z

Ping @bnoordhuis

rickyes · 2020-08-18T15:39:26Z

  utcCache = undefined;
 }

+function isValidCONNECTPath(path) {


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

rickyes · 2020-08-18T15:44:54Z

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


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

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

@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

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.

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

aduh95 · 2023-09-19T14:46:08Z

This would need a rebase if we still want to land it.

github-actions · 2023-09-19T14:46:27Z

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

github-actions · 2023-11-13T00:02:10Z

Closing this because it has stalled. Feel free to reopen if this issue/PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jul 17, 2020

jasnell reviewed Jul 17, 2020

View reviewed changes

Comment thread test/parallel/test-http-request-connect-method.js Outdated

szmarczak reviewed Jul 18, 2020

View reviewed changes

preyunk requested review from bnoordhuis and jasnell July 21, 2020 20:59

jasnell reviewed Jul 21, 2020

View reviewed changes

Comment thread doc/api/errors.md Outdated

jasnell reviewed Jul 21, 2020

View reviewed changes

Comment thread test/parallel/test-http-request-connect-method.js Outdated

preyunk requested a review from jasnell July 22, 2020 14:52

jasnell previously approved these changes Jul 22, 2020

View reviewed changes

szmarczak reviewed Jul 26, 2020

View reviewed changes

bnoordhuis reviewed Jul 27, 2020

View reviewed changes

preyunk added 7 commits August 5, 2020 20:34

http: fixed invalid path with connect method

edd8f6e

Fixes: nodejs#34347

http: fixed invalid path with connect method

0825564

Added tests Fixes: nodejs#34347

Fixed linting

247401a

checked for undefined options.path

989cecd

Added check for valid host:port path

e1283be

Fixes

669ee65

fixes

c9f1597

preyunk force-pushed the connect-pathname branch from e7d3f83 to c9f1597 Compare August 5, 2020 15:10

preyunk requested a review from bnoordhuis August 5, 2020 15:11

bnoordhuis reviewed Aug 6, 2020

View reviewed changes

minor fixes

d8039e3

preyunk requested review from bnoordhuis and jasnell August 6, 2020 10:33

szmarczak reviewed Aug 6, 2020

View reviewed changes

preyunk requested a review from a team as a code owner August 10, 2020 16:06

rickyes reviewed Aug 18, 2020

View reviewed changes

removed regex

9426cbb

preyunk requested review from rickyes and removed request for a team September 6, 2020 17:38

guybedford force-pushed the master branch from dc5a5da to 8e46568 Compare March 29, 2021 21:33

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

aduh95 added the stalled Issues and PRs that are stalled. label Sep 19, 2023

github-actions Bot closed this Nov 13, 2023

efekrskl mentioned this pull request Jul 31, 2026

http: normalize CONNECT request paths #64876

Open

	if (method === 'CONNECT' && this.path !== '/') {
	if (method === 'CONNECT') {

Uh oh!

Uh oh!

Conversation

preyunk commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

preyunk commented Jul 17, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

preyunk Jul 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

preyunk commented Jul 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szmarczak Jul 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

preyunk Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

preyunk commented Aug 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

preyunk Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduh95 commented Sep 19, 2023

Uh oh!

github-actions Bot commented Sep 19, 2023

preyunk commented Jul 17, 2020 •

edited

Loading

preyunk Jul 18, 2020 •

edited

Loading

szmarczak Jul 26, 2020 •

edited

Loading

preyunk Jul 28, 2020 •

edited

Loading

preyunk Aug 19, 2020 •

edited

Loading