HTML: getSVGDocument() / contentDocument #20432

annevk · 2019-11-25T14:05:05Z

Additionally, make content document (also used by contentDocument) perform the same origin-domain comparison on the two documents involved. Tests: web-platform-tests/wpt#20432. Fixes #5094.

html/semantics/embedded-content/the-iframe-element/cross-origin-to-whom.window.js

html/semantics/embedded-content/the-iframe-element/support/document-with-embedded-svg.html

zcorpan · 2019-12-09T13:37:36Z

Reassigning to @domenic since he's reviewing whatwg/html#5109

domenic · 2019-12-09T19:21:59Z

This needs a test for the change mentioned at https://github.com/whatwg/html/pull/5109/files#r353452968.

In particular we want:

A situation where the current settings object is cross-origin-domain with the nested BC's active document, but container's node document is same-origin-domain. Before the change this would return null; now it returns the document.
A situation where the current settings object is same-origin-domain with the nested BC's active document, but container's node document is cross-origin-domain. Before the change this would return the document; now it returns null.

Consider a.example.com which embeds frame1 = a.example.com, frame2 = a.example.com

(1) should be doable with something like: Grab the getSVGDocument function from frame2. Then, change frame2's document.domain to "example.com". Now, call frame2GetSVGDocument.call(frame1). At that time the current settings object is frame2 (which is COD with frame1) but the container's node document is same-origin-domain (both are a.example.com)

(2) should be doable with something like: Grab getSVGDocument from frame2. Then, change frame1 and frame2's document.domain to "example.com". Now, call frame2GetSVGDocument.call(frame2). At that time the current settings object is frame2 (which is SOD with frame1) but the container's node document is COD (since the outer frame hasn't changed document.domain).

annevk · 2019-12-10T08:30:10Z

The current tests already test the case where the SVG's container and the SVG are same origin and the current settings object is cross origin-domain and that doesn't appear to affect things one bit.

(I'm actually wondering how the current tests are not running into "perform a security check" now. Maybe nobody performs that for non-Window-non-Location objects these days.)

These days all implementations only have security checks on a couple of objects. Firefox used to have cross-origin object wrapper, but those are no longer web observable. Tests: html/browsers/origin/cross-origin-objects/cross-origin-objects.html and web-platform-tests/wpt#20432.

annevk · 2019-12-12T16:48:18Z

Added that other test, results are as expected. Current settings object is not involved.

…e origin-domain with the embedder document, but both cross origin-domain with the SVG

domenic

Looks good but please add comments similar to what I suggest, to explain why the expectations are as they are. These kind of tests are really hard to debug or maintain otherwise.

domenic · 2019-12-12T18:28:10Z

html/semantics/embedded-content/the-iframe-element/cross-origin-to-whom.window.js

+        possibleDocument = instance[api]();
+      } else {
+        possibleDocument = instance[api];
+      }


Nit: this could be simpler as

const possibleDocument = api === "getSVGDocument" ? instance[api]() : instance[api];

domenic · 2019-12-12T18:38:05Z

html/semantics/embedded-content/the-iframe-element/cross-origin-to-whom.window.js

+  }
+  frame.onload = t.step_func_done(() => {
+    const instances = Object.keys(elements).map(element => frame.contentDocument.querySelector(element));
+    instances.forEach(instance => assert_apis(instance));


Suggest adding a comment like "Everything is same-origin-domain; this should definitely work"

domenic · 2019-12-12T18:38:48Z

html/semantics/embedded-content/the-iframe-element/cross-origin-to-whom.window.js

+    instances.forEach(instance => assert_apis(instance));
+    document.domain = document.domain;
+    assert_equals(frame.contentDocument, null);
+    instances.forEach(instance => assert_apis(instance));


Suggest adding a comment like "Current settings object is now cross-origin domain, but container node document and nested BC node document are still same-origin domain, so all the APIs should still work".

domenic · 2019-12-12T18:39:17Z

html/semantics/embedded-content/the-iframe-element/cross-origin-to-whom-part-2.window.js

+  }
+  frame.onload = t.step_func_done(() => {
+    const instances = Object.keys(elements).map(element => frame.contentDocument.querySelector(element));
+    instances.forEach(instance => assert_apis(instance));


Siggest adding a comment like "Everything is same-origin domain; this should definitely work"

domenic · 2019-12-12T18:39:56Z

html/semantics/embedded-content/the-iframe-element/cross-origin-to-whom-part-2.window.js

+      const svgDocument = element_to_document(instance);
+      svgDocument.domain = svgDocument.domain;
+    });
+    instances.forEach(instance => assert_apis(instance, true));


Suggest adding a comment like "nested BC node document is now cross-origin domain with container BC's active document and current settings object, so we expect the APIs to start returning null"

domenic · 2019-12-12T18:40:25Z

html/semantics/embedded-content/the-iframe-element/cross-origin-to-whom-part-2.window.js

+    instances.forEach(instance => assert_apis(instance, true));
+    document.domain = document.domain;
+    assert_equals(frame.contentDocument, null);
+    instances.forEach(instance => assert_apis(instance, true));


Suggest adding a comment like "nested BC node document is now same-origin domain with the current settings object, but still cross-origin domain with the container BC's node document, so the APIs should still return null"

domenic · 2019-12-12T18:40:55Z

html/semantics/embedded-content/the-iframe-element/support/document-with-embedded-svg.html

+  let attr = "src";
+  if (name === "object") {
+    attr = "data";
+  }


Nit: this could be simpler as

const attr = name === "object" ? "data" : "src";

domenic · 2019-12-12T18:41:32Z

html/semantics/embedded-content/the-iframe-element/cross-origin-to-whom.window.js

+      } else {
+        possibleDocument = instance[api];
+      }
+      assert_not_equals(possibleDocument, null, `${name}[${api}]`);


Nit:

Suggested change

assert_not_equals(possibleDocument, null, `${name}[${api}]`);

assert_not_equals(possibleDocument, null, `${name}.${api}`);

(here and in the other file)

Additionally, make content document (also used by contentDocument) perform the same origin-domain comparison on the two documents involved rather than involve the current settings object. Tests: web-platform-tests/wpt#20432. Fixes #5094.

HTML: getSVGDocument() / contentDocument

f71c507

For whatwg/html#5094.

wpt-pr-bot added the html label Nov 25, 2019

wpt-pr-bot assigned zcorpan Nov 25, 2019

wpt-pr-bot requested review from domenic, foolip, jdm, jgraham, zcorpan and zqzhang November 25, 2019 14:05

annevk mentioned this pull request Nov 25, 2019

fix: Revert HTMLDocument removal whatwg/html#5104

Open

annevk mentioned this pull request Nov 25, 2019

Define getSVGDocument() in terms of content document whatwg/html#5109

Merged

zcorpan requested changes Nov 26, 2019

View reviewed changes

html/semantics/embedded-content/the-iframe-element/cross-origin-to-whom.window.js Outdated Show resolved Hide resolved

html/semantics/embedded-content/the-iframe-element/support/document-with-embedded-svg.html Outdated Show resolved Hide resolved

zcorpan mentioned this pull request Dec 6, 2019

[docs] Test style guide/review checklist: avoid confusing constructs #20648

Open

nits

0d9d839

annevk requested a review from zcorpan December 9, 2019 08:57

zcorpan approved these changes Dec 9, 2019

View reviewed changes

zcorpan assigned domenic and unassigned zcorpan Dec 9, 2019

annevk mentioned this pull request Dec 10, 2019

Only run "perform a security check" for Window and Location objects whatwg/html#5141

Merged

proof there are only two objects involved, part two

04e428e

let's assert even more to also test current settings object being sam…

be58570

…e origin-domain with the embedder document, but both cross origin-domain with the SVG

domenic approved these changes Dec 12, 2019

View reviewed changes

nits + additional assert

7e87a3e

annevk merged commit b5f3eaf into master Dec 13, 2019

annevk deleted the annevk/getSVGDocument branch December 13, 2019 09:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTML: getSVGDocument() / contentDocument #20432

HTML: getSVGDocument() / contentDocument #20432

annevk commented Nov 25, 2019

zcorpan commented Dec 9, 2019

domenic commented Dec 9, 2019 •

edited

Loading

annevk commented Dec 10, 2019

annevk commented Dec 12, 2019

domenic left a comment

domenic Dec 12, 2019

domenic Dec 12, 2019

domenic Dec 12, 2019

domenic Dec 12, 2019

domenic Dec 12, 2019

domenic Dec 12, 2019

domenic Dec 12, 2019

domenic Dec 12, 2019

	assert_not_equals(possibleDocument, null, `${name}[${api}]`);
	assert_not_equals(possibleDocument, null, `${name}.${api}`);

HTML: getSVGDocument() / contentDocument #20432

HTML: getSVGDocument() / contentDocument #20432

Conversation

annevk commented Nov 25, 2019

zcorpan commented Dec 9, 2019

domenic commented Dec 9, 2019 • edited Loading

annevk commented Dec 10, 2019

annevk commented Dec 12, 2019

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic commented Dec 9, 2019 •

edited

Loading