Remove blink::Platform::CreateDefaultURLLoaderFactory and its callers |
|||||||||
Issue descriptionThe URLLoaderFactory created by blink::Platform::CreateDefaultURLLoaderFactory is not associated with a specific origin. This is undesirable, because it makes it forces such URLLoaderFactory to trust the network::ResourceRequest::request_initiator provided by the factory (also note that the NetworkService doesn't have knowledge about origin locks that content::ChildProcessSecurityPolicyImpl might have applies to the given renderer process - associating the URLLoaderFactory with a given origin has to happen in the browser process, when creating the factory). Let's use this bug to track removal of blink::Platform::CreateDefaultURLLoaderFactory.
,
Oct 3
I've just found issue 751425, but I assume that this is a separate bug.
,
Oct 3
- WIP CL @ https://crrev.com/c/1259824 tries to remove the caller in WebViewPlugin::WebViewHelper::CreateURLLoaderFactory - WIP CL @ https://crrev.com/c/1259613 changes the name of the method to CreateDeprecatedDefaultURLLoaderFactory (not sure if it is worth landing this CL, but it is helpful for showing how many callers+overrides there are)
,
Nov 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/905bf17717a301cb5c8c9b1ac417b40d0a97354e commit 905bf17717a301cb5c8c9b1ac417b40d0a97354e Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Fri Nov 16 02:14:37 2018 Return frame/origin-bound URLLoaderFactory from WebViewPlugin/Helper. This CL modifies WebViewPlugin::WebViewHelper::CreateURLLoaderFactory method so that it delegates creation of URLLoaderFactory to the frame that hosts the plugin. Bug: 891872 Change-Id: I002503730f98adbced92d61c7d74eb53f4865e98 Reviewed-on: https://chromium-review.googlesource.com/c/1259824 Reviewed-by: Tommy Li <tommycli@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Cr-Commit-Position: refs/heads/master@{#608632} [modify] https://crrev.com/905bf17717a301cb5c8c9b1ac417b40d0a97354e/components/plugins/renderer/webview_plugin.cc [modify] https://crrev.com/905bf17717a301cb5c8c9b1ac417b40d0a97354e/third_party/blink/public/platform/platform.h
,
Nov 27
,
Nov 27
,
Nov 29
FWIW, I've tried enumerating all the callers and implementations in a doc here: https://docs.google.com/document/d/115wW7v62cXQxdekdEpptmsDh9CeBy7xqyNJ6BAP_aF8 Feel free to chime-in and help chip-away at this bug :-)
,
Nov 30
Just checking: is this for the network service path only? afaik the default one is only used in some edge conditions there. for the non-network service path, we use the global one. are you planning on changing that code path to send one per frame commit like the NS path (or perhaps ignoring that case for now?)?
,
Nov 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9ff9445e99156f542f1c512515b9707b50169137 commit 9ff9445e99156f542f1c512515b9707b50169137 Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Fri Nov 30 19:37:17 2018 Use frame/origin-bound URLLoaderFactory in PrepareFrameAndViewForPrint. This CL modifies PrepareFrameAndViewForPrint::CreateURLLoaderFactory method so that it delegates creation of URLLoaderFactory to the frame being printed. Bug: 891872 Change-Id: I42a87f908e18b751bf8dd1d192b4fa2e34141d09 Reviewed-on: https://chromium-review.googlesource.com/c/1356022 Reviewed-by: Wei Li <weili@chromium.org> Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Cr-Commit-Position: refs/heads/master@{#612728} [modify] https://crrev.com/9ff9445e99156f542f1c512515b9707b50169137/components/printing/renderer/print_render_frame_helper.cc
,
Nov 30
RE: jam@: #c8: RE: Just checking: is this for the network service path only? I am not sure if I understand the question (and how Blink works with/without NetworkService) to answer the above. URLLoaderFactory that is not associated with a specific origin/frame is undesirable mostly (only?) in NetworkService path, because in NetworkService URLLoaderFactory should be able to get |request_initiator_origin_lock| information from the browser (to avoid trusting the renderer here. With that perspective, presence of a global, non-frame/origin-associated blink::Platform::CreateDefaultURLLoaderFactory blocks issue 871827. Yes. Thisafaik the default one is only used in some edge conditions there. RE: for the non-network service path, we use the global one. are you planning on changing that code path to send one per frame commit like the NS path (or perhaps ignoring that case for now?)? I plan to ignore that case.
,
Dec 3
Loading bug triage: let me make this assigned since lukasza seems to have worked on this.
,
Dec 3
+falken@ for WorkerShadowPage
+iclelland@ and fs@ for sandboxed subresource fetches made for SVG
I wonder if the remaining callers in the product code might be satisfied if the URL requests were made on behalf of an opaque/unique/sandboxed origin.
I see that SVGImage::DataChanged contains the following code:
LocalFrame* frame = nullptr;
{
TRACE_EVENT0("blink", "SVGImage::dataChanged::createFrame");
DCHECK(!frame_client_);
frame_client_ = MakeGarbageCollected<SVGImageLocalFrameClient>(this);
frame = LocalFrame::Create(frame_client_, *page, nullptr);
frame->SetView(LocalFrameView::Create(*frame));
frame->Init();
}
FrameLoader& loader = frame->Loader();
loader.ForceSandboxFlags(kSandboxAll); // <--- *** SANDBOXING FORCED HERE ***
I also strongly suspect that WebPagePopupImpl, InspectorOverlayAgent, ValidationMessageOverlayDelegate either don't make requests at all (if so, https://crrev.com/c/1359051 would address them), or would be satisfied if the requests were made on behalf of an opaque/unique/sandboxed origin.
I am not so sure about WorkerShadowPage (whether it is okay to use opaque origin for subresource fetches OR if it might be possible to use a URLLoaderFactory associated with a specific, non-opaque origin instead)...
,
Dec 4
We're trying to remove WorkerShadowPage completely. Let me look into why this callsite is needed.
,
Dec 5
As I just wrote on the doc, the callsite in WorkerShadowPage is only needed in the non-ServiceWorkerServicification case, as far as I can tell. I'm wondering if blink::Platform::CreateDefaultURLLoaderFactoryBundle has the same problem as blink::Platform::CreateDefaultURLLoaderFactory? I can make service worker behave like shared worker, which would remove the CreateDefaultURLLoaderFactory call, but at the expense of adding CreateDefaultURLLoaderFactoryBundle. See: https://cs.chromium.org/chromium/src/content/renderer/shared_worker/embedded_shared_worker_stub.cc?l=270&rcl=66ed852e522a88a88ec863099ecf1565bcf1ab9e
,
Dec 5
CreateDefaultURLLoaderFactoryBundle is also called for frames (the shared worker code largely copy/pasted that): https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?l=6532&rcl=9bc27b1de4b78e524936c7e4327128bc0bc18843 If we don't care about the non-ServiceWorkerServicification path (since you don't care about the non-NetworkService path, if I'm reading c#10 correctly), another option is to put CHECK(!ServiceWorkerServicification) at the callsite, and then remove it once the non-ServiceWorkerServicification code is removed.
,
Dec 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/95065abbd18c9be7661f8f64929ef17624090f64 commit 95065abbd18c9be7661f8f64929ef17624090f64 Author: Matt Falkenhagen <falken@chromium.org> Date: Wed Dec 05 03:03:16 2018 service worker: Use fake network provider in WebEmbeddedWorkerImplTest. WebEmbeddedWorkerImplTest was using a null ServiceWorkerNetworkProvider. This CL adds a fake provider which implements CreateURLLoader() to return non-null, which matches the ServiceWorkerServicification behavior. This removes callsites to Platform::CreateDefaultURLLoaderFactory() via WorkerShadowPage::CreateURLLoaderFactory(), which is being removed at bug 891872. Bug: 891872 Change-Id: I8592131fea83429b39b9744727a4bdd493734be0 Reviewed-on: https://chromium-review.googlesource.com/c/1361754 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Commit-Queue: Matt Falkenhagen <falken@chromium.org> Cr-Commit-Position: refs/heads/master@{#613836} [modify] https://crrev.com/95065abbd18c9be7661f8f64929ef17624090f64/third_party/blink/renderer/modules/service_worker/web_embedded_worker_impl_test.cc
,
Dec 5
falken@: Thank you very much for looking into WorkerShadowPage and how it relates to this bug - much appreciated! RE: I'm wondering if blink::Platform::CreateDefaultURLLoaderFactoryBundle has the same problem as blink::Platform::CreateDefaultURLLoaderFactory? I am not sure what you mean by blink::Platform::CreateDefaultURLLoaderFactoryBundle. I could not find that method in Chromium code search. I assume that you mean content::RendererBlinkPlatformImpl::CreateDefaultURLLoaderFactoryBundle. blink::Platform::CreateDefaultURLLoaderFactory is problematic, because the URLLoaderFactory it returns is *not* associated with a specific frame and/or origin. This problem is also present in content::RendererBlinkPlatformImpl::CreateDefaultURLLoaderFactoryBundle and RendererBlinkPlatformImpl::CreateNetworkURLLoaderFactory - all of these methods eventually result in a mojo IPC that asks RenderProcessHostImpl for a URLLoaderFactory (via blink::mojom::AssociatedInterfaceProvider). It is preferable for renderer/Blink features to obtain URLLoaderFactory via a frame or worker context instead (e.g. by calling local_frame->Client()->CreateURLLoaderFactory() instead; see also r612728 and r608632 above). Obtaining URLLoaderFactory in such way means that the browser process can associate the factory with a specific frame and/or origin (i.e. the browser process creates such factory via RenderFrameHostImpl::CreateNetworkServiceDefaultFactoryInternal called at RenderFrameHostImpl::CommitNavigation time) and this in turn means that we don't need to trust |request_initiator| coming from the renderer (see issue 871827). RE: WorkerShadowPage is only needed in the non-ServiceWorkerServicification case, as far as I can tell. I am not sure what you mean by ServiceWorkerServicification. I assume that you mean --enable-features=NetworkService. If this assumption is correct, then I think the problematic case is hit even with --enable-features=NetworkService. For example, patch-in https://crrev.com/c/1363638 (at ToT / r613994 / after the r613836 from #c18 above) and try running external/wpt/service-workers/service-worker/referer.https.html with --enable-features=NetworkService - the test will crash, because of the following callstack: #2 content::RendererBlinkPlatformImpl::CreateDefaultURLLoaderFactoryBundle() #3 content::RenderFrameImpl::SetupLoaderFactoryBundle() #4 content::RenderFrameImpl::GetLoaderFactoryBundle() #5 content::RenderFrameImpl::BuildServiceWorkerNetworkProviderForNavigation() #6 content::RenderFrameImpl::DidCreateDocumentLoader() #7 blink::LocalFrameClientImpl::CreateDocumentLoader() #8 blink::FrameLoader::CreateDocumentLoader() #9 blink::FrameLoader::Init() #10 blink::LocalFrame::Init() #11 blink::WebLocalFrameImpl::InitializeCoreFrame() #12 blink::WebLocalFrameImpl::CreateMainFrame() #13 content::RenderFrameImpl::CreateMainFrame() #14 content::RenderViewImpl::Initialize() #15 content::RenderViewImpl::Create() #16 content::RenderThreadImpl::CreateView() I do note that the callstack above doesn't go through blink::Platform::CreateDefaultURLLoaderFactory, but since this is basically the same problem, I think opening a separate bug is not desirable / necessary.
,
Dec 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/363b61e5a8dfd4949ffa369f7e0243ac0e6afb04 commit 363b61e5a8dfd4949ffa369f7e0243ac0e6afb04 Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Wed Dec 05 23:22:29 2018 Make EmptyLocalFrameClient disallow network requests by default. As we attempt to limit the uses of blink::Platform’s CreateDefaultURLLoaderFactory, it makes sense to avoid using it from EmptyLocalFrameClient - this CL disallows the default EmptyLocalFrameClient from making network requests. This CL assumes that WebPagePopupImpl, InspectorOverlayAgent, ValidationMessageOverlayDelegate should not make any network requests. After this CL, these classes will continue using EmptyLocalFrameClient and if they attempt to make fetch a subresource over network, then they will hit the new NOTREACHED (and probably crash shortly after when dereferencing the returned, null URLLoaderFactory). This CL preserves the behavior of SVGImage::SVGImageLocalFrameClient, DummyPageHolder (test code) and PingLocalFrameClient (test code) to continue calling blink::Platform's CreateDefaultURLLoaderFactory. Bug: 891872 Change-Id: I5132fcc66929ff14b25ce2ce636011e1463244f4 Reviewed-on: https://chromium-review.googlesource.com/c/1359051 Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Reviewed-by: Kent Tamura <tkent@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Cr-Commit-Position: refs/heads/master@{#614168} [modify] https://crrev.com/363b61e5a8dfd4949ffa369f7e0243ac0e6afb04/third_party/blink/renderer/core/loader/empty_clients.h [modify] https://crrev.com/363b61e5a8dfd4949ffa369f7e0243ac0e6afb04/third_party/blink/renderer/core/loader/ping_loader_test.cc [modify] https://crrev.com/363b61e5a8dfd4949ffa369f7e0243ac0e6afb04/third_party/blink/renderer/core/svg/graphics/svg_image.cc [modify] https://crrev.com/363b61e5a8dfd4949ffa369f7e0243ac0e6afb04/third_party/blink/renderer/core/testing/dummy_page_holder.cc
,
Dec 6
c#17: > I am not sure what you mean by blink::Platform::CreateDefaultURLLoaderFactoryBundle. I could not find that method in Chromium code search. I assume that you mean content::RendererBlinkPlatformImpl::CreateDefaultURLLoaderFactoryBundle. Sorry yes, that's right. > I am not sure what you mean by ServiceWorkerServicification. I assume that you mean --enable-features=NetworkService. It's --enable-features=ServiceWorkerServicification. But when NetworkService is enabled, it effectively enables SWS as well. It's a subset of NS that can be shipped separately. > For example, patch-in https://crrev.com/c/1363638 (at ToT / r613994 / after the r613836 from #c18 above) Yes I see, but even if you patch that in and remove the callsite from SWNetworkProviderForNavigation code (just use nullptr for |fallback_factory|), we still call it in CommitNavigation: #7 0x7ffb2a00914f content::RendererBlinkPlatformImpl::CreateDefaultURLLoaderFactoryBundle() #8 0x7ffb29fb8547 content::RenderFrameImpl::SetupLoaderFactoryBundle() #9 0x7ffb29fb776a content::RenderFrameImpl::CommitNavigation() That's why I was wondering whether it's OK to call CreateDefaultURLLoaderFactoryBundle() for service worker startup. I understand now it's not, and frames are doing the same thing. I can look more into removing it from service workers, but we'll need a solution to remove it from frames as well.
,
Dec 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3f2252e6d6622f13d03f20e613fcaa13c34cc5b4 commit 3f2252e6d6622f13d03f20e613fcaa13c34cc5b4 Author: Łukasz Anforowicz <lukasza@chromium.org> Date: Sat Dec 08 00:13:51 2018 Revert "Use frame/origin-bound URLLoaderFactory in PrepareFrameAndViewForPrint." This reverts commit 9ff9445e99156f542f1c512515b9707b50169137. Reason for revert: Main suspect for causing https://crbug.com/911250 Original change's description: > Use frame/origin-bound URLLoaderFactory in PrepareFrameAndViewForPrint. > > This CL modifies PrepareFrameAndViewForPrint::CreateURLLoaderFactory > method so that it delegates creation of URLLoaderFactory to the frame > being printed. > > Bug: 891872 > Change-Id: I42a87f908e18b751bf8dd1d192b4fa2e34141d09 > Reviewed-on: https://chromium-review.googlesource.com/c/1356022 > Reviewed-by: Wei Li <weili@chromium.org> > Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> > Cr-Commit-Position: refs/heads/master@{#612728} TBR=weili@chromium.org,lukasza@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 891872 Change-Id: I7bfac93ff70c09b8ef22ca4225b2216076e7fc1b Reviewed-on: https://chromium-review.googlesource.com/c/1368781 Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org> Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Cr-Commit-Position: refs/heads/master@{#614878} [modify] https://crrev.com/3f2252e6d6622f13d03f20e613fcaa13c34cc5b4/components/printing/renderer/print_render_frame_helper.cc
,
Dec 10
,
Dec 10
,
Dec 13
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9f2cede3f8cdd694c865fc394a9af3edc672b10d commit 9f2cede3f8cdd694c865fc394a9af3edc672b10d Author: Kinuko Yasuda <kinuko@chromium.org> Date: Thu Dec 13 11:31:36 2018 Stop using default_factory for AppCache and reduce RBPI::CreateDefaultURLLoaderFactoryBundle usage * Stop using URLLoaderFactoryBundle::default_factory for AppCache, this over-generalization has been causing a lot of confusions. * Deprecate URLLoaderFactoryBundle::default_network_factory, which is no longer needed after removing the default_factory == AppCache hack. * This change also allows us to remove more RendererBlinkPlatformImpl:: CreateDefaultURLLoaderFactoryBundle callsites for most of NetworkService-enabled cases, which was needed only when default_factory was dropped (just to skip AppCache, which is no longer needed), or for initial empty document cases (this is still needed). Bug: 891872 Change-Id: I9b58097430b76d96d96864043b63553e88e3066c Reviewed-on: https://chromium-review.googlesource.com/c/1371310 Commit-Queue: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org> Reviewed-by: Kenichi Ishibashi <bashi@chromium.org> Reviewed-by: Matt Falkenhagen <falken@chromium.org> Cr-Commit-Position: refs/heads/master@{#616269} [modify] https://crrev.com/9f2cede3f8cdd694c865fc394a9af3edc672b10d/content/browser/appcache/appcache_request_handler.cc [modify] https://crrev.com/9f2cede3f8cdd694c865fc394a9af3edc672b10d/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/9f2cede3f8cdd694c865fc394a9af3edc672b10d/content/browser/service_worker/embedded_worker_instance.cc [modify] https://crrev.com/9f2cede3f8cdd694c865fc394a9af3edc672b10d/content/browser/worker_host/shared_worker_host.cc [modify] https://crrev.com/9f2cede3f8cdd694c865fc394a9af3edc672b10d/content/browser/worker_host/shared_worker_host_unittest.cc [modify] https://crrev.com/9f2cede3f8cdd694c865fc394a9af3edc672b10d/content/common/navigation_subresource_loader_params.cc [modify] https://crrev.com/9f2cede3f8cdd694c865fc394a9af3edc672b10d/content/common/navigation_subresource_loader_params.h [modify] https://crrev.com/9f2cede3f8cdd694c865fc394a9af3edc672b10d/content/common/url_loader_factory_bundle.cc [modify] https://crrev.com/9f2cede3f8cdd694c865fc394a9af3edc672b10d/content/common/url_loader_factory_bundle.h [modify] https://crrev.com/9f2cede3f8cdd694c865fc394a9af3edc672b10d/content/common/url_loader_factory_bundle.mojom [modify] https://crrev.com/9f2cede3f8cdd694c865fc394a9af3edc672b10d/content/common/url_loader_factory_bundle_struct_traits.cc [modify] https://crrev.com/9f2cede3f8cdd694c865fc394a9af3edc672b10d/content/common/url_loader_factory_bundle_struct_traits.h [modify] https://crrev.com/9f2cede3f8cdd694c865fc394a9af3edc672b10d/content/renderer/loader/child_url_loader_factory_bundle.cc [modify] https://crrev.com/9f2cede3f8cdd694c865fc394a9af3edc672b10d/content/renderer/loader/child_url_loader_factory_bundle.h [modify] https://crrev.com/9f2cede3f8cdd694c865fc394a9af3edc672b10d/content/renderer/loader/tracked_child_url_loader_factory_bundle.cc [modify] https://crrev.com/9f2cede3f8cdd694c865fc394a9af3edc672b10d/content/renderer/loader/tracked_child_url_loader_factory_bundle.h [modify] https://crrev.com/9f2cede3f8cdd694c865fc394a9af3edc672b10d/content/renderer/render_frame_impl.cc [modify] https://crrev.com/9f2cede3f8cdd694c865fc394a9af3edc672b10d/content/renderer/shared_worker/embedded_shared_worker_stub.cc
,
Jan 3
,
Jan 3
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by lukasza@chromium.org
, Oct 3