From 65b9a07ce9b28753b00f8648344177edc5a739f6 Mon Sep 17 00:00:00 2001 From: Zach Bloomquist Date: Tue, 23 Jun 2020 15:20:27 -0400 Subject: [PATCH 1/3] feat: make cookies have sameSite key by default BREAKING CHANGE: modifies the shape of Cookie objects --- cli/schema/cypress.schema.json | 5 ----- cli/types/cypress.d.ts | 6 ------ .../cypress/integration/commands/cookies_spec.js | 4 +--- packages/driver/src/cy/commands/cookies.js | 14 -------------- packages/server/__snapshots__/2_cookies_spec.js | 9 ++++----- packages/server/lib/config.js | 3 --- packages/server/lib/experiments.ts | 2 -- packages/server/test/e2e/2_cookies_spec.js | 2 -- packages/server/test/unit/config_spec.js | 2 -- 9 files changed, 5 insertions(+), 42 deletions(-) diff --git a/cli/schema/cypress.schema.json b/cli/schema/cypress.schema.json index f68a30ee6c18..c3d2ffb22876 100644 --- a/cli/schema/cypress.schema.json +++ b/cli/schema/cypress.schema.json @@ -224,11 +224,6 @@ "default": "bundled", "description": "If set to 'system', Cypress will try to find a Node.js executable on your path to use when executing your plugins. Otherwise, Cypress will use the Node version bundled with Cypress." }, - "experimentalGetCookiesSameSite": { - "type": "boolean", - "default": false, - "description": "If `true`, Cypress will add `sameSite` values to the objects yielded from `cy.setCookie()`, `cy.getCookie()`, and `cy.getCookies()`. This will become the default behavior in Cypress 5.0." - }, "experimentalSourceRewriting": { "type": "boolean", "default": false, diff --git a/cli/types/cypress.d.ts b/cli/types/cypress.d.ts index dde1a928b720..cb4adf2ca055 100644 --- a/cli/types/cypress.d.ts +++ b/cli/types/cypress.d.ts @@ -2442,12 +2442,6 @@ declare namespace Cypress { * @default { runMode: 1, openMode: null } */ firefoxGcInterval: Nullable, openMode: Nullable }> - /** - * If `true`, Cypress will add `sameSite` values to the objects yielded from `cy.setCookie()`, - * `cy.getCookie()`, and `cy.getCookies()`. This will become the default behavior in Cypress 5.0. - * @default false - */ - experimentalGetCookiesSameSite: boolean /** * Enables AST-based JS/HTML rewriting. This may fix issues caused by the existing regex-based JS/HTML replacement * algorithm. diff --git a/packages/driver/cypress/integration/commands/cookies_spec.js b/packages/driver/cypress/integration/commands/cookies_spec.js index 896f1fd5bb2e..6d2092626075 100644 --- a/packages/driver/cypress/integration/commands/cookies_spec.js +++ b/packages/driver/cypress/integration/commands/cookies_spec.js @@ -474,9 +474,7 @@ describe('src/cy/commands/cookies', () => { }) }) - it('can set cookies with sameSite', { - experimentalGetCookiesSameSite: true, - }, () => { + it('can set cookies with sameSite', () => { Cypress.automation.restore() Cypress.utils.addTwentyYears.restore() diff --git a/packages/driver/src/cy/commands/cookies.js b/packages/driver/src/cy/commands/cookies.js index b5fda3ad6ac2..51e6d1f9ee0a 100644 --- a/packages/driver/src/cy/commands/cookies.js +++ b/packages/driver/src/cy/commands/cookies.js @@ -58,12 +58,6 @@ const normalizeSameSite = (sameSite) => { } module.exports = function (Commands, Cypress, cy, state, config) { - const maybeStripSameSiteProp = (cookie) => { - if (cookie && !Cypress.config('experimentalGetCookiesSameSite')) { - delete cookie.sameSite - } - } - const automateCookies = function (event, obj = {}, log, timeout) { const automate = () => { return Cypress.automation(event, mergeDefaults(obj)) @@ -183,8 +177,6 @@ module.exports = function (Commands, Cypress, cy, state, config) { return automateCookies('get:cookie', { name }, options._log, options.timeout) .then((resp) => { - maybeStripSameSiteProp(resp) - options.cookie = resp return resp @@ -222,10 +214,6 @@ module.exports = function (Commands, Cypress, cy, state, config) { return automateCookies('get:cookies', _.pick(options, 'domain'), options._log, options.timeout) .then((resp) => { - if (Array.isArray(resp)) { - resp.forEach(maybeStripSameSiteProp) - } - options.cookies = resp return resp @@ -299,8 +287,6 @@ module.exports = function (Commands, Cypress, cy, state, config) { return automateCookies('set:cookie', cookie, options._log, options.timeout) .then((resp) => { - maybeStripSameSiteProp(resp) - options.cookie = resp return resp diff --git a/packages/server/__snapshots__/2_cookies_spec.js b/packages/server/__snapshots__/2_cookies_spec.js index 39c46f5cdafa..7bc11a5d62cb 100644 --- a/packages/server/__snapshots__/2_cookies_spec.js +++ b/packages/server/__snapshots__/2_cookies_spec.js @@ -5,11 +5,10 @@ exports['e2e cookies with baseurl'] = ` (Run Starting) ┌────────────────────────────────────────────────────────────────────────────────────────────────┐ - │ Cypress: 1.2.3 │ - │ Browser: FooBrowser 88 │ - │ Specs: 1 found (cookies_spec_baseurl.coffee) │ - │ Searched: cypress/integration/cookies_spec_baseurl.coffee │ - │ Experiments: experimentalGetCookiesSameSite=true │ + │ Cypress: 1.2.3 │ + │ Browser: FooBrowser 88 │ + │ Specs: 1 found (cookies_spec_baseurl.coffee) │ + │ Searched: cypress/integration/cookies_spec_baseurl.coffee │ └────────────────────────────────────────────────────────────────────────────────────────────────┘ diff --git a/packages/server/lib/config.js b/packages/server/lib/config.js index 8c674634a796..0fc5f056b8ad 100644 --- a/packages/server/lib/config.js +++ b/packages/server/lib/config.js @@ -105,7 +105,6 @@ browsers\ // each should start with "experimental" and be camel cased // example: experimentalComponentTesting const experimentalConfigKeys = toWords(`\ -experimentalGetCookiesSameSite experimentalSourceRewriting experimentalComponentTesting experimentalShadowDomSupport @@ -174,7 +173,6 @@ const CONFIG_DEFAULTS = { componentFolder: 'cypress/component', // TODO: example for component testing with subkeys // experimentalComponentTesting: { componentFolder: 'cypress/component' } - experimentalGetCookiesSameSite: false, experimentalSourceRewriting: false, experimentalShadowDomSupport: false, experimentalFetchPolyfill: false, @@ -222,7 +220,6 @@ const validationRules = { // validation for component testing experiment componentFolder: v.isStringOrFalse, // experimental flag validation below - experimentalGetCookiesSameSite: v.isBoolean, experimentalSourceRewriting: v.isBoolean, experimentalShadowDomSupport: v.isBoolean, experimentalFetchPolyfill: v.isBoolean, diff --git a/packages/server/lib/experiments.ts b/packages/server/lib/experiments.ts index 610bf5d194e3..e365e019ac56 100644 --- a/packages/server/lib/experiments.ts +++ b/packages/server/lib/experiments.ts @@ -53,7 +53,6 @@ interface StringValues { const _summaries: StringValues = { experimentalComponentTesting: 'Framework-specific component testing, uses `componentFolder` to load component specs', experimentalSourceRewriting: 'Enables AST-based JS/HTML rewriting. This may fix issues caused by the existing regex-based JS/HTML replacement algorithm.', - experimentalGetCookiesSameSite: 'Adds `sameSite` values to the objects yielded from `cy.setCookie()`, `cy.getCookie()`, and `cy.getCookies()`. This will become the default behavior in Cypress 5.0.', experimentalFetchPolyfill: 'Polyfills `window.fetch` to enable Network spying and stubbing', experimentalShadowDomSupport: 'Enables support for shadow DOM traversal, introduces the `shadow()` command and the `includeShadowDom` option to traversal commands.', } @@ -71,7 +70,6 @@ const _summaries: StringValues = { const _names: StringValues = { experimentalComponentTesting: 'Component Testing', experimentalSourceRewriting: 'Improved source rewriting', - experimentalGetCookiesSameSite: 'Set `sameSite` property when retrieving cookies', experimentalShadowDomSupport: 'Shadow DOM Support', experimentalFetchPolyfill: 'Fetch polyfill', } diff --git a/packages/server/test/e2e/2_cookies_spec.js b/packages/server/test/e2e/2_cookies_spec.js index 2b9d06263068..657e0f877f13 100644 --- a/packages/server/test/e2e/2_cookies_spec.js +++ b/packages/server/test/e2e/2_cookies_spec.js @@ -194,7 +194,6 @@ describe('e2e cookies', () => { // we can remove this extra test case e2e.it('with forced SameSite strictness', { config: { - experimentalGetCookiesSameSite: true, baseUrl, env: { baseUrl, @@ -248,7 +247,6 @@ describe('e2e cookies', () => { ) => { e2e.it(`passes with baseurl: ${baseUrl}`, { config: { - experimentalGetCookiesSameSite: true, baseUrl, env: { baseUrl, diff --git a/packages/server/test/unit/config_spec.js b/packages/server/test/unit/config_spec.js index 9c821ccb04d5..b5006c78b27d 100644 --- a/packages/server/test/unit/config_spec.js +++ b/packages/server/test/unit/config_spec.js @@ -1108,7 +1108,6 @@ describe('lib/config', () => { requestTimeout: { value: 5000, from: 'default' }, responseTimeout: { value: 30000, from: 'default' }, execTimeout: { value: 60000, from: 'default' }, - experimentalGetCookiesSameSite: { value: false, from: 'default' }, experimentalSourceRewriting: { value: false, from: 'default' }, taskTimeout: { value: 60000, from: 'default' }, numTestsKeptInMemory: { value: 50, from: 'default' }, @@ -1185,7 +1184,6 @@ describe('lib/config', () => { requestTimeout: { value: 5000, from: 'default' }, responseTimeout: { value: 30000, from: 'default' }, execTimeout: { value: 60000, from: 'default' }, - experimentalGetCookiesSameSite: { value: false, from: 'default' }, experimentalSourceRewriting: { value: false, from: 'default' }, taskTimeout: { value: 60000, from: 'default' }, numTestsKeptInMemory: { value: 50, from: 'default' }, From 9d34c71942e5aa3385bbcc2d23aabcd6af2af836 Mon Sep 17 00:00:00 2001 From: Zach Bloomquist Date: Thu, 25 Jun 2020 15:03:32 -0400 Subject: [PATCH 2/3] update tests --- .../{2_cookies_spec.js => 2_cookies_spec.ts.js} | 0 .../{4_request_spec.js => 4_request_spec.ts.js} | 0 ...subdomain_spec.js => 5_subdomain_spec.ts.js} | 0 .../{2_cookies_spec.js => 2_cookies_spec.ts} | 8 ++++---- .../{4_request_spec.js => 4_request_spec.ts} | 6 +++--- ...{5_subdomain_spec.js => 5_subdomain_spec.ts} | 10 +++++----- .../integration/cookies_spec_no_baseurl.coffee | 17 ++++++++--------- .../e2e/cypress/integration/request_spec.coffee | 4 ++-- .../cypress/integration/subdomain_spec.coffee | 2 +- 9 files changed, 23 insertions(+), 24 deletions(-) rename packages/server/__snapshots__/{2_cookies_spec.js => 2_cookies_spec.ts.js} (100%) rename packages/server/__snapshots__/{4_request_spec.js => 4_request_spec.ts.js} (100%) rename packages/server/__snapshots__/{5_subdomain_spec.js => 5_subdomain_spec.ts.js} (100%) rename packages/server/test/e2e/{2_cookies_spec.js => 2_cookies_spec.ts} (97%) rename packages/server/test/e2e/{4_request_spec.js => 4_request_spec.ts} (96%) rename packages/server/test/e2e/{5_subdomain_spec.js => 5_subdomain_spec.ts} (93%) diff --git a/packages/server/__snapshots__/2_cookies_spec.js b/packages/server/__snapshots__/2_cookies_spec.ts.js similarity index 100% rename from packages/server/__snapshots__/2_cookies_spec.js rename to packages/server/__snapshots__/2_cookies_spec.ts.js diff --git a/packages/server/__snapshots__/4_request_spec.js b/packages/server/__snapshots__/4_request_spec.ts.js similarity index 100% rename from packages/server/__snapshots__/4_request_spec.js rename to packages/server/__snapshots__/4_request_spec.ts.js diff --git a/packages/server/__snapshots__/5_subdomain_spec.js b/packages/server/__snapshots__/5_subdomain_spec.ts.js similarity index 100% rename from packages/server/__snapshots__/5_subdomain_spec.js rename to packages/server/__snapshots__/5_subdomain_spec.ts.js diff --git a/packages/server/test/e2e/2_cookies_spec.js b/packages/server/test/e2e/2_cookies_spec.ts similarity index 97% rename from packages/server/test/e2e/2_cookies_spec.js rename to packages/server/test/e2e/2_cookies_spec.ts index 657e0f877f13..83a10d35886e 100644 --- a/packages/server/test/e2e/2_cookies_spec.js +++ b/packages/server/test/e2e/2_cookies_spec.ts @@ -1,7 +1,7 @@ -const moment = require('moment') -const parser = require('cookie-parser') -const e2e = require('../support/helpers/e2e').default -const humanInterval = require('human-interval') +import moment from 'moment' +import parser from 'cookie-parser' +import e2e from '../support/helpers/e2e' +import humanInterval from 'human-interval' const onServer = function (app) { app.use(parser()) diff --git a/packages/server/test/e2e/4_request_spec.js b/packages/server/test/e2e/4_request_spec.ts similarity index 96% rename from packages/server/test/e2e/4_request_spec.js rename to packages/server/test/e2e/4_request_spec.ts index fa33ab6e6105..e857488bcacf 100644 --- a/packages/server/test/e2e/4_request_spec.js +++ b/packages/server/test/e2e/4_request_spec.ts @@ -1,6 +1,6 @@ -const bodyParser = require('body-parser') -const cookieParser = require('cookie-parser') -const e2e = require('../support/helpers/e2e').default +import bodyParser from 'body-parser' +import cookieParser from 'cookie-parser' +import e2e from '../support/helpers/e2e' let counts = null diff --git a/packages/server/test/e2e/5_subdomain_spec.js b/packages/server/test/e2e/5_subdomain_spec.ts similarity index 93% rename from packages/server/test/e2e/5_subdomain_spec.js rename to packages/server/test/e2e/5_subdomain_spec.ts index ebcf82fb2109..568330b5a5b2 100644 --- a/packages/server/test/e2e/5_subdomain_spec.js +++ b/packages/server/test/e2e/5_subdomain_spec.ts @@ -1,7 +1,7 @@ -const cors = require('cors') -const parser = require('cookie-parser') -const session = require('express-session') -const e2e = require('../support/helpers/e2e').default +import cors from 'cors' +import parser from 'cookie-parser' +import session from 'express-session' +import e2e from '../support/helpers/e2e' const onServer = function (app) { app.use(parser()) @@ -48,7 +48,7 @@ const onServer = function (app) { cookie: { sameSite: true, }, - }) + }) as Function app.get('/htmlCookies', (req, res) => { const { diff --git a/packages/server/test/support/fixtures/projects/e2e/cypress/integration/cookies_spec_no_baseurl.coffee b/packages/server/test/support/fixtures/projects/e2e/cypress/integration/cookies_spec_no_baseurl.coffee index ebe46a3cd9a9..9fd762d558f9 100644 --- a/packages/server/test/support/fixtures/projects/e2e/cypress/integration/cookies_spec_no_baseurl.coffee +++ b/packages/server/test/support/fixtures/projects/e2e/cypress/integration/cookies_spec_no_baseurl.coffee @@ -12,6 +12,11 @@ describe "cookies", -> }) it "can get all cookies", -> + expectedKeys = ["domain", "name", "value", "path", "secure", "httpOnly", "expiry"] + + if Cypress.isBrowser('firefox') + expectedKeys.push('sameSite') + cy .clearCookie("foo1") .setCookie("foo", "bar").then (c) -> @@ -23,9 +28,7 @@ describe "cookies", -> expect(c.secure).to.eq(false) expect(c.expiry).to.be.a("number") - expect(c).to.have.keys( - "domain", "name", "value", "path", "secure", "httpOnly", "expiry" - ) + expect(c).to.have.keys(expectedKeys) .getCookies() .should("have.length", 1) .then (cookies) -> @@ -39,9 +42,7 @@ describe "cookies", -> expect(c.secure).to.eq(false) expect(c.expiry).to.be.a("number") - expect(c).to.have.keys( - "domain", "name", "value", "path", "secure", "httpOnly", "expiry" - ) + expect(c).to.have.keys(expectedKeys) .clearCookies() .should("be.null") .setCookie("wtf", "bob", {httpOnly: true, path: "/foo", secure: true}) @@ -54,9 +55,7 @@ describe "cookies", -> expect(c.secure).to.eq(true) expect(c.expiry).to.be.a("number") - expect(c).to.have.keys( - "domain", "name", "value", "path", "secure", "httpOnly", "expiry" - ) + expect(c).to.have.keys(expectedKeys) .clearCookie("wtf") .should("be.null") .getCookie("doesNotExist") diff --git a/packages/server/test/support/fixtures/projects/e2e/cypress/integration/request_spec.coffee b/packages/server/test/support/fixtures/projects/e2e/cypress/integration/request_spec.coffee index d59ea40bab53..5b96e92d988c 100644 --- a/packages/server/test/support/fixtures/projects/e2e/cypress/integration/request_spec.coffee +++ b/packages/server/test/support/fixtures/projects/e2e/cypress/integration/request_spec.coffee @@ -20,14 +20,14 @@ describe "redirects + requests", -> expect(cookies[0].secure).to.eq(false) expect(cookies[0].expiry).to.be.closeTo(oneMinuteFromNow, 5) - expect(cookies[1]).to.deep.eq({ + expect(cookies[1]).to.deep.eq(Cypress._.merge({ domain: "localhost" name: "2293-session" value: "true" httpOnly: false path: "/" secure: false - }) + }, (if Cypress.isBrowser('firefox') then { sameSite: 'no_restriction' } else {}))) it "visits to a different superdomain will be resolved twice", -> cy diff --git a/packages/server/test/support/fixtures/projects/e2e/cypress/integration/subdomain_spec.coffee b/packages/server/test/support/fixtures/projects/e2e/cypress/integration/subdomain_spec.coffee index 08d712e80cd3..6af1d6a2b348 100644 --- a/packages/server/test/support/fixtures/projects/e2e/cypress/integration/subdomain_spec.coffee +++ b/packages/server/test/support/fixtures/projects/e2e/cypress/integration/subdomain_spec.coffee @@ -42,7 +42,7 @@ describe "subdomains", -> cy .visit("http://domain.foobar.com:2292") .getCookies().should("have.length", 1) - .getCookie("nomnom").should("deep.eq", { + .getCookie("nomnom").should("include", { domain: ".foobar.com" name: "nomnom" value: "good" From 6f50165da7e4e7eb050b5c522119079701efe90f Mon Sep 17 00:00:00 2001 From: Zach Bloomquist Date: Mon, 29 Jun 2020 12:38:45 -0400 Subject: [PATCH 3/3] add deprecation notice --- packages/server/lib/config.js | 6 +++++- packages/server/lib/errors.js | 5 +++++ packages/server/test/unit/config_spec.js | 11 +++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/packages/server/lib/config.js b/packages/server/lib/config.js index 0fc5f056b8ad..7f9cbd7596b7 100644 --- a/packages/server/lib/config.js +++ b/packages/server/lib/config.js @@ -93,7 +93,8 @@ configKeys.push('componentFolder') const breakingConfigKeys = toWords(`\ videoRecording screenshotOnHeadlessFailure -trashAssetsBeforeHeadlessRuns\ +trashAssetsBeforeHeadlessRuns +experimentalGetCookiesSameSite\ `) // Internal configuration properties the user should be able to overwrite @@ -248,7 +249,10 @@ const validateNoBreakingConfig = (cfg) => { return errors.throw('RENAMED_CONFIG_OPTION', key, 'trashAssetsBeforeRuns') case 'videoRecording': return errors.throw('RENAMED_CONFIG_OPTION', key, 'video') + case 'experimentalGetCookiesSameSite': + return errors.warning('EXPERIMENTAL_SAMESITE_REMOVED') default: + throw new Error(`unknown breaking config key ${key}`) } } }) diff --git a/packages/server/lib/errors.js b/packages/server/lib/errors.js index 8797c2a48d4f..914eae146b65 100644 --- a/packages/server/lib/errors.js +++ b/packages/server/lib/errors.js @@ -923,6 +923,11 @@ const getMsgByType = function (type, arg1 = {}, arg2, arg3) { Enable write permissions to this directory to ensure screenshots and videos are stored. If you don't require screenshots or videos to be stored you can safely ignore this warning.` + case 'EXPERIMENTAL_SAMESITE_REMOVED': + return stripIndent`\ + The \`experimentalGetCookiesSameSite\` configuration option was removed in Cypress 5. Yielding the \`sameSite\` property is now the default behavior of the \`cy.cookie\` commands. + + You can safely remove this option from your config.` default: } } diff --git a/packages/server/test/unit/config_spec.js b/packages/server/test/unit/config_spec.js index b5006c78b27d..e41d83f737ab 100644 --- a/packages/server/test/unit/config_spec.js +++ b/packages/server/test/unit/config_spec.js @@ -1079,6 +1079,17 @@ describe('lib/config', () => { }) }) + // @see https://github.com/cypress-io/cypress/issues/6892 + it('warns if experimentalGetCookiesSameSite is passed', async function () { + const warning = sinon.spy(errors, 'warning') + + await this.defaults('experimentalGetCookiesSameSite', true, { + experimentalGetCookiesSameSite: true, + }) + + expect(warning).to.be.calledWith('EXPERIMENTAL_SAMESITE_REMOVED') + }) + describe('.resolved', () => { it('sets reporter and port to cli', () => { const obj = {