fix: allow for cookies on request of same key to take precedence over…

… cookies in the jar, regardless of how many hierachy cookies exist in the jar
cypress-io · Feb 14, 2023 · 17de188 · 17de188
1 parent 331dd69
commit 17de188
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 29 deletions.
diff --git a/packages/proxy/lib/http/util/cookies.ts b/packages/proxy/lib/http/util/cookies.ts
@@ -97,17 +97,20 @@ export const addCookieJarCookiesToRequest = (applicableCookieJarCookies: Cookie[
 
   // Always have cookies in the jar overwrite cookies in the request if they are the same
   requestCookies.forEach((cookie) => cookieMap.set(cookie.key, cookie))
+
   // Two or more cookies on the same request can happen per https://www.rfc-editor.org/rfc/rfc6265
   // But if a value for that cookie already exists in the cookie jar, do NOT add the cookie jar cookie
+  const cookiesToAddFromJar: Cookie[] = []
+
   applicableCookieJarCookies.forEach((cookie) => {
-    if (cookieMap.get(cookie.key)) {
-      cookieMap.delete(cookie.key)
+    if (!cookieMap.get(cookie.key)) {
+      cookiesToAddFromJar.push(cookie)
     }
   })
 
   const requestCookiesThatNeedToBeAdded = Array.from(cookieMap).map(([key, cookie]) => cookie)
 
-  return applicableCookieJarCookies.concat(requestCookiesThatNeedToBeAdded).map((cookie) => cookie.cookieString()).join('; ')
+  return cookiesToAddFromJar.concat(requestCookiesThatNeedToBeAdded).map((cookie) => cookie.cookieString()).join('; ')
 }
 
 // sameSiteContext is a concept for tough-cookie's cookie jar that helps it

diff --git a/packages/proxy/test/unit/http/request-middleware.spec.ts b/packages/proxy/test/unit/http/request-middleware.spec.ts
@@ -469,43 +469,50 @@ describe('http/request-middleware', () => {
      * These tests are to leverage small integration tests between us and tough-cookie to make sure we are adding cookies correctly to the Cookie header given the above circumstances
      */
       describe('duplicate cookies', () => {
-        describe('does not add request cookie to request if cookie exists in jar, and preserves duplicate cookies when same key/value if', () => {
-          describe('subdomain and TLD', () => {
-            it('matches hierarchy', async () => {
-              const ctx = await getContext(['jar=cookie', 'request=cookie'], ['jar=cookie1; Domain=app.foobar.com', 'jar=cookie2; Domain=foobar.com', 'jar=cookie3; Domain=exclude.foobar.com'], 'http://app.foobar.com/generic', 'http://app.foobar.com/generic')
+        // we want to take the request cookie over anything that might exist in the jar if the key is the same, as the cookie in the jar may be stale at this point
+        it('always adds request cookie over cookie jar cookie if one exists', async () => {
+          const ctx = await getContext(['jar=cookie', 'request=cookie'], ['jar=cookie1; Domain=app.foobar.com', 'jar=cookie2; Domain=foobar.com', 'jar=cookie3; Domain=exclude.foobar.com'], 'http://app.foobar.com/generic', 'http://app.foobar.com/generic')
 
-              await testMiddleware([MaybeAttachCrossOriginCookies], ctx)
+          await testMiddleware([MaybeAttachCrossOriginCookies], ctx)
+
+          expect(ctx.req.headers['cookie']).to.equal('jar=cookie; request=cookie')
+        })
 
-              expect(ctx.req.headers['cookie']).to.equal('jar=cookie1; jar=cookie2; request=cookie')
-            })
+        describe('if no cookie exists on request, cookie jar cookies preserves duplicate cookies when same key/value if subdomain and TLD', () => {
+          it('matches hierarchy', async () => {
+            const ctx = await getContext(['request=cookie'], ['jar=cookie1; Domain=app.foobar.com', 'jar=cookie2; Domain=foobar.com', 'jar=cookie3; Domain=exclude.foobar.com'], 'http://app.foobar.com/generic', 'http://app.foobar.com/generic')
 
-            it('matches hierarchy and gives order to the cookie that was created first', async () => {
-              const ctx = await getContext(['jar=cookie', 'request=cookie'], ['jar=cookie1; Domain=app.foobar.com;', 'jar=cookie2; Domain=.foobar.com;'], 'http://app.foobar.com/generic', 'http://app.foobar.com/generic')
+            await testMiddleware([MaybeAttachCrossOriginCookies], ctx)
 
-              const cookies = ctx.getCookieJar().getCookies('http://app.foobar.com/generic', 'strict')
+            expect(ctx.req.headers['cookie']).to.equal('jar=cookie1; jar=cookie2; request=cookie')
+          })
 
-              const TLDCookie = cookies.find((cookie) => cookie.domain === 'foobar.com')
+          it('matches hierarchy and gives order to the cookie that was created first', async () => {
+            const ctx = await getContext(['request=cookie'], ['jar=cookie1; Domain=app.foobar.com;', 'jar=cookie2; Domain=.foobar.com;'], 'http://app.foobar.com/generic', 'http://app.foobar.com/generic')
 
-              // make the TLD cookie created an hour earlier
-              TLDCookie?.creation?.setHours(TLDCookie?.creation?.getHours() - 1)
-              await testMiddleware([MaybeAttachCrossOriginCookies], ctx)
+            const cookies = ctx.getCookieJar().getCookies('http://app.foobar.com/generic', 'strict')
 
-              expect(ctx.req.headers['cookie']).to.equal('jar=cookie2; jar=cookie1; request=cookie')
-            })
+            const TLDCookie = cookies.find((cookie) => cookie.domain === 'foobar.com')
+
+            // make the TLD cookie created an hour earlier
+            TLDCookie?.creation?.setHours(TLDCookie?.creation?.getHours() - 1)
+            await testMiddleware([MaybeAttachCrossOriginCookies], ctx)
+
+            expect(ctx.req.headers['cookie']).to.equal('jar=cookie2; jar=cookie1; request=cookie')
+          })
 
-            it('matches hierarchy and gives order to the cookie with the most specific path, regardless of creation time', async () => {
-              const ctx = await getContext(['jar=cookie', 'request=cookie'], ['jar=cookie1; Domain=app.foobar.com; Path=/generic', 'jar=cookie2; Domain=.foobar.com;'], 'http://app.foobar.com/generic', 'http://app.foobar.com/generic')
+          it('matches hierarchy and gives order to the cookie with the most specific path, regardless of creation time', async () => {
+            const ctx = await getContext(['request=cookie'], ['jar=cookie1; Domain=app.foobar.com; Path=/generic', 'jar=cookie2; Domain=.foobar.com;'], 'http://app.foobar.com/generic', 'http://app.foobar.com/generic')
 
-              const cookies = ctx.getCookieJar().getCookies('http://app.foobar.com/generic', 'strict')
+            const cookies = ctx.getCookieJar().getCookies('http://app.foobar.com/generic', 'strict')
 
-              const TLDCookie = cookies.find((cookie) => cookie.domain === 'foobar.com')
+            const TLDCookie = cookies.find((cookie) => cookie.domain === 'foobar.com')
 
-              // make the TLD cookie created an hour earlier
-              TLDCookie?.creation?.setHours(TLDCookie?.creation?.getHours() - 1)
-              await testMiddleware([MaybeAttachCrossOriginCookies], ctx)
+            // make the TLD cookie created an hour earlier
+            TLDCookie?.creation?.setHours(TLDCookie?.creation?.getHours() - 1)
+            await testMiddleware([MaybeAttachCrossOriginCookies], ctx)
 
-              expect(ctx.req.headers['cookie']).to.equal('jar=cookie1; jar=cookie2; request=cookie')
-            })
+            expect(ctx.req.headers['cookie']).to.equal('jar=cookie1; jar=cookie2; request=cookie')
           })
         })
 

diff --git a/packages/proxy/test/unit/http/util/cookies.spec.ts b/packages/proxy/test/unit/http/util/cookies.spec.ts
@@ -1,5 +1,6 @@
 import { expect } from 'chai'
-import { calculateSiteContext, getSameSiteContext, shouldAttachAndSetCookies } from '../../../../lib/http/util/cookies'
+import { calculateSiteContext, getSameSiteContext, shouldAttachAndSetCookies, addCookieJarCookiesToRequest } from '../../../../lib/http/util/cookies'
+import { Cookie } from '@packages/server/lib/util/cookies'
 
 context('getSameSiteContext', () => {
   describe('calculates the same site context correctly for', () => {
@@ -263,3 +264,22 @@ context('.calculateSiteContext', () => {
     expect(calculateSiteContext(autUrl, 'https://staging.google2.com')).to.equal('cross-site')
   })
 })
+
+context('.addCookieJarCookiesToRequest', () => {
+  it('takes the request cookie over the cookie jar cookie if multiple cookies of the same key exist', () => {
+    const cookieJarCookies = [
+      new Cookie({
+        key: 'session_id',
+        value: 'bar',
+      }),
+      new Cookie({
+        key: 'csrf_token',
+        value: 'bar',
+      }),
+    ]
+    const requestCookieString = ['session_id=foo', 'csrf_token=foo', '__cypress.initial=true']
+    const calculatedRequestCookies = addCookieJarCookiesToRequest(cookieJarCookies, requestCookieString)
+
+    expect(calculatedRequestCookies).to.equal('session_id=foo; csrf_token=foo; __cypress.initial=true')
+  })
+})