|
|
Created:
3 years, 6 months ago by kinuko Modified:
3 years, 6 months ago CC:
blink-reviews, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, gavinp+loader_chromium.org, jam, Nate Chapin, kinuko+watch, loading-reviews+fetch_chromium.org, loading-reviews_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, tyoshino+watch_chromium.org, yhirano, Yoav Weiss Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove 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}
Committed: https://chromium.googlesource.com/chromium/src/+/74a7fb2fa4307c69812945586b39018ae4f231b2
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : . #Patch Set 5 : . #Patch Set 6 : . #Patch Set 7 : . #Patch Set 8 : . #
Total comments: 7
Patch Set 9 : rebase #Patch Set 10 : Mike comment (link fix) #
Total comments: 6
Patch Set 11 : . #
Messages
Total messages: 72 (57 generated)
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Remove dup'ed code for RequestorOrigin and FirstPartyCookie Also fix some TODOs. BUG=671533 ========== to ========== 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 BUG=671533 ==========
kinuko@chromium.org changed reviewers: + japhet@chromium.org, mkwst@chromium.org
Current code for RequestorOrigin and FirstPartyCookie is too confusing to me (some code looks duplicated but doing slightly different things), so I'd like to remove the dups and reduce the loading code in RenderFrameImpl. What do you think? https://codereview.chromium.org/2918653004/diff/160001/content/renderer/fetch... File content/renderer/fetchers/resource_fetcher_impl.cc (right): https://codereview.chromium.org/2918653004/diff/160001/content/renderer/fetch... content/renderer/fetchers/resource_fetcher_impl.cc:186: request_.SetRequestorOrigin(frame->GetDocument().GetSecurityOrigin()); I think this was missing before this change (i.e. we are sending unique origin in the current code) https://codereview.chromium.org/2918653004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2918653004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:299: SetFirstPartyCookieAndRequestorOrigin(request); Moved here so this is also called in redirects (so that it's called at the same timing as RenderFrameImpl::WillSendRequest) https://codereview.chromium.org/2918653004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:704: void FrameFetchContext::SetFirstPartyCookieAndRequestorOrigin( Now this does mostly same as what RenderFrameImpl::WillSendRequest + FrameFetchContext were doing (I tried to preserve the original behavior but has slight changes) https://codereview.chromium.org/2918653004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:735: GetFrame()->GetDocument()->GetSecurityOrigin()); Wasn't bit sure if we should also check IsSandboxed(kSandboxOrigin) here (this code is from RenderFrameImpl::WillSendRequest) https://codereview.chromium.org/2918653004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp (left): https://codereview.chromium.org/2918653004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp:793: {"http://example.test", false, "", WebURLRequest::kFrameTypeNested, ""}, Now SetFirstPartyCookieAndRequestorOrigin also handle these mostly in the same way
This looks like an improvement to me. Thanks for poking at it. :) LGTM. https://codereview.chromium.org/2918653004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2918653004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:708: // https://tools.ietf.org/html/draft-west-first-party-cookies-04#section-2.1.1 Would you mind updating this to https://tools.ietf.org/html/draft-ietf-httpbis-cookie-same-site-00#section-2....
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2918653004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2918653004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:708: // https://tools.ietf.org/html/draft-west-first-party-cookies-04#section-2.1.1 On 2017/06/07 06:48:44, Mike West wrote: > Would you mind updating this to > https://tools.ietf.org/html/draft-ietf-httpbis-cookie-same-site-00#section-2.... Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Whoa, some tests failing again... will look.
Oh ok, I had uploaded an old patch... will re-up.
Patchset #9 (id:180001) has been deleted
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Re-uploaded. Nate, could you review as well?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tyoshino@chromium.org changed reviewers: + tyoshino@chromium.org
https://codereview.chromium.org/2918653004/diff/220001/content/renderer/rende... File content/renderer/render_frame_impl.cc (left): https://codereview.chromium.org/2918653004/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:4198: // `isNull()` check. https://crbug.com/625969 Add 625969 to the BUG line https://codereview.chromium.org/2918653004/diff/220001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2918653004/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:6242: // Set RequestorInfo and FirstPartyForCookies. RequestorInfo -> RequestorOrigin https://codereview.chromium.org/2918653004/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:6251: if (!info.url_request.GetExtraData()) use request everywhere in this function?
Description was changed from ========== 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 BUG=671533 ========== to ========== 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 BUG=671533, 625969 ==========
Thanks, updated. https://codereview.chromium.org/2918653004/diff/220001/content/renderer/rende... File content/renderer/render_frame_impl.cc (left): https://codereview.chromium.org/2918653004/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:4198: // `isNull()` check. https://crbug.com/625969 On 2017/06/08 04:45:44, tyoshino wrote: > Add 625969 to the BUG line Done. https://codereview.chromium.org/2918653004/diff/220001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2918653004/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:6242: // Set RequestorInfo and FirstPartyForCookies. On 2017/06/08 04:45:45, tyoshino wrote: > RequestorInfo -> RequestorOrigin Done. https://codereview.chromium.org/2918653004/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:6251: if (!info.url_request.GetExtraData()) On 2017/06/08 04:45:45, tyoshino wrote: > use request everywhere in this function? Was a bit afraid it might make the change unnecessarily bigger, let me do it in a follow-up patch.
The CQ bit was checked by kinuko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm It's great if you could explain briefly how RenderFrameImpl::WillSendRequest() has been merged into SetFirstPartyCookieAndRequestorOrigin() especially regarding how first party cookie conditions have been merged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/08 05:30:59, tyoshino wrote: > lgtm > > It's great if you could explain briefly how RenderFrameImpl::WillSendRequest() > has been merged into SetFirstPartyCookieAndRequestorOrigin() especially > regarding how first party cookie conditions have been merged. Do you mean documentation, like in the issue description or in comment?
On 2017/06/08 07:29:56, kinuko wrote: > On 2017/06/08 05:30:59, tyoshino wrote: > > lgtm > > > > It's great if you could explain briefly how RenderFrameImpl::WillSendRequest() > > has been merged into SetFirstPartyCookieAndRequestorOrigin() especially > > regarding how first party cookie conditions have been merged. > > Do you mean documentation, like in the issue description or in comment? I meant CL description, sorry!
Description was changed from ========== 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 BUG=671533, 625969 ========== to ========== 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 ==========
On 2017/06/08 08:01:53, tyoshino wrote: > On 2017/06/08 07:29:56, kinuko wrote: > > On 2017/06/08 05:30:59, tyoshino wrote: > > > lgtm > > > > > > It's great if you could explain briefly how > RenderFrameImpl::WillSendRequest() > > > has been merged into SetFirstPartyCookieAndRequestorOrigin() especially > > > regarding how first party cookie conditions have been merged. > > > > Do you mean documentation, like in the issue description or in comment? > > I meant CL description, sorry! Done.
Nate- I'm thinking about landing this but please feel free to un-CQ or give more comments, I can fix that on this or in a follow-up.
The CQ bit was checked by kinuko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/2918653004/#ps240001 (title: ".")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 240001, "attempt_start_ts": 1496979945035350, "parent_rev": "74db9eb407f16d82c478a2adfe293f861725ba5b", "commit_rev": "74a7fb2fa4307c69812945586b39018ae4f231b2"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} Committed: https://chromium.googlesource.com/chromium/src/+/74a7fb2fa4307c69812945586b39... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:240001) as https://chromium.googlesource.com/chromium/src/+/74a7fb2fa4307c69812945586b39... |