Commit 84e7388
url: improve port validation
If a port is not a number, throw rather than treating the `:` that
delineates the port as part of the path. This is consistent with WHATWG
URL and also mitigates hostname-spoofing.
Concerns about hostname-spoofing were raised and presented in excellent
detail by pyozzi-toss (pyozzi@toss.im/Security-Tech Team in Toss).
PR-URL: #45012
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>1 parent d47f832 commit 84e7388
3 files changed
Lines changed: 18 additions & 18 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -383,7 +383,7 @@ Url.prototype.parse = function parse(url, parseQueryString, slashesDenoteHost) { | |||
| 383 | 383 | ||
| 384 | 384 | // validate a little. | |
| 385 | 385 | if (!ipv6Hostname) { | |
| 386 | - rest = getHostname(this, rest, hostname); | ||
| 386 | + rest = getHostname(this, rest, hostname, url); | ||
| 387 | 387 | } | |
| 388 | 388 | ||
| 389 | 389 | if (this.hostname.length > hostnameMaxLen) { | |
@@ -502,7 +502,7 @@ Url.prototype.parse = function parse(url, parseQueryString, slashesDenoteHost) { | |||
| 502 | 502 | return this; | |
| 503 | 503 | }; | |
| 504 | 504 | ||
| 505 | - function getHostname(self, rest, hostname) { | ||
| 505 | + function getHostname(self, rest, hostname, url) { | ||
| 506 | 506 | for (let i = 0; i < hostname.length; ++i) { | |
| 507 | 507 | const code = hostname.charCodeAt(i); | |
| 508 | 508 | const isValid = (code !== CHAR_FORWARD_SLASH && | |
@@ -512,6 +512,10 @@ function getHostname(self, rest, hostname) { | |||
| 512 | 512 | code !== CHAR_COLON); | |
| 513 | 513 | ||
| 514 | 514 | if (!isValid) { | |
| 515 | + // If leftover starts with :, then it represents an invalid port. | ||
| 516 | + if (hostname.charCodeAt(i) === 58) { | ||
| 517 | + throw new ERR_INVALID_URL(url); | ||
| 518 | + } | ||
| 515 | 519 | self.hostname = hostname.slice(0, i); | |
| 516 | 520 | return `/${hostname.slice(i)}${rest}`; | |
| 517 | 521 | } | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -865,22 +865,6 @@ const parseTests = { | |||
| 865 | 865 | href: 'http://a%0D%22%20%09%0A%3C\'b:b@c/%0D%0Ad/e?f' | |
| 866 | 866 | }, | |
| 867 | 867 | ||
| 868 | - // Git urls used by npm | ||
| 869 | - 'git+ssh://git@github.com:npm/npm': { | ||
| 870 | - protocol: 'git+ssh:', | ||
| 871 | - slashes: true, | ||
| 872 | - auth: 'git', | ||
| 873 | - host: 'github.com', | ||
| 874 | - port: null, | ||
| 875 | - hostname: 'github.com', | ||
| 876 | - hash: null, | ||
| 877 | - search: null, | ||
| 878 | - query: null, | ||
| 879 | - pathname: '/:npm/npm', | ||
| 880 | - path: '/:npm/npm', | ||
| 881 | - href: 'git+ssh://git@github.com/:npm/npm' | ||
| 882 | - }, | ||
| 883 | - | ||
| 884 | 868 | 'https://*': { | |
| 885 | 869 | protocol: 'https:', | |
| 886 | 870 | slashes: true, | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -74,3 +74,15 @@ if (common.hasIntl) { | |||
| 74 | 74 | (e) => e.code === 'ERR_INVALID_URL', | |
| 75 | 75 | 'parsing http://\u00AD/bad.com/'); | |
| 76 | 76 | } | |
| 77 | + | ||
| 78 | + { | ||
| 79 | + const badURLs = [ | ||
| 80 | + 'https://evil.com:.example.com', | ||
| 81 | + 'git+ssh://git@github.com:npm/npm', | ||
| 82 | + ]; | ||
| 83 | + badURLs.forEach((badURL) => { | ||
| 84 | + assert.throws(() => { url.parse(badURL); }, | ||
| 85 | + (e) => e.code === 'ERR_INVALID_URL', | ||
| 86 | + `parsing ${badURL}`); | ||
| 87 | + }); | ||
| 88 | + } | ||
0 commit comments