fix: Improve document.cookie patch #23643
Conversation
|
Thanks for taking the time to open a PR!
|
|
Test summaryRun details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
chrisbreiding
changed the title
There was a problem hiding this comment.
any idea if this fixes the issues inside the cookie_behavior tests with trying to get/set cookies inside cy.origin using the cypress api?
I think we might need to change the patch a little bit to work with cookies of the same key, but different domains/paths (which are apparently more common than I would have thought), but I think there is something we can try that I suggested below!
| }) | ||
|
|
||
| Cypress.on('clear:cookies', () => { | ||
| reset() |
There was a problem hiding this comment.
I wonder if we still need to do this if we store the reference to get:cookies, or just clear the reference out?
There was a problem hiding this comment.
This is to optimistically clear out the cookies when a user calls cy.clearCookies() so there's no delay/race condition waiting to sync with the automation cookies. There's actually no longer any polling to sync with automation cookies. It just caused issues and now all cases of cookies changing in automation are taken care of in a more deterministic fashion
|
Going to move this back into draft until after #23297 is merged, since it's going to need to be heavily refactored after that. |
| cookieJar.removeAllCookies() | ||
| } | ||
|
|
||
| const bindCypressListeners = (Cypress: Cypress.Cypress) => { |
There was a problem hiding this comment.
Do we need to listen for cookies changes even before a cypress instance attaches?
for example, would the following test work? (I haven't run this so it probably doesn't work without modifications)
it('might need to listen without a cypress instance', () => {
visit('localhost')
visit('http://www.foobar.com:3500')
cy.setCookie('foo', 'bar', {domain: foobar.com})
cy.origin('foobar.com:3500', () => {
cy.window((win) => {
expect(win.document.cookies).to.eq('foo=bar')
}
})
}There was a problem hiding this comment.
Currently that fails with The command was expected to run against origin http://localhost:3500 but the application is at origin http://www.foobar.com:3500. I think it makes sense that the user should put it inside the origin callback in that case and then the listeners will be set up as well.
There was a problem hiding this comment.
I think we may want to revisit throwing that error:
The command was expected to run against origin http://localhost:3500 but the application is at origin http://www.foobar.com:3500
I added the limitation that cy.cookies could only be run when the aut was same-site as top, I don't think thats correct anymore since we will be allowing cy.cookies to be set on any domain (i think it can be done now?) and we should probably think about changing this behaviour in #24264
| // We monkey-patch document.cookie when in a cross-origin injection, because | ||
| // document.cookie runs into cross-origin restrictions when the AUT is on | ||
| // a different origin than top. The goal is to make it act like it would | ||
| // if the user's app was run in top. |
There was a problem hiding this comment.
In the case outside of cy.origin where we are same-site but cross-origin, would we want to apply this patch?
We may need to check on this for #24275
There was a problem hiding this comment.
Good to keep in mind. I'll defer to Bill on that one.
There was a problem hiding this comment.
I believe so, at least for hostname specific cookies
There was a problem hiding this comment.
Do we know if this fixes some of cookie assertions in cookie_behavior.cy.ts? I think most of those are fixed with #24264?
| httpOnly: false, | ||
| maxAge: null, | ||
| path: null, | ||
| sameSite: 'lax', |
There was a problem hiding this comment.
I think so. Does this get updated to the known values once the automation cookies sync in?
I don't think so because this only fixes timing issues with |
User facing changelog
document.cookiewould not reflect the correct value in cross-origin testsAdditional details
Previously, the document.cookie patch would have to wait ~250ms to be up-to-date with cookies stored in the browser because it polls automation for them. This could result in document.cookie not being accurate if a page read its value on page load.
This fixes the issue by updating the document.cookie patch's value before the page loads by sending the cookies down from the proxy when the response is intercepted. This makes the correct value available on page load.
PR Tasks
cypress-documentation?type definitions?