1298370 - Add SameSite attribute to Cookie Inspector

Status: RESOLVED FIXED

(DevTools :: Storage Inspector, defect, P2)

Description

[Florian Scholz (Open Web Docs)]

Once we support the SameSite attribute for cookies (bug 795346), we should add a column for this flag in the Storage Inspector.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1298370 - Add SameSite attribute to Cookie Inspector r+pbrosset

Review commit: https://reviewboard.mozilla.org/r/97066/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97066/

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: rebased-dependency.diff

If you want to apply the patch you will need to apply the rebased dependency first.

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8816278 [details]
Bug 1298370 - Add SameSite attribute to Cookie Inspector r+pbrosset

https://reviewboard.mozilla.org/r/97066/#review99094

Sorry for the delay here. The code changes are really simple. It's nice to see how extensible the storage panel is.
I have just one comment about defining the values in the properties file. I don't think they should be there, or they risk being translated, and that might not make sense for DevTools users.
Worth pinging :flod if you're not sure.

::: devtools/client/locales/en-US/storage.properties:48
(Diff revision 1)
> +storage.cookie.sameSite.lax=Lax
> +storage.cookie.sameSite.strict=Strict
> +storage.cookie.sameSite.unset=Unset

Are these ever supposed to be translated? If I'm not mistaken these are special values to the sameSite property. So they are words that people will use whatever their spoken language may be, they won't change.
So maybe they need to be `const` inside a Js file instead?

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Patrick Brosset <:pbro> from comment #3)
> Comment on attachment 8816278 [details]
> Bug 1298370 - Add SameSite attribute to Cookie Inspector
> 
> https://reviewboard.mozilla.org/r/97066/#review99094
> 
> Sorry for the delay here. The code changes are really simple. It's nice to
> see how extensible the storage panel is.

Yes, it is becoming very easy to make changes.

> I have just one comment about defining the values in the properties file. I
> don't think they should be there, or they risk being translated, and that
> might not make sense for DevTools users.
> Worth pinging :flod if you're not sure.
> 
> ::: devtools/client/locales/en-US/storage.properties:48
> (Diff revision 1)
> > +storage.cookie.sameSite.lax=Lax
> > +storage.cookie.sameSite.strict=Strict
> > +storage.cookie.sameSite.unset=Unset
> 
> Are these ever supposed to be translated? If I'm not mistaken these are
> special values to the sameSite property. So they are words that people will
> use whatever their spoken language may be, they won't change.
> So maybe they need to be `const` inside a Js file instead?

You are right, these should never be translated so I will remove them from the properties file and add appropriate comments.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8816278 [details]
Bug 1298370 - Add SameSite attribute to Cookie Inspector r+pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97066/diff/1-2/

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Filter on Brobdingnagian.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1298370 - Add SameSite attribute to Cookie Inspector

Review commit: https://reviewboard.mozilla.org/r/203576/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203576/

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8932528 [details]
Bug 1298370 - Add SameSite attribute to Cookie Inspector

https://reviewboard.mozilla.org/r/203576/#review209298

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff9b8f883b0c
Add SameSite attribute to Cookie Inspector r=pbro

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/ff9b8f883b0c

Comment 11

[Francesco Lodolo [:flod]]

Any reason to spell it "sameSite", where I can only find it spelled as SameSite (uppercase S)?
https://hg.mozilla.org/mozilla-central/diff/ff9b8f883b0c/devtools/client/locales/en-US/storage.properties

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Francesco Lodolo [:flod] from comment #11)
> Any reason to spell it "sameSite", where I can only find it spelled as
> SameSite (uppercase S)?
> https://hg.mozilla.org/mozilla-central/diff/ff9b8f883b0c/devtools/client/
> locales/en-US/storage.properties

We go by the name of the JavaScript property itself and the sameSite property is accessible via cookie.sameSite whereas cookie.SameSite would be undefined.

You can see the nsICookie2 IDL at https://searchfox.org/mozilla-central/source/netwerk/cookie/nsICookie2.idl

Comment 13

[Francesco Lodolo [:flod]]

https://searchfox.org/mozilla-central/source/netwerk/cookie/nsICookie2.idl#72-74

sameSite is spelled like that only when it's mentioned explicitly as an attribute, but it becomes SameSite in the middle of a sentence (e.g. "the SameSite attribute is not present"). Or in https://tools.ietf.org/html/draft-west-first-party-cookies-07#section-4.1

Since this is a label in the UI, I would expect the same treatment. 

Having said that, thanks for the explanation and confirming it's not a typo, that's what I wanted to clarify.

Comment 14

[Chris Mills [:cmills]]

I've documented this:

Added a bullet to the cookies section of the Storage Inspector page:
https://developer.mozilla.org/en-US/docs/Tools/Storage_Inspector#Cookies

Added a note to the Fx59 rel notes:
https://developer.mozilla.org/en-US/Firefox/Releases/59#Changes_for_web_developers

Let me know if this is OK. Thanks!

Also, one more question - the hostonly and secure columns don't appear to be there any more — when we these removed?

Comment 15

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #14)
> I've documented this:
> 
> Added a bullet to the cookies section of the Storage Inspector page:
> https://developer.mozilla.org/en-US/docs/Tools/Storage_Inspector#Cookies
> 
> Added a note to the Fx59 rel notes:
> https://developer.mozilla.org/en-US/Firefox/Releases/
> 59#Changes_for_web_developers
> 
> Let me know if this is OK. Thanks!
> 

Yes, great job.

> Also, one more question - the hostonly and secure columns don't appear to be
> there any more — when we these removed?

They are still there... right-click on a table header to show these columns that are hidden by default.

At the moment column settings are not stored so you have to show them each time, which is annoying. I am planning on fixing that at some point.

Comment 16

[Chris Mills [:cmills]]

(In reply to Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] from comment #15)
> They are still there... right-click on a table header to show these columns
> that are hidden by default.
> 
> At the moment column settings are not stored so you have to show them each
> time, which is annoying. I am planning on fixing that at some point.

Ah, thanks for letting me know Mike. I've added a note to

https://developer.mozilla.org/en-US/docs/Tools/Storage_Inspector#Cookies

to make this clear.