New issue
Advanced search Search tips

Issue 794315 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 31
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 721947
issue 770239
issue 811939



Sign in to add a comment

Lock hosted apps to an origin

Project Member Reported by lukasza@chromium.org, Dec 12 2017

Issue description

Today, even in --site-per-process mode, hosted apps are not locked to an origin:

    // static
    bool ChromeContentBrowserClientExtensionsPart::ShouldLockToOrigin(
        content::BrowserContext* browser_context,
        const GURL& effective_site_url) {
      //  https://crbug.com/160576  workaround: Origin lock to the chrome-extension://
      // scheme for a hosted app would kill processes on legitimate requests for the
      // app's cookies.
      if (effective_site_url.SchemeIs(extensions::kExtensionScheme)) {
        const Extension* extension =
            ExtensionRegistry::Get(browser_context)
                ->enabled_extensions()
                .GetExtensionOrAppByURL(effective_site_url);
        if (extension && extension->is_hosted_app())
          return false;

We are considering enabling locking hosted apps to their *web* origin (not the extension origin).

Without the locking, enforcements in RenderFrameHostImpl::CanCommitOrigin have to be relaxed in the case where the process is not locked to an origin - otherwise tests like ExtensionStorageMonitorTest.TwoHostedAppsInSameOrigin would fail.  Hopefully locking to the web origin will help tighten up the enforcements.


 
Blocking: 770239
Blocking: 721947

Comment 3 by creis@chromium.org, Mar 6 2018

Blocking: 811939
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 17

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/71426a90ee24d449971c658cecbdb2fb2d713e2c

commit 71426a90ee24d449971c658cecbdb2fb2d713e2c
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Fri Aug 17 00:14:43 2018

Lock hosted apps to their underlying web origin.

Previously, hosted apps were exempt from LockToOrigin() even in
--site-per-process mode.  That meant that hosted apps were not subject
to enforcements such as not allowing access to cookies, passwords, or
local storage of other sites.  Worse, it meant that hosted apps could
arbitrarily share a process (e.g., when over process limit), even if
they covered different web sites with --site-per-process.

This CL starts locking hosted apps to their underlying web origin.  If
a frame commits a navigation to URL http://foo.com, which is part of a
hosted app X's web extent, the process for that frame will be locked
to http://foo.com.  Note that the SiteInstance for this frame will
still use a site URL based on the effective URL (i.e.,
chrome-extension://<ext_id_for_X>/), but the origin lock will not be
based on effective URLs.  This requires plumbing to compute the origin
lock as a site URL that does not use an effective URL, and to plumb it
into various places that make process model decisions, such as
RPHI::IsSuitableHost().

Bug:  811939 ,  794315 ,  791796 
Change-Id: Icc9b3c0a04253e581ea35953f3c566308305db59
Reviewed-on: https://chromium-review.googlesource.com/959346
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583895}
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/chrome/browser/ui/extensions/hosted_app_browsertest.cc
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/content/browser/browsing_instance.cc
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/content/browser/child_process_security_policy_impl.cc
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/content/browser/frame_host/navigator_impl_unittest.cc
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/content/browser/isolated_origin_browsertest.cc
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/content/browser/renderer_host/render_process_host_unittest.cc
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/content/browser/site_instance_impl.cc
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/content/browser/site_instance_impl.h
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/content/browser/site_instance_impl_unittest.cc
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/content/public/browser/site_instance.h

Project Member

Comment 5 by bugdroid1@chromium.org, Aug 17

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/218dcacf7e8891fcad111eaffcfe86794ce79b40

commit 218dcacf7e8891fcad111eaffcfe86794ce79b40
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Fri Aug 17 23:12:44 2018

Revert "Lock hosted apps to their underlying web origin."

This reverts commit 71426a90ee24d449971c658cecbdb2fb2d713e2c.

Reason for revert: Causing cookies and localStorage renderer kills involving the Drive app. I found a local repro where an app covers both http and https versions of a site, and a renderer-initiated navigation from http to https does not trigger a process transfer as it should.

Original change's description:
> Lock hosted apps to their underlying web origin.
> 
> Previously, hosted apps were exempt from LockToOrigin() even in
> --site-per-process mode.  That meant that hosted apps were not subject
> to enforcements such as not allowing access to cookies, passwords, or
> local storage of other sites.  Worse, it meant that hosted apps could
> arbitrarily share a process (e.g., when over process limit), even if
> they covered different web sites with --site-per-process.
> 
> This CL starts locking hosted apps to their underlying web origin.  If
> a frame commits a navigation to URL http://foo.com, which is part of a
> hosted app X's web extent, the process for that frame will be locked
> to http://foo.com.  Note that the SiteInstance for this frame will
> still use a site URL based on the effective URL (i.e.,
> chrome-extension://<ext_id_for_X>/), but the origin lock will not be
> based on effective URLs.  This requires plumbing to compute the origin
> lock as a site URL that does not use an effective URL, and to plumb it
> into various places that make process model decisions, such as
> RPHI::IsSuitableHost().
> 
> Bug:  811939 ,  794315 ,  791796 
> Change-Id: Icc9b3c0a04253e581ea35953f3c566308305db59
> Reviewed-on: https://chromium-review.googlesource.com/959346
> Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
> Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
> Reviewed-by: Charlie Reis <creis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#583895}

TBR=creis@chromium.org,rdevlin.cronin@chromium.org,alexmos@chromium.org

Change-Id: Ie459ef9b61eb78b6dad44e253768d8c4234e6abf
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  811939 ,  794315 ,  791796 
Reviewed-on: https://chromium-review.googlesource.com/1180308
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584231}
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/chrome/browser/ui/extensions/hosted_app_browsertest.cc
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/content/browser/browsing_instance.cc
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/content/browser/child_process_security_policy_impl.cc
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/content/browser/frame_host/navigator_impl_unittest.cc
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/content/browser/isolated_origin_browsertest.cc
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/content/browser/renderer_host/render_process_host_unittest.cc
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/content/browser/site_instance_impl.cc
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/content/browser/site_instance_impl.h
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/content/browser/site_instance_impl_unittest.cc
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/content/public/browser/site_instance.h

Owner: alex...@chromium.org
Status: Started (was: Untriaged)
I've just posted an update to  issue 811939  regarding the revert in #5.
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 29

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5f926a551d7803b47517c7f928448ffc944d50d0

commit 5f926a551d7803b47517c7f928448ffc944d50d0
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Wed Aug 29 20:57:30 2018

Reland: lock hosted apps to their underlying web origin.

This is a reland of https://crrev.com/c/959346 with an additional fix
for cross-site navigations between sites covered by the same hosted
app.  The fix modifies site URLs for effective URLs to also include
the non-translated site in the hash.  This ensures that when multiple
sites translate to the same effective app URL, they still have
distinct site URLs.  This ensures proper process swaps when navigating
among these sites.

Original description:

Previously, hosted apps were exempt from LockToOrigin() even in
--site-per-process mode.  That meant that hosted apps were not subject
to enforcements such as not allowing access to cookies, passwords, or
local storage of other sites.  Worse, it meant that hosted apps could
arbitrarily share a process (e.g., when over process limit), even if
they covered different web sites with --site-per-process.

This CL starts locking hosted apps to their underlying web origin.  If
a frame commits a navigation to URL http://foo.com, which is part of a
hosted app X's web extent, the process for that frame will be locked
to http://foo.com.  Note that the SiteInstance for this frame will
still use a site URL based on the effective URL (i.e.,
chrome-extension://<ext_id_for_X>/), but the origin lock will not be
based on effective URLs.  This requires plumbing to compute the origin
lock as a site URL that does not use an effective URL, and to plumb it
into various places that make process model decisions, such as
RPHI::IsSuitableHost().

Bug:  811939 ,  794315 ,  791796 
Change-Id: I32cc54f3ee12ed5762272e5cfe43bd5eca94e2eb
Reviewed-on: https://chromium-review.googlesource.com/959346
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#583895}
Reviewed-on: https://chromium-review.googlesource.com/1194724
Cr-Commit-Position: refs/heads/master@{#587295}
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/chrome/browser/ui/extensions/hosted_app_browsertest.cc
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/browser/browsing_instance.cc
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/browser/child_process_security_policy_impl.cc
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/browser/frame_host/navigator_impl_unittest.cc
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/browser/frame_host/render_frame_host_manager_unittest.cc
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/browser/isolated_origin_browsertest.cc
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/browser/renderer_host/render_process_host_unittest.cc
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/browser/site_instance_impl.cc
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/browser/site_instance_impl.h
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/browser/site_instance_impl_unittest.cc
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/public/browser/site_instance.h

Status: Fixed (was: Started)
See update at  https://crbug.com/811939#c11 .
Project Member

Comment 9 by bugdroid1@chromium.org, Sep 17

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5aa29f5528ae4755cb7ddcf2ef49709b1c4e534d

commit 5aa29f5528ae4755cb7ddcf2ef49709b1c4e534d
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Mon Sep 17 17:04:45 2018

Remove stale comment in ChromeContentBrowserClient::ShouldLockToOrigin.

Now that we lock hosted apps to their underlying origins, this comment
no longer applies.

Bug:  794315 
Change-Id: Ifae0e3d380927d4541d643725462def494905956
Reviewed-on: https://chromium-review.googlesource.com/1227384
Reviewed-by: Ɓukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591713}
[modify] https://crrev.com/5aa29f5528ae4755cb7ddcf2ef49709b1c4e534d/chrome/browser/chrome_content_browser_client.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Oct 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/68e20c09efe0c5c16c19c10d260d81c9d06ae802

commit 68e20c09efe0c5c16c19c10d260d81c9d06ae802
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Fri Oct 12 00:09:08 2018

Switch CanAccessDataForOrigin to use DetermineProcessLockURL.

CanAccessDataForOrigin currently calls GetSiteForURL() to determine
what should be checked against the process's origin lock.  This isn't
entirely accurate, as GetSiteForURL() defaults to using effective
URLs, which we don't want to use for comparing origin locks.
Fortunately, we also pass in null for browser_context, which
effectively avoids effective URL resolution:
ChromeContentBrowserClient::GetEffectiveURL returns back the original
URL in that case.  This CL change this call to use
DetermineProcessLockForURL() instead, which is more correct.

This CL also removes a stale comment about hosted apps not being able
to set cookies.  That is no longer true, since we now lock hosted apps
to their underlying web origin, which doesn't get in the way of IO
thread enforcements.

Bug:  160576 , 718516,  794315 
Change-Id: I092e9bf89b3a9fc5807824bbe51d1de6589ddae3
Reviewed-on: https://chromium-review.googlesource.com/c/1276560
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599028}
[modify] https://crrev.com/68e20c09efe0c5c16c19c10d260d81c9d06ae802/content/browser/child_process_security_policy_impl.cc

Sign in to add a comment