Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

URL.href has side effect if parsing errored #24345

Closed
TimothyGu opened this issue Nov 13, 2018 · 3 comments
Closed

URL.href has side effect if parsing errored #24345

TimothyGu opened this issue Nov 13, 2018 · 3 comments
Labels
whatwg-url Issues and PRs related to the WHATWG URL implementation.

Comments

@TimothyGu
Copy link
Member

  • Version: v11.1.0
  • Platform: macOS
  • Subsystem: url

When passing an invalid URL to URL's href setter, the original URL gets destroyed, in contrary to the spec.

> u = new URL('https://example.com/')
URL {
  href: 'https://example.com/',
  origin: 'https://example.com',
  protocol: 'https:',
  username: '',
  password: '',
  host: 'example.com',
  hostname: 'example.com',
  port: '',
  pathname: '/',
  search: '',
  searchParams: URLSearchParams {},
  hash: '' }
> u.href = ''
{ TypeError [ERR_INVALID_URL]: Invalid URL: 
    at onParseError (internal/url.js:240:17)
    at parse (internal/url.js:249:3)
    at URL.set [as href] (internal/url.js:447:7) input: '' }
> u
URL {
  href: ':',
  origin: 'null',
  protocol: ':',
  username: '',
  password: '',
  host: '',
  hostname: '',
  port: '',
  pathname: '',
  search: '',
  searchParams: URLSearchParams {},
  hash: '' }
@TimothyGu TimothyGu added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Nov 13, 2018
@TimothyGu
Copy link
Member Author

/cc @nodejs/url

@joyeecheung
Copy link
Member

Looks like its accidentally fixed by #24218 , see #24218 (comment)

TimothyGu added a commit to TimothyGu/node that referenced this issue Nov 19, 2018
@TimothyGu
url: reuse existing context in href setter
8be84ac 
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: nodejs#24345
Refs: nodejs#24218 (comment)
@TimothyGu TimothyGu mentioned this issue Nov 19, 2018
Closed
3 tasks
Trott pushed a commit to Trott/io.js that referenced this issue Dec 1, 2018
@TimothyGu @Trott
url: reuse existing context in href setter
f2be20b 
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: nodejs#24345
Refs: nodejs#24218 (comment)

PR-URL: nodejs#24495
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member

Trott commented Dec 1, 2018

Fixed in f2be20b

@Trott Trott closed this as completed Dec 1, 2018
BridgeAR pushed a commit that referenced this issue Dec 5, 2018
@TimothyGu @BridgeAR
url: reuse existing context in href setter
0d7ee19 
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: #24345
Refs: #24218 (comment)

PR-URL: #24495
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
refack pushed a commit to refack/node that referenced this issue Jan 14, 2019
@TimothyGu @refack
url: reuse existing context in href setter
38a37ba 
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: nodejs#24345
Refs: nodejs#24218 (comment)

PR-URL: nodejs#24495
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this issue Feb 12, 2019
@TimothyGu @BethGriggs
url: reuse existing context in href setter
d06ea3e 
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: #24345
Refs: #24218 (comment)

PR-URL: #24495
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this issue Feb 28, 2019
@TimothyGu @rvagg
url: reuse existing context in href setter
d2994c9 
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: #24345
Refs: #24218 (comment)

PR-URL: #24495
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@Trott @TimothyGu @joyeecheung