Skip to content

Commit be54dc0

Browse files
mikesamuelBridgeAR
authored andcommitted
url: use SafeSet to filter known special protocols
Avoids a maintenance hazard when reviewers assume that `hostlessProtocol` and `slashedProtocol` are disjoint. The following may be counter-intuitive: ```js // These objects seem to have no keys in common const hostlessProtocol = { 'javascript': true }; const slashedProtocol = { 'http': true }; // A reasonable reviewer may assumes bothTrue is never truthy function bothTrue(lowerProto) { return hostlessProtocol[lowerProto] && slashedProtocol[lowerProto]; } // But console.log(Boolean(bothTrue('constructor'))); // true ``` This change uses SafeSet instead of plain-old objects. ---- Rejected alternative: We could have used object with a `null` prototype as lookup tables so that `lowerProto` is never treated as a key into `Object.prototype`. ```js const hostlessProtocol = { __proto__: null, 'javascript': true }; const slashedProtocol = { __proto__: null, 'http': true }; function bothTrue(lowerProto) { return hostlessProtocol[lowerProto] && slashedProtocol[lowerProto]; } console.log(Boolean(bothTrue('constructor'))); // false ``` PR-URL: #24703 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
1 parent 9e5a79a commit be54dc0

File tree

1 file changed

+33
-30
lines changed

1 file changed

+33
-30
lines changed

lib/url.js

+33-30
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@ const { toASCII } = process.binding('config').hasIntl ?
2626

2727
const { hexTable } = require('internal/querystring');
2828

29+
const { SafeSet } = require('internal/safe_globals');
30+
2931
const {
3032
ERR_INVALID_ARG_TYPE
3133
} = require('internal/errors').codes;
@@ -76,28 +78,28 @@ const simplePathPattern = /^(\/\/?(?!\/)[^?\s]*)(\?[^\s]*)?$/;
7678

7779
const hostnameMaxLen = 255;
7880
// protocols that can allow "unsafe" and "unwise" chars.
79-
const unsafeProtocol = {
80-
'javascript': true,
81-
'javascript:': true
82-
};
81+
const unsafeProtocol = new SafeSet([
82+
'javascript',
83+
'javascript:'
84+
]);
8385
// protocols that never have a hostname.
84-
const hostlessProtocol = {
85-
'javascript': true,
86-
'javascript:': true
87-
};
86+
const hostlessProtocol = new SafeSet([
87+
'javascript',
88+
'javascript:'
89+
]);
8890
// protocols that always contain a // bit.
89-
const slashedProtocol = {
90-
'http': true,
91-
'http:': true,
92-
'https': true,
93-
'https:': true,
94-
'ftp': true,
95-
'ftp:': true,
96-
'gopher': true,
97-
'gopher:': true,
98-
'file': true,
99-
'file:': true
100-
};
91+
const slashedProtocol = new SafeSet([
92+
'http',
93+
'http:',
94+
'https',
95+
'https:',
96+
'ftp',
97+
'ftp:',
98+
'gopher',
99+
'gopher:',
100+
'file',
101+
'file:'
102+
]);
101103
const {
102104
CHAR_SPACE,
103105
CHAR_TAB,
@@ -267,14 +269,14 @@ Url.prototype.parse = function parse(url, parseQueryString, slashesDenoteHost) {
267269
if (slashesDenoteHost || proto || hostPattern.test(rest)) {
268270
var slashes = rest.charCodeAt(0) === CHAR_FORWARD_SLASH &&
269271
rest.charCodeAt(1) === CHAR_FORWARD_SLASH;
270-
if (slashes && !(proto && hostlessProtocol[lowerProto])) {
272+
if (slashes && !(proto && hostlessProtocol.has(lowerProto))) {
271273
rest = rest.slice(2);
272274
this.slashes = true;
273275
}
274276
}
275277

276-
if (!hostlessProtocol[lowerProto] &&
277-
(slashes || (proto && !slashedProtocol[proto]))) {
278+
if (!hostlessProtocol.has(lowerProto) &&
279+
(slashes || (proto && !slashedProtocol.has(proto)))) {
278280

279281
// there's a hostname.
280282
// the first instance of /, ?, ;, or # ends the host.
@@ -400,7 +402,7 @@ Url.prototype.parse = function parse(url, parseQueryString, slashesDenoteHost) {
400402

401403
// now rest is set to the post-host stuff.
402404
// chop off any delim chars.
403-
if (!unsafeProtocol[lowerProto]) {
405+
if (!unsafeProtocol.has(lowerProto)) {
404406
// First, make 100% sure that any "autoEscape" chars get
405407
// escaped, even if encodeURIComponent doesn't think they
406408
// need to be.
@@ -447,7 +449,7 @@ Url.prototype.parse = function parse(url, parseQueryString, slashesDenoteHost) {
447449
} else if (firstIdx > 0) {
448450
this.pathname = rest.slice(0, firstIdx);
449451
}
450-
if (slashedProtocol[lowerProto] &&
452+
if (slashedProtocol.has(lowerProto) &&
451453
this.hostname && !this.pathname) {
452454
this.pathname = '/';
453455
}
@@ -629,7 +631,7 @@ Url.prototype.format = function format() {
629631

630632
// only the slashedProtocols get the //. Not mailto:, xmpp:, etc.
631633
// unless they had them to begin with.
632-
if (this.slashes || slashedProtocol[protocol]) {
634+
if (this.slashes || slashedProtocol.has(protocol)) {
633635
if (this.slashes || host) {
634636
if (pathname && pathname.charCodeAt(0) !== CHAR_FORWARD_SLASH)
635637
pathname = '/' + pathname;
@@ -701,7 +703,7 @@ Url.prototype.resolveObject = function resolveObject(relative) {
701703
}
702704

703705
// urlParse appends trailing / to urls like http://www.example.com
704-
if (slashedProtocol[result.protocol] &&
706+
if (slashedProtocol.has(result.protocol) &&
705707
result.hostname && !result.pathname) {
706708
result.path = result.pathname = '/';
707709
}
@@ -719,7 +721,7 @@ Url.prototype.resolveObject = function resolveObject(relative) {
719721
// if it is file:, then the host is dropped,
720722
// because that's known to be hostless.
721723
// anything else is assumed to be absolute.
722-
if (!slashedProtocol[relative.protocol]) {
724+
if (!slashedProtocol.has(relative.protocol)) {
723725
var keys = Object.keys(relative);
724726
for (var v = 0; v < keys.length; v++) {
725727
var k = keys[v];
@@ -732,7 +734,7 @@ Url.prototype.resolveObject = function resolveObject(relative) {
732734
result.protocol = relative.protocol;
733735
if (!relative.host &&
734736
!/^file:?$/.test(relative.protocol) &&
735-
!hostlessProtocol[relative.protocol]) {
737+
!hostlessProtocol.has(relative.protocol)) {
736738
const relPath = (relative.pathname || '').split('/');
737739
while (relPath.length && !(relative.host = relPath.shift()));
738740
if (!relative.host) relative.host = '';
@@ -769,7 +771,8 @@ Url.prototype.resolveObject = function resolveObject(relative) {
769771
var removeAllDots = mustEndAbs;
770772
var srcPath = result.pathname && result.pathname.split('/') || [];
771773
var relPath = relative.pathname && relative.pathname.split('/') || [];
772-
var noLeadingSlashes = result.protocol && !slashedProtocol[result.protocol];
774+
var noLeadingSlashes = result.protocol &&
775+
!slashedProtocol.has(result.protocol);
773776

774777
// if the url is a non-slashed url, then relative
775778
// links like ../.. should be able

0 commit comments

Comments
 (0)