CDP Cookies #777
CDP Cookies #777
Conversation
sjorsdonkers
force-pushed
the
cookies
branch
2 times, most recently
from
a9b9b23 to
14c07ee
Compare
| try w.write(cookie.value); | ||
|
|
||
| try w.objectField("domain"); | ||
| try w.write(cookie.domain); // Should we hide a leading dot? |
There was a problem hiding this comment.
We probably, should. I'm not sure if it matters.
To support it we probably need to store the domain with a flag whether it came from url or was set explicitly.
src/cdp/domains/storage.zig
Outdated
| if (self.urls) |urls| { | ||
| for (self.cookies) |*cookie| { | ||
| for (urls) |*url| { | ||
| if (cookie.appliesTo(url, false, false)) { // TBD same_site, should we compare to the pages url? |
There was a problem hiding this comment.
When getting cookies we can only provide a list of urls, not other configuration (or get all cookies).
We probably should not filter out cookies that are not related to the current page, also because this is not part of the Page api.
There was a problem hiding this comment.
Note also that Network.getCookies will default to the Pages url if no urls are provided.
same_site and navigation are hard coded to true now
| // ignore it and use the "default-path" algorithm | ||
| if (explicit_path) |path| { | ||
| if (path.len > 0 and path[0] == '/') { | ||
| return try arena.dupe(u8, path); |
There was a problem hiding this comment.
Should we make sure it ends with a /?
Same as when we extract it from the URL
Else, should we take this difference into account when comparing cookie paths
| const c_no_dot = if (std.mem.startsWith(u8, cookie.domain, ".")) cookie.domain[1..] else cookie.domain; | ||
| const d_no_dot = if (std.mem.startsWith(u8, domain_, ".")) domain_[1..] else domain_; | ||
| if (!std.mem.eql(u8, c_no_dot, d_no_dot)) return false; |
There was a problem hiding this comment.
Related to the other domain . prefix comment.
Is this the right way to compare domains for the delete scenario?
What about for detection duplicate cookies?
| } | ||
| } | ||
| bc.session.cookie_jar.removeExpired(null); | ||
| const writer = CookieWriter{ .cookies = bc.session.cookie_jar.cookies.items }; |
There was a problem hiding this comment.
This doesn't seem right. Storge.getCookies has the unhelpful description of "Returns all browser cookies." I don't know what "all browser cookies" means, but this behavior is different than Chrome.
If you run:
await page.goto('https://www.google.com/');
console.log(await page.cookies()); // Network.getCookies
console.log(await browser.cookies()); // Storage.getCookiesThen in Chrome, this will return 2 cookies for the first one, and 0 for the second. But with this implementation, it returns 2 cookies for both. I wonder if "brower.cookies", aka Storage.getCookies is only those cookies set directly via Storage.setCookies? Either need to figure out how this is implemented in Chrome, or to play with more permutations to figure it...I tried to see if puppeteer had an explanation about the difference, but also nothing.
There was a problem hiding this comment.
browser.cookies()
{"method":"Storage.getCookies","params":{},"id":29}
0 results
context.cookies()
{"method":"Storage.getCookies","params":{"browserContextId":"F84F263F9B18E886D63394D299BF5B59"},"id":30}
2 results
The problem is that when we call browser.createBrowserContext(); we set it as the default and only context.
Chrome however already has a default context, so this will create a 2nd one.
It you replace:
const context = await browser.createBrowserContext();
With:
const context = await browser.defaultBrowserContext();
(and not close that context!)
Storage.getCookies returns 2 results as expected.
This is an issue we need to solve, but not in this PR as the implementation of using the default context is correct. It's just that the default context is incorrect.
Issue created: #783
| // NOTE: The param.url can affect the default domain, (NOT path), secure, source port, and source scheme. | ||
| const uri = if (param.url) |url| std.Uri.parse(url) catch return error.InvalidParams else null; | ||
| const uri_ptr = if (uri) |*u| u else null; | ||
| const domain = try Cookie.parseDomain(a, uri_ptr, param.domain); |
There was a problem hiding this comment.
Probably doesn't matter, since it's weird user-input, but if the domain value is 'htttps://lightpanda.io' (i.e. not a domain, which is why I think we can ignore it), this handles it differently than Chrome. Chrome seems like it'll extract the domain correctly, whereas we'll treat the entire input as a domain as-is.
There was a problem hiding this comment.
I'm not able to reproduce this one, for me Chrome sets http as the domain, which I think we can consider equally useless. (regardless of how many ts)
Network.setCookie
Network.setCookies
deprecated
Network.getAllCookiesNetwork.deleteCookies
Network.getCookies
Network.clearBrowserCookies
deprecated
Network.canClearBrowserCookiesdeprecated
Page.deleteCookieStorage.getCookies
Storage.setCookies
Storage.clearCookies
NOTE:
We currently do not support all cookie parameters, this PR only sets and uses the existing ones.
Existing cookies behavior has been modified as follows:
Puppeteer test: lightpanda-io/demo#51