Lock hosted apps to an origin |
||||
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.
,
Dec 12 2017
,
Mar 6 2018
,
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
,
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
,
Aug 17
I've just posted an update to issue 811939 regarding the revert in #5.
,
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
,
Aug 31
,
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
,
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 |
||||
Comment 1 by lukasza@chromium.org
, Dec 12 2017