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

Cypress incorrectly validates domain of cookies in 3.5.0 #5656

Closed
ghost opened this issue Nov 11, 2019 · 8 comments · Fixed by #5657 or #5862 · May be fixed by ngChile/ngx-devkit-cypress-builder#20 or qsays/grafana#1
Closed

Cypress incorrectly validates domain of cookies in 3.5.0 #5656

ghost opened this issue Nov 11, 2019 · 8 comments · Fixed by #5657 or #5862 · May be fixed by ngChile/ngx-devkit-cypress-builder#20 or qsays/grafana#1
Assignees
Labels
topic: cookies 🍪 type: regression A bug that didn't appear until a specific Cy version release v3.5.0 🐛 Issue present since 3.5.0

Comments

@ghost
Copy link

ghost commented Nov 11, 2019

Current behavior:

Cookies are failing to set for subdomains during cy.request.
E.g. we call /auth of auth.test.server and it returns token cookie with .test.server. In this case cookie is not set.

Desired behavior:

Cookie is set in above example.

Steps to reproduce: (app code and test code)

Look at packages/server/lib/request.coffee#setCookiesOnBrowser:

return if not tough.domainMatch(cookie.domain, parsedUrl.hostname)

And at tough-cookie documentation:

domainMatch(str,domStr[,canonicalize=true])
Answers "does this real domain match the domain in a cookie?". The str is the "current" domain-name and the domStr is the "cookie" domain-name. Matches according to RFC6265 Section 5.1.3, but it helps to think of it as a "suffix match".

So it should be:

return if not tough.domainMatch(parsedUrl.hostname, cookie.domain)

Versions

>=3.5.0

@cypress-bot cypress-bot bot added stage: work in progress stage: needs review The PR code is done & tested, needs review and removed stage: work in progress labels Nov 11, 2019
@flotwig flotwig self-assigned this Nov 19, 2019
@cypress-bot cypress-bot bot added stage: work in progress and removed stage: needs review The PR code is done & tested, needs review labels Nov 19, 2019
@cypress-bot cypress-bot bot added stage: needs review The PR code is done & tested, needs review and removed stage: work in progress labels Nov 20, 2019
@tozes
Copy link

tozes commented Nov 25, 2019

+1, as discussed here, we have exactly the same issue which is stopping us for upgrading from 3.4.1

@fernandopasik
Copy link

@tozes your link was broken
#5688 (comment)

@cypress-bot cypress-bot bot added stage: ready for work The issue is reproducible and in scope and removed stage: needs review The PR code is done & tested, needs review labels Nov 26, 2019
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Nov 26, 2019

The code for this is done in cypress-io/cypress#5657, but has yet to be released.
We'll update this issue and reference the changelog when it's released.

@cypress-bot cypress-bot bot removed the stage: ready for work The issue is reproducible and in scope label Nov 26, 2019
@jennifer-shehane jennifer-shehane added the type: regression A bug that didn't appear until a specific Cy version release label Nov 27, 2019
@jennifer-shehane jennifer-shehane changed the title Cypress incorrectly validates domain of cookies Cypress incorrectly validates domain of cookies in 3.5.0 Nov 27, 2019
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Nov 27, 2019

Released in 3.7.0.

@ghost
Copy link
Author

ghost commented Nov 28, 2019

@jennifer-shehane , @flotwig , @brian-mann,

Shame on my guys, I've missed another incorrect usage of domainMatch in cypress here: https://github.com/cypress-io/cypress/blob/develop/packages/server/lib/browsers/cdp_automation.ts#L24

Which basically means that this issue is only partially resolved in #5657. Unfortunately looks like we have incorrect tests for that code as well: #5816.

I'm not sure if I will be able to pick this up soon enough. Could please one of you reopen this one and do the fix?

@tozes FYI

@flotwig
Copy link
Contributor

flotwig commented Dec 3, 2019

@Donotello That's my bad, I noticed that the tests passed without that patch so I left it out. I forgot to go back and double-check it against tough-cookie's documentation, but I believe you're correct. I'll open a PR.

@flotwig flotwig reopened this Dec 3, 2019
@cypress-bot cypress-bot bot added stage: work in progress stage: needs review The PR code is done & tested, needs review labels Dec 3, 2019
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Dec 6, 2019

The code for this is done in cypress-io/cypress#5862, but has yet to be released.
We'll update this issue and reference the changelog when it's released.

@cypress-bot cypress-bot bot removed the stage: needs review The PR code is done & tested, needs review label Dec 6, 2019
@jennifer-shehane jennifer-shehane added the v3.5.0 🐛 Issue present since 3.5.0 label Dec 10, 2019
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Dec 12, 2019

Released in 3.8.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment