1452715 - Add support for same-site cookie attribute to "Cookies" side-panel

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P3)

Description

[François Marier [:francois]]

We are enabling support for the SameSite cookie attribute (values: Strict, Lax) in Firefox.

It would be nice if the cookie panel of the network tab would show the value of this attribute if it's present.

To test this, log into Github and look at the `__Host-user_session_same_site` cookie.

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: cookie-panel.png

Thanks for reporting this!

Detailed information for anyone interested in fixing this bug:

1) The Network panel is displaying list of cookies is a side panel called "Cookies" (see the attached screenshot). This side panel is implemented as React component: CookiesPanel
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/CookiesPanel.js

2) The screenshot shows also `__Host-user_session_same_site` cookies with attributes like: httpOnly, path, secure, etc. But, the new "SameSite" attribute is missing -> BUG

3) HTTP SetCookie header is parsed using `parseSetCookieHeader` helper:
https://searchfox.org/mozilla-central/rev/b55e1a1cbcaee34878e133fbac20c4c2af6e11b5/devtools/shared/webconsole/network-helper.js#358

This method doesn't support the new attribute "SameSite". This is the place that needs to be fixed.

4) This also needs a test to avoid regression.
There is already a test verifying cookies sorting:
devtools\client\netmonitor\test\browser_net_cookies_sorted.js

This test is loading a helper server side file (*.sjs) that is used to set Set-Cookie HTTP header.
devtools/client/netmonitor/test/sjs_simple-unsorted-cookies-test-server.sjs

The new test will be very similar:

1) Initialize Network monitor: `initNetMonitor(THE_NEW_SJS_FILE_WITH_CUSTOM_COOKIE);`
2) Waiting for page to be loaded `waitForNetworkEvents`
3) Selecting the Cookies panel (simulating mouse click): `EventUtils.sendMouseEvent({ type: "click" }, ...`
4) Verifying that `SameSite` value is there: `let labelCells = document.querySelectorAll(".treeLabelCell");` ...

---

STR:

1) Open DevTools Toolbox, select the Network panel
2) Load github.com, login
3) Select the first document request
4) Select the Cookies side panel and check `__Host-user_session_same_site`
5) `SameSite` attribute is missing -> BUG

---- 

Spec:
https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-02#section-5.3.7

Honza

Comment 2

[Daniel Veditz [:dveditz]]

Note that the Storage tool displays the same-site status (added in bug 1298370) so devtools does know this somewhere. It's just missing on the Network details side-panel.

Comment 3

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

See https://searchfox.org/mozilla-central/source/devtools/server/actors/storage.js#575 for the storage inspector's implementation.

Comment 4

[Vincent Lequertier]

I will work on that :-)

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Great!

Let me know if you have any questions.
Honza

Comment 6

[Vincent Lequertier]

Thanks

Comment 7

[Vincent Lequertier]

Attachment: Bug 1452715 - Add support for same-site cookie attribute to "Cookies" netmonitor side-panel;

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

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8973466 [details]
Bug 1452715 - Add support for same-site cookie attribute to "Cookies" netmonitor side-panel;

https://reviewboard.mozilla.org/r/241854/#review247858

Thanks for the patch, looks great to me!

Just one inline comment.

Honza

::: devtools/client/netmonitor/test/browser_net_set-cookie-same-site.js:28
(Diff revision 1)
> +  EventUtils.sendMouseEvent({ type: "mousedown" },
> +    document.querySelectorAll(".request-list-item")[0]);
> +  await wait;
> +
> +  EventUtils.sendMouseEvent({ type: "mousedown" },
> +    document.querySelectorAll(".request-list-item")[0]);

Why you sending 'mousedown' to the first reqest item again?

Comment 9

[Vincent Lequertier]

Comment on attachment 8973466 [details]
Bug 1452715 - Add support for same-site cookie attribute to "Cookies" netmonitor side-panel;

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

Comment 10

[Vincent Lequertier]

Comment on attachment 8973466 [details]
Bug 1452715 - Add support for same-site cookie attribute to "Cookies" netmonitor side-panel;

https://reviewboard.mozilla.org/r/241854/#review247858

> Why you sending 'mousedown' to the first reqest item again?

That was a mistake, sorry!

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8973466 [details]
Bug 1452715 - Add support for same-site cookie attribute to "Cookies" netmonitor side-panel;

https://reviewboard.mozilla.org/r/241854/#review248174

Looks good to me now, thanks for the patch!

R+

Honza

Comment 12

[Vincent Lequertier]

And thank you for the review!

Try push here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=512270eb189566b391f6873de19344a200dd658c

Comment 13

[Vincent Lequertier]

Everything seems good

Comment 14

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s db06a860d50f616c73df5eea1354613dea05d7c6 -d 31b295f557db: rebasing 463063:db06a860d50f "Bug 1452715 - Add support for same-site cookie attribute to "Cookies" netmonitor side-panel; r=Honza" (tip)
merging devtools/client/netmonitor/test/browser.ini
merging devtools/client/netmonitor/test/head.js
warning: conflicts while merging devtools/client/netmonitor/test/head.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 15

[Vincent Lequertier]

Comment on attachment 8973466 [details]
Bug 1452715 - Add support for same-site cookie attribute to "Cookies" netmonitor side-panel;

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

Comment 16

[Vincent Lequertier]

Rebased the commit on top of latest m-c

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/782b1ae19860
Add support for same-site cookie attribute to "Cookies" netmonitor side-panel; r=Honza

Comment 18

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/782b1ae19860

Comment 19

[Chris Mills [:cmills]]

I have added an extra screenshot and some information about the new attribute being shown in devtools:
https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Cookies

And a note in the Fx62 rel notes:
https://developer.mozilla.org/en-US/Firefox/Releases/62#Developer_tools

Let me know if that looks OK. Thanks!

Comment 20

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Looks great to me thanks Chris!

Honza