Bug 204414

Summary: Incorrect association of the URL object with the value port
Product: WebKit Reporter: Ivan Demidov <scrum>
Component: DOMAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, cdumez, commit-queue, esprehn+autocc, ews-watchlist, gyuyoung.kim, sam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari 13   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Ivan Demidov 2019-11-20 09:54:43 PST
const {hostname, port} = location;
const pathname = 'websocket';
const protocol = 'wss:'
const target = {hostname, pathname, port, protocol, search: ''};

Object.assign(new URL(location), target);

as a result of merging the above code, 0 value gets to the port

URL {
  hash: ""
  host: "bugs.webkit.org:0"
  hostname: "bugs.webkit.org"
  href: "wss://bugs.webkit.org:0/websocket"
  origin: "wss://bugs.webkit.org:0"
  password: ""
  pathname: "/websocket"
  port: "0"
  protocol: "wss:"
  search: ""
  searchParams: URLSearchParams {append: function, delete: function, get: function, getAll: function, has: function, …}
  username: ""
}
Comment 1 Radar WebKit Bug Importer 2019-11-22 16:00:43 PST
<rdar://problem/57444249>
Comment 2 Alex Christensen 2019-11-22 18:41:14 PST
Created attachment 384223 [details]
Patch
Comment 3 Alex Christensen 2019-11-22 21:07:16 PST
Created attachment 384230 [details]
Patch
Comment 4 Sam Weinig 2019-11-25 17:55:49 PST
Comment on attachment 384230 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384230&action=review

> Source/WebCore/html/URLUtils.h:236
> +    unsigned port = value.toUInt(&success);

Not related to this change, but it's a bit odd we use toUInt() here, rather than toUIntStrict(), since the former allows trailing garbage. Probably worth testing, but not in this change. (I also kind of want to change the toNumber functions to return Optionals).
Comment 5 Alex Christensen 2019-12-02 10:13:09 PST
Chromium allows trailing garbage, Firefox does not, the spec is a little unclear and seems to me that it would allow url.port="123?query" to override the path and query, but no browser allows that.  I'm going to commit this as-is right now to save the toUIntStrict discussion for another day, because it doesn't seem to be causing compatibility problems in practice now.
Comment 6 WebKit Commit Bot 2019-12-02 10:56:56 PST
Comment on attachment 384230 [details]
Patch

Clearing flags on attachment: 384230

Committed r252998: <https://trac.webkit.org/changeset/252998>
Comment 7 WebKit Commit Bot 2019-12-02 10:56:57 PST
All reviewed patches have been landed.  Closing bug.