Implements strict secure cookies as the default behavior in //net

Implements strict secure cookies as the default behavior in //net

Strict secure cookies have been behind a runtime and Finch flag. Our
experiments show minimal negative impact on users, so this implements
the feature as the default. It also updates tests to integrate
previously separate strict secure tests into the default tests.

BUG=568188
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2633663003
Cr-Commit-Position: refs/heads/master@{#446710}
Committed: https://chromium.googlesource.com/chromium/src/+/a26e439d02baa9514b09c637bf5fa9f6790c4c3e

[jww, 2017-01-14 00:04:54]

Description was changed from

==========
Implements strict secure cookies as the default behavior in //net

Strict secure cookies have been behind a runtime and Finch flag. Our
experiments show minimal negative impact on users, so this implements
the feature as the default. It also updates tests to integrate
previously separate strict secure tests into the default tests.

BUG=568188
==========

to

==========
Implements strict secure cookies as the default behavior in //net

Strict secure cookies have been behind a runtime and Finch flag. Our
experiments show minimal negative impact on users, so this implements
the feature as the default. It also updates tests to integrate
previously separate strict secure tests into the default tests.

BUG=568188
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jww, 2017-01-14 00:07:27]

jww@chromium.org changed reviewers:
+ mkwst@chromium.org

[jww, 2017-01-14 00:07:28]

Mike, can you take a look at this? I'm going to wait until we land some version
of https://codereview.chromium.org/2623423002/, but this is the CL to put as the
default on ToT Strict Secure Cookies

[Mike West, 2017-01-14 06:24:02]

Wow. That's a lot more code than I'd expected.

The bits I can approve (//net/cookies, //content/shell,
//chrome/browser/extensions/api/cookies, and //chrome/browser/browsing_data)
LGTM % a nit. I agree that we should probably wait to land this, though, until
we figure out if you're going to need to touch all these files in some way to
grab the result of setting a cookie based on the conversation with devtools...

Given the timeline, it's probably worth setting up a meeting rather than waiting
for that patch to cycle through folks' review queue. :)

https://codereview.chromium.org/2633663003/diff/1/net/cookies/cookie_monster.cc
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/2633663003/diff/1/net/cookies/cookie_monster....
net/cookies/cookie_monster.cc:1622: // See:
https://tools.ietf.org/html/draft-west-leave-secure-cookies-alone
Nit: We should update these references to
https://tools.ietf.org/html/draft-ietf-httpbis-cookie-alone.

https://codereview.chromium.org/2633663003/diff/1/net/cookies/cookie_monster....
net/cookies/cookie_monster.cc:1978: current, safe_date, secure_purge_goal,
secure_cookie_its);
Hrm. I don't think we ever updated
https://tools.ietf.org/html/draft-west-cookie-priority-00 with the implications
for `secure`. I'm not sure it's worth your time to tackle that in the last two
weeks you're here, but maybe you could sketch out a summary in a doc somewhere
so I have something to work from? :)

[jww, 2017-01-24 18:09:26]

https://codereview.chromium.org/2633663003/diff/1/net/cookies/cookie_monster.cc
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/2633663003/diff/1/net/cookies/cookie_monster....
net/cookies/cookie_monster.cc:1622: // See:
https://tools.ietf.org/html/draft-west-leave-secure-cookies-alone
On 2017/01/14 06:24:01, Mike West (sloooooow) wrote:
> Nit: We should update these references to
> https://tools.ietf.org/html/draft-ietf-httpbis-cookie-alone.

Done.

https://codereview.chromium.org/2633663003/diff/1/net/cookies/cookie_monster....
net/cookies/cookie_monster.cc:1978: current, safe_date, secure_purge_goal,
secure_cookie_its);
On 2017/01/14 06:24:01, Mike West (sloooooow) wrote:
> Hrm. I don't think we ever updated
> https://tools.ietf.org/html/draft-west-cookie-priority-00 with the
implications
> for `secure`. I'm not sure it's worth your time to tackle that in the last two
> weeks you're here, but maybe you could sketch out a summary in a doc somewhere
> so I have something to work from? :)

Shared a doc with you:
https://docs.google.com/document/d/1fI_vpJ_mF5K_52c5o39wfyVCuPStrswc7E2bFB_o0...

[jww, 2017-01-25 16:38:22]

jww@chromium.org changed reviewers:
+ jochen@chromium.org, mmenke@chromium.org, xunjieli@chromium.org

[jww, 2017-01-25 16:38:23]

xunjieli@, would you mind reviewing headless/*?

jochen@, can you take a look a components/* and content/*?

mmenke@, can you take a lookt at net/* and chrome/browser/net/*?

Thanks!

[jochen (gone - plz use gerrit), 2017-01-25 16:41:00]

components & content lgtm

[mmenke, 2017-01-25 16:41:10]

On 2017/01/25 16:38:23, jww wrote:
> xunjieli@, would you mind reviewing headless/*?

I don't think xunjieli owns headless?

> jochen@, can you take a look a components/* and content/*?
> 
> mmenke@, can you take a lookt at net/* and chrome/browser/net/*?
> 
> Thanks!

I'll take a look later today.

[jww, 2017-01-25 16:41:38]

jww@chromium.org changed reviewers:
+ altimin@chromium.org
- xunjieli@chromium.org

[jww, 2017-01-25 16:41:38]

Whoops, error selecting reviewers. xunjieli, sorry about that, removing you from
reviewers since I misread what you're the owner of.

altimin@, can you take a look at headless/*? Thanks!

[altimin, 2017-01-25 16:50:08]

On 2017/01/25 16:41:38, jww wrote:
> Whoops, error selecting reviewers. xunjieli, sorry about that, removing you
from
> reviewers since I misread what you're the owner of.
> 
> altimin@, can you take a look at headless/*? Thanks!

headless/ lgtm

[mmenke, 2017-01-25 19:40:56]

net/, chrome/browser/net LGTM.  Deferring to mkwst on net/cookies (Did skim
them, but did nothing close to a careful review).

[jww, 2017-01-25 22:23:27]

The CQ bit was checked by jww@chromium.org

[jww, 2017-01-25 22:23:27]

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/2633663003/#ps20001
(title: "Nits")

[commit-bot: I haz the power, 2017-01-25 22:24:11]

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, 2017-01-25 22:28:14]

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

[commit-bot: I haz the power, 2017-01-25 22:28:15]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[jww, 2017-01-25 22:44:03]

The CQ bit was checked by jww@chromium.org

[jww, 2017-01-25 22:44:03]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
jochen@chromium.org, mmenke@chromium.org, altimin@chromium.org
Link to the patchset: https://codereview.chromium.org/2633663003/#ps40001
(title: "Rebase on ToT")

[commit-bot: I haz the power, 2017-01-25 22:44:55]

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, 2017-01-25 22:57:02]

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

[commit-bot: I haz the power, 2017-01-25 22:57:03]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[jww, 2017-01-25 23:36:42]

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

[commit-bot: I haz the power, 2017-01-25 23:37:20]

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

[jww, 2017-01-25 23:38:01]

jww@chromium.org changed reviewers:
+ eugenebut@chromium.org, torne@chromium.org

[jww, 2017-01-25 23:38:02]

torne@chromium.org: Please review changes in android_webview/*

eugenebut@chromium.org: Please review changes in ios/*

Thanks!

[commit-bot: I haz the power, 2017-01-25 23:49:02]

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

[commit-bot: I haz the power, 2017-01-25 23:49:03]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[jww, 2017-01-25 23:54:08]

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

[commit-bot: I haz the power, 2017-01-25 23:54:48]

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

[Eugene But (OOO till 7-30), 2017-01-26 00:10:37]

ios lgtm

[commit-bot: I haz the power, 2017-01-26 00:14:23]

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

[commit-bot: I haz the power, 2017-01-26 00:14:24]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Torne, 2017-01-26 11:53:00]

android_webview LGTM

[jww, 2017-01-26 21:56:20]

The CQ bit was checked by jww@chromium.org

[jww, 2017-01-26 21:56:21]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
jochen@chromium.org, mmenke@chromium.org, altimin@chromium.org,
torne@chromium.org, eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2633663003/#ps100001
(title: "Test fixes")

[commit-bot: I haz the power, 2017-01-26 21:56:59]

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, 2017-01-26 22:22:26]

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

[commit-bot: I haz the power, 2017-01-26 22:22:28]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jww, 2017-01-26 22:48:05]

The CQ bit was checked by jww@chromium.org

[jww, 2017-01-26 22:48:06]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
altimin@chromium.org, torne@chromium.org, eugenebut@chromium.org,
mkwst@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2633663003/#ps120001
(title: "More compile fixes")

[commit-bot: I haz the power, 2017-01-26 22:48:50]

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, 2017-01-27 02:17:55]

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

[commit-bot: I haz the power, 2017-01-27 02:17:56]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mike West, 2017-01-27 08:19:53]

On 2017/01/27 at 02:17:56, commit-bot wrote:
> Try jobs failed on following builders:
>   linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Looks like the following tests are failing on the ChromeOS bots:

- ProfileAuthDataTest.TransferOnSubsequentLogin
- ProfileAuthDataTest.TransferOnFirstLoginWithNewProfile
- ProfileAuthDataTest.TransferOnFirstLoginWithExistingProfile

Think you'll have time to poke at them today? :(

[jww, 2017-01-27 14:55:35]

Yup, I already checked it out last night and figured out the issue, but SSH
is broken on my workstation. Will fix it this morning.

On Fri, Jan 27, 2017 at 12:19 AM <mkwst@chromium.org> wrote:

> On 2017/01/27 at 02:17:56, commit-bot wrote:
> > Try jobs failed on following builders:
> > linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
> (JOB_FAILED,
>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> )
>
> Looks like the following tests are failing on the ChromeOS bots:
>
> - ProfileAuthDataTest.TransferOnSubsequentLogin
> - ProfileAuthDataTest.TransferOnFirstLoginWithNewProfile
> - ProfileAuthDataTest.TransferOnFirstLoginWithExistingProfile
>
> Think you'll have time to poke at them today? :(
>
> https://codereview.chromium.org/2633663003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mike West, 2017-01-27 14:57:45]

On 2017/01/27 at 14:55:35, jww wrote:
> Yup, I already checked it out last night and figured out the issue, but SSH
> is broken on my workstation. Will fix it this morning.

Yay for fixing!

Boo for only having today to do so. :(

[jww, 2017-01-27 16:55:42]

The CQ bit was checked by jww@chromium.org

[jww, 2017-01-27 16:55:43]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
altimin@chromium.org, torne@chromium.org, eugenebut@chromium.org,
mkwst@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2633663003/#ps160001
(title: "Rebase on ToT")

[commit-bot: I haz the power, 2017-01-27 16:56:16]

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, 2017-01-27 18:17:32]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1485536142769520,
"parent_rev": "3fe97521b0c4a1274cbb7e3aea6815b989849870", "commit_rev":
"a26e439d02baa9514b09c637bf5fa9f6790c4c3e"}

[commit-bot: I haz the power, 2017-01-27 18:18:29]

Description was changed from

==========
Implements strict secure cookies as the default behavior in //net

Strict secure cookies have been behind a runtime and Finch flag. Our
experiments show minimal negative impact on users, so this implements
the feature as the default. It also updates tests to integrate
previously separate strict secure tests into the default tests.

BUG=568188
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implements strict secure cookies as the default behavior in //net

Strict secure cookies have been behind a runtime and Finch flag. Our
experiments show minimal negative impact on users, so this implements
the feature as the default. It also updates tests to integrate
previously separate strict secure tests into the default tests.

BUG=568188
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2633663003
Cr-Commit-Position: refs/heads/master@{#446710}
Committed:
https://chromium.googlesource.com/chromium/src/+/a26e439d02baa9514b09c637bf5f...
==========

[commit-bot: I haz the power, 2017-01-27 18:18:31]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/a26e439d02baa9514b09c637bf5f...