-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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: improve server side cookie handling via more accurate top simulation #23728
Conversation
Thanks for taking the time to open a PR!
|
} | ||
|
||
// cross site cookies cannot set lax/strict cookies in the browser for xhr/fetch requests (but ok with navigation/document requests) | ||
if (this.request.resourceType && this.siteContext === 'cross-site' && toughCookie.sameSite !== 'none') { |
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.
I need to update this logic to include the AUTFrame on the request to allow Lax
cookies, and throw out Strict cookies all together if the request is cross-site (I believe tough-cookie handles this case, but not the first)
6a72a23
to
2ce9a4d
Compare
b7e6d48
to
cdbdf21
Compare
going to work on splitting this draft into several smaller PRs linked to each other. I also think it's a good idea to wait on any origin method naming until #23297 lands in |
@@ -144,7 +144,7 @@ const onBeforeAppWindowLoad = (Cypress: Cypress.Cypress, cy: $Cy) => (autWindow: | |||
const onWindowLoadPrimary = ({ url }) => { | |||
cy.isStable(true, 'primary onload') | |||
|
|||
cy.state('autOrigin', cors.getOriginPolicy(url)) | |||
cy.state('autOrigin', cors.getParentOriginPolicy(url)) |
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.
should be autParentOrigin
here
(currently failing)
…ite functions, and enhancing the parsedHost fn
…quest/response middleware
…n if the resourceType is unknown
…thod does not include subdomain, which is a parent of origin policy, as well as add documentation as to why we need this. we know leverage true origin policy in cy.origin, and only leverage superDOmainOriginPolicy for document.domain related items
… super domain origin policy
cdbdf21
to
cb8c26d
Compare
This PR is ultimately going to be closed for the feature branch PR on branch . I will continue to keep this draft up for a wholistic view of the changes, but will close once the feature branch PR is ready for review. |
closing in favor of #23872 |
User facing changelog
More accurately sends/sets cookies in
cy.origin
whenexperimentalSessionAndOrigin
is enabled.Additional details
We have been gradually making improvements to cookie handling in
cy.origin
via #22320 #22594 #22963 #23438 and #23643For full details on why this is needed, please see #23551. This also seems to fix some of the inconsistencies in request length we were seeing in browsers, as well as seems to reduce recaptcha's in third party authentication providers almost entirely
This PR implements the ideas in #23551, via
fetch
andxmlHttpRequest
in the spec bridge and AUT window whency.origin
is active. The patches forfetch
andxmlHttpRequest
capture the absolute URL of the request being made, and if valid, are sent to the Cypress server over websocket to store the credential information of the request, as well as the resource type. Right now, this sends the absolute url to the websocket, which is hashed and stored in a Map. A map was used on the backend as it is easier to clear the contents out without losing the reference (as opposed to an object literal and iterating through the keys and deleting one by one) when weclear
/reset
.X-Cypress-Request
Header to the web extension and electron through the webRequest API (similar toX-Cypress-AUT-is-AUT-Frame
) to add the resource type is a request. This value is set to'true'
as the extension/electron cannot discern the difference between anxhr
vsfetch
request. However, CDP is able to discern the difference, and sendsX-Cypress-Request
with a value ofxhr
orfetch
instead of'true'
. IfX-Cypress-Request
's value isxhr
orfetch
instead of'true'
, this resource type takes precedence over whatever was sent over the socket as it is more reliable and accurate to the request itself.xhr
given theX-Cypress-Request
header is present.GET
,POST
, etc) to make this map more accurate to prevent unwanted collisions. (TODO: add tests for exercising the queue functionality).cy.origin
.experimentalSessionAndOrigin
flag isn't set, and/or top does NOT need to be simulated, we don't attempt any additional logic on handling cookies as they should be sent/set in the browser normally without any issue.experimentalSessionAndOrigin
flag is set AND top needs to be simulated, we then attempt to simulate how attaching/setting cookies would be having requests/responses respectively if the AUT url were top.xhr/fetch
request, we take into account the withCredentials and credentials options on whether or not to attach cookies. If we should attach cookies, we then determine the sameSiteContext for which cookies to apply. This is a bit confusing asstrict
,lax
, andnone
cookies are all sent withsame-site
context assuming the request is configured correctly. To illustrate, I have added the following tables inXhr and Fetch credentials examples
. There are several unit tests available to verify this behavior. If a request is notxhr/fetch
, we attach cookies given the site context as there are no additional configuration options needed (which is much simpler 😄 ).Set-Cookie
header regardless of the top simulation/experimental flag. However, if the experimental flag is set and top needs to be simulated, we pass cookies through to our cookie jar and use the same logic as we do for attaching cookies (see bullet point above). The only additional things we need to check for is theSameSite
attribute.cross-site
requests cannot set Lax Cookies unless it is an AUTFrame request (a top level navigation event in the AUT frame), and cannot set Strict cookies all together. IfSameSite=none
is used, the cookie must beSecure
Xhr and Fetch credentials examples
Given the above requests, lets say we have the following cookies set in our browser and top is on origin
http://app.testme.com:3500/
Given the following request configurations, these are the cookies the would be applied
TODOS
document.domain
to prevent unecessary reloads of the AUTSteps to test
How has the user experience changed?
PR Tasks
cypress-documentation
?type definitions
?