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

Should protocol setter remove tabs and newline? #609

Closed
TimothyGu opened this issue May 31, 2021 · 7 comments
Closed

Should protocol setter remove tabs and newline? #609

TimothyGu opened this issue May 31, 2021 · 7 comments
Labels
topic: api

Comments

@TimothyGu
Copy link
Member

Consider the following test:

u = new URL("https://github.com/whatwg/url/issues/new");
u.protocol = "ht\ntp";
u.href

On Chrome, Firefox, and Safari, u.href retains the original https protocol. This behavior as it relates to the Location object is in fact tested by WPT(!): search for \x0A in html/browsers/history/the-location-interface/location-protocol-setter.html.
Merged
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 31, 2021
@TimothyGu @chromium-wpt-export-bot
kurl: remove tabs and newlines from setter inputs
36e9c7e 
This makes setters of Location.search, HTMLAnchorElement.hash,
URL.pathname, etc., remove tabs and newlines. This aligns with the
WHATWG URL Standard and Safari, as well as Firefox partially.

There are a few exceptions. SetProtocol is unchanged due to an upstream
spec issue [1]. SetUser and SetPass should not remove tabs and newlines
per spec.

[1]: whatwg/url#609

Bug: 1214932
Change-Id: I3bcb6fa9e3b61ee5a0a03b13eca93d42912afb22
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 1, 2021
@TimothyGu @chromium-wpt-export-bot
kurl: remove tabs and newlines from setter inputs
42af65a 
This makes setters of Location.search, HTMLAnchorElement.hash,
URL.pathname, etc., remove tabs and newlines. This aligns with the
WHATWG URL Standard and Safari, as well as Firefox partially.

There are a few exceptions. SetProtocol is unchanged due to an upstream
spec issue [1]. SetUser and SetPass should not remove tabs and newlines
per spec.

[1]: whatwg/url#609

Bug: 1214932
Change-Id: I3bcb6fa9e3b61ee5a0a03b13eca93d42912afb22
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 3, 2021
@TimothyGu @chromium-wpt-export-bot
kurl: remove tabs and newlines from setter inputs
89b8e91 
This makes setters of Location.search, HTMLAnchorElement.hash,
URL.pathname, etc., remove tabs and newlines. This aligns with the
WHATWG URL Standard and Safari, as well as Firefox partially.

There are a few exceptions. SetProtocol is unchanged due to an upstream
spec issue [1]. SetUser and SetPass should not remove tabs and newlines
per spec.

[1]: whatwg/url#609

Bug: 1214932
Change-Id: I3bcb6fa9e3b61ee5a0a03b13eca93d42912afb22
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2928080
Reviewed-by: Kent Tamura <[email protected]>
Commit-Queue: Timothy Gu <[email protected]>
Cr-Commit-Position: refs/heads/master@{#888804}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 3, 2021
@TimothyGu @chromium-wpt-export-bot
kurl: remove tabs and newlines from setter inputs
8c1254a 
This makes setters of Location.search, HTMLAnchorElement.hash,
URL.pathname, etc., remove tabs and newlines. This aligns with the
WHATWG URL Standard and Safari, as well as Firefox partially.

There are a few exceptions. SetProtocol is unchanged due to an upstream
spec issue [1]. SetUser and SetPass should not remove tabs and newlines
per spec.

[1]: whatwg/url#609

Bug: 1214932
Change-Id: I3bcb6fa9e3b61ee5a0a03b13eca93d42912afb22
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2928080
Reviewed-by: Kent Tamura <[email protected]>
Commit-Queue: Timothy Gu <[email protected]>
Cr-Commit-Position: refs/heads/master@{#888804}
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this issue Jun 3, 2021
@TimothyGu
kurl: remove tabs and newlines from setter inputs
4b9226a 
This makes setters of Location.search, HTMLAnchorElement.hash,
URL.pathname, etc., remove tabs and newlines. This aligns with the
WHATWG URL Standard and Safari, as well as Firefox partially.

There are a few exceptions. SetProtocol is unchanged due to an upstream
spec issue [1]. SetUser and SetPass should not remove tabs and newlines
per spec.

[1]: whatwg/url#609

Bug: 1214932
Change-Id: I3bcb6fa9e3b61ee5a0a03b13eca93d42912afb22
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2928080
Reviewed-by: Kent Tamura <[email protected]>
Commit-Queue: Timothy Gu <[email protected]>
Cr-Commit-Position: refs/heads/master@{#888804}
@domenic
Copy link
Member

domenic commented Jun 8, 2021

I don't have a strong feeling here, but I lean toward keeping protocol consistent with all other invocations of the URL parser and getting all browsers to update. It seems pretty gross to have a special carve-out for one particular state override.

On the other hand, when all three browsers do something, probably the spec should be updated to match it, even if that thing is really weird.

I'm curious what @achristensen07 thinks...

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jun 8, 2021
@TimothyGu @moz-wptsync-bot
Bug 1713676 [wpt PR 29168] - kurl: remove tabs and newlines from sett…
11a1b9a 
…er inputs, a=testonly

Automatic update from web-platform-tests
kurl: remove tabs and newlines from setter inputs

This makes setters of Location.search, HTMLAnchorElement.hash,
URL.pathname, etc., remove tabs and newlines. This aligns with the
WHATWG URL Standard and Safari, as well as Firefox partially.

There are a few exceptions. SetProtocol is unchanged due to an upstream
spec issue [1]. SetUser and SetPass should not remove tabs and newlines
per spec.

[1]: whatwg/url#609

Bug: 1214932
Change-Id: I3bcb6fa9e3b61ee5a0a03b13eca93d42912afb22
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2928080
Reviewed-by: Kent Tamura <[email protected]>
Commit-Queue: Timothy Gu <[email protected]>
Cr-Commit-Position: refs/heads/master@{#888804}

--

wpt-commits: 8c1254ac85f709e6a8a6baa7e6c11f9c1fc39e93
wpt-pr: 29168
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Jun 9, 2021
@TimothyGu @moz-wptsync-bot
Bug 1713676 [wpt PR 29168] - kurl: remove tabs and newlines from sett…
4e6a67a 
…er inputs, a=testonly

Automatic update from web-platform-tests
kurl: remove tabs and newlines from setter inputs

This makes setters of Location.search, HTMLAnchorElement.hash,
URL.pathname, etc., remove tabs and newlines. This aligns with the
WHATWG URL Standard and Safari, as well as Firefox partially.

There are a few exceptions. SetProtocol is unchanged due to an upstream
spec issue [1]. SetUser and SetPass should not remove tabs and newlines
per spec.

[1]: whatwg/url#609

Bug: 1214932
Change-Id: I3bcb6fa9e3b61ee5a0a03b13eca93d42912afb22
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2928080
Reviewed-by: Kent Tamura <[email protected]>
Commit-Queue: Timothy Gu <[email protected]>
Cr-Commit-Position: refs/heads/master@{#888804}

--

wpt-commits: 8c1254ac85f709e6a8a6baa7e6c11f9c1fc39e93
wpt-pr: 29168
@TimothyGu TimothyGu mentioned this issue Jul 8, 2021
Closed
@achristensen07
Copy link
Collaborator

Given that all browsers agree on behavior, I think we should change the spec to match behavior in this case

@achristensen07
Copy link
Collaborator

I guess I don't have a strong opinion here. I don't think there will be a lot of compatibility issues if we change this.

@achristensen07
Copy link
Collaborator

Should we at least update the tests to be consistent with the current specs? I also found that http://wpt.live/html/browsers/history/the-location-interface/location-protocol-setter-non-broken-weird.html and http://wpt.live/url/url-setters-a-area.window.html also contradict each other.

@domenic
Copy link
Member

domenic commented Aug 24, 2021

Yeah, we should definitely update the tests to be self-consistent, and probably it's best to align them with the current spec. (Unless we want to update the spec, which I think everyone is OK with, but nobody is insisting on...)

mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
@TimothyGu @copybara-github
kurl: remove tabs and newlines from setter inputs
10057d9 
This makes setters of Location.search, HTMLAnchorElement.hash,
URL.pathname, etc., remove tabs and newlines. This aligns with the
WHATWG URL Standard and Safari, as well as Firefox partially.

There are a few exceptions. SetProtocol is unchanged due to an upstream
spec issue [1]. SetUser and SetPass should not remove tabs and newlines
per spec.

[1]: whatwg/url#609

Bug: 1214932
Change-Id: I3bcb6fa9e3b61ee5a0a03b13eca93d42912afb22
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2928080
Reviewed-by: Kent Tamura <[email protected]>
Commit-Queue: Timothy Gu <[email protected]>
Cr-Commit-Position: refs/heads/master@{#888804}
NOKEYCHECK=True
GitOrigin-RevId: 4b9226ad340958376943b8e10a224363169b3b98
@annevk annevk added the needs tests Moving the issue forward requires someone to write tests label Dec 9, 2022
annevk added a commit to web-platform-tests/wpt that referenced this issue Jan 18, 2023
@annevk
URL and HTML: correct protocol setter tests
44ea4c6 
For whatwg/url#609.
@annevk annevk mentioned this issue Jan 18, 2023
Merged
@annevk
Copy link
Member

annevk commented Jan 18, 2023

#61 added the tests in OP, but #101 was fixed before that and somehow not taken into account. So it seem this was an oversight.

I don't think there is an inconsistency between the tests @achristensen07 points towards. There are different requirements for Location's protocol setter after all. In particular, the resulting scheme has to be an HTTP(S) scheme as nothing else really gives sensible results there.

I created web-platform-tests/wpt#38032 to update the incorrect tests and add some missing scenarios. Suggestions welcome.

Once those are reviewed I'll file implementation bugs and then we can close this issue in my opinion.

@annevk annevk removed the needs tests Moving the issue forward requires someone to write tests label Jan 18, 2023
annevk added a commit to web-platform-tests/wpt that referenced this issue Jan 24, 2023
@annevk
URL and HTML: correct protocol setter tests
865a920 
For whatwg/url#609.
This was referenced Jan 24, 2023
Closed
Open
@annevk
Copy link
Member

annevk commented Jan 24, 2023

Bugs:

@annevk annevk closed this as completed Jan 24, 2023
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Feb 1, 2023
@annevk @moz-wptsync-bot
Bug 1810968 [wpt PR 38032] - URL and HTML: correct protocol setter te…
d265ecc 
…sts, a=testonly

Automatic update from web-platform-tests
URL and HTML: correct protocol setter tests

For whatwg/url#609.
--

wpt-commits: 865a920dd12159c324560f482288094088cba1ce
wpt-pr: 38032
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Feb 3, 2023
@annevk @moz-wptsync-bot
Bug 1810968 [wpt PR 38032] - URL and HTML: correct protocol setter te…
e0e3975 
…sts, a=testonly

Automatic update from web-platform-tests
URL and HTML: correct protocol setter tests

For whatwg/url#609.
--

wpt-commits: 865a920dd12159c324560f482288094088cba1ce
wpt-pr: 38032
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Feb 23, 2023
@CanadaHonk
Bug 1812060 - Strip certain whitespace from protocol setters r=emilio…
41d9ecc 
…,necko-reviewers,valentin

Strip tabs, new lines, and carriage returns from the input given to
protocol setters for Location, URL, and Link.
Also updated WPT expectations.

Reference: whatwg/url#609

Differential Revision: https://phabricator.services.mozilla.com/D170404
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Feb 24, 2023
@CanadaHonk
Bug 1812060 - Strip certain whitespace from protocol setters r=emilio…
06a86d7 
…,necko-reviewers,valentin

Strip tabs, new lines, and carriage returns from the input given to
protocol setters for Location, URL, and Link.
Also updated WPT expectations.

Reference: whatwg/url#609

Differential Revision: https://phabricator.services.mozilla.com/D170404
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Mar 1, 2023
@CanadaHonk
Bug 1812060 - Strip certain whitespace from protocol setters r=emilio…
3894bdc 
…,necko-reviewers,valentin

Strip tabs, new lines, and carriage returns from the input given to
protocol setters for Location, URL, and Link.
Also updated WPT expectations.

Reference: whatwg/url#609

Differential Revision: https://phabricator.services.mozilla.com/D170404
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Mar 1, 2023
@CanadaHonk
Bug 1812060 - Strip certain whitespace from protocol setters r=emilio…
ab3a9de 
…,necko-reviewers,valentin

Strip tabs, new lines, and carriage returns from the input given to
protocol setters for Location, URL, and Link.
Also updated WPT expectations.

Reference: whatwg/url#609

Differential Revision: https://phabricator.services.mozilla.com/D170404
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this issue Mar 28, 2023
@annevk @marcoscaceres
URL and HTML: correct protocol setter tests
ad0e884 
For whatwg/url#609.
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this issue Jun 1, 2024
@annevk @moz-wptsync-bot
Bug 1810968 [wpt PR 38032] - URL and HTML: correct protocol setter te…
cadebbb 
…sts, a=testonly

Automatic update from web-platform-tests
URL and HTML: correct protocol setter tests

For whatwg/url#609.
--

wpt-commits: 865a920dd12159c324560f482288094088cba1ce
wpt-pr: 38032
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
topic: api
Milestone
No milestone
Development

No branches or pull requests
4 participants
@domenic @achristensen07 @TimothyGu @annevk