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

feat: same origin spec bridges #23885

Merged
merged 11 commits into from
Oct 4, 2022
Merged

Conversation

AtofStryker
Copy link
Contributor

@AtofStryker AtofStryker commented Sep 19, 2022

User facing changelog

cy.origin's url argument must be an exact origin match to the origin of the AUT

Additional details

In order to support google authentication within cy.origin and eliminate inconsistencies, we are moving away from same super domain origin spec bridge to strictly same origin spec bridges. This solves a few issues we have been running into with origin policy and introduces a breaking change into

Steps to test

How has the user experience changed?

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)
  • Has a PR for user-facing changes been opened in cypress-documentation? After looking at the documentation, all of our examples actually use same origin spec bridges 😄
  • Have API changes been updated in the type definitions?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Sep 19, 2022

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Sep 19, 2022



Test summary

4121 0 350 0Flakiness 0


Run details

Project cypress
Status Passed
Commit 15d2ada
Started Oct 4, 2022 10:08 PM
Ended Oct 4, 2022 10:20 PM
Duration 12:05 💡
OS Linux Debian - 11.4
Browser Electron 106

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

@AtofStryker AtofStryker force-pushed the proposal/same-origin-spec-bridges branch from c07c596 to ee0da74 Compare September 19, 2022 21:14
@AtofStryker AtofStryker changed the title Proposal/same origin spec bridges Feat: same origin spec bridges Sep 19, 2022
@AtofStryker AtofStryker force-pushed the proposal/same-origin-spec-bridges branch from ee0da74 to 1fd4699 Compare September 19, 2022 22:17
@AtofStryker AtofStryker changed the title Feat: same origin spec bridges feat: same origin spec bridges Sep 19, 2022
@AtofStryker AtofStryker force-pushed the proposal/same-origin-spec-bridges branch 4 times, most recently from 0a1086b to 310d864 Compare September 20, 2022 19:37
Base automatically changed from chore/refactor-origin-policy-and-add-utils to feature/simulated-top-cookie-handling September 21, 2022 22:27
@AtofStryker AtofStryker changed the base branch from feature/simulated-top-cookie-handling to feat/implement-simulated-top-req-res-middleware September 22, 2022 15:38
@AtofStryker AtofStryker force-pushed the proposal/same-origin-spec-bridges branch 3 times, most recently from 406d74d to d9c583b Compare September 22, 2022 16:49
Base automatically changed from feat/implement-simulated-top-req-res-middleware to feature/simulated-top-cookie-handling September 23, 2022 14:04
chore: refactor spec bridges to strictly enforce same origin

fix: wrap fullCrossOrigin injection around feature flag inside buffered response
@@ -296,17 +297,20 @@ const createApp = (port) => {
})

app.get('/test-request-credentials', (req, res) => {
const origin = cors.getOrigin(req['headers']['referer'])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

since many of the requests are now same origin, we don't get the origin header, so we grab it from the referrer for the cookie_behavior tests

@@ -207,7 +206,7 @@ const attachToWindow = (autWindow: Window) => {

cy.isStable(false, 'beforeunload')

cy.Cookies.setInitial()
// NOTE: we intentionally do not set the cy.Cookies.setInitial() inside the spec bridge as we are not doing full injection and this leads to cookie side effects
Copy link
Contributor

Choose a reason for hiding this comment

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

What side effects was this causing? Is this a separate issue from "same origin spec bridges" or was it exposed during these code changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Main issue is sometimes setting a cookie on the domain that isn't cleared in firefox that we don't even need because we don't do full injection. I would say it was always an issue and was exposed more so with these changes.

example within firefox before:
spec bridge was creating a __cypress.initial cookie on foobar.com domain (not .foobar.com)
AUT is at www.foobar.com and makes requests. __cypress.initial cookie isn't attached because it doesn't fit the domain policy (foobar.com vs .foobar.com)
Screen Shot 2022-10-04 at 10 58 22 AM

example within firefox after:
spec bridge was creating a __cypress.initial cookie on www.foobar.com domain (www.foobar.com)
AUT is at www.foobar.com and makes requests. __cypress.initial cookie is attached and is never cleared
Screen Shot 2022-10-04 at 11 08 03 AM

Not sure if this is an underlying issue with js-cookie, but the domain that is set in firefox for the cookie is different than that of chromium browsers. We don't see it in chromium browsers because the domain set in the spec bridge is localhost
Screen Shot 2022-10-04 at 10 44 17 AM

Copy link
Contributor

@chrisbreiding chrisbreiding left a comment

Choose a reason for hiding this comment

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

Looks good! But after viewing all these diffs, I now having this burned into my mind 😜

www

@AtofStryker
Copy link
Contributor Author

Looks good! But after viewing all these diffs, I now having this burned into my mind 😜

www www

How else would you know it's the World Wide Web? 😆

@AtofStryker AtofStryker merged commit 695dd27 into develop Oct 4, 2022
@AtofStryker AtofStryker deleted the proposal/same-origin-spec-bridges branch October 4, 2022 22:26
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Oct 11, 2022

Released in 10.10.0.

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

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Oct 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.