Issue metadata
Sign in to add a comment
|
Cleanup request initiator. |
||||||||||||||||||||||||
Issue descriptionWe're currently setting a request's initiator in RenderFrameImpl, which is too high up the stack to reliably get the correct origin information for the requestor, and we're defaulting to a unique origin which makes it tough to distinguish between user-initiated requests and requests initiated from unique origins.
,
Jul 14 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/47673cd03085ab0879683e2a782a4eb3a4eecb4b commit 47673cd03085ab0879683e2a782a4eb3a4eecb4b Author: mkwst <mkwst@chromium.org> Date: Thu Jul 14 09:48:58 2016 Set 'ResourceRequest::requestorOrigin' in Blink for more request types. This patch ensures that we set the correct request initiator for all requests that run through Blink's ResourceFetcher (which is basically everything except navigation) by extending 'FrameFetchContext::setFirstPartyForCookies' into a more general-purpose setter that also takes care of 'requestorOrigin', applying these setting to requests sent through 'PingLoader' and 'BeaconLoader'. This doesn't have any web-visible effect, but it will allow us to remove the requestor origin setting from 'content::RenderFrameImpl' in the somewhat near future. As a drive-by, this fixes 'FrameFetchContext::setFirstPartyForCookies's current implementation, which was providing the wrong first-party for nested and cross-origin requests sent through 'PingLoader' and 'BeaconLoader'. BUG= 625969 R=jochen@chromium.org Review-Url: https://codereview.chromium.org/2147473002 Cr-Commit-Position: refs/heads/master@{#405459} [modify] https://crrev.com/47673cd03085ab0879683e2a782a4eb3a4eecb4b/content/browser/loader/resource_dispatcher_host_browsertest.cc [modify] https://crrev.com/47673cd03085ab0879683e2a782a4eb3a4eecb4b/content/renderer/render_frame_impl.cc [add] https://crrev.com/47673cd03085ab0879683e2a782a4eb3a4eecb4b/content/test/data/nested_page_with_subresources.html [add] https://crrev.com/47673cd03085ab0879683e2a782a4eb3a4eecb4b/content/test/data/page_with_subresources.html [modify] https://crrev.com/47673cd03085ab0879683e2a782a4eb3a4eecb4b/third_party/WebKit/Source/core/fetch/FetchContext.cpp [modify] https://crrev.com/47673cd03085ab0879683e2a782a4eb3a4eecb4b/third_party/WebKit/Source/core/fetch/FetchContext.h [modify] https://crrev.com/47673cd03085ab0879683e2a782a4eb3a4eecb4b/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp [modify] https://crrev.com/47673cd03085ab0879683e2a782a4eb3a4eecb4b/third_party/WebKit/Source/core/loader/BeaconLoader.cpp [modify] https://crrev.com/47673cd03085ab0879683e2a782a4eb3a4eecb4b/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp [modify] https://crrev.com/47673cd03085ab0879683e2a782a4eb3a4eecb4b/third_party/WebKit/Source/core/loader/FrameFetchContext.h [modify] https://crrev.com/47673cd03085ab0879683e2a782a4eb3a4eecb4b/third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp [modify] https://crrev.com/47673cd03085ab0879683e2a782a4eb3a4eecb4b/third_party/WebKit/Source/core/loader/FrameLoadRequest.cpp [modify] https://crrev.com/47673cd03085ab0879683e2a782a4eb3a4eecb4b/third_party/WebKit/Source/core/loader/PingLoader.cpp
,
Sep 9 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/65299f214fad8f609c5872bcdd5f4ea82ec29713 commit 65299f214fad8f609c5872bcdd5f4ea82ec29713 Author: mek <mek@chromium.org> Date: Fri Sep 09 21:11:13 2016 Correctly set requestor origin for worker initiated requests. ResourceRequest.requestTorOrigin() can never be null (since it is initialized to a unique origin rather than null). Because of that the check in FrameFetchContext::populateRequestData meant that this code would never actually set the requestor origin of a request. For requests from frames the code in RenderFrameImpl::willSendRequest would then replace the origin with the correct one anyway, but no such code exists for shared/service workers. BUG= 645281 , 625969 Review-Url: https://codereview.chromium.org/2323143002 Cr-Commit-Position: refs/heads/master@{#417712} [modify] https://crrev.com/65299f214fad8f609c5872bcdd5f4ea82ec29713/third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-basics.html [modify] https://crrev.com/65299f214fad8f609c5872bcdd5f4ea82ec29713/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/foreign-fetch-worker.js [modify] https://crrev.com/65299f214fad8f609c5872bcdd5f4ea82ec29713/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp [modify] https://crrev.com/65299f214fad8f609c5872bcdd5f4ea82ec29713/third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp
,
Sep 12 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3fa4d0dbbd3f3839ec00a64e5c711bc0f3658416 commit 3fa4d0dbbd3f3839ec00a64e5c711bc0f3658416 Author: Joshua Bell <jsbell@chromium.org> Date: Mon Sep 12 17:16:38 2016 Correctly set requestor origin for worker initiated requests. ResourceRequest.requestTorOrigin() can never be null (since it is initialized to a unique origin rather than null). Because of that the check in FrameFetchContext::populateRequestData meant that this code would never actually set the requestor origin of a request. For requests from frames the code in RenderFrameImpl::willSendRequest would then replace the origin with the correct one anyway, but no such code exists for shared/service workers. BUG= 645281 , 625969 Review-Url: https://codereview.chromium.org/2323143002 Cr-Commit-Position: refs/heads/master@{#417712} (cherry picked from commit 65299f214fad8f609c5872bcdd5f4ea82ec29713) Review URL: https://codereview.chromium.org/2332023002 . Cr-Commit-Position: refs/branch-heads/2840@{#303} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/3fa4d0dbbd3f3839ec00a64e5c711bc0f3658416/third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-basics.html [modify] https://crrev.com/3fa4d0dbbd3f3839ec00a64e5c711bc0f3658416/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/foreign-fetch-worker.js [modify] https://crrev.com/3fa4d0dbbd3f3839ec00a64e5c711bc0f3658416/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp [modify] https://crrev.com/3fa4d0dbbd3f3839ec00a64e5c711bc0f3658416/third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp
,
Sep 27 2016
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3fa4d0dbbd3f3839ec00a64e5c711bc0f3658416 commit 3fa4d0dbbd3f3839ec00a64e5c711bc0f3658416 Author: Joshua Bell <jsbell@chromium.org> Date: Mon Sep 12 17:16:38 2016 Correctly set requestor origin for worker initiated requests. ResourceRequest.requestTorOrigin() can never be null (since it is initialized to a unique origin rather than null). Because of that the check in FrameFetchContext::populateRequestData meant that this code would never actually set the requestor origin of a request. For requests from frames the code in RenderFrameImpl::willSendRequest would then replace the origin with the correct one anyway, but no such code exists for shared/service workers. BUG= 645281 , 625969 Review-Url: https://codereview.chromium.org/2323143002 Cr-Commit-Position: refs/heads/master@{#417712} (cherry picked from commit 65299f214fad8f609c5872bcdd5f4ea82ec29713) Review URL: https://codereview.chromium.org/2332023002 . Cr-Commit-Position: refs/branch-heads/2840@{#303} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/3fa4d0dbbd3f3839ec00a64e5c711bc0f3658416/third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-basics.html [modify] https://crrev.com/3fa4d0dbbd3f3839ec00a64e5c711bc0f3658416/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/foreign-fetch-worker.js [modify] https://crrev.com/3fa4d0dbbd3f3839ec00a64e5c711bc0f3658416/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp [modify] https://crrev.com/3fa4d0dbbd3f3839ec00a64e5c711bc0f3658416/third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp
,
Nov 18 2016
,
Jun 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/74a7fb2fa4307c69812945586b39018ae4f231b2 commit 74a7fb2fa4307c69812945586b39018ae4f231b2 Author: kinuko <kinuko@chromium.org> Date: Fri Jun 09 03:50:22 2017 Remove dup'ed code for RequestorOrigin and FirstPartyCookie - Remove RequestorOrigin and FirstPartyCookie code in RenderFrameImpl (except for PlzNavigate's main frame resource case) - Most of the code now lives in FrameFetchContext - Also fix TODO to initialize ResourceRequest::RequestorOrigin with null Some implementation notes: Before this change, the logic was like: 1. When ResourceFetcher populates an initial resource request FetchContext::SetFirstPartyCookieAndRequestorOrigin was called in PopulateResourceRequest, and this used to do: if (!GetDocument()) return; if (request.FirstPartyForCookies().IsNull()) { request.SetFirstPartyForCookies( GetDocument() ? GetDocument()->FirstPartyForCookies() // [A] : SecurityOrigin::UrlWithUniqueSecurityOrigin()); // [B] } if (request.GetFrameType() == WebURLRequest::kFrameTypeNone && request.RequestorOrigin()->IsUnique()) { request.SetRequestorOrigin(GetDocument()->IsSandboxed(kSandboxOrigin) ? SecurityOrigin::Create(document_->Url()) // [a] : document_->GetSecurityOrigin()); // [b] } 2. After that, FrameFetchContext::WillSendRequest does: if (request.FirstPartyForCookies().IsEmpty()) { if (request.GetFrameType() == blink::WebURLRequest::kFrameTypeTopLevel) request.SetFirstPartyForCookies(request.Url()); // [C] else request.SetFirstPartyForCookies( frame_->GetDocument().FirstPartyForCookies()); // [D] } WebDocument frame_document = frame_->GetDocument(); if (request.RequestorOrigin().IsUnique() && !frame_document.GetSecurityOrigin().IsUnique()) { request.SetRequestorOrigin(frame_document.GetSecurityOrigin()); // [c] } This logic is also called on redirects. Note that: - [B] case is invalid as we return null-document case - [C],[D],[c] happens only if document is null, and for initial top-level frame request case this is always the case - Therefore: [A][C][D] are the valid cases for first-party-cookie, and [a][b][c] are for requestor-origin After this change, FrameFetchContext::WillSendRequest does: if (request.FirstPartyForCookies().IsNull()) { if (request.GetFrameType() == WebURLRequest::kFrameTypeTopLevel) { request.SetFirstPartyForCookies(request.Url()); // == [C] (document is null) } else { Document* document = GetDocument() ? GetDocument() : GetFrame()->GetDocument(); request.SetFirstPartyForCookies(document->FirstPartyForCookies()); // == [A][D] } } if (!request.RequestorOrigin()) { if (request.GetFrameType() == WebURLRequest::kFrameTypeNone) { Document* document = GetDocument(); request.SetRequestorOrigin(document->IsSandboxed(kSandboxOrigin) ? SecurityOrigin::Create(document->Url()) // [a] : document->GetSecurityOrigin()); // [b] } else { request.SetRequestorOrigin( GetFrame()->GetDocument()->GetSecurityOrigin()); // [c] } } BUG= 671533 , 625969 Review-Url: https://codereview.chromium.org/2918653004 Cr-Commit-Position: refs/heads/master@{#478191} [modify] https://crrev.com/74a7fb2fa4307c69812945586b39018ae4f231b2/content/renderer/fetchers/resource_fetcher_impl.cc [modify] https://crrev.com/74a7fb2fa4307c69812945586b39018ae4f231b2/content/renderer/render_frame_impl.cc [modify] https://crrev.com/74a7fb2fa4307c69812945586b39018ae4f231b2/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp [modify] https://crrev.com/74a7fb2fa4307c69812945586b39018ae4f231b2/third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp [modify] https://crrev.com/74a7fb2fa4307c69812945586b39018ae4f231b2/third_party/WebKit/Source/core/loader/WorkerFetchContext.cpp [modify] https://crrev.com/74a7fb2fa4307c69812945586b39018ae4f231b2/third_party/WebKit/Source/platform/loader/fetch/ResourceRequest.cpp [modify] https://crrev.com/74a7fb2fa4307c69812945586b39018ae4f231b2/third_party/WebKit/Source/platform/loader/fetch/ResourceRequestTest.cpp
,
Oct 20 2017
I am temporary working on it to make issue 648608 working. I have WIP CL.
,
Nov 10 2017
,
Dec 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/224aa617bd5eeda7c35fc96f42296879cd3fb671 commit 224aa617bd5eeda7c35fc96f42296879cd3fb671 Author: arthursonzogni <arthursonzogni@chromium.org> Date: Thu Dec 07 15:31:51 2017 Make navigation's initiator correct. Before this patch, the navigation initiator/requestor was set too high in the stack to be reliable. There were pieces of code that were trying to guess what the initiator was (and it was wrong sometimes.) This CL wires this data from where is it available to where is is needed. We say that a request has no initiator when the navigation was not initiated from a document, but from the browser (omnibox, bookmarks, ...). Before this CL and without PlzNavigate, if there was no initiator, the initiator was set to request's url. Now, it doesn't. PlzNavigate and non-PlzNavigate initiators are now the same. It fixes a security issue with Cookies ( issue 648608 ). A regression test is added Bug: 648608 , 625969 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo;master.tryserver.chromium.linux:linux_site_isolation Change-Id: Ic23f20590a7cf422e08eefe354ff4057f016a987 Reviewed-on: https://chromium-review.googlesource.com/730729 Reviewed-by: Camille Lamy <clamy@chromium.org> Reviewed-by: Mike West <mkwst@chromium.org> Reviewed-by: Matt Menke <mmenke@chromium.org> Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Cr-Commit-Position: refs/heads/master@{#522434} [modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/chrome/renderer/net/net_error_helper.cc [modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/chrome/test/data/extensions/api_test/webrequest/framework.js [modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/content/browser/loader/resource_dispatcher_host_browsertest.cc [modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/content/browser/loader/resource_dispatcher_host_impl.cc [modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/content/renderer/render_frame_impl.cc [modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/testing/buildbot/filters/site-per-process.content_browsertests.filter [modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/third_party/WebKit/Source/core/frame/LocalFrame.cpp [modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/third_party/WebKit/Source/core/frame/WebLocalFrameImpl.cpp [modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp [modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/third_party/WebKit/Source/core/loader/FrameFetchContext.h [modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/third_party/WebKit/Source/core/loader/FrameLoadRequest.cpp [modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/third_party/WebKit/Source/core/loader/NavigationScheduler.cpp [modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/third_party/WebKit/Source/core/page/CreateWindow.cpp [modify] https://crrev.com/224aa617bd5eeda7c35fc96f42296879cd3fb671/third_party/WebKit/Source/core/page/DragController.cpp
,
Dec 7 2017
I am giving the ownership back to @mkwst. I don't know if this issue is fully fixed by my last patch or if there are any additional steps to do.
,
Jan 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ec09d1903c3cfadbaf6953afd1bc9d4280b54b44 commit ec09d1903c3cfadbaf6953afd1bc9d4280b54b44 Author: arthursonzogni <arthursonzogni@chromium.org> Date: Mon Jan 15 11:16:00 2018 Fix initiator for reload initiated by DevTools. This CL fixes issue 795694 . It was introduced by https://chromium-review.googlesource.com/730729. Why is this bug related to DevTools? | | In LocalFrame::Reload, there are two path for reload: | 1) The first is used by: | * window.location.reload() | * window.history.go(0) | * DOMPluginArray::refresh() | | This path use the NavigationScheduler::ScheduleReload(). There is | no issue with it. | | 2) The second is used by: | * DevTools page.Reload | * window.internals.forceReload([...]) | | There is a bug here. It turns out that DevTools was one of the | only ways to use this path. Other than that, DevTools and this | regression are not really connected. What is the issue with the second path and chrome://dino ? | | When using the chrome://dino URL, there are 2 differents URL. | * chrome://dino is used in the omnibox and in the renderer | HistoryItem. | * chrome-error://chromewebdata/ is the renderer current document | URL. | The issue is that the wrong one was used as the initiator. Since | chrome://dino is a WebUI URL, the navigation was blocked. This CL restores the reload behavior to what it was before https://chromium-review.googlesource.com/730729. That is to say, the FrameLoadRequest origin_document is always nullptr and the request's initiator is the request's url (the right one this time). A regression test is included to cover this particular path. Bug: 795694 , 648608 , 625969 Change-Id: I6064b1852b56aa301de0ad16e4ffbbc025e246f4 Reviewed-on: https://chromium-review.googlesource.com/836607 Reviewed-by: Camille Lamy <clamy@chromium.org> Reviewed-by: Nate Chapin <japhet@chromium.org> Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org> Cr-Commit-Position: refs/heads/master@{#529246} [modify] https://crrev.com/ec09d1903c3cfadbaf6953afd1bc9d4280b54b44/content/browser/devtools/render_frame_devtools_agent_host_browsertest.cc [modify] https://crrev.com/ec09d1903c3cfadbaf6953afd1bc9d4280b54b44/third_party/WebKit/Source/core/frame/LocalFrame.cpp [modify] https://crrev.com/ec09d1903c3cfadbaf6953afd1bc9d4280b54b44/third_party/WebKit/Source/core/loader/FrameLoader.cpp [modify] https://crrev.com/ec09d1903c3cfadbaf6953afd1bc9d4280b54b44/third_party/WebKit/Source/core/loader/NavigationScheduler.cpp
,
Feb 18 2018
,
Mar 8 2018
,
Mar 8 2018
,
Mar 20 2018
|
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by bugdroid1@chromium.org
, Jul 7 2016