Closed Bug 1452715 Opened 2 years ago Closed Last year

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

Categories

(DevTools :: Netmonitor, enhancement, P3)

enhancement

Tracking

(firefox62 fixed)

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: francois, Assigned: vi.le, Mentored)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete, good-first-bug)

Attachments

(2 files)

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.
Severity: normal → enhancement
Priority: -- → P3
Attached image 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
Mentor: odvarko
Has STR: --- → yes
Keywords: good-first-bug
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.
Summary: Add support for same-site cookie attribute → Add support for same-site cookie attribute to "Cookies" side-panel
I will work on that :-)
Assignee: nobody → vi.le
Status: NEW → ASSIGNED
Great!

Let me know if you have any questions.
Honza
Thanks
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?
Attachment #8973466 - Flags: review?(odvarko)
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 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
Attachment #8973466 - Flags: review?(odvarko) → review+
Everything seems good
Keywords: checkin-needed
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)
Rebased the commit on top of latest m-c
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
https://hg.mozilla.org/mozilla-central/rev/782b1ae19860
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Product: Firefox → DevTools
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!
Flags: needinfo?(odvarko)
Looks great to me thanks Chris!

Honza
Flags: needinfo?(odvarko)
You need to log in before you can comment on or make changes to this bug.