Accessing form.action as property URI-encodes spaces but not curly braces

• Component: Forms
• Status: RESOLVED CONFIGURATION CHANGED

[Adam Luikart]

Reduction: https://jsfiddle.net/adamesque/nqp56nuo/

If you have a form element with an action attr that contains spaces and curly braces, accessing the action as a property on the DOM element returns a fully-qualified URL with spaces URI-encoded, but curly braces are not replaced.

Both Chrome & Firefox replace the braces with their URI-encoded equivalents.

This breaks code in the wild like https://github.com/indeedeng/proctor-webapp-library/blob/master/src/main/resources/META-INF/resources/static/scripts/app/editor.js#L220 which expects curly braces to be encoded.

Not sure what the "correct" behavior should be here, but consistency with other browsers would be nice.

[Alexey Proskuryakov]

This is quite interesting, because in a slightly different context, percent escaping curly braces caused a serious issue (bug 116887).

[Anne van Kesteren]

Took me ten minutes, and a little angst that browsers would differ per place where you can put a URL, but the difference here is that bug 116887 is about the query string, which per https://url.spec.whatwg.org/#query-state should indeed not have { and } escaped. However, per https://url.spec.whatwg.org/#path-state the path uses https://url.spec.whatwg.org/#default-encode-set which does include { and }.

[Alexey Proskuryakov]

Thank you Anne! Seems pretty clear that we should fix this then.

[Ilguiz Latypov]

I think of a URL as a string that needs parsing before obtaining its values.  Both the encoded and the raw curly brace would unambiguously reduce to the raw curly brace after decoding the URL.  I understand the advantage of the approach "accept liberally, output conservatively" in terms of the browser presenting DOM elements to Javascript, so the suggested correction makes sense to me.

I see that RFC 3986, while formally calling for encoding any character outside the following characters,

  ALPHA / DIGIT / "-" / "." / "_" / "~" 
  / "!" / "$" / "&" / "'" / "(" / ")" / "*" / "+" / "," / ";" / "=" 
  / ":" / "@"

suggests that "it is sometimes better for usability to avoid percent-encoding" special characters.  These two parts of the RFC look self-contradictory.

As for bug 116887, I obtained the same response from Washington Post regardless of the encoding, and I interpret this as an early conservative output.  Perhaps, the server did produce different results a few years back, and the conservative output of the browser on sending the request could work that around.

[Alex Christensen]

This difference is also evident from a much simpler test case:

alert(new URL("https://webkit.org/{path}/file.html"))

[Alex Christensen]

Created attachment 279846 [details]
Patch

[Alex Christensen]

This first patch is extremely under-tested, probably breaks something, and isn't optimized.  I'm expecting an r-, but I'm interested if you think I'm going in the right direction.

Our URL parser is a mess :(

[WebKit Commit Bot]

Attachment 279846 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/URL.cpp:300:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/URL.cpp:302:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 279846 [details]
Patch

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

> LayoutTests/fast/dom/DOMURL/parsing.html:61
> +shouldBe("breakDownURL('https://webkit.org/{path}/file.html?{query}')", "'protocol=https:, host=webkit.org, pathname=/%7Bpath%7D/file.html, search=?{query}, origin=https://webkit.org, toString=https://webkit.org/%7Bpath%7D/file.html?{query}'");

Can this also be tested with an end to end test (loading a resource with curly braces in the path)?

[Build Bot]

Comment on attachment 279846 [details]
Patch

Attachment 279846 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1383190

New failing tests:
http/tests/misc/script-defer-after-slow-stylesheet.html
fast/url/url-credentials-escaping.html
fast/css/uri-token-parsing.html
http/tests/security/no-popup-from-sandbox-top.html
fast/loader/url-strip-cr-lf-tab.html
fast/url/path.html

[Build Bot]

Created attachment 279849 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 279846 [details]
Patch

Attachment 279846 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1383192

New failing tests:
http/tests/misc/script-defer-after-slow-stylesheet.html
fast/url/url-credentials-escaping.html
fast/css/uri-token-parsing.html
http/tests/security/no-popup-from-sandbox-top.html
fast/loader/url-strip-cr-lf-tab.html
fast/url/path.html

[Build Bot]

Created attachment 279850 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 279846 [details]
Patch

Attachment 279846 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1383204

New failing tests:
http/tests/misc/script-defer-after-slow-stylesheet.html
fast/url/url-credentials-escaping.html
fast/css/uri-token-parsing.html
http/tests/security/no-popup-from-sandbox-top.html
fast/loader/url-strip-cr-lf-tab.html
fast/url/path.html

[Build Bot]

Created attachment 279856 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 279846 [details]
Patch

Attachment 279846 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1383209

New failing tests:
http/tests/misc/script-defer-after-slow-stylesheet.html
fast/url/url-credentials-escaping.html
fast/css/uri-token-parsing.html
mathml/opentype/horizontal-munderover.html
http/tests/security/no-popup-from-sandbox-top.html
fast/loader/url-strip-cr-lf-tab.html
fast/url/path.html

[Build Bot]

Created attachment 279857 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Anne van Kesteren]

Per https://jsdom.github.io/whatwg-url/#url=aHR0cHM6Ly93ZWJraXQub3JnL3twYXRofS9maWxlLmh0bWw=&base=YWJvdXQ6Ymxhbms= we are now correct for this input.