Skip to content

Commit

Permalink
Remove setInnerHTML completely
Browse files Browse the repository at this point in the history
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 <masonfreed@chromium.org>
Commit-Queue: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#830501}
  • Loading branch information
mfreed7 authored and Commit Bot committed Nov 24, 2020
1 parent 13ac053 commit af02396
Show file tree
Hide file tree
Showing 20 changed files with 64 additions and 183 deletions.
2 changes: 0 additions & 2 deletions third_party/blink/renderer/bindings/generated_in_core.gni
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
1 change: 0 additions & 1 deletion third_party/blink/renderer/bindings/idl_in_core.gni
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
1 change: 0 additions & 1 deletion third_party/blink/renderer/core/core_idl_files.gni
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
18 changes: 6 additions & 12 deletions third_party/blink/renderer/core/dom/element.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)) {
Expand All @@ -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 {
Expand Down
7 changes: 2 additions & 5 deletions third_party/blink/renderer/core/dom/element.h
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ class ExceptionState;
class FloatQuad;
class FloatSize;
class FocusOptions;
class SetInnerHTMLOptions;
class GetInnerHTMLOptions;
class HTMLTemplateElement;
class Image;
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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;
Expand Down
3 changes: 1 addition & 2 deletions third_party/blink/renderer/core/dom/element.idl
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

This file was deleted.

24 changes: 3 additions & 21 deletions third_party/blink/renderer/core/dom/shadow_root.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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());
Expand Down
8 changes: 0 additions & 8 deletions third_party/blink/renderer/core/dom/shadow_root.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ namespace blink {

class Document;
class ExceptionState;
class SetInnerHTMLOptions;
class GetInnerHTMLOptions;
class ShadowRootV0;
class SlotAssignment;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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);
Expand Down
3 changes: 1 addition & 2 deletions third_party/blink/renderer/core/dom/shadow_root.idl
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {});
};

Expand Down
5 changes: 1 addition & 4 deletions third_party/blink/renderer/core/dom/slot_assignment_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) \
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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>';
Expand All @@ -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};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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">
Expand Down Expand Up @@ -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);
Expand All @@ -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');
Expand All @@ -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");
Expand All @@ -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">
Expand Down
Loading

0 comments on commit af02396

Please sign in to comment.