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

chrisbreiding · 2022-03-02T18:46:27Z

Closes [Multi-domain]: Test authentication workflow #19849

User facing changelog

N/A

Additional details

For multi-domain authentication workflows, when the secondary domain attempts to set a cookie and the cookie is SameSite=Strict/Lax, the browser won't set it since top is a different origin. This is also the case when navigating back to the primary domain, even though the origin is the same as top, since it's cross-origin and not a top-level navigation.

To resolve this, when a request attempts to set a cookie for a request from the AUT that is cross-origin, we force the cookie to be SameSite=None; Secure, so that it gets set by the browser. Firefox won't set SameSite=None; Secure cookies for http://localhost in an iframe, so we remove the Secure flag in that case.

This is a temporary solution for the experimental release of multi-domain support. In the future, this will be replaced with a more robust solution that provides the necessary cookies to cross-origin requests without changing the SameSite attribute of the cookie when a request attempts to set it.

PR Tasks

Have tests been added/updated?
N/A Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
N/A Has a PR for user-facing changes been opened in cypress-documentation?
N/A Have API changes been updated in the type definitions?
N/A Have new configuration options been added to the cypress.schema.json?

data sent through the communicator has to be an object, or else 'a string' gets turned into { 0: 'a', 1: ' ', 2: 's', ... }

cypress-bot · 2022-03-02T18:46:30Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

cypress · 2022-03-04T19:52:09Z

Test summary

5273 • 0 • 71 • 1 • 3

Run details


Project	cypress
Status	Passed
Commit	`796df1d`
Started	Mar 21, 2022 6:25 PM
Ended	Mar 21, 2022 6:40 PM
Duration	14:50 💡
OS	Linux Debian - 10.10
Browser	Electron 94

View run in Cypress Dashboard ➡️

Flakiness

	commands/actions/click_spec.js	1
1	... > throws when any member of the subject isnt visible
	settings_spec.js	1
1	Settings > file preference panel > loads preferred editor, available editors and shows spinner
	cy/timers_spec.js	1
1	driver/src/cy/timers > can cancel setIntervals paused or unpaused

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

…e flag set

packages/driver/cypress/integration/e2e/multi-domain/cookie_login_spec.ts

packages/network/lib/uri.ts

flotwig

This is a temporary solution for the experimental release of multi-domain support. In the future, this will be replaced with a more robust solution that provides the necessary cookies to cross-origin requests without changing the SameSite attribute of the cookie when a request attempts to set it.

@chrisbreiding this is the "don't modify the broken header, just use automation to force it to be set" approach, correct? Is there already a task created for this somewhere?

flotwig · 2022-03-17T20:36:32Z

packages/proxy/test/unit/http/response-middleware.spec.ts

+ })
+ })
+
+ describe('cookie modification specifics - Firefox + https://localhost', function () {


nit: kinda feels like all these should be nested describes in a top-level describe('cookie modification specifics')

Addressed in c0a3ce8

flotwig · 2022-03-17T20:39:31Z

packages/proxy/lib/http/response-middleware.ts

+ const browser = this.getCurrentBrowser() || { family: null }
+ const url = new URL(this.req.proxiedUrl)
+ const isLocalhost = uri.isLocalhost(url)


these 3 lines should go inside of if (cookies), since they are not used outside of that scope

Addressed in e88519a

flotwig · 2022-03-17T20:42:41Z

packages/proxy/lib/http/index.ts

@@ -43,7 +44,10 @@ type HttpMiddlewareCtx<T> = {
 debug: Debug.Debugger
 middleware: HttpMiddlewareStacks
 deferSourceMapRewrite: (opts: { js: string, url: string }) => string
+ getCurrentBrowser: () => Browser | Partial<Browser> & Pick<Browser, 'family'> | null


my apologies for all the changes you had to make to this file... in retrospect, this was an unmaintainable way to set up how ctx works. this area needs a refactor, like, yesterday. not related to this PR, just sharing my shame 😄

flotwig · 2022-03-17T20:53:18Z

packages/network/lib/uri.ts

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


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?

Addressed in a2f7bd8

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.

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

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.

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

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.

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

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+

flotwig · 2022-03-17T20:58:22Z

packages/server/lib/browsers/cdp_automation.ts

+ private sendDebuggerCommandFn: SendDebuggerCommand,
+ onFn: OnFn,
+ private automation: Automation,
+ private options: any,


can you type this please, and maybe move sendDebuggerCommandFn, onFn, and automation to this options object? let's keep this signature as clean as possible, it has been ballooning

Addressed in 4ed06ad

flotwig · 2022-03-17T21:01:11Z

packages/proxy/lib/http/response-middleware.ts

+ // Secure is required in Chrome for SameSite=None cookies to be set, but if
+ // in Firefox and http://localhost, it causes the cookie not to be set,
+ // so we need to ensure there is no Secure in that case


i'd also like to see some links to defintiive documentation (RFC, source code, authoritative reference...) for this, i believe you, but it's important to clearly track browser errata like this since it changes under our feet all the time...

I dug a bit deeper into this and the reason there's a difference in Firefox is that it doesn't consider http://localhost to be a secure context when loaded by the Cypress-launched browser, even though it's normally considered a secure context.

Here's an existing issue about it. It has something to do with the way we launch Firefox. I dug into it a little bit, but it needs a bit more research and I decided to time-box the effort since it's out of scope for this work.

The fact is that http://localhost won't allow SameSite=None; Secure cookies to be set even outside of multi-domain. The question then is whether we should have this workaround at all to support it for multi-domain when it doesn't even work in general. I decided to keep the workaround in for the following reasons:

It's only going in the experimental release and will be removed once better cookie handling is implemented.

The primary use-case for multi-domain is authentication, so I think it's important to get cookies working consistently between browsers for authentication purposes.

We may need a similar workaround outside of multi-domain anyway, though I don't want to go that far at this moment. I feel it's best to scope this to the multi-domain work and experimental release instead of doing a general workaround without doing more research into why Firefox doesn't consider the browser a secure context.

I improved the comment here, explaining what I did above in brief and linking to the spec regarding secure contexts.

chrisbreiding · 2022-03-18T13:41:02Z

@chrisbreiding this is the "don't modify the broken header, just use automation to force it to be set" approach, correct? Is there already a task created for this somewhere?

Yeah, that's right. Here's the issue for it.

packages/proxy/lib/http/response-middleware.ts

chrisbreiding added 4 commits February 28, 2022 10:36

fix issue with url:changed

73f4b56

data sent through the communicator has to be an object, or else 'a string' gets turned into { 0: 'a', 1: ' ', 2: 's', ... }

remove obsolete event emission

7774a40

Merge branch 'feature-multidomain' into issue-19849-auth-workflow

31eb352

force SameSite=None for cross-origin AUT cookies

a6dc42f

chrisbreiding requested a review from a team as a code owner March 2, 2022 18:46

chrisbreiding requested review from jennifer-shehane and removed request for a team March 2, 2022 18:46

chrisbreiding added 2 commits March 2, 2022 13:48

fix wrong selector

5b07648

add test for initial AUT request

320bdd4

chrisbreiding marked this pull request as draft March 2, 2022 19:11

chrisbreiding removed the request for review from jennifer-shehane March 2, 2022 19:11

reset previousAUTRequestUrl between tests

a0464b5

chrisbreiding and others added 13 commits March 14, 2022 10:34

Merge branch 'feature-multidomain' into issue-19849-auth-workflow

028dc76

handle firefox case where http://localhost cookies are set with Secur…

7375a9b

…e flag set

Merge branch 'feature-multidomain' into issue-19849-auth-workflow

4e99df3

fix types

0e89b36

require experimental flag for cookie manipulation

dab0cca

Merge branch 'feature-multidomain' into issue-19849-auth-workflow

be3a7c0

Merge branch 'feature-multidomain' into issue-19849-auth-workflow

a6d46c9

Merge branch 'feature-multidomain' into issue-19849-auth-workflow

6b53e93

fix tests

f9c4127

fix tests

1ae7359

fix issue with logs having err property

2f82382

put cdp cookies change behind experimental flag

84b2948

empty commit for clean ci re-run

530304c

chrisbreiding requested review from mschile, mjhenkes and AtofStryker March 17, 2022 16:19

chrisbreiding requested a review from flotwig March 17, 2022 16:33

chrisbreiding added 2 commits March 17, 2022 14:22

remove unnecessary changes

3411590

one more

8a0f2b1

mjhenkes reviewed Mar 17, 2022

View reviewed changes

packages/driver/cypress/integration/e2e/multi-domain/cookie_login_spec.ts Show resolved Hide resolved

mjhenkes reviewed Mar 17, 2022

View reviewed changes

packages/network/lib/uri.ts Show resolved Hide resolved

flotwig suggested changes Mar 17, 2022

View reviewed changes

flotwig reviewed Mar 17, 2022

View reviewed changes

chrisbreiding added 3 commits March 18, 2022 09:08

organize response-middleware tests better

c0a3ce8

refactor cookie handling

e88519a

improve cdp options types

4ed06ad

chrisbreiding mentioned this pull request Mar 18, 2022

Multi-domain: Improve cookie handling #20685

Closed

chrisbreiding added 2 commits March 18, 2022 13:54

add rfc comments to isLocalhost and add .localhost support

a2f7bd8

fix reference

58c3714

mjhenkes approved these changes Mar 18, 2022

View reviewed changes

chrisbreiding added 2 commits March 21, 2022 09:20

fix test

1b111e6

improve samesite cookie handling comment

aeed87e

chrisbreiding requested a review from flotwig March 21, 2022 13:45

Merge branch 'feature-multidomain' into issue-19849-auth-workflow

3dfaf14

AtofStryker reviewed Mar 21, 2022

View reviewed changes

packages/proxy/lib/http/response-middleware.ts Outdated Show resolved Hide resolved

packages/proxy/lib/http/response-middleware.ts Outdated Show resolved Hide resolved

packages/proxy/lib/http/response-middleware.ts Show resolved Hide resolved

add/improve types

796df1d

mschile approved these changes Mar 21, 2022

View reviewed changes

AtofStryker self-requested a review March 21, 2022 19:08

AtofStryker approved these changes Mar 21, 2022

View reviewed changes

flotwig approved these changes Mar 22, 2022

View reviewed changes

Merge branch 'feature-multidomain' into issue-19849-auth-workflow

b808da0

chrisbreiding merged commit 22b35be into feature-multidomain Mar 22, 2022

chrisbreiding deleted the issue-19849-auth-workflow branch March 22, 2022 13:57

cypress-bot bot mentioned this pull request Mar 22, 2022

[Multi-domain]: Test authentication workflow #19849

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

chrisbreiding commented Mar 2, 2022 •

edited

Loading

cypress-bot bot commented Mar 2, 2022

cypress bot commented Mar 4, 2022 •

edited

Loading

flotwig left a comment

flotwig Mar 17, 2022

chrisbreiding Mar 18, 2022

flotwig Mar 17, 2022

chrisbreiding Mar 18, 2022

flotwig Mar 17, 2022

flotwig Mar 17, 2022

chrisbreiding Mar 18, 2022

mschile Mar 18, 2022

flotwig Mar 18, 2022

chrisbreiding Mar 21, 2022

mschile Mar 21, 2022

chrisbreiding Mar 21, 2022

flotwig Mar 22, 2022

mschile Mar 22, 2022

flotwig Mar 17, 2022 •

edited

Loading

chrisbreiding Mar 18, 2022

flotwig Mar 17, 2022

chrisbreiding Mar 21, 2022

chrisbreiding commented Mar 18, 2022


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

		export function isLocalhost (url: URL) {

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

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

Conversation

chrisbreiding commented Mar 2, 2022 • edited Loading

User facing changelog

Additional details

PR Tasks

cypress-bot bot commented Mar 2, 2022

cypress bot commented Mar 4, 2022 • edited Loading

Test summary

Run details

Flakiness

flotwig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flotwig Mar 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbreiding commented Mar 18, 2022

chrisbreiding commented Mar 2, 2022 •

edited

Loading

cypress bot commented Mar 4, 2022 •

edited

Loading

flotwig Mar 17, 2022 •

edited

Loading