Closed Bug 1298370 Opened 5 years ago Closed 4 years ago

Add SameSite attribute to Cookie Inspector

Categories

(DevTools :: Storage Inspector, defect, P2)

defect

Tracking

(firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: fs, Assigned: miker)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [todo-mr][t3])

Attachments

(1 file, 2 obsolete files)

Once we support the SameSite attribute for cookies (bug 795346), we should add a column for this flag in the Storage Inspector.
Priority: -- → P2
Depends on: 1286858
No longer depends on: samesite-cookies
Attachment #8816278 - Flags: review?(pbrosset)
Attached patch rebased-dependency.diff (obsolete) — Splinter Review
If you want to apply the patch you will need to apply the rebased dependency first.
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
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?
Attachment #8816278 - Flags: review?(pbrosset) → review+
(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.
Whiteboard: [papercut-mr]
Filter on Brobdingnagian.
Whiteboard: [papercut-mr] → [todo-mr]
Whiteboard: [todo-mr] → [todo-mr][t3]
Attachment #8816280 - Attachment is obsolete: true
Attachment #8816278 - Attachment is obsolete: true
Comment on attachment 8932528 [details]
Bug 1298370 - Add SameSite attribute to Cookie Inspector

https://reviewboard.mozilla.org/r/203576/#review209298
Attachment #8932528 - Flags: review?(pbrosset) → review+
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ff9b8f883b0c
Add SameSite attribute to Cookie Inspector r=pbro
https://hg.mozilla.org/mozilla-central/rev/ff9b8f883b0c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
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
Flags: needinfo?(mratcliffe)
(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
Flags: needinfo?(mratcliffe)
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.
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?
Flags: needinfo?(mratcliffe)
(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.
Flags: needinfo?(mratcliffe)
(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.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.