Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: make cookies have sameSite key by default #7790

Merged
merged 4 commits into from
Jun 29, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions cli/schema/cypress.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
6 changes: 0 additions & 6 deletions cli/types/cypress.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2442,12 +2442,6 @@ declare namespace Cypress {
* @default { runMode: 1, openMode: null }
*/
firefoxGcInterval: Nullable<number | { runMode: Nullable<number>, openMode: Nullable<number> }>
/**
* 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.
Expand Down
4 changes: 1 addition & 3 deletions packages/driver/cypress/integration/commands/cookies_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down
14 changes: 0 additions & 14 deletions packages/driver/src/cy/commands/cookies.js
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 │
└────────────────────────────────────────────────────────────────────────────────────────────────┘


Expand Down
9 changes: 5 additions & 4 deletions packages/server/lib/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -105,7 +106,6 @@ browsers\
// each should start with "experimental" and be camel cased
// example: experimentalComponentTesting
const experimentalConfigKeys = toWords(`\
experimentalGetCookiesSameSite
experimentalSourceRewriting
experimentalComponentTesting
experimentalShadowDomSupport
Expand Down Expand Up @@ -174,7 +174,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,
Expand Down Expand Up @@ -222,7 +221,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,
Expand Down Expand Up @@ -251,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}`)
}
}
})
Expand Down
5 changes: 5 additions & 0 deletions packages/server/lib/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

I think the convention when listing versions is to say in Cypress version x.x.x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the only prior art I could find, which also isn't that:

Note: In Cypress 4.0, Canary must be launched as \`chrome:canary\`, not \`canary\`.

Copy link
Member

Choose a reason for hiding this comment

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

In our migration guide we usually say Cypress 5, so as not to be restricted of it only applying to Cypress 5.0.0 and not to 5.1.0, etc.


You can safely remove this option from your config.`
default:
}
}
Expand Down
2 changes: 0 additions & 2 deletions packages/server/lib/experiments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.',
}
Expand All @@ -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',
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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())
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -248,7 +247,6 @@ describe('e2e cookies', () => {
) => {
e2e.it(`passes with baseurl: ${baseUrl}`, {
config: {
experimentalGetCookiesSameSite: true,
baseUrl,
env: {
baseUrl,
Expand Down
Original file line number Diff line number Diff line change
@@ -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

Expand Down
Original file line number Diff line number Diff line change
@@ -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())
Expand Down Expand Up @@ -48,7 +48,7 @@ const onServer = function (app) {
cookie: {
sameSite: true,
},
})
}) as Function

app.get('/htmlCookies', (req, res) => {
const {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) ->
Expand All @@ -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) ->
Expand All @@ -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})
Expand All @@ -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")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
13 changes: 11 additions & 2 deletions packages/server/test/unit/config_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -1108,7 +1119,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' },
Expand Down Expand Up @@ -1185,7 +1195,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' },
Expand Down