1605354 - Add support for `urls` to `Network.getCookies()`

Status: RESOLVED FIXED

(Remote Protocol :: CDP, enhancement, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

From the docs, the code in Chrom(ium), and by testing in a live session it's not clear what this parameter actually does. Might it be some kind of filtering, or adding additional urls to get the cookies from. When I tested with the chrome-remote-interface client the list of cookies is always the same for eg. http://heise.de.

We would need some feedback from Chrome engineers in how to handle this option.

Puppeteer is using it, so adding it to the reserved backlog.

Comment 1

[Andrey Kosyakov]

This is a filter, yes -- the host part of the URL is used as a key to retrieve the cookies for given host, and scheme and path are then used to match "secure" and "path" attributes in the returned cookies.
The retrieval/filtering logic is here: https://cs.chromium.org/chromium/src/net/cookies/cookie_monster.cc?rcl=3d246751a62e4b7dea22abb64eab73792b87c5ed&l=576
There are some puppeteer tests that cover cookie filtering by URL: https://github.com/puppeteer/puppeteer/blob/1de9906260269491c9011fcab33a58e280603fd3/test/cookies.spec.js#L121

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thank you. So to wrap-up and hopefully I understood that correctly:

1. If no domain filter is set the currently open page including all its iframes (would need bug 1605362 first) is traversed for collecting cookies
2. If a domain filter is set only the cookies for the given domains are getting retrieved, and the current page isn't obeyed at all

With this list of cookies:

1. Filter out the cookies which don't match the path of the URL / domain
2. Filter out the cookies which don't match the security flag

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

We should keep it as task given that it is one of the remaining patches needed to get most of the cookie tests passing.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Improved cookie handling will not be part of the beta2 milestone.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

CC'ing nafees87n on this bug given that he has shown interest to work on it.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Hi Nafees. It's been a while since we have heard last. I wonder if you are still interested in working on this bug? Thanks.

Comment 7

[nafees87n]

Hi Henrik
I got occupied in other work, hence could not work on this.
Today I got a chance to look at this. What I can conclude is that if no domain filter is present, the current flow is good enough else, I need to filter based on domain. Here I could not understand where I could get the URL to match the cookie's domain. I need help with that part.

Thanks

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Sure! So the urls are coming via an argument to the Network.getCookies command. That means you will have to add it after this line like:

  async getCookies(options = {}) {
    const { urls = default_urls } = options;

Whereby default_urls is the code that you recently added to get the URLs of all the browsing contexts in the current tree. You could move that code into an internal helper method (starting with an underscore) in the same file, similar to _buildCookie. Then we could just call this method.

When done please make sure to run again the cookie related tests of the Puppeteer unit tests. I'm fairly sure that some are going to pass now.

Lets me know if there are remaining questions which I could help with.

Comment 9

[nafees87n]

Attachment: Bug 1605354 - [remote] Support for Network.getCookies's "urls" parameter. r=whimboo

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Hi @nafees87n. I haven't heard from you for a while and wonder if I could help to get you going in case you are blocked. Please let me know if you are still interested to finish up this patch. Thanks!

Comment 11

[nafees87n]

Hi @whimboo Got a bit busy. Will submit the patch in a couple of days

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Hi nafees87n, I'm not sure if you didn't get the Phabricator emails for my review or if you are under time constraints again. Please le me know again if you are interested to continue on this patch. Thanks.

Comment 13

[nafees87n]

Hi whimboo
Apologies for the inconvenience. I am under time constraints. Hence couldn't get time to work on this. I'll try to complete this patch asap.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

No worries and I hope the back and forth is something you can still cope with. If not please really let me know and I will have to finish it off. Maybe it sounded easier at the beginning but getting all the required tests together is actually time consuming but also very helpful for a stable implementation of the feature. As such thanks a lot for still updating the patch! It's really appreciated!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/78e7be5c38c7
[remote] Support for Network.getCookies's "urls" parameter. r=webdriver-reviewers,whimboo

Comment 16

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/78e7be5c38c7

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks a lot Nafees87n for implementing this feature! And I assume that you have learned quit a bit. If you are interested and have time in the future please drop me a message and let me know what you are eager to learn. That way we can find something else for you.