Skip to content

Commit

Permalink
url: prioritize toString when stringifying
Browse files Browse the repository at this point in the history
The ES addition operator calls the ToPrimitive() abstract operation
without hint String, leading a subsequent OrdinaryToPrimitive() to call
valueOf() first on an object rather than the desired toString().

Instead, use template literals which directly call ToString() abstract
operation, per Web IDL spec.

PR-URL: #12507
Fixes: b610a4d "url: enforce valid UTF-8 in WHATWG parser"
Refs: b610a4d#commitcomment-21200056
Refs: https://tc39.github.io/ecma262/#sec-addition-operator-plus-runtime-semantics-evaluation
Refs: https://tc39.github.io/ecma262/#sec-template-literals-runtime-semantics-evaluation
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
TimothyGu authored and evanlucas committed May 1, 2017
1 parent 7c9ca0f commit d6fe916
Show file tree
Hide file tree
Showing 9 changed files with 45 additions and 21 deletions.
26 changes: 13 additions & 13 deletions lib/internal/url.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ const IteratorPrototype = Object.getPrototypeOf(
const unpairedSurrogateRe =
/([^\uD800-\uDBFF]|^)[\uDC00-\uDFFF]|[\uD800-\uDBFF](?![\uDC00-\uDFFF])/;
function toUSVString(val) {
const str = '' + val;
const str = `${val}`;
// As of V8 5.5, `str.search()` (and `unpairedSurrogateRe[@@search]()`) are
// slower than `unpairedSurrogateRe.exec()`.
const match = unpairedSurrogateRe.exec(str);
Expand Down Expand Up @@ -215,7 +215,7 @@ function onParseHashComplete(flags, protocol, username, password,
class URL {
constructor(input, base) {
// toUSVString is not needed.
input = '' + input;
input = `${input}`;
if (base !== undefined && !(base instanceof URL))
base = new URL(base);
parse(this, input, base);
Expand Down Expand Up @@ -326,7 +326,7 @@ Object.defineProperties(URL.prototype, {
},
set(input) {
// toUSVString is not needed.
input = '' + input;
input = `${input}`;
parse(this, input);
}
},
Expand All @@ -345,7 +345,7 @@ Object.defineProperties(URL.prototype, {
},
set(scheme) {
// toUSVString is not needed.
scheme = '' + scheme;
scheme = `${scheme}`;
if (scheme.length === 0)
return;
binding.parse(scheme, binding.kSchemeStart, null, this[context],
Expand All @@ -360,7 +360,7 @@ Object.defineProperties(URL.prototype, {
},
set(username) {
// toUSVString is not needed.
username = '' + username;
username = `${username}`;
if (!this.hostname)
return;
const ctx = this[context];
Expand All @@ -381,7 +381,7 @@ Object.defineProperties(URL.prototype, {
},
set(password) {
// toUSVString is not needed.
password = '' + password;
password = `${password}`;
if (!this.hostname)
return;
const ctx = this[context];
Expand All @@ -407,7 +407,7 @@ Object.defineProperties(URL.prototype, {
set(host) {
const ctx = this[context];
// toUSVString is not needed.
host = '' + host;
host = `${host}`;
if (this[cannotBeBase] ||
(this[special] && host.length === 0)) {
// Cannot set the host if cannot-be-base is set or
Expand All @@ -432,7 +432,7 @@ Object.defineProperties(URL.prototype, {
set(host) {
const ctx = this[context];
// toUSVString is not needed.
host = '' + host;
host = `${host}`;
if (this[cannotBeBase] ||
(this[special] && host.length === 0)) {
// Cannot set the host if cannot-be-base is set or
Expand All @@ -457,7 +457,7 @@ Object.defineProperties(URL.prototype, {
},
set(port) {
// toUSVString is not needed.
port = '' + port;
port = `${port}`;
const ctx = this[context];
if (!ctx.host || this[cannotBeBase] ||
this.protocol === 'file:')
Expand All @@ -481,7 +481,7 @@ Object.defineProperties(URL.prototype, {
},
set(path) {
// toUSVString is not needed.
path = '' + path;
path = `${path}`;
if (this[cannotBeBase])
return;
binding.parse(path, binding.kPathStart, null, this[context],
Expand Down Expand Up @@ -530,7 +530,7 @@ Object.defineProperties(URL.prototype, {
set(hash) {
const ctx = this[context];
// toUSVString is not needed.
hash = '' + hash;
hash = `${hash}`;
if (this.protocol === 'javascript:')
return;
if (!hash) {
Expand Down Expand Up @@ -1122,12 +1122,12 @@ function originFor(url, base) {

function domainToASCII(domain) {
// toUSVString is not needed.
return binding.domainToASCII('' + domain);
return binding.domainToASCII(`${domain}`);
}

function domainToUnicode(domain) {
// toUSVString is not needed.
return binding.domainToUnicode('' + domain);
return binding.domainToUnicode(`${domain}`);
}

// Utility function that converts a URL object into an ordinary
Expand Down
5 changes: 4 additions & 1 deletion test/parallel/test-whatwg-url-searchparams-append.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,10 @@ test(function() {
params.set('a');
}, /^TypeError: "name" and "value" arguments must be specified$/);

const obj = { toString() { throw new Error('toString'); } };
const obj = {
toString() { throw new Error('toString'); },
valueOf() { throw new Error('valueOf'); }
};
const sym = Symbol();
assert.throws(() => params.set(obj, 'b'), /^Error: toString$/);
assert.throws(() => params.set('a', obj), /^Error: toString$/);
Expand Down
5 changes: 4 additions & 1 deletion test/parallel/test-whatwg-url-searchparams-constructor.js
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,10 @@ test(() => {
}

{
const obj = { toString() { throw new Error('toString'); } };
const obj = {
toString() { throw new Error('toString'); },
valueOf() { throw new Error('valueOf'); }
};
const sym = Symbol();

assert.throws(() => new URLSearchParams({ a: obj }), /^Error: toString$/);
Expand Down
5 changes: 4 additions & 1 deletion test/parallel/test-whatwg-url-searchparams-delete.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,10 @@ test(function() {
params.delete();
}, /^TypeError: "name" argument must be specified$/);

const obj = { toString() { throw new Error('toString'); } };
const obj = {
toString() { throw new Error('toString'); },
valueOf() { throw new Error('valueOf'); }
};
const sym = Symbol();
assert.throws(() => params.delete(obj), /^Error: toString$/);
assert.throws(() => params.delete(sym),
Expand Down
5 changes: 4 additions & 1 deletion test/parallel/test-whatwg-url-searchparams-get.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,10 @@ test(function() {
params.get();
}, /^TypeError: "name" argument must be specified$/);

const obj = { toString() { throw new Error('toString'); } };
const obj = {
toString() { throw new Error('toString'); },
valueOf() { throw new Error('valueOf'); }
};
const sym = Symbol();
assert.throws(() => params.get(obj), /^Error: toString$/);
assert.throws(() => params.get(sym),
Expand Down
5 changes: 4 additions & 1 deletion test/parallel/test-whatwg-url-searchparams-getall.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,10 @@ test(function() {
params.getAll();
}, /^TypeError: "name" argument must be specified$/);

const obj = { toString() { throw new Error('toString'); } };
const obj = {
toString() { throw new Error('toString'); },
valueOf() { throw new Error('valueOf'); }
};
const sym = Symbol();
assert.throws(() => params.getAll(obj), /^Error: toString$/);
assert.throws(() => params.getAll(sym),
Expand Down
5 changes: 4 additions & 1 deletion test/parallel/test-whatwg-url-searchparams-has.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,10 @@ test(function() {
params.has();
}, /^TypeError: "name" argument must be specified$/);

const obj = { toString() { throw new Error('toString'); } };
const obj = {
toString() { throw new Error('toString'); },
valueOf() { throw new Error('valueOf'); }
};
const sym = Symbol();
assert.throws(() => params.has(obj), /^Error: toString$/);
assert.throws(() => params.has(sym),
Expand Down
5 changes: 4 additions & 1 deletion test/parallel/test-whatwg-url-searchparams-set.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,10 @@ test(function() {
params.set('a');
}, /^TypeError: "name" and "value" arguments must be specified$/);

const obj = { toString() { throw new Error('toString'); } };
const obj = {
toString() { throw new Error('toString'); },
valueOf() { throw new Error('valueOf'); }
};
const sym = Symbol();
assert.throws(() => params.append(obj, 'b'), /^Error: toString$/);
assert.throws(() => params.append('a', obj), /^Error: toString$/);
Expand Down
5 changes: 4 additions & 1 deletion test/parallel/test-whatwg-url-setters.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,10 @@ startURLSettersTests()

{
const url = new URL('http://example.com/');
const obj = { toString() { throw new Error('toString'); } };
const obj = {
toString() { throw new Error('toString'); },
valueOf() { throw new Error('valueOf'); }
};
const sym = Symbol();
const props = Object.getOwnPropertyDescriptors(Object.getPrototypeOf(url));
for (const [name, { set }] of Object.entries(props)) {
Expand Down

0 comments on commit d6fe916

Please sign in to comment.