Sanitize SVG animation attributes which could set JavaScript URL values.

Sanitize SVG animation attributes which could set JavaScript URL values.

When pasting we remove the hrefs from links with javascript: URLs
(among other things) because the scripts may be malicious. But SVG
animations can update hrefs indirectly, and we were not scrutinizing
those. This extends the sanitization to remove attributes from SVG
animations that contain JavaScript URLs because the animation could be
directed to set a href.

BUG=452059
TEST=webkit_unit_tests UnsafeSVGAttributeSanitizationTest.*

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197006

[dominicc (has gone to gerrit), 2015-06-10 08:54:18]

dominicc@chromium.org changed reviewers:
+ ojan@chromium.org, tsepez@chromium.org

[dominicc (has gone to gerrit), 2015-06-10 08:54:39]

PTAL

[dcheng, 2015-06-10 09:35:26]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2015-06-10 09:35:28]

https://codereview.chromium.org/1171223004/diff/1/Source/core/clipboard/Paste...
File Source/core/clipboard/Pasteboard.h (right):

https://codereview.chromium.org/1171223004/diff/1/Source/core/clipboard/Paste...
Source/core/clipboard/Pasteboard.h:49: Pasteboard();
Can we just use Pasteboard::generalPasteboard() in the unit test, so we don't
have to make this public?

https://codereview.chromium.org/1171223004/diff/1/Source/core/svg/SVGAnimateE...
File Source/core/svg/SVGAnimateElement.cpp (right):

https://codereview.chromium.org/1171223004/diff/1/Source/core/svg/SVGAnimateE...
Source/core/svg/SVGAnimateElement.cpp:63: for (auto part : parts) {
This copies a String on each iteration. While Blink String objects are cheap to
copy, for (const auto& part : parts) would result in no copy at all.

https://codereview.chromium.org/1171223004/diff/1/Source/core/svg/UnsafeSVGAt...
File Source/core/svg/UnsafeSVGAttributeSanitizationTest.cpp (right):

https://codereview.chromium.org/1171223004/diff/1/Source/core/svg/UnsafeSVGAt...
Source/core/svg/UnsafeSVGAttributeSanitizationTest.cpp:58:
pasteboard.writeHTML(htmlToPaste, blankURL(), "", false);
Can you remind me how this works for the Blink unit tests? Internally,
Pasteboard uses Platform::current()->clipboard() to get to the clipboard: are we
installing a test Platform implementation that exposes a mocked WebClipboard?

https://codereview.chromium.org/1171223004/diff/1/Source/core/svg/UnsafeSVGAt...
Source/core/svg/UnsafeSVGAttributeSanitizationTest.cpp:59:
frame.editor().pasteWithPasteboard(&pasteboard);
How about frame.editor().execCommand("paste") to avoid the need to friend a test
function in Editor's declaration?

https://codereview.chromium.org/1171223004/diff/1/Source/core/svg/UnsafeSVGAt...
Source/core/svg/UnsafeSVGAttributeSanitizationTest.cpp:75: const char*
unsafeContent =
Nit: const char* unsafeContent is actually a mutable pointer to a string
constant, and can be reassigned. const char unsafeContent[] is completely
immutable and uses one pointer less storage.

[Tom Sepez, 2015-06-10 17:29:02]

lgtm

https://codereview.chromium.org/1171223004/diff/1/Source/core/svg/UnsafeSVGAt...
File Source/core/svg/UnsafeSVGAttributeSanitizationTest.cpp (right):

https://codereview.chromium.org/1171223004/diff/1/Source/core/svg/UnsafeSVGAt...
Source/core/svg/UnsafeSVGAttributeSanitizationTest.cpp:75: const char*
unsafeContent =
On 2015/06/10 09:35:28, dcheng wrote:
> Nit: const char* unsafeContent is actually a mutable pointer to a string
> constant, and can be reassigned. const char unsafeContent[] is completely
> immutable and uses one pointer less storage.
nit: Don't you want static const char unsafeContent[] in that case?

https://codereview.chromium.org/1171223004/diff/1/Source/core/svg/UnsafeSVGAt...
Source/core/svg/UnsafeSVGAttributeSanitizationTest.cpp:94:
UnsafeSVGAttributeSanitizationTest,
Nit: funny indentation? I'd expect UnsafeSVGAttributeSanitizationTest to be on
the same line with TEST(

https://codereview.chromium.org/1171223004/diff/1/Source/core/svg/UnsafeSVGAt...
Source/core/svg/UnsafeSVGAttributeSanitizationTest.cpp:104: "            
attributeName='ng:href' values='evil;javascript:alert()'>"
nit: How about writing JavASCRipT in mixed case in one of these, just because? 
Also, I presume HTML entities are already resolved, but writing j&#x41vascript:
or jAvascript: might be nice, too.

[dominicc (has gone to gerrit), 2015-06-12 06:05:19]

Thanks for the useful feedback, PTAL.

Summary of what's changed:

- Test inputs are static const char[] instead of const char*; iterating value
parts uses const auto& instead of auto.
- Uses Pasteboard::generalPasteboard. @dcheng, The clipboard is mocked by
TestBlinkWebUnitTestSupport I think. This is much better, thank you.
- Uses executeCommand("Paste") instead of friending and hitting
pasteWithPasteboard directly. This is much better--the integration test tests
more of the integration.
- Added mixed case and entities to tests.
- I made the unit tests more precisely test specific isX functions instead of
stripScriptingAttributes.
- Improved some comments, organization.

@tsepez, the thing with weird indentation around TEST( is what
check-webkit-style wants, ie indent four spaces. :/

PTAL and CQ if this LG? kthx

[dcheng, 2015-06-12 06:46:11]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2015-06-12 06:46:11]

lgtm

[dcheng, 2015-06-12 06:46:11]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1171223004/#ps20001
(title: "Thanks for feedback.")

[commit-bot: I haz the power, 2015-06-12 06:46:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1171223004/20001

[commit-bot: I haz the power, 2015-06-12 07:13:45]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197006