Fix web accessible resource checks in ShouldAllowOpenURL for M55

Fix web accessible resource checks in ShouldAllowOpenURL for M55

This is a manual merge which contains two CLs, r429532 and r431074, and
also removes DumpWithoutCrashing calls.  Descriptions of both below.

Part 1
------
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;
  }

Review-Url: https://codereview.chromium.org/2454563003
Cr-Commit-Position: refs/heads/master@{#429532}
(cherry picked from commit 1c3da4f745a8a4d67573ca386fc9ad24ced878d4)

Part 2
------

Allow navigations to non-web-accessible resources from chrome schemes.

Recent fixes for web accessible resource checks in ShouldAllowOpenURL
(https://codereview.chromium.org/2454563003) removed the free pass
that non-HTTP(S) and non-extensions schemes used to have to load
non-web-accessible resources.  However, data gathered in that CL
showed that this is breaking navigations from NTP pages (e.g., if an
extensions page appears on the recent tiles) and chrome://history when
clicking on an extensions page.  To fix these, this CL relaxes the
rules in ShouldAllowOpenURL to allow chrome:// and chrome-search://
pages to navigate to non-WAR extension pages.

BUG=656752, 662602
R=nick@chromium.org
TBR=asvitkine@chromium.org, rdevlin.cronin@chromium.org
Review-Url: https://codereview.chromium.org/2486843003
Cr-Commit-Position: refs/heads/master@{#431074}
(cherry picked from commit cf5145bb7919c53d6866829a0f4fa7f2af0b7ca4)
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://chromium.googlesource.com/chromium/src/+/5a5e4bf10d71da4ec995cb6caf92b6f770dc5d49

[alexmos, 2016-11-15 02:20:06]

Description was changed from

==========
Fix web accessible resource checks in ShouldAllowOpenURL for M55

This is a manual merge which contains two CLs, r429532 and r431074, and
also removing DumpWithoutCrashing calls.  Descriptions of both below.

Part 1
------
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;
  }

Review-Url: https://codereview.chromium.org/2454563003
Cr-Commit-Position: refs/heads/master@{#429532}
(cherry picked from commit 1c3da4f745a8a4d67573ca386fc9ad24ced878d4)

Part 2
------

Allow navigations to non-web-accessible resources from chrome schemes.

Recent fixes for web accessible resource checks in ShouldAllowOpenURL
(https://codereview.chromium.org/2454563003) removed the free pass
that non-HTTP(S) and non-extensions schemes used to have to load
non-web-accessible resources.  However, data gathered in that CL
showed that this is breaking navigations from NTP pages (e.g., if an
extensions page appears on the recent tiles) and chrome://history when
clicking on an extensions page.  To fix these, this CL relaxes the
rules in ShouldAllowOpenURL to allow chrome:// and chrome-search://
pages to navigate to non-WAR extension pages.

BUG=656752

Review-Url: https://codereview.chromium.org/2486843003
Cr-Commit-Position: refs/heads/master@{#431074}
(cherry picked from commit cf5145bb7919c53d6866829a0f4fa7f2af0b7ca4)
==========

to

==========
Fix web accessible resource checks in ShouldAllowOpenURL for M55

This is a manual merge which contains two CLs, r429532 and r431074, and
also removing DumpWithoutCrashing calls.  Descriptions of both below.

Part 1
------
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;
  }

Review-Url: https://codereview.chromium.org/2454563003
Cr-Commit-Position: refs/heads/master@{#429532}
(cherry picked from commit 1c3da4f745a8a4d67573ca386fc9ad24ced878d4)

Part 2
------

Allow navigations to non-web-accessible resources from chrome schemes.

Recent fixes for web accessible resource checks in ShouldAllowOpenURL
(https://codereview.chromium.org/2454563003) removed the free pass
that non-HTTP(S) and non-extensions schemes used to have to load
non-web-accessible resources.  However, data gathered in that CL
showed that this is breaking navigations from NTP pages (e.g., if an
extensions page appears on the recent tiles) and chrome://history when
clicking on an extensions page.  To fix these, this CL relaxes the
rules in ShouldAllowOpenURL to allow chrome:// and chrome-search://
pages to navigate to non-WAR extension pages.

BUG=656752

Review-Url: https://codereview.chromium.org/2486843003
Cr-Commit-Position: refs/heads/master@{#431074}
(cherry picked from commit cf5145bb7919c53d6866829a0f4fa7f2af0b7ca4)
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[alexmos, 2016-11-15 02:20:48]

Description was changed from

==========
Fix web accessible resource checks in ShouldAllowOpenURL for M55

This is a manual merge which contains two CLs, r429532 and r431074, and
also removing DumpWithoutCrashing calls.  Descriptions of both below.

Part 1
------
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;
  }

Review-Url: https://codereview.chromium.org/2454563003
Cr-Commit-Position: refs/heads/master@{#429532}
(cherry picked from commit 1c3da4f745a8a4d67573ca386fc9ad24ced878d4)

Part 2
------

Allow navigations to non-web-accessible resources from chrome schemes.

Recent fixes for web accessible resource checks in ShouldAllowOpenURL
(https://codereview.chromium.org/2454563003) removed the free pass
that non-HTTP(S) and non-extensions schemes used to have to load
non-web-accessible resources.  However, data gathered in that CL
showed that this is breaking navigations from NTP pages (e.g., if an
extensions page appears on the recent tiles) and chrome://history when
clicking on an extensions page.  To fix these, this CL relaxes the
rules in ShouldAllowOpenURL to allow chrome:// and chrome-search://
pages to navigate to non-WAR extension pages.

BUG=656752

Review-Url: https://codereview.chromium.org/2486843003
Cr-Commit-Position: refs/heads/master@{#431074}
(cherry picked from commit cf5145bb7919c53d6866829a0f4fa7f2af0b7ca4)
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix web accessible resource checks in ShouldAllowOpenURL for M55

This is a manual merge which contains two CLs, r429532 and r431074, and
also removes DumpWithoutCrashing calls.  Descriptions of both below.

Part 1
------
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;
  }

Review-Url: https://codereview.chromium.org/2454563003
Cr-Commit-Position: refs/heads/master@{#429532}
(cherry picked from commit 1c3da4f745a8a4d67573ca386fc9ad24ced878d4)

Part 2
------

Allow navigations to non-web-accessible resources from chrome schemes.

Recent fixes for web accessible resource checks in ShouldAllowOpenURL
(https://codereview.chromium.org/2454563003) removed the free pass
that non-HTTP(S) and non-extensions schemes used to have to load
non-web-accessible resources.  However, data gathered in that CL
showed that this is breaking navigations from NTP pages (e.g., if an
extensions page appears on the recent tiles) and chrome://history when
clicking on an extensions page.  To fix these, this CL relaxes the
rules in ShouldAllowOpenURL to allow chrome:// and chrome-search://
pages to navigate to non-WAR extension pages.

BUG=656752

Review-Url: https://codereview.chromium.org/2486843003
Cr-Commit-Position: refs/heads/master@{#431074}
(cherry picked from commit cf5145bb7919c53d6866829a0f4fa7f2af0b7ca4)
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[alexmos, 2016-11-15 17:32:01]

Description was changed from

==========
Fix web accessible resource checks in ShouldAllowOpenURL for M55

This is a manual merge which contains two CLs, r429532 and r431074, and
also removes DumpWithoutCrashing calls.  Descriptions of both below.

Part 1
------
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;
  }

Review-Url: https://codereview.chromium.org/2454563003
Cr-Commit-Position: refs/heads/master@{#429532}
(cherry picked from commit 1c3da4f745a8a4d67573ca386fc9ad24ced878d4)

Part 2
------

Allow navigations to non-web-accessible resources from chrome schemes.

Recent fixes for web accessible resource checks in ShouldAllowOpenURL
(https://codereview.chromium.org/2454563003) removed the free pass
that non-HTTP(S) and non-extensions schemes used to have to load
non-web-accessible resources.  However, data gathered in that CL
showed that this is breaking navigations from NTP pages (e.g., if an
extensions page appears on the recent tiles) and chrome://history when
clicking on an extensions page.  To fix these, this CL relaxes the
rules in ShouldAllowOpenURL to allow chrome:// and chrome-search://
pages to navigate to non-WAR extension pages.

BUG=656752

Review-Url: https://codereview.chromium.org/2486843003
Cr-Commit-Position: refs/heads/master@{#431074}
(cherry picked from commit cf5145bb7919c53d6866829a0f4fa7f2af0b7ca4)
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix web accessible resource checks in ShouldAllowOpenURL for M55

This is a manual merge which contains two CLs, r429532 and r431074, and
also removes DumpWithoutCrashing calls.  Descriptions of both below.

Part 1
------
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;
  }

Review-Url: https://codereview.chromium.org/2454563003
Cr-Commit-Position: refs/heads/master@{#429532}
(cherry picked from commit 1c3da4f745a8a4d67573ca386fc9ad24ced878d4)

Part 2
------

Allow navigations to non-web-accessible resources from chrome schemes.

Recent fixes for web accessible resource checks in ShouldAllowOpenURL
(https://codereview.chromium.org/2454563003) removed the free pass
that non-HTTP(S) and non-extensions schemes used to have to load
non-web-accessible resources.  However, data gathered in that CL
showed that this is breaking navigations from NTP pages (e.g., if an
extensions page appears on the recent tiles) and chrome://history when
clicking on an extensions page.  To fix these, this CL relaxes the
rules in ShouldAllowOpenURL to allow chrome:// and chrome-search://
pages to navigate to non-WAR extension pages.

BUG=656752,662602

Review-Url: https://codereview.chromium.org/2486843003
Cr-Commit-Position: refs/heads/master@{#431074}
(cherry picked from commit cf5145bb7919c53d6866829a0f4fa7f2af0b7ca4)
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[alexmos, 2016-11-15 17:37:55]

alexmos@chromium.org changed reviewers:
+ ncarter@chromium.org

[alexmos, 2016-11-15 17:37:56]

Nick, can you please double-check this manual merge of the ShouldAllowOpenURL
bits to M55?

[alexmos, 2016-11-15 17:38:32]

alexmos@chromium.org changed reviewers:
+ nick@chromium.org
- ncarter@chromium.org

[alexmos, 2016-11-15 17:38:33]

Nick, can you please double-check this manual merge of the ShouldAllowOpenURL
bits to M55?

[ncarter (slow), 2016-11-15 18:12:59]

lgtm

[alexmos, 2016-11-15 18:21:01]

Description was changed from

==========
Fix web accessible resource checks in ShouldAllowOpenURL for M55

This is a manual merge which contains two CLs, r429532 and r431074, and
also removes DumpWithoutCrashing calls.  Descriptions of both below.

Part 1
------
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;
  }

Review-Url: https://codereview.chromium.org/2454563003
Cr-Commit-Position: refs/heads/master@{#429532}
(cherry picked from commit 1c3da4f745a8a4d67573ca386fc9ad24ced878d4)

Part 2
------

Allow navigations to non-web-accessible resources from chrome schemes.

Recent fixes for web accessible resource checks in ShouldAllowOpenURL
(https://codereview.chromium.org/2454563003) removed the free pass
that non-HTTP(S) and non-extensions schemes used to have to load
non-web-accessible resources.  However, data gathered in that CL
showed that this is breaking navigations from NTP pages (e.g., if an
extensions page appears on the recent tiles) and chrome://history when
clicking on an extensions page.  To fix these, this CL relaxes the
rules in ShouldAllowOpenURL to allow chrome:// and chrome-search://
pages to navigate to non-WAR extension pages.

BUG=656752,662602

Review-Url: https://codereview.chromium.org/2486843003
Cr-Commit-Position: refs/heads/master@{#431074}
(cherry picked from commit cf5145bb7919c53d6866829a0f4fa7f2af0b7ca4)
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix web accessible resource checks in ShouldAllowOpenURL for M55

This is a manual merge which contains two CLs, r429532 and r431074, and
also removes DumpWithoutCrashing calls.  Descriptions of both below.

Part 1
------
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;
  }

Review-Url: https://codereview.chromium.org/2454563003
Cr-Commit-Position: refs/heads/master@{#429532}
(cherry picked from commit 1c3da4f745a8a4d67573ca386fc9ad24ced878d4)

Part 2
------

Allow navigations to non-web-accessible resources from chrome schemes.

Recent fixes for web accessible resource checks in ShouldAllowOpenURL
(https://codereview.chromium.org/2454563003) removed the free pass
that non-HTTP(S) and non-extensions schemes used to have to load
non-web-accessible resources.  However, data gathered in that CL
showed that this is breaking navigations from NTP pages (e.g., if an
extensions page appears on the recent tiles) and chrome://history when
clicking on an extensions page.  To fix these, this CL relaxes the
rules in ShouldAllowOpenURL to allow chrome:// and chrome-search://
pages to navigate to non-WAR extension pages.

BUG=656752,662602
TBR=rdevlin.cronin@chromium.org,asvitkine@chromium.org
Review-Url: https://codereview.chromium.org/2486843003
Cr-Commit-Position: refs/heads/master@{#431074}
(cherry picked from commit cf5145bb7919c53d6866829a0f4fa7f2af0b7ca4)
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[alexmos, 2016-11-15 18:26:03]

Description was changed from

==========
Fix web accessible resource checks in ShouldAllowOpenURL for M55

This is a manual merge which contains two CLs, r429532 and r431074, and
also removes DumpWithoutCrashing calls.  Descriptions of both below.

Part 1
------
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;
  }

Review-Url: https://codereview.chromium.org/2454563003
Cr-Commit-Position: refs/heads/master@{#429532}
(cherry picked from commit 1c3da4f745a8a4d67573ca386fc9ad24ced878d4)

Part 2
------

Allow navigations to non-web-accessible resources from chrome schemes.

Recent fixes for web accessible resource checks in ShouldAllowOpenURL
(https://codereview.chromium.org/2454563003) removed the free pass
that non-HTTP(S) and non-extensions schemes used to have to load
non-web-accessible resources.  However, data gathered in that CL
showed that this is breaking navigations from NTP pages (e.g., if an
extensions page appears on the recent tiles) and chrome://history when
clicking on an extensions page.  To fix these, this CL relaxes the
rules in ShouldAllowOpenURL to allow chrome:// and chrome-search://
pages to navigate to non-WAR extension pages.

BUG=656752,662602
TBR=rdevlin.cronin@chromium.org,asvitkine@chromium.org
Review-Url: https://codereview.chromium.org/2486843003
Cr-Commit-Position: refs/heads/master@{#431074}
(cherry picked from commit cf5145bb7919c53d6866829a0f4fa7f2af0b7ca4)
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix web accessible resource checks in ShouldAllowOpenURL for M55

This is a manual merge which contains two CLs, r429532 and r431074, and
also removes DumpWithoutCrashing calls.  Descriptions of both below.

Part 1
------
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;
  }

Review-Url: https://codereview.chromium.org/2454563003
Cr-Commit-Position: refs/heads/master@{#429532}
(cherry picked from commit 1c3da4f745a8a4d67573ca386fc9ad24ced878d4)

Part 2
------

Allow navigations to non-web-accessible resources from chrome schemes.

Recent fixes for web accessible resource checks in ShouldAllowOpenURL
(https://codereview.chromium.org/2454563003) removed the free pass
that non-HTTP(S) and non-extensions schemes used to have to load
non-web-accessible resources.  However, data gathered in that CL
showed that this is breaking navigations from NTP pages (e.g., if an
extensions page appears on the recent tiles) and chrome://history when
clicking on an extensions page.  To fix these, this CL relaxes the
rules in ShouldAllowOpenURL to allow chrome:// and chrome-search://
pages to navigate to non-WAR extension pages.

BUG=656752,662602
R=nick@chromium.org
TBR=asvitkine@chromium.org, rdevlin.cronin@chromium.org
Review-Url: https://codereview.chromium.org/2486843003
Cr-Commit-Position: refs/heads/master@{#431074}
(cherry picked from commit cf5145bb7919c53d6866829a0f4fa7f2af0b7ca4)
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed:
https://chromium.googlesource.com/chromium/src/+/5a5e4bf10d71da4ec995cb6caf92...
==========

[alexmos, 2016-11-15 18:26:04]

Committed patchset #2 (id:20001) manually as
5a5e4bf10d71da4ec995cb6caf92b6f770dc5d49 (presubmit successful).