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

Avoid creating cookie prepended with dot ('.') #25174

Closed
olivierboudet opened this issue Dec 15, 2022 · 36 comments · Fixed by #25761
Closed

Avoid creating cookie prepended with dot ('.') #25174

olivierboudet opened this issue Dec 15, 2022 · 36 comments · Fixed by #25761
Labels

Comments

@olivierboudet
Copy link

Current behavior

First, i have to tell that I am testing a company internal SSO implementation (based on Keycloak). I saw this comment (#1342 (comment)) on another issue which tells to test SSO with cy.request to simulate the authentication flow. In our case, we precisely want to test the flow, involving usage of multiple cookies with different domains, all set by Keycloak, not by cy.setCookie().

Current behavior:

When user is authenticated, Keycloak sets some cookies but cypress duplicates them with domain prefixed by a dot.
In the cypress console, we can see cookies set by cypress:

[
   
    {
        "name": "KEYCLOAK_IDENTITY",
        "value": "",
        "path": "/auth/realms/myrealm/",
        "domain": ".keycloak.local",
        "secure": false,
        "httpOnly": true,
        "sameSite": "lax"
    },
]

Keycloak set this one in the server response:

 {
        "name": "KEYCLOAK_IDENTITY",
        "value": "eyJhbGciOiJIUzI1NiIsInR5cCIgOiAiSldUIiwia2lkIiA6ICIxNzA2N2ZhNS03NjQ2LTRmNjUtOTBkZi1jYWE5NjJmZThjODcifQ.eyJleHAiOjE3MzE1NzU5NDQsImlhdCI6MTY3MTA5NTk0NCwianRpIjoiYmYyNGRhMDItZmU4MS00OWQ3LTk1MzgtMjU0NDk5NDQ5ZmFi......",
        "path": "/auth/realms/myrealm/",
        "domain": "keycloak.local",
        "secure": true,
        "httpOnly": true,
        "hostOnly": true,
        "sameSite": "no_restriction"
    },

Desired behavior

Cypress should allow to disable the automatic creation of new cookies prefixed by a dot.
At least, the value should not be empty.

Test code to reproduce

Too difficult to provide here, it needs a full setup of Keycloak with multiple applications to test SSO.

This issue is already detailed in other issues :

We have found a workaround which is to manually remove all cookies automatically set by Cypress, which is a very dirty hack:

When("user register", () => {
  cy.clickJsConsoleLogin();
  cy.registerRandomUser();
  cy.activateEmail();
  cy.clearCookie('KEYCLOAK_IDENTITY', {domain: Cypress.env('keycloakUrl').replace('https://', '.')})
});

Cypress Version

12.1.0

Node version

14.18.0

Operating System

Ubuntu 20.04

Debug Logs

No response

Other

No response

@verheyenkoen
Copy link
Contributor

Similar issue with a go-based-backend. The cypress intervention (likely by using document.cookie = '...') causes a second cookie with dot-prefix to be present which is also sent back-and-forth to the server and the server cannot deal with it, causing behavior you don't get without Cypress.

@AtofStryker AtofStryker self-assigned this Dec 15, 2022
@AtofStryker
Copy link
Contributor

Hi @olivierboudet . Thank you for opening an issue. Would you or @verheyenkoen be able to help provide me with a repro? I have an old reproduction repository I used for #22282 but that is only using cy.origin(). Would you be able to do something similar to this repro but taylor it to cy.request()?

@AtofStryker
Copy link
Contributor

I have been able to reproduce this. Is this what you are seeing?

@verheyenkoen
Copy link
Contributor

verheyenkoen commented Dec 15, 2022

@AtofStryker Yes, the cookies are duplicated with one of the domains being dot-prefixed. Not sure but could have to do with the httpOnly attribute being true for these cookies. Anyway, they are sent back to the sever with roundtrip and that is in some use cases causing problems.

This means you don't need a reproduction repo anymore?

@olivierboudet
Copy link
Author

In my case, I observe a similar thing but the value of new cookies are empty :

image

I think it would not be an issue if the value was not empty.

@AtofStryker AtofStryker added the Reproducible Can be reproduced label Dec 15, 2022
@AtofStryker
Copy link
Contributor

@verheyenkoen If the screenshot is the identical issue I don't think we need an additional reproduction, but can use the #22282 reproduction with Cypress 12.

@olivierboudet however it sounds like your issue might be slightly different. A reproduction would likely be useful here, but if not We can likely go off the salesforce reproduction I have

@Bertg
Copy link

Bertg commented Dec 19, 2022

I've seen similar issues on our solution. This is a Rack (ruby) based application.

So far we have only observed this in tests which use multiple domains. We actually have a test failure there, where a cookie does not get stored, even though present in the cookie headers. (We have had no time to file a decent bug report on this yet.)

I think both issues may be related to the changes introduced in Cypress 12. If I understand correctly quite some work was done to make cross-domain tests work better.

@olivierboudet
Copy link
Author

I think both issues may be related to the changes introduced in Cypress 12. If I understand correctly quite some work was done to make cross-domain tests work better.

Yes, we saw the issue just after upgraded to cypress 12 (from cypress 10)

@Bertg
Copy link

Bertg commented Dec 19, 2022

May also be related to #25205

@verheyenkoen
Copy link
Contributor

@AtofStryker Looks like the same issue indeed.

In our case it were new tests so can't say if this "bug" is new to Cypress 12 but our test case is also touching multiple subdomains.

@AtofStryker
Copy link
Contributor

We did significant cookie work for Cypress 12, including introducing new cookie commands and behaviors. My guess is this is an unintended side effect of those changes. Hopefully we can investigate soon!

@wulfovitch
Copy link

We have the same problem since updating to Cypress 12.
Cookies are created twice, one of them with a prefixed dot (".").
We are currently using the workaround mentioned above when interaction with cookies is needed:

cy.clearCookie('NAME_OF_THE_CORRESPONDING_COOKIE', { domain: "." + baseUrl });

I hope this bug will be resolved soon. This workaround is far from ideal.

@szymon-niedrygas
Copy link

The same behavior for us.
We tried to use cy.clearCookies({domain: .ourDomain.com}) but it doesn't work.
It's a huge discomfort for us so we're looking forward to a resolution.

@wulfovitch
Copy link

Clearing all cookies does not work for us either. You need to specify which cookie to delete and do at the same step as your app under test will delete the corresponding cookie without the dot ".".

@valscion
Copy link

valscion commented Jan 5, 2023

I've written a reproduction case here: https://github.com/valscion/cypress-cookie-issue-reproduction

The branch with Cypress v11.2.0 is passing:

while the main branch with Cypress v12.3.0 is erroring out because the spec asserts that only one cookie is set but there are two.

In that reproduction repository, we can see that we have two cookies being set with Cypress v12 where as v11.2 didn't set them. What's strange about the v12 behavior is that I needed to add a small timeout before reading document.cookie to see that there are more than one cookie being set. On page load there was only one cookie present but I wasn't able to figure out if that cookie was the one with the dot prefix or without it.

Cypress v12.3.0

cypress-12 3 0-cookies

Cypress v11.2.0

cypress-11 2 0-no-double-cookie

Manual steps

manual-steps

@Bertg
Copy link

Bertg commented Jan 5, 2023

@valscion Great effort, well done!

@valscion
Copy link

valscion commented Jan 5, 2023

Regarding this section from the comment by @verheyenkoen in #25174 (comment):

Not sure but could have to do with the httpOnly attribute being true for these cookies.

I was able to get the same broken behavior both with httpOnly and without it. The test I wrote just has to avoid using httpOnly as the assertions are done based on what document.cookie has on JS side.

cypress-cookie-issue-reproduction

@valscion
Copy link

valscion commented Jan 5, 2023

More on our context — we're upgrading from Cypress v11.2.0 to v12 and now we have a test where a message that should be shown using Ruby on Rails' Flash messages functionality that in our case works with special cookies.

However, with v12, we spot that there exists two cookies in the Cypress test, where now the one with the dot prefix is being used and the flash message is not being rendered as the application would render when not ran with Cypress:

venuu

This causes our test to fail because the UI isn't showing the "Reply sent!" message as we'd expect to happen.

venuu

Right now this is blocking us from upgrading to Cypress v12 as I can't think of a workaround for this situation.

@valscion
Copy link

Is there anything I could do to help with this issue? This is blocking us from upgrading to Cypress 12.x right now.

@AtofStryker
Copy link
Contributor

Hi @valscion. We are planning to start looking at this issue next sprint (in around 2ish weeks). If you want to get started early though, you might be able to debug some of the cookie code. My guess is it may not be the CDP Automation code as if I remember correctly this also happens in Firefox. I wonder if parsing the cookies on server has a different domain and sets them twice. These are all guesses as I haven't had time to look into the code in depth, but they might be great places to start!

@valscion
Copy link

Cool, thanks for the pointers! Let's see if I can figure out any extra details ☺️

@leonid-sviderskii-mtl
Copy link

I'd like to add a couple of cents too:
The issue happens with the csrf token cookie so whenever cypress needs to reload the page / navigate to another page, the API requests do fail due to csrf token mismatch error.

In my case, I have to delete both cookies before any move, as otherwise, I still have the mismatch issue.

@valscion
Copy link

if I remember correctly this also happens in Firefox.

Yes I was able to verify with my reproduction repository that this also happens with Firefox:

I updated the Cypress v11.2.0 baseline test to also run with Firefox and that also passes with Firefox:

@valscion
Copy link

valscion commented Jan 24, 2023

It looks like the issue is indeed somewhere in the CDP Automation code. These parts seem suspicious to me:

// without this logic, a cookie being set on 'foo.com' will only be set for 'foo.com', not other subdomains
export function isHostOnlyCookie (cookie) {
if (cookie.domain[0] === '.') return false
const parsedDomain = cors.parseDomain(cookie.domain)
// make every cookie non-hostOnly
// unless it's a top-level domain (localhost, ...) or IP address
return parsedDomain && parsedDomain.tld !== cookie.domain
}

Called in many places, one of them being here:

const normalizeGetCookieProps = (cookie: Protocol.Network.Cookie): CyCookie => {
if (cookie.expires === -1) {
// @ts-ignore
delete cookie.expires
}
if (isHostOnlyCookie(cookie)) {
// @ts-ignore
cookie.hostOnly = true
}


I tried to follow the code path on how we'd end up in those places but I didn't get that far. I also didn't have time today to figure out how I could try changing some code inside Cypress CDP Automation parts and then check if they'd fix my issue.

I described my journey here as well as added logs of things that seems to be suspicious to me: https://gist.github.com/valscion/d4f0bf080598d42603f807fba87b8b3a

@pionl
Copy link

pionl commented Jan 25, 2023

Hi, we have same issues with Laravel and XSRF-TOKEN. Deleting cookies did work for some test cases but still on some visits the session is restored and I cant delete it. Thank you :)

@AtofStryker
Copy link
Contributor

OK so I had some time to actually dig into this today and figure out what is going on. From what I can see, this is what is causing these duplicate cookies to show up with a prepended dot in the domain. But first, its probably good to document this cookie workflow for what is/supposed to happen in Cypress 12:

  • With the release of Cypress 12, Cypress stores certain cross-origin cookies in a server side cookie jar because the browser may not have permission to set these cookies due to Cypress running in an iframe. These cookies would otherwise be same origin, but in this case are not due to window.top and the Application under test being cross-origin. These cookies are added in the proxy layer of Cypress in the response-middleware
  • When these simulated cookies are added, they are sent down to the client to what we call a "cross-origin spec bridge" to sync cookie values that should be there into document.cookie and other resources. It's important to note that with Cypress 12, fullCrossOrigin injection might be used on a cross-origin html request, even if you aren't leveraging cy.origin().
  • When these cookies make it to the "spec-bridge", cookies are patched and then synced through the automation client with the subsequent middleware being released.
  • These simulated cookies, assuming they aren't added to the browser previously, make it up to the automation client to be preprocessed before being sent to the appropriate automation client.
  • These cookies are then sent to the appropriate automation client (in this case CDP for chromium) to go through a similar process. If the cookie is not considered a host only cookie, the dot is prepended onto the domain name here (which is the suspicion @valscion pointed out here). The return value from this process is ultimately setting cookies these cookies via CDP.

This is a pretty complex workflow that takes a bit to put together, but it's not entirely necessary to understanding the problem. So what is actually happening?

From what I can see, some of the cookies we are setting in the server side cookie jar are actually cookies that requests have the ability to set in the browser, either through SameSite=None attributes or top level redirects that allow SameSite=Lax cookie setting. These cookies are getting set in the server side cookie jar, AND the browser. When the duplicated cookies set in the jar make it through the automation client, the hostOnly prepended dot is applied, which is really just a side effect of this incorrect behavior. If you are able to run the cypress dev environment locally, if you comment out this line the duplicate cookies should dissappear.

This explains why the regression appeared in v12, and is happening for those who do not even use cy.origin().

I am still thinking of solutions, but one way we might be able to fix this is to be a bit smarter about what cookies we set in the serverside cookie jar, filtering out any values we know will actually be set by the browser to avoid any type of duplication or side effects. I don't think this solution is super involved and is something we can unit test fairly easily,

@valscion
Copy link

valscion commented Feb 1, 2023

Wow thanks for the explanation @AtofStryker, this functionality is quite interesting indeed!

Let me know if you need help with verifying some fixes or change ideas and I can take them for a spin also in our actual application and not just the reproduction repository.

If you are able to run the cypress dev environment locally, if you comment out this line the duplicate cookies should dissappear.

Would it be useful if I test this out myself?

@AtofStryker
Copy link
Contributor

Would it be useful if I test this out myself?

@valscion I would say so just to confirm the duplicate cookie issues resolve. Others can also feel free to try it!

@AtofStryker
Copy link
Contributor

Seems to be related to #25205

@AtofStryker
Copy link
Contributor

Anyone willing to try this binary to see if it resolves the prepended dot issues?

@leonid-sviderskii-mtl
Copy link

leonid-sviderskii-mtl commented Feb 14, 2023

I tested the darwin-x64 version on macOS with Chrome v110.

I removed the code block with the csrf cookie removal and I confirm that the test fails on 12.3 but passes on 12.6 and I see only one cookie set for the domain without . So for me, it's a success 🎉

@AtofStryker
Copy link
Contributor

This should be making it in today or tomorrow with 12.6.1. Hang tight!

@valscion
Copy link

Anyone willing to try this binary to see if it resolves the prepended dot issues?

I also verified that this binary did fix my reproduction repository locally: https://github.com/valscion/cypress-cookie-issue-reproduction

@ankorstore-haddowg
Copy link

This should be making it in today or tomorrow with 12.6.1. Hang tight!

Any update on this patch? Just upgraded from v10 only to be scuppered by this issue.

@AtofStryker
Copy link
Contributor

@ankorstore-haddowg Our patch release was delayed with an unknown ETA. My best guess at the moment is we will be releasing this week, if not worse case a week from today.

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Feb 25, 2023

Released in 12.7.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v12.7.0, please open a new issue.

@cypress-bot cypress-bot bot removed the stage: investigating Someone from Cypress is looking into this label Feb 25, 2023
@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Feb 25, 2023
@AtofStryker AtofStryker removed their assignment Mar 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.