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: Make cross-origin document.cookie work #22594

Merged
merged 9 commits into from Jun 30, 2022

Conversation

chrisbreiding
Copy link
Contributor

@chrisbreiding chrisbreiding commented Jun 29, 2022

User facing changelog

  • Fixed how document.cookie works when testing multiple origins

How has the user experience changed?

Previously, some authentication providers that rely on document.cookie would not function correctly because it behaves differently when used in an iframe (the AUT) that has a different origin than top. This PR fixes issues with document.cookie, making it behave as if the user's app is being run in top.

PR Tasks

  • Have tests been added/updated?
  • Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • N/A Has a PR for user-facing changes been opened in cypress-documentation?
  • N/A Have API changes been updated in the type definitions?

@chrisbreiding chrisbreiding requested a review from a team as a code owner June 29, 2022 15:02
@chrisbreiding chrisbreiding requested review from mschile and removed request for a team June 29, 2022 15:02
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jun 29, 2022

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Jun 29, 2022



Test summary

37717 0 456 0Flakiness 9


Run details

Project cypress
Status Passed
Commit 68eb57d
Started Jun 30, 2022 5:17 PM
Ended Jun 30, 2022 5:35 PM
Duration 18:15 💡
OS Linux Debian - 10.11
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

commands/xhr.cy.js Flakiness
1 ... > logs request + response headers
2 ... > logs request + response headers
3 ... > logs Method, Status, URL, and XHR
4 ... > logs response
cypress/proxy-logging.cy.ts Flakiness
1 Proxy Logging > request logging > xhr log has response body/status code when xhr response is logged first
This comment includes only the first 5 flaky tests. See all 9 flaky tests in the 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

done()
})

cy.get('[data-cy="welcome"]').as('welcome_button')
cy.get('[data-cy="cross-origin-secondary-link"]').as('link')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the element with data-cy="welcome" since it wasn't referenced elsewhere, so I had to update this test.

Copy link
Contributor

@AtofStryker AtofStryker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified this works with #22568 to support microsoftonline and login.live.com which I think closes #21307 🎉

packages/runner/injection/cookies.js Show resolved Hide resolved
packages/runner/injection/cookies.js Outdated Show resolved Hide resolved
packages/runner/injection/cookies.js Show resolved Hide resolved
@@ -40,6 +41,8 @@ const findCypress = () => {

const Cypress = findCypress()

patchDocumentCookie(Cypress)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be valuable to patch this in a non-cy.origin use case? It seems like it might be if it makes document.cookie behave more like as if the AUT is top.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's only an issue if the top origin doesn't match the AUT origin, so it's not necessary in other cases.


// fixes tough-cookie defaulting undefined/invalid SameSite to 'none'
// https://github.com/salesforce/tough-cookie/issues/191
const hasUnspecifiedSameSite = toughCookie.sameSite === 'none' && !sameSiteNoneRe.test(cookie)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider contributing this fix to toughCookie? Is this something other people would want too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like they already have a PR to fix it: salesforce/tough-cookie#240

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.

10.3 broke support for oauth flow with Spotify Handle cross-origin document.cookie
3 participants