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

Issue 625969 link

Starred by 2 users

Issue metadata

Status: Duplicate
Merged: issue 811669
Owner:
Buried. Ping if important.
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 648608



Sign in to add a comment

Cleanup request initiator.

Project Member Reported by mkwst@chromium.org, Jul 6 2016

Issue description

We'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.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 7 2016

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

commit fe9c3e778fcdeedaedd3dbd96d6f2b30f12fb6eb
Author: mkwst <mkwst@chromium.org>
Date: Thu Jul 07 12:31:54 2016

Explicitly set a requestor origin when calling into WebLocalFrame::loadRequest

Various tests (and, in particular, the testrunner itself) use loadRequest
as a shortcut around various bits of the loading infrastructure. This patch
has zero web-visible impact, but will ensure that the DCHECK I'd like to add
in https://codereview.chromium.org/2128503002 doesn't cause most tests to
explode.

BUG= 625969 
R=jochen@chromium.org

Review-Url: https://codereview.chromium.org/2125263002
Cr-Commit-Position: refs/heads/master@{#404135}

[modify] https://crrev.com/fe9c3e778fcdeedaedd3dbd96d6f2b30f12fb6eb/chrome/renderer/net/net_error_helper.cc
[modify] https://crrev.com/fe9c3e778fcdeedaedd3dbd96d6f2b30f12fb6eb/components/printing/renderer/print_web_view_helper.cc
[modify] https://crrev.com/fe9c3e778fcdeedaedd3dbd96d6f2b30f12fb6eb/content/public/test/render_view_test.cc
[modify] https://crrev.com/fe9c3e778fcdeedaedd3dbd96d6f2b30f12fb6eb/content/shell/renderer/layout_test/blink_test_runner.cc
[modify] https://crrev.com/fe9c3e778fcdeedaedd3dbd96d6f2b30f12fb6eb/third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp
[modify] https://crrev.com/fe9c3e778fcdeedaedd3dbd96d6f2b30f12fb6eb/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
[modify] https://crrev.com/fe9c3e778fcdeedaedd3dbd96d6f2b30f12fb6eb/third_party/WebKit/Source/web/tests/sim/SimTest.cpp

Project Member

Comment 2 by bugdroid1@chromium.org, 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

Project Member

Comment 3 by bugdroid1@chromium.org, 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

Project Member

Comment 4 by bugdroid1@chromium.org, Sep 12 2016

Labels: merge-merged-2840
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

Blocking: 648608
Project Member

Comment 6 by bugdroid1@chromium.org, 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

Comment 7 by rbyers@chromium.org, Nov 18 2016

Components: Blink>SecurityFeature
Project Member

Comment 8 by bugdroid1@chromium.org, 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

Cc: mkwst@chromium.org arthurso...@chromium.org
Owner: arthurso...@chromium.org
I am temporary working on it to make  issue 648608  working. I have WIP CL.
Labels: Hotlist-EnamelAndFriendsFixIt
Project Member

Comment 11 by bugdroid1@chromium.org, 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

Owner: mkwst@chromium.org
Status: Assigned (was: Started)
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.
Project Member

Comment 13 by bugdroid1@chromium.org, 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

Labels: -Hotlist-EnamelAndFriendsFixIt
Components: Blink>Loader
Labels: OOR-CORS
Cc: toyoshim@chromium.org
Mergedinto: 811669
Status: Duplicate (was: Assigned)
Let me merge this to 811669

Sign in to add a comment