From d13c0f6f249be0fcb0290fd58ee60136024a7bcf Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Thu, 9 Feb 2023 13:29:06 -0500 Subject: [PATCH 01/40] chore: add regression tests for duplicate cookies and bad expiry times --- .../cypress/e2e/e2e/origin/cookie_misc.cy.ts | 195 ++++++++++++++++++ .../fixtures/duplicate-hostOnly-cookie.html | 19 ++ packages/driver/cypress/plugins/server.js | 11 + 3 files changed, 225 insertions(+) create mode 100644 packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts create mode 100644 packages/driver/cypress/fixtures/duplicate-hostOnly-cookie.html diff --git a/packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts b/packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts new file mode 100644 index 000000000000..d2a13aabc0c6 --- /dev/null +++ b/packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts @@ -0,0 +1,195 @@ +import { makeRequestForCookieBehaviorTests as makeRequest } from '../../../support/utils' + +describe('misc cookie tests', () => { + // NOTE: For this test to work correctly, we need to have a FQDN, not localhost (www.foobar.com). + // FIXES: https://github.com/cypress-io/cypress/issues/25174 (cookies are duplicated with prepended dot (.)) + it('does not duplicate cookies with a prepended dot for cookies that are stored inside the server side cookie jar', () => { + cy.visit('https://www.foobar.com:3502/fixtures/duplicate-hostOnly-cookie.html') + + // does a 302 redirect back to www.foobar.com primary-origin page, but sets a sameSite=None cookie + cy.get('[data-cy="cookie-cross-origin-redirects"]').click() + + cy.getCookies({ domain: 'www.foobar.com' }).then((cookies) => { + expect(cookies).to.have.length(1) + + const singleCookie = cookies[0] + + expect(singleCookie).to.have.property('name', 'foo') + expect(singleCookie).to.have.property('value', 'bar') + expect(singleCookie).to.have.property('domain', 'www.foobar.com') + }) + }) + + /** + * FIXES: + * https://github.com/cypress-io/cypress/issues/25205 (cookies set with expired time with value deleted show up as set with value deleted) + * https://github.com/cypress-io/cypress/issues/25495 (session cookies set with expired time with value deleted show up as set with value deleted) + * https://github.com/cypress-io/cypress/issues/25148 (cannot log into azure, shows cookies are disabled/blocked) + */ + describe('expiring cookies', () => { + before(() => { + cy.origin(`https://app.foobar.com:3503`, () => { + const { makeRequestForCookieBehaviorTests: makeRequest } = require('../../../support/utils') + + // @ts-ignore + window.makeRequest = makeRequest + }) + }) + + describe('removes cookies that are set with an expired expiry time from the server side cookie jar / browser via CDP', () => { + it('works with Max-Age=0', () => { + cy.visit(`https://www.foobar.com:3502/fixtures/primary-origin.html`) + + cy.visit(`https://app.foobar.com:3503/fixtures/secondary-origin.html`) + cy.origin(`https://app.foobar.com:3503`, () => { + cy.window().then((win) => { + return cy.wrap(makeRequest(win, `/set-cookie?cookie=foo=bar; Domain=.foobar.com;`)) + }) + + cy.getCookie('foo').its('value').should('eq', 'bar') + + cy.window().then((win) => { + return cy.wrap(makeRequest(win, `/set-cookie?cookie=foo=deleted; Domain=.foobar.com; Max-Age=0;`)) + }) + + cy.getCookie('foo').should('eq', null) + }) + }) + + it('works with expires=Thu, 01-Jan-1970 00:00:01 GMT', () => { + cy.visit(`https://www.foobar.com:3502/fixtures/primary-origin.html`) + + cy.visit(`https://app.foobar.com:3503/fixtures/secondary-origin.html`) + cy.origin(`https://app.foobar.com:3503`, () => { + cy.window().then((win) => { + return cy.wrap(makeRequest(win, `/set-cookie?cookie=foo=bar; Domain=.foobar.com;`)) + }) + + cy.getCookie('foo').its('value').should('eq', 'bar') + + cy.window().then((win) => { + return cy.wrap(makeRequest(win, `/set-cookie?cookie=foo=deleted; Domain=.foobar.com; expires=Thu, 01-Jan-1970 00:00:01 GMT;`)) + }) + + cy.getCookie('foo').should('eq', null) + }) + }) + + it('works with expires=Tues, 01-Jan-1980 00:00:01 GMT', () => { + cy.visit(`https://www.foobar.com:3502/fixtures/primary-origin.html`) + + cy.visit(`https://app.foobar.com:3503/fixtures/secondary-origin.html`) + cy.origin(`https://app.foobar.com:3503`, () => { + cy.window().then((win) => { + return cy.wrap(makeRequest(win, `/set-cookie?cookie=foo=bar; Domain=.foobar.com;`)) + }) + + cy.getCookie('foo').its('value').should('eq', 'bar') + + cy.window().then((win) => { + return cy.wrap(makeRequest(win, `/set-cookie?cookie=foo=deleted; Domain=.foobar.com; expires=Tues, 01-Jan-1980 00:00:01 GMT; Max-Age=0;`)) + }) + + cy.getCookie('foo').should('eq', null) + }) + }) + + it('work with expires=Thu, 01-Jan-1970 00:00:01 GMT and Max-Age=0', () => { + cy.visit(`https://www.foobar.com:3502/fixtures/primary-origin.html`) + + cy.visit(`https://app.foobar.com:3503/fixtures/secondary-origin.html`) + cy.origin(`https://app.foobar.com:3503`, () => { + cy.window().then((win) => { + return cy.wrap(makeRequest(win, `/set-cookie?cookie=foo=bar; Domain=.foobar.com;`)) + }) + + cy.getCookie('foo').its('value').should('eq', 'bar') + + cy.window().then((win) => { + return cy.wrap(makeRequest(win, `/set-cookie?cookie=foo=deleted; Domain=.foobar.com; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0;`)) + }) + + cy.getCookie('foo').should('eq', null) + }) + }) + }) + + describe('removes cookies that are set with an expired expiry time from the document.cookie patch / browser via CDP', () => { + it('works with Max-Age=0', () => { + cy.visit(`https://www.foobar.com:3502/fixtures/primary-origin.html`) + + cy.visit(`https://app.foobar.com:3503/fixtures/secondary-origin.html`) + cy.origin(`https://app.foobar.com:3503`, () => { + cy.window().then((win) => { + win.document.cookie = 'foo=bar' + }) + + cy.getCookie('foo').its('value').should('eq', 'bar') + + cy.window().then((win) => { + win.document.cookie = 'foo=deleted; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0;' + }) + + cy.getCookie('foo').should('eq', null) + }) + }) + + it('works with expires=Thu, 01-Jan-1970 00:00:01 GMT', () => { + cy.visit(`https://www.foobar.com:3502/fixtures/primary-origin.html`) + + cy.visit(`https://app.foobar.com:3503/fixtures/secondary-origin.html`) + cy.origin(`https://app.foobar.com:3503`, () => { + cy.window().then((win) => { + win.document.cookie = 'foo=bar' + }) + + cy.getCookie('foo').its('value').should('eq', 'bar') + + cy.window().then((win) => { + win.document.cookie = 'foo=deleted; Max-Age=0' + }) + + cy.getCookie('foo').should('eq', null) + }) + }) + + it('works with expires=Tues, 01-Jan-1980 00:00:01 GMT', () => { + cy.visit(`https://www.foobar.com:3502/fixtures/primary-origin.html`) + + cy.visit(`https://app.foobar.com:3503/fixtures/secondary-origin.html`) + cy.origin(`https://app.foobar.com:3503`, () => { + cy.window().then((win) => { + win.document.cookie = 'foo=bar' + }) + + cy.getCookie('foo').its('value').should('eq', 'bar') + + cy.window().then((win) => { + win.document.cookie = 'foo=deleted; expires=Tues, 01-Jan-1980 00:00:01 GMT' + }) + + cy.getCookie('foo').should('eq', null) + }) + }) + + it('expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0', () => { + cy.visit(`https://www.foobar.com:3502/fixtures/primary-origin.html`) + + cy.visit(`https://app.foobar.com:3503/fixtures/secondary-origin.html`) + cy.origin(`https://app.foobar.com:3503`, () => { + cy.window().then((win) => { + win.document.cookie = 'foo=bar' + }) + + cy.getCookie('foo').its('value').should('eq', 'bar') + + cy.window().then((win) => { + win.document.cookie = 'foo=deleted; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0' + }) + + cy.getCookie('foo').should('eq', null) + }) + }) + }) + }) +}) diff --git a/packages/driver/cypress/fixtures/duplicate-hostOnly-cookie.html b/packages/driver/cypress/fixtures/duplicate-hostOnly-cookie.html new file mode 100644 index 000000000000..ed604187d087 --- /dev/null +++ b/packages/driver/cypress/fixtures/duplicate-hostOnly-cookie.html @@ -0,0 +1,19 @@ + + + + + + Trigger Cross Origin Redirect Loop that sets cookies + + + diff --git a/packages/driver/cypress/plugins/server.js b/packages/driver/cypress/plugins/server.js index a27eba20791d..66efc48a5d08 100644 --- a/packages/driver/cypress/plugins/server.js +++ b/packages/driver/cypress/plugins/server.js @@ -296,6 +296,17 @@ const createApp = (port) => { .sendStatus(200) }) + app.get('/set-same-site-none-cookie-on-redirect', (req, res) => { + const { redirect, cookie } = req.query + const cookieDecoded = decodeURIComponent(cookie) + + const cookieVal = `${cookieDecoded}; SameSite=None; Secure` + + res + .header('Set-Cookie', cookieVal) + .redirect(302, redirect) + }) + app.get('/test-request-credentials', (req, res) => { const origin = cors.getOrigin(req['headers']['referer']) From 2b976383eefa422e894573785c5517dd1c27419a Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Thu, 9 Feb 2023 13:29:30 -0500 Subject: [PATCH 02/40] avoid prepending domain with dot for cookies that are set with the server side jar. This is to avoid the cookie being duplicated if it is set or overridden in a different context (request that can actually set the cookie or via document.domain) --- packages/proxy/lib/http/util/cookies.ts | 4 ++++ packages/server/lib/automation/cookies.ts | 7 +++++++ packages/server/lib/browsers/cdp_automation.ts | 5 ++++- packages/server/lib/util/cookies.ts | 14 +++++++++++++- .../test/unit/browsers/cdp_automation_spec.ts | 17 +++++++++++++++++ 5 files changed, 45 insertions(+), 2 deletions(-) diff --git a/packages/proxy/lib/http/util/cookies.ts b/packages/proxy/lib/http/util/cookies.ts index a320a7b570ce..27943cc0eaf3 100644 --- a/packages/proxy/lib/http/util/cookies.ts +++ b/packages/proxy/lib/http/util/cookies.ts @@ -238,6 +238,10 @@ export class CookiesHelper { try { this.cookieJar.setCookie(toughCookie, this.request.url, this.sameSiteContext) + // If a cookie is stored in the jar, we want to make sure that the domain set in the cookie here MATCHES + // the domain of what is set in CDP to prevent duplications or strange overwrite behavior + // @ts-expect-error + toughCookie.isStoredInServerSideCookieJar = true } catch (err) { this.debug('adding cookie to jar failed: %s', err.message) } diff --git a/packages/server/lib/automation/cookies.ts b/packages/server/lib/automation/cookies.ts index ee45dcc89704..cd9aac763523 100644 --- a/packages/server/lib/automation/cookies.ts +++ b/packages/server/lib/automation/cookies.ts @@ -46,6 +46,13 @@ const normalizeCookieProps = function (props) { cookie.expiry = props.expirationDate } + // if the cookie is stored inside the server side cookie jar, + // we want to make the automation client aware so the domain property + // isn't mutated to prevent duplicate setting of cookies from different contexts + if (props.isStoredInServerSideCookieJar) { + cookie.isStoredInServerSideCookieJar = true + } + return cookie } diff --git a/packages/server/lib/browsers/cdp_automation.ts b/packages/server/lib/browsers/cdp_automation.ts index b43e65763f19..843dcd775eaf 100644 --- a/packages/server/lib/browsers/cdp_automation.ts +++ b/packages/server/lib/browsers/cdp_automation.ts @@ -108,7 +108,10 @@ const normalizeSetCookieProps = (cookie: CyCookie): Protocol.Network.SetCookieRe .value() // without this logic, a cookie being set on 'foo.com' will only be set for 'foo.com', not other subdomains - if (!cookie.hostOnly && isHostOnlyCookie(cookie)) { + // However, we do NOT want to prefix the domain for cookies that are stored inside our server-side cookie jar, + // as it might lead to duplicate cookies when another request or script overrides the value + // @ts-expect-error + if (!cookie.hostOnly && isHostOnlyCookie(cookie) && !cookie.isStoredInServerSideCookieJar) { setCookieRequest.domain = `.${cookie.domain}` } diff --git a/packages/server/lib/util/cookies.ts b/packages/server/lib/util/cookies.ts index 650c8c7b9277..71ca290c1d49 100644 --- a/packages/server/lib/util/cookies.ts +++ b/packages/server/lib/util/cookies.ts @@ -24,11 +24,15 @@ export const toughCookieToAutomationCookie = (toughCookie: Cookie, defaultDomain sameSite: toughCookie.sameSite === 'none' ? 'no_restriction' : toughCookie.sameSite, secure: toughCookie.secure, value: toughCookie.value, + // @ts-expect-error + ...(toughCookie.isStoredInServerSideCookieJar ? { + isStoredInServerSideCookieJar: true, + } : {}), } } export const automationCookieToToughCookie = (automationCookie: AutomationCookie, defaultDomain: string): Cookie => { - return new Cookie({ + const cookie = new Cookie({ domain: automationCookie.domain || defaultDomain, expires: automationCookie.expiry != null && isFinite(automationCookie.expiry) ? new Date(automationCookie.expiry * 1000) : undefined, httpOnly: automationCookie.httpOnly, @@ -39,6 +43,14 @@ export const automationCookieToToughCookie = (automationCookie: AutomationCookie secure: automationCookie.secure, value: automationCookie.value, }) + + // @ts-expect-error + if (automationCookie.isStoredInServerSideCookieJar) { + // @ts-expect-error + cookie.isStoredInServerSideCookieJar = true + } + + return cookie } const sameSiteNoneRe = /; +samesite=(?:'none'|"none"|none)/i diff --git a/packages/server/test/unit/browsers/cdp_automation_spec.ts b/packages/server/test/unit/browsers/cdp_automation_spec.ts index 8a7a03704b39..ff5c9645e45b 100644 --- a/packages/server/test/unit/browsers/cdp_automation_spec.ts +++ b/packages/server/test/unit/browsers/cdp_automation_spec.ts @@ -204,6 +204,23 @@ context('lib/browsers/cdp_automation', () => { }) }) + it('does not mutate domain property if cookie is set in the server side cookie jar to prevent duplication / inconsistent overwrite behavior', function () { + this.sendDebuggerCommand + .withArgs('Network.setCookie', { domain: 'google.com', name: 'session', value: 'key', path: '/' }) + .resolves({ success: true }) + .withArgs('Network.getAllCookies') + .resolves({ + cookies: [ + { name: 'session', value: 'key', path: '/', domain: 'google.com', secure: false, httpOnly: false }, + ], + }) + + return this.onRequest('set:cookie', { domain: 'google.com', name: 'session', value: 'key', path: '/', isStoredInServerSideCookieJar: true }) + .then((resp) => { + expect(resp).to.deep.eq({ domain: 'google.com', expirationDate: undefined, hostOnly: true, httpOnly: false, name: 'session', value: 'key', path: '/', secure: false, sameSite: undefined }) + }) + }) + it('rejects with error', function () { return this.onRequest('set:cookie', { domain: 'foo', path: '/bar' }) .then(() => { From 9b6db60b655d2c9673c02c01b2bba63fbdc085d8 Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Wed, 8 Feb 2023 12:02:32 -0500 Subject: [PATCH 03/40] feat: use cookie.toString() in the cookie patch to more accurately set cookies on the document, which should include other properties besides key=value --- packages/runner/injection/patches/cookies.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/runner/injection/patches/cookies.ts b/packages/runner/injection/patches/cookies.ts index 3193d062f72e..307428c87d8a 100644 --- a/packages/runner/injection/patches/cookies.ts +++ b/packages/runner/injection/patches/cookies.ts @@ -53,7 +53,7 @@ export const patchDocumentCookie = (requestCookies: AutomationCookie[]) => { const getDocumentCookieValue = () => { return cookieJar.getCookies(url, undefined).map((cookie: ToughCookie) => { - return `${cookie.key}=${cookie.value}` + return cookie.toString() }).join('; ') } From 3a0567577b7dc48b2e26a0636c76df94ab2c96e1 Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Mon, 23 Jan 2023 15:26:18 -0500 Subject: [PATCH 04/40] fix: add logic to handle expired cookies in the document.cookie patch, as well as in CDP --- packages/runner/injection/patches/cookies.ts | 46 +++++++++++++------- packages/server/lib/automation/cookies.ts | 10 ++++- packages/server/lib/util/cookies.ts | 16 ++++++- 3 files changed, 52 insertions(+), 20 deletions(-) diff --git a/packages/runner/injection/patches/cookies.ts b/packages/runner/injection/patches/cookies.ts index 307428c87d8a..249ef59606b4 100644 --- a/packages/runner/injection/patches/cookies.ts +++ b/packages/runner/injection/patches/cookies.ts @@ -91,23 +91,37 @@ export const patchDocumentCookie = (requestCookies: AutomationCookie[]) => { // if result is undefined, it was invalid and couldn't be parsed if (!parsedCookie) return getDocumentCookieValue() - // we should be able to pass in parsedCookie here instead of the string - // value, but tough-cookie doesn't recognize it using an instanceof - // check and throws an error. because we can't, we have to massage - // some of the properties below to be correct - const cookie = setCookie(stringValue) - - if (cookie) { - cookie.sameSite = parsedCookie.sameSite - - if (!parsedCookie.path) { - cookie.path = '/' + // if the cookie is not expired, don't set it + if (parsedCookie.expiryTime() < Date.now()) { + cookieJar.removeCookie({ + name: parsedCookie.key, + path: parsedCookie.path || '/', + domain: parsedCookie.domain as string, + }) + + // send the cookie to the server so it can be removed from the browser + // via aututomation. If the cookie expiry is set inside the server-side cookie jar, + // the cookie will be automatically removed. + sendCookieToServer(toughCookieToAutomationCookie(parsedCookie, domain)) + } else { + // we should be able to pass in parsedCookie here instead of the string + // value, but tough-cookie doesn't recognize it using an instanceof + // check and throws an error. because we can't, we have to massage + // some of the properties below to be correct + const cookie = setCookie(stringValue) + + if (cookie) { + cookie.sameSite = parsedCookie.sameSite + + if (!parsedCookie.path) { + cookie.path = '/' + } + + // send the cookie to the server so it can be set in the browser via + // automation and in our server-side cookie jar so it's available + // to subsequent injections + sendCookieToServer(toughCookieToAutomationCookie(cookie, domain)) } - - // send the cookie to the server so it can be set in the browser via - // automation and in our server-side cookie jar so it's available - // to subsequent injections - sendCookieToServer(toughCookieToAutomationCookie(cookie, domain)) } return getDocumentCookieValue() diff --git a/packages/server/lib/automation/cookies.ts b/packages/server/lib/automation/cookies.ts index cd9aac763523..677826779c8e 100644 --- a/packages/server/lib/automation/cookies.ts +++ b/packages/server/lib/automation/cookies.ts @@ -5,7 +5,7 @@ import { isHostOnlyCookie } from '../browsers/cdp_automation' export interface AutomationCookie { domain: string - expiry: number | null + expiry: 'Infinity' | '-Infinity' | number | null httpOnly: boolean maxAge: 'Infinity' | '-Infinity' | number | null name: string @@ -33,7 +33,13 @@ const normalizeCookieProps = function (props) { const cookie = _.pick(props, COOKIE_PROPERTIES) - if (props.expiry != null) { + if (props.expiry === '-Infinity') { + cookie.expiry = -Infinity + // set the cookie to expired so when set, the cookie is removed + cookie.expirationDate = 0 + } else if (props.expiry === 'Infinity') { + cookie.expiry = null + } else if (props.expiry != null) { // when sending cookie props we need to convert // expiry to expirationDate delete cookie.expiry diff --git a/packages/server/lib/util/cookies.ts b/packages/server/lib/util/cookies.ts index 71ca290c1d49..aff04106b1cc 100644 --- a/packages/server/lib/util/cookies.ts +++ b/packages/server/lib/util/cookies.ts @@ -16,7 +16,8 @@ export const toughCookieToAutomationCookie = (toughCookie: Cookie, defaultDomain return { domain: toughCookie.domain || defaultDomain, - expiry: isFinite(expiry) ? expiry / 1000 : null, + // if expiry is Infinity or -Infinity, this operation is a no-op + expiry: (expiry === Infinity || expiry === -Infinity) ? expiry.toString() as '-Infinity' | 'Infinity' : expiry / 1000, httpOnly: toughCookie.httpOnly, maxAge: toughCookie.maxAge, name: toughCookie.key, @@ -32,9 +33,20 @@ export const toughCookieToAutomationCookie = (toughCookie: Cookie, defaultDomain } export const automationCookieToToughCookie = (automationCookie: AutomationCookie, defaultDomain: string): Cookie => { + let expiry: Date | undefined = undefined + + if (automationCookie.expiry != null) { + if (isFinite(automationCookie.expiry as number)) { + expiry = new Date(automationCookie.expiry as number * 1000) + } else if (automationCookie.expiry === '-Infinity') { + // if negative Infinity, the cookie is Date(0), has expired and is slated to be removed + expiry = new Date(0) + } + } + const cookie = new Cookie({ domain: automationCookie.domain || defaultDomain, - expires: automationCookie.expiry != null && isFinite(automationCookie.expiry) ? new Date(automationCookie.expiry * 1000) : undefined, + expires: expiry, httpOnly: automationCookie.httpOnly, maxAge: automationCookie.maxAge || 'Infinity', key: automationCookie.name, From 27c607636bb41be14f439532567c039c66e57c2b Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Thu, 9 Feb 2023 14:01:22 -0500 Subject: [PATCH 05/40] chore: build binary for cookie fixes for users to test --- .circleci/workflows.yml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.circleci/workflows.yml b/.circleci/workflows.yml index 15701f41fac8..33d3226e8923 100644 --- a/.circleci/workflows.yml +++ b/.circleci/workflows.yml @@ -28,7 +28,7 @@ mainBuildFilters: &mainBuildFilters only: - develop - /^release\/\d+\.\d+\.\d+$/ - - 'emily/next-version' + - 'fix-duplicate-and-expired-cookies' # usually we don't build Mac app - it takes a long time # but sometimes we want to really confirm we are doing the right thing @@ -37,7 +37,7 @@ macWorkflowFilters: &darwin-workflow-filters when: or: - equal: [ develop, << pipeline.git.branch >> ] - - equal: [ 'mschile/chrome_memory_fix', << pipeline.git.branch >> ] + - equal: [ 'fix-duplicate-and-expired-cookies', << pipeline.git.branch >> ] - matches: pattern: /^release\/\d+\.\d+\.\d+$/ value: << pipeline.git.branch >> @@ -46,7 +46,7 @@ linuxArm64WorkflowFilters: &linux-arm64-workflow-filters when: or: - equal: [ develop, << pipeline.git.branch >> ] - - equal: [ 'mschile/chrome_memory_fix', << pipeline.git.branch >> ] + - equal: [ 'fix-duplicate-and-expired-cookies', << pipeline.git.branch >> ] - matches: pattern: /^release\/\d+\.\d+\.\d+$/ value: << pipeline.git.branch >> @@ -64,7 +64,7 @@ windowsWorkflowFilters: &windows-workflow-filters when: or: - equal: [ develop, << pipeline.git.branch >> ] - - equal: [ 'mschile/chrome_memory_fix', << pipeline.git.branch >> ] + - equal: [ 'fix-duplicate-and-expired-cookies', << pipeline.git.branch >> ] - matches: pattern: /^release\/\d+\.\d+\.\d+$/ value: << pipeline.git.branch >> @@ -130,7 +130,7 @@ commands: - run: name: Check current branch to persist artifacts command: | - if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "emily/next-version" ]]; then + if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "fix-duplicate-and-expired-cookies" ]]; then echo "Not uploading artifacts or posting install comment for this branch." circleci-agent step halt fi From 70b709c4f3e35417f7920fd028bd42f531218789 Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Thu, 9 Feb 2023 14:15:22 -0500 Subject: [PATCH 06/40] chore: change name of fixture to something more accurate --- packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts | 2 +- ...y-cookie.html => trigger-cross-origin-redirect-to-self.html} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename packages/driver/cypress/fixtures/{duplicate-hostOnly-cookie.html => trigger-cross-origin-redirect-to-self.html} (100%) diff --git a/packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts b/packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts index d2a13aabc0c6..2bb6398d4049 100644 --- a/packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts +++ b/packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts @@ -4,7 +4,7 @@ describe('misc cookie tests', () => { // NOTE: For this test to work correctly, we need to have a FQDN, not localhost (www.foobar.com). // FIXES: https://github.com/cypress-io/cypress/issues/25174 (cookies are duplicated with prepended dot (.)) it('does not duplicate cookies with a prepended dot for cookies that are stored inside the server side cookie jar', () => { - cy.visit('https://www.foobar.com:3502/fixtures/duplicate-hostOnly-cookie.html') + cy.visit('https://www.foobar.com:3502/fixtures/trigger-cross-origin-redirect-to-self.html') // does a 302 redirect back to www.foobar.com primary-origin page, but sets a sameSite=None cookie cy.get('[data-cy="cookie-cross-origin-redirects"]').click() diff --git a/packages/driver/cypress/fixtures/duplicate-hostOnly-cookie.html b/packages/driver/cypress/fixtures/trigger-cross-origin-redirect-to-self.html similarity index 100% rename from packages/driver/cypress/fixtures/duplicate-hostOnly-cookie.html rename to packages/driver/cypress/fixtures/trigger-cross-origin-redirect-to-self.html From 91c3c7cd0e3b4fbb86176d028c5d243ff17a49e9 Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Thu, 9 Feb 2023 14:15:54 -0500 Subject: [PATCH 07/40] chore: comment why we are using the toughCookie toString method in the patch --- packages/runner/injection/patches/cookies.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/runner/injection/patches/cookies.ts b/packages/runner/injection/patches/cookies.ts index 249ef59606b4..f95a1f6a7590 100644 --- a/packages/runner/injection/patches/cookies.ts +++ b/packages/runner/injection/patches/cookies.ts @@ -53,6 +53,7 @@ export const patchDocumentCookie = (requestCookies: AutomationCookie[]) => { const getDocumentCookieValue = () => { return cookieJar.getCookies(url, undefined).map((cookie: ToughCookie) => { + // opt for the TouchCookie.toString() method to get cookie type, domain, expiry, and other attributes that might be set on the cookie that are pertinent return cookie.toString() }).join('; ') } From dcdb11c53d6491843e13088dc43f5ce49023fdb3 Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Thu, 9 Feb 2023 14:16:07 -0500 Subject: [PATCH 08/40] [run ci] From 844c8315b4bc666a1b203c6200360e8023d39824 Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Thu, 9 Feb 2023 14:18:46 -0500 Subject: [PATCH 09/40] chore: add changelog entry --- cli/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index 1d9bfef2f316..699095d56991 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -6,6 +6,7 @@ _Released 02/14/2023 (PENDING)_ **Bugfixes:** - Fixed an issue with the Cloud project selection modal not showing the correct prompts. Fixes [#25520](https://github.com/cypress-io/cypress/issues/25520). + - Fixed an issue where cookies were being duplicated with the same hostname, but a prepended dot. Fixed an issue where cookies may not be expiring correctly. Fixes [#25174](https://github.com/cypress-io/cypress/issues/25174), [#25205](https://github.com/cypress-io/cypress/issues/25205) and [#25495](https://github.com/cypress-io/cypress/issues/25495). **Features:** From 38a65a6d7c04dff8e5663d9fee1b7df1b5828fb4 Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Thu, 9 Feb 2023 14:20:24 -0500 Subject: [PATCH 10/40] [run ci] From 4a9cfb5f4ab845022a78de0095ca4efd39f2662d Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Mon, 13 Feb 2023 13:28:35 -0500 Subject: [PATCH 11/40] fix: revert back to key=value when getting document.cookie as those are the only values are displayed (oversight on my end) --- packages/runner/injection/patches/cookies.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/runner/injection/patches/cookies.ts b/packages/runner/injection/patches/cookies.ts index f95a1f6a7590..68eeeaf6c2f4 100644 --- a/packages/runner/injection/patches/cookies.ts +++ b/packages/runner/injection/patches/cookies.ts @@ -53,8 +53,7 @@ export const patchDocumentCookie = (requestCookies: AutomationCookie[]) => { const getDocumentCookieValue = () => { return cookieJar.getCookies(url, undefined).map((cookie: ToughCookie) => { - // opt for the TouchCookie.toString() method to get cookie type, domain, expiry, and other attributes that might be set on the cookie that are pertinent - return cookie.toString() + return `${cookie.key}=${cookie.value}` }).join('; ') } From 8927a3c67f258688f7b9128fb36c14b1c26d94b5 Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Mon, 13 Feb 2023 13:29:02 -0500 Subject: [PATCH 12/40] [run ci] From 864baff2fd4bc2ff3c3ca4e0c011fe5edf68032a Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Mon, 13 Feb 2023 14:39:32 -0500 Subject: [PATCH 13/40] chore: make compatible with cypress.require --- .../cypress/e2e/e2e/origin/cookie_misc.cy.ts | 23 ++++++++----------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts b/packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts index 2bb6398d4049..1f848137c63f 100644 --- a/packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts +++ b/packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts @@ -1,5 +1,3 @@ -import { makeRequestForCookieBehaviorTests as makeRequest } from '../../../support/utils' - describe('misc cookie tests', () => { // NOTE: For this test to work correctly, we need to have a FQDN, not localhost (www.foobar.com). // FIXES: https://github.com/cypress-io/cypress/issues/25174 (cookies are duplicated with prepended dot (.)) @@ -29,10 +27,7 @@ describe('misc cookie tests', () => { describe('expiring cookies', () => { before(() => { cy.origin(`https://app.foobar.com:3503`, () => { - const { makeRequestForCookieBehaviorTests: makeRequest } = require('../../../support/utils') - - // @ts-ignore - window.makeRequest = makeRequest + window.makeRequest = Cypress.require('../../../support/utils').makeRequestForCookieBehaviorTests }) }) @@ -43,13 +38,13 @@ describe('misc cookie tests', () => { cy.visit(`https://app.foobar.com:3503/fixtures/secondary-origin.html`) cy.origin(`https://app.foobar.com:3503`, () => { cy.window().then((win) => { - return cy.wrap(makeRequest(win, `/set-cookie?cookie=foo=bar; Domain=.foobar.com;`)) + return cy.wrap(window.makeRequest(win, `/set-cookie?cookie=foo=bar; Domain=.foobar.com;`)) }) cy.getCookie('foo').its('value').should('eq', 'bar') cy.window().then((win) => { - return cy.wrap(makeRequest(win, `/set-cookie?cookie=foo=deleted; Domain=.foobar.com; Max-Age=0;`)) + return cy.wrap(window.makeRequest(win, `/set-cookie?cookie=foo=deleted; Domain=.foobar.com; Max-Age=0;`)) }) cy.getCookie('foo').should('eq', null) @@ -62,13 +57,13 @@ describe('misc cookie tests', () => { cy.visit(`https://app.foobar.com:3503/fixtures/secondary-origin.html`) cy.origin(`https://app.foobar.com:3503`, () => { cy.window().then((win) => { - return cy.wrap(makeRequest(win, `/set-cookie?cookie=foo=bar; Domain=.foobar.com;`)) + return cy.wrap(window.makeRequest(win, `/set-cookie?cookie=foo=bar; Domain=.foobar.com;`)) }) cy.getCookie('foo').its('value').should('eq', 'bar') cy.window().then((win) => { - return cy.wrap(makeRequest(win, `/set-cookie?cookie=foo=deleted; Domain=.foobar.com; expires=Thu, 01-Jan-1970 00:00:01 GMT;`)) + return cy.wrap(window.makeRequest(win, `/set-cookie?cookie=foo=deleted; Domain=.foobar.com; expires=Thu, 01-Jan-1970 00:00:01 GMT;`)) }) cy.getCookie('foo').should('eq', null) @@ -81,13 +76,13 @@ describe('misc cookie tests', () => { cy.visit(`https://app.foobar.com:3503/fixtures/secondary-origin.html`) cy.origin(`https://app.foobar.com:3503`, () => { cy.window().then((win) => { - return cy.wrap(makeRequest(win, `/set-cookie?cookie=foo=bar; Domain=.foobar.com;`)) + return cy.wrap(window.makeRequest(win, `/set-cookie?cookie=foo=bar; Domain=.foobar.com;`)) }) cy.getCookie('foo').its('value').should('eq', 'bar') cy.window().then((win) => { - return cy.wrap(makeRequest(win, `/set-cookie?cookie=foo=deleted; Domain=.foobar.com; expires=Tues, 01-Jan-1980 00:00:01 GMT; Max-Age=0;`)) + return cy.wrap(window.makeRequest(win, `/set-cookie?cookie=foo=deleted; Domain=.foobar.com; expires=Tues, 01-Jan-1980 00:00:01 GMT; Max-Age=0;`)) }) cy.getCookie('foo').should('eq', null) @@ -100,13 +95,13 @@ describe('misc cookie tests', () => { cy.visit(`https://app.foobar.com:3503/fixtures/secondary-origin.html`) cy.origin(`https://app.foobar.com:3503`, () => { cy.window().then((win) => { - return cy.wrap(makeRequest(win, `/set-cookie?cookie=foo=bar; Domain=.foobar.com;`)) + return cy.wrap(window.makeRequest(win, `/set-cookie?cookie=foo=bar; Domain=.foobar.com;`)) }) cy.getCookie('foo').its('value').should('eq', 'bar') cy.window().then((win) => { - return cy.wrap(makeRequest(win, `/set-cookie?cookie=foo=deleted; Domain=.foobar.com; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0;`)) + return cy.wrap(window.makeRequest(win, `/set-cookie?cookie=foo=deleted; Domain=.foobar.com; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0;`)) }) cy.getCookie('foo').should('eq', null) From 868c52c7567d47b67fceee1c5196e51fcce75746 Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Mon, 13 Feb 2023 15:53:39 -0500 Subject: [PATCH 14/40] fix: add tests for hostOnly/non hostOnly cookies to make sure property gets sent up to automation client correctly. No longer need custom cookie prop to determine destination --- .../cypress/e2e/e2e/origin/cookie_misc.cy.ts | 21 +++++++++++++++++-- ...trigger-cross-origin-redirect-to-self.html | 6 ++++-- packages/proxy/lib/http/util/cookies.ts | 4 +--- packages/runner/injection/patches/cookies.ts | 15 +++++++++++-- packages/server/lib/automation/cookies.ts | 13 ++++++------ .../server/lib/browsers/cdp_automation.ts | 3 +-- packages/server/lib/util/cookies.ts | 18 +++++----------- 7 files changed, 49 insertions(+), 31 deletions(-) diff --git a/packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts b/packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts index 1f848137c63f..8ef0e1b5f7ca 100644 --- a/packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts +++ b/packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts @@ -1,11 +1,11 @@ describe('misc cookie tests', () => { // NOTE: For this test to work correctly, we need to have a FQDN, not localhost (www.foobar.com). // FIXES: https://github.com/cypress-io/cypress/issues/25174 (cookies are duplicated with prepended dot (.)) - it('does not duplicate cookies with a prepended dot for cookies that are stored inside the server side cookie jar', () => { + it('does not duplicate cookies with a prepended dot for cookies that are stored inside the server side cookie jar (host only)', () => { cy.visit('https://www.foobar.com:3502/fixtures/trigger-cross-origin-redirect-to-self.html') // does a 302 redirect back to www.foobar.com primary-origin page, but sets a sameSite=None cookie - cy.get('[data-cy="cookie-cross-origin-redirects"]').click() + cy.get('[data-cy="cookie-cross-origin-redirects-host-only"]').click() cy.getCookies({ domain: 'www.foobar.com' }).then((cookies) => { expect(cookies).to.have.length(1) @@ -18,6 +18,23 @@ describe('misc cookie tests', () => { }) }) + it('does not duplicate cookies with a prepended dot for cookies that are stored inside the server side cookie jar (non-host only)', () => { + cy.visit('https://www.foobar.com:3502/fixtures/trigger-cross-origin-redirect-to-self.html') + + // does a 302 redirect back to www.foobar.com primary-origin page, but sets a sameSite=None cookie + cy.get('[data-cy="cookie-cross-origin-redirects"]').click() + + cy.getCookies({ domain: 'www.foobar.com' }).then((cookies) => { + expect(cookies).to.have.length(1) + + const singleCookie = cookies[0] + + expect(singleCookie).to.have.property('name', 'foo') + expect(singleCookie).to.have.property('value', 'bar') + expect(singleCookie).to.have.property('domain', '.www.foobar.com') + }) + }) + /** * FIXES: * https://github.com/cypress-io/cypress/issues/25205 (cookies set with expired time with value deleted show up as set with value deleted) diff --git a/packages/driver/cypress/fixtures/trigger-cross-origin-redirect-to-self.html b/packages/driver/cypress/fixtures/trigger-cross-origin-redirect-to-self.html index ed604187d087..3dc647db6925 100644 --- a/packages/driver/cypress/fixtures/trigger-cross-origin-redirect-to-self.html +++ b/packages/driver/cypress/fixtures/trigger-cross-origin-redirect-to-self.html @@ -3,7 +3,8 @@ - Trigger Cross Origin Redirect Loop that sets cookies + Trigger Cross Origin Redirect Loop that sets host only cookie + Trigger Cross Origin Redirect Loop that sets non host only cookie diff --git a/packages/proxy/lib/http/util/cookies.ts b/packages/proxy/lib/http/util/cookies.ts index 27943cc0eaf3..dab80c9b7c8d 100644 --- a/packages/proxy/lib/http/util/cookies.ts +++ b/packages/proxy/lib/http/util/cookies.ts @@ -237,11 +237,9 @@ export class CookiesHelper { } try { - this.cookieJar.setCookie(toughCookie, this.request.url, this.sameSiteContext) // If a cookie is stored in the jar, we want to make sure that the domain set in the cookie here MATCHES // the domain of what is set in CDP to prevent duplications or strange overwrite behavior - // @ts-expect-error - toughCookie.isStoredInServerSideCookieJar = true + this.cookieJar.setCookie(toughCookie, this.request.url, this.sameSiteContext) } catch (err) { this.debug('adding cookie to jar failed: %s', err.message) } diff --git a/packages/runner/injection/patches/cookies.ts b/packages/runner/injection/patches/cookies.ts index 68eeeaf6c2f4..89f013d93684 100644 --- a/packages/runner/injection/patches/cookies.ts +++ b/packages/runner/injection/patches/cookies.ts @@ -6,18 +6,25 @@ import { import { Cookie as ToughCookie } from 'tough-cookie' import type { AutomationCookie } from '@packages/server/lib/automation/cookies' +function isHostOnlyCookie (domain) { + return domain[0] !== '.' +} + const parseDocumentCookieString = (documentCookieString: string): AutomationCookie[] => { if (!documentCookieString || !documentCookieString.trim().length) return [] return documentCookieString.split(';').map((cookieString) => { - const [name, value] = cookieString.split('=') + const [name, value, domain] = cookieString.split('=') + + let cookieDomain = domain || location.hostname return { name: name.trim(), value: value.trim(), - domain: location.hostname, + domain: cookieDomain, expiry: null, httpOnly: false, + hostOnly: isHostOnlyCookie(cookieDomain), maxAge: null, path: null, sameSite: 'lax', @@ -88,6 +95,10 @@ export const patchDocumentCookie = (requestCookies: AutomationCookie[]) => { const stringValue = `${newValue}` const parsedCookie = CookieJar.parse(stringValue) + if (parsedCookie?.hostOnly === null) { + parsedCookie.hostOnly = isHostOnlyCookie(parsedCookie.domain || domain) + } + // if result is undefined, it was invalid and couldn't be parsed if (!parsedCookie) return getDocumentCookieValue() diff --git a/packages/server/lib/automation/cookies.ts b/packages/server/lib/automation/cookies.ts index 677826779c8e..a1211d89d6cb 100644 --- a/packages/server/lib/automation/cookies.ts +++ b/packages/server/lib/automation/cookies.ts @@ -7,6 +7,7 @@ export interface AutomationCookie { domain: string expiry: 'Infinity' | '-Infinity' | number | null httpOnly: boolean + hostOnly: boolean maxAge: 'Infinity' | '-Infinity' | number | null name: string path: string | null @@ -31,6 +32,11 @@ const normalizeCookieProps = function (props) { return props } + // if the cookie is stored inside the server side cookie jar, + // we want to make the automation client aware so the domain property + // isn't mutated to prevent duplicate setting of cookies from different contexts. + // This should be handled by the hostOnly property + const cookie = _.pick(props, COOKIE_PROPERTIES) if (props.expiry === '-Infinity') { @@ -52,13 +58,6 @@ const normalizeCookieProps = function (props) { cookie.expiry = props.expirationDate } - // if the cookie is stored inside the server side cookie jar, - // we want to make the automation client aware so the domain property - // isn't mutated to prevent duplicate setting of cookies from different contexts - if (props.isStoredInServerSideCookieJar) { - cookie.isStoredInServerSideCookieJar = true - } - return cookie } diff --git a/packages/server/lib/browsers/cdp_automation.ts b/packages/server/lib/browsers/cdp_automation.ts index 843dcd775eaf..d2d07f11d408 100644 --- a/packages/server/lib/browsers/cdp_automation.ts +++ b/packages/server/lib/browsers/cdp_automation.ts @@ -110,8 +110,7 @@ const normalizeSetCookieProps = (cookie: CyCookie): Protocol.Network.SetCookieRe // without this logic, a cookie being set on 'foo.com' will only be set for 'foo.com', not other subdomains // However, we do NOT want to prefix the domain for cookies that are stored inside our server-side cookie jar, // as it might lead to duplicate cookies when another request or script overrides the value - // @ts-expect-error - if (!cookie.hostOnly && isHostOnlyCookie(cookie) && !cookie.isStoredInServerSideCookieJar) { + if (!cookie.hostOnly && isHostOnlyCookie(cookie)) { setCookieRequest.domain = `.${cookie.domain}` } diff --git a/packages/server/lib/util/cookies.ts b/packages/server/lib/util/cookies.ts index aff04106b1cc..f0f46c17138a 100644 --- a/packages/server/lib/util/cookies.ts +++ b/packages/server/lib/util/cookies.ts @@ -19,16 +19,14 @@ export const toughCookieToAutomationCookie = (toughCookie: Cookie, defaultDomain // if expiry is Infinity or -Infinity, this operation is a no-op expiry: (expiry === Infinity || expiry === -Infinity) ? expiry.toString() as '-Infinity' | 'Infinity' : expiry / 1000, httpOnly: toughCookie.httpOnly, + // we want to make sure the hostOnly property is respected when syncing with CDP/extension to prevent duplicates + hostOnly: toughCookie.hostOnly || false, maxAge: toughCookie.maxAge, name: toughCookie.key, path: toughCookie.path, sameSite: toughCookie.sameSite === 'none' ? 'no_restriction' : toughCookie.sameSite, secure: toughCookie.secure, value: toughCookie.value, - // @ts-expect-error - ...(toughCookie.isStoredInServerSideCookieJar ? { - isStoredInServerSideCookieJar: true, - } : {}), } } @@ -44,10 +42,12 @@ export const automationCookieToToughCookie = (automationCookie: AutomationCookie } } - const cookie = new Cookie({ + return new Cookie({ domain: automationCookie.domain || defaultDomain, expires: expiry, httpOnly: automationCookie.httpOnly, + // we want to make sure the hostOnly property is respected when syncing with CDP/extension to prevent duplicates + hostOnly: automationCookie.hostOnly || false, maxAge: automationCookie.maxAge || 'Infinity', key: automationCookie.name, path: automationCookie.path || undefined, @@ -55,14 +55,6 @@ export const automationCookieToToughCookie = (automationCookie: AutomationCookie secure: automationCookie.secure, value: automationCookie.value, }) - - // @ts-expect-error - if (automationCookie.isStoredInServerSideCookieJar) { - // @ts-expect-error - cookie.isStoredInServerSideCookieJar = true - } - - return cookie } const sameSiteNoneRe = /; +samesite=(?:'none'|"none"|none)/i From f545ab12b81888a231f529e8d496e2f1bf92e60a Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Mon, 13 Feb 2023 15:54:06 -0500 Subject: [PATCH 15/40] [run ci] From 5b0a9bf27fac8768bcca2d37a06e10cf80e7a610 Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Mon, 13 Feb 2023 15:57:46 -0500 Subject: [PATCH 16/40] fix: stale unit test --- packages/server/test/unit/browsers/cdp_automation_spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/server/test/unit/browsers/cdp_automation_spec.ts b/packages/server/test/unit/browsers/cdp_automation_spec.ts index ff5c9645e45b..b54e262b4788 100644 --- a/packages/server/test/unit/browsers/cdp_automation_spec.ts +++ b/packages/server/test/unit/browsers/cdp_automation_spec.ts @@ -204,7 +204,7 @@ context('lib/browsers/cdp_automation', () => { }) }) - it('does not mutate domain property if cookie is set in the server side cookie jar to prevent duplication / inconsistent overwrite behavior', function () { + it('resolves with the cookie props (host only)', function () { this.sendDebuggerCommand .withArgs('Network.setCookie', { domain: 'google.com', name: 'session', value: 'key', path: '/' }) .resolves({ success: true }) @@ -215,7 +215,7 @@ context('lib/browsers/cdp_automation', () => { ], }) - return this.onRequest('set:cookie', { domain: 'google.com', name: 'session', value: 'key', path: '/', isStoredInServerSideCookieJar: true }) + return this.onRequest('set:cookie', { domain: 'google.com', name: 'session', value: 'key', path: '/', hostOnly: true }) .then((resp) => { expect(resp).to.deep.eq({ domain: 'google.com', expirationDate: undefined, hostOnly: true, httpOnly: false, name: 'session', value: 'key', path: '/', secure: false, sameSite: undefined }) }) From ea31a02a75920a4d745cd171eab339157e85128f Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Mon, 13 Feb 2023 15:58:59 -0500 Subject: [PATCH 17/40] chore: adjust comments --- packages/proxy/lib/http/util/cookies.ts | 2 -- packages/runner/injection/patches/cookies.ts | 2 ++ 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/proxy/lib/http/util/cookies.ts b/packages/proxy/lib/http/util/cookies.ts index dab80c9b7c8d..a320a7b570ce 100644 --- a/packages/proxy/lib/http/util/cookies.ts +++ b/packages/proxy/lib/http/util/cookies.ts @@ -237,8 +237,6 @@ export class CookiesHelper { } try { - // If a cookie is stored in the jar, we want to make sure that the domain set in the cookie here MATCHES - // the domain of what is set in CDP to prevent duplications or strange overwrite behavior this.cookieJar.setCookie(toughCookie, this.request.url, this.sameSiteContext) } catch (err) { this.debug('adding cookie to jar failed: %s', err.message) diff --git a/packages/runner/injection/patches/cookies.ts b/packages/runner/injection/patches/cookies.ts index 89f013d93684..f66061eb6164 100644 --- a/packages/runner/injection/patches/cookies.ts +++ b/packages/runner/injection/patches/cookies.ts @@ -96,6 +96,8 @@ export const patchDocumentCookie = (requestCookies: AutomationCookie[]) => { const parsedCookie = CookieJar.parse(stringValue) if (parsedCookie?.hostOnly === null) { + // we want to make sure the hostOnly property is respected when syncing with CDP/extension to prevent duplicates. + // in the case it is not set, we need to calculate it parsedCookie.hostOnly = isHostOnlyCookie(parsedCookie.domain || domain) } From b93ce75b69458654a7201ae12feee8da34bdc472 Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Mon, 13 Feb 2023 15:59:07 -0500 Subject: [PATCH 18/40] [run ci] From 48b95503d31e922083818420c137ce0089dfc359 Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Mon, 13 Feb 2023 16:00:30 -0500 Subject: [PATCH 19/40] fix: bad domain logic --- packages/runner/injection/patches/cookies.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/runner/injection/patches/cookies.ts b/packages/runner/injection/patches/cookies.ts index f66061eb6164..a2d7a7214c3b 100644 --- a/packages/runner/injection/patches/cookies.ts +++ b/packages/runner/injection/patches/cookies.ts @@ -14,9 +14,9 @@ const parseDocumentCookieString = (documentCookieString: string): AutomationCook if (!documentCookieString || !documentCookieString.trim().length) return [] return documentCookieString.split(';').map((cookieString) => { - const [name, value, domain] = cookieString.split('=') + const [name, value] = cookieString.split('=') - let cookieDomain = domain || location.hostname + let cookieDomain = location.hostname return { name: name.trim(), From 2df76b71ddaa67236bdeffde4413cc6cf920eabe Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Mon, 13 Feb 2023 16:00:35 -0500 Subject: [PATCH 20/40] [run ci] From 3431437c31f8aa71e315a1ba25b3acda7aa0e995 Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Mon, 13 Feb 2023 16:02:04 -0500 Subject: [PATCH 21/40] chore: remove irrelevant comment --- packages/server/lib/browsers/cdp_automation.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/server/lib/browsers/cdp_automation.ts b/packages/server/lib/browsers/cdp_automation.ts index d2d07f11d408..b43e65763f19 100644 --- a/packages/server/lib/browsers/cdp_automation.ts +++ b/packages/server/lib/browsers/cdp_automation.ts @@ -108,8 +108,6 @@ const normalizeSetCookieProps = (cookie: CyCookie): Protocol.Network.SetCookieRe .value() // without this logic, a cookie being set on 'foo.com' will only be set for 'foo.com', not other subdomains - // However, we do NOT want to prefix the domain for cookies that are stored inside our server-side cookie jar, - // as it might lead to duplicate cookies when another request or script overrides the value if (!cookie.hostOnly && isHostOnlyCookie(cookie)) { setCookieRequest.domain = `.${cookie.domain}` } From c1d8360dbbd8853bcb7c43276351a5c5fc6cfb3a Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Mon, 13 Feb 2023 16:03:21 -0500 Subject: [PATCH 22/40] [run ci] From 22c46f3c58f6f48aefa1f021653b6fce586d8a97 Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Tue, 14 Feb 2023 09:46:58 -0500 Subject: [PATCH 23/40] fix: adjust cookie login text to spec hostOnly cookie within the cookie patch. This should yield the same behavior as we are bound to same origin within the spec bridge --- packages/driver/cypress/e2e/e2e/origin/cookie_login.cy.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/driver/cypress/e2e/e2e/origin/cookie_login.cy.ts b/packages/driver/cypress/e2e/e2e/origin/cookie_login.cy.ts index 679bb32419cc..e54da5957830 100644 --- a/packages/driver/cypress/e2e/e2e/origin/cookie_login.cy.ts +++ b/packages/driver/cypress/e2e/e2e/origin/cookie_login.cy.ts @@ -723,7 +723,7 @@ describe('cy.origin - cookie login', { browser: '!webkit' }, () => { cy.getCookie('key').then((cookie) => { expect(Cypress._.omit(cookie, 'expiry')).to.deep.equal({ - domain: '.www.foobar.com', + domain: 'www.foobar.com', httpOnly: false, name: 'key', path: '/fixtures', @@ -851,7 +851,7 @@ describe('cy.origin - cookie login', { browser: '!webkit' }, () => { cy.get('[data-cy="doc-cookie"]').invoke('text').should('equal', 'name=value') cy.getCookie('name').then((cookie) => { expect(Cypress._.omit(cookie, 'expiry')).to.deep.equal({ - domain: '.www.foobar.com', + domain: 'www.foobar.com', httpOnly: false, name: 'name', path: '/', From fd23fb14f7a8ec0bb4df66b38832314c5ce6327c Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Tue, 14 Feb 2023 09:48:26 -0500 Subject: [PATCH 24/40] [run ci] From 331dd6959ae90e838bbb8fdc34eb6dbfc553c08b Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Tue, 14 Feb 2023 09:50:26 -0500 Subject: [PATCH 25/40] [run ci] From 17de1883ab709669922de63793db8539fefa8067 Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Tue, 14 Feb 2023 10:31:23 -0500 Subject: [PATCH 26/40] fix: allow for cookies on request of same key to take precedence over cookies in the jar, regardless of how many hierachy cookies exist in the jar --- packages/proxy/lib/http/util/cookies.ts | 9 ++- .../test/unit/http/request-middleware.spec.ts | 57 +++++++++++-------- .../proxy/test/unit/http/util/cookies.spec.ts | 22 ++++++- 3 files changed, 59 insertions(+), 29 deletions(-) diff --git a/packages/proxy/lib/http/util/cookies.ts b/packages/proxy/lib/http/util/cookies.ts index a320a7b570ce..b88bc242edd7 100644 --- a/packages/proxy/lib/http/util/cookies.ts +++ b/packages/proxy/lib/http/util/cookies.ts @@ -97,17 +97,20 @@ export const addCookieJarCookiesToRequest = (applicableCookieJarCookies: Cookie[ // Always have cookies in the jar overwrite cookies in the request if they are the same requestCookies.forEach((cookie) => cookieMap.set(cookie.key, cookie)) + // Two or more cookies on the same request can happen per https://www.rfc-editor.org/rfc/rfc6265 // But if a value for that cookie already exists in the cookie jar, do NOT add the cookie jar cookie + const cookiesToAddFromJar: Cookie[] = [] + applicableCookieJarCookies.forEach((cookie) => { - if (cookieMap.get(cookie.key)) { - cookieMap.delete(cookie.key) + if (!cookieMap.get(cookie.key)) { + cookiesToAddFromJar.push(cookie) } }) const requestCookiesThatNeedToBeAdded = Array.from(cookieMap).map(([key, cookie]) => cookie) - return applicableCookieJarCookies.concat(requestCookiesThatNeedToBeAdded).map((cookie) => cookie.cookieString()).join('; ') + return cookiesToAddFromJar.concat(requestCookiesThatNeedToBeAdded).map((cookie) => cookie.cookieString()).join('; ') } // sameSiteContext is a concept for tough-cookie's cookie jar that helps it diff --git a/packages/proxy/test/unit/http/request-middleware.spec.ts b/packages/proxy/test/unit/http/request-middleware.spec.ts index 58995e35c758..3d31fdf13760 100644 --- a/packages/proxy/test/unit/http/request-middleware.spec.ts +++ b/packages/proxy/test/unit/http/request-middleware.spec.ts @@ -469,43 +469,50 @@ describe('http/request-middleware', () => { * These tests are to leverage small integration tests between us and tough-cookie to make sure we are adding cookies correctly to the Cookie header given the above circumstances */ describe('duplicate cookies', () => { - describe('does not add request cookie to request if cookie exists in jar, and preserves duplicate cookies when same key/value if', () => { - describe('subdomain and TLD', () => { - it('matches hierarchy', async () => { - const ctx = await getContext(['jar=cookie', 'request=cookie'], ['jar=cookie1; Domain=app.foobar.com', 'jar=cookie2; Domain=foobar.com', 'jar=cookie3; Domain=exclude.foobar.com'], 'http://app.foobar.com/generic', 'http://app.foobar.com/generic') + // we want to take the request cookie over anything that might exist in the jar if the key is the same, as the cookie in the jar may be stale at this point + it('always adds request cookie over cookie jar cookie if one exists', async () => { + const ctx = await getContext(['jar=cookie', 'request=cookie'], ['jar=cookie1; Domain=app.foobar.com', 'jar=cookie2; Domain=foobar.com', 'jar=cookie3; Domain=exclude.foobar.com'], 'http://app.foobar.com/generic', 'http://app.foobar.com/generic') - await testMiddleware([MaybeAttachCrossOriginCookies], ctx) + await testMiddleware([MaybeAttachCrossOriginCookies], ctx) + + expect(ctx.req.headers['cookie']).to.equal('jar=cookie; request=cookie') + }) - expect(ctx.req.headers['cookie']).to.equal('jar=cookie1; jar=cookie2; request=cookie') - }) + describe('if no cookie exists on request, cookie jar cookies preserves duplicate cookies when same key/value if subdomain and TLD', () => { + it('matches hierarchy', async () => { + const ctx = await getContext(['request=cookie'], ['jar=cookie1; Domain=app.foobar.com', 'jar=cookie2; Domain=foobar.com', 'jar=cookie3; Domain=exclude.foobar.com'], 'http://app.foobar.com/generic', 'http://app.foobar.com/generic') - it('matches hierarchy and gives order to the cookie that was created first', async () => { - const ctx = await getContext(['jar=cookie', 'request=cookie'], ['jar=cookie1; Domain=app.foobar.com;', 'jar=cookie2; Domain=.foobar.com;'], 'http://app.foobar.com/generic', 'http://app.foobar.com/generic') + await testMiddleware([MaybeAttachCrossOriginCookies], ctx) - const cookies = ctx.getCookieJar().getCookies('http://app.foobar.com/generic', 'strict') + expect(ctx.req.headers['cookie']).to.equal('jar=cookie1; jar=cookie2; request=cookie') + }) - const TLDCookie = cookies.find((cookie) => cookie.domain === 'foobar.com') + it('matches hierarchy and gives order to the cookie that was created first', async () => { + const ctx = await getContext(['request=cookie'], ['jar=cookie1; Domain=app.foobar.com;', 'jar=cookie2; Domain=.foobar.com;'], 'http://app.foobar.com/generic', 'http://app.foobar.com/generic') - // make the TLD cookie created an hour earlier - TLDCookie?.creation?.setHours(TLDCookie?.creation?.getHours() - 1) - await testMiddleware([MaybeAttachCrossOriginCookies], ctx) + const cookies = ctx.getCookieJar().getCookies('http://app.foobar.com/generic', 'strict') - expect(ctx.req.headers['cookie']).to.equal('jar=cookie2; jar=cookie1; request=cookie') - }) + const TLDCookie = cookies.find((cookie) => cookie.domain === 'foobar.com') + + // make the TLD cookie created an hour earlier + TLDCookie?.creation?.setHours(TLDCookie?.creation?.getHours() - 1) + await testMiddleware([MaybeAttachCrossOriginCookies], ctx) + + expect(ctx.req.headers['cookie']).to.equal('jar=cookie2; jar=cookie1; request=cookie') + }) - it('matches hierarchy and gives order to the cookie with the most specific path, regardless of creation time', async () => { - const ctx = await getContext(['jar=cookie', 'request=cookie'], ['jar=cookie1; Domain=app.foobar.com; Path=/generic', 'jar=cookie2; Domain=.foobar.com;'], 'http://app.foobar.com/generic', 'http://app.foobar.com/generic') + it('matches hierarchy and gives order to the cookie with the most specific path, regardless of creation time', async () => { + const ctx = await getContext(['request=cookie'], ['jar=cookie1; Domain=app.foobar.com; Path=/generic', 'jar=cookie2; Domain=.foobar.com;'], 'http://app.foobar.com/generic', 'http://app.foobar.com/generic') - const cookies = ctx.getCookieJar().getCookies('http://app.foobar.com/generic', 'strict') + const cookies = ctx.getCookieJar().getCookies('http://app.foobar.com/generic', 'strict') - const TLDCookie = cookies.find((cookie) => cookie.domain === 'foobar.com') + const TLDCookie = cookies.find((cookie) => cookie.domain === 'foobar.com') - // make the TLD cookie created an hour earlier - TLDCookie?.creation?.setHours(TLDCookie?.creation?.getHours() - 1) - await testMiddleware([MaybeAttachCrossOriginCookies], ctx) + // make the TLD cookie created an hour earlier + TLDCookie?.creation?.setHours(TLDCookie?.creation?.getHours() - 1) + await testMiddleware([MaybeAttachCrossOriginCookies], ctx) - expect(ctx.req.headers['cookie']).to.equal('jar=cookie1; jar=cookie2; request=cookie') - }) + expect(ctx.req.headers['cookie']).to.equal('jar=cookie1; jar=cookie2; request=cookie') }) }) diff --git a/packages/proxy/test/unit/http/util/cookies.spec.ts b/packages/proxy/test/unit/http/util/cookies.spec.ts index 24a923f0574e..ee50b45b043e 100644 --- a/packages/proxy/test/unit/http/util/cookies.spec.ts +++ b/packages/proxy/test/unit/http/util/cookies.spec.ts @@ -1,5 +1,6 @@ import { expect } from 'chai' -import { calculateSiteContext, getSameSiteContext, shouldAttachAndSetCookies } from '../../../../lib/http/util/cookies' +import { calculateSiteContext, getSameSiteContext, shouldAttachAndSetCookies, addCookieJarCookiesToRequest } from '../../../../lib/http/util/cookies' +import { Cookie } from '@packages/server/lib/util/cookies' context('getSameSiteContext', () => { describe('calculates the same site context correctly for', () => { @@ -263,3 +264,22 @@ context('.calculateSiteContext', () => { expect(calculateSiteContext(autUrl, 'https://staging.google2.com')).to.equal('cross-site') }) }) + +context('.addCookieJarCookiesToRequest', () => { + it('takes the request cookie over the cookie jar cookie if multiple cookies of the same key exist', () => { + const cookieJarCookies = [ + new Cookie({ + key: 'session_id', + value: 'bar', + }), + new Cookie({ + key: 'csrf_token', + value: 'bar', + }), + ] + const requestCookieString = ['session_id=foo', 'csrf_token=foo', '__cypress.initial=true'] + const calculatedRequestCookies = addCookieJarCookiesToRequest(cookieJarCookies, requestCookieString) + + expect(calculatedRequestCookies).to.equal('session_id=foo; csrf_token=foo; __cypress.initial=true') + }) +}) From c3c1088e7c36a7f4663bd005d6577017547e125b Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Tue, 14 Feb 2023 10:35:57 -0500 Subject: [PATCH 27/40] chore: fix cookie misc tests for cy.origin (dont run cy.origin) --- packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts b/packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts index 8ef0e1b5f7ca..98c3f08c54db 100644 --- a/packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts +++ b/packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts @@ -41,7 +41,7 @@ describe('misc cookie tests', () => { * https://github.com/cypress-io/cypress/issues/25495 (session cookies set with expired time with value deleted show up as set with value deleted) * https://github.com/cypress-io/cypress/issues/25148 (cannot log into azure, shows cookies are disabled/blocked) */ - describe('expiring cookies', () => { + describe('expiring cookies', { browser: '!webkit' }, () => { before(() => { cy.origin(`https://app.foobar.com:3503`, () => { window.makeRequest = Cypress.require('../../../support/utils').makeRequestForCookieBehaviorTests From d44b2022c37669c42359fcb5c7918dab62682314 Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Tue, 14 Feb 2023 10:36:21 -0500 Subject: [PATCH 28/40] [run ci] From 173bda06806f1b7e172ac6b5eef22443990c8c3e Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Tue, 14 Feb 2023 11:13:22 -0500 Subject: [PATCH 29/40] chore: skip misc cookie tests in webkit as headless behavior doesn't clear cookies between tests correctly --- packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts b/packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts index 98c3f08c54db..cf5aee56a85d 100644 --- a/packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts +++ b/packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts @@ -1,4 +1,5 @@ -describe('misc cookie tests', () => { +// FIXME: currently cookies aren't cleared properly in headless mode with webkit between tests, as the below tests (excluding cy.origin) pass headfully locally. +describe('misc cookie tests', { browser: '!webkit' }, () => { // NOTE: For this test to work correctly, we need to have a FQDN, not localhost (www.foobar.com). // FIXES: https://github.com/cypress-io/cypress/issues/25174 (cookies are duplicated with prepended dot (.)) it('does not duplicate cookies with a prepended dot for cookies that are stored inside the server side cookie jar (host only)', () => { From dca9773fcf8fc27bd063fe2a211c070786582403 Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Tue, 14 Feb 2023 13:32:13 -0500 Subject: [PATCH 30/40] Revert "fix: allow for cookies on request of same key to take precedence over cookies in the jar, regardless of how many hierachy cookies exist in the jar" This reverts commit 17de1883ab709669922de63793db8539fefa8067. --- packages/proxy/lib/http/util/cookies.ts | 9 +-- .../test/unit/http/request-middleware.spec.ts | 57 ++++++++----------- .../proxy/test/unit/http/util/cookies.spec.ts | 22 +------ 3 files changed, 29 insertions(+), 59 deletions(-) diff --git a/packages/proxy/lib/http/util/cookies.ts b/packages/proxy/lib/http/util/cookies.ts index b88bc242edd7..a320a7b570ce 100644 --- a/packages/proxy/lib/http/util/cookies.ts +++ b/packages/proxy/lib/http/util/cookies.ts @@ -97,20 +97,17 @@ export const addCookieJarCookiesToRequest = (applicableCookieJarCookies: Cookie[ // Always have cookies in the jar overwrite cookies in the request if they are the same requestCookies.forEach((cookie) => cookieMap.set(cookie.key, cookie)) - // Two or more cookies on the same request can happen per https://www.rfc-editor.org/rfc/rfc6265 // But if a value for that cookie already exists in the cookie jar, do NOT add the cookie jar cookie - const cookiesToAddFromJar: Cookie[] = [] - applicableCookieJarCookies.forEach((cookie) => { - if (!cookieMap.get(cookie.key)) { - cookiesToAddFromJar.push(cookie) + if (cookieMap.get(cookie.key)) { + cookieMap.delete(cookie.key) } }) const requestCookiesThatNeedToBeAdded = Array.from(cookieMap).map(([key, cookie]) => cookie) - return cookiesToAddFromJar.concat(requestCookiesThatNeedToBeAdded).map((cookie) => cookie.cookieString()).join('; ') + return applicableCookieJarCookies.concat(requestCookiesThatNeedToBeAdded).map((cookie) => cookie.cookieString()).join('; ') } // sameSiteContext is a concept for tough-cookie's cookie jar that helps it diff --git a/packages/proxy/test/unit/http/request-middleware.spec.ts b/packages/proxy/test/unit/http/request-middleware.spec.ts index 3d31fdf13760..58995e35c758 100644 --- a/packages/proxy/test/unit/http/request-middleware.spec.ts +++ b/packages/proxy/test/unit/http/request-middleware.spec.ts @@ -469,50 +469,43 @@ describe('http/request-middleware', () => { * These tests are to leverage small integration tests between us and tough-cookie to make sure we are adding cookies correctly to the Cookie header given the above circumstances */ describe('duplicate cookies', () => { - // we want to take the request cookie over anything that might exist in the jar if the key is the same, as the cookie in the jar may be stale at this point - it('always adds request cookie over cookie jar cookie if one exists', async () => { - const ctx = await getContext(['jar=cookie', 'request=cookie'], ['jar=cookie1; Domain=app.foobar.com', 'jar=cookie2; Domain=foobar.com', 'jar=cookie3; Domain=exclude.foobar.com'], 'http://app.foobar.com/generic', 'http://app.foobar.com/generic') + describe('does not add request cookie to request if cookie exists in jar, and preserves duplicate cookies when same key/value if', () => { + describe('subdomain and TLD', () => { + it('matches hierarchy', async () => { + const ctx = await getContext(['jar=cookie', 'request=cookie'], ['jar=cookie1; Domain=app.foobar.com', 'jar=cookie2; Domain=foobar.com', 'jar=cookie3; Domain=exclude.foobar.com'], 'http://app.foobar.com/generic', 'http://app.foobar.com/generic') - await testMiddleware([MaybeAttachCrossOriginCookies], ctx) - - expect(ctx.req.headers['cookie']).to.equal('jar=cookie; request=cookie') - }) + await testMiddleware([MaybeAttachCrossOriginCookies], ctx) - describe('if no cookie exists on request, cookie jar cookies preserves duplicate cookies when same key/value if subdomain and TLD', () => { - it('matches hierarchy', async () => { - const ctx = await getContext(['request=cookie'], ['jar=cookie1; Domain=app.foobar.com', 'jar=cookie2; Domain=foobar.com', 'jar=cookie3; Domain=exclude.foobar.com'], 'http://app.foobar.com/generic', 'http://app.foobar.com/generic') + expect(ctx.req.headers['cookie']).to.equal('jar=cookie1; jar=cookie2; request=cookie') + }) - await testMiddleware([MaybeAttachCrossOriginCookies], ctx) + it('matches hierarchy and gives order to the cookie that was created first', async () => { + const ctx = await getContext(['jar=cookie', 'request=cookie'], ['jar=cookie1; Domain=app.foobar.com;', 'jar=cookie2; Domain=.foobar.com;'], 'http://app.foobar.com/generic', 'http://app.foobar.com/generic') - expect(ctx.req.headers['cookie']).to.equal('jar=cookie1; jar=cookie2; request=cookie') - }) + const cookies = ctx.getCookieJar().getCookies('http://app.foobar.com/generic', 'strict') - it('matches hierarchy and gives order to the cookie that was created first', async () => { - const ctx = await getContext(['request=cookie'], ['jar=cookie1; Domain=app.foobar.com;', 'jar=cookie2; Domain=.foobar.com;'], 'http://app.foobar.com/generic', 'http://app.foobar.com/generic') + const TLDCookie = cookies.find((cookie) => cookie.domain === 'foobar.com') - const cookies = ctx.getCookieJar().getCookies('http://app.foobar.com/generic', 'strict') + // make the TLD cookie created an hour earlier + TLDCookie?.creation?.setHours(TLDCookie?.creation?.getHours() - 1) + await testMiddleware([MaybeAttachCrossOriginCookies], ctx) - const TLDCookie = cookies.find((cookie) => cookie.domain === 'foobar.com') - - // make the TLD cookie created an hour earlier - TLDCookie?.creation?.setHours(TLDCookie?.creation?.getHours() - 1) - await testMiddleware([MaybeAttachCrossOriginCookies], ctx) - - expect(ctx.req.headers['cookie']).to.equal('jar=cookie2; jar=cookie1; request=cookie') - }) + expect(ctx.req.headers['cookie']).to.equal('jar=cookie2; jar=cookie1; request=cookie') + }) - it('matches hierarchy and gives order to the cookie with the most specific path, regardless of creation time', async () => { - const ctx = await getContext(['request=cookie'], ['jar=cookie1; Domain=app.foobar.com; Path=/generic', 'jar=cookie2; Domain=.foobar.com;'], 'http://app.foobar.com/generic', 'http://app.foobar.com/generic') + it('matches hierarchy and gives order to the cookie with the most specific path, regardless of creation time', async () => { + const ctx = await getContext(['jar=cookie', 'request=cookie'], ['jar=cookie1; Domain=app.foobar.com; Path=/generic', 'jar=cookie2; Domain=.foobar.com;'], 'http://app.foobar.com/generic', 'http://app.foobar.com/generic') - const cookies = ctx.getCookieJar().getCookies('http://app.foobar.com/generic', 'strict') + const cookies = ctx.getCookieJar().getCookies('http://app.foobar.com/generic', 'strict') - const TLDCookie = cookies.find((cookie) => cookie.domain === 'foobar.com') + const TLDCookie = cookies.find((cookie) => cookie.domain === 'foobar.com') - // make the TLD cookie created an hour earlier - TLDCookie?.creation?.setHours(TLDCookie?.creation?.getHours() - 1) - await testMiddleware([MaybeAttachCrossOriginCookies], ctx) + // make the TLD cookie created an hour earlier + TLDCookie?.creation?.setHours(TLDCookie?.creation?.getHours() - 1) + await testMiddleware([MaybeAttachCrossOriginCookies], ctx) - expect(ctx.req.headers['cookie']).to.equal('jar=cookie1; jar=cookie2; request=cookie') + expect(ctx.req.headers['cookie']).to.equal('jar=cookie1; jar=cookie2; request=cookie') + }) }) }) diff --git a/packages/proxy/test/unit/http/util/cookies.spec.ts b/packages/proxy/test/unit/http/util/cookies.spec.ts index ee50b45b043e..24a923f0574e 100644 --- a/packages/proxy/test/unit/http/util/cookies.spec.ts +++ b/packages/proxy/test/unit/http/util/cookies.spec.ts @@ -1,6 +1,5 @@ import { expect } from 'chai' -import { calculateSiteContext, getSameSiteContext, shouldAttachAndSetCookies, addCookieJarCookiesToRequest } from '../../../../lib/http/util/cookies' -import { Cookie } from '@packages/server/lib/util/cookies' +import { calculateSiteContext, getSameSiteContext, shouldAttachAndSetCookies } from '../../../../lib/http/util/cookies' context('getSameSiteContext', () => { describe('calculates the same site context correctly for', () => { @@ -264,22 +263,3 @@ context('.calculateSiteContext', () => { expect(calculateSiteContext(autUrl, 'https://staging.google2.com')).to.equal('cross-site') }) }) - -context('.addCookieJarCookiesToRequest', () => { - it('takes the request cookie over the cookie jar cookie if multiple cookies of the same key exist', () => { - const cookieJarCookies = [ - new Cookie({ - key: 'session_id', - value: 'bar', - }), - new Cookie({ - key: 'csrf_token', - value: 'bar', - }), - ] - const requestCookieString = ['session_id=foo', 'csrf_token=foo', '__cypress.initial=true'] - const calculatedRequestCookies = addCookieJarCookiesToRequest(cookieJarCookies, requestCookieString) - - expect(calculatedRequestCookies).to.equal('session_id=foo; csrf_token=foo; __cypress.initial=true') - }) -}) From 86a8cd83792a046146bb43d869f8812dd0fc800c Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Tue, 14 Feb 2023 13:33:34 -0500 Subject: [PATCH 31/40] [run ci] From 4352ef5f3ec4bed55ef54933c65966d351d830b7 Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Wed, 15 Feb 2023 10:22:46 -0500 Subject: [PATCH 32/40] chore: split changelog entry into two parts --- cli/CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index 974f0ad1ab56..a83beb8640be 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -5,7 +5,8 @@ _Released 03/1/2023 (PENDING)_ **Bugfixes:** -- Fixed an issue where cookies were being duplicated with the same hostname, but a prepended dot. Fixed an issue where cookies may not be expiring correctly. Fixes [#25174](https://github.com/cypress-io/cypress/issues/25174), [#25205](https://github.com/cypress-io/cypress/issues/25205) and [#25495](https://github.com/cypress-io/cypress/issues/25495). +- Fixed an issue where cookies were being duplicated with the same hostname, but with a prepended dot. Fixes [#25174](https://github.com/cypress-io/cypress/issues/25174). +- Fixed an issue where cookies may not be expiring correctly. Fixes [#25205](https://github.com/cypress-io/cypress/issues/25205) and [#25495](https://github.com/cypress-io/cypress/issues/25495). ## 12.6.0 From a67c06d4b42f9ed3d7368bafe6940d4648bec1d0 Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Wed, 15 Feb 2023 16:10:00 -0500 Subject: [PATCH 33/40] chore: update logic to remove else statement and add comments --- packages/runner/injection/patches/cookies.ts | 42 +++++++++++--------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/packages/runner/injection/patches/cookies.ts b/packages/runner/injection/patches/cookies.ts index a2d7a7214c3b..586191311ded 100644 --- a/packages/runner/injection/patches/cookies.ts +++ b/packages/runner/injection/patches/cookies.ts @@ -104,7 +104,9 @@ export const patchDocumentCookie = (requestCookies: AutomationCookie[]) => { // if result is undefined, it was invalid and couldn't be parsed if (!parsedCookie) return getDocumentCookieValue() - // if the cookie is not expired, don't set it + // if the cookie is expired, remove it in our cookie jar + // and via setting it inside our automation client with the correct expiry. + // This will have the effect of removing the cookie if (parsedCookie.expiryTime() < Date.now()) { cookieJar.removeCookie({ name: parsedCookie.key, @@ -116,25 +118,27 @@ export const patchDocumentCookie = (requestCookies: AutomationCookie[]) => { // via aututomation. If the cookie expiry is set inside the server-side cookie jar, // the cookie will be automatically removed. sendCookieToServer(toughCookieToAutomationCookie(parsedCookie, domain)) - } else { - // we should be able to pass in parsedCookie here instead of the string - // value, but tough-cookie doesn't recognize it using an instanceof - // check and throws an error. because we can't, we have to massage - // some of the properties below to be correct - const cookie = setCookie(stringValue) - - if (cookie) { - cookie.sameSite = parsedCookie.sameSite - - if (!parsedCookie.path) { - cookie.path = '/' - } - - // send the cookie to the server so it can be set in the browser via - // automation and in our server-side cookie jar so it's available - // to subsequent injections - sendCookieToServer(toughCookieToAutomationCookie(cookie, domain)) + + return getDocumentCookieValue() + } + + // we should be able to pass in parsedCookie here instead of the string + // value, but tough-cookie doesn't recognize it using an instanceof + // check and throws an error. because we can't, we have to massage + // some of the properties below to be correct + const cookie = setCookie(stringValue) + + if (cookie) { + cookie.sameSite = parsedCookie.sameSite + + if (!parsedCookie.path) { + cookie.path = '/' } + + // send the cookie to the server so it can be set in the browser via + // automation and in our server-side cookie jar so it's available + // to subsequent injections + sendCookieToServer(toughCookieToAutomationCookie(cookie, domain)) } return getDocumentCookieValue() From 6106b1feb185b993a008927fd0f7c01a72837cee Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Wed, 15 Feb 2023 16:21:38 -0500 Subject: [PATCH 34/40] [run ci] From 0b9e237bddc8c9b600db8f9e946746e482e4770b Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Wed, 15 Feb 2023 16:23:46 -0500 Subject: [PATCH 35/40] chore: readd windows snapshot branch in workflows --- .circleci/workflows.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.circleci/workflows.yml b/.circleci/workflows.yml index a32b8a279d7a..97878988b892 100644 --- a/.circleci/workflows.yml +++ b/.circleci/workflows.yml @@ -64,7 +64,8 @@ windowsWorkflowFilters: &windows-workflow-filters when: or: - equal: [ develop, << pipeline.git.branch >> ] - - equal: [ 'fix-duplicate-and-expired-cookies', << pipeline.git.branch >> ] + # use the following branch as well to ensure that v8 snapshot cache updates are fully tested + - equal: [ 'update-v8-snapshot-cache-on-develop', 'fix-duplicate-and-expired-cookies', << pipeline.git.branch >> ] - matches: pattern: /^release\/\d+\.\d+\.\d+$/ value: << pipeline.git.branch >> From 95589fd19f2faf30bc7adf78838493a877cd6f36 Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Wed, 15 Feb 2023 16:23:50 -0500 Subject: [PATCH 36/40] [run ci] From ef902f17de16db50ccd4c57ccc5d51bee1dad1fc Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Wed, 15 Feb 2023 16:25:52 -0500 Subject: [PATCH 37/40] chore: fix workflows from bad merge --- .circleci/workflows.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.circleci/workflows.yml b/.circleci/workflows.yml index 97878988b892..566e6cd6363f 100644 --- a/.circleci/workflows.yml +++ b/.circleci/workflows.yml @@ -132,7 +132,11 @@ commands: name: Check current branch to persist artifacts command: | if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "fix-duplicate-and-expired-cookies" ]]; then + echo "Not uploading artifacts or posting install comment for this branch." circleci-agent step halt + fi + + restore_workspace_binaries: steps: - attach_workspace: at: ~/ From d5a28761d8d8e0a43d8f4ee5293cfceb38f2011f Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Wed, 15 Feb 2023 16:25:57 -0500 Subject: [PATCH 38/40] [run ci] From 856781222ffa68a2613b4709510de8843d096656 Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Wed, 15 Feb 2023 19:29:48 -0500 Subject: [PATCH 39/40] Revert "chore: split changelog entry into two parts" This reverts commit 4352ef5f3ec4bed55ef54933c65966d351d830b7. --- cli/CHANGELOG.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index a83beb8640be..974f0ad1ab56 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -5,8 +5,7 @@ _Released 03/1/2023 (PENDING)_ **Bugfixes:** -- Fixed an issue where cookies were being duplicated with the same hostname, but with a prepended dot. Fixes [#25174](https://github.com/cypress-io/cypress/issues/25174). -- Fixed an issue where cookies may not be expiring correctly. Fixes [#25205](https://github.com/cypress-io/cypress/issues/25205) and [#25495](https://github.com/cypress-io/cypress/issues/25495). +- Fixed an issue where cookies were being duplicated with the same hostname, but a prepended dot. Fixed an issue where cookies may not be expiring correctly. Fixes [#25174](https://github.com/cypress-io/cypress/issues/25174), [#25205](https://github.com/cypress-io/cypress/issues/25205) and [#25495](https://github.com/cypress-io/cypress/issues/25495). ## 12.6.0 From 3ec78f1dd57a35085c9fe7909031bfbde80324b1 Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Wed, 15 Feb 2023 19:30:05 -0500 Subject: [PATCH 40/40] [run ci]