Skip to content

Commit 7048cba

Browse files
mikesamuelBethGriggs
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 <joyeec9h3@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 691e1a6 commit 7048cba

1 file changed

Lines changed: 33 additions & 30 deletions

File tree

‎lib/url.js‎

Lines changed: 33 additions & 30 deletions
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
consthostnameMaxLen=255;
7880
// protocols that can allow "unsafe" and "unwise" chars.
79-
constunsafeProtocol={
80-
'javascript': true,
81-
'javascript:': true
82-
};
81+
constunsafeProtocol=newSafeSet([
82+
'javascript',
83+
'javascript:'
84+
]);
8385
// protocols that never have a hostname.
84-
consthostlessProtocol={
85-
'javascript': true,
86-
'javascript:': true
87-
};
86+
consthostlessProtocol=newSafeSet([
87+
'javascript',
88+
'javascript:'
89+
]);
8890
// protocols that always contain a // bit.
89-
constslashedProtocol={
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+
constslashedProtocol=newSafeSet([
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
varslashes=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
}elseif(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
varkeys=Object.keys(relative);
724726
for(varv=0;v<keys.length;v++){
725727
vark=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
constrelPath=(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
varremoveAllDots=mustEndAbs;
770772
varsrcPath=result.pathname&&result.pathname.split('/')||[];
771773
varrelPath=relative.pathname&&relative.pathname.split('/')||[];
772-
varnoLeadingSlashes=result.protocol&&!slashedProtocol[result.protocol];
774+
varnoLeadingSlashes=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)