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: support host only cookies #25853

Merged
merged 20 commits into from
Feb 17, 2023
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
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
4 changes: 4 additions & 0 deletions cli/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@

_Released 03/1/2023 (PENDING)_

**Features:**

- It is now possible to set `hostOnly` cookies with [`cy.setCookie()`](https://docs.cypress.io/api/commands/setcookie) for a given domain. Addresses [#16856](https://github.com/cypress-io/cypress/issues/16856) and [#17527](https://github.com/cypress-io/cypress/issues/17527).

**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).
Expand Down
38 changes: 38 additions & 0 deletions cli/types/cypress.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3569,12 +3569,49 @@ declare namespace Cypress {
action: 'select' | 'drag-drop'
}

/**
* Options that control how the `cy.setCookie` command
* sets the cookie in the browser.
* @see https://on.cypress.io/setcookie#Arguments
*/
interface SetCookieOptions extends Loggable, Timeoutable {
/**
* The path of the cookie.
* @default "/"
*/
path: string
/**
* Represents the domain the cookie belongs to (e.g. "docs.cypress.io", "github.com").
* @default location.hostname
*/
domain: string
/**
* Whether a cookie's scope is limited to secure channels, such as HTTPS.
* @default false
*/
secure: boolean
/**
* Whether or not the cookie is HttpOnly, meaning the cookie is inaccessible to client-side scripts.
* The Cypress cookie API has access to HttpOnly cookies.
* @default false
*/
httpOnly: boolean
/**
* Whether or not the cookie is a host-only cookie, meaning the request's host must exactly match the domain of the cookie.
* @default false
*/
hostOnly: boolean
Copy link
Member

Choose a reason for hiding this comment

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

Can we add descriptions for typescript to render for users?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added in 58851a5. Is this what you are referring to?

Copy link
Member

Choose a reason for hiding this comment

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

I'd expect each property to be documented, so some of this is back-fill work 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't be we better off pointing them to the docs in the comment, which will have a more easily readable description for each property?

Copy link
Member

Choose a reason for hiding this comment

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

I assume no because users want to see it inline when typing their options not clicking to the docs? I don't really leverage typescript annotations but I assume thats why people want them, everything upfront while coding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think thats fair, especially since we do it for other types. I added them in b3c1585. Let me know what you think!

/**
* The cookie's expiry time, specified in seconds since Unix Epoch.
* The default is expiry is 20 years in the future from current time.
*/
expiry: number
/**
* The cookie's SameSite value. If set, should be one of `lax`, `strict`, or `no_restriction`.
* `no_restriction` is the equivalent of `SameSite=None`. Pass `undefined` to use the browser's default.
* Note: `no_restriction` can only be used if the secure flag is set to `true`.
* @default undefined
*/
sameSite: SameSiteStatus
}

Expand Down Expand Up @@ -6104,6 +6141,7 @@ declare namespace Cypress {
value: string
path: string
domain: string
hostOnly?: boolean
httpOnly: boolean
secure: boolean
expiry?: number
Expand Down
8 changes: 8 additions & 0 deletions cli/types/tests/cypress-tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1063,6 +1063,14 @@ namespace CypressSetCookieTests {
expiry: 12345,
sameSite: 'lax',
})
cy.setCookie('name', 'value', {
domain: 'www.foobar.com',
path: '/',
secure: false,
httpOnly: false,
hostOnly: true,
sameSite: 'lax',
})
cy.setCookie('name', 'value', { log: true, timeout: 10, domain: 'localhost' })

cy.setCookie('name') // $ExpectError
Expand Down
50 changes: 42 additions & 8 deletions packages/driver/cypress/e2e/commands/cookies.cy.js
Original file line number Diff line number Diff line change
Expand Up @@ -562,6 +562,40 @@ describe('src/cy/commands/cookies - no stub', () => {
})
})
})

it('sets the cookie on the specified domain as hostOnly and validates hostOnly property persists through related commands that fetch cookies', () => {
const isWebkit = Cypress.browser.name.includes('webkit')

cy.visit('http://www.barbaz.com:3500/fixtures/generic.html')
cy.setCookie('foo', 'bar', { hostOnly: true })

cy.getCookie('foo').its('domain').should('eq', 'www.barbaz.com')
if (!isWebkit) {
cy.getCookie('foo').its('hostOnly').should('eq', true)
}

cy.getCookies().then((cookies) => {
expect(cookies).to.have.lengthOf(1)

const cookie = cookies[0]

expect(cookie).to.have.property('domain', 'www.barbaz.com')
if (!isWebkit) {
expect(cookie).to.have.property('hostOnly', true)
}
})

cy.getAllCookies().then((cookies) => {
expect(cookies).to.have.lengthOf(1)

const cookie = cookies[0]

expect(cookie).to.have.property('domain', 'www.barbaz.com')
if (!isWebkit) {
expect(cookie).to.have.property('hostOnly', true)
}
})
})
})

describe('src/cy/commands/cookies', () => {
Expand Down Expand Up @@ -785,7 +819,7 @@ describe('src/cy/commands/cookies', () => {

it('#consoleProps', () => {
cy.getCookies().then(function (cookies) {
expect(cookies).to.deep.eq([{ name: 'foo', value: 'bar', domain: 'localhost', path: '/', secure: true, httpOnly: false }])
expect(cookies).to.deep.eq([{ name: 'foo', value: 'bar', domain: 'localhost', path: '/', secure: true, httpOnly: false, hostOnly: false }])
const c = this.lastLog.invoke('consoleProps')

expect(c['Yielded']).to.deep.eq(cookies)
Expand Down Expand Up @@ -938,7 +972,7 @@ describe('src/cy/commands/cookies', () => {

it('#consoleProps', () => {
cy.getCookies().then(function (cookies) {
expect(cookies).to.deep.eq([{ name: 'foo', value: 'bar', domain: 'localhost', path: '/', secure: true, httpOnly: false }])
expect(cookies).to.deep.eq([{ name: 'foo', value: 'bar', domain: 'localhost', path: '/', secure: true, httpOnly: false, hostOnly: false }])
const c = this.lastLog.invoke('consoleProps')

expect(c['Yielded']).to.deep.eq(cookies)
Expand All @@ -955,7 +989,7 @@ describe('src/cy/commands/cookies', () => {
})

cy.getCookie('foo').should('deep.eq', {
name: 'foo', value: 'bar', domain: 'localhost', path: '/', secure: true, httpOnly: false,
name: 'foo', value: 'bar', domain: 'localhost', path: '/', secure: true, httpOnly: false, hostOnly: true,
})
.then(() => {
expect(Cypress.automation).to.be.calledWith(
Expand Down Expand Up @@ -1196,7 +1230,7 @@ describe('src/cy/commands/cookies', () => {
.then(() => {
expect(Cypress.automation).to.be.calledWith(
'set:cookie',
{ domain: 'localhost', name: 'foo', value: 'bar', path: '/', secure: false, httpOnly: false, expiry: 12345, sameSite: undefined },
{ domain: 'localhost', name: 'foo', value: 'bar', path: '/', secure: false, httpOnly: false, hostOnly: false, expiry: 12345, sameSite: undefined },
)
})
})
Expand All @@ -1212,7 +1246,7 @@ describe('src/cy/commands/cookies', () => {
.then(() => {
expect(Cypress.automation).to.be.calledWith(
'set:cookie',
{ domain: 'brian.dev.local', name: 'foo', value: 'bar', path: '/foo', secure: true, httpOnly: true, expiry: 987, sameSite: undefined },
{ domain: 'brian.dev.local', name: 'foo', value: 'bar', path: '/foo', secure: true, httpOnly: true, hostOnly: false, expiry: 987, sameSite: undefined },
)
})
})
Expand Down Expand Up @@ -1461,7 +1495,7 @@ describe('src/cy/commands/cookies', () => {

Cypress.automation
.withArgs('set:cookie', {
domain: 'localhost', name: 'foo', value: 'bar', path: '/', secure: false, httpOnly: false, expiry: 12345, sameSite: undefined,
domain: 'localhost', name: 'foo', value: 'bar', path: '/', secure: false, httpOnly: false, hostOnly: false, expiry: 12345, sameSite: undefined,
})
.resolves({
name: 'foo', value: 'bar', domain: 'localhost', path: '/', secure: true, httpOnly: false, hostOnly: true,
Expand Down Expand Up @@ -1494,7 +1528,7 @@ describe('src/cy/commands/cookies', () => {

it('#consoleProps', () => {
cy.setCookie('foo', 'bar').then(function (cookie) {
expect(cookie).to.deep.eq({ name: 'foo', value: 'bar', domain: 'localhost', path: '/', secure: true, httpOnly: false })
expect(cookie).to.deep.eq({ name: 'foo', value: 'bar', domain: 'localhost', path: '/', secure: true, httpOnly: false, hostOnly: true })
const c = this.lastLog.invoke('consoleProps')

expect(c['Yielded']).to.deep.eq(cookie)
Expand Down Expand Up @@ -1688,7 +1722,7 @@ describe('src/cy/commands/cookies', () => {
const c = this.lastLog.invoke('consoleProps')

expect(c['Yielded']).to.eq('null')
expect(c['Cleared Cookie']).to.deep.eq({ name: 'foo', value: 'bar', domain: 'localhost', path: '/', secure: true, httpOnly: false })
expect(c['Cleared Cookie']).to.deep.eq({ name: 'foo', value: 'bar', domain: 'localhost', path: '/', secure: true, httpOnly: false, hostOnly: false })
})
})

Expand Down
2 changes: 2 additions & 0 deletions packages/driver/cypress/e2e/e2e/origin/cookie_login.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -725,6 +725,7 @@ describe('cy.origin - cookie login', { browser: '!webkit' }, () => {
expect(Cypress._.omit(cookie, 'expiry')).to.deep.equal({
domain: 'www.foobar.com',
httpOnly: false,
hostOnly: true,
name: 'key',
path: '/fixtures',
sameSite: 'strict',
Expand Down Expand Up @@ -853,6 +854,7 @@ describe('cy.origin - cookie login', { browser: '!webkit' }, () => {
expect(Cypress._.omit(cookie, 'expiry')).to.deep.equal({
domain: 'www.foobar.com',
httpOnly: false,
hostOnly: true,
name: 'name',
path: '/',
sameSite: 'lax',
Expand Down
6 changes: 2 additions & 4 deletions packages/driver/src/cy/commands/cookies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@ import Promise from 'bluebird'
import $utils from '../../cypress/utils'
import $errUtils from '../../cypress/error_utils'

// TODO: add hostOnly to COOKIE_PROPS
// https://github.com/cypress-io/cypress/issues/363
// https://github.com/cypress-io/cypress/issues/17527
const COOKIE_PROPS = 'name value path secure httpOnly expiry domain sameSite'.split(' ')
const COOKIE_PROPS = 'name value path secure hostOnly httpOnly expiry domain sameSite'.split(' ')
Copy link
Member

Choose a reason for hiding this comment

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

  1. linked issues need removed

  2. the change to pickCookieProps change cascades into the return type of cy.getCookie() and cy.getCookies() as well as cy.clearCookie()

  3. We likely need to update CookieOptions for all commands for this to be validate in all places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

I got the documentation up to date in cypress-io/cypress-documentation#5071 to add the cookie option return types. I added the types to cookie options in 19c0663 and removed the links. I don't think clearCookie needs to be updated even though it uses pickCookieProps since it nulls out the subject.

Copy link
Member

Choose a reason for hiding this comment

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

Funny our types already included hostOnly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit of a blur at this point because in some cases it might be in the automation client, but it shouldnt be in client types yet I don't think?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I may have added that. I rebased this branch quite a bit and the changeset distinction might have got lost between rebases.


function pickCookieProps (cookie) {
if (!cookie) return cookie
Expand Down Expand Up @@ -359,6 +356,7 @@ export default function (Commands, Cypress: InternalCypress.Cypress, cy, state,
path: '/',
secure: false,
httpOnly: false,
hostOnly: false,
log: true,
expiry: $utils.addTwentyYears(),
})
Expand Down
2 changes: 2 additions & 0 deletions packages/extension/app/background.js
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,8 @@ const setOneCookie = (props) => {
}

if (props.hostOnly) {
// If the hostOnly prop is available, delete the domain.
// This will wind up setting a hostOnly cookie based on the calculated cookieURL above.
delete props.domain
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -256,13 +256,19 @@ describe('cookies', () => {

_.times(n + 1, (i) => {
['foo', 'bar'].forEach((tag) => {
const domain = (i % 2) === (8 - n) ? expectedDomain : altDomain

const expectedCookie = {
'name': `name${tag}${i}`,
'value': `val${tag}${i}`,
'path': '/',
'domain': (i % 2) === (8 - n) ? expectedDomain : altDomain,
// eslint-disable-next-line object-shorthand
'domain': domain,
'secure': false,
'httpOnly': false,
...(domain[0] !== '.' && domain !== 'localhost' && domain !== '127.0.0.1' ? {
'hostOnly': true,
} : {}),
}

if (defaultSameSite) {
Expand Down