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

Consider moving away from document.domain to adhere to better security practices #29590

Open
AtofStryker opened this issue May 29, 2024 · 3 comments

Comments

@AtofStryker
Copy link
Contributor

AtofStryker commented May 29, 2024

What would you like?

I would like to consider removing document.domain injection and making cy.visit() require a full navigation when a subdomain navigation occurs, changing the cookie APIs to set the cookie on the current domain (not super domain), and to better adhere to full origin specifications without strange exceptions when it comes to origin and domain nomenclature.

document.domain modification deprecation
The origin specification

Why is this needed?

With the introduction of Chrome 119, Chrome and other browsers now bucket all requests to an origin server with a given Origin-Agent-Cluster key:

The browser will ensure that all pages from a given origin are either origin-keyed or they are not. This means that:

If the first page from an origin does not set the header, then no other pages from that origin will be origin-keyed, even if those other pages do set the header.
If the first page from an origin sets the header and is made origin-keyed, then all other pages from that origin will be origin-keyed whether they ask for it or not.

Cypress ran into this in it's own system tests in #29391 and we patched a work around internally (see thread on PR).

This means the Agent-Origin-Cluster header needs to be set on the first page request. However, this is difficult for Cypress for a few reasons:

Cypress only injects into cy.origin() or the Application Under Test (AUT). It is sometimes impossible to know when injection is going to be required in the future for a request that has already been sent to an origin server, which gives us two options:

  • We set Agent-Origin-Cluster: ?0 on every origin server page request, which is not only a bad security practice, but almost guarantees we will be continuing an uphill battle fighting browser security, which we don't want.
  • We remove document.domain injection, which would likely fix a slew of problems:
    • Cookie management in the server becomes more deterministic and should solve Azure AD B2C bad request when redirecting / using cy.origin() #25806 as we no longer have to infer super domain cookies and can use normal browser nomenclature.
    • Fixes cookies implicitly locally as requests would now attach correct cookies for given domain requests
      • For example, look at the following cypress spec:
          describe('cookies', () => {
            it('does not send cookie like expected', () => {
              cy.visit('https://example.cypress.io')
              // do a navigation to a sub domain
              cy.visit('https://docs.cypress.io')
              cy.setCookie('foo', 'bar', {
                domain: 'docs.cypress.io'
              }).then(async() => {
                // this does NOT send the foo=bar cookie as the host is example.cypress.io and the request is considered cross origin
                await fetch('docs.cypress.io')
              })
            })
          
            it('sends cookie like expected', () => {
              // run this in a fresh cypress instance to make sure top is reset
              cy.visit('https://docs.cypress.io')
              cy.setCookie('foo', 'bar', {
                domain: 'docs.cypress.io'
              }).then(async() => {
                // sends the foo=bar cookie
                await fetch('docs.cypress.io')
              })
            })
          })
        These tests have different behavior depending on the order they are run and which window.top domain is set first, which contrasts our best practices on test determinism. Adhereing closer to the browser specification and doing a document reload makes sure cookies are sent in the correct context.
      • Should allow us to change our cookie apies to set the cookie on the fully qualified domain and not the super domain (API breaking change), which better aligns with browser cookie api behavior which is more secure https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/cookies/set
      • cy.origin() now becomes more clear, which needs to be used on any origin navigation and not odd exceptions like sub domain navigation.
      • Likely a lot of other issues are implicitly fixed by this, such as

Other

No response

@cacieprins
Copy link
Contributor

cacieprins commented Oct 11, 2024

The solution we are moving forward with, in Cypress 14:

  • document.domain injection will be removed. document.domain is deprecated, and is explicitly prevented when the site in question is operating in an Origin-Agent-Cluster context. This has the following breaking changes:
    • cy.origin will be required in order to interact with any page that has a different hostname than the first visit() of the test.
    • Cypress cookie commands will align more closely with same-origin restrictions.
  • A configuration value (TBD) will be added to Cypress Config. When this value is set to true, document.domain injection will revert to its v13 behavior. This configuration option will be marked as deprecated, and will be removed in Cypress 15.

@alexsch01
Copy link
Contributor

Is there a way to test this behavior on existing project before Cypress 14 comes out?

@jennifer-shehane
Copy link
Member

@alexsch01 This is the branch we're working from, so you could find a commit and download the binary of one of those. It unfortunately doesn't have this work yet though as it's not complete. https://docs.cypress.io/app/references/advanced-installation#Install-pre-release-version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Building
Development

No branches or pull requests

4 participants