-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Update error message to be more specific about same-origin policy #6118
Conversation
Thanks for the contribution! Below are some guidelines Cypress uses when doing PR reviews.
PR Review ChecklistIf any of the following requirements can't be met, leave a comment in the review selecting 'Request changes', otherwise 'Approve'. User Experience
Functionality
Maintainability
Quality
Internal
|
Test summaryRun details
View run in 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 |
# Conflicts: # packages/driver/src/cypress/error_messages.coffee # packages/server/__snapshots__/6_web_security_spec.coffee.js
Can we mention this in the error somehow? I feel like this is the most important information for the user to understand the error, and yet it is not mentioned anywhere in the error. |
Co-Authored-By: Zach Bloomquist <github@chary.us>
Co-Authored-By: Zach Bloomquist <github@chary.us>
Co-Authored-By: Zach Bloomquist <github@chary.us>
Co-Authored-By: Zach Bloomquist <github@chary.us>
Co-Authored-By: Zach Bloomquist <github@chary.us>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actual same-origin policy qualifies that two URLs have the same origin if the
protocol
,port
(if specified), andhost
are the same. So the error message is now more specific about this and will link to a better written doc also.
Can we mention this in the error somehow? I feel like this is the most important information for the user to understand the error, and yet it is not mentioned anywhere in the error.
@flotwig OK! I updated this again off of Brian's suggestion. A new specific error will show for each scenario - so different messages for if different port, protocol, or superdomain. |
Co-Authored-By: Zach Bloomquist <github@chary.us>
Co-Authored-By: Zach Bloomquist <github@chary.us>
Co-Authored-By: Zach Bloomquist <github@chary.us>
Co-Authored-By: Zach Bloomquist <github@chary.us>
Co-Authored-By: Zach Bloomquist <github@chary.us>
Co-Authored-By: Zach Bloomquist <github@chary.us>
…all differences when necessary.
|
||
You may only visit a single unique domain per test. | ||
The new URL is considered a different origin because the following parts of the URL are different: {{differences}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this could be improved to be a little more natural-sounding with 1 item, but eh, this is probably clear enough. what do you think? here's what i mean, with 1 list item it sounds like this:
The new URL is considered a different origin because the following parts of the URL are different: port
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error messages aren't great to work with as are and need the error improvements PR to make this a function which could actually be manipulated easier to write this more natural. So, this is good as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
Co-Authored-By: Zach Bloomquist <github@chary.us>
Updated the 'differences' to be indented as well as the urls for better readability off of @brian-mann suggestions. |
https
thenhttp
of same subdomain fails saying you can only visit a single unique domain per test #6048User facing changelog
The error messages displayed when rerouting to a non same-origin domain has been updated to more accurately reflect the rules around same-origin policy.
Additional details
The test below fails in Cypress (this is as designed). The problem is that the error message says you can only visit the same superdomain, so users are like....uh....these are the same superdomain.
The actual same-origin policy qualifies that two URLs have the same origin if the
protocol
,port
(if specified), andhost
are the same. So the error message is now more specific about this and will link to a better written doc also.How has the user experience changed?
Before (cross origin error)
After (cross origin error)
Before (visit error)
After (visit error)
When different protocol
When different port
When different superdomain
When all different
PR Tasks
cypress-documentation
? Update mention of 'single domain' to be more specific about 'cross-origin' cypress-documentation#2364type definitions
?cypress.schema.json
?