Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: return non-secure cookies with HTTPS URLs #5507

Merged
merged 3 commits into from
Feb 19, 2021
Merged

fix: return non-secure cookies with HTTPS URLs #5507

merged 3 commits into from
Feb 19, 2021

Conversation

zevisert
Copy link
Contributor

Cookies have a "Secure" attribute which tells the browsers that a given cookie should only be sent via HTTPS. In it's absense "Secure" is falsy and these cookies should be sent with both HTTP and HTTPS requests. Playwright now returns only the "Non-Secure" cookies for HTTP URLs, and both "Secure" and "Non-Secure" cookies for HTTPS URLs.

Fixes #5504

Cookies have a "Secure" attribute which tells the browsers
that a given cookie should only be sent via HTTPS. In it's
absense "Secure" is falsy and these cookies should be sent
with both HTTP and HTTPS requests. Playwright now returns
only the "Non-Secure" cookies for HTTP URLs, and both
"Secure" and "Non-Secure" cookies for HTTPS URLs.

Fixes #5504
@zevisert
Copy link
Contributor Author

I didn't end up changing the API docs for this fix; under BrowserContext.cookies we currently have

If no URLs are specified, this method returns all cookies. If URLs are specified, only cookies that affect those URLs are returned.

And that seems appropriate still. I thought about adding another sentence there, along the lines of...

If no URLs are specified, this method returns all cookies. If URLs are specified, only cookies that affect those URLs are returned. Secure cookies are only returned for HTTPS URLs, while non-secure cookies are returned for both HTTP and HTTPS URLs.

But that seems like part of "only cookies that affect those URLs". Let me know what you think!

Some tests in //tests/browsercontext-cookies.spec.ts
used a mix of Array.sort and str.localeCompare. This
comes from a suggestion from @mxschmitt.

See #5507
@zevisert
Copy link
Contributor Author

Oh darn, that last commit should have read chore: use sets for group equality in cookie tests, not in context text 🤦. If it's a big deal, let me know and I can rebase

src/server/network.ts Outdated Show resolved Hide resolved
@aslushnikov aslushnikov merged commit cbcc609 into microsoft:master Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unsecure cookies are not returned from BrowserContext.cookies(...) for HTTPS urls
3 participants