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: experimental skip domain injection #25307

Merged
merged 45 commits into from
Jan 9, 2023

Conversation

AtofStryker
Copy link
Contributor

@AtofStryker AtofStryker commented Dec 29, 2022

User facing changelog

Adds the experimentalSkipDomainInjection option to disable Cypress from setting document.domain on injection.

The experimentalSkipDomainInjection option is either an array of strings/globs to match against url hostnames. If there is a match, document.domain will not be injected. This means that all cross-origin navigations in the provided config, including sub domain navigations, will require a cy.origin() block for Cypress to interact with the page.

Additional details

The goal of this flag is to unblock Cypress users who are running into document.domain issues with the sites they are testing, which includes Salesforce Winter '22. This can also be a diagnostic tool in the future as well for other sites where we expect document.domain to possibly be an issue, and we can simply ask users to toggle on the flag and see if the situation improves

Currently the goal of this experimental flag is to unblock users with document.domain. We want to observe this experiment usage which should educate how it evolves over time. There are no plans to make this feature GA, and at best this may be a supported configuration option in the future that is not experimental (unless setting document.domain is fully removed, in which case we may revisit this).

cypress-documentation likely will need to add info snippets in the following areas, including information on the experimental flag (added in cypress-io/cypress-documentation#4955):

Steps to test

Since this change requires a experimental config option that the server needs to be aware of, system tests were added to fully test the expected behavior in regards to navigation

How has the user experience changed?

PR Tasks

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Dec 29, 2022

Thanks for taking the time to open a PR!

* @see https://developer.mozilla.org/en-US/docs/Web/API/Document/domain
* @default false
*/
experimentalUseDefaultDocumentDomain: boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

What changes would it take to make this the default behavior for all users (taking it out of experimental)?

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 it depends where we land, whether its an "all or nothing" approach like what's implemented right now or something that is configurable per domain. As for now, the only thing we would need to do to take this out of experimental is making the policyForDomain always same-origin and reverse the conditional which is pretty straight forward. And the change would be breaking.

However I think right now it likely may not work with same origin iframes within the AUT, since we need partial document.domain injection for same-origin localhost iframes. There might need to be some caveats around this, which would probably be a good idea to document.

@cypress
Copy link

cypress bot commented Dec 29, 2022



Test summary

26418 0 1179 0Flakiness 45


Run details

Project cypress
Status Passed
Commit 100a8cb
Started Jan 6, 2023 9:05 PM
Ended Jan 6, 2023 9:24 PM
Duration 19:13 💡
OS Linux Debian -
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

create-from-component.cy.ts Flakiness
1 ... > runs generated spec
2 ... > runs generated spec
commands/net_stubbing.cy.ts Flakiness
1 network stubbing > intercepting request > can delay and throttle a StaticResponse
2 network stubbing > intercepting request > can delay and throttle a StaticResponse
3 network stubbing > intercepting request > can delay and throttle a StaticResponse
This comment includes only the first 5 flaky tests. See all 45 flaky tests in the Cypress Dashboard.

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

feat: cut over experimental flag to take list of known problematic domains via string/glob pattern

run ci

chore: update system test and fix broken config
@AtofStryker AtofStryker force-pushed the feat/experimentalDefaultDocumentDomain branch from 3d391c3 to fc18ac6 Compare December 29, 2022 19:39
@AtofStryker
Copy link
Contributor Author

I would have expected a new error message or two for cross-origin errors for when this is set to remind users they excluded this domain and cy.origin() is required for this specific super domains.

Currently we are just reusing the same-origin / same-super-domain messaging in error_messages, which for the domain configured would error for an origin mismatch and not domain. But right now I think Google is the only exception to this rule, so I wonder if we can add a plug for the experiment and if the flag is turned on say something like " Since experiment is on and you have this domain configured, you'd need a cy.origin() block when performing cross origin navigations". Do you think something like that would be helpful? Right now I feel the error is a bit contextless

@AtofStryker
Copy link
Contributor Author

@emilyrohrbough the cross origin error looks something like this for subdomains with the option configured correctly:

const code = errPartial`
{
e2e: {
experimentalSkipDomainInjection: ['*.salesforce.com', '*.force.com', '*.google.com', 'google.com']
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I go with something less specific here?

Choose a reason for hiding this comment

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

I've opened the #24418 issue, and I think this should be changed for the following domains also: *.my.site.com
taken from https://help.salesforce.com/s/articleView?id=sf.domain_name_enhanced.htm&type=5

Copy link
Contributor Author

@AtofStryker AtofStryker Jan 5, 2023

Choose a reason for hiding this comment

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

@alexanderg1982 this should resolve #24418. This line here is more so to recommend users on how to configure the option if the config isn't inside the e2e object. I think it's too specific since we just want to recommend where to move the option. When we release this, you should be able to do:

experimentalSkipDomainInjection: ['power-inspiration-1088-dev-ed.scratch.my.site.com']

Copy link
Member

Choose a reason for hiding this comment

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

@AtofStryker Maybe it would be good to call out this is a recommendation?

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'm wondering if we just want to reference the enhanced domain names in the docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexanderg1982 I added the enhanced domain reference in the docs to help guide the correct domains. Thank you for sharing this!

@emilyrohrbough
Copy link
Member

" Since experiment is on and you have this domain configured, you'd need a cy.origin() block when performing cross origin navigations". Do you think something like that would be helpful? Right now I feel the error is a bit contextless

I do think this would be helpful, especially because there are workflows working for users when on Firefox so it'd could be helpful to provide more direction.

@emilyrohrbough emilyrohrbough changed the title feat: experimental default document domain feat: experimental skip domain injection Jan 6, 2023
@AtofStryker
Copy link
Contributor Author

Added some context in the cross origin error @emilyrohrbough in e27acf7 . It isn't smart enough right now to compare the url, but we could add that in the future or if we really think it would be helpful right now

@AtofStryker AtofStryker merged commit d470f59 into develop Jan 9, 2023
@AtofStryker AtofStryker deleted the feat/experimentalDefaultDocumentDomain branch January 9, 2023 15:00
tgriesser added a commit that referenced this pull request Jan 18, 2023
* develop: (45 commits)
  fix: re-enable CYPRESS_INTERNAL_VITE_DEV development (#25364)
  fix: add skip domain injection description (#25463)
  fix: revert CSP header and script-src addition (#25445)
  chore: Update v8 snapshot cache (#25401)
  feat: Do not strip CSP headers from HTTPResponse (#24760)
  fix: keep spaces in formatted output in test runner (#24687)
  fix: Restrict dependency versions to known supported ranges (#25380)
  chore: Update v8 snapshot cache (#25370)
  feat: experimental skip domain injection (#25307)
  chore: support vite v4 for component testing (#25365)
  feat: Use JSX/TSX in generated spec filenames (#25318)
  docs(angular): Properties that are spied upon have to be defined within `componentProperties` instead of on root level. (#25359)
  chore: remove lint-changed from scripts/docs (#25308)
  chore: bump to 12.3.0 [skip ci] (#25355)
  fix: make NODE_ENV "production" for prod builds of launchpad (#25320)
  fix: .contains() should only return one element at all times (#25250)
  feat: add currentRetry to Cypress API (#25297)
  chore: release @cypress/webpack-dev-server-v3.2.2
  chore: release create-cypress-tests-v2.0.1
  fix: change wording for spec creation (#25271)
  ...
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jan 24, 2023

Released in 12.4.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v12.4.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Jan 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
7 participants