fix: same site cookie context and duplicate cookies #23438

AtofStryker · 2022-08-18T20:04:11Z

Closes Cookie behavior issue after 10.3.0 #23132

User facing changelog

Fixes an issue introduced in 10.3.0 and further exposed in 10.4.0 that omitted same-site cookies when URL Scheme, Domain, and Top Level Domain match, but the ports are different (same-site).

Additional details

In #22320 we added improved cookie handling that handles cookie management in the proxy server. When we calculate SameSiteContext, we were using SameOriginPolicy to calculate the context of the request. This is a bit different than SameSite, which cookies use to determine context as subdomains and ports do not need to match to be considered same-site.

In #22963 we removed the isAUTFrame check in checkIfNeedsCrossOriginHandling. In the case of #23132 (comment), I think isAUTFrame used to evaluate to false for the fetch/XHR requests being made, which leveraged Set-Cookie over X-Set-Cookie. After 10.4.0, the check was removed and X-Set-Cookie was being used as it believed the requests needed to handle the cookies in a third party context, when in fact they did not.

Until #23551 is implemented, we should likely use Set-Cookie, even if the setting of the actual cookie fails, as we currently cannot discern the difference between requests that are specified in the browser with XMLHttpRequest Credentials or fetch credentials and once that aren't (or use more strict fetch credentials like same-origin or omit).

Steps to test

Unit tests were added for functions getSameSiteContext in @packages/proxy and urlOriginsMatch in packages/network to verify the behavior is what we expect.

Unit tests for verifying duplicate cookie behavior and integration tests with tough-cookie were also added to make sure we integrate correctly.

E2E tests were added in cookie_behavior.cy.ts to serve as a base for when we explore #23551 to make sure the correct spec behavior is implemented. There are several tests with FIXMEs as well as comments to what the assertions should be. In the current context, we may optimistically apply cookies to request, as if each request was called withCredentials or with included credentials, which the tests should demonstrate.

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?
Have API changes been updated in the type definitions?

cypress-bot · 2022-08-18T20:04:13Z

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.

cypress · 2022-08-18T20:25:26Z

Test summary

4020 • 0 • 434 • 0 • 0

Run details


Project	cypress
Status	Passed
Commit	`b7e9993`
Started	Sep 8, 2022 3:10 AM
Ended	Sep 8, 2022 3:23 AM
Duration	13:03 💡
OS	Linux Debian - 11.3
Browser	Webkit 16

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

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

chrisbreiding

The unit tests are great, but could you add a more e2e-style test? Best place is probably cookie_login.cy.ts. We could have passing unit tests for an incorrect implementation. A test that actually runs through the browser will give us a bit more confidence it's correct.

chrisbreiding · 2022-08-19T19:17:15Z

packages/network/test/unit/cors_spec.ts

+      this.isFalse = (url1, url2) => {
+        expect(cors.urlOriginsMatch(url1, url2)).to.be.false
+      }
+
+      this.isTrue = (url1, url2) => {
+        expect(cors.urlOriginsMatch(url1, url2)).to.be.true
+      }


Maybe assertOriginsDontMatch and assertOriginsMatch instead? I feel like isFalse/isTrue should return whether the argument is false or true.

These could also be defined in the context's scope so there wouldn't be a need to use this. everywhere, e.g.:

context('.urlOriginsMatch', () => { const assertOriginsDontMatch = () => { /* ... */ } it('test', () => { assertOriginsDontMatch('https://foo.bar:443', this.url) assertOriginsDontMatch('https://foo.bar:80', this.url) // .. and so on ... }) })

I like that style a lot better. I went through the other tests in lib/cors and refactored them in 8404853

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

sameSiteContext calculation method and add tests

…k of the cookie or not in the server side cookie jar

… appropriately to request header Cookie

…r. Add additional tests to verify expected behavior

…or vs what spec behavior should/will be

AtofStryker · 2022-08-25T22:03:18Z

The unit tests are great, but could you add a more e2e-style test? Best place is probably cookie_login.cy.ts. We could have passing unit tests for an incorrect implementation. A test that actually runs through the browser will give us a bit more confidence it's correct.

@chrisbreiding the tests in cookie_login from what I can see look correct. For the behaviors that could we different or strange, I added a new spec called cookie_behavior in the origin directory that tests several different scenarios of how cookies can be applied. Right now, some of those are incorrect and are documented with FIXMEs, but currently showcase how we currently set/send cookies. It should be really useful when we implement #23551 (which I still need to fill out the description)

…ite none cookies in firefox

…-site-cookie-context

emilyrohrbough · 2022-08-30T14:28:32Z

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

@@ -191,7 +191,8 @@ describe('cy.origin - cookie login', () => {
      verifyIdpNotLoggedIn({ expectNullCookie: false })
    })

-    it('SameSite=None -> not logged in', () => {
+    // FIXME: Currently in Firefox, the default cookie setting in the extension is no_restriction, which can be set with Secure=false.
+    it('SameSite=None -> not logged in', { browser: '!firefox' }, () => {


is this broken or the tests arn't aligned? should we add a test specifically for firefox?

What I believe was happening here before is that since this request that sets cookies was seemed "cross-origin", X-Set-Cookie was being used in the header and not setting the cookie in the browser. In the server side cookie jar, we ignore cookies that are SameSite=None and Secure=false.

Now, since we use Set-Cookie all the time, the cookie is actually set in firefox because we currently allow SameSite=None and Secure=false to be set in the extension, which actually allows this test to log in.

We can add a test that verifies this behavior, but I am somewhat of the opinion that we shouldn't allow setting insecure samesite none cookies in firefox.

Now, since we use Set-Cookie all the time, the cookie is actually set in firefox because we currently allow SameSite=None and Secure=false to be set in the extension, which actually allows this test to log in.

I would expect this to be a problem for our user if it works in chrome one way and differently in firefox. Shouldn't we be updating the extension to have the same checks / ignore the same attributes as the server-side cookie jar that is using to manage the cookies for chrome & electron ?

emilyrohrbough · 2022-08-30T14:30:01Z

packages/driver/cypress/plugins/server.js

-const httpsApp = createApp(httpsPort)
-const httpsServer = httpsProxy.httpsServer(httpsApp)
+httpsPorts.forEach((port) => {
+  const httpsApp = createApp(port)


Do we need to create a full new app for these tests? Could we dumb this down create an express app with the two/three routes needed for this test?

The issue is we need a different port on https, which we currently do not have to verify same-site cookie behavior for differing ports. The added routes in the server aren't the reason for the new app, which I don't think is obvious at all. I'm thinking some documentation/comments are needed here.

added some docs in ec8b751

emilyrohrbough · 2022-08-30T14:30:38Z

packages/network/test/unit/cors_spec.ts

-        expect(cors.parseUrlIntoDomainTldPort(url)).to.deep.eq(obj)
-      }
-    })
+    const assertParsesUrlCorrectly = (url, obj) => {


nit: named assert but uses expect

updated to expectUrlToBeParsedCorrectly in a2637b9

emilyrohrbough · 2022-08-30T14:32:31Z

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

+  const { port: port2, ...parsedUrl2 } = cors.parseUrlIntoDomainTldPort(url2)
+
+  // If HTTPS, ports NEED to match. Otherwise, HTTP ports can be different and are same origin
+  const doPortsPassSameSchemeCheck = port1 !== port2 ? (port1 !== '443' && port2 !== '443') : true


noob question - what does 443 represent?

default HTTPS port. Only reason I am adding the check here is due to the logic into parseUrlIntoDomainTldPort. I am wondering for checking same-site capability if we want to go off the protocol scheme as well, since 443 might be SSL, and there could be other ports that are https.

For example, https://www.testme.com:8443 and (https://app.testme.com:4849|http://app.testme.com:443) are all same-site, but this function would evaluate false | true. Since we infer port fromparseUrlIntoDomainTldPort, I am not sure this is an issue though

emilyrohrbough · 2022-08-30T14:35:49Z

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

@@ -19,15 +60,15 @@ interface RequestDetails {
 // see https://github.com/salesforce/tough-cookie#samesite-cookies
 export const getSameSiteContext = (autUrl: string | undefined, requestUrl: string, isAUTFrameRequest: boolean) => {


Suggested change

export const getSameSiteContext = (autUrl: string | undefined, requestUrl: string, isAUTFrameRequest: boolean) => {

export const getSameSiteContext = (autUrl: string | undefined, requestUrl: string, isAUTFrameRequest: boolean): Protocol.Network.CookieSameSite => {

I'm not sure we want to pull the CDP types into the proxy, but we could also make a reserved type for this even though it is currently implied

emilyrohrbough · 2022-08-30T14:37:21Z

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

-  const existingCookies = this.req.headers['cookie'] ? [this.req.headers['cookie']] : []
-  const cookiesToAdd = cookies.map((cookie) => `${cookie.key}=${cookie.value}`)
+  const applicableCookiesInCookieJar = this.getCookieJar().getCookies(this.req.proxiedUrl, sameSiteContext)
+  const cookiesOnRequestString = (this.req.headers['cookie'] ? this.req.headers['cookie'] : '').split('; ')


Suggested change

const cookiesOnRequestString = (this.req.headers['cookie'] ? this.req.headers['cookie'] : '').split('; ')

const cookiesOnRequest = (this.req.headers['cookie'] || '').split('; )

addressed in 99d5ed2

emilyrohrbough · 2022-08-30T14:40:19Z

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

-  const cookies = this.getCookieJar().getCookies(this.req.proxiedUrl, sameSiteContext)
-  const existingCookies = this.req.headers['cookie'] ? [this.req.headers['cookie']] : []
-  const cookiesToAdd = cookies.map((cookie) => `${cookie.key}=${cookie.value}`)
+  const applicableCookiesInCookieJar = this.getCookieJar().getCookies(this.req.proxiedUrl, sameSiteContext)


Should addCookieJarCookiesToRequest handle pulling the cookie jar cookies?

I figured not since it really it just a utility and didn't want to make it state aware.

emilyrohrbough · 2022-08-30T14:41:13Z

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

+  return doPortsPassSameSchemeCheck && _.isEqual(parsedUrl1, parsedUrl2)
+}
+
+export const addCookieJarCookiesToRequest = (applicableCookieJarCookies: Cookie[] = [], requestCookieString: string[] = []): string => {


Suggested change

export const addCookieJarCookiesToRequest = (applicableCookieJarCookies: Cookie[] = [], requestCookieString: string[] = []): string => {

export const addCookieJarCookiesToRequest = (applicableCookieJarCookies: Cookie[] = [], requestCookies: string[] = []): string => {

addressed (somewhat) in 99d5ed2. The name was used below so I went with requestCookieStringArray

…-site-cookie-context

mjhenkes · 2022-09-06T14:51:15Z

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

+  }
+
+  beforeEach(() => {
+    // FIXME: clearing cookies in the browser currently does not clear cookies in the server-side cookie jar


Could you log issues for the FIXMES? seems like we should get them on the plan.

AtofStryker linked an issue Aug 18, 2022 that may be closed by this pull request

Cookie behavior issue after 10.3.0 #23132

Closed

AtofStryker changed the title ~~Fix/same site cookie context~~ fix: same site cookie context Aug 18, 2022

AtofStryker mentioned this pull request Aug 18, 2022

fix: Allow for urlOriginsMatch to ignore port unless url scheme is HTTPS to fit same origin policy #23411

Closed

4 tasks

AtofStryker requested review from chrisbreiding, flotwig, mschile, emilyrohrbough and mjhenkes August 18, 2022 21:55

AtofStryker self-assigned this Aug 18, 2022

AtofStryker marked this pull request as ready for review August 18, 2022 21:57

emilyrohrbough reviewed Aug 18, 2022

View reviewed changes

packages/proxy/lib/http/response-middleware.ts Outdated Show resolved Hide resolved

chrisbreiding requested changes Aug 19, 2022

View reviewed changes

AtofStryker added 8 commits August 25, 2022 17:22

test: refactor and add tests in the cors package

d6a117c

fix: add areUrlsSameSite method to cookies package and fix

d0c87cc

sameSiteContext calculation method and add tests

fix: always use Set-Cookie optimistically whether or not we keep trac…

b6ff533

…k of the cookie or not in the server side cookie jar

chore: add failing unit tests for postpending cookies

e86a6da

chore: add tough cookie integration tests to verify we append cookies…

d154a6d

… appropriately to request header Cookie

fix: do not duplicate cookies in request if existing in the cookie ja…

bb4f743

…r. Add additional tests to verify expected behavior

test: add cookie behavior tests that document current expected behavi…

7356989

…or vs what spec behavior should/will be

test: add misc tests that check for cookie order

d02fc1f

AtofStryker force-pushed the fix/same-site-cookie-context branch from f1f8cbb to d02fc1f Compare August 25, 2022 21:43

chore: update debug logs in request to discern cookies

b466b80

AtofStryker changed the title ~~fix: same site cookie context~~ fix: same site cookie context and duplicate cookies Aug 25, 2022

AtofStryker linked an issue Aug 25, 2022 that may be closed by this pull request

Prepending Stale Cookies to requests after 10.3 #22751

Closed

AtofStryker mentioned this pull request Aug 25, 2022

fix: duplicate cookies in proxy #23473

Closed

4 tasks

AtofStryker requested review from chrisbreiding and emilyrohrbough August 25, 2022 22:03

AtofStryker added 2 commits August 29, 2022 17:33

skip SameSite=none test in firefox as we currently low insecure sames…

5213499

…ite none cookies in firefox

Merge branch 'develop' of github.com:cypress-io/cypress into fix/same…

e5dc9e4

…-site-cookie-context

AtofStryker requested a review from chrisbreiding August 29, 2022 22:50

emilyrohrbough reviewed Aug 30, 2022

View reviewed changes

AtofStryker added 4 commits August 30, 2022 10:55

Merge branch 'develop' of github.com:cypress-io/cypress into fix/same…

51a20d9

…-site-cookie-context

chore: apply suggestions from code review

99d5ed2

chore: change expects to expect

a2637b9

chore: add documentation for why we need an additional HTTPS port

ec8b751

AtofStryker requested a review from emilyrohrbough August 30, 2022 15:25

AtofStryker added 2 commits September 1, 2022 15:37

remove X-Set-Cookie fixmes

56db09e

Merge branch 'develop' of github.com:cypress-io/cypress into fix/same…

b406117

…-site-cookie-context

chrisbreiding approved these changes Sep 2, 2022

View reviewed changes

Merge branch 'develop' into fix/same-site-cookie-context

b3b6ef4

mjhenkes reviewed Sep 6, 2022

View reviewed changes

mjhenkes approved these changes Sep 6, 2022

View reviewed changes

AtofStryker added 3 commits September 7, 2022 14:33

Merge branch 'develop' into fix/same-site-cookie-context

6306db8

Merge branch 'develop' into fix/same-site-cookie-context

cee4317

Merge branch 'develop' into fix/same-site-cookie-context

b7e9993

AtofStryker merged commit 9cdb33b into develop Sep 8, 2022

AtofStryker deleted the fix/same-site-cookie-context branch September 8, 2022 03:19

cypress-bot bot mentioned this pull request Sep 8, 2022

Cookie behavior issue after 10.3.0 #23132

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: same site cookie context and duplicate cookies #23438

fix: same site cookie context and duplicate cookies #23438

AtofStryker commented Aug 18, 2022 •

edited

Loading

cypress-bot bot commented Aug 18, 2022

cypress bot commented Aug 18, 2022 •

edited

Loading

chrisbreiding left a comment

chrisbreiding Aug 19, 2022

AtofStryker Aug 22, 2022

AtofStryker commented Aug 25, 2022

emilyrohrbough Aug 30, 2022

AtofStryker Aug 30, 2022

emilyrohrbough Sep 6, 2022

emilyrohrbough Aug 30, 2022

AtofStryker Aug 30, 2022

AtofStryker Aug 30, 2022

emilyrohrbough Aug 30, 2022

AtofStryker Aug 30, 2022

emilyrohrbough Aug 30, 2022

AtofStryker Aug 30, 2022

emilyrohrbough Aug 30, 2022

AtofStryker Aug 30, 2022

emilyrohrbough Aug 30, 2022

AtofStryker Aug 30, 2022

emilyrohrbough Aug 30, 2022

AtofStryker Aug 30, 2022

emilyrohrbough Aug 30, 2022

AtofStryker Aug 30, 2022

mjhenkes Sep 6, 2022

		@@ -19,15 +60,15 @@ interface RequestDetails {
		// see https://github.com/salesforce/tough-cookie#samesite-cookies
		export const getSameSiteContext = (autUrl: string \| undefined, requestUrl: string, isAUTFrameRequest: boolean) => {

	const cookiesOnRequestString = (this.req.headers['cookie'] ? this.req.headers['cookie'] : '').split('; ')
	const cookiesOnRequest = (this.req.headers['cookie'] \|\| '').split('; )

	export const addCookieJarCookiesToRequest = (applicableCookieJarCookies: Cookie[] = [], requestCookieString: string[] = []): string => {
	export const addCookieJarCookiesToRequest = (applicableCookieJarCookies: Cookie[] = [], requestCookies: string[] = []): string => {

fix: same site cookie context and duplicate cookies #23438

fix: same site cookie context and duplicate cookies #23438

Conversation

AtofStryker commented Aug 18, 2022 • edited Loading

User facing changelog

Additional details

Steps to test

How has the user experience changed?

PR Tasks

cypress-bot bot commented Aug 18, 2022

cypress bot commented Aug 18, 2022 • edited Loading

Test summary

Run details

chrisbreiding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtofStryker commented Aug 25, 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

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

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 commented Aug 18, 2022 •

edited

Loading

cypress bot commented Aug 18, 2022 •

edited

Loading