Strip away event handlers and JavaScript URLs when copying

https://bugs.webkit.org/show_bug.cgi?id=178375 Reviewed by Wenson Hsieh. Source/WebCore: Don't serialize event handlers and URLs with javascript protocol when serializing HTML since they're not safe to be pasted elsewhere. Test: editing/pasteboard/copying-html-strips-javascript-url-and-event-handler.html * dom/Element.cpp: (WebCore::Element::isEventHandlerAttribute const): (WebCore::isEventHandlerAttribute): Deleted. * dom/Element.h: * editing/markup.cpp: (WebCore::StyledMarkupAccumulator::appendElement): LayoutTests: Added a regression test. * editing/pasteboard/copying-html-strips-javascript-url-and-event-handler-expected.txt: Added. * editing/pasteboard/copying-html-strips-javascript-url-and-event-handler.html: Added. Canonical link: https://commits.webkit.org/194600@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@223462 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Oct 17, 2017 · 3ecb93dddddba36b854ef88173a26715dbd6fac0 · 3ecb93d
1 parent 7f22cab
commit 3ecb93dddddba36b854ef88173a26715dbd6fac0
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,15 @@
+2017-10-16  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Strip away event handlers and JavaScript URLs when copying
+        https://bugs.webkit.org/show_bug.cgi?id=178375
+
+        Reviewed by Wenson Hsieh.
+
+        Added a regression test.
+
+        * editing/pasteboard/copying-html-strips-javascript-url-and-event-handler-expected.txt: Added.
+        * editing/pasteboard/copying-html-strips-javascript-url-and-event-handler.html: Added.
+
 2017-10-16  Dean Jackson  <dino@apple.com>
 
         WebGL clamps drawingBufferWidth to 4096 pixels on a 5120 monitor/canvas

diff --git a/...ests/editing/pasteboard/copying-html-strips-javascript-url-and-event-handler-expected.txt b/...ests/editing/pasteboard/copying-html-strips-javascript-url-and-event-handler-expected.txt
@@ -0,0 +1,12 @@
+This tests copying and pasting a markup with an event handler and a javascript URL. WebKit should strip them away upon copy.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS html.includes("hello") is true
+PASS html.includes("world") is true
+PASS html.includes("dangerousCode") is false
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/editing/pasteboard/copying-html-strips-javascript-url-and-event-handler.html b/LayoutTests/editing/pasteboard/copying-html-strips-javascript-url-and-event-handler.html
@@ -0,0 +1,46 @@
+<!DOCTYPE html>
+<html>
+<body>
+<div id="container">
+<button onclick="runTest()">Copy</button>
+<div id="source" contenteditable><a href="javascript:dangerousCode()">hello</a>, <b onmouseover="dangerousCode()">world</b>. WebKit</div>
+<div id="destination" onpaste="doPaste(event)" contenteditable>Paste here</div>
+</div>
+<script src="../../resources/js-test-pre.js"></script>
+<script>
+
+description('This tests copying and pasting a markup with an event handler and a javascript URL. WebKit should strip them away upon copy.');
+jsTestIsAsync = true;
+
+function dangerousCode() { }
+
+function runTest() {
+    source.focus();
+    document.execCommand('selectAll');
+    document.execCommand('copy');
+
+    destination.focus();
+    document.execCommand('selectAll');
+    if (window.testRunner)
+        document.execCommand('paste');
+}
+
+function doPaste(event) {
+    html = event.clipboardData.getData('text/html');
+    shouldBeTrue('html.includes("hello")');
+    shouldBeTrue('html.includes("world")');
+    shouldBeFalse('html.includes("dangerousCode")');
+
+    container.remove();
+    finishJSTest();
+}
+
+if (window.testRunner)
+    window.onload = runTest;
+
+successfullyParsed = true;
+
+</script>
+<script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,22 @@
+2017-10-16  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Strip away event handlers and JavaScript URLs when copying
+        https://bugs.webkit.org/show_bug.cgi?id=178375
+
+        Reviewed by Wenson Hsieh.
+
+        Don't serialize event handlers and URLs with javascript protocol when serializing HTML
+        since they're not safe to be pasted elsewhere.
+
+        Test: editing/pasteboard/copying-html-strips-javascript-url-and-event-handler.html
+
+        * dom/Element.cpp:
+        (WebCore::Element::isEventHandlerAttribute const):
+        (WebCore::isEventHandlerAttribute): Deleted.
+        * dom/Element.h:
+        * editing/markup.cpp:
+        (WebCore::StyledMarkupAccumulator::appendElement):
+
 2017-10-16  Dean Jackson  <dino@apple.com>
 
         WebGL clamps drawingBufferWidth to 4096 pixels on a 5120 monitor/canvas

diff --git a/Source/WebCore/dom/Element.cpp b/Source/WebCore/dom/Element.cpp
@@ -1514,7 +1514,7 @@ void Element::storeDisplayContentsStyle(std::unique_ptr<RenderStyle> style)
 // It is a simple solution that has the advantage of not requiring any
 // code or configuration change if a new event handler is defined.
 
-static inline bool isEventHandlerAttribute(const Attribute& attribute)
+bool Element::isEventHandlerAttribute(const Attribute& attribute) const
 {
     return attribute.name().namespaceURI().isNull() && attribute.name().localName().startsWith("on");
 }
@@ -1527,7 +1527,7 @@ bool Element::isJavaScriptURLAttribute(const Attribute& attribute) const
 void Element::stripScriptingAttributes(Vector<Attribute>& attributeVector) const
 {
     attributeVector.removeAllMatching([this](auto& attribute) -> bool {
-        return isEventHandlerAttribute(attribute)
+        return this->isEventHandlerAttribute(attribute)
             || this->isJavaScriptURLAttribute(attribute)
             || this->isHTMLContentAttribute(attribute);
     });

diff --git a/Source/WebCore/dom/Element.h b/Source/WebCore/dom/Element.h
@@ -242,6 +242,9 @@ class Element : public ContainerNode {
     // Only called by the parser immediately after element construction.
     void parserSetAttributes(const Vector<Attribute>&);
 
+    bool isEventHandlerAttribute(const Attribute&) const;
+    bool isJavaScriptURLAttribute(const Attribute&) const;
+
     // Remove attributes that might introduce scripting from the vector leaving the element unchanged.
     void stripScriptingAttributes(Vector<Attribute>&) const;
 
@@ -650,8 +653,6 @@ class Element : public ContainerNode {
     void detachAllAttrNodesFromElement();
     void detachAttrNodeFromElementWithValue(Attr*, const AtomicString& value);
 
-    bool isJavaScriptURLAttribute(const Attribute&) const;
-
     // Anyone thinking of using this should call document instead of ownerDocument.
     void ownerDocument() const = delete;
 

diff --git a/Source/WebCore/editing/markup.cpp b/Source/WebCore/editing/markup.cpp
@@ -401,6 +401,8 @@ void StyledMarkupAccumulator::appendElement(StringBuilder& out, const Element& e
             // We'll handle the style attribute separately, below.
             if (attribute.name() == styleAttr && shouldOverrideStyleAttr)
                 continue;
+            if (element.isEventHandlerAttribute(attribute) || element.isJavaScriptURLAttribute(attribute))
+                continue;
             appendAttribute(out, element, attribute, 0);
         }
     }