Bug 157153 - Accessing form.action as property URI-encodes spaces but not curly braces
Summary: Accessing form.action as property URI-encodes spaces but not curly braces
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: WebKit Nightly Build
Hardware: Macintosh OS X 10.11
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-28 15:42 PDT by Adam Luikart
Modified: 2016-05-26 16:13 PDT (History)
10 users (show)

See Also:


Attachments
Patch (7.05 KB, patch)
2016-05-25 18:20 PDT, Alex Christensen
achristensen: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (877.32 KB, application/zip)
2016-05-25 19:06 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (840.19 KB, application/zip)
2016-05-25 19:10 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-yosemite (1.47 MB, application/zip)
2016-05-25 19:20 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (706.60 KB, application/zip)
2016-05-25 19:29 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Luikart 2016-04-28 15:42:05 PDT
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.
Comment 1 Alexey Proskuryakov 2016-04-28 19:50:02 PDT
This is quite interesting, because in a slightly different context, percent escaping curly braces caused a serious issue (bug 116887).
Comment 2 Anne van Kesteren 2016-04-29 00:26:18 PDT
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 }.
Comment 3 Alexey Proskuryakov 2016-04-29 10:18:21 PDT
Thank you Anne! Seems pretty clear that we should fix this then.
Comment 4 Ilguiz Latypov 2016-05-25 16:23:53 PDT
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.
Comment 5 Alex Christensen 2016-05-25 17:16:26 PDT
This difference is also evident from a much simpler test case:

alert(new URL("https://webkit.org/{path}/file.html"))
Comment 6 Alex Christensen 2016-05-25 18:20:15 PDT
Created attachment 279846 [details]
Patch
Comment 7 Alex Christensen 2016-05-25 18:21:18 PDT
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 :(
Comment 8 WebKit Commit Bot 2016-05-25 18:21:46 PDT
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.
Comment 9 Alexey Proskuryakov 2016-05-25 19:05:57 PDT
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)?
Comment 10 Build Bot 2016-05-25 19:06:54 PDT
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
Comment 11 Build Bot 2016-05-25 19:06:58 PDT
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
Comment 12 Build Bot 2016-05-25 19:10:37 PDT
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
Comment 13 Build Bot 2016-05-25 19:10:40 PDT
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
Comment 14 Build Bot 2016-05-25 19:20:14 PDT
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
Comment 15 Build Bot 2016-05-25 19:20:18 PDT
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
Comment 16 Build Bot 2016-05-25 19:29:14 PDT
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
Comment 17 Build Bot 2016-05-25 19:29:18 PDT
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