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: Disallow same-superdomain-origin cy.origin blocks #24569

Merged

Conversation

mjhenkes
Copy link
Member

@mjhenkes mjhenkes commented Nov 7, 2022

User facing changelog

  • cy.origin will now prevent users from setting up same-superdomain-origin cy.origin blocks. In these cases cy.origin is not required and users would be better served by not using the command.

Additional details

There are several changes included in this pr

  1. Creating a cy.origin block with the same-superdomain-origin as top will throw an error.
  2. Added a list of domains which are exceptions to the above rule and will only throw an error if the urls are strictly same-origin. Google is the only domain that is currently excepted.
  3. Correspondingly we now use the same list of domains to know when to inject 'fullCrossOrigin' in requests for the excepted domains that violate same-origin but not same-superdomain-origin.
  4. I noticed when top is in a secure context (https), we fail to create a spec bridge for an insecure context (http). I added a timeout for cy.origin to handle cases when the spec bridge isn't created. Previously this would cause a hang in the test.

Steps to test

Run the tests added in the PR.

How has the user experience changed?

Same domain error cy cy.origin:
Screen Shot 2022-11-07 at 1 06 32 PM

Failed to create spec bridge error:
Screen Shot 2022-11-07 at 1 05 18 PM

PR Tasks

  • Have tests been added/updated?
  • Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • Has a PR for user-facing changes been opened in cypress-documentation?
  • Have API changes been updated in the type definitions?

@mjhenkes mjhenkes self-assigned this Nov 7, 2022
@cypress-bot
Copy link

cypress-bot bot commented Nov 7, 2022

Thanks for taking the time to open a PR!

@mjhenkes mjhenkes changed the title Disallow same-superdomain-origin cy.origin blocks Fix: Disallow same-superdomain-origin cy.origin blocks Nov 7, 2022

const origin = location.origin

// This is set while IN the cy.origin command.
cy.state('currentActiveOrigin', origin)

return new Bluebird((resolve, reject, onCancel) => {
const cleanup = ({ readyForOriginFailed }: {readyForOriginFailed?: boolean} = {}): void => {
Copy link
Member Author

Choose a reason for hiding this comment

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

readyForOriginFailed was dead code

})
}

export function urlMatchesOriginProps (url, props) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I removed a couple of functions that were not used anywhere:

  • urlMatchesOriginProps
  • urlMatchesPolicyProps
  • urlMatchesSameSiteProps

urlsSuperDomainOriginMatch and urlMatchesSuperDomainOriginProps are replaced by urlMatchesPolicyBasedOnDomain and urlMatchesPolicyBasedOnDomainProps

@@ -39,13 +39,14 @@ module.exports = {
})
},

handleCrossOriginIframe (req, res) {
handleCrossOriginIframe (req, res, namespace) {
Copy link
Member Author

Choose a reason for hiding this comment

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

These are changes to make cypress in cypress work better (not totally fixed yet)

// TODO: The below route is not technically correct for cypress in cypress tests.
// We should be using 'config.namespace' to provide the namespace instead of hard coding __cypress, however,
// In the runner when we create the spec bridge we have no knowledge of the namespace used by the server so
// we create a spec bridge for the namespace of the server specified in the config, but that server hasn't been created.
// To fix this I think we need to find a way to listen in the cypress in cypress server for routes from the server the
// cypress instance thinks should exist, but that's outside the current scope.
Copy link
Member Author

Choose a reason for hiding this comment

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

Comment says it all

const { subdomain: _unused, ...remoteProps } = cors.parseUrlIntoHostProtocolDomainTldPort(remoteOrigin)
const remoteProps = cors.parseUrlIntoHostProtocolDomainTldPort(remoteOrigin)
Copy link
Member Author

Choose a reason for hiding this comment

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

Without this change, checking same-origin of something like www.google.com would always fail since the www was getting dropped. We need to keep subdomain.

@mjhenkes mjhenkes marked this pull request as ready for review November 7, 2022 19:08
@cypress
Copy link

cypress bot commented Nov 7, 2022



Test summary

4179 0 1095 0Flakiness 1


Run details

Project cypress
Status Passed
Commit 03d7842
Started Nov 8, 2022 4:37 PM
Ended Nov 8, 2022 4:54 PM
Duration 16:35 💡
OS Linux Debian -
Browser WebKit 16

View run in Cypress Dashboard ➡️


Flakiness

cypress/e2e/commands/xhr.cy.js Flakiness
1 src/cy/commands/xhr > abort > aborts xhrs currently in flight

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

Copy link
Contributor

@AtofStryker AtofStryker left a comment

Choose a reason for hiding this comment

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

I wonder if we need to change the messaging on whether the user provides same origin vs cross origin but is same super domain origin to help point them to the right place

packages/driver/cypress/e2e/e2e/origin/origin.cy.ts Outdated Show resolved Hide resolved
packages/driver/cypress/e2e/e2e/origin/validation.cy.ts Outdated Show resolved Hide resolved
packages/driver/src/cypress/error_messages.ts Show resolved Hide resolved
packages/network/lib/cors.ts Show resolved Hide resolved
packages/server/lib/server-e2e.ts Outdated Show resolved Hide resolved
@chrisbreiding chrisbreiding changed the title Fix: Disallow same-superdomain-origin cy.origin blocks fix: Disallow same-superdomain-origin cy.origin blocks Nov 7, 2022
@AtofStryker AtofStryker self-requested a review November 8, 2022 14:55
@mjhenkes mjhenkes merged commit 23299ac into develop Nov 9, 2022
81 of 83 checks passed
@mjhenkes mjhenkes deleted the matth/fix/disallow-same-superDomainOrigin-origin-blocks branch November 9, 2022 14:29
@@ -11,6 +11,8 @@ const debug = debugModule('cypress:network:cors')
// match IP addresses or anything following the last .
const customTldsRe = /(^[\d\.]+$|\.[^\.]+$)/

const strictSameOriginDomains = Object.freeze(['google.com'])

Choose a reason for hiding this comment

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

@mjhenkes can we allow somehow parametrizing this? we have mobile tests that are on the same domain, but they require different user-agent. we were trying to override it with cy.origin() and it worked perfectly well till this PR change...

Copy link
Contributor

Choose a reason for hiding this comment

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

@rnovosad would you be able to share an example?

Choose a reason for hiding this comment

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

@AtofStryker I provided it in the comment #2100 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

@rnovosad got it. I will reply to the thread there!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cy.origin should throw an error when block is declared within same-origin context
4 participants