fix: add isAUTIframe check inside the shouldAttachAndSetCookies, move…

… the siteContext info to the cookies package, simplify top-simulation util, and add better method documentation

packages/proxy/lib/http/util/cookies.ts

@@ -2,17 +2,32 @@ import _ from 'lodash'
 import type Debug from 'debug'
 import { URL } from 'url'
 import { cors } from '@packages/network'
+import { urlOriginsMatch, urlSameSiteMatch } from '@packages/network/lib/cors'
 import { AutomationCookie, Cookie, CookieJar, toughCookieToAutomationCookie } from '@packages/server/lib/util/cookies'
-import { calculateSiteContext } from './top-simulation'
 import type { RequestCredentialLevel, RequestResourceType } from '../../types'
 
+type SiteContext = 'same-origin' | 'same-site' | 'cross-site'
+
 interface RequestDetails {
   url: string
   isAUTFrame: boolean
   needsCrossOriginHandling: boolean
 }
 
-export const shouldAttachAndSetCookies = (requestUrl: string, AUTUrl: string | undefined, resourceType?: RequestResourceType, credentialLevel?: RequestCredentialLevel): boolean => {
+/**
+ * Determines whether or not a request should attach cookies from the tough-cookie jar or whether a response should set cookies inside the tough-cookie jar.
+ * same-origin requests send cookies by default (unless 'omit' is specified by the fetch API). Otherwise, for same-site/cross-site requests, credentials either need to
+ * be 'include' via the fetch API or 'true for XmlHttpRequest. If the AUT Iframe is making the request, this is likely a navigation and we should attach/set cookies in the browser,
+ * which is critical for lax cookies
+ * @param {string} requestUrl - the url of the request
+ * @param {string} AUTUrl - The current url of the app under test
+ * @param {RequestResourceType} [resourceType] -
+ * @param {RequestCredentialLevel} [credentialLevel] - The credentialLevel of the request. For `fetch` this is `omit|same-origin|include` (defaults to same-origin)
+ * and for `XmlHttpRequest` it is `true|false` (defaults to false)
+ * @param {isAutFrame} [boolean] - whether or not the request is from the AUT Iframe or not
+ * @returns {boolean}
+ */
+export const shouldAttachAndSetCookies = (requestUrl: string, AUTUrl: string | undefined, resourceType?: RequestResourceType, credentialLevel?: RequestCredentialLevel, isAutFrame?: boolean): boolean => {
   if (!AUTUrl) return false
 
   const siteContext = calculateSiteContext(requestUrl, AUTUrl)
@@ -24,28 +39,56 @@ export const shouldAttachAndSetCookies = (requestUrl: string, AUTUrl: string | u
         return false
       }
 
-      // if the siteContext is same-origin and the credential status is undefined, same-origin, or include, attach cookies.
-      // Otherwise, if the credentials are set to include, attach cookies
+      // attach cookies here if at least one of the following is true
+      // a) The siteContext is 'same-origin' (since 'omit' is handled above)
+      // b) The credentialLevel is 'include' regardless of siteContext
       if (siteContext === 'same-origin' || credentialLevel === 'include') {
         return true
       }
 
       return false
     case 'xhr':
-      // if context is same-origin regardless of credential status, attach cookies
-      // otherwise, if withCredentials is set to true, attach cookies
-
+      // attach cookies here if at least one of the following is true
+      // a) The siteContext is 'same-origin'
+      // b) The credentialLevel (withCredentials) is set to true
       if (siteContext === 'same-origin' || credentialLevel) {
         return true
       }
 
       return false
     default:
-      // if we cannot determine a resource level, we likely should store the cookie as it is a navigation or another event
-      return true
+      // if we cannot determine a resource level, we likely should store the cookie as it is a navigation or another event as long as the context is same-origin
+      if (siteContext === 'same-origin' || isAutFrame) {
+        return true
+      }
+
+      return false
   }
 }
 
+/**
+ * Calculates the site context of two urls.
+ * This is needed to figure out if cookies are sent by default, as well as if first/third-party cookies apply to a given request
+ * @see https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#third-party_cookies
+ *
+ * @param {string} url1 - the first url being compared
+ * @param {string} url2 - the second url being compared
+ * @returns {SiteContext} - the appropriate site context. This is most similar to the Sec-Fetch-Site Directive when
+ * calculating the siteContext barring the none option. @see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Sec-Fetch-Site#directives
+ * to see definitions for same-origin, same-site, and cross-site
+ */
+export const calculateSiteContext = (url1: string, url2: string): SiteContext => {
+  if (urlOriginsMatch(url1, url2)) {
+    return 'same-origin'
+  }
+
+  if (urlSameSiteMatch(url1, url2)) {
+    return 'same-site'
+  }
+
+  return 'cross-site'
+}
+
 export const addCookieJarCookiesToRequest = (applicableCookieJarCookies: Cookie[] = [], requestCookieStringArray: string[] = []): string => {
   const cookieMap = new Map<string, Cookie>()
   const requestCookies: Cookie[] = requestCookieStringArray.map((cookie) => CookieJar.parse(cookie)).filter((cookie) => cookie !== undefined) as Cookie[]

packages/proxy/lib/http/util/top-simulation.ts

@@ -1,6 +1,4 @@
-import { urlOriginsMatch, urlSameSiteMatch } from '@packages/network/lib/cors'
 import type { HttpMiddlewareThis } from '../index'
-import type { SecFetchSite } from '../../types'
 
 export const doesTopNeedToBeSimulated = <T>(ctx: HttpMiddlewareThis<T>): boolean => {
   const currentAUTUrl = ctx.getAUTUrl()
@@ -12,20 +10,5 @@ export const doesTopNeedToBeSimulated = <T>(ctx: HttpMiddlewareThis<T>): boolean
 
   // only simulate top if the AUT is NOT the primary origin, meaning that we should treat the AUT as top
   // or the request is the AUT frame, which is common for redirects and navigations.
-  const doesTopNeedToSimulating = !ctx.remoteStates.isPrimaryOrigin(currentAUTUrl) || ctx.req.isAUTFrame
-
-  return doesTopNeedToSimulating
-}
-
-// @see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Sec-Fetch-Site
-export const calculateSiteContext = (url1: string, url2: string, isUserInteraction = false): SecFetchSite => {
-  if (urlOriginsMatch(url1, url2)) {
-    return 'same-origin'
-  }
-
-  if (urlSameSiteMatch(url1, url2)) {
-    return 'same-site'
-  }
-
-  return isUserInteraction ? 'none' : 'cross-site'
+  return !ctx.remoteStates.isPrimaryOrigin(currentAUTUrl) || ctx.req.isAUTFrame
 }

packages/proxy/lib/types.ts

@@ -21,8 +21,6 @@ export type RequestCredentialLevel = 'same-origin' | 'include' | 'omit' | boolea
 
 export type CypressWantsInjection = 'full' | 'fullCrossOrigin' | 'partial' | false
 
-export type SecFetchSite = 'same-origin' | 'same-site' | 'cross-site' | 'none'
-
 /**
  * An outgoing response to an incoming request to the Cypress web server.
  */

packages/proxy/test/unit/http/util/cookies.spec.ts

@@ -1,5 +1,5 @@
 import { expect } from 'chai'
-import { getSameSiteContext, shouldAttachAndSetCookies } from '../../../../lib/http/util/cookies'
+import { calculateSiteContext, getSameSiteContext, shouldAttachAndSetCookies } from '../../../../lib/http/util/cookies'
 
 context('getSameSiteContext', () => {
   describe('calculates the same site context correctly for', () => {
@@ -231,13 +231,35 @@ context('shouldAttachAndSetCookies', () => {
   })
 
   context('misc', () => {
-    it('returns true if the resource type is unknown (could be a navigation request to set top level cookies', () => {
+    it('returns true if the resource type is unknown, but the request comes from the aut frame (could be a navigation request to set top level cookies)', () => {
       // possibly a navigation request for a document or another resource. If this is the case, attach cookies based on the siteContext and cookies should be attached regardless
-      expect(shouldAttachAndSetCookies('http://www.foobar.com:3500/index.html', autUrl)).to.be.true
+      expect(shouldAttachAndSetCookies('http://www.foobar.com:3500/index.html', autUrl, undefined, undefined, true)).to.be.true
     })
 
-    it('return false if the AUT url is undefined', () => {
-      expect(shouldAttachAndSetCookies('http://www.foobar.com:3500/index.html', undefined)).to.be.false
+    it('returns true if the resource type is unknown, but the request is same-origin', () => {
+      // possibly a navigation request for a document or another resource. If this is the case, attach cookies based on the siteContext and cookies should be attached regardless
+      expect(shouldAttachAndSetCookies('http://www.foobar.com:3500/index.html', 'http://www.foobar.com:3500/index.html')).to.be.true
+    })
+
+    it('returns false if the resource type is unknown and the request does NOT come from the AUTFrame', () => {
+      // possibly a navigation request for a document or another resource. If this is the case, attach cookies based on the siteContext and cookies should be attached regardless
+      expect(shouldAttachAndSetCookies('http://www.foobar.com:3500/index.html', autUrl)).to.be.false
     })
   })
 })
+
+context('.calculateSiteContext', () => {
+  const autUrl = 'https://staging.google.com'
+
+  it('calculates same-origin correctly for same-origin / same-site urls', () => {
+    expect(calculateSiteContext(autUrl, 'https://staging.google.com')).to.equal('same-origin')
+  })
+
+  it('calculates same-site correctly for cross-origin / same-site urls', () => {
+    expect(calculateSiteContext(autUrl, 'https://app.google.com')).to.equal('same-site')
+  })
+
+  it('calculates cross-site correctly for cross-origin / cross-site urls', () => {
+    expect(calculateSiteContext(autUrl, 'https://staging.google2.com')).to.equal('cross-site')
+  })
+})

packages/proxy/test/unit/http/util/top-simulation.spec.ts

@@ -1,7 +1,7 @@
 import { expect } from 'chai'
 import sinon from 'sinon'
 import { HttpMiddlewareThis } from '../../../../lib/http'
-import { calculateSiteContext, doesTopNeedToBeSimulated } from '../../../../lib/http/util/top-simulation'
+import { doesTopNeedToBeSimulated } from '../../../../lib/http/util/top-simulation'
 
 context('.doesTopNeedToBeSimulated', () => {
   const autUrl = 'http://localhost:8080'
@@ -59,23 +59,3 @@ context('.doesTopNeedToBeSimulated', () => {
     expect(doesTopNeedToBeSimulated(mockCtx)).to.be.true
   })
 })
-
-context('.calculateSiteContext', () => {
-  const autUrl = 'https://staging.google.com'
-
-  it('calculates same-origin correctly for same-origin / same-site urls', () => {
-    expect(calculateSiteContext(autUrl, 'https://staging.google.com')).to.equal('same-origin')
-  })
-
-  it('calculates same-site correctly for cross-origin / same-site urls', () => {
-    expect(calculateSiteContext(autUrl, 'https://app.google.com')).to.equal('same-site')
-  })
-
-  it('calculates cross-site correctly for cross-origin / cross-site urls', () => {
-    expect(calculateSiteContext(autUrl, 'https://staging.google2.com')).to.equal('cross-site')
-  })
-
-  it('returns "none" if the interaction is triggered by the user, regardless of other properties', () => {
-    expect(calculateSiteContext(autUrl, 'https://staging.google2.com', true)).to.equal('none')
-  })
-})