Implement further enforcement of Trusted Types for Attributes

https://bugs.webkit.org/show_bug.cgi?id=274267 Reviewed by Ryosuke Niwa. This patch adds trusted types enforcement to Attr textContent, value and nodeValue. It also improves error handling for mutating attributes within default policy. * LayoutTests/imported/w3c/web-platform-tests/trusted-types/block-string-assignment-to-attribute-via-attribute-node-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/modify-attributes-in-callback-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/trusted-types/modify-attributes-in-callback.html: * Source/WebCore/dom/Attr.cpp: (WebCore::Attr::setValue): (WebCore::Attr::setNodeValue): * Source/WebCore/dom/Attr.h: * Source/WebCore/dom/CharacterData.cpp: (WebCore::CharacterData::setNodeValue): * Source/WebCore/dom/CharacterData.h: * Source/WebCore/dom/Element.cpp: (WebCore::Element::validateAttributeIndex const): (WebCore::Element::setAttribute): * Source/WebCore/dom/Element.h: * Source/WebCore/dom/Node.cpp: (WebCore::Node::setNodeValue): (WebCore::Node::setTextContent): * Source/WebCore/dom/Node.h: Canonical link: https://commits.webkit.org/279118@main
WebKit · May 22, 2024 · be4cdc9 · be4cdc9
1 parent f33204d
commit be4cdc9
Show file tree

Hide file tree

Showing 11 changed files with 85 additions and 42 deletions.
diff --git a/...-tests/trusted-types/block-string-assignment-to-attribute-via-attribute-node-expected.txt b/...-tests/trusted-types/block-string-assignment-to-attribute-via-attribute-node-expected.txt
@@ -1,22 +1,16 @@
+CONSOLE MESSAGE: This requires a TrustedScriptURL value else it violates the following Content Security Policy directive: "require-trusted-types-for 'script'"
+CONSOLE MESSAGE: This requires a TrustedScriptURL value else it violates the following Content Security Policy directive: "require-trusted-types-for 'script'"
+CONSOLE MESSAGE: This requires a TrustedHTML value else it violates the following Content Security Policy directive: "require-trusted-types-for 'script'"
+CONSOLE MESSAGE: This requires a TrustedHTML value else it violates the following Content Security Policy directive: "require-trusted-types-for 'script'"
+CONSOLE MESSAGE: This requires a TrustedScript value else it violates the following Content Security Policy directive: "require-trusted-types-for 'script'"
+CONSOLE MESSAGE: This requires a TrustedScript value else it violates the following Content Security Policy directive: "require-trusted-types-for 'script'"
 
 
 PASS Sanity check: Setting non-TT attributes still works.
-FAIL Set script.src via textContent assert_throws_js: function "_ => {
-      element.attributes[0].textContent = "sldkjsfldk";
-    }" did not throw
-FAIL Set script.src via nodeValue assert_throws_js: function "_ => {
-      element.attributes[0].nodeValue = "sdflkgjdlkgjdg";
-    }" did not throw
-FAIL Set iframe.srcdoc via textContent assert_throws_js: function "_ => {
-      element.attributes[0].textContent = "sldkjsfldk";
-    }" did not throw
-FAIL Set iframe.srcdoc via nodeValue assert_throws_js: function "_ => {
-      element.attributes[0].nodeValue = "sdflkgjdlkgjdg";
-    }" did not throw
-FAIL Set div.onclick via textContent assert_throws_js: function "_ => {
-      element.attributes[0].textContent = "sldkjsfldk";
-    }" did not throw
-FAIL Set div.onclick via nodeValue assert_throws_js: function "_ => {
-      element.attributes[0].nodeValue = "sdflkgjdlkgjdg";
-    }" did not throw
+PASS Set script.src via textContent
+PASS Set script.src via nodeValue
+PASS Set iframe.srcdoc via textContent
+PASS Set iframe.srcdoc via nodeValue
+PASS Set div.onclick via textContent
+PASS Set div.onclick via nodeValue
 
diff --git a/.../imported/w3c/web-platform-tests/trusted-types/modify-attributes-in-callback-expected.txt b/.../imported/w3c/web-platform-tests/trusted-types/modify-attributes-in-callback-expected.txt
@@ -2,4 +2,7 @@
 
 PASS Ensure the right attributes are modified.
 PASS Ensure the deleted attributes is modified.
+PASS Ensure toggleAttribute results in an empty attribute.
+PASS Ensure setAttribute results in right attribute value.
+PASS Ensure setAttributeNS results in right attribute value.
 
diff --git a/LayoutTests/imported/w3c/web-platform-tests/trusted-types/modify-attributes-in-callback.html b/LayoutTests/imported/w3c/web-platform-tests/trusted-types/modify-attributes-in-callback.html
@@ -7,6 +7,7 @@
 </head>
 <body>
 <iframe id="iframe" data-x="" srcdoc="content" onmouseover=""></iframe>
+<div id="div"></div>
 <script>
   // This is a regression test for https://g-issues.chromium.org/issues/333739948
   // The test should hold true for any browser that supports Trusted Types.
@@ -17,6 +18,14 @@
       assert_equals(sink, 'HTMLIFrameElement srcdoc');
       iframe.removeAttribute(target);
       return s;
+    },
+    createScript: (s) => {
+      if (s == 'accepted') {
+        return s;
+      }
+
+      div.setAttribute('onmouseover', 'accepted');
+      return s;
     }
   });
 
@@ -37,4 +46,25 @@
     assert_equals(iframe.srcdoc, "new srcdoc value");
   }, "Ensure the deleted attributes is modified.");
 
+  test(t => {
+    div.toggleAttribute('onmouseover');
+    assert_equals(div.attributes.length, 2);
+    assert_equals(div.attributes.onmouseover.value, '');
+    div.removeAttribute('onmouseover');
+  }, "Ensure toggleAttribute results in an empty attribute.");
+
+  test(t => {
+    div.setAttribute('onmouseover', 'foo');
+    assert_equals(div.attributes.length, 2);
+    assert_equals(div.attributes.onmouseover.value, 'foo');
+    div.removeAttribute('onmouseover');
+  }, "Ensure setAttribute results in right attribute value.");
+
+  test(t => {
+    div.setAttributeNS(null, 'onmouseover', 'foo');
+    assert_equals(div.attributes.length, 2);
+    assert_equals(div.attributes.onmouseover.value, 'foo');
+    div.removeAttribute('onmouseover');
+  }, "Ensure setAttributeNS results in right attribute value.");
+
 </script>
diff --git a/Source/WebCore/dom/Attr.cpp b/Source/WebCore/dom/Attr.cpp
@@ -94,17 +94,19 @@ ExceptionOr<void> Attr::setPrefix(const AtomString& prefix)
     return { };
 }
 
-void Attr::setValue(const AtomString& value)
+ExceptionOr<void> Attr::setValue(const AtomString& value)
 {
     if (RefPtr element = m_element.get())
-        element->setAttribute(qualifiedName(), value);
+        return element->setAttribute(qualifiedName(), value, true);
     else
         m_standaloneValue = value;
+
+    return { };
 }
 
-void Attr::setNodeValue(const String& value)
+ExceptionOr<void> Attr::setNodeValue(const String& value)
 {
-    setValue(value.isNull() ? emptyAtom() : AtomString(value));
+    return setValue(value.isNull() ? emptyAtom() : AtomString(value));
 }
 
 Ref<Node> Attr::cloneNodeInternal(Document& targetDocument, CloningOperation)

diff --git a/Source/WebCore/dom/Attr.h b/Source/WebCore/dom/Attr.h
@@ -45,7 +45,7 @@ class Attr final : public Node {
     Element* ownerElement() const { return m_element.get(); }
 
     WEBCORE_EXPORT AtomString value() const;
-    WEBCORE_EXPORT void setValue(const AtomString&);
+    WEBCORE_EXPORT ExceptionOr<void> setValue(const AtomString&);
 
     const QualifiedName& qualifiedName() const { return m_name; }
 
@@ -65,7 +65,7 @@ class Attr final : public Node {
     String nodeName() const final { return name(); }
 
     String nodeValue() const final { return value(); }
-    void setNodeValue(const String&) final;
+    ExceptionOr<void> setNodeValue(const String&) final;
 
     ExceptionOr<void> setPrefix(const AtomString&) final;
 

diff --git a/Source/WebCore/dom/CharacterData.cpp b/Source/WebCore/dom/CharacterData.cpp
@@ -162,9 +162,10 @@ String CharacterData::nodeValue() const
     return m_data;
 }
 
-void CharacterData::setNodeValue(const String& nodeValue)
+ExceptionOr<void> CharacterData::setNodeValue(const String& nodeValue)
 {
     setData(nodeValue);
+    return { };
 }
 
 void CharacterData::setDataWithoutUpdate(const String& data)

diff --git a/Source/WebCore/dom/CharacterData.h b/Source/WebCore/dom/CharacterData.h
@@ -65,7 +65,7 @@ class CharacterData : public Node {
 
 private:
     String nodeValue() const final;
-    void setNodeValue(const String&) final;
+    ExceptionOr<void> setNodeValue(const String&) final;
     void notifyParentAfterChange(const ContainerNode::ChildChange&);
 
     void parentOrShadowHostNode() const = delete; // Call parentNode() instead.

diff --git a/Source/WebCore/dom/Element.cpp b/Source/WebCore/dom/Element.cpp
@@ -1987,10 +1987,8 @@ static ExceptionOr<String> trustedTypesCompliantAttributeValue(const String attr
 
 ALWAYS_INLINE unsigned Element::validateAttributeIndex(unsigned index, const QualifiedName& qname) const
 {
-    if (index == ElementData::attributeNotFound)
-        return index;
-
-    ASSERT(elementData());
+    if (!elementData())
+        return ElementData::attributeNotFound;
 
     if ((index < elementData()->length()) && (elementData()->attributeAt(index).name() == qname))
         return index;
@@ -2064,11 +2062,24 @@ ExceptionOr<void> Element::setAttribute(const AtomString& qualifiedName, const T
     return { };
 }
 
-void Element::setAttribute(const QualifiedName& name, const AtomString& value)
+ExceptionOr<void> Element::setAttribute(const QualifiedName& name, const AtomString& value, bool enforceTrustedTypes)
 {
     synchronizeAttribute(name);
-    unsigned index = elementData() ? elementData()->findAttributeIndexByName(name) : ElementData::attributeNotFound;
-    setAttributeInternal(index, name, value, InSynchronizationOfLazyAttribute::No);
+    if (enforceTrustedTypes && document().scriptExecutionContext()->settingsValues().trustedTypesEnabled) {
+        auto attributeTypeAndSink = trustedTypeForAttribute(nodeName(), name.localName().convertToASCIILowercase(), this->namespaceURI(), name.namespaceURI());
+        auto attributeValue = trustedTypesCompliantAttributeValue(attributeTypeAndSink.attributeType, value, this, attributeTypeAndSink.sink);
+
+        if (attributeValue.hasException())
+            return attributeValue.releaseException();
+
+        unsigned index = elementData() ? elementData()->findAttributeIndexByName(name) : ElementData::attributeNotFound;
+        setAttributeInternal(index, name, AtomString(attributeValue.releaseReturnValue()), InSynchronizationOfLazyAttribute::No);
+    } else {
+        unsigned index = elementData() ? elementData()->findAttributeIndexByName(name) : ElementData::attributeNotFound;
+        setAttributeInternal(index, name, value, InSynchronizationOfLazyAttribute::No);
+    }
+
+    return { };
 }
 
 void Element::setAttributeWithoutOverwriting(const QualifiedName& name, const AtomString& value)

diff --git a/Source/WebCore/dom/Element.h b/Source/WebCore/dom/Element.h
@@ -166,7 +166,7 @@ class Element : public ContainerNode {
     AtomString getAttributeForBindings(const QualifiedName&, ResolveURLs = ResolveURLs::NoExcludingURLsForPrivacy) const;
     template<typename... QualifiedNames>
     inline const AtomString& getAttribute(const QualifiedName&, const QualifiedNames&...) const;
-    WEBCORE_EXPORT void setAttribute(const QualifiedName&, const AtomString& value);
+    WEBCORE_EXPORT ExceptionOr<void> setAttribute(const QualifiedName&, const AtomString& value, bool enforceTrustedTypes = false);
     void setAttributeWithoutOverwriting(const QualifiedName&, const AtomString& value);
     WEBCORE_EXPORT void setAttributeWithoutSynchronization(const QualifiedName&, const AtomString& value);
     void setSynchronizedLazyAttribute(const QualifiedName&, const AtomString& value);

diff --git a/Source/WebCore/dom/Node.cpp b/Source/WebCore/dom/Node.cpp
@@ -500,9 +500,10 @@ String Node::nodeValue() const
     return String();
 }
 
-void Node::setNodeValue(const String&)
+ExceptionOr<void> Node::setNodeValue(const String&)
 {
     // By default, setting nodeValue has no effect.
+    return { };
 }
 
 RefPtr<NodeList> Node::childNodes()
@@ -1798,26 +1799,27 @@ String Node::textContent(bool convertBRsToNewlines) const
     return isNullString ? String() : content.toString();
 }
 
-void Node::setTextContent(String&& text)
+ExceptionOr<void> Node::setTextContent(String&& text)
 {           
     switch (nodeType()) {
     case ATTRIBUTE_NODE:
     case TEXT_NODE:
     case CDATA_SECTION_NODE:
     case COMMENT_NODE:
     case PROCESSING_INSTRUCTION_NODE:
-        setNodeValue(WTFMove(text));
-        return;
+        return setNodeValue(WTFMove(text));
     case ELEMENT_NODE:
     case DOCUMENT_FRAGMENT_NODE:
         uncheckedDowncast<ContainerNode>(*this).stringReplaceAll(WTFMove(text));
-        return;
+        return { };
     case DOCUMENT_NODE:
     case DOCUMENT_TYPE_NODE:
         // Do nothing.
-        return;
+        return { };
     }
     ASSERT_NOT_REACHED();
+
+    return { };
 }
 
 static SHA1::Digest hashPointer(const void* pointer)

diff --git a/Source/WebCore/dom/Node.h b/Source/WebCore/dom/Node.h
@@ -143,7 +143,7 @@ class Node : public EventTarget {
     inline bool hasTagName(const SVGQualifiedName&) const;
     virtual String nodeName() const = 0;
     virtual String nodeValue() const;
-    virtual void setNodeValue(const String&);
+    virtual ExceptionOr<void> setNodeValue(const String&);
     NodeType nodeType() const { return nodeTypeFromBitFields(m_typeBitFields); }
     virtual size_t approximateMemoryCost() const { return sizeof(*this); }
     ContainerNode* parentNode() const;
@@ -208,7 +208,7 @@ class Node : public EventTarget {
     WEBCORE_EXPORT const AtomString& lookupNamespaceURI(const AtomString& prefix) const;
 
     WEBCORE_EXPORT String textContent(bool convertBRsToNewlines = false) const;
-    WEBCORE_EXPORT void setTextContent(String&&);
+    WEBCORE_EXPORT ExceptionOr<void> setTextContent(String&&);
 
     Node* lastDescendant() const;
     Node* firstDescendant() const;