[downloads] Set initiator when handling downloads via a[download]

[downloads] Set initiator when handling downloads via a[download]

Downloads initiated via clicking on an anchor with a 'download'
attribute should behave the same way with regard to cookie handling as a
download initiated via a top level navigation.

This CL adds logic to set the initiator origin for URLRequests initiated
in response to such downloads so that the handling of SameSite cookies
follows suit with top level navigations.

R=mkwst@chromium.org, clamy@chromium.org
BUG=648043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/e6567ba989cbfd9780e0a5cdff960a3ae903b0ad
Cr-Commit-Position: refs/heads/master@{#439137}

[Mike West, 2016-12-09 12:40:26]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 12:40:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 13:43:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-09 13:43:41]

Dry run: This issue passed the CQ dry run.

[asanka, 2016-12-12 21:57:47]

Description was changed from

==========
[downloads] Add a test for a[download] with SameSite=Strict cookie.

R=mkwst@chromium.org
BUG=648043
==========

to

==========
[downloads] Add a test for a[download] with SameSite=Strict cookie.

R=mkwst@chromium.org
BUG=648043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[asanka, 2016-12-12 21:58:52]

Description was changed from

==========
[downloads] Add a test for a[download] with SameSite=Strict cookie.

R=mkwst@chromium.org
BUG=648043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[downloads] Set initiator when handling downloads via a[download]

R=mkwst@chromium.org, clamy@chromium.org
BUG=648043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[asanka, 2016-12-12 22:00:44]

Description was changed from

==========
[downloads] Set initiator when handling downloads via a[download]

R=mkwst@chromium.org, clamy@chromium.org
BUG=648043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[downloads] Set initiator when handling downloads via a[download]

Downloads initiated via clicking on an anchor with a 'download'
attribute should behave the same way with regard to cookie handling as a
download initiated via a top level navigation.

This CL adds logic to set the initiator origin for URLRequests initiated
in response to such downloads so that the handling of SameSite cookies
follows suit with top level navigations.

R=mkwst@chromium.org, clamy@chromium.org
BUG=648043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[asanka, 2016-12-12 22:02:45]

PTAL? I'll add OWNERS once you confirm that this is OK.

[asanka, 2016-12-13 02:32:55]

The CQ bit was checked by asanka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 02:33:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 02:46:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 02:46:29]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Mike West, 2016-12-13 14:04:50]

Compilation error aside, the test result looks correct to me.

//third_party/WebKit LGTM, as does the IPC change.

[asanka, 2016-12-13 15:22:44]

The CQ bit was checked by asanka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 15:23:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[asanka, 2016-12-13 15:38:17]

asanka@chromium.org changed reviewers:
+ clamy@chromium.org

[asanka, 2016-12-13 16:05:02]

On 2016/12/13 at 14:04:50, mkwst wrote:
> Compilation error aside, the test result looks correct to me.

Yup. Forgot to update an override. Looks like the builds are green now. The only
difference introduced in PS 3 is the change to render_view_host_unittest.cc to
catch up with render_frame_message_filter.h changes.

> //third_party/WebKit LGTM, as does the IPC change.

Thanks!

clamy: Could you have a look at the //content changes?

[commit-bot: I haz the power, 2016-12-13 17:11:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 17:11:24]

Dry run: This issue passed the CQ dry run.

[clamy, 2016-12-14 16:25:39]

Thanks! Two questions below. I haven't had time to look at the unit tests, will
do so tomorrow.

https://codereview.chromium.org/2561903002/diff/40001/content/browser/downloa...
File content/browser/download/download_request_core.cc (right):

https://codereview.chromium.org/2561903002/diff/40001/content/browser/downloa...
content/browser/download/download_request_core.cc:190:
request->set_initiator(params->initiator());
Sanity check: Does this happen before or after
URLRequestHttpJob::AddCookieHeaderAndStart? If before, the initiator won't
necessarily be the same as url & first_party_for_cookies, meaning that we may
not set the strict cookies. Is that ok?

https://codereview.chromium.org/2561903002/diff/40001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2561903002/diff/40001/content/common/frame_me...
content/common/frame_messages.h:433: IPC_STRUCT_MEMBER(url::Origin,
initiator_origin)
Should this be an Optional<url::Origin>, considering that it will be put in an
Optional<url::Origin> and that we're setting all initiators transmitted from the
renderer as optional?

[asanka, 2016-12-14 18:21:30]

https://codereview.chromium.org/2561903002/diff/40001/content/browser/downloa...
File content/browser/download/download_request_core.cc (right):

https://codereview.chromium.org/2561903002/diff/40001/content/browser/downloa...
content/browser/download/download_request_core.cc:190:
request->set_initiator(params->initiator());
On 2016/12/14 at 16:25:38, clamy wrote:
> Sanity check: Does this happen before or after
URLRequestHttpJob::AddCookieHeaderAndStart? If before, the initiator won't
necessarily be the same as url & first_party_for_cookies, meaning that we may
not set the strict cookies. Is that ok?

Yup. This happens prior to request->Start() being called. The URLRequestJob
substrates including URLRequestHttpJob are only instantiated after
URLRequest::Start() is called. The request is started only after it is handed
over to the ResourceLoader stack or URLDownloader. We are just setting up the
URLRequest at this stage.

The initiator and first_party_for_cookies can be different for downloads that
cross origins. SameSite=Strict cookies not being sent for these requests is the
intended effect.

https://codereview.chromium.org/2561903002/diff/40001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2561903002/diff/40001/content/common/frame_me...
content/common/frame_messages.h:433: IPC_STRUCT_MEMBER(url::Origin,
initiator_origin)
On 2016/12/14 at 16:25:38, clamy wrote:
> Should this be an Optional<url::Origin>, considering that it will be put in an
Optional<url::Origin> and that we're setting all initiators transmitted from the
renderer as optional?

The intent is that URLRequest::initiator() and in turn
DownloadUrlParameters::initiator() can be optional, but DownloadUrl requests
coming from a renderer *must* have an initiator_origin. If one is unavailable,
we'd end up with a unique origin which has subtly different semantics than a
null origin. I.e. a unique initiator would result in no SameSite cookies being
sent while a null initiator may result in SameSite=Lax cookies being sent for
safe methods.

mkwest: Is the above reasonable? Should a renderer be able to initiate a
download with a null initiator origin? I intentionally made it not so.

There are other users of the DownloadUrlParameters API (a.k.a.
DownloadManager::DownloadURL() API). These should be able to make requests
without specifying an initiator since there may not always be one. E.g. a
programmatic download initiated by Chrome itself wouldn't have an initiator
origin.

[clamy, 2016-12-16 14:51:15]

Thanks! Lgtm.

[asanka, 2016-12-16 15:21:41]

On 2016/12/16 at 14:51:15, clamy wrote:
> Thanks! Lgtm.

Thanks!

[asanka, 2016-12-16 15:22:07]

The CQ bit was checked by asanka@chromium.org

[asanka, 2016-12-16 15:22:07]

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/2561903002/#ps40001
(title: "Fix compilation")

[commit-bot: I haz the power, 2016-12-16 15:22:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 17:36:05]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481901727375130,
"parent_rev": "1f9be09151c5726e13d5a9090bc21e60245bb159", "commit_rev":
"82210c03a3d620ddb9f95027d72c15d0f3763060"}

[commit-bot: I haz the power, 2016-12-16 17:36:44]

Description was changed from

==========
[downloads] Set initiator when handling downloads via a[download]

Downloads initiated via clicking on an anchor with a 'download'
attribute should behave the same way with regard to cookie handling as a
download initiated via a top level navigation.

This CL adds logic to set the initiator origin for URLRequests initiated
in response to such downloads so that the handling of SameSite cookies
follows suit with top level navigations.

R=mkwst@chromium.org, clamy@chromium.org
BUG=648043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[downloads] Set initiator when handling downloads via a[download]

Downloads initiated via clicking on an anchor with a 'download'
attribute should behave the same way with regard to cookie handling as a
download initiated via a top level navigation.

This CL adds logic to set the initiator origin for URLRequests initiated
in response to such downloads so that the handling of SameSite cookies
follows suit with top level navigations.

R=mkwst@chromium.org, clamy@chromium.org
BUG=648043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2561903002
==========

[commit-bot: I haz the power, 2016-12-16 17:36:45]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-16 17:40:00]

Description was changed from

==========
[downloads] Set initiator when handling downloads via a[download]

Downloads initiated via clicking on an anchor with a 'download'
attribute should behave the same way with regard to cookie handling as a
download initiated via a top level navigation.

This CL adds logic to set the initiator origin for URLRequests initiated
in response to such downloads so that the handling of SameSite cookies
follows suit with top level navigations.

R=mkwst@chromium.org, clamy@chromium.org
BUG=648043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2561903002
==========

to

==========
[downloads] Set initiator when handling downloads via a[download]

Downloads initiated via clicking on an anchor with a 'download'
attribute should behave the same way with regard to cookie handling as a
download initiated via a top level navigation.

This CL adds logic to set the initiator origin for URLRequests initiated
in response to such downloads so that the handling of SameSite cookies
follows suit with top level navigations.

R=mkwst@chromium.org, clamy@chromium.org
BUG=648043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/e6567ba989cbfd9780e0a5cdff960a3ae903b0ad
Cr-Commit-Position: refs/heads/master@{#439137}
==========

[commit-bot: I haz the power, 2016-12-16 17:40:01]

Patchset 3 (id:??) landed as
https://crrev.com/e6567ba989cbfd9780e0a5cdff960a3ae903b0ad
Cr-Commit-Position: refs/heads/master@{#439137}