Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2454563003:
Fix web accessible resource checks in ShouldAllowOpenURL (Closed)
|
Created:
4 years, 1 month ago by alexmos Modified:
4 years, 1 month ago CC:
chromium-reviews, extensions-reviews_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, chromium-apps-reviews_chromium.org, site-isolation-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix web accessible resource checks in ShouldAllowOpenURL
(This is joint work with nick@)
When a web page navigates an extension main frame to another extension
URL via a proxy, RenderFrameProxyHost::OnOpenURL calls
NavigatorImpl::RequestTransferURL, which checks whether the
destination URL points to a web-accessible resource in
ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL.
This CL fixes several problems with that check:
1. It doesn't work properly for blob/filesystem URL, since it
references schemes on the destination GURL directly. This leads to
a security problem where all blob/filesystem URLs are allowed. To
fix this, use url::Origin to handle nested URL cases properly,
similarly to other fixes in this area (e.g., issue 645028).
2. It ignores cases where the source SiteInstance's site URL isn't an
HTTP, HTTPS, or extension scheme. Thus, a new tab with a data or
about:blank URL bypasses WAR checks. This artificial restriction
apparently was added to make a test pass several years ago. This
CL removes it, and fixes one test that was wrong and which relied
on this logic. The test is
ExtensionBrowserTest.WindowOpenNoPrivileges; it tried to
window.open a non-web-accessible resource while incorrectly
assuming it was web-accessible. Since it did so from about:blank
with a blank SiteInstance, this bug allowed the test to work.
3. The things that ShouldAllowOpenURL does block aren't blocked
correctly for transfers. RequestTransferURL rewrites the blocked
URL to about:blank, and later, NavigatorImpl::NavigateToEntry
decides that this is a transfer to the same RFH
(is_transfer_to_same == true), so it happily lets the old
navigation continue and load the *old* (disallowed) URL. To fix
this, we return early instead of rewriting dest_url to
about:blank.
4. The SiteInstance plumbed through to ShouldAllowOpenURL when going
via RenderFrameProxyHost::OnOpenURL is wrong: it takes the current
SiteInstance of the frame that's being navigated, rather than the
SiteInstance in which the navigation on the proxy was started.
RenderFrameProxyHost::OnOpenURL already passes the right
SiteInstance as source_site_instance to RequestTransferURL, so it
just needs to use it when it's available.
Two new tests are added:
NestedURLNavigationsViaProxyBlocked for case 1.
WindowOpenInaccessibleResourceFromDataURL for case 2.
The fixes to ShouldAllowOpenURL also apply to subframe navigations as
well as main frame navigations (since it doesn't distinguish between
the two). That is, subframe navigations that go through proxies,
transfers, or OpenURL, will now also have the unsafe blob/filesystem
URL blocking enforced, whereas before this was only done for main
frames by other checks. The test exercising this for subframes,
NestedURLNavigationsViaProxyBlocked, was updated accordingly.
Why don't our other checks work?
--------------------------------
1. ExtensionNavigationThrottle also performs web-accessible resource
checks checks, but that currently happens only for subframes. Thus,
ShouldAllowOpenURL are the only checks that would work in this
scenario for main frames. Fundamentally, ExtensionNavigationThrottle
can't perform the WAR checks for main frames until we plumb through
the initiator SiteInstance (i.e., who navigated the proxy, and not the
StartingSiteInstance, which corresponds to the frame that's being
navigated) to NavigationHandle.
2. ChromeExtensionNetworkDelegate also blocks unsafe navigations to
blob/filesystem URLs. It doesn't apply in this case, since the process
where this navigation is happening is already an extension process.
3. url_request_util::AllowCrossRendererResourceLoad also allows this
navigation, as it explicitly excludes all main frames from WAR
enforcement:
// Navigating the main frame to an extension URL is allowed, even if not
// explicitly listed as web_accessible_resource.
if (info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) {
*allowed = true;
return true;
}
BUG=656752, 645028, 652887
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/1c3da4f745a8a4d67573ca386fc9ad24ced878d4
Cr-Commit-Position: refs/heads/master@{#429532}
Patch Set 1 #Patch Set 2 : Test fixes #Patch Set 3 : Try to remove incorrect scheme checks #Patch Set 4 : Fix ExtensionBrowserTest.WindowOpenNoPrivileges #Patch Set 5 : Cleanup + test #Patch Set 6 : More cleanup #Patch Set 7 : Tighten check a bit more #
Total comments: 15
Patch Set 8 : Add metrics #Patch Set 9 : Rebase #
Total comments: 31
Patch Set 10 : Charlie's comments #
Total comments: 2
Patch Set 11 : Devlin's and Alexei's comments #
Total comments: 4
Patch Set 12 : Round 2 of Devlin's comments #Dependent Patchsets: Messages
Total messages: 83 (62 generated)
Description was changed from ========== Fix web accessible resource checks for nested URLs in ShouldAllowOpenURL BUG=656752 ========== to ========== Fix web accessible resource checks for nested URLs in ShouldAllowOpenURL BUG=656752 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by alexmos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Fix web accessible resource checks for nested URLs in ShouldAllowOpenURL BUG=656752 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix web accessible resource checks for nested URLs in ShouldAllowOpenURL BUG=656752, 645028 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by alexmos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by alexmos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by alexmos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by alexmos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by alexmos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by alexmos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by alexmos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
Fix web accessible resource checks for nested URLs in ShouldAllowOpenURL
BUG=656752, 645028
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
to
==========
Fix web accessible resource checks for nested URLs in ShouldAllowOpenURL
(This is joint work with nick@)
When a web page navigates an extension main frame to another extension
URL via a proxy, RenderFrameProxyHost::OnOpenURL calls
NavigatorImpl::RequestTransferURL, which checks whether the
destination URL points to a web-accessible resource in
ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL.
This CL fixes several problems with that check:
1. It doesn't work properly for blob/filesystem URL, since it
references schemes on the destination GURL directly. This leads to
a security problem where all blob/filesystem URLs are allowed. To
fix this, use url::Origin to handle nested URL cases properly,
similarly to other fixes in this area (e.g., issue 645028).
2. It ignores cases where the source SiteInstance's site URL isn't an
HTTP, HTTPS, or extension scheme. Thus, a new tab with a data or
about:blank URL bypasses WAR checks. This artificial restriction
apparently was added to make a test pass several years ago. This
CL removes it, and fixes one test that was wrong and which relied
on this logic. The test is
ExtensionBrowserTest.WindowOpenNoPrivileges; it tried to
window.open a non-web-accessible resource while incorrectly
assuming it was web-accessible. Since it did so from about:blank
with a blank SiteInstance, this bug allowed the test to work.
3. The things that ShouldAllowOpenURL does block aren't blocked
correctly for transfers. RequestTransferURL rewrites the blocked
URL to about:blank, and later, NavigatorImpl::NavigateToEntry
decides that this is a transfer to the same RFH
(is_transfer_to_same == true), so it happily lets the old
navigation continue and load the *old* (disallowed) URL. To fix
this, we return early instead of rewriting dest_url to
about:blank.
4. The SiteInstance plumbed through to ShouldAllowOpenURL when going
via RenderFrameProxyHost::OnOpenURL is wrong: it takes the current
SiteInstance of the frame that's being navigated, rather than the
SiteInstance in which the navigation on the proxy was started.
RenderFrameProxyHost::OnOpenURL already passes the right
SiteInstance as source_site_instance to RequestTransferURL, so it
just needs to use it when it's available.
Two new tests are added:
NestedURLNavigationsViaProxyBlocked for case 1.
WindowOpenInaccessibleResourceFromDataURL for case 2.
Why don't our other checks work?
--------------------------------
1. ExtensionNavigationThrottle also performs web-accessible resource
checks checks, but that currently happens only for subframes. Thus,
ShouldAllowOpenURL are the only checks that would work in this
scenario for main frames. Fundamentally, ExtensionNavigationThrottle
can't perform the WAR checks for main frames until we plumb through
the initiator SiteInstance (i.e., who navigated the proxy, and not the
StartingSiteInstance, which corresponds to the frame that's being
navigated) to NavigationHandle.
2. ChromeExtensionNetworkDelegate also blocks unsafe navigations to
blob/filesystem URLs. It doesn't apply in this case, since the process
where this navigation is happening is already an extension process.
3. url_request_util::AllowCrossRendererResourceLoad also allows this
navigation, as it explicitly excludes all main frames from WAR
enforcement:
// Navigating the main frame to an extension URL is allowed, even if not
// explicitly listed as web_accessible_resource.
if (info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) {
*allowed = true;
return true;
}
BUG=656752, 645028
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from
==========
Fix web accessible resource checks for nested URLs in ShouldAllowOpenURL
(This is joint work with nick@)
When a web page navigates an extension main frame to another extension
URL via a proxy, RenderFrameProxyHost::OnOpenURL calls
NavigatorImpl::RequestTransferURL, which checks whether the
destination URL points to a web-accessible resource in
ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL.
This CL fixes several problems with that check:
1. It doesn't work properly for blob/filesystem URL, since it
references schemes on the destination GURL directly. This leads to
a security problem where all blob/filesystem URLs are allowed. To
fix this, use url::Origin to handle nested URL cases properly,
similarly to other fixes in this area (e.g., issue 645028).
2. It ignores cases where the source SiteInstance's site URL isn't an
HTTP, HTTPS, or extension scheme. Thus, a new tab with a data or
about:blank URL bypasses WAR checks. This artificial restriction
apparently was added to make a test pass several years ago. This
CL removes it, and fixes one test that was wrong and which relied
on this logic. The test is
ExtensionBrowserTest.WindowOpenNoPrivileges; it tried to
window.open a non-web-accessible resource while incorrectly
assuming it was web-accessible. Since it did so from about:blank
with a blank SiteInstance, this bug allowed the test to work.
3. The things that ShouldAllowOpenURL does block aren't blocked
correctly for transfers. RequestTransferURL rewrites the blocked
URL to about:blank, and later, NavigatorImpl::NavigateToEntry
decides that this is a transfer to the same RFH
(is_transfer_to_same == true), so it happily lets the old
navigation continue and load the *old* (disallowed) URL. To fix
this, we return early instead of rewriting dest_url to
about:blank.
4. The SiteInstance plumbed through to ShouldAllowOpenURL when going
via RenderFrameProxyHost::OnOpenURL is wrong: it takes the current
SiteInstance of the frame that's being navigated, rather than the
SiteInstance in which the navigation on the proxy was started.
RenderFrameProxyHost::OnOpenURL already passes the right
SiteInstance as source_site_instance to RequestTransferURL, so it
just needs to use it when it's available.
Two new tests are added:
NestedURLNavigationsViaProxyBlocked for case 1.
WindowOpenInaccessibleResourceFromDataURL for case 2.
Why don't our other checks work?
--------------------------------
1. ExtensionNavigationThrottle also performs web-accessible resource
checks checks, but that currently happens only for subframes. Thus,
ShouldAllowOpenURL are the only checks that would work in this
scenario for main frames. Fundamentally, ExtensionNavigationThrottle
can't perform the WAR checks for main frames until we plumb through
the initiator SiteInstance (i.e., who navigated the proxy, and not the
StartingSiteInstance, which corresponds to the frame that's being
navigated) to NavigationHandle.
2. ChromeExtensionNetworkDelegate also blocks unsafe navigations to
blob/filesystem URLs. It doesn't apply in this case, since the process
where this navigation is happening is already an extension process.
3. url_request_util::AllowCrossRendererResourceLoad also allows this
navigation, as it explicitly excludes all main frames from WAR
enforcement:
// Navigating the main frame to an extension URL is allowed, even if not
// explicitly listed as web_accessible_resource.
if (info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) {
*allowed = true;
return true;
}
BUG=656752, 645028
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
to
==========
Fix web accessible resource checks for nested URLs in ShouldAllowOpenURL
(This is joint work with nick@)
When a web page navigates an extension main frame to another extension
URL via a proxy, RenderFrameProxyHost::OnOpenURL calls
NavigatorImpl::RequestTransferURL, which checks whether the
destination URL points to a web-accessible resource in
ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL.
This CL fixes several problems with that check:
1. It doesn't work properly for blob/filesystem URL, since it
references schemes on the destination GURL directly. This leads to
a security problem where all blob/filesystem URLs are allowed. To
fix this, use url::Origin to handle nested URL cases properly,
similarly to other fixes in this area (e.g., issue 645028).
2. It ignores cases where the source SiteInstance's site URL isn't an
HTTP, HTTPS, or extension scheme. Thus, a new tab with a data or
about:blank URL bypasses WAR checks. This artificial restriction
apparently was added to make a test pass several years ago. This
CL removes it, and fixes one test that was wrong and which relied
on this logic. The test is
ExtensionBrowserTest.WindowOpenNoPrivileges; it tried to
window.open a non-web-accessible resource while incorrectly
assuming it was web-accessible. Since it did so from about:blank
with a blank SiteInstance, this bug allowed the test to work.
3. The things that ShouldAllowOpenURL does block aren't blocked
correctly for transfers. RequestTransferURL rewrites the blocked
URL to about:blank, and later, NavigatorImpl::NavigateToEntry
decides that this is a transfer to the same RFH
(is_transfer_to_same == true), so it happily lets the old
navigation continue and load the *old* (disallowed) URL. To fix
this, we return early instead of rewriting dest_url to
about:blank.
4. The SiteInstance plumbed through to ShouldAllowOpenURL when going
via RenderFrameProxyHost::OnOpenURL is wrong: it takes the current
SiteInstance of the frame that's being navigated, rather than the
SiteInstance in which the navigation on the proxy was started.
RenderFrameProxyHost::OnOpenURL already passes the right
SiteInstance as source_site_instance to RequestTransferURL, so it
just needs to use it when it's available.
Two new tests are added:
NestedURLNavigationsViaProxyBlocked for case 1.
WindowOpenInaccessibleResourceFromDataURL for case 2.
The fixes to ShouldAllowOpenURL also apply to subframe navigations as
well as main frame navigations (since it doesn't distinguish between
the two). That is, subframe navigations that go through proxies,
transfers, or OpenURL, will now also have the unsafe blob/filesystem
URL blocking enforced, whereas before this was only done for main
frames by other checks. The test exercising this for subframes,
NestedURLNavigationsViaProxyBlocked, was updated accordingly.
Why don't our other checks work?
--------------------------------
1. ExtensionNavigationThrottle also performs web-accessible resource
checks checks, but that currently happens only for subframes. Thus,
ShouldAllowOpenURL are the only checks that would work in this
scenario for main frames. Fundamentally, ExtensionNavigationThrottle
can't perform the WAR checks for main frames until we plumb through
the initiator SiteInstance (i.e., who navigated the proxy, and not the
StartingSiteInstance, which corresponds to the frame that's being
navigated) to NavigationHandle.
2. ChromeExtensionNetworkDelegate also blocks unsafe navigations to
blob/filesystem URLs. It doesn't apply in this case, since the process
where this navigation is happening is already an extension process.
3. url_request_util::AllowCrossRendererResourceLoad also allows this
navigation, as it explicitly excludes all main frames from WAR
enforcement:
// Navigating the main frame to an extension URL is allowed, even if not
// explicitly listed as web_accessible_resource.
if (info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) {
*allowed = true;
return true;
}
BUG=656752, 645028
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
Description was changed from
==========
Fix web accessible resource checks for nested URLs in ShouldAllowOpenURL
(This is joint work with nick@)
When a web page navigates an extension main frame to another extension
URL via a proxy, RenderFrameProxyHost::OnOpenURL calls
NavigatorImpl::RequestTransferURL, which checks whether the
destination URL points to a web-accessible resource in
ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL.
This CL fixes several problems with that check:
1. It doesn't work properly for blob/filesystem URL, since it
references schemes on the destination GURL directly. This leads to
a security problem where all blob/filesystem URLs are allowed. To
fix this, use url::Origin to handle nested URL cases properly,
similarly to other fixes in this area (e.g., issue 645028).
2. It ignores cases where the source SiteInstance's site URL isn't an
HTTP, HTTPS, or extension scheme. Thus, a new tab with a data or
about:blank URL bypasses WAR checks. This artificial restriction
apparently was added to make a test pass several years ago. This
CL removes it, and fixes one test that was wrong and which relied
on this logic. The test is
ExtensionBrowserTest.WindowOpenNoPrivileges; it tried to
window.open a non-web-accessible resource while incorrectly
assuming it was web-accessible. Since it did so from about:blank
with a blank SiteInstance, this bug allowed the test to work.
3. The things that ShouldAllowOpenURL does block aren't blocked
correctly for transfers. RequestTransferURL rewrites the blocked
URL to about:blank, and later, NavigatorImpl::NavigateToEntry
decides that this is a transfer to the same RFH
(is_transfer_to_same == true), so it happily lets the old
navigation continue and load the *old* (disallowed) URL. To fix
this, we return early instead of rewriting dest_url to
about:blank.
4. The SiteInstance plumbed through to ShouldAllowOpenURL when going
via RenderFrameProxyHost::OnOpenURL is wrong: it takes the current
SiteInstance of the frame that's being navigated, rather than the
SiteInstance in which the navigation on the proxy was started.
RenderFrameProxyHost::OnOpenURL already passes the right
SiteInstance as source_site_instance to RequestTransferURL, so it
just needs to use it when it's available.
Two new tests are added:
NestedURLNavigationsViaProxyBlocked for case 1.
WindowOpenInaccessibleResourceFromDataURL for case 2.
The fixes to ShouldAllowOpenURL also apply to subframe navigations as
well as main frame navigations (since it doesn't distinguish between
the two). That is, subframe navigations that go through proxies,
transfers, or OpenURL, will now also have the unsafe blob/filesystem
URL blocking enforced, whereas before this was only done for main
frames by other checks. The test exercising this for subframes,
NestedURLNavigationsViaProxyBlocked, was updated accordingly.
Why don't our other checks work?
--------------------------------
1. ExtensionNavigationThrottle also performs web-accessible resource
checks checks, but that currently happens only for subframes. Thus,
ShouldAllowOpenURL are the only checks that would work in this
scenario for main frames. Fundamentally, ExtensionNavigationThrottle
can't perform the WAR checks for main frames until we plumb through
the initiator SiteInstance (i.e., who navigated the proxy, and not the
StartingSiteInstance, which corresponds to the frame that's being
navigated) to NavigationHandle.
2. ChromeExtensionNetworkDelegate also blocks unsafe navigations to
blob/filesystem URLs. It doesn't apply in this case, since the process
where this navigation is happening is already an extension process.
3. url_request_util::AllowCrossRendererResourceLoad also allows this
navigation, as it explicitly excludes all main frames from WAR
enforcement:
// Navigating the main frame to an extension URL is allowed, even if not
// explicitly listed as web_accessible_resource.
if (info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) {
*allowed = true;
return true;
}
BUG=656752, 645028
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
to
==========
Fix web accessible resource checks in ShouldAllowOpenURL
(This is joint work with nick@)
When a web page navigates an extension main frame to another extension
URL via a proxy, RenderFrameProxyHost::OnOpenURL calls
NavigatorImpl::RequestTransferURL, which checks whether the
destination URL points to a web-accessible resource in
ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL.
This CL fixes several problems with that check:
1. It doesn't work properly for blob/filesystem URL, since it
references schemes on the destination GURL directly. This leads to
a security problem where all blob/filesystem URLs are allowed. To
fix this, use url::Origin to handle nested URL cases properly,
similarly to other fixes in this area (e.g., issue 645028).
2. It ignores cases where the source SiteInstance's site URL isn't an
HTTP, HTTPS, or extension scheme. Thus, a new tab with a data or
about:blank URL bypasses WAR checks. This artificial restriction
apparently was added to make a test pass several years ago. This
CL removes it, and fixes one test that was wrong and which relied
on this logic. The test is
ExtensionBrowserTest.WindowOpenNoPrivileges; it tried to
window.open a non-web-accessible resource while incorrectly
assuming it was web-accessible. Since it did so from about:blank
with a blank SiteInstance, this bug allowed the test to work.
3. The things that ShouldAllowOpenURL does block aren't blocked
correctly for transfers. RequestTransferURL rewrites the blocked
URL to about:blank, and later, NavigatorImpl::NavigateToEntry
decides that this is a transfer to the same RFH
(is_transfer_to_same == true), so it happily lets the old
navigation continue and load the *old* (disallowed) URL. To fix
this, we return early instead of rewriting dest_url to
about:blank.
4. The SiteInstance plumbed through to ShouldAllowOpenURL when going
via RenderFrameProxyHost::OnOpenURL is wrong: it takes the current
SiteInstance of the frame that's being navigated, rather than the
SiteInstance in which the navigation on the proxy was started.
RenderFrameProxyHost::OnOpenURL already passes the right
SiteInstance as source_site_instance to RequestTransferURL, so it
just needs to use it when it's available.
Two new tests are added:
NestedURLNavigationsViaProxyBlocked for case 1.
WindowOpenInaccessibleResourceFromDataURL for case 2.
The fixes to ShouldAllowOpenURL also apply to subframe navigations as
well as main frame navigations (since it doesn't distinguish between
the two). That is, subframe navigations that go through proxies,
transfers, or OpenURL, will now also have the unsafe blob/filesystem
URL blocking enforced, whereas before this was only done for main
frames by other checks. The test exercising this for subframes,
NestedURLNavigationsViaProxyBlocked, was updated accordingly.
Why don't our other checks work?
--------------------------------
1. ExtensionNavigationThrottle also performs web-accessible resource
checks checks, but that currently happens only for subframes. Thus,
ShouldAllowOpenURL are the only checks that would work in this
scenario for main frames. Fundamentally, ExtensionNavigationThrottle
can't perform the WAR checks for main frames until we plumb through
the initiator SiteInstance (i.e., who navigated the proxy, and not the
StartingSiteInstance, which corresponds to the frame that's being
navigated) to NavigationHandle.
2. ChromeExtensionNetworkDelegate also blocks unsafe navigations to
blob/filesystem URLs. It doesn't apply in this case, since the process
where this navigation is happening is already an extension process.
3. url_request_util::AllowCrossRendererResourceLoad also allows this
navigation, as it explicitly excludes all main frames from WAR
enforcement:
// Navigating the main frame to an extension URL is allowed, even if not
// explicitly listed as web_accessible_resource.
if (info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) {
*allowed = true;
return true;
}
BUG=656752, 645028
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
alexmos@chromium.org changed reviewers: + creis@chromium.org, nick@chromium.org
Nick - please take a look. Charlie - please also take a look. (Since parts of this CL came from Nick, and we've been discussing this closely, it'd be good to get someone else other than the two of us to sanity check all this, as it's definitely tricky.) https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (left): https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:507: (from_url.SchemeIsHTTPOrHTTPS() || from_url.SchemeIs(kExtensionScheme))) { Lifting the (bogus) scheme restriction on from_url only affected ExtensionBrowserTest.WindowOpenNoPrivileges, which was just an invalid test (attempted to window.open a non-WAR page; the fix is to make it WAR). However, I also added a DWOC for cases that we wouldn't have blocked previously due to this below, so we can catch any regressions. https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:575: base::debug::DumpWithoutCrashing(); Should we also collect UMA stats for some of these cases, or is the DWOC sufficient? https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/extension_resource_request_policy_apitest.cc (right): https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensi... chrome/browser/extensions/extension_resource_request_policy_apitest.cc:359: WaitForLoadStop(web_contents); Previously, the subframe was blocked with an error page by the ExtensionNavigationThrottle checks. Now, we block this earlier in ShouldAllowOpenURL, by canceling the navigation, so we won't get an error page anymore (but rather stay on the previous page). I hope this is ok -- I've adjusted the test expectations accordingly. https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensi... chrome/browser/extensions/process_manager_browsertest.cc:805: // below can be removed. Alternatively, to avoid the confusion with PlzNavigate, we could just skip this for loop completely for PlzNav until we fix the ExtensionNavigationThrottle blob checks (that jam@ moved there from ChromeExtensionsNetworkDelegate) to work for subframes. https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensi... chrome/browser/extensions/process_manager_browsertest.cc:809: if (IsIsolateExtensionsEnabled() && The reason this starts working (with --isolate-extensions), is because ShouldAllowOpenURL enforces the block when navigating via a proxy for both main frames and subframes. Without --isolate-extensions, there's no proxy, so that logic doesn't get triggered. https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/window_open_apitest.cc (right): https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensi... chrome/browser/extensions/window_open_apitest.cc:294: WindowOpenInaccessibleResourceFromDataURL) { This is checking the case that would've previously been incorrectly allowed by ShouldAllowOpenURL due to the SchemeIsHTTPOrHTTPS() restriction. https://codereview.chromium.org/2454563003/diff/120001/chrome/test/data/exten... File chrome/test/data/extensions/uitest/window_open/manifest.json (right): https://codereview.chromium.org/2454563003/diff/120001/chrome/test/data/exten... chrome/test/data/extensions/uitest/window_open/manifest.json:7: "web_accessible_resources": ["newtab.html"] Necessary to fix ExtensionBrowserTest.WindowOpenNoPrivileges, which does a window.open on newtab.html and expects it to work. https://codereview.chromium.org/2454563003/diff/120001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2454563003/diff/120001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:818: source_site_instance ? source_site_instance : current_site_instance, We could get here two ways: from RFPH::OnOpenURL or from a transfer. Transfers always pass in a null source_site_instance, so that case will function the same as before. Proxy navigations always pass in a valid source_site_instance (the proxy's SiteInstance), and I think that one should be used in that case. (If A is navigating B1 to B2, we want to perform the WAR checks to see if A is allowed to load B2.) Can redirects happen for chrome-extension:// URLs? (I'm trying to think whether that could be abused to lose the source_site_instance if we navigate a proxy to a URL that redirects to another URL.)
Description was changed from
==========
Fix web accessible resource checks in ShouldAllowOpenURL
(This is joint work with nick@)
When a web page navigates an extension main frame to another extension
URL via a proxy, RenderFrameProxyHost::OnOpenURL calls
NavigatorImpl::RequestTransferURL, which checks whether the
destination URL points to a web-accessible resource in
ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL.
This CL fixes several problems with that check:
1. It doesn't work properly for blob/filesystem URL, since it
references schemes on the destination GURL directly. This leads to
a security problem where all blob/filesystem URLs are allowed. To
fix this, use url::Origin to handle nested URL cases properly,
similarly to other fixes in this area (e.g., issue 645028).
2. It ignores cases where the source SiteInstance's site URL isn't an
HTTP, HTTPS, or extension scheme. Thus, a new tab with a data or
about:blank URL bypasses WAR checks. This artificial restriction
apparently was added to make a test pass several years ago. This
CL removes it, and fixes one test that was wrong and which relied
on this logic. The test is
ExtensionBrowserTest.WindowOpenNoPrivileges; it tried to
window.open a non-web-accessible resource while incorrectly
assuming it was web-accessible. Since it did so from about:blank
with a blank SiteInstance, this bug allowed the test to work.
3. The things that ShouldAllowOpenURL does block aren't blocked
correctly for transfers. RequestTransferURL rewrites the blocked
URL to about:blank, and later, NavigatorImpl::NavigateToEntry
decides that this is a transfer to the same RFH
(is_transfer_to_same == true), so it happily lets the old
navigation continue and load the *old* (disallowed) URL. To fix
this, we return early instead of rewriting dest_url to
about:blank.
4. The SiteInstance plumbed through to ShouldAllowOpenURL when going
via RenderFrameProxyHost::OnOpenURL is wrong: it takes the current
SiteInstance of the frame that's being navigated, rather than the
SiteInstance in which the navigation on the proxy was started.
RenderFrameProxyHost::OnOpenURL already passes the right
SiteInstance as source_site_instance to RequestTransferURL, so it
just needs to use it when it's available.
Two new tests are added:
NestedURLNavigationsViaProxyBlocked for case 1.
WindowOpenInaccessibleResourceFromDataURL for case 2.
The fixes to ShouldAllowOpenURL also apply to subframe navigations as
well as main frame navigations (since it doesn't distinguish between
the two). That is, subframe navigations that go through proxies,
transfers, or OpenURL, will now also have the unsafe blob/filesystem
URL blocking enforced, whereas before this was only done for main
frames by other checks. The test exercising this for subframes,
NestedURLNavigationsViaProxyBlocked, was updated accordingly.
Why don't our other checks work?
--------------------------------
1. ExtensionNavigationThrottle also performs web-accessible resource
checks checks, but that currently happens only for subframes. Thus,
ShouldAllowOpenURL are the only checks that would work in this
scenario for main frames. Fundamentally, ExtensionNavigationThrottle
can't perform the WAR checks for main frames until we plumb through
the initiator SiteInstance (i.e., who navigated the proxy, and not the
StartingSiteInstance, which corresponds to the frame that's being
navigated) to NavigationHandle.
2. ChromeExtensionNetworkDelegate also blocks unsafe navigations to
blob/filesystem URLs. It doesn't apply in this case, since the process
where this navigation is happening is already an extension process.
3. url_request_util::AllowCrossRendererResourceLoad also allows this
navigation, as it explicitly excludes all main frames from WAR
enforcement:
// Navigating the main frame to an extension URL is allowed, even if not
// explicitly listed as web_accessible_resource.
if (info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) {
*allowed = true;
return true;
}
BUG=656752, 645028
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
to
==========
Fix web accessible resource checks in ShouldAllowOpenURL
(This is joint work with nick@)
When a web page navigates an extension main frame to another extension
URL via a proxy, RenderFrameProxyHost::OnOpenURL calls
NavigatorImpl::RequestTransferURL, which checks whether the
destination URL points to a web-accessible resource in
ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL.
This CL fixes several problems with that check:
1. It doesn't work properly for blob/filesystem URL, since it
references schemes on the destination GURL directly. This leads to
a security problem where all blob/filesystem URLs are allowed. To
fix this, use url::Origin to handle nested URL cases properly,
similarly to other fixes in this area (e.g., issue 645028).
2. It ignores cases where the source SiteInstance's site URL isn't an
HTTP, HTTPS, or extension scheme. Thus, a new tab with a data or
about:blank URL bypasses WAR checks. This artificial restriction
apparently was added to make a test pass several years ago. This
CL removes it, and fixes one test that was wrong and which relied
on this logic. The test is
ExtensionBrowserTest.WindowOpenNoPrivileges; it tried to
window.open a non-web-accessible resource while incorrectly
assuming it was web-accessible. Since it did so from about:blank
with a blank SiteInstance, this bug allowed the test to work.
3. The things that ShouldAllowOpenURL does block aren't blocked
correctly for transfers. RequestTransferURL rewrites the blocked
URL to about:blank, and later, NavigatorImpl::NavigateToEntry
decides that this is a transfer to the same RFH
(is_transfer_to_same == true), so it happily lets the old
navigation continue and load the *old* (disallowed) URL. To fix
this, we return early instead of rewriting dest_url to
about:blank.
4. The SiteInstance plumbed through to ShouldAllowOpenURL when going
via RenderFrameProxyHost::OnOpenURL is wrong: it takes the current
SiteInstance of the frame that's being navigated, rather than the
SiteInstance in which the navigation on the proxy was started.
RenderFrameProxyHost::OnOpenURL already passes the right
SiteInstance as source_site_instance to RequestTransferURL, so it
just needs to use it when it's available.
Two new tests are added:
NestedURLNavigationsViaProxyBlocked for case 1.
WindowOpenInaccessibleResourceFromDataURL for case 2.
The fixes to ShouldAllowOpenURL also apply to subframe navigations as
well as main frame navigations (since it doesn't distinguish between
the two). That is, subframe navigations that go through proxies,
transfers, or OpenURL, will now also have the unsafe blob/filesystem
URL blocking enforced, whereas before this was only done for main
frames by other checks. The test exercising this for subframes,
NestedURLNavigationsViaProxyBlocked, was updated accordingly.
Why don't our other checks work?
--------------------------------
1. ExtensionNavigationThrottle also performs web-accessible resource
checks checks, but that currently happens only for subframes. Thus,
ShouldAllowOpenURL are the only checks that would work in this
scenario for main frames. Fundamentally, ExtensionNavigationThrottle
can't perform the WAR checks for main frames until we plumb through
the initiator SiteInstance (i.e., who navigated the proxy, and not the
StartingSiteInstance, which corresponds to the frame that's being
navigated) to NavigationHandle.
2. ChromeExtensionNetworkDelegate also blocks unsafe navigations to
blob/filesystem URLs. It doesn't apply in this case, since the process
where this navigation is happening is already an extension process.
3. url_request_util::AllowCrossRendererResourceLoad also allows this
navigation, as it explicitly excludes all main frames from WAR
enforcement:
// Navigating the main frame to an extension URL is allowed, even if not
// explicitly listed as web_accessible_resource.
if (info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) {
*allowed = true;
return true;
}
BUG=656752, 645028, 652887
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:575: base::debug::DumpWithoutCrashing(); On 2016/10/28 00:29:41, alexmos wrote: > Should we also collect UMA stats for some of these cases, or is the DWOC > sufficient? Let's add UMA metrics too, since they will be mergeable, unlike these DWOCs. (An extension-id-valued Rappor trial would actually be ideal for this, but I don't think we have a drop-in way to create them.) This is really on the edge of what DWOC is for, but I'm okay with it. In general we shouldn't be using DWOC to collect metrics about webpage behavior, but the second DWOC here targets a rare and suspicious enough case that I think it is okay for canary, and the first is targeted to exactly the behavior we're locking down. https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/window_open_apitest.cc (right): https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensi... chrome/browser/extensions/window_open_apitest.cc:319: newtab->GetMainFrame()->GetSiteInstance()->GetSiteURL().scheme()); If you add UMA stats, you could use a histogram_tester here to validate that we blocked it for the reason you expect. https://codereview.chromium.org/2454563003/diff/120001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2454563003/diff/120001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:818: source_site_instance ? source_site_instance : current_site_instance, On 2016/10/28 00:29:42, alexmos wrote: > We could get here two ways: from RFPH::OnOpenURL or from a transfer. Transfers > always pass in a null source_site_instance, so that case will function the same > as before. Proxy navigations always pass in a valid source_site_instance (the > proxy's SiteInstance), and I think that one should be used in that case. (If A > is navigating B1 to B2, we want to perform the WAR checks to see if A is allowed > to load B2.) > > Can redirects happen for chrome-extension:// URLs? (I'm trying to think whether > that could be abused to lose the source_site_instance if we navigate a proxy to > a URL that redirects to another URL.) I looked at the extension protocol handler code, re: redirects. I don't think we can get a redirect response from a load of a chrome-extension:// resource, but it looks like we actually do allow an http server to redirect to a chrome-extension URL, and it seems like tests are actually doing that: https://cs.chromium.org/chromium/src/chrome/browser/prerender/prerender_brows... Very surprised by that. blobs and filesystems are not allowed as redirect targets, though, so while redirects might be another possible avenue around enforcement of web accessible resources, I don't think redirects could be used as a way around the defenses for bug 656752.
The CQ bit was checked by alexmos@chromium.org to run a CQ dry run
https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:575: base::debug::DumpWithoutCrashing(); On 2016/10/28 21:45:03, ncarter wrote: > On 2016/10/28 00:29:41, alexmos wrote: > > Should we also collect UMA stats for some of these cases, or is the DWOC > > sufficient? > > Let's add UMA metrics too, since they will be mergeable, unlike these DWOCs. > > (An extension-id-valued Rappor trial would actually be ideal for this, but I > don't think we have a drop-in way to create them.) > > This is really on the edge of what DWOC is for, but I'm okay with it. In general > we shouldn't be using DWOC to collect metrics about webpage behavior, but the > second DWOC here targets a rare and suspicious enough case that I think it is > okay for canary, and the first is targeted to exactly the behavior we're locking > down. Metrics added, PTAL. https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/window_open_apitest.cc (right): https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensi... chrome/browser/extensions/window_open_apitest.cc:319: newtab->GetMainFrame()->GetSiteInstance()->GetSiteURL().scheme()); On 2016/10/28 21:45:03, ncarter wrote: > If you add UMA stats, you could use a histogram_tester here to validate that we > blocked it for the reason you expect. Done. Never used histogram_tester before, so wasn't sure whether it was worth exposing the ShouldAllowOpenURLFailureReason enum values to tests or just using hardcoded values. I did the latter for now, as the enum covers such obscure cases that I'm not sure it's worth exposing more widely, but also happy to do the former (is there a good place for the enum?). https://codereview.chromium.org/2454563003/diff/120001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2454563003/diff/120001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:818: source_site_instance ? source_site_instance : current_site_instance, On 2016/10/28 21:45:03, ncarter wrote: > On 2016/10/28 00:29:42, alexmos wrote: > > We could get here two ways: from RFPH::OnOpenURL or from a transfer. > Transfers > > always pass in a null source_site_instance, so that case will function the > same > > as before. Proxy navigations always pass in a valid source_site_instance (the > > proxy's SiteInstance), and I think that one should be used in that case. (If A > > is navigating B1 to B2, we want to perform the WAR checks to see if A is > allowed > > to load B2.) > > > > Can redirects happen for chrome-extension:// URLs? (I'm trying to think > whether > > that could be abused to lose the source_site_instance if we navigate a proxy > to > > a URL that redirects to another URL.) > > I looked at the extension protocol handler code, re: redirects. > > I don't think we can get a redirect response from a load of a > chrome-extension:// resource, but it looks like we actually do allow an http > server to redirect to a chrome-extension URL, and it seems like tests are > actually doing that: > > https://cs.chromium.org/chromium/src/chrome/browser/prerender/prerender_brows... > > Very surprised by that. > > blobs and filesystems are not allowed as redirect targets, though, so while > redirects might be another possible avenue around enforcement of web accessible > resources, I don't think redirects could be used as a way around the defenses > for bug 656752. Yes, that's very surprising. Sounds like it's worth looking into whether that can be used to bypass the regular WAR checks on transfers in a follow-up.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
lgtm https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/window_open_apitest.cc (right): https://codereview.chromium.org/2454563003/diff/120001/chrome/browser/extensi... chrome/browser/extensions/window_open_apitest.cc:319: newtab->GetMainFrame()->GetSiteInstance()->GetSiteURL().scheme()); On 2016/10/31 23:34:48, alexmos wrote: > On 2016/10/28 21:45:03, ncarter wrote: > > If you add UMA stats, you could use a histogram_tester here to validate that > we > > blocked it for the reason you expect. > > Done. Never used histogram_tester before, so wasn't sure whether it was worth > exposing the ShouldAllowOpenURLFailureReason enum values to tests or just using > hardcoded values. I did the latter for now, as the enum covers such obscure > cases that I'm not sure it's worth exposing more widely, but also happy to do > the former (is there a good place for the enum?). Hardcoded values are the way to go. It's arguably a layering violation, but I actually kind of like this kind of deliberate brittleness for "tests in chrome/ that are really tests of logic in content/".
The CQ bit was checked by alexmos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
alexmos@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Devlin, can you please review chrome/browser/extensions for OWNERS?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
Wow. That's an impressive list of issues to fix. I like where it's headed, and I hope that we'll have more meaningful enforcement of web accessible resources after this. That said, we want to be careful about not breaking extensions with this. I'm not sure that I've found concrete problems, but here are some things we'll want to make sure we aren't regressing: 1) Are browser-initiated navigations to non-web-accessible resources allowed today? Would this break them in any cases (e.g., a redirect that goes through RequestTransferURL)? 2) Are renderer-initiated navigations to non-web-accessible resources allowed in the main frame today? For example, can you have a top-level link to a non-accessible options.html page for an extension? (I'm not sure what the intended behavior for WAR is here. I'm hoping not.) If so, would this CL regress that? https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:520: url::Origin to_origin(to_url); Maybe mention why this is important (for blob/filesystem URLs) in a comment, to give other developers a reminder? https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:521: if (to_origin.scheme() != kExtensionScheme) { I'm curious-- is |to_url| an effective URL or not? I'm curious if hosted apps were handled by this method before. (The origin's scheme will only be chrome-extension:// for hosted apps if we're given the effective URL.) I'm guessing this method should always return true for hosted apps (where everything is web accessible), but I want to be sure which part of the method is handling that. If |to_url| is not an effective URL, we can mention the hosted app aspect here. If it is an effective URL, we'll want to mention it on the GetByID call below. https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:537: const Extension* extension = nit: to_extension? https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:538: registry->enabled_extensions().GetByID(to_origin.host()); Interesting. As mentioned above, calling GetByID before GetExtensionOrAppByURL will likely be different for hosted apps, which won't return their extension object for GetByID. (That's only if |to_url| is an effective URL.) If so, let's mention the hosted app aspect here. https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... chrome/browser/extensions/process_manager_browsertest.cc:809: // below can be removed. Maybe file a PlzNavigate bug for this? I don't want them to forget about this since it's not on the list of disabled tests for them. https://codereview.chromium.org/2454563003/diff/160001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2454563003/diff/160001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:740: current_site_instance, url)) { Sanity check: Are we ok with sending the current_site_instance here? Maybe we should add a comment about why it differs from RequestTransferURL if so. https://codereview.chromium.org/2454563003/diff/160001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:741: dest_url = GURL(url::kAboutBlankURL); Should we be returning here, as below?
The CQ bit was checked by alexmos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for reviewing, Charlie! For the two questions about regressions, Nick and I tried a few cases with and without this CL and didn't find a difference that would cause extra breakage. Let me know if you think of other cases I should try. Details below. > I'm not sure that I've found concrete problems, but here are some things we'll > want to make sure we aren't regressing: > > 1) Are browser-initiated navigations to non-web-accessible resources allowed > today? Would this break them in any cases (e.g., a redirect that goes through > RequestTransferURL)? Yes, these are allowed both before and after this CL, and disallowed in redirects both before and after this CL. Test cases: 1) From web or extension page, go to non-web-accessible extension page via omnibox: allowed in both cases. 2) From web or extension page, go to a web URL that redirects to a non-WAR for same extension via omnibox: disallowed in both cases (ends up on about:blank in M54; cancels the navigation with this CL -- because of the change to return early in RequestTransferURL). Per Nick's earlier investigation, an extension URL can't redirect to another extension URL. 3) Go back/forward to non-WAR - allowed in both cases. > 2) Are renderer-initiated navigations to non-web-accessible resources allowed in > the main frame today? For example, can you have a top-level link to a > non-accessible options.html page for an extension? (I'm not sure what the > intended behavior for WAR is here. I'm hoping not.) If so, would this CL > regress that? In summary, allowed when starting from the same extension, disallowed when starting from a web page, disallowed in transfers. Test cases: 1) Start on a web page, click a link to non-WAR for extension: denied in both cases (navigates to about:blank instead). 2) Start on a web page, click a link to web URL that redirects to non-WAR for extension: denied in both cases, and navigation is canceled, with an error in console. I think the renderer-side WAR checks do this (ResourceRequestPolicy::CanRequestResource, called from ChromeExtensionsRendererClient::WillSendRequest) 3) Start on an extensions page, click a link to non-WAR for same extension: allowed in both cases. 4) Start on an extensions page, click a link to web URL that redirects to non-WAR for same extension: denied in both cases (navigates to about:blank on M54, canceled navigation with this CL, no error message). In general, any blocking for non-web-accessible pages in main frames would already be applied before this CL whenever we start from a HTTP/HTTPS site URL, and we just keep that behavior. The only case (hopefully) that should be different is: 5) Open a new tab, navigate to "data:text/html,foo" (or about:blank), then do a renderer-initiated navigation to a non-web-accessible resource for extension. That works today, but stops working after this CL. https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:520: url::Origin to_origin(to_url); On 2016/11/01 18:12:19, Charlie Reis wrote: > Maybe mention why this is important (for blob/filesystem URLs) in a comment, to > give other developers a reminder? Done. https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:521: if (to_origin.scheme() != kExtensionScheme) { On 2016/11/01 18:12:19, Charlie Reis wrote: > I'm curious-- is |to_url| an effective URL or not? I'm curious if hosted apps > were handled by this method before. (The origin's scheme will only be > chrome-extension:// for hosted apps if we're given the effective URL.) > > I'm guessing this method should always return true for hosted apps (where > everything is web accessible), but I want to be sure which part of the method is > handling that. If |to_url| is not an effective URL, we can mention the hosted > app aspect here. If it is an effective URL, we'll want to mention it on the > GetByID call below. I think |to_url| is not an effective URL. For hosted apps, this is returning false both before and after this CL (i.e., request not handled here, which ends up returning true in ChromeContentBrowserClient::ShouldAllowOpenURL). Before, that happened on the very bottom of the method, because to_url.SchemeIs(kExtensionScheme) was false. Now, it happens here. I added a comment mentioning that hosted apps would fall into the case. (I did a quick sanity-check on this using the Drive app, and this is indeed what seems to happen.) https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:537: const Extension* extension = On 2016/11/01 18:12:19, Charlie Reis wrote: > nit: to_extension? Done. https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:538: registry->enabled_extensions().GetByID(to_origin.host()); On 2016/11/01 18:12:19, Charlie Reis wrote: > Interesting. As mentioned above, calling GetByID before GetExtensionOrAppByURL > will likely be different for hosted apps, which won't return their extension > object for GetByID. (That's only if |to_url| is an effective URL.) > > If so, let's mention the hosted app aspect here. We should never get here for hosted apps (we'd return false early above), and I think the behavior of GetExtensionOrAppByURL and GetByID is the same for all other cases. I added a mention about hosted apps to the comment above the "return false". https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... chrome/browser/extensions/process_manager_browsertest.cc:809: // below can be removed. On 2016/11/01 18:12:19, Charlie Reis wrote: > Maybe file a PlzNavigate bug for this? I don't want them to forget about this > since it's not on the list of disabled tests for them. Done - filed https://crbug.com/661324 and mentioned here. https://codereview.chromium.org/2454563003/diff/160001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2454563003/diff/160001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:740: current_site_instance, url)) { On 2016/11/01 18:12:19, Charlie Reis wrote: > Sanity check: Are we ok with sending the current_site_instance here? Maybe we > should add a comment about why it differs from RequestTransferURL if so. I added a comment - I think |current_site_instance| is still ok here. We'd get here if one of the frames in the current frame's SiteInstance navigates the current frame to a new URL, right? It seems reasonable to do the WAR checks against the RFH's current SiteInstance, as that's where the navigation was initiated. It's confusing to have both |source_site_instance| and |current_site_instance| though. This is called by only two places: RFHI::OnOpenURL, which always computes |source_site_instance| as the current RFH's SiteInstance (i.e., equal to |current_site_instance|), and in a test, which passes nullptr for |source_site_instance|. So really there's no difference between the two SiteInstances, and seems like we could get rid of the |source_site_instance| that's passed here altogether and just use |current_site_instance| here and for OpenURLParams.source_site_instance. I'm happy to do that here while I'm at it, or in a followup CL. https://codereview.chromium.org/2454563003/diff/160001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:741: dest_url = GURL(url::kAboutBlankURL); On 2016/11/01 18:12:19, Charlie Reis wrote: > Should we be returning here, as below? Good question. I think this is ok as-is, since a request hasn't been made yet when we're here, and when it's made below, it will always use |dest_url|. So there's no danger that we'll hit the |is_transfer_to_same| path and ignore the rewritten |dest_url| like we do for the other case. That said, it might be better to also return here for consistency. That may need some more test updates though (I haven't checked), so I'd vote for trying this in a follow-up CL.
On 2016/11/01 22:04:22, alexmos wrote: > Thanks for reviewing, Charlie! > > For the two questions about regressions, Nick and I tried a few cases with and > without this CL and didn't find a difference that would cause extra breakage. > Let me know if you think of other cases I should try. Details below. > > > I'm not sure that I've found concrete problems, but here are some things we'll > > want to make sure we aren't regressing: > > > > 1) Are browser-initiated navigations to non-web-accessible resources allowed > > today? Would this break them in any cases (e.g., a redirect that goes through > > RequestTransferURL)? > > Yes, these are allowed both before and after this CL, and disallowed in > redirects both before and after this CL. Test cases: > > 1) From web or extension page, go to non-web-accessible extension page via > omnibox: allowed in both cases. > 2) From web or extension page, go to a web URL that redirects to a non-WAR for > same extension via omnibox: disallowed in both cases (ends up on about:blank in > M54; cancels the navigation with this CL -- because of the change to return > early in RequestTransferURL). Per Nick's earlier investigation, an extension > URL can't redirect to another extension URL. > 3) Go back/forward to non-WAR - allowed in both cases. > > > > 2) Are renderer-initiated navigations to non-web-accessible resources allowed > in > > the main frame today? For example, can you have a top-level link to a > > non-accessible options.html page for an extension? (I'm not sure what the > > intended behavior for WAR is here. I'm hoping not.) If so, would this CL > > regress that? > > In summary, allowed when starting from the same extension, disallowed when > starting from a web page, disallowed in transfers. Test cases: > > 1) Start on a web page, click a link to non-WAR for extension: denied in both > cases (navigates to about:blank instead). > 2) Start on a web page, click a link to web URL that redirects to non-WAR for > extension: denied in both cases, and navigation is canceled, with an error in > console. I think the renderer-side WAR checks do this > (ResourceRequestPolicy::CanRequestResource, called from > ChromeExtensionsRendererClient::WillSendRequest) > 3) Start on an extensions page, click a link to non-WAR for same extension: > allowed in both cases. > 4) Start on an extensions page, click a link to web URL that redirects to > non-WAR for same extension: denied in both cases (navigates to about:blank on > M54, canceled navigation with this CL, no error message). > > In general, any blocking for non-web-accessible pages in main frames would > already be applied before this CL whenever we start from a HTTP/HTTPS site URL, > and we just keep that behavior. The only case (hopefully) that should be > different is: > 5) Open a new tab, navigate to "data:text/html,foo" (or about:blank), then do a > renderer-initiated navigation to a non-web-accessible resource for extension. > That works today, but stops working after this CL. Thanks for working through all these! It sounds like this is a strict improvement then. LGTM! https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:521: if (to_origin.scheme() != kExtensionScheme) { On 2016/11/01 22:04:22, alexmos wrote: > On 2016/11/01 18:12:19, Charlie Reis wrote: > > I'm curious-- is |to_url| an effective URL or not? I'm curious if hosted apps > > were handled by this method before. (The origin's scheme will only be > > chrome-extension:// for hosted apps if we're given the effective URL.) > > > > I'm guessing this method should always return true for hosted apps (where > > everything is web accessible), but I want to be sure which part of the method > is > > handling that. If |to_url| is not an effective URL, we can mention the hosted > > app aspect here. If it is an effective URL, we'll want to mention it on the > > GetByID call below. > > I think |to_url| is not an effective URL. For hosted apps, this is returning > false both before and after this CL (i.e., request not handled here, which ends > up returning true in ChromeContentBrowserClient::ShouldAllowOpenURL). Before, > that happened on the very bottom of the method, because > to_url.SchemeIs(kExtensionScheme) was false. Now, it happens here. I added a > comment mentioning that hosted apps would fall into the case. (I did a quick > sanity-check on this using the Drive app, and this is indeed what seems to > happen.) Acknowledged. https://codereview.chromium.org/2454563003/diff/160001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2454563003/diff/160001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:740: current_site_instance, url)) { On 2016/11/01 22:04:22, alexmos wrote: > On 2016/11/01 18:12:19, Charlie Reis wrote: > > Sanity check: Are we ok with sending the current_site_instance here? Maybe we > > should add a comment about why it differs from RequestTransferURL if so. > > I added a comment - I think |current_site_instance| is still ok here. We'd get > here if one of the frames in the current frame's SiteInstance navigates the > current frame to a new URL, right? It seems reasonable to do the WAR checks > against the RFH's current SiteInstance, as that's where the navigation was > initiated. Interesting-- yes. While it could have been a different frame initiating the navigation, it necessarily was in the same SiteInstance if it came through RequestOpenURL rather than RequestTransferURL. > > It's confusing to have both |source_site_instance| and |current_site_instance| > though. This is called by only two places: RFHI::OnOpenURL, which always > computes |source_site_instance| as the current RFH's SiteInstance (i.e., equal > to |current_site_instance|), and in a test, which passes nullptr for > |source_site_instance|. So really there's no difference between the two > SiteInstances, and seems like we could get rid of the |source_site_instance| > that's passed here altogether and just use |current_site_instance| here and for > OpenURLParams.source_site_instance. I'm happy to do that here while I'm at it, > or in a followup CL. Are we not considering changing RenderFrameProxyHost back to using RequestOpenURL anymore, now that we found an alternate fix for https://crbug.com/647772? (That may be one reason source_site_instance was useful there.) Anyway, I'm fine with removing it (either here or in a followup) if nothing needs it at the moment. https://codereview.chromium.org/2454563003/diff/160001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:741: dest_url = GURL(url::kAboutBlankURL); On 2016/11/01 22:04:22, alexmos wrote: > On 2016/11/01 18:12:19, Charlie Reis wrote: > > Should we be returning here, as below? > > Good question. I think this is ok as-is, since a request hasn't been made yet > when we're here, and when it's made below, it will always use |dest_url|. So > there's no danger that we'll hit the |is_transfer_to_same| path and ignore the > rewritten |dest_url| like we do for the other case. That said, it might be > better to also return here for consistency. That may need some more test > updates though (I haven't checked), so I'd vote for trying this in a follow-up > CL. Sure, cleaning it up separately sounds fine.
Thanks! https://codereview.chromium.org/2454563003/diff/160001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2454563003/diff/160001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:740: current_site_instance, url)) { On 2016/11/01 23:16:27, Charlie Reis wrote: > On 2016/11/01 22:04:22, alexmos wrote: > > On 2016/11/01 18:12:19, Charlie Reis wrote: > > > Sanity check: Are we ok with sending the current_site_instance here? Maybe > we > > > should add a comment about why it differs from RequestTransferURL if so. > > > > I added a comment - I think |current_site_instance| is still ok here. We'd > get > > here if one of the frames in the current frame's SiteInstance navigates the > > current frame to a new URL, right? It seems reasonable to do the WAR checks > > against the RFH's current SiteInstance, as that's where the navigation was > > initiated. > > Interesting-- yes. While it could have been a different frame initiating the > navigation, it necessarily was in the same SiteInstance if it came through > RequestOpenURL rather than RequestTransferURL. > > > > It's confusing to have both |source_site_instance| and |current_site_instance| > > though. This is called by only two places: RFHI::OnOpenURL, which always > > computes |source_site_instance| as the current RFH's SiteInstance (i.e., equal > > to |current_site_instance|), and in a test, which passes nullptr for > > |source_site_instance|. So really there's no difference between the two > > SiteInstances, and seems like we could get rid of the |source_site_instance| > > that's passed here altogether and just use |current_site_instance| here and > for > > OpenURLParams.source_site_instance. I'm happy to do that here while I'm at > it, > > or in a followup CL. > > Are we not considering changing RenderFrameProxyHost back to using > RequestOpenURL anymore, now that we found an alternate fix for > https://crbug.com/647772 (That may be one reason source_site_instance was > useful there.) > > Anyway, I'm fine with removing it (either here or in a followup) if nothing > needs it at the moment. Yes, after also checking with lukasza@, I don't think we have any reasons to convert RFPH to use RequestOpenURL at the moment. So I think it should be fine to remove source_site_instance; I'll put together a small followup for this.
alexmos@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine@: can you please review histograms.xml?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:531: ExtensionRegistry* registry = ExtensionRegistry::Get(profile); Not your code, but ExtensionRegistry doesn't need a profile; you can just pass BrowserContext in directly here. Also, let's use site_instance->GetBrowserContext() rather than going through the RPH. https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:532: if (!registry) { |registry| should never ever be null (even in tests). Let's make this a CHECK (or DCHECK), especially since apparently the default behavior is to say "Sure, this is fiiiiine." :) https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/extension_resource_request_policy_apitest.cc (right): https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_resource_request_policy_apitest.cc:365: EXPECT_NE("Private", content); It looks like the previous version of this checked that we ended up on an error page. Should we preserve that check here? https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/window_open_apitest.cc (right): https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... chrome/browser/extensions/window_open_apitest.cc:303: GURL extension_url(std::string(extensions::kExtensionScheme) + nit: extension_url = Extension::GetResourceURL( GetBaseURLFromExtensionId(loast_loaded_extension_id()), "test.html"); https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... chrome/browser/extensions/window_open_apitest.cc:321: newtab->GetMainFrame()->GetSiteInstance()->GetSiteURL().scheme()); Can we check what the page is supposed to be? (i.e., an error page?)
https://codereview.chromium.org/2454563003/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2454563003/diff/180001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:563: FAILURE_BLOB_URL, FAILURE_LAST); Please make a helper function to log this histogram that takes the enum as the param. This has two benefits: - Each macro expands to a lot of code, so it will reduce binary bloat. - You don't repeat yourself - e.g. no chance for typos in the metric name.
The CQ bit was checked by alexmos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, Devlin and Alexei. Comments addressed below. https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:531: ExtensionRegistry* registry = ExtensionRegistry::Get(profile); On 2016/11/02 16:19:22, Devlin wrote: > Not your code, but ExtensionRegistry doesn't need a profile; you can just pass > BrowserContext in directly here. Also, let's use > site_instance->GetBrowserContext() rather than going through the RPH. Done. https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:532: if (!registry) { On 2016/11/02 16:19:22, Devlin wrote: > |registry| should never ever be null (even in tests). Let's make this a CHECK > (or DCHECK), especially since apparently the default behavior is to say "Sure, > this is fiiiiine." :) Thanks, good to learn about these. :) I just removed the null check completely. Didn't add a CHECK since we'd crash accessing it on the next line anyway. https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/extension_resource_request_policy_apitest.cc (right): https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_resource_request_policy_apitest.cc:365: EXPECT_NE("Private", content); On 2016/11/02 16:19:22, Devlin wrote: > It looks like the previous version of this checked that we ended up on an error > page. Should we preserve that check here? I removed the error page expectation because there's no error page anymore in this case. We used to block this later in ExtensionNavigationThrottle, which did result in an error page, but now the blocking happens earlier in ShouldAllowOpenURL (since this is navigating a remote subframe to a non-web-accessible extension page), which just cancels the navigation without going to an error page. I did mention this to Nasko briefly (he wrote the test), and he was ok with this. https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/window_open_apitest.cc (right): https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... chrome/browser/extensions/window_open_apitest.cc:303: GURL extension_url(std::string(extensions::kExtensionScheme) + On 2016/11/02 16:19:22, Devlin wrote: > nit: extension_url = Extension::GetResourceURL( > GetBaseURLFromExtensionId(loast_loaded_extension_id()), "test.html"); Done. https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... chrome/browser/extensions/window_open_apitest.cc:321: newtab->GetMainFrame()->GetSiteInstance()->GetSiteURL().scheme()); On 2016/11/02 16:19:22, Devlin wrote: > Can we check what the page is supposed to be? (i.e., an error page?) See my previous comment - ShouldAllowOpenURL doesn't result in an error page (before or after my fixes), so the best we can do is check that we didn't somehow load an extension page from the data URL here. https://codereview.chromium.org/2454563003/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2454563003/diff/180001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:563: FAILURE_BLOB_URL, FAILURE_LAST); On 2016/11/02 16:35:47, Alexei Svitkine (slow) wrote: > Please make a helper function to log this histogram that takes the enum as the > param. > > This has two benefits: > - Each macro expands to a lot of code, so it will reduce binary bloat. > - You don't repeat yourself - e.g. no chance for typos in the metric name. Done.
histograms lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/extension_resource_request_policy_apitest.cc (right): https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_resource_request_policy_apitest.cc:365: EXPECT_NE("Private", content); On 2016/11/02 17:32:46, alexmos wrote: > On 2016/11/02 16:19:22, Devlin wrote: > > It looks like the previous version of this checked that we ended up on an > error > > page. Should we preserve that check here? > > I removed the error page expectation because there's no error page anymore in > this case. We used to block this later in ExtensionNavigationThrottle, which > did result in an error page, but now the blocking happens earlier in > ShouldAllowOpenURL (since this is navigating a remote subframe to a > non-web-accessible extension page), which just cancels the navigation without > going to an error page. I did mention this to Nasko briefly (he wrote the > test), and he was ok with this. Two thoughts: - Is there any way we can test that we are on the right page? e.g. if we're supposed to end on about:blank, we could verify an empty body, etc. I'm a little worried that asserting the content isn't equivalent to the exact content is a little fragile. - We seem to be treating attempted access to non-war pages very differently frequently. IIRC, we have at least two different errors we show, and also sometimes about:blank pages (or canceled navigations?). All of these achieve the main goal - don't load the page - but I'm worried that it leads to a confusing story for us, extension devs, users, etc, and might also make things like fingerprinting easier. Nasko and I were chatting about actually documenting what should happen in various cases, and I think this might end up being an important part. I'm not sure if there's anything much to do in this CL; I just worry we're making any consistent story harder to achieve (but we're already far enough away that this probably won't make a difference). https://codereview.chromium.org/2454563003/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2454563003/diff/200001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:549: if (from_extension && from_extension->id() == to_extension->id()) { again, not your code, but if two extensions have the same id, they should point to the same object (i.e., from_extension == to_extension should be true). Can we add a CHECK_EQ(from_extension, to_extension) in the block, or change the if? https://codereview.chromium.org/2454563003/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/window_open_apitest.cc (right): https://codereview.chromium.org/2454563003/diff/200001/chrome/browser/extensi... chrome/browser/extensions/window_open_apitest.cc:322: EXPECT_NE(std::string(extensions::kExtensionScheme), nit: EXPECT_FALSE(GetSiteUrl().SchemeIs(kExtensionSchem))?
The CQ bit was checked by alexmos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/extension_resource_request_policy_apitest.cc (right): https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_resource_request_policy_apitest.cc:365: EXPECT_NE("Private", content); On 2016/11/02 20:50:57, Devlin wrote: > On 2016/11/02 17:32:46, alexmos wrote: > > On 2016/11/02 16:19:22, Devlin wrote: > > > It looks like the previous version of this checked that we ended up on an > > error > > > page. Should we preserve that check here? > > > > I removed the error page expectation because there's no error page anymore in > > this case. We used to block this later in ExtensionNavigationThrottle, which > > did result in an error page, but now the blocking happens earlier in > > ShouldAllowOpenURL (since this is navigating a remote subframe to a > > non-web-accessible extension page), which just cancels the navigation without > > going to an error page. I did mention this to Nasko briefly (he wrote the > > test), and he was ok with this. > > Two thoughts: > - Is there any way we can test that we are on the right page? e.g. if we're > supposed to end on about:blank, we could verify an empty body, etc. I'm a > little worried that asserting the content isn't equivalent to the exact content > is a little fragile. Unfortunately, this is tricky. The result depends on whether or not --isolate-extensions is on. If it's on, we'll stay on the public.html page (hitting the new blocking logic that cancels the navigation), and if it's off, we'll still land at the error page, since the block would still happen in ExtensionNavigationThrottle. This check is an easy way to handle both while we're in the transient state where we might flip --isolate-extensions on and off as we try to ship it. I added a comment explaining this and a TODO to strengthen the check. I could handle two error modes differently in a stricter check, but didn't go there given that --isolate-extensions will soon be the default again. (But let me know if you'd prefer that.) > - We seem to be treating attempted access to non-war pages very differently > frequently. IIRC, we have at least two different errors we show, and also > sometimes about:blank pages (or canceled navigations?). All of these achieve > the main goal - don't load the page - but I'm worried that it leads to a > confusing story for us, extension devs, users, etc, and might also make things > like fingerprinting easier. Nasko and I were chatting about actually > documenting what should happen in various cases, and I think this might end up > being an important part. I'm not sure if there's anything much to do in this > CL; I just worry we're making any consistent story harder to achieve (but we're > already far enough away that this probably won't make a difference). I totally agree with this, and I think we should try to consolidate the various kinds of WAR checks and failure modes. It's really confusing, for example, that the renderer-side checks aren't always consistent with the browser-side checks (e.g., the WAR check in ResourceRequestPolicy::CanRequestResource happily lets the navigation in this test through (due to the |is_own_resource| case, which doesn't realize that another frame is navigating the target frame), whereas it should really block it and output a helpful console error). I was going to try and clean up the about:blank cases to also lead to canceled navigations (I'm not yet sure if that's going to break something). But I'm leaving all that for future followups. https://codereview.chromium.org/2454563003/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2454563003/diff/200001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:549: if (from_extension && from_extension->id() == to_extension->id()) { On 2016/11/02 20:50:57, Devlin wrote: > again, not your code, but if two extensions have the same id, they should point > to the same object (i.e., from_extension == to_extension should be true). > > Can we add a CHECK_EQ(from_extension, to_extension) in the block, or change the > if? Done. Changed the if. https://codereview.chromium.org/2454563003/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/window_open_apitest.cc (right): https://codereview.chromium.org/2454563003/diff/200001/chrome/browser/extensi... chrome/browser/extensions/window_open_apitest.cc:322: EXPECT_NE(std::string(extensions::kExtensionScheme), On 2016/11/02 20:50:57, Devlin wrote: > nit: EXPECT_FALSE(GetSiteUrl().SchemeIs(kExtensionSchem))? Done.
https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/extension_resource_request_policy_apitest.cc (right): https://codereview.chromium.org/2454563003/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_resource_request_policy_apitest.cc:365: EXPECT_NE("Private", content); On 2016/11/03 00:43:26, alexmos wrote: > On 2016/11/02 20:50:57, Devlin wrote: > > On 2016/11/02 17:32:46, alexmos wrote: > > > On 2016/11/02 16:19:22, Devlin wrote: > > > > It looks like the previous version of this checked that we ended up on an > > > error > > > > page. Should we preserve that check here? > > > > > > I removed the error page expectation because there's no error page anymore > in > > > this case. We used to block this later in ExtensionNavigationThrottle, > which > > > did result in an error page, but now the blocking happens earlier in > > > ShouldAllowOpenURL (since this is navigating a remote subframe to a > > > non-web-accessible extension page), which just cancels the navigation > without > > > going to an error page. I did mention this to Nasko briefly (he wrote the > > > test), and he was ok with this. > > > > Two thoughts: > > - Is there any way we can test that we are on the right page? e.g. if we're > > supposed to end on about:blank, we could verify an empty body, etc. I'm a > > little worried that asserting the content isn't equivalent to the exact > content > > is a little fragile. > > Unfortunately, this is tricky. The result depends on whether or not > --isolate-extensions is on. If it's on, we'll stay on the public.html page > (hitting the new blocking logic that cancels the navigation), and if it's off, > we'll still land at the error page, since the block would still happen in > ExtensionNavigationThrottle. This check is an easy way to handle both while > we're in the transient state where we might flip --isolate-extensions on and off > as we try to ship it. I added a comment explaining this and a TODO to > strengthen the check. I could handle two error modes differently in a stricter > check, but didn't go there given that --isolate-extensions will soon be the > default again. (But let me know if you'd prefer that.) Okay, this is fine for now, thanks for the TODO.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by alexmos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org, creis@chromium.org, asvitkine@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2454563003/#ps220001 (title: "Round 2 of Devlin's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from
==========
Fix web accessible resource checks in ShouldAllowOpenURL
(This is joint work with nick@)
When a web page navigates an extension main frame to another extension
URL via a proxy, RenderFrameProxyHost::OnOpenURL calls
NavigatorImpl::RequestTransferURL, which checks whether the
destination URL points to a web-accessible resource in
ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL.
This CL fixes several problems with that check:
1. It doesn't work properly for blob/filesystem URL, since it
references schemes on the destination GURL directly. This leads to
a security problem where all blob/filesystem URLs are allowed. To
fix this, use url::Origin to handle nested URL cases properly,
similarly to other fixes in this area (e.g., issue 645028).
2. It ignores cases where the source SiteInstance's site URL isn't an
HTTP, HTTPS, or extension scheme. Thus, a new tab with a data or
about:blank URL bypasses WAR checks. This artificial restriction
apparently was added to make a test pass several years ago. This
CL removes it, and fixes one test that was wrong and which relied
on this logic. The test is
ExtensionBrowserTest.WindowOpenNoPrivileges; it tried to
window.open a non-web-accessible resource while incorrectly
assuming it was web-accessible. Since it did so from about:blank
with a blank SiteInstance, this bug allowed the test to work.
3. The things that ShouldAllowOpenURL does block aren't blocked
correctly for transfers. RequestTransferURL rewrites the blocked
URL to about:blank, and later, NavigatorImpl::NavigateToEntry
decides that this is a transfer to the same RFH
(is_transfer_to_same == true), so it happily lets the old
navigation continue and load the *old* (disallowed) URL. To fix
this, we return early instead of rewriting dest_url to
about:blank.
4. The SiteInstance plumbed through to ShouldAllowOpenURL when going
via RenderFrameProxyHost::OnOpenURL is wrong: it takes the current
SiteInstance of the frame that's being navigated, rather than the
SiteInstance in which the navigation on the proxy was started.
RenderFrameProxyHost::OnOpenURL already passes the right
SiteInstance as source_site_instance to RequestTransferURL, so it
just needs to use it when it's available.
Two new tests are added:
NestedURLNavigationsViaProxyBlocked for case 1.
WindowOpenInaccessibleResourceFromDataURL for case 2.
The fixes to ShouldAllowOpenURL also apply to subframe navigations as
well as main frame navigations (since it doesn't distinguish between
the two). That is, subframe navigations that go through proxies,
transfers, or OpenURL, will now also have the unsafe blob/filesystem
URL blocking enforced, whereas before this was only done for main
frames by other checks. The test exercising this for subframes,
NestedURLNavigationsViaProxyBlocked, was updated accordingly.
Why don't our other checks work?
--------------------------------
1. ExtensionNavigationThrottle also performs web-accessible resource
checks checks, but that currently happens only for subframes. Thus,
ShouldAllowOpenURL are the only checks that would work in this
scenario for main frames. Fundamentally, ExtensionNavigationThrottle
can't perform the WAR checks for main frames until we plumb through
the initiator SiteInstance (i.e., who navigated the proxy, and not the
StartingSiteInstance, which corresponds to the frame that's being
navigated) to NavigationHandle.
2. ChromeExtensionNetworkDelegate also blocks unsafe navigations to
blob/filesystem URLs. It doesn't apply in this case, since the process
where this navigation is happening is already an extension process.
3. url_request_util::AllowCrossRendererResourceLoad also allows this
navigation, as it explicitly excludes all main frames from WAR
enforcement:
// Navigating the main frame to an extension URL is allowed, even if not
// explicitly listed as web_accessible_resource.
if (info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) {
*allowed = true;
return true;
}
BUG=656752, 645028, 652887
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
to
==========
Fix web accessible resource checks in ShouldAllowOpenURL
(This is joint work with nick@)
When a web page navigates an extension main frame to another extension
URL via a proxy, RenderFrameProxyHost::OnOpenURL calls
NavigatorImpl::RequestTransferURL, which checks whether the
destination URL points to a web-accessible resource in
ChromeContentBrowserClientExtensionsPart::ShouldAllowOpenURL.
This CL fixes several problems with that check:
1. It doesn't work properly for blob/filesystem URL, since it
references schemes on the destination GURL directly. This leads to
a security problem where all blob/filesystem URLs are allowed. To
fix this, use url::Origin to handle nested URL cases properly,
similarly to other fixes in this area (e.g., issue 645028).
2. It ignores cases where the source SiteInstance's site URL isn't an
HTTP, HTTPS, or extension scheme. Thus, a new tab with a data or
about:blank URL bypasses WAR checks. This artificial restriction
apparently was added to make a test pass several years ago. This
CL removes it, and fixes one test that was wrong and which relied
on this logic. The test is
ExtensionBrowserTest.WindowOpenNoPrivileges; it tried to
window.open a non-web-accessible resource while incorrectly
assuming it was web-accessible. Since it did so from about:blank
with a blank SiteInstance, this bug allowed the test to work.
3. The things that ShouldAllowOpenURL does block aren't blocked
correctly for transfers. RequestTransferURL rewrites the blocked
URL to about:blank, and later, NavigatorImpl::NavigateToEntry
decides that this is a transfer to the same RFH
(is_transfer_to_same == true), so it happily lets the old
navigation continue and load the *old* (disallowed) URL. To fix
this, we return early instead of rewriting dest_url to
about:blank.
4. The SiteInstance plumbed through to ShouldAllowOpenURL when going
via RenderFrameProxyHost::OnOpenURL is wrong: it takes the current
SiteInstance of the frame that's being navigated, rather than the
SiteInstance in which the navigation on the proxy was started.
RenderFrameProxyHost::OnOpenURL already passes the right
SiteInstance as source_site_instance to RequestTransferURL, so it
just needs to use it when it's available.
Two new tests are added:
NestedURLNavigationsViaProxyBlocked for case 1.
WindowOpenInaccessibleResourceFromDataURL for case 2.
The fixes to ShouldAllowOpenURL also apply to subframe navigations as
well as main frame navigations (since it doesn't distinguish between
the two). That is, subframe navigations that go through proxies,
transfers, or OpenURL, will now also have the unsafe blob/filesystem
URL blocking enforced, whereas before this was only done for main
frames by other checks. The test exercising this for subframes,
NestedURLNavigationsViaProxyBlocked, was updated accordingly.
Why don't our other checks work?
--------------------------------
1. ExtensionNavigationThrottle also performs web-accessible resource
checks checks, but that currently happens only for subframes. Thus,
ShouldAllowOpenURL are the only checks that would work in this
scenario for main frames. Fundamentally, ExtensionNavigationThrottle
can't perform the WAR checks for main frames until we plumb through
the initiator SiteInstance (i.e., who navigated the proxy, and not the
StartingSiteInstance, which corresponds to the frame that's being
navigated) to NavigationHandle.
2. ChromeExtensionNetworkDelegate also blocks unsafe navigations to
blob/filesystem URLs. It doesn't apply in this case, since the process
where this navigation is happening is already an extension process.
3. url_request_util::AllowCrossRendererResourceLoad also allows this
navigation, as it explicitly excludes all main frames from WAR
enforcement:
// Navigating the main frame to an extension URL is allowed, even if not
// explicitly listed as web_accessible_resource.
if (info->GetResourceType() == content::RESOURCE_TYPE_MAIN_FRAME) {
*allowed = true;
return true;
}
BUG=656752, 645028, 652887
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/1c3da4f745a8a4d67573ca386fc9ad24ced878d4
Cr-Commit-Position: refs/heads/master@{#429532}
==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/1c3da4f745a8a4d67573ca386fc9ad24ced878d4 Cr-Commit-Position: refs/heads/master@{#429532}
Message was sent while issue was closed.
FYI: Findit identified this CL at revision 429532 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... |
