Skip to content

Commit

Permalink
Revert "Fenced frames: Disallow URLs with potentially dangling markup"
Browse files Browse the repository at this point in the history 
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]: 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 <gtanzer@chromium.org>
> Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
> Commit-Queue: Dominic Farolino <dom@chromium.org>
> 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 <lukasza@chromium.org>
Reviewed-by: Sebastien Lalancette <seblalancette@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: Sebastien Lalancette <seblalancette@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1015011}
NOKEYCHECK=True
GitOrigin-RevId: 245696bb639221c56332cdea83bd961c99330183
  • Loading branch information
@anforowicz
anforowicz authored and Copybara-Service committed Jun 16, 2022
1 parent a6ae8b2 commit 94ed574
Show file tree
Hide file tree
Showing 6 changed files with 7 additions and 138 deletions.
5 changes: 2 additions & 3 deletions blink/common/fenced_frame/fenced_frame_utils.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:";
Expand Down
42 changes: 0 additions & 42 deletions ...l/fenced-frame-mparch/wpt_internal/fenced_frame/disallowed-navigations.https-expected.txt
View file

This file was deleted.

24 changes: 0 additions & 24 deletions ...nced-frame-shadow-dom/wpt_internal/fenced_frame/disallowed-navigations.https-expected.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,37 +7,13 @@ 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.

66 changes: 5 additions & 61 deletions blink/web_tests/wpt_internal/fenced_frame/disallowed-navigations.https.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,17 +45,6 @@
`;
}

// 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.
Expand Down Expand Up @@ -83,18 +72,17 @@
});
}, "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
// - 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 => {
Expand Down Expand Up @@ -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
Expand All @@ -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)}`);
Expand Down Expand Up @@ -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}'`);
}

</script>

</body>
7 changes: 0 additions & 7 deletions blink/web_tests/wpt_internal/fenced_frame/resources/report-url.html
View file

This file was deleted.

1 change: 0 additions & 1 deletion blink/web_tests/wpt_internal/fenced_frame/resources/report-url.html.headers
View file

This file was deleted.

0 comments on commit 94ed574

Please sign in to comment.