breaking: no longer inject document.domain by default #30770

cacieprins · 2024-12-16T21:14:36Z

Docs PR: cypress-io/cypress-documentation#5974

Additional details

Chrome is in the process of deprecating the setter for document.domain. As well, our usage of document.domain caused issues with certain sites that used origin agent clusters for security.

Cypress used document.domain to allow for navigation between subdomains in a single test without requiring the use of cy.origin().With the deprecation of document.domain, Cypress can no longer rely on the availability of a setter on document.domain.

Because this is a fairly disruptive change for users who rely on cy.origin() to navigate between subdomains, a new configuration option is being introduced. injectDocumentDomain can be set to true in order to re-enable the injection of document.domain setters in Cypress. This configuration option is marked as deprecated, and you will receive a warning when Cypress is launched with this option set to true. It will be removed in Cypress 15.

Steps to test

How has the user experience changed?

The cy.origin() command must now be used when navigating between subdomains, unless the injectDocumentDomain e2e configuration option is enabled. When this option is enabled, there will be a warning when Cypress starts.

PR Tasks

Have tests been added/updated?
Has a PR for user-facing changes been opened in cypress-documentation? Config option - injecting document.domain cypress-documentation#5974
[NA] Have API changes been updated in the type definitions?

…entDomain

…morphism

…rigins

… add injectDocumentDomain cases

…ments

cacieprins · 2024-12-20T14:24:17Z

.circleci/workflows.yml

+
+            if << parameters.inject-document-domain >> ; then
+              YARN_CMD="cypress:run:inject-document-domain"
+              PARALLEL=""


This can't be parallel because it ends up with the same build id but a different set of specs from the run without injectDocumentDomain

cacieprins · 2024-12-20T14:26:33Z

.circleci/workflows.yml

@@ -3000,6 +3046,8 @@ linux-x64-workflow: &linux-x64-workflow
          - driver-integration-tests-firefox
          - driver-integration-tests-chrome
          - driver-integration-tests-chrome-beta
+          - driver-integration-tests-chrome-inject-document-domain
+          - driver-integration-tests-chrome-beta-inject-document-domain


adding firefox and electron to this mix was going to be pretty cumbersome - I'd like to refactor workflows.yml to use matrix parameters, which would make all of this so much easier.

cacieprins · 2024-12-20T14:27:35Z

cli/types/cypress.d.ts

@@ -70,8 +70,7 @@ declare namespace Cypress {
    strategy: 'file' | 'http'
    origin: string
    fileServer: string | null
-    props: Record<string, any>
-    visiting: string
+    props: Record<string, any> | null


visiting was never used or set internally that I could find, and props not being nullable was inconsistent with the implementation. Potential improvement: import this type directly from packages/server/lib/remote_states so it stays consistent with the implementation

cacieprins · 2024-12-20T14:32:43Z

npm/cypress-schematic/src/ct.spec.ts

+      const projectPath: string = Fixtures.projectPath(project)
+
+      beforeEach(async () => {
+        await Fixtures.scaffoldProject(project)


moving the scaffoldAngularProject function inline into a beforeEach here keeps the DRY while improving the readability of the spec

should we consider moving these changes into its own PR for a quick review and to reduce changeset here?

I think so - This refactor was done to help understand why the tests were failing, and don't actually change the nature of the tests other than clarification of stdout.

cacieprins · 2024-12-20T14:34:08Z

npm/cypress-schematic/src/ct.spec.ts

+      }, timeout)
+
+      afterEach(() => {
+        Fixtures.removeProject(project)


Moving the cleanup to an afterEach ensures that the project is removed after the last it(), rather than left in place

cacieprins · 2024-12-20T14:51:34Z

packages/driver/src/cypress/error_utils.ts

@@ -316,10 +324,6 @@ export class CypressError extends Error {
  }
 }

-const getUserInvocationStackFromError = (err) => {


This function removed for clarity sake

cacieprins · 2024-12-20T14:54:27Z

packages/driver/src/cypress/stack_utils.ts

-    if (specWindow.Cypress && specWindow.Cypress.isBrowser('firefox')) {
-      stack = stackWithLinesDroppedFromMarker(stack, '__cypress/tests', true)
+    // firefox and chrome throw stacks that include lines from cypress
+    // So we drop the lines until we get to the spec stackframe (includes __cypress)


expanding this from __cypress/tests ensures that cy in cy component tests retain the correct stack

cacieprins · 2024-12-20T15:17:18Z

packages/server/lib/socket-base.ts

@@ -164,6 +163,7 @@ export class SocketBase {
    const cdpIo = this._cdpIo = this.createCDPIo(socketIoRoute)

    automation.use({
+      // @ts-ignore - this error is new, but not introduced in the most recent edit. TODO: fix


I think this was causing issues with linting; VScode was showing an error here with the file open, but lint only started failing against it when I changed it. For ref,

onPush is defined as a NullableMiddlewareHook = (() => void) | null, which means either this function never receives the expected props, or onPush actually being NullableMiddlewareHook instead of something else is in question.

This is all CDP/BiDi signature related so I would be surprised if the change was introduced here

cacieprins · 2024-12-20T15:21:44Z

packages/server/test/integration/cypress_spec.js

-    it('logs error and exits when project folder has read permissions only and cannot write cypress.config.js', function () {
+    // this test is skipped because its failure causes websocket integration tests to fail.
+    // this test should be revisited, as the error it's asserting on probably can never be
+    // actually thrown by Cypress.


This test is never run in CI (where tests are run as root), and it always fails locally.

The ERROR_WRITING_FILE error is thrown by _logWriteErr in packages/server/lib/util/settings.ts, which is referenced by _write() in the same file. _write() is only referenced by writeForTesting() in the same file, which is only referenced in tests, and not runtime code.

cacieprins · 2024-12-20T15:26:36Z

packages/server/test/support/fixtures/server/expected_head_inject_document_domain.html

maybe not all of these extra files are necessary - I'm spending some time going through and pruning unnecessary ones. (maybe these should be snapshots instead of fixtures, since they're expected data instead of test input data)

jennifer-shehane

@cacieprins Please feel free to bypass Brian and I as codeowner approvals and merge once you get 2 review approvals. The exposure of the injectDocumentDomain config and removal of experimental is acceptable as an exposed portion of our public API.

jennifer-shehane · 2024-12-20T16:05:13Z

cli/types/cypress.d.ts

@@ -3113,6 +3112,12 @@ declare namespace Cypress {
     * @default null
     */
    experimentalSkipDomainInjection: string[] | null


@cacieprins This experimentalSkipDomainInjection type should be removed.

Looks to need some adjustments in @packages.config where we need to remove the option experimentalSkipDomainInjection as well as adding an option that it was removed here and updating EXPERIMENTAL_USE_DEFAULT_DOCUMENT_DOMAIN_E2E_ONLY to the new option

I think full removal of experimentalSkipDomainInjection should be in a follow-up PR, to keep the scope of this as manageable as possible

jennifer-shehane · 2024-12-20T16:13:34Z

packages/errors/src/errors.ts

@@ -1350,19 +1350,29 @@ export const AllCypressErrors = {
    return errTemplate`\
    The ${fmt.highlight(`justInTimeCompile`)} configuration is only supported for Component Testing.`
  },
+  // TODO: link to docs on the new injectDocumentDomain config option


@cacieprins Is this wanted to be done before 14 releases? If so, I would add an on link and we can route it to the https://docs.cypress.io/app/references/error-messages where the specific error explanation would be in the 14 docs branch.

Yep, that work is in progress

AtofStryker · 2024-12-20T17:52:06Z

cli/types/cypress.d.ts

@@ -3113,6 +3112,12 @@ declare namespace Cypress {
     * @default null
     */
    experimentalSkipDomainInjection: string[] | null


Looks to need some adjustments in @packages.config where we need to remove the option experimentalSkipDomainInjection as well as adding an option that it was removed here and updating EXPERIMENTAL_USE_DEFAULT_DOCUMENT_DOMAIN_E2E_ONLY to the new option

AtofStryker · 2024-12-20T17:52:46Z

npm/cypress-schematic/src/ct.spec.ts

+      const projectPath: string = Fixtures.projectPath(project)
+
+      beforeEach(async () => {
+        await Fixtures.scaffoldProject(project)


should we consider moving these changes into its own PR for a quick review and to reduce changeset here?

AtofStryker · 2024-12-20T17:53:47Z

package.json

@@ -278,5 +278,6 @@
    "devtools-protocol": "0.0.1346313",
    "sharp": "0.29.3",
    "vue-template-compiler": "2.6.12"
-  }
+  },
+  "packageManager": "[email protected]+sha512.ff4579ab459bb25aa7c0ff75b62acebe576f6084b36aa842971cf250a5d8c6cd3bc9420b22ce63c7f93a0857bc6ef29291db39c3e7a23aab5adfd5a4dd6c5d71"


I think develop is on 1.22.22 which gets set via yarn set version. We might need to merge develop into 14 which I can look into shortly

AtofStryker · 2024-12-20T17:54:23Z

packages/config/__snapshots__/index.spec.ts.js

@@ -218,6 +220,7 @@ exports['config/src/index .getPublicConfigKeys returns list of public config key
  'experimentalRunAllSpecs',
  'experimentalMemoryManagement',
  'experimentalModifyObstructiveThirdPartyCode',
+  'injectDocumentDomain',
  'experimentalSkipDomainInjection',


this should be removed from the snapshots

Suggested change

'experimentalSkipDomainInjection',

'',

AtofStryker · 2024-12-20T17:55:06Z

packages/config/src/options.ts

+    validation: validate.isBoolean,
+    requireRestartOnChange: 'server',
+  },
+  {
    name: 'experimentalSkipDomainInjection',


shouldn't need this anymore as well

AtofStryker · 2024-12-20T18:30:07Z

packages/server/lib/socket-base.ts

@@ -164,6 +163,7 @@ export class SocketBase {
    const cdpIo = this._cdpIo = this.createCDPIo(socketIoRoute)

    automation.use({
+      // @ts-ignore - this error is new, but not introduced in the most recent edit. TODO: fix


This is all CDP/BiDi signature related so I would be surprised if the change was introduced here

AtofStryker · 2024-12-20T18:34:55Z

packages/server/lib/socket-base.ts


-          cookieJar.setCookie(automationCookieToToughCookie(cookie, domain), url, sameSiteContext)
+          cookieJar.setCookie(automationCookieToToughCookie(cookie, origin), url, sameSiteContext)


does it set the origin or the domain on the cookie? I see this is just a port of the existing logic, but I wonder if it's correct. For example:

for URL https://www.foobar.com/baz the

origin: https://www.foobar.com

domain: www.foobar.com

my guess is tough-cookie parses this out for us but I want to double check here. Are you able to verify that?

It looks like they are handling things correctly on there end and it shouldn't matter. I don't think there are existing bugs with this I just wanted to be sure that origin ultimately translates to domain on the tough-cookie side of things

This is a good catch - I'll change this to hostname to more accurately match the RFC that tough-cookie implements.

Addressed in fb1219c

system-tests/__snapshots__/protocol_spec.js

system-tests/test/experimental_skip_domain_injection_spec.ts

AtofStryker · 2024-12-20T18:44:54Z

system-tests/test/results_spec.ts

@@ -27,7 +24,7 @@ const normalizeResults = (resultsJson) => {
  .replace(majorVersionRegex, '"$1": "X"')
  .replace(osNameRegex, '"$1": "linux"')
  .replace(archRegex, '"arch": "x64"')
-  .replace(stackLineRegex, '"displayError": "$1 <stack lines>"')
+  .replace(/(\Wn {4}at.+)"/g, ' <stack lines>"')


Can we add a comment to what is this regex doing? I'm not great at reading it on first glance 😅

Would we not want the definition of this regex to be outside of these function calls?

Co-authored-by: Bill Glesias <[email protected]>

AtofStryker · 2024-12-20T18:54:34Z

also wonder if we can remove the MaybeSetOriginAgentClusterHeader middleware now

…nject-document-domain test subset

…ains

…ub.com/cypress-io/cypress into cacie/29590/document-domain-subdomains

cacieprins added 30 commits October 22, 2024 11:11

remove experimentalSkipDomainInjection, add and deprecate injectDocum…

5b42419

…entDomain

remove experimentalSkipDomainInjection, add injectDocumentDomain

151952d

begin rethreading domain injection

dc4fb0b

complete document domain transition

330adc4

move some cookie specs to separate test run

2000405

origin and privileged commands with default docdom inject

0f822b5

fix privileged channel when injecting document domain

8275e4d

rm unnecessary .getOrigin abstraction in cors lib

4a52817

move remote-states in prep for refactor Replace Conditional with Poly…

b3b60a1

…morphism

refactor remote states to strategy pattern

a8f6406

cookie commands work as expected w cross origin bridge on different o…

c54224b

…rigins

some origin tests updated

110996b

run tests with document domain enabled

4895219

run tests actually

33d33cd

use correct config, swap conditional

1d348c9

check-ts

8b943d0

inject documetn domain for webkit tests

1166858

do not exec injectDocumetnDomain in parallel

25c3ac7

fix ServerBase construction in tests to include cfg now

b4c0be3

pass cfg to ServerBase

3e613f0

improved integration tests

b6090f7

remove document domain checks for all server integration specs - will…

4da149d

… add injectDocumentDomain cases

tests for injecting document domain when configured to

9bcc1f3

square away server integration tests

e8ccb4d

ensure cookies are set correctly, potentially

db047b7

errors pkg snapshots

55fa5b1

fix config tests

018816c

fixing config tests

5622cc5

somewhat improves tests for cors policies in packages/network

b01df45

fix ts err in server-base

03f34bb

cacieprins added 2 commits December 19, 2024 14:47

remove stack_utils attempt at normalizing wsp

0300676

various cleanup: remove commented console logs, add more detailed com…

4e090de

…ments

cacieprins commented Dec 20, 2024

View reviewed changes

cacieprins changed the title ~~Cacie/29590/document domain subdomains - draft~~ breaking: no longer inject document.domain by default Dec 20, 2024

cacieprins marked this pull request as ready for review December 20, 2024 15:38

cacieprins requested review from brian-mann and jennifer-shehane as code owners December 20, 2024 15:38

jennifer-shehane assigned cacieprins Dec 20, 2024

jennifer-shehane reviewed Dec 20, 2024

View reviewed changes

AtofStryker reviewed Dec 20, 2024

View reviewed changes

cacieprins and others added 4 commits December 20, 2024 13:46

add on links to error messages

58db915

remove experimentalSkipDomainInjection from exported type defs

728e4bf

Update system-tests/test/experimental_skip_domain_injection_spec.ts

bed8fb3

Co-authored-by: Bill Glesias <[email protected]>

Update packages/driver/cypress/e2e/e2e/origin/cookie_misc.cy.ts

a0a151a

Co-authored-by: Bill Glesias <[email protected]>

cacieprins added 5 commits December 20, 2024 14:16

no need to coerce a boolean value to a booleanc

6c52c93

export base config from primary cypress config in driver for use in i…

34e0573

…nject-document-domain test subset

lift experimentalSkipDomainInjection breaking option to root

c03e62b

rollback config/options changes

0539a72

rm invalid comment

a8f0663

cacieprins and others added 6 commits December 20, 2024 14:51

use hostname instead of origin to create cookie from automation cookie

fb1219c

clarify stack regex in results_spec

ea57654

lint

b86bd00

Merge branch 'release/14.0.0' into cacie/29590/document-domain-subdom…

a20a34e

…ains

take a stab at the changelog entries for this

1bece96

Merge branch 'cacie/29590/document-domain-subdomains' of https://gith…

8ce3f5e

…ub.com/cypress-io/cypress into cacie/29590/document-domain-subdomains

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

breaking: no longer inject document.domain by default #30770

breaking: no longer inject document.domain by default #30770

cacieprins commented Dec 16, 2024 •

edited by jennifer-shehane

Loading

cacieprins Dec 20, 2024

cacieprins Dec 20, 2024

cacieprins Dec 20, 2024

cacieprins Dec 20, 2024

AtofStryker Dec 20, 2024

cacieprins Dec 20, 2024 •

edited

Loading

cacieprins Dec 20, 2024

cacieprins Dec 20, 2024

cacieprins Dec 20, 2024

cacieprins Dec 20, 2024

AtofStryker Dec 20, 2024

cacieprins Dec 20, 2024

cacieprins Dec 20, 2024

jennifer-shehane left a comment

jennifer-shehane Dec 20, 2024

AtofStryker Dec 20, 2024

cacieprins Dec 20, 2024

jennifer-shehane Dec 20, 2024

cacieprins Dec 20, 2024

AtofStryker Dec 20, 2024

AtofStryker Dec 20, 2024

AtofStryker Dec 20, 2024

AtofStryker Dec 20, 2024

AtofStryker Dec 20, 2024

AtofStryker Dec 20, 2024

AtofStryker Dec 20, 2024

AtofStryker Dec 20, 2024

cacieprins Dec 20, 2024

cacieprins Dec 20, 2024

AtofStryker Dec 20, 2024

jennifer-shehane Dec 23, 2024

AtofStryker commented Dec 20, 2024


		cookieJar.setCookie(automationCookieToToughCookie(cookie, domain), url, sameSiteContext)
		cookieJar.setCookie(automationCookieToToughCookie(cookie, origin), url, sameSiteContext)

breaking: no longer inject document.domain by default #30770

Are you sure you want to change the base?

breaking: no longer inject document.domain by default #30770

Conversation

cacieprins commented Dec 16, 2024 • edited by jennifer-shehane Loading

Additional details

Steps to test

How has the user experience changed?

PR Tasks

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

cacieprins Dec 20, 2024 • 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

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

jennifer-shehane 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtofStryker commented Dec 20, 2024

cacieprins commented Dec 16, 2024 •

edited by jennifer-shehane

Loading

cacieprins Dec 20, 2024 •

edited

Loading