New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 846346 link

Starred by 3 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug


Sign in to add a comment

CORB: Need to ensure that requests from extension content scripts are allowed/blocked in a proper way

Project Member Reported by lukasza@chromium.org, May 24 2018

Issue description

CrossSiteDocumentResourceHandler tries to ensure that requests from extension content scripts are not blocked by Cross-Origin Read Blocking (CORB):

      // Give embedder a chance to skip document blocking for this response.
      const char* initiator_scheme_exception =
          GetContentClient()
              ->browser()
              ->GetInitatorSchemeBypassingDocumentBlocking();
      if (initiator_scheme_exception && request()->initiator().has_value() &&
          request()->initiator()->scheme() == initiator_scheme_exception) {
        return false;
      }

Note that in the CORB/NetworkService integration CL (https://crrev.com/c/1033535) the check above is getting extracted into CrossOriginReadBlocking::ShouldBlockBasedOnHeaders.

There are 2 things missing AFAIK:

1. There is no *explicit* regresion test for this behavior.  There may be some accidental test coverage (via RemoteDebuggingTest.RemoteDebugger maybe/AFAIR?) but it seems fragile - we should create a test that explicitly uses a HTTP response that is CORB-eligible.

2. I am not sure how to *properly* preserve this behavior in CORB/NetworkService integration.  After https://crrev.com/c/1033535 CrossOriginReadBlocking::ShouldBlockBasedOnHeaders will check |initiator.scheme()|, but this is not the right thing to do in the long term - in the long-term we need to:
    - Ensure that extension content scripts get a separate URLLoaderFactory (separate from the regular web contents committed into a frame/document)
    - Ensure that allowing/blocking of CORB-eligible responses is more granular - in particular, extensions (only in extension manifest v3?) should be able to declare which origins they need to issues requests to.  CORB should still block requests from other 
 

Comment 1 by dxie@google.com, Jun 27 2018

Labels: Proj-Servicification Hotlist-KnownIssue OS-Windows

Comment 2 by jam@chromium.org, Jun 27 2018

Labels: -Hotlist-KnownIssue -Proj-Servicification
This doesn't seem specific to network service? i.e. we already check the initiator scheme with NS.

Similarly for using separate URLLoaderFactory, that can happen for both code paths (NS enabled/disabled).

From my reading, I don't see anything NS specific so I'll remove the servicification bug. Please re-add if I missed something.
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 6

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

commit ce869f0b41da0860b0d3260aaa846c71e3acd33b
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Mon Aug 06 18:33:37 2018

CORB: Explicit test that requests from content scripts are not blocked.

Bug: 846346
Change-Id: I943bbecdd669c6e7c4eb5d3c5de6331dd35e0b1e
Reviewed-on: https://chromium-review.googlesource.com/1159269
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Nick Carter <nick@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580929}
[modify] https://crrev.com/ce869f0b41da0860b0d3260aaa846c71e3acd33b/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/ce869f0b41da0860b0d3260aaa846c71e3acd33b/chrome/browser/chrome_content_browser_client.h
[add] https://crrev.com/ce869f0b41da0860b0d3260aaa846c71e3acd33b/chrome/browser/extensions/cross_origin_read_blocking_browsertest.cc
[modify] https://crrev.com/ce869f0b41da0860b0d3260aaa846c71e3acd33b/chrome/test/BUILD.gn
[add] https://crrev.com/ce869f0b41da0860b0d3260aaa846c71e3acd33b/chrome/test/data/nosniff.xml
[add] https://crrev.com/ce869f0b41da0860b0d3260aaa846c71e3acd33b/chrome/test/data/nosniff.xml.mock-http-headers
[modify] https://crrev.com/ce869f0b41da0860b0d3260aaa846c71e3acd33b/content/browser/loader/cross_site_document_resource_handler.cc
[modify] https://crrev.com/ce869f0b41da0860b0d3260aaa846c71e3acd33b/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/ce869f0b41da0860b0d3260aaa846c71e3acd33b/content/public/browser/content_browser_client.h
[modify] https://crrev.com/ce869f0b41da0860b0d3260aaa846c71e3acd33b/content/public/browser/site_isolation_policy.cc

Blocking: 871827
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 8

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

commit faa34ebca55212003d2a56cafebaab599659d530
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Wed Aug 08 01:02:50 2018

Rappor logging of extensions that would be impacted by CORB.

This CL moves processing of |excluded_initiator_scheme| outside of
network::CrossOriginReadBlocking.  The postponed allowing of requests
initiated from content scripts allows to report them to
LogInitiatorSchemeBypassingDocumentBlocking only if they would actually
be blocked by CORB (in a meaningful way - non-empty response body,
non-4xx, etc.).

Bug: 846346
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: Ifcc3e610506b757d8b8e4d6fe36f28bb783e7944
Reviewed-on: https://chromium-review.googlesource.com/1161194
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Nick Carter <nick@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581422}
[modify] https://crrev.com/faa34ebca55212003d2a56cafebaab599659d530/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/faa34ebca55212003d2a56cafebaab599659d530/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/faa34ebca55212003d2a56cafebaab599659d530/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
[modify] https://crrev.com/faa34ebca55212003d2a56cafebaab599659d530/chrome/browser/extensions/chrome_content_browser_client_extensions_part.h
[modify] https://crrev.com/faa34ebca55212003d2a56cafebaab599659d530/chrome/browser/extensions/cross_origin_read_blocking_browsertest.cc
[add] https://crrev.com/faa34ebca55212003d2a56cafebaab599659d530/chrome/test/data/nosniff.empty
[add] https://crrev.com/faa34ebca55212003d2a56cafebaab599659d530/chrome/test/data/nosniff.empty.mock-http-headers
[modify] https://crrev.com/faa34ebca55212003d2a56cafebaab599659d530/content/browser/loader/cross_site_document_resource_handler.cc
[modify] https://crrev.com/faa34ebca55212003d2a56cafebaab599659d530/content/browser/loader/cross_site_document_resource_handler.h
[modify] https://crrev.com/faa34ebca55212003d2a56cafebaab599659d530/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/faa34ebca55212003d2a56cafebaab599659d530/content/public/browser/content_browser_client.h
[modify] https://crrev.com/faa34ebca55212003d2a56cafebaab599659d530/services/network/cross_origin_read_blocking.cc
[modify] https://crrev.com/faa34ebca55212003d2a56cafebaab599659d530/services/network/cross_origin_read_blocking.h
[modify] https://crrev.com/faa34ebca55212003d2a56cafebaab599659d530/services/network/url_loader.cc
[modify] https://crrev.com/faa34ebca55212003d2a56cafebaab599659d530/tools/metrics/histograms/histograms.xml
[modify] https://crrev.com/faa34ebca55212003d2a56cafebaab599659d530/tools/metrics/rappor/rappor.xml

The rappor code from #c5 seems to be working.  I've gathered some very early results (only Canary + Dev with very little data harvested so far) into https://docs.google.com/document/d/1ReuTBgn6HDIO0QKsjzBZz2Bz9F9_sEW4X-rz6IogURo (Google-internal, sorry...).
The CL in #c5 tries to limit logging to requests coming from content scripts, but apparently fails and is logging too much.  In particular, it logs the following requests:
- initiator SiteInstance: chrome-guest://andfmajejfpjojledngpdaibbhkffipo/?uv-0.046046002002287256
- initiator origin: Chrome App
- request url: https url

I am not sure what is the best way to remediate the issue above:
- Just ignore Chrome Apps in the Rappor results?
- Change Chromium code to avoid logging if the request comes from a GuestView process?
    - GuestView threat model is special - if there is no Site Isolation within GuestView,
      then we can also consider disabling CORB (at least for requests that both A) come
      from GuestView renderer and B) have initiator=chrome-extension://).
- Identify requests coming from content scripts in a more direct/explicit way?
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 28

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

commit aea893e0728afb8e81b2960fe92387bfc255be3d
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Tue Aug 28 22:35:12 2018

CrossOriginFetchFromContentScript Rappor metric should ignore GuestViews

Bug: 846346
Change-Id: Ic84e15d46c3dd8286ef774088f60dab52a8d51d8
Reviewed-on: https://chromium-review.googlesource.com/1183843
Reviewed-by: Nick Carter <nick@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586894}
[modify] https://crrev.com/aea893e0728afb8e81b2960fe92387bfc255be3d/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
[modify] https://crrev.com/aea893e0728afb8e81b2960fe92387bfc255be3d/tools/metrics/rappor/rappor.xml

I've put together my notes about "URLLoaderFactory for content scripts" at https://docs.google.com/document/d/1fuBUmLZj1H319jMkK8waUbf5WdQS0N95J_mWXVzbP7A (viewable by anyone, please ping me if you'd like to add comments).
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 6

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

commit 737f88e80089753792a4f66d7caa0c928d358975
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Thu Sep 06 18:02:24 2018

New test for CORB vs *declarative* content scripts.

This CL makes sure that tests for cross-origin fetches from content
scripts cover both 1) programmatic content scripts (i.e. ones triggered
by chrome.tabs.executeScript) and 2) declarative content scripts (i.e.
ones listed under "content_scripts" entry in an extension manifest).

The extra test coverage is desirable to ensure that the future work to
provide a separate URLLoaderFactory for content scripts gives correct
results both for programmatic and declarative content scripts.  For
example - providing the URLLoaderFactory in ExtensionMsg_ExecuteCode
would be wrong because it would not cover declarative content scripts.

Bug: 846346
Change-Id: Ibe3f32fd69d502e0a0874fde7636629e5b6693fa
Reviewed-on: https://chromium-review.googlesource.com/1208816
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589212}
[modify] https://crrev.com/737f88e80089753792a4f66d7caa0c928d358975/chrome/browser/extensions/cross_origin_read_blocking_browsertest.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Sep 21

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

commit 056df68642e3bf7442aabf8fec579e8e064d8304
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Fri Sep 21 20:16:58 2018

Rename s/factories/scheme_specific_factories/ and other refactorings.

This CL cleans-up URLLoaderFactoryBundle[Info] in preparation for adding
initiator-origin-keyed factories in the future (for supporting
URLLoaderFactories with extension-specific permissions for requests
initiated by content scripts).

Changes in the CL:

- s/factories/scheme_specific_factories/
- s/factories_info/scheme_specific_factory_infos/
    Rationale for the changes above:
    - Making it more explicit that these factories are keyed by a scheme
    - Differentiating the name from |initiator_specific_factories| that
      is planned to be introduced in a follow-up CL
      (https://crrev.com/c/1228478).

- Introducing URLLoaderFactoryBundle::SchemeMap
- Introducing URLLoaderFactoryBundleInfo::SchemeMap
- Fixes for some of issues reported by: git cl lint
- Removing URLLoaderFactoryBundle::RegisterFactory that had no callers.
- Making URLLoaderFactoryBundle::GetFactoryForURL protected
  (it used to be unnecessarily public)
- Having ChildURLLoaderFactoryBundle delegate to its base class
  (URLLoaderFactoryBundle) in its implementation of GetFactoryForURL
  and CreateLoaderAndStart methods.

There are no intended behavior changes in this CL
(well except skipping a DCHECK in GetFactoryForURL method so that
the method can be reused by subclasses with more relaxed invariants).

Bug: 846346
Change-Id: Ibbd07debaf518e2b71cf09a2fd92c8b8104b7411
Reviewed-on: https://chromium-review.googlesource.com/1227135
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593316}
[modify] https://crrev.com/056df68642e3bf7442aabf8fec579e8e064d8304/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/056df68642e3bf7442aabf8fec579e8e064d8304/content/browser/service_worker/embedded_worker_instance.cc
[modify] https://crrev.com/056df68642e3bf7442aabf8fec579e8e064d8304/content/browser/shared_worker/shared_worker_service_impl.cc
[modify] https://crrev.com/056df68642e3bf7442aabf8fec579e8e064d8304/content/common/url_loader_factory_bundle.cc
[modify] https://crrev.com/056df68642e3bf7442aabf8fec579e8e064d8304/content/common/url_loader_factory_bundle.h
[modify] https://crrev.com/056df68642e3bf7442aabf8fec579e8e064d8304/content/common/url_loader_factory_bundle.mojom
[modify] https://crrev.com/056df68642e3bf7442aabf8fec579e8e064d8304/content/common/url_loader_factory_bundle_struct_traits.cc
[modify] https://crrev.com/056df68642e3bf7442aabf8fec579e8e064d8304/content/common/url_loader_factory_bundle_struct_traits.h
[modify] https://crrev.com/056df68642e3bf7442aabf8fec579e8e064d8304/content/renderer/loader/child_url_loader_factory_bundle.cc
[modify] https://crrev.com/056df68642e3bf7442aabf8fec579e8e064d8304/content/renderer/loader/child_url_loader_factory_bundle.h
[modify] https://crrev.com/056df68642e3bf7442aabf8fec579e8e064d8304/content/renderer/loader/tracked_child_url_loader_factory_bundle.cc
[modify] https://crrev.com/056df68642e3bf7442aabf8fec579e8e064d8304/content/renderer/loader/tracked_child_url_loader_factory_bundle.h

Project Member

Comment 12 by bugdroid1@chromium.org, Sep 28

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

commit 762733655d35f606fdc5e2ae433caf1c7b590d0e
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Fri Sep 28 14:48:26 2018

s/GURL/url::Origin/ in ContentBrowserClient::WillCreateURLLoaderFactory.

Changing the type of the parameter from GURL to
url::Origin is desirable because:

1) The only user of the parameter
   (ProxyingURLLoaderFactory::MaybeProxyRequest) was looking at the origin,
   rather than at the URL.

2) Current users of the WillCreateURLLoaderFactory's parameter
   (ProxyingURLLoaderFactory::MaybeProxyRequest) and future callees of
   RenderFrameHostImpl::CreateNetworkServiceDefaultFactoryInternal
   (extensions::URLLoaderFactoryManager - see
   https://crrev.com/c/1228478) should make decisions based on the
   origin rather than URL (URLs with blob:, data: and about: schemes
   are tricky here).

Bug: 846346
Change-Id: Ia1acc3735664c2f6f6290bd23b5431b633381644
Reviewed-on: https://chromium-review.googlesource.com/1234361
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595089}
[modify] https://crrev.com/762733655d35f606fdc5e2ae433caf1c7b590d0e/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/762733655d35f606fdc5e2ae433caf1c7b590d0e/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/762733655d35f606fdc5e2ae433caf1c7b590d0e/chrome/browser/signin/chrome_signin_proxying_url_loader_factory.cc
[modify] https://crrev.com/762733655d35f606fdc5e2ae433caf1c7b590d0e/chrome/browser/signin/chrome_signin_proxying_url_loader_factory.h
[modify] https://crrev.com/762733655d35f606fdc5e2ae433caf1c7b590d0e/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/762733655d35f606fdc5e2ae433caf1c7b590d0e/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/762733655d35f606fdc5e2ae433caf1c7b590d0e/content/browser/loader/navigation_url_loader_impl.cc
[modify] https://crrev.com/762733655d35f606fdc5e2ae433caf1c7b590d0e/content/browser/storage_partition_impl.cc
[modify] https://crrev.com/762733655d35f606fdc5e2ae433caf1c7b590d0e/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/762733655d35f606fdc5e2ae433caf1c7b590d0e/content/public/browser/content_browser_client.h
[modify] https://crrev.com/762733655d35f606fdc5e2ae433caf1c7b590d0e/extensions/shell/browser/shell_content_browser_client.cc
[modify] https://crrev.com/762733655d35f606fdc5e2ae433caf1c7b590d0e/extensions/shell/browser/shell_content_browser_client.h

Project Member

Comment 13 by bugdroid1@chromium.org, Oct 3

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

commit 2f8101670e3ac82b0d80bdd58b9d16334699360c
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Wed Oct 03 21:38:07 2018

Separate URLLoaderFactory for extension content scripts.

CORB makes an exception for content scripts, because they are allowed
to receive cross-origin responses from origins covered by the
extension manifest.  This CL is desirable to:
1. Avoid making this exception when no content scripts have been
   injected into frames hosted in a given renderer process.
2. Pave a way for future CLs to
   2a. stop making this exception if a specific extension doesn't need it.
   2b. make the exception more granular (e.g. instead of turning off
       CORB altogether, turn it off for a few, select origins).

This CL makes the following code changes:

1. Adds |initiator_specific_factories| field to
   content.mojom.URLLoaderFactoryBundle.

2. Introduces public //content APIs for saying that an origin requires a
   separate URLLoaderFactory
       RenderFrameHost::MarkInitiatorAsRequiringSeparateURLLoaderFactory
   this API is called by the following methods of
   extensions::URLLoaderFactoryManager:
     - ReadyToCommitNavigation
     - WillExecuteCode

3. Introduces public //content APIs for overiding the URLLoaderFactory
   created by RenderFrameHostImpl:
       ContentBrowserClient::CreateURLLoaderFactoryForNetworkRequests
   This API is overriden at the //chrome layer and ultimately delegated
   to extensions::URLLoaderFactoryManager::CreateFactory.
   This API is called by RenderFrameHostImpl for both
     - Creating the default URLLoaderFactory for extension frames
     - Creating initiator-specific factories as requested by
       MarkInitiatorAsRequiringSeparateURLLoaderFactory (see above).

4. Moves code that evaluates whether content script matches the given
   document so it can be reused by
   URLLoaderFactoryManagerForContentScripts.  This includes:
   - A little bit of code moved from UserScriptSet::GetInjectionForScript
     to UserScript::MatchesDocument
   - ScriptContext::GetEffectiveDocumentURL which got replicated into
     helper functions in an anonymous namespace used by
     URLLoaderFactoryManagerForContentScripts

5. Removes URLLoaderFactoryParams::corb_excluded_initiator_scheme.
   For content scripts, this flag is made obsolete by the changes above.


For more details please also see the design document at:
https://docs.google.com/document/d/1fuBUmLZj1H319jMkK8waUbf5WdQS0N95J_mWXVzbP7A


Bug: 846346
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I28e14e9b8f6eca4f41687609da56177ec94b4272
Reviewed-on: https://chromium-review.googlesource.com/c/1228478
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596378}
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/chrome/browser/extensions/chrome_content_browser_client_extensions_part.h
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/content/browser/security_exploit_browsertest.cc
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/content/browser/service_worker/embedded_worker_instance.cc
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/content/browser/shared_worker/shared_worker_service_impl.cc
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/content/common/url_loader_factory_bundle.cc
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/content/common/url_loader_factory_bundle.h
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/content/common/url_loader_factory_bundle.mojom
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/content/common/url_loader_factory_bundle_struct_traits.cc
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/content/common/url_loader_factory_bundle_struct_traits.h
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/content/public/browser/content_browser_client.h
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/content/public/browser/render_frame_host.h
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/content/public/browser/render_process_host.h
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/content/public/browser/site_isolation_policy.cc
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/content/public/test/mock_render_process_host.cc
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/content/public/test/mock_render_process_host.h
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/content/renderer/loader/child_url_loader_factory_bundle.cc
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/content/renderer/loader/child_url_loader_factory_bundle.h
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/content/renderer/loader/tracked_child_url_loader_factory_bundle.cc
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/content/renderer/loader/tracked_child_url_loader_factory_bundle.h
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/extensions/browser/BUILD.gn
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/extensions/browser/extension_web_contents_observer.cc
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/extensions/browser/script_executor.cc
[add] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/extensions/browser/url_loader_factory_manager.cc
[add] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/extensions/browser/url_loader_factory_manager.h
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/extensions/common/user_script.cc
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/extensions/common/user_script.h
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/extensions/renderer/user_script_set.cc
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/extensions/shell/browser/shell_content_browser_client.cc
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/extensions/shell/browser/shell_content_browser_client.h
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/services/network/public/mojom/network_context.mojom
[modify] https://crrev.com/2f8101670e3ac82b0d80bdd58b9d16334699360c/services/network/url_loader.cc

Project Member

Comment 14 by bugdroid1@chromium.org, Oct 4

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

commit 4a491a17f95f4ec949d411a166995b2cb541019e
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Thu Oct 04 16:44:01 2018

Cover service workers and foreground pages in CORB Extension Test.

This CL adds the following 2 test cases to the
CrossOriginReadBlockingExtensionTest test suite:

1) FromForegroundPage_NoSniffXml

   This test performs the fetch from a foreground tab that has committed
   an extension page.  This test is quite similar to the already
   existing FromBackgroundPage_NoSniffXml test, but might be worth
   keeping (e.g. in case background pages go away in the future, or in
   case the code handling background-vs-foreground page diverges).

2) FromServiceWorker_NoSniffXml

   This test covers the scenario where a fetch from a foreground tab is
   handled by the extension's service worker.  This scenario means that
   CORB correctly handles the response from the network to the service
   worker - this response in the NetworkService world is handled by
   a URLLoaderFactory that is created separately from the factory
   created by
   RenderFrameHostImpl::CreateNetworkServiceDefaultFactoryInternal.

Bug: 846346
Change-Id: Id8c1ac892b4c3f25ef2b37f980b234bb8f25b6e5
Reviewed-on: https://chromium-review.googlesource.com/c/1257151
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596705}
[modify] https://crrev.com/4a491a17f95f4ec949d411a166995b2cb541019e/chrome/browser/extensions/cross_origin_read_blocking_browsertest.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Oct 4

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

commit 6255033fe7bf5cc5aeef3b0a44cd4f236590c33f
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Thu Oct 04 16:55:36 2018

Cover about:blank and about:srcdoc frames in CORB Extension Test.

This CL expands the following 2 test cases from the
CrossOriginReadBlockingExtensionTest test suite:

1) FromForegroundPage_NoSniffXml

   This CL adds coverage of a fetch performed from an about:srcdoc frame
   that belongs to an extension origin.

2) FromDeclarativeContentScript_NoSniffXml

   This CL adds coverage of a fetch performed from a content script
   injected into an about:blank page.

In both cases above, the fetch is performed before the browser commits
the frame.  The new tests help ensure that in this scenario the
extension scripts are still subject to relaxed CORB rules - because of
the timing the relaxed CORB rules do not depend on
URLLoaderFactoryManager::ReadyToCommitNavigation, but rather on having
the renderer inherit the right URLLoaderFactory for the subframe.

Bug: 846346
Change-Id: I45a4eba6d1351f2bf722f31fbcb7f201cd990d34
Reviewed-on: https://chromium-review.googlesource.com/c/1259291
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596711}
[modify] https://crrev.com/6255033fe7bf5cc5aeef3b0a44cd4f236590c33f/chrome/browser/extensions/cross_origin_read_blocking_browsertest.cc

Project Member

Comment 16 by bugdroid1@chromium.org, Oct 6

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

commit 637e7d6d78125de6ea88a0ea9d50b7bebabb22c3
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Sat Oct 06 05:44:12 2018

Tests for URLLoaderFactoryManager::DoContentScriptsMatchNavigatingFrame.

This CL adds test coverage for
URLLoaderFactoryManager::DoContentScriptsMatchNavigatingFrame.

To enable navigating in extensions_browsertests, this CL:
- Adds an overload of content::NavigateToURL that takes WebContents*
  (rather than Shell*) as an argument and exposes it through the
  public content/public/test/browser_test_utils.h
- Tweaks constructor of extensions::AppShellTest so that it sets
  up the |embedded_test_server| to serve files from the
  //extensions/test/data directory.

Bug: 846346
Change-Id: I4bba6bee18beb5d5e8855a4b3b91fda6a04ca587
Reviewed-on: https://chromium-review.googlesource.com/c/1252009
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597412}
[modify] https://crrev.com/637e7d6d78125de6ea88a0ea9d50b7bebabb22c3/content/public/test/browser_test_utils.cc
[modify] https://crrev.com/637e7d6d78125de6ea88a0ea9d50b7bebabb22c3/content/public/test/browser_test_utils.h
[modify] https://crrev.com/637e7d6d78125de6ea88a0ea9d50b7bebabb22c3/content/public/test/content_browser_test_utils.cc
[modify] https://crrev.com/637e7d6d78125de6ea88a0ea9d50b7bebabb22c3/extensions/browser/BUILD.gn
[modify] https://crrev.com/637e7d6d78125de6ea88a0ea9d50b7bebabb22c3/extensions/browser/url_loader_factory_manager.cc
[modify] https://crrev.com/637e7d6d78125de6ea88a0ea9d50b7bebabb22c3/extensions/browser/url_loader_factory_manager.h
[add] https://crrev.com/637e7d6d78125de6ea88a0ea9d50b7bebabb22c3/extensions/browser/url_loader_factory_manager_browsertest.cc
[modify] https://crrev.com/637e7d6d78125de6ea88a0ea9d50b7bebabb22c3/extensions/shell/test/shell_test.cc
[add] https://crrev.com/637e7d6d78125de6ea88a0ea9d50b7bebabb22c3/extensions/test/data/empty.html

I wanted to note that r596378 (from #c13 above) had to be temporarily/partially reverted by r599434 to deal with  issue 894766 .
Project Member

Comment 18 by bugdroid1@chromium.org, Nov 8

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

commit 4b630b4eb06f30a040d3f9c2a81166f41061ef3f
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Thu Nov 08 05:23:35 2018

Fix and re-enable separate URLLoaderFactories for extensions (AppCache).

Overview
========

This CL re-enables separate URLLoaderFactories for extensions.  This is
possible because of AppCache-related fixes described below.  Re-enabling
is done by effectively reverting r599434.

AppCache is important, because it represents the scenarios when
RenderFrameHostImpl::CommitNavigation uses a non-network-bound default
URLLoaderFactory (e.g. in these scenarios the default factory is *not*
created via CreateNetworkServiceDefaultFactoryAndObserve).
A regression test for such scenarios is added in
CrossOriginReadBlockingExtensionTest.ProgrammaticContentScriptVsAppCache

For an overview of separate URLLoaderFactories for extensions, see
https://docs.google.com/document/d/1fuBUmLZj1H319jMkK8waUbf5WdQS0N95J_mWXVzbP7A


Avoiding cloberring of the default factory
==========================================

This CL ensures that the initial injection of a content script doesn't
result in cloberring the default URLLoaderFactory, by making sure
RenderFrameHostImpl::MarkInitiatorsAsRequiringSeparateURLLoaderFactory
is capable of pushing down only the new, initiator-specific factories
(without touching the default factory and/or other, untouched
initiator-specific factories).  This also requires ensuring that
RenderFrameHostImpl::CreateInitiatorSpecificURLLoaderFactories takes
as input the origins that need new factories (instead of using all the
origins that the frame needs initiator-specific factories for).


Handling NetworkService crashes
===============================

By having RenderFrameHostImpl::CreateInitiatorSpecificURLLoaderFactories
call CreateNetworkServiceDefaultFactoryAndObserve instead of
CreateNetworkServiceDefaultFactoryInternal, this CL ensures that a
NetworkService crash is detected when initiator-specific (network-bound)
factories are used (and that the crash is detected even if the default
factory is not network-bound).  This makes sure that content scripts in
the test continue to be able to issue network-bound requests after the
crash.

Additionally, introducing
|recreate_default_url_loader_factory_after_network_service_crash_| field
ensures that if the default factory is not network-bound, then it will
not get cloberred when handling a network service crash.  This helps
ensure that the final AppCache request made by the test is successful.


Bug: 846346
Change-Id: Ie53710d17526c140ef8ee65a41de5d3312bfae5b
Reviewed-on: https://chromium-review.googlesource.com/c/1318082
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606352}
[modify] https://crrev.com/4b630b4eb06f30a040d3f9c2a81166f41061ef3f/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
[modify] https://crrev.com/4b630b4eb06f30a040d3f9c2a81166f41061ef3f/chrome/browser/extensions/cross_origin_read_blocking_browsertest.cc
[modify] https://crrev.com/4b630b4eb06f30a040d3f9c2a81166f41061ef3f/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/4b630b4eb06f30a040d3f9c2a81166f41061ef3f/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/4b630b4eb06f30a040d3f9c2a81166f41061ef3f/content/common/url_loader_factory_bundle.mojom
[modify] https://crrev.com/4b630b4eb06f30a040d3f9c2a81166f41061ef3f/content/public/browser/render_frame_host.h
[modify] https://crrev.com/4b630b4eb06f30a040d3f9c2a81166f41061ef3f/content/public/browser/site_isolation_policy.cc
[add] https://crrev.com/4b630b4eb06f30a040d3f9c2a81166f41061ef3f/content/test/data/appcache/logo2.png
[add] https://crrev.com/4b630b4eb06f30a040d3f9c2a81166f41061ef3f/content/test/data/appcache/logo3.png
[modify] https://crrev.com/4b630b4eb06f30a040d3f9c2a81166f41061ef3f/content/test/data/appcache/simple_page.manifest
[modify] https://crrev.com/4b630b4eb06f30a040d3f9c2a81166f41061ef3f/extensions/browser/url_loader_factory_manager.cc
[modify] https://crrev.com/4b630b4eb06f30a040d3f9c2a81166f41061ef3f/extensions/shell/browser/shell_content_browser_client.cc
[modify] https://crrev.com/4b630b4eb06f30a040d3f9c2a81166f41061ef3f/services/network/public/mojom/network_context.mojom
[modify] https://crrev.com/4b630b4eb06f30a040d3f9c2a81166f41061ef3f/services/network/url_loader.cc

Project Member

Comment 19 by bugdroid1@chromium.org, Nov 13

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

commit 89bc625d2b7c8b49370c9bfed33509cb51cf2334
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Tue Nov 13 22:44:47 2018

Fix and re-enable URLLoaderFactories for extensions (WebUI follow-up).

This is a follow-up to https://crrev.com/c/1318082 - the earlier CL
fully fixes and re-enables URLLoaderFactories for extensions, but some
additional hygiene improvements are also possible and made in the
current, WebUI-focused CL.  The current CL adds a new test to help make
sure that some (WebUI-focused) test coverage remains even after
AppCache deprecation finally happens.

The regression test verifies that the default factory wasn't cloberred
by ensuring that one of chrome://resources can still be loaded.

For an overview of separate URLLoaderFactories for extensions, see
https://docs.google.com/document/d/1fuBUmLZj1H319jMkK8waUbf5WdQS0N95J_mWXVzbP7A


Bug: 846346
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: Id1fbfe684514b4cf5e80558df123d78003063632
Reviewed-on: https://chromium-review.googlesource.com/c/1279177
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607779}
[modify] https://crrev.com/89bc625d2b7c8b49370c9bfed33509cb51cf2334/chrome/browser/extensions/cross_origin_read_blocking_browsertest.cc

Project Member

Comment 20 by bugdroid1@chromium.org, Nov 16

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

commit 5f12700c92d5ea851a0cf52cb6f18971dcc0b543
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Fri Nov 16 18:24:09 2018

<webview> content scripts also need a separate URLLoaderFactory.

After r606352, default URLLoaderFactory does not make any CORB
exceptions for extensions and/or apps.  Separate URLLoaderFactories
(relaxing CORB) need to be created for content scripts.

One scenario, where separate URLLoaderFactories should be created, is
content scripts injected into <webview> tag.  Such scripts execute
fetches with |request_initiator| set to the origin of the <webview>
embedder and so permissions of the embedder need to be taken into
account (e.g. a Chrome App might be able to make cross-origin requests).

This CL adds a regression test for this scenario (without product code
changes the new test fails with --enable-features=NetworkService flag)
and fixes the product code to trigger creation of separate
URLLoaderFactory when needed.

For an overview of separate URLLoaderFactories for extensions, see
https://docs.google.com/document/d/1fuBUmLZj1H319jMkK8waUbf5WdQS0N95J_mWXVzbP7A

Bug: 846346
Change-Id: I6ac0f42df7562a01c14cbe6740417b6d94b51810
Reviewed-on: https://chromium-review.googlesource.com/c/1337822
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608859}
[modify] https://crrev.com/5f12700c92d5ea851a0cf52cb6f18971dcc0b543/chrome/browser/extensions/cross_origin_read_blocking_browsertest.cc
[modify] https://crrev.com/5f12700c92d5ea851a0cf52cb6f18971dcc0b543/extensions/browser/guest_view/web_view/web_view_guest.cc
[modify] https://crrev.com/5f12700c92d5ea851a0cf52cb6f18971dcc0b543/extensions/browser/guest_view/web_view/web_view_guest.h
[modify] https://crrev.com/5f12700c92d5ea851a0cf52cb6f18971dcc0b543/extensions/browser/url_loader_factory_manager.cc

Project Member

Comment 21 by bugdroid1@chromium.org, Dec 10

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

commit 2ce10ecdabd79844e6c98271afcaba8692b4cca2
Author: Charlie Reis <creis@chromium.org>
Date: Mon Dec 10 23:55:16 2018

Update histogram expiration for CORB ContentScripts metric.

BUG=904569, 846346

Change-Id: I0911c9fca33feba1e3406346ffa6472c58b8a0dd
Reviewed-on: https://chromium-review.googlesource.com/c/1367107
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615332}
[modify] https://crrev.com/2ce10ecdabd79844e6c98271afcaba8692b4cca2/tools/metrics/histograms/histograms.xml

Cc: alex...@chromium.org nasko@chromium.org jsc...@chromium.org
 Issue 726951  has been merged into this issue.
Project Member

Comment 23 by bugdroid1@chromium.org, Jan 2

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

commit ead46f86bd723d8177071f9cce59d36399d3f7ff
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Wed Jan 02 23:12:32 2019

Prepare to enforce CORB for non-allowlisted extensions.

This CL adds a feature that, when enabled, will stop giving content
scripts a URLLoaderFactory with |is_corb_enabled| set to false
(unless a given extension is on an allowlist).  To minimize churn
and risk, the feature is disabled by default for now.

Tests added in the CL ensure proper behavior when:
1. The feature is disabled (the default, content scripts are able
   to make cross-origin requests)
2. The feature is enabled and the test extension is allowlisted (same
   behavior expectations as above)
3. The feature is enabled and the test extension is *not* allowlisted
   (new, desirable behavior: CORB enforcement for cross-origin requests)

The allowlist is computed as a sum of
1) Extensions.CrossOriginFetchFromContentScript2 Rappor data
   (see kHardcodedPartOfCorbAllowlist introduced by the CL in
   //extensions/browser/url_loader_factory_manager.cc)
2) An optional field trial param (see kBypassCorbAllowlistParamName).

Bug: 846346
Change-Id: I1ab69261cb092b709042a9a963781acc95e8cde6
Reviewed-on: https://chromium-review.googlesource.com/c/1354260
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619529}
[modify] https://crrev.com/ead46f86bd723d8177071f9cce59d36399d3f7ff/base/sha1.h
[modify] https://crrev.com/ead46f86bd723d8177071f9cce59d36399d3f7ff/chrome/browser/extensions/DEPS
[modify] https://crrev.com/ead46f86bd723d8177071f9cce59d36399d3f7ff/chrome/browser/extensions/cross_origin_read_blocking_browsertest.cc
[add] https://crrev.com/ead46f86bd723d8177071f9cce59d36399d3f7ff/chrome/test/data/favicon/title1_with_data_uri_icon.html
[modify] https://crrev.com/ead46f86bd723d8177071f9cce59d36399d3f7ff/extensions/browser/url_loader_factory_manager.cc
[modify] https://crrev.com/ead46f86bd723d8177071f9cce59d36399d3f7ff/extensions/common/extension_features.cc
[modify] https://crrev.com/ead46f86bd723d8177071f9cce59d36399d3f7ff/extensions/common/extension_features.h

Project Member

Comment 24 by bugdroid1@chromium.org, Jan 3

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

commit 4261a7e39f2d01d4651c903d937dd2cb400abb37
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Thu Jan 03 02:02:37 2019

Ability to force CORB extensions allowlist to be empty.

This gives extension authors a way to test whether their extension
is ready to be removed from the allowlist.

Bug: 846346
Change-Id: Ide112dab32076ac2a554115b7ca815a23385050c
Reviewed-on: https://chromium-review.googlesource.com/c/1368783
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619559}
[modify] https://crrev.com/4261a7e39f2d01d4651c903d937dd2cb400abb37/extensions/browser/url_loader_factory_manager.cc
[modify] https://crrev.com/4261a7e39f2d01d4651c903d937dd2cb400abb37/extensions/common/switches.cc
[modify] https://crrev.com/4261a7e39f2d01d4651c903d937dd2cb400abb37/extensions/common/switches.h

Blockedon: 918660
Project Member

Comment 26 by bugdroid1@chromium.org, Jan 9

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

commit dbef86360b1a346a70f941cb98b83be048aaf3b8
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Wed Jan 09 00:14:05 2019

Update docs to account for hardening of content scripts VS CORB

Bug: 846346
Change-Id: Ic0672e9b2af5f39dcca359560683a6036291d555
Reviewed-on: https://chromium-review.googlesource.com/c/1391776
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620955}
[modify] https://crrev.com/dbef86360b1a346a70f941cb98b83be048aaf3b8/chrome/common/extensions/docs/templates/articles/content_scripts.html
[modify] https://crrev.com/dbef86360b1a346a70f941cb98b83be048aaf3b8/chrome/common/extensions/docs/templates/articles/xhr.html

r620935 should have been associated with this bug:

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

commit 763a3b389cf197921f1718bc7db7d6ede8e3024b
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Tue Jan 08 23:40:16 2019

Enable BypassCorbOnlyForExtensionsAllowlist feature.

Bug:  918660 
Change-Id: Ieb2c3434dc7ca8ec4d20f6e1daefa67e2202cd81
Reviewed-on: https://chromium-review.googlesource.com/c/1398803
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620935}
[modify] https://crrev.com/763a3b389cf197921f1718bc7db7d6ede8e3024b/extensions/common/extension_features.cc
r620935 from #c27 initially landed in 73.0.3666.0

It seems to work as expected:
- When launching Chrome with:
  --force-empty-corb-allowlist --enable-features=NetworkService
  the affected extensions stop working
  (and the CORB error shows up in DevTools console)

- When launching Chrome with just --enable-features=NetworkService
  the affected extensions continue to work (at least there are no
  big error messages as with --force-empty-corb-allowlist)

Blockedon: 920638
Let's track issue 920638 as a soft / dotted-line dependency (it is not required for enforcing CORB for compromised renderers, but CORB-vs-CORS inconsistency for content scripts is definitely undesirable).
Project Member

Comment 31 by bugdroid1@chromium.org, Jan 11

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

commit 15515f51ffbc819cca05aa4109390c138e3b1e66
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Fri Jan 11 23:54:36 2019

Remove apps and themes from kHardcodedPartOfCorbAllowlist.

CORB doesn't impact:
- Chrome Apps (which are explicitly excluded by
  DoContentScriptsDependOnRelaxedCorb function in
  //extensions/browser/url_loader_factory_manager.cc)
- Chrome Hosted Apps and Chrome Themes (which are both unable to perform
  cross-origin requests from content scripts)

Nevertheless, entries in the kHardcodedPartOfCorbAllowlist accidentally
included entries which corresponded to apps and/or themes.  This is
somewhat expected given the design of RAPPOR data gathering pipeline
which (in an attempt to protect private of Chrome users) noisifies the
data and can return false positives (this is especially expected in
cases where the set of all expected values is now known - we know
extensions in the Chrome Web Store, but we can't account for extension
ids corresponding to extensions under development or not published
through the public store).  See also:
- https://www.chromium.org/developers/design-documents/rappor
- (internal, Google-only) go/rappor101 which describes this
  problem in the "3.2 Limitations" section.

Because of the above, we used Chrome Web Store metadata to classify
items on kHardcodedPartOfCorbAllowlist as either extension, app,
platform_app or theme and removed all non-extension items from the
allowlist.

Bug: 846346
Change-Id: I45fd9437f3dae9b57d6f5d1ae79f389b5a528bbc
Reviewed-on: https://chromium-review.googlesource.com/c/1407604
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622227}
[modify] https://crrev.com/15515f51ffbc819cca05aa4109390c138e3b1e66/extensions/browser/url_loader_factory_manager.cc

Comment 32 by lukasza@chromium.org, Today (13 hours ago)

Blockedon: 924333

Sign in to add a comment