From d2994c943d32a8390167460771b952e61c744364 Mon Sep 17 00:00:00 2001 From: Timothy Gu Date: Tue, 13 Nov 2018 15:53:35 -0800 Subject: [PATCH] url: reuse existing context in href setter Correctness-wise, this removes side effects in the href setter if parsing fails. Style-wise, this allows removing the parse() wrapper function around C++ _parse(). Also fix an existing bug with whitespace trimming in C++ that was previously circumvented by additionally trimming the input in JavaScript. Fixes: https://github.com/nodejs/node/issues/24345 Refs: https://github.com/nodejs/node/pull/24218#issuecomment-438476591 PR-URL: https://github.com/nodejs/node/pull/24495 Reviewed-By: Anna Henningsen Reviewed-By: Joyee Cheung Reviewed-By: Gus Caplan Reviewed-By: Franziska Hinkelmann Reviewed-By: Daijiro Wachi Reviewed-By: Ruben Bridgewater Reviewed-By: Colin Ihrig Reviewed-By: James M Snell --- lib/internal/url.js | 38 +++++++++---------- src/node_url.cc | 1 + ...test-whatwg-url-custom-href-side-effect.js | 15 ++++++++ 3 files changed, 33 insertions(+), 21 deletions(-) create mode 100644 test/parallel/test-whatwg-url-custom-href-side-effect.js diff --git a/lib/internal/url.js b/lib/internal/url.js index 965f647c2784dd..a3d155efea4003 100644 --- a/lib/internal/url.js +++ b/lib/internal/url.js @@ -43,7 +43,7 @@ const { domainToUnicode: _domainToUnicode, encodeAuth, toUSVString: _toUSVString, - parse: _parse, + parse, setURLConstructor, URL_FLAGS_CANNOT_BE_BASE, URL_FLAGS_HAS_FRAGMENT, @@ -243,14 +243,6 @@ function onParseError(flags, input) { throw error; } -// Reused by URL constructor and URL#href setter. -function parse(url, input, base) { - const base_context = base ? base[context] : undefined; - url[context] = new URLContext(); - _parse(input.trim(), -1, base_context, undefined, - onParseComplete.bind(url), onParseError); -} - function onParseProtocolComplete(flags, protocol, username, password, host, port, path, query, fragment) { const ctx = this[context]; @@ -319,10 +311,13 @@ class URL { constructor(input, base) { // toUSVString is not needed. input = `${input}`; + let base_context; if (base !== undefined) { - base = new URL(base); + base_context = new URL(base)[context]; } - parse(this, input, base); + this[context] = new URLContext(); + parse(input, -1, base_context, undefined, onParseComplete.bind(this), + onParseError); } get [special]() { @@ -445,7 +440,8 @@ Object.defineProperties(URL.prototype, { set(input) { // toUSVString is not needed. input = `${input}`; - parse(this, input); + parse(input, -1, undefined, undefined, onParseComplete.bind(this), + onParseError); } }, origin: { // readonly @@ -491,8 +487,8 @@ Object.defineProperties(URL.prototype, { (ctx.host === '' || ctx.host === null)) { return; } - _parse(scheme, kSchemeStart, null, ctx, - onParseProtocolComplete.bind(this)); + parse(scheme, kSchemeStart, null, ctx, + onParseProtocolComplete.bind(this)); } }, username: { @@ -555,7 +551,7 @@ Object.defineProperties(URL.prototype, { // Cannot set the host if cannot-be-base is set return; } - _parse(host, kHost, null, ctx, onParseHostComplete.bind(this)); + parse(host, kHost, null, ctx, onParseHostComplete.bind(this)); } }, hostname: { @@ -572,7 +568,7 @@ Object.defineProperties(URL.prototype, { // Cannot set the host if cannot-be-base is set return; } - _parse(host, kHostname, null, ctx, onParseHostnameComplete.bind(this)); + parse(host, kHostname, null, ctx, onParseHostnameComplete.bind(this)); } }, port: { @@ -592,7 +588,7 @@ Object.defineProperties(URL.prototype, { ctx.port = null; return; } - _parse(port, kPort, null, ctx, onParsePortComplete.bind(this)); + parse(port, kPort, null, ctx, onParsePortComplete.bind(this)); } }, pathname: { @@ -611,8 +607,8 @@ Object.defineProperties(URL.prototype, { path = `${path}`; if (this[cannotBeBase]) return; - _parse(path, kPathStart, null, this[context], - onParsePathComplete.bind(this)); + parse(path, kPathStart, null, this[context], + onParsePathComplete.bind(this)); } }, search: { @@ -635,7 +631,7 @@ Object.defineProperties(URL.prototype, { ctx.query = ''; ctx.flags |= URL_FLAGS_HAS_QUERY; if (search) { - _parse(search, kQuery, null, ctx, onParseSearchComplete.bind(this)); + parse(search, kQuery, null, ctx, onParseSearchComplete.bind(this)); } } initSearchParams(this[searchParams], search); @@ -669,7 +665,7 @@ Object.defineProperties(URL.prototype, { if (hash[0] === '#') hash = hash.slice(1); ctx.fragment = ''; ctx.flags |= URL_FLAGS_HAS_FRAGMENT; - _parse(hash, kFragment, null, ctx, onParseHashComplete.bind(this)); + parse(hash, kFragment, null, ctx, onParseHashComplete.bind(this)); } }, toJSON: { diff --git a/src/node_url.cc b/src/node_url.cc index 164b1d3787432c..dfff0c1fbc0a81 100644 --- a/src/node_url.cc +++ b/src/node_url.cc @@ -1375,6 +1375,7 @@ void URL::Parse(const char* input, else break; } + input = p; len = end - p; } diff --git a/test/parallel/test-whatwg-url-custom-href-side-effect.js b/test/parallel/test-whatwg-url-custom-href-side-effect.js new file mode 100644 index 00000000000000..f161f6bbe9546e --- /dev/null +++ b/test/parallel/test-whatwg-url-custom-href-side-effect.js @@ -0,0 +1,15 @@ +'use strict'; + +// Tests below are not from WPT. +const common = require('../common'); +const assert = require('assert'); + +const ref = new URL('http://example.com/path'); +const url = new URL('http://example.com/path'); +common.expectsError(() => { + url.href = ''; +}, { + type: TypeError +}); + +assert.deepStrictEqual(url, ref);