From 94ed5743a1862cb4a886d92a8b12de02fd8a9dba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Anforowicz?= Date: Thu, 16 Jun 2022 18:35:13 +0000 Subject: [PATCH] Revert "Fenced frames: Disallow URLs with potentially dangling markup" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit fe04c0639254e5d021da539d321f2e3a64a0085c. Reason for revert: Suspecting that this CL caused https://crbug.com/1337025 Original change's description: > 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]: https://github.com/whatwg/fetch/pull/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 > Reviewed-by: Alex Moshchuk > Commit-Queue: Dominic Farolino > Cr-Commit-Position: refs/heads/main@{#1014928} Bug: 1301333, 1318970 Change-Id: If4884825408c38882f439d8c5e47ba0271dc67a1 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3710059 Owners-Override: Łukasz Anforowicz Reviewed-by: Sebastien Lalancette Bot-Commit: Rubber Stamper Auto-Submit: Łukasz Anforowicz Reviewed-by: Łukasz Anforowicz Commit-Queue: Sebastien Lalancette Cr-Commit-Position: refs/heads/main@{#1015011} NOKEYCHECK=True GitOrigin-RevId: 245696bb639221c56332cdea83bd961c99330183 --- .../common/fenced_frame/fenced_frame_utils.cc | 5 +- .../disallowed-navigations.https-expected.txt | 42 ------------ .../disallowed-navigations.https-expected.txt | 24 ------- .../disallowed-navigations.https.html | 66 ++----------------- .../fenced_frame/resources/report-url.html | 7 -- .../resources/report-url.html.headers | 1 - 6 files changed, 7 insertions(+), 138 deletions(-) delete mode 100644 blink/web_tests/platform/generic/virtual/fenced-frame-mparch/wpt_internal/fenced_frame/disallowed-navigations.https-expected.txt delete mode 100644 blink/web_tests/wpt_internal/fenced_frame/resources/report-url.html delete mode 100644 blink/web_tests/wpt_internal/fenced_frame/resources/report-url.html.headers diff --git a/blink/common/fenced_frame/fenced_frame_utils.cc b/blink/common/fenced_frame/fenced_frame_utils.cc index 1932339ddf1..0b2f9394e1b 100644 --- a/blink/common/fenced_frame/fenced_frame_utils.cc +++ b/blink/common/fenced_frame/fenced_frame_utils.cc @@ -16,9 +16,8 @@ namespace blink { bool IsValidFencedFrameURL(const GURL& url) { if (!url.is_valid()) return false; - return (url.SchemeIs(url::kHttpsScheme) || url.IsAboutBlank() || - net::IsLocalhost(url)) && - !url.parsed_for_possibly_invalid_spec().potentially_dangling_markup; + return url.SchemeIs(url::kHttpsScheme) || url.IsAboutBlank() || + net::IsLocalhost(url); } const char kURNUUIDprefix[] = "urn:uuid:"; diff --git a/blink/web_tests/platform/generic/virtual/fenced-frame-mparch/wpt_internal/fenced_frame/disallowed-navigations.https-expected.txt b/blink/web_tests/platform/generic/virtual/fenced-frame-mparch/wpt_internal/fenced_frame/disallowed-navigations.https-expected.txt deleted file mode 100644 index 130205a23fe..00000000000 --- a/blink/web_tests/platform/generic/virtual/fenced-frame-mparch/wpt_internal/fenced_frame/disallowed-navigations.https-expected.txt +++ /dev/null @@ -1,42 +0,0 @@ -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 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 - 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 -// - https: URLs with dangling markup +function getTimeoutPromise(t) { + return new Promise(resolve => + t.step_timeout(() => resolve("NOT LOADED"), 2000)); +} + for (const mode of ['opaque-ads', 'default']) { promise_test(async t => { @@ -138,19 +126,6 @@ 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 @@ -159,7 +134,6 @@ // - 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)}`); @@ -201,36 +175,6 @@ 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}'`); -} - 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 deleted file mode 100644 index e0b7d0982ae..00000000000 --- a/blink/web_tests/wpt_internal/fenced_frame/resources/report-url.html +++ /dev/null @@ -1,7 +0,0 @@ - - -A page embedded as a fenced frame that reports the document URL - 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 deleted file mode 100644 index 6247f6d6321..00000000000 --- a/blink/web_tests/wpt_internal/fenced_frame/resources/report-url.html.headers +++ /dev/null @@ -1 +0,0 @@ -Supports-Loading-Mode: fenced-frame \ No newline at end of file