fix: only attach cookies if coming from the AUTFrame or is same-origi…

…n if the resourceType is unknown

packages/driver/cypress/e2e/e2e/origin/cookie_behavior.cy.ts

@@ -490,7 +490,7 @@ describe('Cookie Behavior with experimentalSessionAndOrigin=true', () => {
             // this should have the same effect as same-origin option for same-site/cross-origin requests, but adding here incase our implementation is not consistent
             it('does not set or send same-site cookies if "omit" credentials option is specified', () => {
               cy.intercept(`${scheme}://app.foobar.com:${crossOriginPort}/test-request-credentials`, (req) => {
-                // expect(req['headers']['cookie']).to.equal(undefined)
+                expect(req['headers']['cookie']).to.equal(undefined)
                 req.reply({
                   statusCode: 200,
                 })

packages/proxy/lib/http/request-middleware.ts

@@ -77,7 +77,7 @@ const MaybeAttachCrossOriginCookies: RequestMiddleware = function () {
 
   // Top needs to be simulated since the AUT is in a cross origin state. Get the requestedWith and credentials and see what cookies need to be attached
   const currentAUTUrl = this.getAUTUrl()
-  const shouldCookiesBeAttachedToRequest = shouldAttachAndSetCookies(this.req.proxiedUrl, currentAUTUrl, this.req.requestedWith, this.req.credentialsLevel)
+  const shouldCookiesBeAttachedToRequest = shouldAttachAndSetCookies(this.req.proxiedUrl, currentAUTUrl, this.req.requestedWith, this.req.credentialsLevel, this.req.isAUTFrame)
 
   this.debug(`should cookies be attached to request?: ${shouldCookiesBeAttachedToRequest}`)
   if (!shouldCookiesBeAttachedToRequest) {

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

@@ -14,7 +14,7 @@ interface RequestDetails {
   credentialLevel?: RequestCredentialLevel
 }
 
-export const shouldAttachAndSetCookies = (requestUrl: string, AUTUrl: string | undefined, resourceType?: RequestResourceType, credentialLevel?: RequestCredentialLevel): 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)
@@ -43,8 +43,12 @@ export const shouldAttachAndSetCookies = (requestUrl: string, AUTUrl: string | u
 
       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
   }
 }
 
@@ -174,17 +178,19 @@ export class CookiesHelper {
       return
     }
 
-    // don't set the cookie in our own cookie jar if the cookie would otherwise fail being set in the browser if the AUT Url
-    // was actually top. This prevents cookies from being applied to our cookie jar when they shouldn't, preventing possible security implications.
-    if (!shouldAttachAndSetCookies(this.request.url, this.currentAUTUrl, this.request.resourceType, this.request.credentialLevel)) {
-      this.debug(`not setting cookie for ${this.request.url} with simulated top ${ this.currentAUTUrl} for ${ this.request.resourceType}:${this.request.credentialLevel}, cookie: ${toughCookie}`)
+    // 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') {
+      this.debug(`cannot set cookie with SameSite=${toughCookie.sameSite} when site context is ${this.siteContext}`)
 
       return
     }
 
-    // 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') {
-      this.debug(`cannot set cookie with SameSite=${toughCookie.sameSite} when site context is ${this.siteContext}`)
+    // don't set the cookie in our own cookie jar if the cookie would otherwise fail being set in the browser if the AUT Url
+    // was actually top. This prevents cookies from being applied to our cookie jar when they shouldn't, preventing possible security implications.
+    const shouldSetCookieGivenSiteContext = shouldAttachAndSetCookies(this.request.url, this.currentAUTUrl, this.request.resourceType, this.request.credentialLevel, this.request.isAUTFrame)
+
+    if (!shouldSetCookieGivenSiteContext) {
+      this.debug(`not setting cookie for ${this.request.url} with simulated top ${ this.currentAUTUrl} for ${ this.request.resourceType}:${this.request.credentialLevel}, cookie: ${toughCookie}`)
 
       return
     }

packages/proxy/test/unit/http/request-middleware.spec.ts

@@ -349,6 +349,28 @@ describe('http/request-middleware', () => {
       expect(ctx.req.headers['cookie']).to.equal('request=cookie')
     })
 
+    it(`allows setting cookies on request if resource type cannot be determined, but comes from the AUT frame (likely in the case of documents or redirects)`, async function () {
+      const ctx = await getContext([], ['jar=cookie'], 'http://foobar.com/index.html', 'http://app.foobar.com/index.html')
+
+      ctx.req.requestedWith = undefined
+      ctx.req.credentialsLevel = undefined
+      ctx.req.isAUTFrame = true
+      await testMiddleware([MaybeAttachCrossOriginCookies], ctx)
+
+      expect(ctx.req.headers['cookie']).to.equal('jar=cookie')
+    })
+
+    it(`otherwise, does not allow setting cookies if request type cannot be determined and is not from the AUT and is cross-origin`, async function () {
+      const ctx = await getContext([], ['jar=cookie'], 'http://foobar.com/index.html', 'http://app.foobar.com/index.html')
+
+      ctx.req.requestedWith = undefined
+      ctx.req.credentialsLevel = undefined
+      ctx.req.isAUTFrame = false
+      await testMiddleware([MaybeAttachCrossOriginCookies], ctx)
+
+      expect(ctx.req.headers['cookie']).to.be.undefined
+    })
+
     it('sets the cookie header to undefined if no cookies exist on the request, none in the jar, but cookies should be attached', async () => {
       const ctx = await getContext([], [], 'http://foobar.com', 'http://app.foobar.com')
 
@@ -507,8 +529,7 @@ describe('http/request-middleware', () => {
           proxiedUrl: requestUrl,
           isAUTFrame: true,
           headers: {
-
-            cookie: requestCookieStrings.join('; '),
+            cookie: requestCookieStrings.join('; ') || undefined,
           },
         },
       } as HttpMiddlewareThis<any>

packages/proxy/test/unit/http/response-middleware.spec.ts

@@ -1257,7 +1257,7 @@ describe('http/response-middleware', function () {
       expect(appendStub).to.be.calledWith('Set-Cookie', 'cookie3=value3; SameSite=None')
     })
 
-    it(`allows setting cookies if request type cannot be determined (likely in the case of documents or redirects)`, async function () {
+    it(`allows setting cookies if request type cannot be determined, but comes from the AUT frame (likely in the case of documents or redirects)`, async function () {
       const appendStub = sinon.stub()
 
       const cookieJar = {
@@ -1271,6 +1271,7 @@ describe('http/response-middleware', function () {
           append: appendStub,
         },
         req: {
+          isAUTFrame: true,
           proxiedUrl: 'https://www.barbaz.com/index.html',
         },
         incomingRes: {
@@ -1290,7 +1291,42 @@ describe('http/response-middleware', function () {
         key: 'cookie',
         value: 'value',
         sameSite: 'lax',
-      }), 'https://www.barbaz.com/index.html', 'none')
+      }), 'https://www.barbaz.com/index.html', 'lax')
+
+      // send to browser anyway even though these will likely fail to be set
+      expect(appendStub).to.be.calledWith('Set-Cookie', 'cookie=value')
+    })
+
+    it(`otherwise, does not allow setting cookies if request type cannot be determined and is not from the AUT and is cross-origin`, async function () {
+      const appendStub = sinon.stub()
+
+      const cookieJar = {
+        getAllCookies: () => [{ key: 'cookie', value: 'value' }],
+        setCookie: sinon.stub(),
+      }
+
+      const ctx = prepareContext({
+        cookieJar,
+        res: {
+          append: appendStub,
+        },
+        req: {
+          proxiedUrl: 'https://www.barbaz.com/some-image.png',
+        },
+        incomingRes: {
+          headers: {
+            'set-cookie': ['cookie=value'],
+          },
+        },
+      })
+
+      // a case where top would need to be simulated
+      ctx.getAUTUrl = () => 'https://www.foobar.com/index.html'
+      ctx.remoteStates.isPrimaryOrigin = () => false
+
+      await testMiddleware([MaybeCopyCookiesFromIncomingRes], ctx)
+
+      expect(cookieJar.setCookie).not.to.have.been.called
 
       // send to browser anyway even though these will likely fail to be set
       expect(appendStub).to.be.calledWith('Set-Cookie', 'cookie=value')

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

@@ -231,9 +231,19 @@ 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('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
     })
 
     it('return false if the AUT url is undefined', () => {