From 72831c9a35da6f5e5a5caab204cf43bb11a1c639 Mon Sep 17 00:00:00 2001 From: Bill Glesias Date: Fri, 9 Sep 2022 13:56:22 -0400 Subject: [PATCH] fix: only attach cookies if coming from the AUTFrame or is same-origin if the resourceType is unknown --- .../e2e/e2e/origin/cookie_behavior.cy.ts | 2 +- packages/proxy/lib/http/request-middleware.ts | 2 +- packages/proxy/lib/http/util/cookies.ts | 26 +++++++----- .../test/unit/http/request-middleware.spec.ts | 25 +++++++++++- .../unit/http/response-middleware.spec.ts | 40 ++++++++++++++++++- .../proxy/test/unit/http/util/cookies.spec.ts | 14 ++++++- 6 files changed, 91 insertions(+), 18 deletions(-) diff --git a/packages/driver/cypress/e2e/e2e/origin/cookie_behavior.cy.ts b/packages/driver/cypress/e2e/e2e/origin/cookie_behavior.cy.ts index d15ec2d3fada..d0e6c81554c7 100644 --- a/packages/driver/cypress/e2e/e2e/origin/cookie_behavior.cy.ts +++ b/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, }) diff --git a/packages/proxy/lib/http/request-middleware.ts b/packages/proxy/lib/http/request-middleware.ts index bd5c713f985c..b4f4f2050fee 100644 --- a/packages/proxy/lib/http/request-middleware.ts +++ b/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) { diff --git a/packages/proxy/lib/http/util/cookies.ts b/packages/proxy/lib/http/util/cookies.ts index e7e9a23e4154..af3c9216d857 100644 --- a/packages/proxy/lib/http/util/cookies.ts +++ b/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 } diff --git a/packages/proxy/test/unit/http/request-middleware.spec.ts b/packages/proxy/test/unit/http/request-middleware.spec.ts index 998236a2f954..650ffd5fde56 100644 --- a/packages/proxy/test/unit/http/request-middleware.spec.ts +++ b/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 diff --git a/packages/proxy/test/unit/http/response-middleware.spec.ts b/packages/proxy/test/unit/http/response-middleware.spec.ts index 140cbafdef32..792da42a576c 100644 --- a/packages/proxy/test/unit/http/response-middleware.spec.ts +++ b/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') diff --git a/packages/proxy/test/unit/http/util/cookies.spec.ts b/packages/proxy/test/unit/http/util/cookies.spec.ts index 698448aacfc2..40d26fbfcb92 100644 --- a/packages/proxy/test/unit/http/util/cookies.spec.ts +++ b/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', () => {