Commit 11a66767 authored by Raymes Khoury's avatar Raymes Khoury Committed by Commit Bot

Revert "Allow webrtc requests to be made from about:blank URLs that are secure contexts"

This reverts commit 16d3c419.

Based on more discussion about this in https://bugs.chromium.org/p/chromium/issues/detail?id=742049 I've decided to revert the patch we landed here.

I'm going to let the behavior that we released in M60 stand, which is that mic/camera requests from about:blank origins will fail. The reasons being:
1) It's consistent with the behavior of other permissions
2) It's potentially confusing/misleading for a user granting permission because the origin displayed in the omnibox is "about:blank".

If the behavior is changed such that document.write triggers a navigation and the URL in omnibox is updated to reflect the origin, then it would be more acceptable to allow permission requests to occur.

Original change's description:
> Allow webrtc requests to be made from about:blank URLs that are secure contexts
> 
> https://codereview.chromium.org/2880503002 added checks to
> PermissionContextBase::GetPermissionStatus that ensured that the
> embedding origin was secure if the permission required a secure context.
> The problem is that in the case of about:blank URLs, the browser does
> not know if they are secure or not. They may be secure contexts (from
> the perspective of blink) if opened and modified by a secure context.
> 
> This change caused media permissions to stop working from about:blank
> URLs so it is removed. There are still renderer-side checks which ensure
> that the current context is secure before permitting media access. In
> the long term we should unify the browser/renderer-side secure context
> checks.
> 
> BUG=740540
> 
> Change-Id: Iff319f62284f9d22ca54706526b2747a73477e86
> Reviewed-on: https://chromium-review.googlesource.com/569544
> Reviewed-by: Timothy Loh <timloh@chromium.org>
> Commit-Queue: Raymes Khoury <raymes@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#486288}

TBR=raymes@chromium.org,timloh@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 740540
Change-Id: I55be050873745910e282be420d4ac8030a2d141f
Reviewed-on: https://chromium-review.googlesource.com/572479Reviewed-by: default avatarRaymes Khoury <raymes@chromium.org>
Commit-Queue: Raymes Khoury <raymes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487325}
parent bbec1033
......@@ -164,17 +164,3 @@ TEST_F(MediaStreamDevicePermissionContextTests, TestMicSecureQueryingUrl) {
TEST_F(MediaStreamDevicePermissionContextTests, TestCameraSecureQueryingUrl) {
TestSecureQueryingUrl(CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA);
}
// An embedding origin of about:blank should not be blocked. crbug.com/740540.
TEST_F(MediaStreamDevicePermissionContextTests, TestAboutBlankNotBlocked) {
TestPermissionContext permission_context(
profile(), CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA);
GURL secure_url("https://www.example.com");
EXPECT_EQ(
CONTENT_SETTING_ASK,
permission_context
.GetPermissionStatus(nullptr /* render_frame_host */, secure_url,
GURL("about:blank").GetOrigin())
.content_setting);
}
......@@ -230,20 +230,21 @@ PermissionResult PermissionContextBase::GetPermissionStatus(
}
if (IsRestrictedToSecureOrigins()) {
// TODO(raymes): The secure origin check here in the browser should match
// what we do in blink (i.e. what is described in the secure context spec).
// Right now, we can't even check IsOriginSecure(embedding_origin) because
// the |embedding_origin| is obtained from the WebContents which does match
// the origin of the document in blink in all cases. For example, an
// about:blank URL may be a secure context in blink, but it is not treated
// as such in the browser at present. The |requesting_origin| is passed from
// blink and so is accurate under normal circumstances but may be forged by
// a compromised renderer so even this check below is not particularly
// secure...
if (!content::IsOriginSecure(requesting_origin)) {
return PermissionResult(CONTENT_SETTING_BLOCK,
PermissionStatusSource::UNSPECIFIED);
}
// TODO(raymes): We should check the entire chain of embedders here whenever
// possible as this corresponds to the requirements of the secure contexts
// spec and matches what is implemented in blink. Right now we just check
// the top level and requesting origins. Note: chrome-extension:// origins
// are currently exempt from checking the embedder chain. crbug.com/530507.
if (!requesting_origin.SchemeIs(extensions::kExtensionScheme) &&
!content::IsOriginSecure(embedding_origin)) {
return PermissionResult(CONTENT_SETTING_BLOCK,
PermissionStatusSource::UNSPECIFIED);
}
}
// Check whether the feature is enabled for the frame by feature policy. We
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment