Remove setInnerHTML completely

The conversation [1] about the recent changes to setInnerHTML have led to the conclusion [2] that perhaps we shouldn't add a new XSS sink method at all. That would "fix" the declarative Shadow DOM problem, but would create a new sink that all security libraries would need to know about and handle. Seems like not a good trade. In the meantime, a polyfill can stand in for setInnerHTML: Element.prototype.setInnerHTML = function(content) { const fragment = (new DOMParser()).parseFromString(`<pre>${content}</pre>`, 'text/html', {includeShadowRoots: true}); this.replaceChildren(...fragment.body.firstChild.childNodes); }; [1] whatwg/dom#912 [2] whatwg/dom#912 (comment) Bug: 1042130 Change-Id: Ibaf15a3edf86be9a720225dea2ba2741f2882b8c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2555589 Auto-Submit: Mason Freed <[email protected]> Commit-Queue: Kouhei Ueno <[email protected]> Reviewed-by: Kouhei Ueno <[email protected]> Cr-Commit-Position: refs/heads/master@{#830501} GitOrigin-RevId: af023967d0ff024b3008265de09a68d5041f919c
mjfroman · Nov 24, 2020 · c14c4be · c14c4be
1 parent 36d59f1
commit c14c4be
Show file tree

Hide file tree

Showing 20 changed files with 64 additions and 183 deletions.
diff --git a/blink/renderer/bindings/generated_in_core.gni b/blink/renderer/bindings/generated_in_core.gni
@@ -301,8 +301,6 @@ generated_dictionary_sources_in_core = [
   "$root_gen_dir/third_party/blink/renderer/bindings/core/v8/v8_scroll_to_options.h",
   "$root_gen_dir/third_party/blink/renderer/bindings/core/v8/v8_security_policy_violation_event_init.cc",
   "$root_gen_dir/third_party/blink/renderer/bindings/core/v8/v8_security_policy_violation_event_init.h",
-  "$root_gen_dir/third_party/blink/renderer/bindings/core/v8/v8_set_inner_html_options.cc",
-  "$root_gen_dir/third_party/blink/renderer/bindings/core/v8/v8_set_inner_html_options.h",
   "$root_gen_dir/third_party/blink/renderer/bindings/core/v8/v8_shadow_root_init.cc",
   "$root_gen_dir/third_party/blink/renderer/bindings/core/v8/v8_shadow_root_init.h",
   "$root_gen_dir/third_party/blink/renderer/bindings/core/v8/v8_stream_pipe_options.cc",

diff --git a/blink/renderer/bindings/idl_in_core.gni b/blink/renderer/bindings/idl_in_core.gni
@@ -162,7 +162,6 @@ static_idl_files_in_core = get_path_info(
           "//third_party/blink/renderer/core/dom/pointer_lock_options.idl",
           "//third_party/blink/renderer/core/dom/processing_instruction.idl",
           "//third_party/blink/renderer/core/dom/range.idl",
-          "//third_party/blink/renderer/core/dom/set_inner_html_options.idl",
           "//third_party/blink/renderer/core/dom/shadow_root.idl",
           "//third_party/blink/renderer/core/dom/shadow_root_init.idl",
           "//third_party/blink/renderer/core/dom/static_range.idl",

diff --git a/blink/renderer/core/core_idl_files.gni b/blink/renderer/core/core_idl_files.gni
@@ -652,7 +652,6 @@ core_dictionary_idl_files =
                     "dom/idle_request_options.idl",
                     "dom/mutation_observer_init.idl",
                     "dom/pointer_lock_options.idl",
-                    "dom/set_inner_html_options.idl",
                     "dom/shadow_root_init.idl",
                     "dom/events/add_event_listener_options.idl",
                     "dom/events/custom_event_init.idl",

diff --git a/blink/renderer/core/dom/element.cc b/blink/renderer/core/dom/element.cc
@@ -44,7 +44,6 @@
 #include "third_party/blink/renderer/bindings/core/v8/v8_pointer_lock_options.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_scroll_into_view_options.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_scroll_to_options.h"
-#include "third_party/blink/renderer/bindings/core/v8/v8_set_inner_html_options.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_shadow_root_init.h"
 #include "third_party/blink/renderer/core/accessibility/ax_context.h"
 #include "third_party/blink/renderer/core/accessibility/ax_object_cache.h"
@@ -4467,13 +4466,11 @@ String Element::outerHTML() const {
 }
 
 void Element::SetInnerHTMLInternal(const String& html,
-                                   const SetInnerHTMLOptions* options,
+                                   bool include_shadow_roots,
                                    ExceptionState& exception_state) {
   if (html.IsEmpty() && !HasNonInBodyInsertionMode()) {
     setTextContent(html);
   } else {
-    bool include_shadow_roots =
-        options->hasIncludeShadowRoots() && options->includeShadowRoots();
     if (DocumentFragment* fragment = CreateFragmentForInnerOuterHTML(
             html, this, kAllowScriptingContent, "innerHTML",
             include_shadow_roots, exception_state)) {
@@ -4493,16 +4490,13 @@ void Element::SetInnerHTMLInternal(const String& html,
 void Element::setInnerHTML(const String& html,
                            ExceptionState& exception_state) {
   probe::BreakableLocation(GetExecutionContext(), "Element.setInnerHTML");
-  const SetInnerHTMLOptions options;
-  SetInnerHTMLInternal(html, &options, exception_state);
+  SetInnerHTMLInternal(html, /*include_shadow_roots=*/false, exception_state);
 }
 
-void Element::setInnerHTMLWithOptions(const String& html,
-                                      const SetInnerHTMLOptions* options,
-                                      ExceptionState& exception_state) {
-  DCHECK(RuntimeEnabledFeatures::DeclarativeShadowDOMEnabled(
-      GetExecutionContext()));
-  SetInnerHTMLInternal(html, options, exception_state);
+void Element::setInnerHTMLWithDeclarativeShadowDOMForTesting(
+    const String& html) {
+  SetInnerHTMLInternal(html, /*include_shadow_roots=*/true,
+                       ASSERT_NO_EXCEPTION);
 }
 
 String Element::getInnerHTML(const GetInnerHTMLOptions* options) const {

diff --git a/blink/renderer/core/dom/element.h b/blink/renderer/core/dom/element.h
@@ -66,7 +66,6 @@ class ExceptionState;
 class FloatQuad;
 class FloatSize;
 class FocusOptions;
-class SetInnerHTMLOptions;
 class GetInnerHTMLOptions;
 class HTMLTemplateElement;
 class Image;
@@ -698,9 +697,7 @@ class CORE_EXPORT Element : public ContainerNode, public Animatable {
   String innerHTML() const;
   String outerHTML() const;
   void setInnerHTML(const String&, ExceptionState& = ASSERT_NO_EXCEPTION);
-  void setInnerHTMLWithOptions(const String&,
-                               const SetInnerHTMLOptions*,
-                               ExceptionState& = ASSERT_NO_EXCEPTION);
+  void setInnerHTMLWithDeclarativeShadowDOMForTesting(const String& html);
   String getInnerHTML(const GetInnerHTMLOptions* options) const;
   void setOuterHTML(const String&, ExceptionState& = ASSERT_NO_EXCEPTION);
 
@@ -1190,7 +1187,7 @@ class CORE_EXPORT Element : public ContainerNode, public Animatable {
                                                         const AtomicString&);
 
   void SetInnerHTMLInternal(const String&,
-                            const SetInnerHTMLOptions*,
+                            bool include_shadow_roots,
                             ExceptionState&);
 
   ElementRareData* GetElementRareData() const;

diff --git a/blink/renderer/core/dom/element.idl b/blink/renderer/core/dom/element.idl
@@ -96,8 +96,7 @@ callback ScrollStateCallback = void (ScrollState scrollState);
     [Affects=Nothing, CEReactions, CustomElementCallbacks, RaisesException=Setter] attribute [TreatNullAs=EmptyString, StringContext=TrustedHTML] DOMString outerHTML;
     [CEReactions, CustomElementCallbacks, RaisesException] void insertAdjacentHTML(DOMString position, HTMLString text);
 
-    // Declarative Shadow DOM setInnerHTML/getInnerHTML() functions.
-    [Affects=Nothing, RuntimeEnabled=DeclarativeShadowDOM, RuntimeCallStatsCounter=ElementSetInnerHTML, RaisesException, ImplementedAs=setInnerHTMLWithOptions] void setInnerHTML(HTMLString html, optional SetInnerHTMLOptions options = {});
+    // Declarative Shadow DOM getInnerHTML() function.
     [Affects=Nothing, RuntimeEnabled=DeclarativeShadowDOM, RuntimeCallStatsCounter=ElementGetInnerHTML] HTMLString getInnerHTML(optional GetInnerHTMLOptions options = {});
 
     // Pointer Lock

diff --git a/blink/renderer/core/dom/set_inner_html_options.idl b/blink/renderer/core/dom/set_inner_html_options.idl
diff --git a/blink/renderer/core/dom/shadow_root.cc b/blink/renderer/core/dom/shadow_root.cc
@@ -28,7 +28,6 @@
 
 #include "third_party/blink/public/platform/platform.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_get_inner_html_options.h"
-#include "third_party/blink/renderer/bindings/core/v8/v8_set_inner_html_options.h"
 #include "third_party/blink/renderer/core/css/resolver/style_resolver.h"
 #include "third_party/blink/renderer/core/css/style_change_reason.h"
 #include "third_party/blink/renderer/core/css/style_engine.h"
@@ -133,32 +132,15 @@ String ShadowRoot::getInnerHTML(const GetInnerHTMLOptions* options) const {
       include_closed_roots);
 }
 
-void ShadowRoot::SetInnerHTMLInternal(const String& html,
-                                      const SetInnerHTMLOptions* options,
-                                      ExceptionState& exception_state) {
-  bool include_shadow_roots =
-      options->hasIncludeShadowRoots() && options->includeShadowRoots();
+void ShadowRoot::setInnerHTML(const String& html,
+                              ExceptionState& exception_state) {
   if (DocumentFragment* fragment = CreateFragmentForInnerOuterHTML(
           html, &host(), kAllowScriptingContent, "innerHTML",
-          include_shadow_roots, exception_state)) {
+          /*include_shadow_roots=*/false, exception_state)) {
     ReplaceChildrenWithFragment(this, fragment, exception_state);
   }
 }
 
-void ShadowRoot::setInnerHTML(const String& html,
-                              ExceptionState& exception_state) {
-  const SetInnerHTMLOptions options;
-  SetInnerHTMLInternal(html, &options, exception_state);
-}
-
-void ShadowRoot::setInnerHTMLWithOptions(const String& html,
-                                         const SetInnerHTMLOptions* options,
-                                         ExceptionState& exception_state) {
-  DCHECK(RuntimeEnabledFeatures::DeclarativeShadowDOMEnabled(
-      GetExecutionContext()));
-  SetInnerHTMLInternal(html, options, exception_state);
-}
-
 void ShadowRoot::RebuildLayoutTree(WhitespaceAttacher& whitespace_attacher) {
   DCHECK(!NeedsReattachLayoutTree());
   DCHECK(!ChildNeedsReattachLayoutTree());

diff --git a/blink/renderer/core/dom/shadow_root.h b/blink/renderer/core/dom/shadow_root.h
@@ -40,7 +40,6 @@ namespace blink {
 
 class Document;
 class ExceptionState;
-class SetInnerHTMLOptions;
 class GetInnerHTMLOptions;
 class ShadowRootV0;
 class SlotAssignment;
@@ -141,9 +140,6 @@ class CORE_EXPORT ShadowRoot final : public DocumentFragment, public TreeScope {
   String innerHTML() const;
   String getInnerHTML(const GetInnerHTMLOptions* options) const;
   void setInnerHTML(const String&, ExceptionState& = ASSERT_NO_EXCEPTION);
-  void setInnerHTMLWithOptions(const String&,
-                               const SetInnerHTMLOptions*,
-                               ExceptionState& = ASSERT_NO_EXCEPTION);
 
   Node* Clone(Document&, CloneChildrenFlag) const override;
 
@@ -194,10 +190,6 @@ class CORE_EXPORT ShadowRoot final : public DocumentFragment, public TreeScope {
 
   SlotAssignment& EnsureSlotAssignment();
 
-  void SetInnerHTMLInternal(const String&,
-                            const SetInnerHTMLOptions*,
-                            ExceptionState&);
-
   void AddChildShadowRoot() { ++child_shadow_root_count_; }
   void RemoveChildShadowRoot() {
     DCHECK_GT(child_shadow_root_count_, 0u);

diff --git a/blink/renderer/core/dom/shadow_root.idl b/blink/renderer/core/dom/shadow_root.idl
@@ -36,8 +36,7 @@ interface ShadowRoot : DocumentFragment {
     readonly attribute boolean delegatesFocus;
     [RuntimeEnabled=ManualSlotting] readonly attribute SlotAssignmentMode slotAssignment;
 
-    // Declarative Shadow DOM setInnerHTML/getInnerHTML() functions.
-    [Affects=Nothing, RuntimeEnabled=DeclarativeShadowDOM, RuntimeCallStatsCounter=ElementSetInnerHTML, RaisesException, ImplementedAs=setInnerHTMLWithOptions] void setInnerHTML(HTMLString html, optional SetInnerHTMLOptions options = {});
+    // Declarative Shadow DOM getInnerHTML() function.
     [Affects=Nothing, RuntimeEnabled=DeclarativeShadowDOM, RuntimeCallStatsCounter=ElementGetInnerHTML] HTMLString getInnerHTML(optional GetInnerHTMLOptions options = {});
 };
 

diff --git a/blink/renderer/core/dom/slot_assignment_test.cc b/blink/renderer/core/dom/slot_assignment_test.cc
@@ -5,7 +5,6 @@
 #include "third_party/blink/renderer/core/dom/flat_tree_traversal.h"
 
 #include "testing/gtest/include/gtest/gtest.h"
-#include "third_party/blink/renderer/bindings/core/v8/v8_set_inner_html_options.h"
 #include "third_party/blink/renderer/core/dom/document.h"
 #include "third_party/blink/renderer/core/dom/element.h"
 #include "third_party/blink/renderer/core/dom/node.h"
@@ -70,9 +69,7 @@ void SlotAssignmentTest::SetUp() {
 
 void SlotAssignmentTest::SetBody(const char* html) {
   Element* body = GetDocument().body();
-  SetInnerHTMLOptions options;
-  options.setIncludeShadowRoots(true);
-  body->setInnerHTMLWithOptions(String::FromUTF8(html), &options);
+  body->setInnerHTMLWithDeclarativeShadowDOMForTesting(String::FromUTF8(html));
   RemoveWhiteSpaceOnlyTextNode(*body);
 }
 

diff --git a/blink/renderer/platform/bindings/runtime_call_stats.h b/blink/renderer/platform/bindings/runtime_call_stats.h
@@ -250,7 +250,6 @@ class PLATFORM_EXPORT RuntimeCallStats {
 #define CALLBACK_COUNTERS(V)                       \
   BINDINGS_METHOD(V, ElementGetBoundingClientRect) \
   BINDINGS_METHOD(V, ElementGetInnerHTML)          \
-  BINDINGS_METHOD(V, ElementSetInnerHTML)          \
   BINDINGS_METHOD(V, EventTargetDispatchEvent)     \
   BINDINGS_METHOD(V, HTMLElementClick)             \
   BINDINGS_METHOD(V, NodeAppendChild)              \

diff --git a/...ests/external/wpt/shadow-dom/declarative/declarative-shadow-dom-attachment.tentative.html b/...ests/external/wpt/shadow-dom/declarative/declarative-shadow-dom-attachment.tentative.html
@@ -5,6 +5,7 @@
 <script src='/resources/testharness.js'></script>
 <script src='/resources/testharnessreport.js'></script>
 <script src='../resources/shadow-dom-utils.js'></script>
+<script src="support/helpers.js"></script>
 
 <script>
 const shadowContent = '<span>Shadow tree</span><slot></slot>';
@@ -18,7 +19,7 @@
   const declarativeString = `<${elementType} id=theelement>${getDeclarativeContent(mode, delegatesFocus)}
      <span class='lightdom'>${lightDomTextContent}</span></${elementType}>`;
   const wrapper = document.createElement('div');
-  wrapper.setInnerHTML(declarativeString, {includeShadowRoots: true});
+  setInnerHTML(wrapper, declarativeString);
   const element = wrapper.querySelector('#theelement');
   return {wrapper: wrapper, element: element};
 }

diff --git a/...web_tests/external/wpt/shadow-dom/declarative/declarative-shadow-dom-basic.tentative.html b/...web_tests/external/wpt/shadow-dom/declarative/declarative-shadow-dom-basic.tentative.html
@@ -5,6 +5,7 @@
 <link rel="help" href="https://github.com/whatwg/dom/issues/831">
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
+<script src="support/helpers.js"></script>
 
 <div id="host" style="display:none">
   <template shadowroot="open">
@@ -32,14 +33,14 @@
 
 test(() => {
   const div = document.createElement('div');
-  div.setInnerHTML(`
+  setInnerHTML(div,`
     <div id="host">
       <template shadowroot="open">
         <slot id="s1" name="slot1"></slot>
       </template>
       <div id="c1" slot="slot1"></div>
     </div>
-    `, {includeShadowRoots: true});
+    `);
   const host = div.querySelector('#host');
   const c1 = host.querySelector('#c1');
   assert_true(!!c1);
@@ -53,12 +54,12 @@
 
 test(() => {
   const div = document.createElement('div');
-  div.setInnerHTML(`
+  setInnerHTML(div,`
     <div id="host">
       <template shadowroot="invalid">
       </template>
     </div>
-    `, {includeShadowRoots: true});
+    `);
   const host = div.querySelector('#host');
   assert_equals(host.shadowRoot, null, "Shadow root was found");
   const tmpl = host.querySelector('template');
@@ -69,25 +70,25 @@
 
 test(() => {
   const div = document.createElement('div');
-  div.setInnerHTML(`
+  setInnerHTML(div,`
     <div id="host">
       <template shadowroot="closed">
       </template>
     </div>
-    `, {includeShadowRoots: true});
+    `);
   const host = div.querySelector('#host');
   assert_equals(host.shadowRoot, null, "Closed shadow root");
   assert_equals(host.querySelector('template'), null, "No template - converted to shadow root");
 }, 'Declarative Shadow DOM: Closed shadow root attribute');
 
 test(() => {
   const div = document.createElement('div');
-  div.setInnerHTML(`
+  setInnerHTML(div,`
     <div id="host">
       <template shadowroot="open">
         <slot id="s1" name="slot1"></slot>
     </div>
-    `, {includeShadowRoots: true});
+    `);
   const host = div.querySelector('#host');
   assert_equals(host.querySelector('#s1'), null, "Should be inside shadow root");
   assert_equals(host.querySelector('template'), null, "No leftover template node");
@@ -98,35 +99,25 @@
 
 test(() => {
   const div = document.createElement('div');
-  div.setInnerHTML(`
+  setInnerHTML(div,`
     <div id="host">
       <template shadowroot="open" shadowrootdelegatesfocus>
       </template>
     </div>
-    `, {includeShadowRoots: true});
+    `);
   var host = div.querySelector('#host');
   assert_true(!!host.shadowRoot,"No shadow root found");
   assert_true(host.shadowRoot.delegatesFocus,"delegatesFocus should be true");
-  div.setInnerHTML(`
+  setInnerHTML(div,`
     <div id="host">
       <template shadowroot="open">
       </template>
     </div>
-    `, {includeShadowRoots: true});
+    `);
   host = div.querySelector('#host');
   assert_true(!!host.shadowRoot,"No shadow root found");
   assert_false(host.shadowRoot.delegatesFocus,"delegatesFocus should be false without the shadowrootdelegatesfocus attribute");
 }, 'Declarative Shadow DOM: delegates focus attribute');
-
-test(() => {
-  const host = document.createElement('div');
-  // Root element of setInnerHTML is a <template shadowroot>:
-  host.setInnerHTML('<template shadowroot=open></template>', {allowShadowRoot: true});
-  assert_equals(host.shadowRoot, null, "Shadow root should not be present");
-  const tmpl = host.querySelector('template');
-  assert_true(!!tmpl,"Template should still be present");
-  assert_equals(tmpl.getAttribute('shadowroot'),"open","'shadowroot' attribute should still be present");
-}, 'Declarative Shadow DOM: setInnerHTML root element');
 </script>
 
 <div id="multi-host" style="display:none">