Skip to content

Commit

Permalink
url: fix treatment of some values as non-empty
Browse files Browse the repository at this point in the history
In addition to null, undefined and the empty string
are treated as empty (removing the component from the url).

The string '#' is treated same as empty values when
setting .hash.

The string '?' is treated same as empty values when
setting .search.

PR-URL: #1589
Fixes: #1588
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
  • Loading branch information
petkaantonov committed May 3, 2015
1 parent dbdd81a commit 6687721
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 22 deletions.
24 changes: 14 additions & 10 deletions lib/url.js
Original file line number Diff line number Diff line change
Expand Up @@ -879,7 +879,7 @@ Object.defineProperty(Url.prototype, 'port', {
return null;
},
set: function(v) {
if (v === null) {
if (isConsideredEmpty(v)) {
this._port = -1;
if (this._host)
this._host = null;
Expand Down Expand Up @@ -941,7 +941,7 @@ Object.defineProperty(Url.prototype, 'path', {
return (p == null && s) ? ('/' + s) : null;
},
set: function(v) {
if (v === null) {
if (isConsideredEmpty(v)) {
this._pathname = this._search = null;
return;
}
Expand Down Expand Up @@ -973,7 +973,7 @@ Object.defineProperty(Url.prototype, 'protocol', {
return proto;
},
set: function(v) {
if (v === null) {
if (isConsideredEmpty(v)) {
this._protocol = null;
} else {
var proto = '' + v;
Expand Down Expand Up @@ -1001,7 +1001,7 @@ Object.defineProperty(Url.prototype, 'href', {
var parsesQueryStrings = this._parsesQueryStrings;
// Reset properties.
Url.call(this);
if (v !== null && v !== '')
if (!isConsideredEmpty(v))
this.parse('' + v, parsesQueryStrings, false);
},
enumerable: true,
Expand All @@ -1013,7 +1013,7 @@ Object.defineProperty(Url.prototype, 'auth', {
return this._auth;
},
set: function(v) {
this._auth = v === null ? null : '' + v;
this._auth = isConsideredEmpty(v) ? null : '' + v;
this._href = '';
},
enumerable: true,
Expand All @@ -1026,7 +1026,7 @@ Object.defineProperty(Url.prototype, 'host', {
return this._host;
},
set: function(v) {
if (v === null) {
if (isConsideredEmpty(v)) {
this._port = -1;
this._hostname = this._host = null;
} else {
Expand All @@ -1053,7 +1053,7 @@ Object.defineProperty(Url.prototype, 'hostname', {
return this._hostname;
},
set: function(v) {
if (v === null) {
if (isConsideredEmpty(v)) {
this._hostname = null;

if (this._hasValidPort())
Expand All @@ -1080,7 +1080,7 @@ Object.defineProperty(Url.prototype, 'hash', {
return this._hash;
},
set: function(v) {
if (v === null) {
if (isConsideredEmpty(v) || v === '#') {
this._hash = null;
} else {
var hash = '' + v;
Expand All @@ -1100,7 +1100,7 @@ Object.defineProperty(Url.prototype, 'search', {
return this._search;
},
set: function(v) {
if (v === null) {
if (isConsideredEmpty(v) || v === '?') {
this._search = this._query = null;
} else {
var search = escapeSearch('' + v);
Expand All @@ -1125,7 +1125,7 @@ Object.defineProperty(Url.prototype, 'pathname', {
return this._pathname;
},
set: function(v) {
if (v === null) {
if (isConsideredEmpty(v)) {
this._pathname = null;
} else {
var pathname = escapePathName('' + v).replace(/\\/g, '/');
Expand All @@ -1144,6 +1144,10 @@ Object.defineProperty(Url.prototype, 'pathname', {
configurable: true
});

function isConsideredEmpty(value) {
return value === null || value === undefined || value === '';
}

// Search `char1` (integer code for a character) in `string`
// starting from `fromIndex` and ending at `string.length - 1`
// or when a stop character is found.
Expand Down
59 changes: 47 additions & 12 deletions test/parallel/test-url-accessors.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,10 @@ const accessorTests = [{
}, {
// Setting href to non-null non-string coerces to string
url: 'google',
set: {href: undefined},
set: {href: 0},
test: {
path: 'undefined',
href: 'undefined'
path: '0',
href: '0'
}
}, {
// Setting port is reflected in host
Expand Down Expand Up @@ -180,8 +180,8 @@ const accessorTests = [{
url: 'http://www.google.com',
set: {search: ''},
test: {
search: '?',
path: '/?'
search: null,
path: '/'
}
}, {

Expand All @@ -203,10 +203,11 @@ const accessorTests = [{
}, {

// Empty hash is ok
url: 'http://www.google.com',
url: 'http://www.google.com#hash',
set: {hash: ''},
test: {
hash: '#'
hash: null,
href: 'http://www.google.com/'
}
}, {

Expand Down Expand Up @@ -252,7 +253,8 @@ const accessorTests = [{
url: 'http://www.google.com',
set: {pathname: ''},
test: {
pathname: '/'
pathname: null,
href: 'http://www.google.com'
}
}, {
// Null path is ok
Expand Down Expand Up @@ -290,11 +292,12 @@ const accessorTests = [{
protocol: null
}
}, {
// Empty protocol is invalid
// Empty protocol is ok
url: 'http://www.google.com/path',
set: {protocol: ''},
test: {
protocol: 'http:'
protocol: null,
href: '//www.google.com/path'
}
}, {
// Set query to an object
Expand Down Expand Up @@ -327,9 +330,9 @@ const accessorTests = [{
url: 'http://www.google.com/path?key=value',
set: {path: '?key2=value2'},
test: {
pathname: '/',
pathname: null,
search: '?key2=value2',
href: 'http://www.google.com/?key2=value2'
href: 'http://www.google.com?key2=value2'
}
}, {
// path is reflected in search and pathname 3
Expand All @@ -349,6 +352,38 @@ const accessorTests = [{
search: null,
href: 'http://www.google.com'
}
}, {
// setting hash to '' removes any hash
url: 'http://www.google.com/#hash',
set: {hash: ''},
test: {
hash: null,
href: 'http://www.google.com/'
}
}, {
// setting hash to '#' removes any hash
url: 'http://www.google.com/#hash',
set: {hash: '#'},
test: {
hash: null,
href: 'http://www.google.com/'
}
}, {
// setting search to '' removes any search
url: 'http://www.google.com/?search',
set: {search: ''},
test: {
search: null,
href: 'http://www.google.com/'
}
}, {
// setting search to '?' removes any search
url: 'http://www.google.com/?search',
set: {search: '?'},
test: {
search: null,
href: 'http://www.google.com/'
}
}

];
Expand Down

0 comments on commit 6687721

Please sign in to comment.