François Marier [:francois] Reporter
	Description • 6 years ago

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.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 1 • 6 years ago

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

	Daniel Veditz [:dveditz]
	Comment 2 • 6 years ago

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.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 3 • 6 years ago

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

	Vincent Lequertier Assignee
	Comment 4 • 6 years ago

I will work on that :-)

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 5 • 6 years ago

Great!

Let me know if you have any questions.
Honza

	Vincent Lequertier Assignee
	Comment 6 • 6 years ago

Thanks

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 8 • 6 years ago
mozreview-review

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?

	Vincent Lequertier Assignee
	Comment 10 • 6 years ago
mozreview-review-reply

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!

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 11 • 6 years ago
mozreview-review

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

	Vincent Lequertier Assignee
	Comment 12 • 6 years ago

And thank you for the review!

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

	Vincent Lequertier Assignee
	Comment 13 • 6 years ago

Everything seems good

	Mozilla Autoland
	Comment 14 • 6 years ago

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)

	Vincent Lequertier Assignee
	Comment 16 • 6 years ago

Rebased the commit on top of latest m-c

	Pulsebot
	Comment 17 • 6 years ago

Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/782b1ae19860
Add support for same-site cookie attribute to "Cookies" netmonitor side-panel; r=Honza

	Andrei Ciure[:aciure]
	Comment 18 • 6 years ago
bugherder

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

	Chris Mills [:cmills]
	Comment 19 • 6 years ago

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!

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 20 • 6 years ago

Looks great to me thanks Chris!

Honza