Bug 90667

Summary: [JSC] HTML extensions to String.prototype should escape " as " in argument values
Product: WebKit Reporter: Mathias Bynens <mathias>
Component: JavaScriptCoreAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, barraclough, benjamin, cdumez, dglazkov, fpizlo, gtk-ews, gustavo, mathias, oliver, peter, tmpsantos, webkit.review.bot, xan.lopez
Priority: P2 Keywords: WebExposed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://mathias.html5.org/tests/javascript/string/
Bug Depends on: 101257    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
gtk-ews: commit-queue-
Patch
none
Patch
benjamin: review-
Patch
webkit.review.bot: commit-queue-
Patch none

Mathias Bynens
Reported 2012-07-06 01:51:02 PDT
Currently: > '_'.link('a"b') '<a href="a"b">_</a>' Expected result: > '_'.link('a"b') '<a href="a&quot;b">_</a>' The problem here is JSC doesn’t escape " into &quot; at the moment, which is a potential security risk (XSS vector). For this reason, Chrome/V8 escapes " into &quot;. Firefox/Spidermonkey is going to change its behavior to do the same: https://bugzilla.mozilla.org/show_bug.cgi?id=352437 Opera/Carakan will change its behavior too, as soon as other browsers change (bug DSK-369206). http://mathias.html5.org/specs/javascript/#escapeattributevalue requires escaping the ". Tests: http://mathias.html5.org/tests/javascript/string/ Here’s a list of the methods that have this issue: * String.prototype.anchor(name) * String.prototype.fontcolor(color) * String.prototype.fontsize(size) * String.prototype.link(href) See http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/runtime/StringPrototype.cpp.
Attachments
Patch (6.60 KB, patch)
2012-11-05 11:19 PST, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (6.54 KB, patch)
2012-11-05 12:39 PST, Chris Dumez 		gtk-ews: commit-queue-
DetailsFormatted DiffDiff
Patch (6.48 KB, patch)
2012-11-05 16:19 PST, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (6.48 KB, patch)
2012-11-07 02:37 PST, Chris Dumez 		benjamin: review-
DetailsFormatted DiffDiff
Patch (15.57 KB, patch)
2012-11-08 00:26 PST, Chris Dumez 		webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
Patch (17.08 KB, patch)
2012-11-08 11:11 PST, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Mathias Bynens
Comment 1 2012-08-04 06:57:35 PDT
(In reply to comment #0) > For this reason, Chrome/V8 escapes " into &quot;. Firefox/Spidermonkey is going to change its behavior to do the same: https://bugzilla.mozilla.org/show_bug.cgi?id=352437 Update: Firefox/Spidermonkey just landed this change. https://bugzilla.mozilla.org/show_bug.cgi?id=352437#c16
Adam Barth
Comment 2 2012-08-04 12:16:14 PDT
> '_'.link('a"b') "<a href="a&quot;b">_</a>" Seems to work as expected in V8.
Mathias Bynens
Comment 3 2012-08-04 12:49:57 PDT
(In reply to comment #2) > > '_'.link('a"b') > "<a href="a&quot;b">_</a>" > > Seems to work as expected in V8. That’s what I said in comment #0, no? V8 does the right thing and escapes " as &quot; correctly, but it also (needlessly) escapes some other characters, i.e. U+0027 APOSTROPHE and the < and > characters. These other escapes aren’t needed for security, and no other engine applies them. For this reason, I’ve filed a bug and submitted a patch against that behavior here: http://code.google.com/p/v8/issues/detail?id=2217
Chris Dumez
Comment 4 2012-11-05 11:19:46 PST
Created attachment 172367 [details] Patch
Benjamin Poulain
Comment 5 2012-11-05 12:14:31 PST
Comment on attachment 172367 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172367&action=review No clue if the patch is correct, but some comments regarding performance: > Source/JavaScriptCore/runtime/StringPrototype.cpp:1391 > + String color = a0.toString(exec)->value(exec); JSValue has toWTFString(), it is more efficient for some types when the value is not a string. > Source/JavaScriptCore/runtime/StringPrototype.cpp:1392 > + // Escape quotation mark. The Comment is not adding anything. > Source/JavaScriptCore/runtime/StringPrototype.cpp:1393 > + color.replace('"', ASCIILiteral("&quot;")); I don't like the idea of creating a new String every time. ASCIILiteral() will save you some time, but we can do better. What about adding StringImpl::replace() for string literals?
Chris Dumez
Comment 6 2012-11-05 12:37:01 PST
Comment on attachment 172367 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172367&action=review >> Source/JavaScriptCore/runtime/StringPrototype.cpp:1391 >> + String color = a0.toString(exec)->value(exec); > > JSValue has toWTFString(), it is more efficient for some types when the value is not a string. Ok >> Source/JavaScriptCore/runtime/StringPrototype.cpp:1392 >> + // Escape quotation mark. > > The Comment is not adding anything. Ok >> Source/JavaScriptCore/runtime/StringPrototype.cpp:1393 >> + color.replace('"', ASCIILiteral("&quot;")); > > I don't like the idea of creating a new String every time. ASCIILiteral() will save you some time, but we can do better. > What about adding StringImpl::replace() for string literals? I think this is a good idea this should probably be done in a separate patch, right?
Chris Dumez
Comment 7 2012-11-05 12:39:48 PST
Created attachment 172380 [details] Patch Take Benjamin's feedback into consideration. I haven't added a StringImpl::replace() that takes a literal yet though.
kov's GTK+ EWS bot
Comment 8 2012-11-05 12:57:53 PST
Comment on attachment 172380 [details] Patch Attachment 172380 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/14722812
Benjamin Poulain
Comment 9 2012-11-05 14:00:15 PST
> >> Source/JavaScriptCore/runtime/StringPrototype.cpp:1393 > >> + color.replace('"', ASCIILiteral("&quot;")); > > > > I don't like the idea of creating a new String every time. ASCIILiteral() will save you some time, but we can do better. > > What about adding StringImpl::replace() for string literals? > > I think this is a good idea this should probably be done in a separate patch, right? Yep, that would be better in a separate patch.
Chris Dumez
Comment 10 2012-11-05 16:19:37 PST
Created attachment 172431 [details] Patch Stop using ASCIILiteral(). Calling String::replace() with a literal replacement string will no longer construct a WTF::String object after Bug 101257 is closed.
Chris Dumez
Comment 11 2012-11-07 02:37:26 PST
Created attachment 172746 [details] Patch Use replaceWithLiteral() instead of replace() for efficiency (now that the dependency patch landed). Any feedback on this patch?
Benjamin Poulain
Comment 12 2012-11-07 13:19:01 PST
Comment on attachment 172746 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172746&action=review The patch itself looks good. I think the tests could be better. > LayoutTests/ChangeLog:14 > + * fast/js/script-tests/string-prototype-escape-attribute.js: Added. > + * fast/js/string-prototype-escape-attribute-expected.txt: Added. > + * fast/js/string-prototype-escape-attribute.html: Added. I think the test names are not descriptive enough. Wouldn't it be better to create one test for each of those functions? A quick grep in fast/js tells me their coverage may be bad, but maybe they are tested elsewhere? > LayoutTests/fast/js/script-tests/string-prototype-escape-attribute.js:5 > +shouldBe("'_'.anchor('\x22')", '"<a name=\\"&quot;\\">_</a>"'); Why use \x22 instead of "? it looks more confusing. You should also have tests with the quote being with some text and pathologically bad examples, e.g: "_".anchor('" href="http://evil.com"')
Chris Dumez
Comment 13 2012-11-08 00:26:59 PST
Created attachment 172946 [details] Patch Take Benjamin's feedback into consideration. I added a lot of more tests.
WebKit Review Bot
Comment 14 2012-11-08 10:21:09 PST
Comment on attachment 172946 [details] Patch Attachment 172946 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14762736 New failing tests: fast/js/string-fontsize.html fast/js/string-anchor.html fast/js/string-link.html fast/js/string-fontcolor.html
Chris Dumez
Comment 15 2012-11-08 10:36:57 PST
It appears V8 does not throw for null / undefined arguments. According to [1] it is supposed to throw a type error when CheckObjectCoercible is called for null or undefined. According to [2], CheckObjectCoercible should be called for those methods. This is causing the new tests to fail on Chromium port. [1] http://ecma-international.org/ecma-262/5.1/#sec-9.10 [2] http://mathias.html5.org/specs/javascript/#string.prototype.anchor
Benjamin Poulain
Comment 16 2012-11-08 10:49:53 PST
(In reply to comment #15) > It appears V8 does not throw for null / undefined arguments. > > According to [1] it is supposed to throw a type error when CheckObjectCoercible is called for null or undefined. > > According to [2], CheckObjectCoercible should be called for those methods. > > This is causing the new tests to fail on Chromium port. > > [1] http://ecma-international.org/ecma-262/5.1/#sec-9.10 > [2] http://mathias.html5.org/specs/javascript/#string.prototype.anchor That shows the test coverage has already improved :) Just in case, check what the other browsers are doing, sometime a spec is ignored by everyone. If JSC is not the only engine with that behavior, I guess we'll just add a failure in Chromium's test expectations.
Chris Dumez
Comment 17 2012-11-08 10:56:01 PST
(In reply to comment #16) > (In reply to comment #15) > > It appears V8 does not throw for null / undefined arguments. > > > > According to [1] it is supposed to throw a type error when CheckObjectCoercible is called for null or undefined. > > > > According to [2], CheckObjectCoercible should be called for those methods. > > > > This is causing the new tests to fail on Chromium port. > > > > [1] http://ecma-international.org/ecma-262/5.1/#sec-9.10 > > [2] http://mathias.html5.org/specs/javascript/#string.prototype.anchor > > That shows the test coverage has already improved :) > > Just in case, check what the other browsers are doing, sometime a spec is ignored by everyone. If JSC is not the only engine with that behavior, I guess we'll just add a failure in Chromium's test expectations. It appears there is already a bug filed for that: http://code.google.com/p/v8/issues/detail?id=2218 It seems that Opera and Firefox throw, IE doesn't. Should I skip these new test cases for Chromium port? Or should I remove those checks from the test cases?
Benjamin Poulain
Comment 18 2012-11-08 11:09:31 PST
> Should I skip these new test cases for Chromium port? Or should I remove those checks from the test cases? Don't remove the checks, it is great you added them. You can just add a failure expectation for Chromium.
Chris Dumez
Comment 19 2012-11-08 11:11:28 PST
Created attachment 173076 [details] Patch Skip new tests for chromium port.
Benjamin Poulain
Comment 20 2012-11-08 15:44:23 PST
Comment on attachment 173076 [details] Patch Looks good to me, good tests too.
WebKit Review Bot
Comment 21 2012-11-08 16:05:32 PST
Comment on attachment 173076 [details] Patch Clearing flags on attachment: 173076 Committed r133966: <http://trac.webkit.org/changeset/133966>
WebKit Review Bot
Comment 22 2012-11-08 16:05:40 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.