Currently: > '_'.link('a"b') '<a href="a"b">_</a>' Expected result: > '_'.link('a"b') '<a href="a"b">_</a>' The problem here is JSC doesn’t escape " into " at the moment, which is a potential security risk (XSS vector). For this reason, Chrome/V8 escapes " into ". 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.
(In reply to comment #0) > For this reason, Chrome/V8 escapes " into ". 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
> '_'.link('a"b') "<a href="a"b">_</a>" Seems to work as expected in V8.
(In reply to comment #2) > > '_'.link('a"b') > "<a href="a"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 " 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
Created attachment 172367 [details] Patch
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(""")); 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?
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(""")); > > 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?
Created attachment 172380 [details] Patch Take Benjamin's feedback into consideration. I haven't added a StringImpl::replace() that takes a literal yet though.
Comment on attachment 172380 [details] Patch Attachment 172380 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/14722812
> >> Source/JavaScriptCore/runtime/StringPrototype.cpp:1393 > >> + color.replace('"', ASCIILiteral(""")); > > > > 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.
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.
Created attachment 172746 [details] Patch Use replaceWithLiteral() instead of replace() for efficiency (now that the dependency patch landed). Any feedback on this patch?
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=\\""\\">_</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"')
Created attachment 172946 [details] Patch Take Benjamin's feedback into consideration. I added a lot of more tests.
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
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
(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.
(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?
> 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.
Created attachment 173076 [details] Patch Skip new tests for chromium port.
Comment on attachment 173076 [details] Patch Looks good to me, good tests too.
Comment on attachment 173076 [details] Patch Clearing flags on attachment: 173076 Committed r133966: <http://trac.webkit.org/changeset/133966>
All reviewed patches have been landed. Closing bug.