Fenced frames: Disallow URLs with potentially dangling markup

There is an old Fetch Standard PR up for review that blocks resource requests whose URL contains potentially dangling markup [1]. This is for security purposes, see [2] and [3]. While non-standard yet, Chromium has shipped this behavior, and we intend to do the same for fenced frames. This CL implements potentially dangling markup restrictions on all embedder-provided URLs for fenced frame navigations. When a URL with dangling markup is passed to SharedStorage's `selectURL()` method, it is parsed and partially sanitized, therefore the resulting urn:uuid can be successfully navigated to. When crbug.com/1318970 is fixed, SharedStorage will reject these URLs as inputs. [1]: whatwg/fetch#519 [2]: https://bugs.chromium.org/p/chromium/issues/detail?id=1039885 [3]: https://bugs.chromium.org/p/chromium/issues/detail?id=1301333 Bug: 1301333, 1318970 Change-Id: I1ada9de23b05795499408988529fa3a49486aea3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3702854 Reviewed-by: Garrett Tanzer <[email protected]> Reviewed-by: Alex Moshchuk <[email protected]> Commit-Queue: Dominic Farolino <[email protected]> Cr-Commit-Position: refs/heads/main@{#1014928} NOKEYCHECK=True GitOrigin-RevId: fe04c0639254e5d021da539d321f2e3a64a0085c
mjfroman · Jun 16, 2022 · 1d2d666 · 1d2d666
1 parent 35e0a76
commit 1d2d666
Show file tree

Hide file tree

Showing 6 changed files with 138 additions and 7 deletions.
diff --git a/blink/common/fenced_frame/fenced_frame_utils.cc b/blink/common/fenced_frame/fenced_frame_utils.cc
@@ -16,8 +16,9 @@ namespace blink {
 bool IsValidFencedFrameURL(const GURL& url) {
   if (!url.is_valid())
     return false;
-  return url.SchemeIs(url::kHttpsScheme) || url.IsAboutBlank() ||
-         net::IsLocalhost(url);
+  return (url.SchemeIs(url::kHttpsScheme) || url.IsAboutBlank() ||
+          net::IsLocalhost(url)) &&
+         !url.parsed_for_possibly_invalid_spec().potentially_dangling_markup;
 }
 
 const char kURNUUIDprefix[] = "urn:uuid:";

diff --git a/...l/fenced-frame-mparch/wpt_internal/fenced_frame/disallowed-navigations.https-expected.txt b/...l/fenced-frame-mparch/wpt_internal/fenced_frame/disallowed-navigations.https-expected.txt
@@ -0,0 +1,42 @@
+This is a testharness.js-based test.
+PASS iframe data: URL
+PASS iframe blob: URL
+PASS iframe javascript: URL
+PASS fenced frame mode=opaque-ads data: URL
+PASS fenced frame mode=opaque-ads blob: URL
+PASS fenced frame mode=opaque-ads javascript: URL
+PASS fenced frame mode=opaque-ads http: URL
+PASS fenced frame mode=opaque-ads dangling-markup URL with 'blo
+ck<ed'
+PASS fenced frame mode=opaque-ads dangling-markup URL with 'blo\rck<ed'
+PASS fenced frame mode=opaque-ads dangling-markup URL with 'blo	ck<ed'
+PASS fenced frame mode=opaque-ads dangling-markup URL with 'blo<ck
+ed'
+PASS fenced frame mode=opaque-ads dangling-markup URL with 'blo<ck\red'
+PASS fenced frame mode=opaque-ads dangling-markup URL with 'blo<ck	ed'
+PASS fenced frame mode=default data: URL
+PASS fenced frame mode=default blob: URL
+PASS fenced frame mode=default javascript: URL
+PASS fenced frame mode=default http: URL
+PASS fenced frame mode=default dangling-markup URL with 'blo
+ck<ed'
+PASS fenced frame mode=default dangling-markup URL with 'blo\rck<ed'
+PASS fenced frame mode=default dangling-markup URL with 'blo	ck<ed'
+PASS fenced frame mode=default dangling-markup URL with 'blo<ck
+ed'
+PASS fenced frame mode=default dangling-markup URL with 'blo<ck\red'
+PASS fenced frame mode=default dangling-markup URL with 'blo<ck	ed'
+PASS fenced frame opaque URN => data: URL
+PASS fenced frame opaque URN => blob: URL
+PASS fenced frame opaque URN => javascript: URL
+PASS fenced frame opaque URN => http: URL
+FAIL fenced frame opaque URN => https: URL with dangling markup 'blo
+ck<ed' assert_equals: expected "NOT LOADED" but got "https://web-platform.test:8444/wpt_internal/fenced_frame/resources/report-url.html?blo<cked="
+FAIL fenced frame opaque URN => https: URL with dangling markup 'blo\rck<ed' assert_equals: expected "NOT LOADED" but got "https://web-platform.test:8444/wpt_internal/fenced_frame/resources/report-url.html?blo<cked="
+FAIL fenced frame opaque URN => https: URL with dangling markup 'blo	ck<ed' assert_equals: expected "NOT LOADED" but got "https://web-platform.test:8444/wpt_internal/fenced_frame/resources/report-url.html?blo<cked="
+FAIL fenced frame opaque URN => https: URL with dangling markup 'blo<ck
+ed' assert_equals: expected "NOT LOADED" but got "https://web-platform.test:8444/wpt_internal/fenced_frame/resources/report-url.html?blo<cked="
+FAIL fenced frame opaque URN => https: URL with dangling markup 'blo<ck\red' assert_equals: expected "NOT LOADED" but got "https://web-platform.test:8444/wpt_internal/fenced_frame/resources/report-url.html?blo<cked="
+FAIL fenced frame opaque URN => https: URL with dangling markup 'blo<ck	ed' assert_equals: expected "NOT LOADED" but got "https://web-platform.test:8444/wpt_internal/fenced_frame/resources/report-url.html?blo<cked="
+Harness: the test ran to completion.
+
diff --git a/...nced-frame-shadow-dom/wpt_internal/fenced_frame/disallowed-navigations.https-expected.txt b/...nced-frame-shadow-dom/wpt_internal/fenced_frame/disallowed-navigations.https-expected.txt
@@ -7,13 +7,37 @@ PASS fenced frame mode=opaque-ads data: URL
 PASS fenced frame mode=opaque-ads blob: URL
 PASS fenced frame mode=opaque-ads javascript: URL
 PASS fenced frame mode=opaque-ads http: URL
+PASS fenced frame mode=opaque-ads dangling-markup URL with 'blo
+ck<ed'
+PASS fenced frame mode=opaque-ads dangling-markup URL with 'blo\rck<ed'
+PASS fenced frame mode=opaque-ads dangling-markup URL with 'blo	ck<ed'
+PASS fenced frame mode=opaque-ads dangling-markup URL with 'blo<ck
+ed'
+PASS fenced frame mode=opaque-ads dangling-markup URL with 'blo<ck\red'
+PASS fenced frame mode=opaque-ads dangling-markup URL with 'blo<ck	ed'
 PASS fenced frame mode=default data: URL
 PASS fenced frame mode=default blob: URL
 PASS fenced frame mode=default javascript: URL
 PASS fenced frame mode=default http: URL
+PASS fenced frame mode=default dangling-markup URL with 'blo
+ck<ed'
+PASS fenced frame mode=default dangling-markup URL with 'blo\rck<ed'
+PASS fenced frame mode=default dangling-markup URL with 'blo	ck<ed'
+PASS fenced frame mode=default dangling-markup URL with 'blo<ck
+ed'
+PASS fenced frame mode=default dangling-markup URL with 'blo<ck\red'
+PASS fenced frame mode=default dangling-markup URL with 'blo<ck	ed'
 PASS fenced frame opaque URN => data: URL
 PASS fenced frame opaque URN => blob: URL
 PASS fenced frame opaque URN => javascript: URL
 PASS fenced frame opaque URN => http: URL
+FAIL fenced frame opaque URN => https: URL with dangling markup 'blo
+ck<ed' assert_equals: expected "NOT LOADED" but got "https://web-platform.test:8444/wpt_internal/fenced_frame/resources/report-url.html?blo<cked="
+FAIL fenced frame opaque URN => https: URL with dangling markup 'blo\rck<ed' assert_equals: expected "NOT LOADED" but got "https://web-platform.test:8444/wpt_internal/fenced_frame/resources/report-url.html?blo<cked="
+FAIL fenced frame opaque URN => https: URL with dangling markup 'blo	ck<ed' assert_equals: expected "NOT LOADED" but got "https://web-platform.test:8444/wpt_internal/fenced_frame/resources/report-url.html?blo<cked="
+FAIL fenced frame opaque URN => https: URL with dangling markup 'blo<ck
+ed' assert_equals: expected "NOT LOADED" but got "https://web-platform.test:8444/wpt_internal/fenced_frame/resources/report-url.html?blo<cked="
+FAIL fenced frame opaque URN => https: URL with dangling markup 'blo<ck\red' assert_equals: expected "NOT LOADED" but got "https://web-platform.test:8444/wpt_internal/fenced_frame/resources/report-url.html?blo<cked="
+FAIL fenced frame opaque URN => https: URL with dangling markup 'blo<ck	ed' assert_equals: expected "NOT LOADED" but got "https://web-platform.test:8444/wpt_internal/fenced_frame/resources/report-url.html?blo<cked="
 Harness: the test ran to completion.
 
diff --git a/blink/web_tests/wpt_internal/fenced_frame/disallowed-navigations.https.html b/blink/web_tests/wpt_internal/fenced_frame/disallowed-navigations.https.html
@@ -45,6 +45,17 @@
   `;
 }
 
+// These are used in tests that rely on URLs containing dangling markup. See
+// https://github.com/whatwg/fetch/pull/519.
+const kDanglingMarkupSubstrings = [
+  "blo\nck<ed",
+  "blo\rck<ed",
+  "blo\tck<ed",
+  "blo<ck\ned",
+  "blo<ck\red",
+  "blo<ck\ted",
+];
+
 // These are just baseline tests asserting that this test's machinery to load
 // blob:, data:, and javascript: URLs work properly in contexts where they are
 // expected to.
@@ -72,17 +83,18 @@
   });
 }, "iframe javascript: URL");
 
+function getTimeoutPromise(t) {
+  return new Promise(resolve =>
+      t.step_timeout(() => resolve("NOT LOADED"), 2000));
+}
+
 // The following tests ensure that an embedder cannot navigate a fenced frame
 // (with different modes) to:
 //   - data: URLs
 //   - blob: URLs
 //   - javascript: URLs
 //   - http: URLs
-function getTimeoutPromise(t) {
-  return new Promise(resolve =>
-      t.step_timeout(() => resolve("NOT LOADED"), 2000));
-}
-
+//   - https: URLs with dangling markup
 for (const mode of ['opaque-ads', 'default']) {
 
   promise_test(async t => {
@@ -126,6 +138,19 @@
     assert_equals(result, "NOT LOADED");
   }, `fenced frame mode=${mode} http: URL`);
 
+  // These tests assert that fenced frames cannot be navigated to HTTPs URLs
+  // with dangling markup.
+  for (substring of kDanglingMarkupSubstrings) {
+    promise_test(async t => {
+      const key = token();
+      let url_string = generateURL("resources/embeddee.html?blocked", [key]).toString();
+      url_string = url_string.replace("blocked", substring);
+      const fencedframe = attachFencedFrame(url_string, /*mode=*/mode);
+      const loaded_promise = nextValueFromServer(key);
+      const result = await Promise.any([loaded_promise, getTimeoutPromise(t)]);
+      assert_equals(result, "NOT LOADED");
+    }, `fenced frame mode=${mode} dangling-markup URL with '${substring}'`);
+  }
 } // end for.
 
 // The following tests ensure that an embedder cannot navigate a
@@ -134,6 +159,7 @@
 //   - blob: URL
 //   - javascript: URL
 //   - http: URL
+//   - https: URL with dangling markup
 promise_test(async t => {
   const key = token();
   const urn = await generateURN(`data:text/html,${createLocalSource(key)}`);
@@ -175,6 +201,36 @@
   const result = await Promise.any([loaded_promise, getTimeoutPromise(t)]);
   assert_equals(result, "NOT LOADED");
 }, "fenced frame opaque URN => http: URL");
+
+// These tests assert that fenced frames cannot be navigated to a urn:uuid URL
+// that represents an HTTPS URLs with dangling markup.
+for (substring of kDanglingMarkupSubstrings) {
+  promise_test(async t => {
+    const key = token();
+
+    // Copied from from `generateURN()`, since we have to modify the final URL
+    // that goes into `selectURL`.
+    try {
+      await sharedStorage.worklet.addModule(
+        "/wpt_internal/shared_storage/resources/simple-module.js");
+    } catch (e) {
+      // See documentation in `generateURN()`.
+    }
+
+    let url_string = generateURL("resources/report-url.html?blocked", [key]).toString();
+    url_string = url_string.replace("blocked", substring);
+
+    const urn = await sharedStorage.selectURL(
+      "test-url-selection-operation", [url_string], {data: {'mockResult': 0}}
+    );
+
+    const fencedframe = attachFencedFrame(urn, /*mode=*/'opaque-ads');
+    const loaded_promise = nextValueFromServer(key);
+    const result = await Promise.any([loaded_promise, getTimeoutPromise(t)]);
+    assert_equals(result, "NOT LOADED");
+  }, `fenced frame opaque URN => https: URL with dangling markup '${substring}'`);
+}
+
 </script>
 
 </body>
diff --git a/blink/web_tests/wpt_internal/fenced_frame/resources/report-url.html b/blink/web_tests/wpt_internal/fenced_frame/resources/report-url.html
@@ -0,0 +1,7 @@
+<!DOCTYPE html>
+<script src="utils.js"></script>
+<title>A page embedded as a fenced frame that reports the document URL</title>
+<script>
+const [uuid] = parseKeylist();
+writeValueToServer(uuid, location.href);
+</script>
diff --git a/blink/web_tests/wpt_internal/fenced_frame/resources/report-url.html.headers b/blink/web_tests/wpt_internal/fenced_frame/resources/report-url.html.headers
@@ -0,0 +1 @@
+Supports-Loading-Mode: fenced-frame