fix: Improve cross-origin cookie handling #22320

chrisbreiding · 2022-06-14T19:50:10Z

User facing changelog

Fixed various issues with cookies when testing across multiple origins

Additional details

Previously, we worked around handling cross-origin cookie limitations by forcing all cookies to be SameSite=None. This introduces a new approach where all cookies are captured in a server-side cookie jar and, where needed, cookies are attached to cross-origin requests to simulate what the browser would do if the AUT were in the top frame.

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?

cypress-bot · 2022-06-14T19:50:12Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

packages/driver/src/cy/commands/sessions/manager.ts

emilyrohrbough · 2022-06-14T20:00:45Z

packages/server/lib/automation/automation.ts

@@ -4,6 +4,7 @@ import { Cookies } from './cookies'
 import { Screenshot } from './screenshot'
 import type { BrowserPreRequest } from '@packages/proxy'
 import type { AutomationMiddleware, OnRequestEvent } from '@packages/types'
+import { removeAllCookies, removeCookie } from '../cookie-jar'


without lookin into this deeply, why not merge the cookie jar and the cookies class? the names make it sound like one or the other could leverage each other and/or they could be merged?

Seems like the Cookies class might be able to.

They serve different purposes.

lib/cookie-jar is a stateful wrapper around tough-cookie's cookie implementation and stores cookies in memory for the sake of cross-origin testing.

lib/automation/cookies is a stateless wrapper around automating cookies. It's more about taking actions with cookies than storing them.

I don't really see anything that can be shared between them and I think it makes sense to keep their concerns separate.

hmm okay. Since the names are so similar, thoughts on updating both files to add a quick blur that describes their intent?

Good idea. Added in ab6e987 (#22320)

packages/proxy/lib/http/response-middleware.ts

emilyrohrbough · 2022-06-14T20:10:35Z

packages/server/lib/cookie-jar.ts

+  }
+
+  removeAllCookies () {
+    this._cookieJar.removeAllCookiesSync()


Does this need to be sync?

It doesn't need to, but there's no reason for it not to be sync. The cookie store is kept in memory, so all this ends up doing is basically cookieJar.cookies = {}. There's no sync file system access to worry about or anything.

cypress · 2022-06-14T20:20:43Z

Test summary

37681 • 0 • 456 • 0 • 4

Run details


Project	cypress
Status	Passed
Commit	`bae7061`
Started	Jun 23, 2022 2:08 PM
Ended	Jun 23, 2022 2:26 PM
Duration	17:40 💡
OS	Linux Debian - 10.11
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	commands/actions/click.cy.js	1
1	... > scroll-behavior > can scroll to and click elements in html with scroll-behavior: smooth
	cypress/proxy-logging.cy.ts	2
1	Proxy Logging > request logging > xhr log has response body/status code when xhr response is logged second
2	Proxy Logging > request logging > xhr log has response body/status code when xhr response is logged second
	create-react-app.cy.ts	1
1	Working with cra-ejected > should live-reload on src changes

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

packages/driver/cypress/e2e/e2e/origin/cookie_login.cy.ts

…tion

packages/proxy/lib/http/util/cookies.ts

mschile · 2022-06-16T04:16:55Z

packages/proxy/lib/http/util/cookies.ts

+  async capturePreviousCookies () {
+    // this plays a part in adding cross-origin cookies to the browser via
+    // automation. if the request doesn't need cross-origin handling, this
+    // is a nooop


Suggested change

// is a nooop

// is a noop

I just wanted to emphasize that this does nooothing in that situation 😅

Addressed in 52b6874 (#22320).

mschile · 2022-06-16T04:17:04Z

packages/proxy/lib/http/util/cookies.ts

+  async getAddedCookies () {
+    // this plays a part in adding cross-origin cookies to the browser via
+    // automation. if the request doesn't need cross-origin handling, this
+    // is a nooop


Suggested change

// is a nooop

// is a noop

Addressed in 52b6874 (#22320).

mschile · 2022-06-16T04:28:47Z

packages/server/lib/socket-base.ts

@@ -474,6 +476,8 @@ export class SocketBase {
              return this.localBus.emit('cross:origin:release:html')
            case 'cross:origin:finished':
              return this.localBus.emit('cross:origin:finished', args[0])
+            case 'url:changed':


Is this related to cookies or something that was missed previously?

No, this is vestigial from a previous implementation. I just missed removing it. Addressed in 3c558b5 (#22320).

AtofStryker

Obligatory Cookie Monster GIF

packages/proxy/lib/http/request-middleware.ts

AtofStryker · 2022-06-16T14:42:20Z

packages/proxy/lib/http/response-middleware.ts

@@ -393,99 +393,101 @@ const MaybePreventCaching: ResponseMiddleware = function () {
  this.next()
 }

-const determineIfNeedsCrossOriginHandling = (ctx: HttpMiddlewareThis<ResponseMiddlewareProps>) => {
-  const previousAUTRequestUrl = ctx.getPreviousAUTRequestUrl()
+const checkNeedsCrossOriginHandling = (ctx: HttpMiddlewareThis<ResponseMiddlewareProps>) => {


Suggested change

const checkNeedsCrossOriginHandling = (ctx: HttpMiddlewareThis<ResponseMiddlewareProps>) => {

const checkIfNeedsCrossOriginHandling = (ctx: HttpMiddlewareThis<ResponseMiddlewareProps>) => {

Addressed in 95124a3 (#22320)

packages/proxy/package.json

packages/driver/cypress.config.ts

packages/driver/cypress/e2e/e2e/origin/cookie_login.cy.ts

packages/proxy/test/unit/http/response-middleware.spec.ts

packages/server/lib/automation/cookies.ts

packages/server/lib/browsers/cdp_automation.ts

packages/server/lib/server-base.ts

packages/proxy/lib/http/util/cookies.ts

packages/proxy/lib/http/response-middleware.ts

AtofStryker · 2022-06-23T14:25:41Z

packages/server/lib/browsers/cdp_automation.ts

@@ -300,6 +300,11 @@ export class CdpAutomation {
          return this.getCookie(data)
        })

+      case 'add:cookies':
+        setCookie = data.map((cookie) => normalizeSetCookieProps(cookie)) as Protocol.Network.SetCookieRequest[]


Wonder if there is additional benefit to specifying the setCookie type to show it might be a single SetCookieRequest or an array of them on line 278, and then the consuming code just casts it, though I don't think this has a lot of benefit from a typings perspective.

let setCookie: Protocol.Network.SetCookieRequest | Protocol.Network.SetCookieRequest[]

…esser/CLOUD-577-spec-list-display-latest-runs-batching * muaz/CLOUD-577-spec-list-display-latest-runs: fix: Update "Request Access" button state after requesting access (ACI) (#22499) feat: Support "Queued" latest run status (#22497) fix: remove ctx.cloud.reset in tests, handle infinite loop in stale results (#22483) chore: add reporter webpack to gulp watch script (#22386) fix: Increase timeout for npm-webpack-dev-server tests (#22489) fix: Time out unmatched prerequests in proxy to avoid leaking memory (#22462) fix: Sort results in findCrossOriginLogs test helper to deterministic (#22481) fix: memory leak caused by storing base64 encoded files recieved by CDP `Network.requestWillBeSent` (#22460) fix: Improve cross-origin cookie handling (#22320) feat: Add button to clear value from search fields (#22202) chore: Add test to verify settings panels are collapsed by default (#22382) fix: process_profiler follow up work for v10 (#22363) chore: Update Chrome (stable) to 103.0.5060.53 (#22441) refactor: use design system windicss config (#21503) chore: update readme logo (#22433) chore: Update Chrome (beta) to 103.0.5060.53 (#22351) chore: updating version (#22432) Trigger Build

chrisbreiding added 11 commits June 13, 2022 10:15

fix: improve cross-origin cookie handling

57f1267

Merge branch 'develop' into issue-20685-improve-cookie-handling

157acf1

get automation in a better way

729133f

refactor cookie handling

ac6bcb3

remove obsolete tests

e657c81

extract cookie jar from session code

9c7d372

update doc / add comments

5e10c47

use proxy aut url instead of remote states

ceb127e

get cy.clearCookie() and cy.clearCookies() working

d5f816b

refactor tests

12dbc43

refactor cookie functions into cookie jar

7a42d38

chrisbreiding requested review from mjhenkes, mschile, AtofStryker and emilyrohrbough June 14, 2022 19:50

chrisbreiding requested a review from a team as a code owner June 14, 2022 19:50

chrisbreiding added 2 commits June 14, 2022 15:54

Merge branch 'develop' into issue-20685-improve-cookie-handling

f1cb1dc

cleanup

e1a5b29

emilyrohrbough reviewed Jun 14, 2022

View reviewed changes

chrisbreiding added 2 commits June 15, 2022 09:33

remove redundant cookie clearing

d34f408

fix import references

756af9f

mjhenkes reviewed Jun 15, 2022

View reviewed changes

packages/driver/cypress/e2e/e2e/origin/cookie_login.cy.ts Show resolved Hide resolved

mjhenkes reviewed Jun 15, 2022

View reviewed changes

packages/driver/cypress/e2e/e2e/origin/cookie_login.cy.ts Outdated Show resolved Hide resolved

chrisbreiding added 3 commits June 15, 2022 10:32

fix issues with cookie jar

c989b01

error if this.next() is called more than once in same middleware func…

3c81b36

…tion

fix test

efe10f3

mjhenkes approved these changes Jun 15, 2022

View reviewed changes

mschile requested changes Jun 16, 2022

View reviewed changes

AtofStryker mentioned this pull request Jun 16, 2022

Cy10 multi domain: redirect triggered keycloak login form is unresponsive #22282

Closed

chrisbreiding added 3 commits June 21, 2022 09:12

fix issues with automation cookies in firefox

0ae6188

add comments to clarify various cookie utilities

ab6e987

fix issues caused by stability changes

7eee1fa

AtofStryker requested changes Jun 21, 2022

View reviewed changes

skip test with known issue in firefox

dd0927b

chrisbreiding requested review from AtofStryker, mschile and emilyrohrbough June 22, 2022 13:31

chrisbreiding added 5 commits June 22, 2022 09:37

remove accidentally committed config

f45b180

add/improve types

db8684a

apply review suggestions

95124a3

add middleware unit tests

c22269b

update sameSiteContext comments

4903cb1

mschile approved these changes Jun 23, 2022

View reviewed changes

chrisbreiding added 4 commits June 23, 2022 08:24

Merge branch 'develop' into issue-20685-improve-cookie-handling

ba6c0ac

fix handling of conflicting cookies

235764f

fix test

298ac4d

fix test

bae7061

AtofStryker approved these changes Jun 23, 2022

View reviewed changes

chrisbreiding merged commit a21c942 into develop Jun 23, 2022

chrisbreiding deleted the issue-20685-improve-cookie-handling branch June 23, 2022 15:00

This was referenced Jun 23, 2022

Multi-domain: Improve cookie handling #20685

Closed

Cypress 9.6.0- Unable to login to applications via Github using cy.origin #21363

Closed

chrisbreiding mentioned this pull request Jun 23, 2022

Handle cross-origin document.cookie #22479

Closed

AtofStryker mentioned this pull request Aug 18, 2022

fix: same site cookie context and duplicate cookies #23438

Merged

4 tasks

AtofStryker mentioned this pull request Sep 8, 2022

feat: improve server side cookie handling via more accurate top simulation #23728

Closed

4 tasks

AtofStryker mentioned this pull request Sep 19, 2022

feat: Improve simulated top cookie handling #23872

Merged

4 tasks

lmiller1990 mentioned this pull request Dec 19, 2022

Cypress is running really slow after upgrading to v10 #22868

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Improve cross-origin cookie handling #22320

fix: Improve cross-origin cookie handling #22320

chrisbreiding commented Jun 14, 2022

cypress-bot bot commented Jun 14, 2022

emilyrohrbough Jun 14, 2022

chrisbreiding Jun 15, 2022

emilyrohrbough Jun 16, 2022

chrisbreiding Jun 21, 2022

emilyrohrbough Jun 14, 2022

chrisbreiding Jun 15, 2022

cypress bot commented Jun 14, 2022 •

edited

Loading

mschile Jun 16, 2022

chrisbreiding Jun 16, 2022

chrisbreiding Jun 16, 2022

mschile Jun 16, 2022

chrisbreiding Jun 16, 2022

mschile Jun 16, 2022

chrisbreiding Jun 16, 2022

AtofStryker left a comment •

edited

Loading

AtofStryker Jun 16, 2022

chrisbreiding Jun 22, 2022

AtofStryker Jun 23, 2022

	const checkNeedsCrossOriginHandling = (ctx: HttpMiddlewareThis<ResponseMiddlewareProps>) => {
	const checkIfNeedsCrossOriginHandling = (ctx: HttpMiddlewareThis<ResponseMiddlewareProps>) => {

fix: Improve cross-origin cookie handling #22320

fix: Improve cross-origin cookie handling #22320

Conversation

chrisbreiding commented Jun 14, 2022

User facing changelog

Additional details

PR Tasks

cypress-bot bot commented Jun 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypress bot commented Jun 14, 2022 • edited Loading

Test summary

Run details

Flakiness

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtofStryker left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypress bot commented Jun 14, 2022 •

edited

Loading

AtofStryker left a comment •

edited

Loading