Skip to content

Commit

Permalink
Ensure opaque URL paths always roundtrip
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=289160

Reviewed by NOBODY (OOPS!).

Instead of attempting to account for opaque paths sometimes ending in
spaces in the API implementation and failing because that did not
account for serialization, this new approach never lets them end with
spaces, period. This is standardized here:
whatwg/url#844

Test changes are upstreamed here:
web-platform-tests/wpt#51129

* LayoutTests/imported/w3c/web-platform-tests/url/a-element-origin-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/url/a-element-origin-xhtml-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/url/a-element-xhtml_exclude=(file_javascript_mailto)-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/url/a-element_exclude=(file_javascript_mailto)-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/url/resources/setters_tests.json:
* LayoutTests/imported/w3c/web-platform-tests/url/resources/urltestdata.json:
* LayoutTests/imported/w3c/web-platform-tests/url/url-constructor.any.worker_exclude=(file_javascript_mailto)-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/url/url-constructor.any_exclude=(file_javascript_mailto)-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/url/url-origin.any-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/url/url-origin.any.worker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/url/url-setters-a-area.window-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/url/url-setters.any.worker_exclude=(file_javascript_mailto)-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/url/url-setters.any_exclude=(file_javascript_mailto)-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/url/urlsearchparams-delete.any-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/url/urlsearchparams-delete.any.js:
(test):
* LayoutTests/imported/w3c/web-platform-tests/url/urlsearchparams-delete.any.worker-expected.txt:
* Source/WTF/wtf/URL.cpp:
(WTF::URL::removeFragmentIdentifier):
(WTF::URL::removeQueryAndFragmentIdentifier):
(WTF::URL::setQuery):
(WTF::URL::maybeTrimTrailingSpacesFromOpaquePath): Deleted.
* Source/WTF/wtf/URL.h:
* Source/WTF/wtf/URLParser.cpp:
(WTF::URLParser::parse):
  • Loading branch information
annevk committed Mar 5, 2025
1 parent c807626 commit 787f9e7
Show file tree
Hide file tree
Showing 19 changed files with 145 additions and 39 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,11 @@ PASS Parsing origin: <//www.example2.com> against <http://www.example.com/test>
PASS Parsing origin: <http://ExAmPlE.CoM> against <http://other.com/>
PASS Parsing origin: <http://GOO​⁠goo.com> against <http://other.com/>
PASS Parsing origin: <\0 http://example.com/ \r > against <about:blank>
PASS Parsing origin: <non-special:opaque > against <about:blank>
PASS Parsing origin: <non-special:opaque ?hi> against <about:blank>
PASS Parsing origin: <non-special:opaque #hi> against <about:blank>
PASS Parsing origin: <non-special:opaque x?hi> against <about:blank>
PASS Parsing origin: <non-special:opaque x#hi> against <about:blank>
PASS Parsing origin: <http://www.foo。bar.com> against <http://other.com/>
PASS Parsing origin: <https://x/�?�#�> against <about:blank>
PASS Parsing origin: <http://Go.com> against <http://other.com/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,11 @@ PASS Parsing origin: <//www.example2.com> against <http://www.example.com/test>
PASS Parsing origin: <http://ExAmPlE.CoM> against <http://other.com/>
PASS Parsing origin: <http://GOO​⁠goo.com> against <http://other.com/>
PASS Parsing origin: <\0 http://example.com/ \r > against <about:blank>
PASS Parsing origin: <non-special:opaque > against <about:blank>
PASS Parsing origin: <non-special:opaque ?hi> against <about:blank>
PASS Parsing origin: <non-special:opaque #hi> against <about:blank>
PASS Parsing origin: <non-special:opaque x?hi> against <about:blank>
PASS Parsing origin: <non-special:opaque x#hi> against <about:blank>
PASS Parsing origin: <http://www.foo。bar.com> against <http://other.com/>
PASS Parsing origin: <https://x/�?�#�> against <about:blank>
PASS Parsing origin: <http://Go.com> against <http://other.com/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,11 @@ PASS Parsing: <http://[:]> against <http://other.com/>
PASS Parsing: <http://GOO  goo.com> against <http://other.com/>
PASS Parsing: <http://GOO​⁠goo.com> against <http://other.com/>
PASS Parsing: <\0 http://example.com/ \r > against <about:blank>
PASS Parsing: <non-special:opaque > against <about:blank>
PASS Parsing: <non-special:opaque ?hi> against <about:blank>
PASS Parsing: <non-special:opaque #hi> against <about:blank>
PASS Parsing: <non-special:opaque x?hi> against <about:blank>
PASS Parsing: <non-special:opaque x#hi> against <about:blank>
PASS Parsing: <http://www.foo。bar.com> against <http://other.com/>
PASS Parsing: <http://﷐zyx.com> against <http://other.com/>
PASS Parsing: <http://%ef%b7%90zyx.com> against <http://other.com/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,11 @@ PASS Parsing: <http://[:]> against <http://other.com/>
PASS Parsing: <http://GOO  goo.com> against <http://other.com/>
PASS Parsing: <http://GOO​⁠goo.com> against <http://other.com/>
PASS Parsing: <\0 http://example.com/ \r > against <about:blank>
PASS Parsing: <non-special:opaque > against <about:blank>
PASS Parsing: <non-special:opaque ?hi> against <about:blank>
PASS Parsing: <non-special:opaque #hi> against <about:blank>
PASS Parsing: <non-special:opaque x?hi> against <about:blank>
PASS Parsing: <non-special:opaque x#hi> against <about:blank>
PASS Parsing: <http://www.foo。bar.com> against <http://other.com/>
PASS Parsing: <http://﷐zyx.com> against <http://other.com/>
PASS Parsing: <http://%ef%b7%90zyx.com> against <http://other.com/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2192,15 +2192,15 @@
"href": "data:space ?query#fragment",
"new_value": "",
"expected": {
"href": "data:space #fragment",
"href": "data:space#fragment",
"search": ""
}
},
{
"href": "sc:space ?query#fragment",
"new_value": "",
"expected": {
"href": "sc:space #fragment",
"href": "sc:space#fragment",
"search": ""
}
},
Expand Down Expand Up @@ -2357,7 +2357,7 @@
}
},
{
"comment": "Drop trailing spaces from trailing opaque paths",
"comment": "Drop trailing spaces from opaque paths",
"href": "data:space #fragment",
"new_value": "",
"expected": {
Expand All @@ -2376,19 +2376,19 @@
}
},
{
"comment": "Do not drop trailing spaces from non-trailing opaque paths",
"comment": "Drop trailing spaces from opaque paths",
"href": "data:space ?query#fragment",
"new_value": "",
"expected": {
"href": "data:space ?query",
"href": "data:space?query",
"hash": ""
}
},
{
"href": "sc:space ?query#fragment",
"new_value": "",
"expected": {
"href": "sc:space ?query",
"href": "sc:space?query",
"hash": ""
}
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3778,6 +3778,81 @@
"search": "",
"hash": ""
},
{
"input": "non-special:opaque ",
"base": null,
"href": "non-special:opaque",
"origin": "null",
"protocol": "non-special:",
"username": "",
"password": "",
"host": "",
"hostname": "",
"port": "",
"pathname": "opaque",
"search": "",
"hash": ""
},
{
"input": "non-special:opaque ?hi",
"base": null,
"href": "non-special:opaque?hi",
"origin": "null",
"protocol": "non-special:",
"username": "",
"password": "",
"host": "",
"hostname": "",
"port": "",
"pathname": "opaque",
"search": "?hi",
"hash": ""
},
{
"input": "non-special:opaque #hi",
"base": null,
"href": "non-special:opaque#hi",
"origin": "null",
"protocol": "non-special:",
"username": "",
"password": "",
"host": "",
"hostname": "",
"port": "",
"pathname": "opaque",
"search": "",
"hash": "#hi"
},
{
"input": "non-special:opaque x?hi",
"base": null,
"href": "non-special:opaque x?hi",
"origin": "null",
"protocol": "non-special:",
"username": "",
"password": "",
"host": "",
"hostname": "",
"port": "",
"pathname": "opaque x",
"search": "?hi",
"hash": ""
},
{
"input": "non-special:opaque x#hi",
"base": null,
"href": "non-special:opaque x#hi",
"origin": "null",
"protocol": "non-special:",
"username": "",
"password": "",
"host": "",
"hostname": "",
"port": "",
"pathname": "opaque x",
"search": "",
"hash": "#hi"
},
"Ideographic full stop (full-width period for Chinese, etc.) should be treated as a dot. U+3002 is mapped to U+002E (dot)",
{
"input": "http://www.foo。bar.com",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,11 @@ PASS Parsing: <http://[:]> against <http://other.com/>
PASS Parsing: <http://GOO  goo.com> against <http://other.com/>
PASS Parsing: <http://GOO​⁠goo.com> against <http://other.com/>
PASS Parsing: <\0 http://example.com/ \r > without base
PASS Parsing: <non-special:opaque > without base
PASS Parsing: <non-special:opaque ?hi> without base
PASS Parsing: <non-special:opaque #hi> without base
PASS Parsing: <non-special:opaque x?hi> without base
PASS Parsing: <non-special:opaque x#hi> without base
PASS Parsing: <http://www.foo。bar.com> against <http://other.com/>
PASS Parsing: <http://﷐zyx.com> against <http://other.com/>
PASS Parsing: <http://%ef%b7%90zyx.com> against <http://other.com/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,11 @@ PASS Parsing: <http://[:]> against <http://other.com/>
PASS Parsing: <http://GOO  goo.com> against <http://other.com/>
PASS Parsing: <http://GOO​⁠goo.com> against <http://other.com/>
PASS Parsing: <\0 http://example.com/ \r > without base
PASS Parsing: <non-special:opaque > without base
PASS Parsing: <non-special:opaque ?hi> without base
PASS Parsing: <non-special:opaque #hi> without base
PASS Parsing: <non-special:opaque x?hi> without base
PASS Parsing: <non-special:opaque x#hi> without base
PASS Parsing: <http://www.foo。bar.com> against <http://other.com/>
PASS Parsing: <http://﷐zyx.com> against <http://other.com/>
PASS Parsing: <http://%ef%b7%90zyx.com> against <http://other.com/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,11 @@ PASS Origin parsing: <//www.example2.com> against <http://www.example.com/test>
PASS Origin parsing: <http://ExAmPlE.CoM> against <http://other.com/>
PASS Origin parsing: <http://GOO​⁠goo.com> against <http://other.com/>
PASS Origin parsing: <\0 http://example.com/ \r > without base
PASS Origin parsing: <non-special:opaque > without base
PASS Origin parsing: <non-special:opaque ?hi> without base
PASS Origin parsing: <non-special:opaque #hi> without base
PASS Origin parsing: <non-special:opaque x?hi> without base
PASS Origin parsing: <non-special:opaque x#hi> without base
PASS Origin parsing: <http://www.foo。bar.com> against <http://other.com/>
PASS Origin parsing: <https://x/�?�#�> without base
PASS Origin parsing: <http://Go.com> against <http://other.com/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,11 @@ PASS Origin parsing: <//www.example2.com> against <http://www.example.com/test>
PASS Origin parsing: <http://ExAmPlE.CoM> against <http://other.com/>
PASS Origin parsing: <http://GOO​⁠goo.com> against <http://other.com/>
PASS Origin parsing: <\0 http://example.com/ \r > without base
PASS Origin parsing: <non-special:opaque > without base
PASS Origin parsing: <non-special:opaque ?hi> without base
PASS Origin parsing: <non-special:opaque #hi> without base
PASS Origin parsing: <non-special:opaque x?hi> without base
PASS Origin parsing: <non-special:opaque x#hi> without base
PASS Origin parsing: <http://www.foo。bar.com> against <http://other.com/>
PASS Origin parsing: <https://x/�?�#�> without base
PASS Origin parsing: <http://Go.com> against <http://other.com/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -518,12 +518,12 @@ PASS <a>: Setting <http://example.net>.hash = '%c3%89té' Bytes already percent-
PASS <area>: Setting <http://example.net>.hash = '%c3%89té' Bytes already percent-encoded are left as-is
PASS <a>: Setting <javascript:alert(1)>.hash = 'castle'
PASS <area>: Setting <javascript:alert(1)>.hash = 'castle'
PASS <a>: Setting <data:space #fragment>.hash = '' Drop trailing spaces from trailing opaque paths
PASS <area>: Setting <data:space #fragment>.hash = '' Drop trailing spaces from trailing opaque paths
PASS <a>: Setting <data:space #fragment>.hash = '' Drop trailing spaces from opaque paths
PASS <area>: Setting <data:space #fragment>.hash = '' Drop trailing spaces from opaque paths
PASS <a>: Setting <sc:space #fragment>.hash = ''
PASS <area>: Setting <sc:space #fragment>.hash = ''
PASS <a>: Setting <data:space ?query#fragment>.hash = '' Do not drop trailing spaces from non-trailing opaque paths
PASS <area>: Setting <data:space ?query#fragment>.hash = '' Do not drop trailing spaces from non-trailing opaque paths
PASS <a>: Setting <data:space ?query#fragment>.hash = '' Drop trailing spaces from opaque paths
PASS <area>: Setting <data:space ?query#fragment>.hash = '' Drop trailing spaces from opaque paths
PASS <a>: Setting <sc:space ?query#fragment>.hash = ''
PASS <area>: Setting <sc:space ?query#fragment>.hash = ''
PASS <a>: Setting <http://example.net>.hash = ' ' Trailing space should be encoded
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -227,9 +227,9 @@ PASS URL: Setting <a:/>.hash = '\0
PASS URL: Setting <http://example.net>.hash = 'a\0b' Percent-encode NULLs in fragment
PASS URL: Setting <non-spec:/>.hash = 'a\0b' Percent-encode NULLs in fragment
PASS URL: Setting <http://example.net>.hash = '%c3%89té' Bytes already percent-encoded are left as-is
PASS URL: Setting <data:space #fragment>.hash = '' Drop trailing spaces from trailing opaque paths
PASS URL: Setting <data:space #fragment>.hash = '' Drop trailing spaces from opaque paths
PASS URL: Setting <sc:space #fragment>.hash = ''
PASS URL: Setting <data:space ?query#fragment>.hash = '' Do not drop trailing spaces from non-trailing opaque paths
PASS URL: Setting <data:space ?query#fragment>.hash = '' Drop trailing spaces from opaque paths
PASS URL: Setting <sc:space ?query#fragment>.hash = ''
PASS URL: Setting <http://example.net>.hash = ' ' Trailing space should be encoded
PASS URL: Setting <http://example.net>.hash = '\0' Trailing C0 control should be encoded
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -227,9 +227,9 @@ PASS URL: Setting <a:/>.hash = '\0
PASS URL: Setting <http://example.net>.hash = 'a\0b' Percent-encode NULLs in fragment
PASS URL: Setting <non-spec:/>.hash = 'a\0b' Percent-encode NULLs in fragment
PASS URL: Setting <http://example.net>.hash = '%c3%89té' Bytes already percent-encoded are left as-is
PASS URL: Setting <data:space #fragment>.hash = '' Drop trailing spaces from trailing opaque paths
PASS URL: Setting <data:space #fragment>.hash = '' Drop trailing spaces from opaque paths
PASS URL: Setting <sc:space #fragment>.hash = ''
PASS URL: Setting <data:space ?query#fragment>.hash = '' Do not drop trailing spaces from non-trailing opaque paths
PASS URL: Setting <data:space ?query#fragment>.hash = '' Drop trailing spaces from opaque paths
PASS URL: Setting <sc:space ?query#fragment>.hash = ''
PASS URL: Setting <http://example.net>.hash = ' ' Trailing space should be encoded
PASS URL: Setting <http://example.net>.hash = '\0' Trailing C0 control should be encoded
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ PASS Delete basics
PASS Deleting appended multiple
PASS Deleting all params removes ? from URL
PASS Removing non-existent param removes ? from URL
PASS Changing the query of a URL with an opaque path can impact the path
PASS Changing the query of a URL with an opaque path can impact the path if the URL has no fragment
PASS Changing the query of a URL with an opaque path with trailing spaces
PASS Changing the query of a URL with an opaque path with trailing spaces and a fragment
PASS Two-argument delete()
PASS Two-argument delete() respects undefined as second arg

Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,15 @@ test(() => {
assert_equals(url.search, '');
assert_equals(url.pathname, 'space');
assert_equals(url.href, 'data:space');
}, 'Changing the query of a URL with an opaque path can impact the path');
}, 'Changing the query of a URL with an opaque path with trailing spaces');

test(() => {
const url = new URL('data:space ?test#test');
url.searchParams.delete('test');
assert_equals(url.search, '');
assert_equals(url.pathname, 'space ');
assert_equals(url.href, 'data:space #test');
}, 'Changing the query of a URL with an opaque path can impact the path if the URL has no fragment');
assert_equals(url.pathname, 'space');
assert_equals(url.href, 'data:space#test');
}, 'Changing the query of a URL with an opaque path with trailing spaces and a fragment');

test(() => {
const params = new URLSearchParams();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ PASS Delete basics
PASS Deleting appended multiple
PASS Deleting all params removes ? from URL
PASS Removing non-existent param removes ? from URL
PASS Changing the query of a URL with an opaque path can impact the path
PASS Changing the query of a URL with an opaque path can impact the path if the URL has no fragment
PASS Changing the query of a URL with an opaque path with trailing spaces
PASS Changing the query of a URL with an opaque path with trailing spaces and a fragment
PASS Two-argument delete()
PASS Two-argument delete() respects undefined as second arg

15 changes: 0 additions & 15 deletions Source/WTF/wtf/URL.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -704,22 +704,12 @@ void URL::setFragmentIdentifier(StringView identifier)
parseAllowingC0AtEnd(makeString(StringView(m_string).left(m_queryEnd), '#', identifier));
}

void URL::maybeTrimTrailingSpacesFromOpaquePath()
{
if (!m_isValid || !hasOpaquePath() || hasFragmentIdentifier() || hasQuery())
return;

parse(makeString(StringView(m_string).left(m_pathEnd)));
}

void URL::removeFragmentIdentifier()
{
if (!m_isValid)
return;

m_string = m_string.left(m_queryEnd);

maybeTrimTrailingSpacesFromOpaquePath();
}

void URL::removeQueryAndFragmentIdentifier()
Expand All @@ -729,8 +719,6 @@ void URL::removeQueryAndFragmentIdentifier()

m_string = m_string.left(m_pathEnd);
m_queryEnd = m_pathEnd;

maybeTrimTrailingSpacesFromOpaquePath();
}

void URL::setQuery(StringView newQuery)
Expand All @@ -747,9 +735,6 @@ void URL::setQuery(StringView newQuery)
newQuery,
StringView(m_string).substring(m_queryEnd)
));

if (newQuery.isNull())
maybeTrimTrailingSpacesFromOpaquePath();
}

static String escapePathWithoutCopying(StringView path)
Expand Down
2 changes: 0 additions & 2 deletions Source/WTF/wtf/URL.h
Original file line number Diff line number Diff line change
Expand Up @@ -262,8 +262,6 @@ class URL {
void parse(String&&);
void parseAllowingC0AtEnd(String&&);

void maybeTrimTrailingSpacesFromOpaquePath();

friend WTF_EXPORT_PRIVATE bool protocolHostAndPortAreEqual(const URL&, const URL&);

#if USE(CF)
Expand Down
8 changes: 8 additions & 0 deletions Source/WTF/wtf/URLParser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1133,6 +1133,7 @@ void URLParser::parse(std::span<const CharacterType> input, const URL& base, con
ASSERT(m_asciiBuffer.isEmpty());

Vector<UChar> queryBuffer;
unsigned numOpaquePathTrailingSpaces = 0;

auto endIndex = input.size();
if (UNLIKELY(nonUTF8QueryEncoding == URLTextEncodingSentinelAllowingC0AtEnd))
Expand Down Expand Up @@ -1776,7 +1777,14 @@ void URLParser::parse(std::span<const CharacterType> input, const URL& base, con
appendToASCIIBuffer('/');
++c;
m_url.m_pathAfterLastSlash = currentPosition(c);
} else if (*c == ' ') {
syntaxViolation(c);
++numOpaquePathTrailingSpaces;
++c;
} else {
for (size_t i = 0; i < numOpaquePathTrailingSpaces; i++)
appendToASCIIBuffer(' ');
numOpaquePathTrailingSpaces = 0;
utf8PercentEncode<isInC0ControlEncodeSet>(c);
++c;
}
Expand Down

0 comments on commit 787f9e7

Please sign in to comment.