Percent-encode additional characters in "fragment state". #347

mikewest · 2017-10-09T12:58:04Z

Currently, we percent-encode characters in "fragment state" using the C0
control percent-encode set. Firefox encodes more than that, and it seems
reasonable to align around that behavior for reasons spelled out in #291
and the comments of #344.

This patch adds a new "fragment percent-encode set" which contains the
C0 control percent-encode set, along with:

0x22 (")
0x3C (<)
0x3E (>)
0x60 (`)

Closes #344.

Preview | Diff

Currently, we percent-encode characters in "fragment state" using the C0 control percent-encode set. Firefox encodes more than that, and it seems reasonable to align around that behavior for reasons spelled out in #291 and the comments of #344. This patch adds a new "fragment percent-encode set" which contains the C0 control percent-encode set, along with: * 0x22 (") * 0x3C (<) * 0x3E (>) * 0x60 (`) Closes #344.

mikewest · 2017-10-09T12:58:17Z

I'll put tests together shortly.

Tests for the new percent-encoding rules introduced in whatwg/url#347.

TimothyGu · 2017-10-09T17:24:15Z

url.bs

@@ -177,6 +177,9 @@ U+003E (>), U+003F (?), U+0060 (`), U+007B ({), and U+007D (}).
 <a>path percent-encode set</a> and U+002F (/), U+003A (:), U+003B (;), U+003D (=), U+0040 (@),
 U+005B ([), U+005C (\), U+005D (]), U+005E (^), and U+007C (|).

+<p>The <dfn>fragment percent-encode set</dfn> is the <a>C0 control percent-encode set</a> and


How about we try to define path percent-encode set as a superset of fragment percent-encode set, for parity with userinfo->path.

Makes sense to me. Poked at it in f258f4d. WDYT?

annevk · 2017-10-10T07:52:41Z

url.bs

@@ -169,9 +169,11 @@ contains bytes that are not <a>ASCII bytes</a> might be insecure and is not reco
 <p>The <dfn oldids=simple-encode-set>C0 control percent-encode set</dfn> are the <a>C0 controls</a>
 and all <a>code points</a> greater than U+007E (~).

+<p>The <dfn>fragment percent-encode set</dfn> is the <a>C0 control percent-encode set</a> and
+U+0022 SPACE, U+0022 ("), U+003C(&lt;), U+003E (&gt;), and U+0060 (`).


U+0020 SPACE.

Yup. That was super-dumb. Fixed in d1d85b2.

annevk

Final nit I suspect, other than getting the tests in order.

annevk · 2017-10-10T10:19:47Z

url.bs

@@ -169,9 +169,11 @@ contains bytes that are not <a>ASCII bytes</a> might be insecure and is not reco
 <p>The <dfn oldids=simple-encode-set>C0 control percent-encode set</dfn> are the <a>C0 controls</a>
 and all <a>code points</a> greater than U+007E (~).

+<p>The <dfn>fragment percent-encode set</dfn> is the <a>C0 control percent-encode set</a> and
+U+0020 SPACE, U+0022 ("), U+003C(&lt;), U+003E (&gt;), and U+0060 (`).


Missing space after U+003C.

annevk

Waiting for web-platform-tests/wpt#7641 now.

Follows whatwg/url#347.

domenic · 2017-10-10T20:23:24Z

The OP/commit message omit U+0020 space

Implements the changes to fragment processing described in whatwg/url#347, which adds a new "fragment percent-encode set" which contains the C0 control percent-encode set, along with: * 0x20 SPACE * 0x22 (") * 0x3C (<) * 0x3E (>) * 0x60 (`) This brings our implementation into line with Firefox. Bug: 758523 Change-Id: I25de642017ccb69473626a327ad194b3431a11ed

annevk · 2017-10-16T08:30:47Z

Tests moved to web-platform-tests/wpt#7776 though setter tests are not updated. Seems like jsdom doesn't run those.

Implements the changes to fragment processing described in whatwg/url#347, which adds a new "fragment percent-encode set" which contains the C0 control percent-encode set, along with: * 0x20 SPACE * 0x22 (") * 0x3C (<) * 0x3E (>) * 0x60 (`) This brings our implementation into line with Firefox. Bug: 758523 Change-Id: I25de642017ccb69473626a327ad194b3431a11ed

mikewest · 2017-12-01T13:10:29Z

Snapping back to this after @noncombatant pinged the internal bug. Sorry I lost the thread.

@domenic, @annevk: Does the current set of tests on web-platform-tests/wpt#7776 make sense to you? If so, I think we're done here. :) If not, I'll fix them.

annevk · 2017-12-01T13:37:39Z

Looks good to me, but I'll let @domenic verify with the jsdom impl.

Implements the changes to fragment processing described in whatwg/url#347, which adds a new "fragment percent-encode set" which contains the C0 control percent-encode set, along with: * 0x20 SPACE * 0x22 (") * 0x3C (<) * 0x3E (>) * 0x60 (`) This brings our implementation into line with Firefox. Bug: 758523 Change-Id: I25de642017ccb69473626a327ad194b3431a11ed

domenic · 2017-12-01T22:25:23Z

Tests are still not quite right, so it's worth straightening that out first.

Implements the changes to fragment processing described in whatwg/url#347, which adds a new "fragment percent-encode set" which contains the C0 control percent-encode set, along with: * 0x20 SPACE * 0x22 (") * 0x3C (<) * 0x3E (>) * 0x60 (`) This brings our implementation into line with Firefox. Bug: 758523 Change-Id: I25de642017ccb69473626a327ad194b3431a11ed

domenic · 2017-12-04T18:48:46Z

Tests good, this (and its tests) can be merged :)

Implements the changes to fragment processing described in whatwg/url#347, which adds a new "fragment percent-encode set" which contains the C0 control percent-encode set, along with: * 0x20 SPACE * 0x22 (") * 0x3C (<) * 0x3E (>) * 0x60 (`)

annevk · 2017-12-05T08:19:50Z

@mikewest could you file implementation bugs? I'm guessing just for Edge/Safari?

mikewest · 2017-12-05T08:30:52Z

WebKit: https://bugs.webkit.org/show_bug.cgi?id=180396
Edge: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/14951215/ (looks like the latter has some escaping issues with URLs. :( )

annevk · 2017-12-05T08:35:30Z

Thanks, I emailed some folks at Microsoft to look into that, since it makes reporting bugs against Edge much harder than it should be.

Follows whatwg/url#347.

Implements the changes to fragment processing described in whatwg/url#347, which adds a new "fragment percent-encode set" which contains the C0 control percent-encode set, along with: * 0x20 SPACE * 0x22 (") * 0x3C (<) * 0x3E (>) * 0x60 (`) This brings our implementation into line with Firefox. Bug: 758523 Change-Id: I25de642017ccb69473626a327ad194b3431a11ed Reviewed-on: https://chromium-review.googlesource.com/719004 Commit-Queue: Mike West <mkwst@chromium.org> Reviewed-by: Jochen Eisinger <jochen@chromium.org> Cr-Commit-Position: refs/heads/master@{#523383}

RByers · 2018-01-17T15:52:57Z

For the record, chromium bug here: https://bugs.chromium.org/p/chromium/issues/detail?id=803103

Percent-encode UTF8 characters in URL fragment identifiers. This brings us into line with Firefox, Safari, and the spec. Bug: 758523 Reviewed-on: https://chromium-review.googlesource.com/668363 Commit-Queue: Mike West <mkwst@chromium.org> Reviewed-by: Jochen Eisinger <jochen@chromium.org> Reviewed-by: Andy Paicu <andypaicu@chromium.org> Cr-Commit-Position: refs/heads/master@{#507481} Encode ' ', '"', '<', '>', and '`' in URL fragments. Implements the changes to fragment processing described in whatwg/url#347, which adds a new "fragment percent-encode set" which contains the C0 control percent-encode set, along with: * 0x20 SPACE * 0x22 (") * 0x3C (<) * 0x3E (>) * 0x60 (`) This brings our implementation into line with Firefox. Bug: 758523 Reviewed-on: https://chromium-review.googlesource.com/719004 Commit-Queue: Mike West <mkwst@chromium.org> Reviewed-by: Jochen Eisinger <jochen@chromium.org> Cr-Commit-Position: refs/heads/master@{#523383} Change-Id: I25de642017ccb69473626a327ad194b3431a11ed Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>

Implements the changes to fragment processing described in whatwg/url#347, which adds a new "fragment percent-encode set" which contains the C0 control percent-encode set, along with: * 0x20 SPACE * 0x22 (") * 0x3C (<) * 0x3E (>) * 0x60 (`) This brings our implementation into line with Firefox. Bug: 758523 Change-Id: I25de642017ccb69473626a327ad194b3431a11ed Reviewed-on: https://chromium-review.googlesource.com/719004 Commit-Queue: Mike West <mkwst@chromium.org> Reviewed-by: Jochen Eisinger <jochen@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#523383} Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: 01c25d47d2d22456368363e576083d766eedf8f6

Follows: whatwg/url#347

Implements the changes to fragment processing described in whatwg/url#347, which adds a new "fragment percent-encode set" which contains the C0 control percent-encode set, along with: * 0x20 SPACE * 0x22 (") * 0x3C (<) * 0x3E (>) * 0x60 (`) This brings our implementation into line with Firefox. Bug: 758523 Change-Id: I25de642017ccb69473626a327ad194b3431a11ed Reviewed-on: https://chromium-review.googlesource.com/719004 Commit-Queue: Mike West <mkwst@chromium.org> Reviewed-by: Jochen Eisinger <jochen@chromium.org> Cr-Commit-Position: refs/heads/master@{#523383} Former-commit-id: 01c25d47d2d22456368363e576083d766eedf8f6

mikewest added a commit to web-platform-tests/wpt that referenced this pull request Oct 9, 2017

URL: Percent-encode additional characters in "fragment state".

3879b23

Tests for the new percent-encoding rules introduced in whatwg/url#347.

This was referenced Oct 9, 2017

URL: Percent-encode additional characters in "fragment state". web-platform-tests/wpt#7641

Closed

Consider percent-encoding more characters in "fragment state" #344

Closed

TimothyGu reviewed Oct 9, 2017

View reviewed changes

fixup fragment => path

f258f4d

annevk requested changes Oct 10, 2017

View reviewed changes

fixup dumb typo

d1d85b2

annevk reviewed Oct 10, 2017

View reviewed changes

fixup Another dumb typo.

6c80db9

annevk approved these changes Oct 10, 2017

View reviewed changes

domenic added a commit to jsdom/whatwg-url that referenced this pull request Oct 10, 2017

Spec update: percent-encode more in fragments

214b900

Follows whatwg/url#347.

domenic mentioned this pull request Oct 10, 2017

Spec update: percent-encode more in fragments jsdom/whatwg-url#109

Merged

chromium-wpt-export-bot mentioned this pull request Oct 15, 2017

Encode ' ', '"', '<', '>', and '`' in URL fragments. web-platform-tests/wpt#7776

Merged

annevk merged commit 7a3c69f into whatwg:master Dec 5, 2017

mikewest deleted the fragment-percent-encode branch December 5, 2017 08:23

domenic added a commit to jsdom/whatwg-url that referenced this pull request Dec 5, 2017

Spec update: percent-encode more in fragments

e1a098a

Follows whatwg/url#347.

domenic added a commit to jsdom/whatwg-url that referenced this pull request Dec 5, 2017

Spec update: percent-encode more in fragments

f98656f

Follows whatwg/url#347.

annevk mentioned this pull request May 6, 2020

Unescaped '>' should probably not be allowed in URLs #291

Closed

rmisev added a commit to rmisev/url_whatwg that referenced this pull request May 24, 2020

Add fragment percent encode set & encode fragments

25aacdc

Follows: whatwg/url#347

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Percent-encode additional characters in "fragment state". #347

Percent-encode additional characters in "fragment state". #347

mikewest commented Oct 9, 2017 •

edited by pr-preview bot

mikewest commented Oct 9, 2017

TimothyGu Oct 9, 2017

mikewest Oct 10, 2017

annevk Oct 10, 2017

mikewest Oct 10, 2017

annevk left a comment

annevk Oct 10, 2017

annevk left a comment

domenic commented Oct 10, 2017

annevk commented Oct 16, 2017

mikewest commented Dec 1, 2017

annevk commented Dec 1, 2017

domenic commented Dec 1, 2017

domenic commented Dec 4, 2017

annevk commented Dec 5, 2017

mikewest commented Dec 5, 2017

annevk commented Dec 5, 2017

RByers commented Jan 17, 2018

Percent-encode additional characters in "fragment state". #347

Percent-encode additional characters in "fragment state". #347

Conversation

mikewest commented Oct 9, 2017 • edited by pr-preview bot

mikewest commented Oct 9, 2017

TimothyGu Oct 9, 2017

Choose a reason for hiding this comment

mikewest Oct 10, 2017

Choose a reason for hiding this comment

annevk Oct 10, 2017

Choose a reason for hiding this comment

mikewest Oct 10, 2017

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

annevk Oct 10, 2017

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

domenic commented Oct 10, 2017

annevk commented Oct 16, 2017

mikewest commented Dec 1, 2017

annevk commented Dec 1, 2017

domenic commented Dec 1, 2017

domenic commented Dec 4, 2017

annevk commented Dec 5, 2017

mikewest commented Dec 5, 2017

annevk commented Dec 5, 2017

RByers commented Jan 17, 2018

mikewest commented Oct 9, 2017 •

edited by pr-preview bot