Add console warning and tests for strict secure cookies.

Add console warning and tests for strict secure cookies.

This adds a console warning if the Set-Cookie header is present on an
insecure connection to warn that the cookie will be ignored. Also adds
LayoutTests to verify this behavior.

BUG=568188

[jww, 2017-01-12 06:30:46]

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

[jww, 2017-01-12 06:30:47]

Mike, this is the console warning we mentioned, and some tests. Can you take a
look and decide if it's too hack-y?

Also, I don't believe we have a console warning for a document.cookie added
secure cookie from an insecure origin, and we only discussed Set-Cookie. It
seems like we ought to have that as well. What do you think?

[Mike West, 2017-01-12 07:59:02]

mkwst@chromium.org changed reviewers:
+ dgozman@chromium.org, pfeldman@chromium.org

[Mike West, 2017-01-12 07:59:03]

This doesn't strike me as hacky; it's very much in line with the existing
console warning. LGTM, conceptually!

Perhaps dgozman@ or pfeldman@ can give you more feedback on the devtools
details.

https://codereview.chromium.org/2623423002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js (right):

https://codereview.chromium.org/2623423002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:389: var
cookies = SDK.CookieParser.parseSetCookie(this._manager._target,
response.headers['Set-Cookie']);
Oh, nice! We already have a parser! :)

[dgozman, 2017-01-12 21:31:24]

Does this happen often? I'm worried about console being spammed with theses
messages vs. their usefulness. Maybe we should put the warning in Network Panel
(request details -> cookies)?

[jww, 2017-01-12 22:38:22]

On 2017/01/12 21:31:24, dgozman wrote:
> Does this happen often? I'm worried about console being spammed with theses
> messages vs. their usefulness. Maybe we should put the warning in Network
Panel
> (request details -> cookies)?

Unfortunately, we don't have a stat on this particular failure. To our best
guess, it appears to be relatively rare, in so far as this experiment is running
on a non-negligible percent of Stable right now, and we've gotten no complaints.
Moreover, since this is a web platform change that "happens" to live in
//net/cookies, IMO, the JavaScript console is the right place for this message
to live, but I'd love to get mkwst@'s thoughts as well.

[jww, 2017-01-12 23:53:39]

On 2017/01/12 22:38:22, jww wrote:
> On 2017/01/12 21:31:24, dgozman wrote:
> > Does this happen often? I'm worried about console being spammed with theses
> > messages vs. their usefulness. Maybe we should put the warning in Network
> Panel
> > (request details -> cookies)?
> 
> Unfortunately, we don't have a stat on this particular failure. To our best
> guess, it appears to be relatively rare, in so far as this experiment is
running
> on a non-negligible percent of Stable right now, and we've gotten no
complaints.
> Moreover, since this is a web platform change that "happens" to live in
> //net/cookies, IMO, the JavaScript console is the right place for this message
> to live, but I'd love to get mkwst@'s thoughts as well.

I lied! We do have data, hoo-ray! We have histograms for cookie source scheme,
and whether the cookie is secure or not. I'll send you the details offline, but
the occurrence of a cookie being created with an insecure scheme and being
marked as secure is extremely low.

[dgozman, 2017-01-13 00:11:38]

> I lied! We do have data, hoo-ray! We have histograms for cookie source scheme,
> and whether the cookie is secure or not. I'll send you the details offline,
but
> the occurrence of a cookie being created with an insecure scheme and being
> marked as secure is extremely low.
Looks like not that much. Hopefully, that doesn't happen on some common
ads/analytics network, or everyone would be spammed with warnings.

About implementation: we should instead add messages on backend. To achieve
this, you need to plumb a warning from net to somewhere in the browser process
and call RenderFrameHost::AddMessageToConsole with a warning level.

[jww, 2017-01-13 00:13:26]

On 2017/01/13 00:11:38, dgozman wrote:
> > I lied! We do have data, hoo-ray! We have histograms for cookie source
scheme,
> > and whether the cookie is secure or not. I'll send you the details offline,
> but
> > the occurrence of a cookie being created with an insecure scheme and being
> > marked as secure is extremely low.
> Looks like not that much. Hopefully, that doesn't happen on some common
> ads/analytics network, or everyone would be spammed with warnings.
> 
> About implementation: we should instead add messages on backend. To achieve
> this, you need to plumb a warning from net to somewhere in the browser process
> and call RenderFrameHost::AddMessageToConsole with a warning level.

The thing about that is... the //net folks told me to take this approach, since
//net doesn't know about messages, consoles and such. I suppose that means we're
at a bit of a standstill. mkwst@, thoughts?

[Mike West, 2017-01-13 11:51:14]

On 2017/01/13 at 00:13:26, jww wrote:
> On 2017/01/13 00:11:38, dgozman wrote:
> > > I lied! We do have data, hoo-ray! We have histograms for cookie source
scheme,
> > > and whether the cookie is secure or not. I'll send you the details
offline,
> > but
> > > the occurrence of a cookie being created with an insecure scheme and being
> > > marked as secure is extremely low.
> > Looks like not that much. Hopefully, that doesn't happen on some common
> > ads/analytics network, or everyone would be spammed with warnings.
> > 
> > About implementation: we should instead add messages on backend. To achieve
> > this, you need to plumb a warning from net to somewhere in the browser
process
> > and call RenderFrameHost::AddMessageToConsole with a warning level.
> 
> The thing about that is... the //net folks told me to take this approach,
since //net doesn't know about messages, consoles and such. I suppose that means
we're at a bit of a standstill. mkwst@, thoughts?

I think we need a console message. I don't have a strong opinion about how we
produce it.

Conceptually, I agree with dgozman@ that it would be better for //net to be
responsible for firing off the error, and //content for turning that error into
a console message. Practically, that requires a lot of refactoring in //net and
the associated binding that //net folks don't seem happy about undertaking.

Given that we already have one cookie warning in devtools, it seems reasonable
to me to add another. dgozman@, if you disagree, it's probably best for you and
jww@ to sit in a room with mmenke@, et al. to come to agreement on a strategy
going forward.

[jww, 2017-01-16 05:44:06]

+mmenke@, since we care about his opinion on console warnings, cookies, etc.

[mmenke, 2017-01-17 18:45:31]

mmenke@chromium.org changed reviewers:
+ mmenke@chromium.org

[mmenke, 2017-01-17 18:45:32]

The message LGTM.

[pfeldman, 2017-01-17 19:18:33]

@mike: it is not just the front-end vs backend. Unless you report if on the
backend, it won't be there upon opening devtools. Backend-reported console
messages are cached even when devtools is not yet there.

[mmenke, 2017-01-17 19:28:09]

On 2017/01/13 11:51:14, Mike West (sloooooow) wrote:
> On 2017/01/13 at 00:13:26, jww wrote:
> > On 2017/01/13 00:11:38, dgozman wrote:
> > > > I lied! We do have data, hoo-ray! We have histograms for cookie source
> scheme,
> > > > and whether the cookie is secure or not. I'll send you the details
> offline,
> > > but
> > > > the occurrence of a cookie being created with an insecure scheme and
being
> > > > marked as secure is extremely low.
> > > Looks like not that much. Hopefully, that doesn't happen on some common
> > > ads/analytics network, or everyone would be spammed with warnings.
> > > 
> > > About implementation: we should instead add messages on backend. To
achieve
> > > this, you need to plumb a warning from net to somewhere in the browser
> process
> > > and call RenderFrameHost::AddMessageToConsole with a warning level.
> > 
> > The thing about that is... the //net folks told me to take this approach,
> since //net doesn't know about messages, consoles and such. I suppose that
means
> we're at a bit of a standstill. mkwst@, thoughts?
> 
> I think we need a console message. I don't have a strong opinion about how we
> produce it.
> 
> Conceptually, I agree with dgozman@ that it would be better for //net to be
> responsible for firing off the error, and //content for turning that error
into
> a console message. Practically, that requires a lot of refactoring in //net
and
> the associated binding that //net folks don't seem happy about undertaking.
> 
> Given that we already have one cookie warning in devtools, it seems reasonable
> to me to add another. dgozman@, if you disagree, it's probably best for you
and
> jww@ to sit in a room with mmenke@, et al. to come to agreement on a strategy
> going forward.

Sorry, missed this.  It does make conceptually make sense that when net/ handles
errors it should also be generating the error text, but we'd have to design and
wire up a whole new API for that, which seems a pretty major undertaking.  We're
also (ever so slowly) starting to shift towards mojo, which has an influence on
how everything will talk to net/, going forward.

[mmenke, 2017-01-17 19:31:49]

On 2017/01/17 19:28:09, mmenke wrote:
> On 2017/01/13 11:51:14, Mike West (sloooooow) wrote:
> > On 2017/01/13 at 00:13:26, jww wrote:
> > > On 2017/01/13 00:11:38, dgozman wrote:
> > > > > I lied! We do have data, hoo-ray! We have histograms for cookie source
> > scheme,
> > > > > and whether the cookie is secure or not. I'll send you the details
> > offline,
> > > > but
> > > > > the occurrence of a cookie being created with an insecure scheme and
> being
> > > > > marked as secure is extremely low.
> > > > Looks like not that much. Hopefully, that doesn't happen on some common
> > > > ads/analytics network, or everyone would be spammed with warnings.
> > > > 
> > > > About implementation: we should instead add messages on backend. To
> achieve
> > > > this, you need to plumb a warning from net to somewhere in the browser
> > process
> > > > and call RenderFrameHost::AddMessageToConsole with a warning level.
> > > 
> > > The thing about that is... the //net folks told me to take this approach,
> > since //net doesn't know about messages, consoles and such. I suppose that
> means
> > we're at a bit of a standstill. mkwst@, thoughts?
> > 
> > I think we need a console message. I don't have a strong opinion about how
we
> > produce it.
> > 
> > Conceptually, I agree with dgozman@ that it would be better for //net to be
> > responsible for firing off the error, and //content for turning that error
> into
> > a console message. Practically, that requires a lot of refactoring in //net
> and
> > the associated binding that //net folks don't seem happy about undertaking.
> > 
> > Given that we already have one cookie warning in devtools, it seems
reasonable
> > to me to add another. dgozman@, if you disagree, it's probably best for you
> and
> > jww@ to sit in a room with mmenke@, et al. to come to agreement on a
strategy
> > going forward.
> 
> Sorry, missed this.  It does make conceptually make sense that when net/
handles
> errors it should also be generating the error text, but we'd have to design
and
> wire up a whole new API for that, which seems a pretty major undertaking. 
We're
> also (ever so slowly) starting to shift towards mojo, which has an influence
on
> how everything will talk to net/, going forward.

Also worth noting we already have a ton of bloaty single-use APIs in net/, and
I'm reluctant to add more, particularly as the messages would have to pass a
bunch of layers before arriving at devtools.  I'm inclined to say ugly as this
is, we should stick with it for now.

[pfeldman, 2017-01-17 19:37:29]

Here is what I think. This warning message is either important or not.

If it is important, it needs to be delivered to the user, even if devtools
window is opened post factum
If it is not important, let's not bother with it at all.

[jww, 2017-01-17 23:51:04]

On 2017/01/17 19:37:29, pfeldman wrote:
> Here is what I think. This warning message is either important or not.
> 
> If it is important, it needs to be delivered to the user, even if devtools
> window is opened post factum
> If it is not important, let's not bother with it at all.

mmenke@, dgozman@, and I are meeting tomorrow to discuss our options.
Personally, I think there's a median position where we believe //net should have
a way to place messages in the console, but we don't have a good way to do that
yet, and its a larger undertaking than we'd like to go through right now,
especially with the //net API changes currently underway. But hopefully we can
see what makes the most sense to everyone tomorrow.

[jww, 2017-01-18 22:33:02]

On 2017/01/17 23:51:04, jww wrote:
> On 2017/01/17 19:37:29, pfeldman wrote:
> > Here is what I think. This warning message is either important or not.
> > 
> > If it is important, it needs to be delivered to the user, even if devtools
> > window is opened post factum
> > If it is not important, let's not bother with it at all.
> 
> mmenke@, dgozman@, and I are meeting tomorrow to discuss our options.
> Personally, I think there's a median position where we believe //net should
have
> a way to place messages in the console, but we don't have a good way to do
that
> yet, and its a larger undertaking than we'd like to go through right now,
> especially with the //net API changes currently underway. But hopefully we can
> see what makes the most sense to everyone tomorrow.

Hi all. Follow up from my our meeting:

mmenke@ said that building the infrastructure for console messaging from //net
would be surprisingly complex to build and maintain, and there is probably not a
lot of use for it. Having said that, there *might* be things, such as caching,
for which it would be useful to have console warnings.

jww@ pointed out that Cookies are particularly odd because they like in //net
but are kind of a web API, which is why we want this console warning in the
first place (since we generally would console warn about a similar change in web
API behavior).

dgozman@ said that he's not convinced that the warning is useful enough to build
the //net infrastructure or have the devtools front-end hack.

In the end, we were all pretty convinced that it's not worth building the //net
infrastructure because we couldn't come up with similar cases that it would make
sense to have console warnings for, especially given the building and
maintenance cost. We agreed that especially since this is an API removal, it
might make sense to put the warning in the devtools frontend code (as done in
this CL) for a temporary bit of time.

Thinking this over, I suggest that we put it in the devtools front end for 6
months. This would give it time to percolate up to stable, make sure nothing
wonky is happening, and give folks time to adjust. dgozman@, does that sound
reasonable to you?

[mmenke, 2017-01-18 22:40:17]

On 2017/01/18 22:33:02, jww wrote:
> On 2017/01/17 23:51:04, jww wrote:
> > On 2017/01/17 19:37:29, pfeldman wrote:
> > > Here is what I think. This warning message is either important or not.
> > > 
> > > If it is important, it needs to be delivered to the user, even if devtools
> > > window is opened post factum
> > > If it is not important, let's not bother with it at all.
> > 
> > mmenke@, dgozman@, and I are meeting tomorrow to discuss our options.
> > Personally, I think there's a median position where we believe //net should
> have
> > a way to place messages in the console, but we don't have a good way to do
> that
> > yet, and its a larger undertaking than we'd like to go through right now,
> > especially with the //net API changes currently underway. But hopefully we
can
> > see what makes the most sense to everyone tomorrow.
> 
> Hi all. Follow up from my our meeting:
> 
> mmenke@ said that building the infrastructure for console messaging from //net
> would be surprisingly complex to build and maintain, and there is probably not
a
> lot of use for it. Having said that, there *might* be things, such as caching,
> for which it would be useful to have console warnings.
> 
> jww@ pointed out that Cookies are particularly odd because they like in //net
> but are kind of a web API, which is why we want this console warning in the
> first place (since we generally would console warn about a similar change in
web
> API behavior).
> 
> dgozman@ said that he's not convinced that the warning is useful enough to
build
> the //net infrastructure or have the devtools front-end hack.
> 
> In the end, we were all pretty convinced that it's not worth building the
//net
> infrastructure because we couldn't come up with similar cases that it would
make
> sense to have console warnings for, especially given the building and
> maintenance cost. We agreed that especially since this is an API removal, it
> might make sense to put the warning in the devtools frontend code (as done in
> this CL) for a temporary bit of time.
> 
> Thinking this over, I suggest that we put it in the devtools front end for 6
> months. This would give it time to percolate up to stable, make sure nothing
> wonky is happening, and give folks time to adjust. dgozman@, does that sound
> reasonable to you?

Worth noting that one thing that makes this a bit unusual is that it's not a
hard failure - most network things are, and we have a way to pass network error
codes down.  There are other soft failure cases (Ignoring invalid headers, for
instance), but they're less common, generally less important, and code for them
is strewn everywhere.  If we wanted to log other metadata (Like detailed caching
information), a more flexible API that could handle that sort of thing may be
more useful, but also a lot more work, and add more overhead.

[Mike West, 2017-01-19 20:29:09]

On 2017/01/18 at 22:40:17, mmenke wrote:
> On 2017/01/18 22:33:02, jww wrote:
> > On 2017/01/17 23:51:04, jww wrote:
> > > On 2017/01/17 19:37:29, pfeldman wrote:
> > > > Here is what I think. This warning message is either important or not.
> > > > 
> > > > If it is important, it needs to be delivered to the user, even if
devtools
> > > > window is opened post factum
> > > > If it is not important, let's not bother with it at all.
> > > 
> > > mmenke@, dgozman@, and I are meeting tomorrow to discuss our options.
> > > Personally, I think there's a median position where we believe //net
should
> > have
> > > a way to place messages in the console, but we don't have a good way to do
> > that
> > > yet, and its a larger undertaking than we'd like to go through right now,
> > > especially with the //net API changes currently underway. But hopefully we
can
> > > see what makes the most sense to everyone tomorrow.
> > 
> > Hi all. Follow up from my our meeting:
> > 
> > mmenke@ said that building the infrastructure for console messaging from
//net
> > would be surprisingly complex to build and maintain, and there is probably
not a
> > lot of use for it. Having said that, there *might* be things, such as
caching,
> > for which it would be useful to have console warnings.
> > 
> > jww@ pointed out that Cookies are particularly odd because they like in
//net
> > but are kind of a web API, which is why we want this console warning in the
> > first place (since we generally would console warn about a similar change in
web
> > API behavior).
> > 
> > dgozman@ said that he's not convinced that the warning is useful enough to
build
> > the //net infrastructure or have the devtools front-end hack.
> > 
> > In the end, we were all pretty convinced that it's not worth building the
//net
> > infrastructure because we couldn't come up with similar cases that it would
make
> > sense to have console warnings for, especially given the building and
> > maintenance cost. We agreed that especially since this is an API removal, it
> > might make sense to put the warning in the devtools frontend code (as done
in
> > this CL) for a temporary bit of time.
> > 
> > Thinking this over, I suggest that we put it in the devtools front end for 6
> > months. This would give it time to percolate up to stable, make sure nothing
> > wonky is happening, and give folks time to adjust. dgozman@, does that sound
> > reasonable to you?
> 
> Worth noting that one thing that makes this a bit unusual is that it's not a
hard failure - most network things are, and we have a way to pass network error
codes down.  There are other soft failure cases (Ignoring invalid headers, for
instance), but they're less common, generally less important, and code for them
is strewn everywhere.  If we wanted to log other metadata (Like detailed caching
information), a more flexible API that could handle that sort of thing may be
more useful, but also a lot more work, and add more overhead.

A temporary warning SGTM.

[dgozman, 2017-01-20 21:59:09]

I'm in favor of not landing the warning at all. Looks like nobody feels it's
important enough to invest into proper solution, so let's not do it. Otherwise
we are going to increase our technical debt without obvious benefits.

[mmenke, 2017-01-20 22:01:22]

On 2017/01/20 21:59:09, dgozman wrote:
> I'm in favor of not landing the warning at all. Looks like nobody feels it's
> important enough to invest into proper solution, so let's not do it. Otherwise
> we are going to increase our technical debt without obvious benefits.

My opinion is that it's a tiny amount of technical debt to save developer time,
so is worth landing.  There currently just doesn't seem to be a compelling
argument for a new path from net all the way down the devtools.

[pfeldman, 2017-01-20 22:32:51]

> My opinion is that it's a tiny amount of technical debt to save developer
time,
> so is worth landing.  There currently just doesn't seem to be a compelling
> argument for a new path from net all the way down the devtools.

Technical debt is not a concern, not reliably delivering a warning is. User
expects warnings to be surfaced after opening devtools. not lgtm.

[pfeldman, 2017-02-23 01:17:16]

Description was changed from

==========
Add console warning and tests for strict secure cookies.

This adds a console warning if the Set-Cookie header is present on an
insecure connection to warn that the cookie will be ignored. Also adds
LayoutTests to verify this behavior.

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

to

==========
Add console warning and tests for strict secure cookies.

This adds a console warning if the Set-Cookie header is present on an
insecure connection to warn that the cookie will be ignored. Also adds
LayoutTests to verify this behavior.

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

[pfeldman, 2017-02-23 01:17:16]

pfeldman@chromium.org changed reviewers:
- pfeldman@chromium.org

[mmenke, 2017-03-06 21:15:18]

mmenke@chromium.org changed reviewers:
- mmenke@chromium.org

[dgozman, 2017-07-15 05:47:01]

dgozman@chromium.org changed reviewers:
- dgozman@chromium.org