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

fix(multi-domain): Handle SameSite cookies #20450

Merged
merged 32 commits into from
Mar 22, 2022
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
73f4b56
fix issue with url:changed
chrisbreiding Feb 28, 2022
7774a40
remove obsolete event emission
chrisbreiding Feb 28, 2022
31eb352
Merge branch 'feature-multidomain' into issue-19849-auth-workflow
chrisbreiding Feb 28, 2022
a6dc42f
force SameSite=None for cross-origin AUT cookies
chrisbreiding Mar 2, 2022
5b07648
fix wrong selector
chrisbreiding Mar 2, 2022
320bdd4
add test for initial AUT request
chrisbreiding Mar 2, 2022
a0464b5
reset previousAUTRequestUrl between tests
chrisbreiding Mar 4, 2022
028dc76
Merge branch 'feature-multidomain' into issue-19849-auth-workflow
chrisbreiding Mar 14, 2022
7375a9b
handle firefox case where http://localhost cookies are set with Secur…
chrisbreiding Mar 16, 2022
4e99df3
Merge branch 'feature-multidomain' into issue-19849-auth-workflow
chrisbreiding Mar 16, 2022
0e89b36
fix types
chrisbreiding Mar 16, 2022
dab0cca
require experimental flag for cookie manipulation
chrisbreiding Mar 16, 2022
be3a7c0
Merge branch 'feature-multidomain' into issue-19849-auth-workflow
chrisbreiding Mar 16, 2022
a6d46c9
Merge branch 'feature-multidomain' into issue-19849-auth-workflow
chrisbreiding Mar 16, 2022
6b53e93
Merge branch 'feature-multidomain' into issue-19849-auth-workflow
chrisbreiding Mar 16, 2022
f9c4127
fix tests
chrisbreiding Mar 16, 2022
1ae7359
fix tests
chrisbreiding Mar 16, 2022
2f82382
fix issue with logs having err property
chrisbreiding Mar 16, 2022
84b2948
put cdp cookies change behind experimental flag
chrisbreiding Mar 17, 2022
530304c
empty commit for clean ci re-run
chrisbreiding Mar 17, 2022
3411590
remove unnecessary changes
chrisbreiding Mar 17, 2022
8a0f2b1
one more
chrisbreiding Mar 17, 2022
c0a3ce8
organize response-middleware tests better
chrisbreiding Mar 18, 2022
e88519a
refactor cookie handling
chrisbreiding Mar 18, 2022
4ed06ad
improve cdp options types
chrisbreiding Mar 18, 2022
a2f7bd8
add rfc comments to isLocalhost and add .localhost support
chrisbreiding Mar 18, 2022
58c3714
fix reference
chrisbreiding Mar 18, 2022
1b111e6
fix test
chrisbreiding Mar 21, 2022
aeed87e
improve samesite cookie handling comment
chrisbreiding Mar 21, 2022
3dfaf14
Merge branch 'feature-multidomain' into issue-19849-auth-workflow
chrisbreiding Mar 21, 2022
796df1d
add/improve types
chrisbreiding Mar 21, 2022
b808da0
Merge branch 'feature-multidomain' into issue-19849-auth-workflow
chrisbreiding Mar 22, 2022
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
42 changes: 42 additions & 0 deletions packages/driver/cypress/fixtures/auth/cookie-login.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
<!DOCTYPE html>
<html>
<head>
</head>
<body>
<select data-cy="sameSite">
<option value="">Empty</option>
<option value="None">None</option>
<option value="Lax">Lax</option>
<option value="Strict">Strict</option>
<option value="Invalid">Invalid</option>
</select>
<label>
<input type="checkbox" data-cy="secure" /> Secure
</label>
<label for="name">Username: </label>
<input data-cy="username" type="text" required>
<button data-cy="login">Login</button>

<script>
document.querySelector('[data-cy="login"]').addEventListener('click', () => {
const queryString = window.location.search;
const urlParams = new URLSearchParams(queryString);
const redirect = urlParams.get('redirect')
const username = document.querySelector('[data-cy=username]').value
const sameSite = document.querySelector('[data-cy=sameSite]').value
const secure = document.querySelector('[data-cy=secure]').checked
let url = `https://www.foobar.com:3502/cookie-login?redirect=${redirect}&username=${username}`

if (sameSite) {
url += `&sameSite=${sameSite}`
}

if (secure) {
url += `&secure=true`
}

window.location.href = url
})
</script>
</body>
</html>
2 changes: 2 additions & 0 deletions packages/driver/cypress/fixtures/multi-domain.html
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
<a data-cy="files-form-link" href="http://www.foobar.com:3500/fixtures/files-form.html">http://www.foobar.com:3500/fixtures/files-form.html</a>
<a data-cy="errors-link" href="http://www.foobar.com:3500/fixtures/errors.html">http://www.foobar.com:3500/fixtures/errors.html</a>
<a data-cy="screenshots-link" href="http://www.foobar.com:3500/fixtures/screenshots.html">http://www.foobar.com:3500/fixtures/screenshots.html</a>
<a data-cy="cookie-login" href="http://www.foobar.com:3500/fixtures/auth/cookie-login.html?redirect=http://localhost:3500/login">Login with Social</a>
<a data-cy="welcome" href="http://localhost:3500/welcome">Go to Welcome</a>
</div>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
// @ts-ignore / session support is needed for visiting about:blank between tests
describe('multi-domain - cookie login', { experimentalSessionSupport: true }, () => {
const verifyLoggedIn = (username) => {
cy.get('h1')
.invoke('text')
.should('equal', `Welcome, ${username}!`)
}

it('works in a session', () => {
cy.session('ZJohnson', () => {
cy.visit('/fixtures/multi-domain.html')
cy.get('[data-cy="cookie-login"]').click()
cy.switchToDomain('http://foobar.com:3500', () => {
cy.get('[data-cy="username"]').type('ZJohnson')
cy.get('[data-cy="login"]').click()
})
}, {
validate () {
cy.getCookie('user').its('value').should('equal', 'ZJohnson')
},
})

cy.visit('/welcome')
verifyLoggedIn('ZJohnson')
})

describe('SameSite handling', () => {
beforeEach(() => {
cy.visit('/fixtures/multi-domain.html')
cy.get('[data-cy="cookie-login"]').click()
})

it('works with no SameSite, no Secure', () => {
cy.switchToDomain('http://foobar.com:3500', () => {
cy.get('[data-cy="username"]').type('AJohnson')
cy.get('[data-cy="login"]').click()
})

verifyLoggedIn('AJohnson')
})

it('works with SameSite=None, Secure', () => {
cy.switchToDomain('http://foobar.com:3500', () => {
cy.get('[data-cy="sameSite"]').select('None')
cy.get('[data-cy="secure"]').check()
cy.get('[data-cy="username"]').type('BJohnson')
cy.get('[data-cy="login"]').click()
})

verifyLoggedIn('BJohnson')
})

it('works with SameSite=None, no Secure', () => {
cy.switchToDomain('http://foobar.com:3500', () => {
cy.get('[data-cy="sameSite"]').select('None')
cy.get('[data-cy="username"]').type('CJohnson')
cy.get('[data-cy="login"]').click()
})

verifyLoggedIn('CJohnson')
})

it('works with SameSite=Lax, Secure', () => {
cy.switchToDomain('http://foobar.com:3500', () => {
cy.get('[data-cy="sameSite"]').select('Lax')
cy.get('[data-cy="secure"]').check()
cy.get('[data-cy="username"]').type('DJohnson')
chrisbreiding marked this conversation as resolved.
Show resolved Hide resolved
cy.get('[data-cy="login"]').click()
})

verifyLoggedIn('DJohnson')
})

it('works with SameSite=Strict, Secure', () => {
cy.switchToDomain('http://foobar.com:3500', () => {
cy.get('[data-cy="sameSite"]').select('Strict')
cy.get('[data-cy="secure"]').check()
cy.get('[data-cy="username"]').type('EJohnson')
cy.get('[data-cy="login"]').click()
})

verifyLoggedIn('EJohnson')
})

it('works with invalid SameSite, Secure', () => {
cy.switchToDomain('http://foobar.com:3500', () => {
cy.get('[data-cy="sameSite"]').select('Invalid')
cy.get('[data-cy="secure"]').check()
cy.get('[data-cy="username"]').type('FJohnson')
cy.get('[data-cy="login"]').click()
})

verifyLoggedIn('FJohnson')
})
})
})
66 changes: 62 additions & 4 deletions packages/driver/cypress/plugins/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ const multer = require('multer')
const upload = multer({ dest: 'cypress/_test-output/' })

const PATH_TO_SERVER_PKG = path.dirname(require.resolve('@packages/server'))
const { getPathToDist } = require('@packages/resolve-dist')

const httpPorts = [3500, 3501]
const httpsPort = 3502
Expand All @@ -27,6 +26,7 @@ const createApp = (port) => {
})

app.use(require('cors')())
app.use(require('cookie-parser')())
app.use(require('compression')())
app.use(bodyParser.urlencoded({ extended: false }))
app.use(bodyParser.json())
Expand Down Expand Up @@ -190,9 +190,67 @@ const createApp = (port) => {
.send('<html><body>server error</body></html>')
})

app.get('/cypress_multi_domain_runner.js', (req, res) => {
res.type('application/javascript')
res.sendFile(getPathToDist('runner', 'cypress_multi_domain_runner.js'))
const getCookieAdditions = ({ sameSite, secure }) => {
let additions = ''

if (sameSite) additions += `; SameSite=${sameSite}`

if (secure) additions += '; Secure'

return additions
}

app.get('/cookie-login', (req, res) => {
const { username, redirect } = req.query

res
.header('Set-Cookie', `user=${username}${getCookieAdditions(req.query)}`)
.redirect(302, `/verify-cookie-login?username=${username}&redirect=${redirect}`)
})

app.get('/verify-cookie-login', (req, res) => {
if (!req.cookies.user) {
return res
.status(403)
.send('<html><body><h1>Not logged in</h1></body></html>')
}

const { username, redirect } = req.query

res.send(`
<html>
<body>
<h1>Redirecting ${username}...</h1>
<script>
setTimeout(() => {
window.location.href = '${redirect}?username=${username}'
}, 1000)
</script>
</body>
</html>
`)
})

app.get('/login', (req, res) => {
const { username } = req.query

if (!username) {
return res.send('<html><body><h1>Must specify username to log in</h1></body></html>')
}

// can't use res.cookie() because it won't allow setting an invalid
// SameSite value, which we want to test
res
.header('Set-Cookie', `user=${username}${getCookieAdditions(req.query)}`)
.redirect(302, '/welcome')
})

app.get('/welcome', (req, res) => {
if (!req.cookies.user) {
return res.send('<html><body><h1>No user found</h1></body></html>')
}

res.send(`<html><body><h1>Welcome, ${req.cookies.user}!</h1></body></html>`)
})

let _var = ''
Expand Down
1 change: 1 addition & 0 deletions packages/driver/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
"chokidar-cli": "2.1.0",
"clone": "2.1.2",
"compression": "1.7.4",
"cookie-parser": "1.4.5",
"core-js-pure": "3.21.0",
"cors": "2.8.5",
"cypress-multi-reporters": "1.4.0",
Expand Down
3 changes: 0 additions & 3 deletions packages/driver/src/cypress/cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -936,9 +936,6 @@ export class $Cy extends EventEmitter2 implements ITimeouts, IStability, IAssert
// TODO: handle no longer error when ended early
cy.doneEarly()

// if using multi-domain, unbind any listeners waiting for a done() callback to come from cross domain
// @ts-ignore
Cypress.multiDomainCommunicator.emit('unbind:done:called')
originalDone(err)

// return null else we there are situations
Expand Down
9 changes: 9 additions & 0 deletions packages/network/lib/cors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,15 @@ export function urlMatchesOriginPolicyProps (urlStr, props) {
return _.isEqual(parsedUrl, props)
}

export function urlOriginsMatch (url1, url2) {
if (!url1 || !url2) return false

const parsedUrl1 = parseUrlIntoDomainTldPort(url1)
const parsedUrl2 = parseUrlIntoDomainTldPort(url2)

return _.isEqual(parsedUrl1, parsedUrl2)
}

export function urlMatchesOriginProtectionSpace (urlStr, origin) {
const normalizedUrl = uri.addDefaultPort(urlStr).format()
const normalizedOrigin = uri.addDefaultPort(origin).format()
Expand Down
14 changes: 13 additions & 1 deletion packages/network/lib/uri.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
// - https://nodejs.org/api/url.html#url_url_format_urlobject

import _ from 'lodash'
import url from 'url'
import url, { URL } from 'url'
chrisbreiding marked this conversation as resolved.
Show resolved Hide resolved

// yup, protocol contains a: ':' colon
// at the end of it (-______________-)
Expand Down Expand Up @@ -87,3 +87,15 @@ export function addDefaultPort (urlToCheck) {
export function getPath (urlToCheck) {
return url.parse(urlToCheck).path
}

const localhostIPRegex = /^127(?:\.(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)){3}$/

export function isLocalhost (url: URL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what about localhost6? 😛 i'm mostly kidding... but i do think that this area might be error-prone, especially since i don't see any links to RFCs or browser source code showing how they do this heuristic. do you have any references for how browsers decide this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in a2f7bd8

Copy link
Contributor

Choose a reason for hiding this comment

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

There is similar code in https://github.com/cypress-io/cypress/blob/develop/packages/driver/src/cypress/location.ts#L16 for verifying localhost as well. Seems like it would be pretty simple to update that code to use this.

Copy link
Contributor

Choose a reason for hiding this comment

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

that regex you linked actually looks wrong, i don't think 0.0.0.0 resolves to localhost

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 don't know why it's specifically checking for localhost there. Seems like it should just be a general IP address check since it's trying to determine if it's a url it can normalize.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it's addressing this requirement:

Cypress automatically prepends the http:// protocol to common hosts. If you're not using one of these 3 hosts, then make sure to provide the protocol.

cy.visit('localhost:3000') // Visits http://localhost:3000
cy.visit('0.0.0.0:3000') // Visits http://0.0.0.0:3000
cy.visit('127.0.0.1:3000') // Visits http://127.0.0.1:3000

https://docs.cypress.io/api/commands/visit#Protocol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good find. In that case, it seems like that variable name is a bit misleading, but we probably shouldn't change the behavior since it's documented that way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Except for maybe 0.0.0.0... I don't think there's any possible way to load something from the null route

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there are some references to using 0.0.0.0 in the issues: https://github.com/cypress-io/cypress/issues?q=is%3Aissue+0.0.0.0+

return Boolean(
url.hostname === 'localhost'
// [::1] is the IPv6 localhost address.
|| url.hostname === '[::1]'
// 127.0.0.0/8 are considered localhost for IPv4.
|| url.hostname.match(localhostIPRegex),
)
}
40 changes: 40 additions & 0 deletions packages/network/test/unit/uri_spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import { expect } from 'chai'
import { URL } from 'url'

import { uri } from '../../lib'

describe('lib/uri', () => {
context('.isLocalhost', () => {
it('http://localhost is localhost', () => {
expect(uri.isLocalhost(new URL('http://localhost'))).to.be.true
})

it('https://localhost is localhost', () => {
expect(uri.isLocalhost(new URL('https://localhost'))).to.be.true
})

it('http://127.0.0.1 is localhost', () => {
expect(uri.isLocalhost(new URL('http://127.0.0.1'))).to.be.true
})

it('http://127.0.0.9 is localhost', () => {
expect(uri.isLocalhost(new URL('http://127.0.0.9'))).to.be.true
})

it('http://[::1] is localhost', () => {
expect(uri.isLocalhost(new URL('http://[::1]'))).to.be.true
})

it('http://128.0.0.1 is NOT localhost', () => {
expect(uri.isLocalhost(new URL('http://128.0.0.1'))).to.be.false
})

it('http:foobar.com is NOT localhost', () => {
expect(uri.isLocalhost(new URL('http:foobar.com'))).to.be.false
})

it('https:foobar.com is NOT localhost', () => {
expect(uri.isLocalhost(new URL('https:foobar.com'))).to.be.false
})
})
})
Loading