Skip to content

Commit

Permalink
chore: simulated cookie fixes 1 (#24060)
Browse files Browse the repository at this point in the history
* chore: add documentation to CDP,electron, and web extension for selected resource types

* chore: change nomenclature of X-Cypress-Request to X-Cypress-Is-XHR-Or-Fetch

* chore: remove no longer applicable comment for socket code

* chore: add comments to the resourceType/credential manager
  • Loading branch information
AtofStryker authored Oct 3, 2022
1 parent 18a3c9a commit a41b104
Show file tree
Hide file tree
Showing 10 changed files with 55 additions and 34 deletions.
9 changes: 8 additions & 1 deletion packages/extension/app/background.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,15 @@ const connect = function (host, path, extraOpts) {
const requestModifications = {
requestHeaders: [
...(details.requestHeaders || []),
/**
* Unlike CDP, the web extensions onBeforeSendHeaders resourceType cannot discern the difference
* between fetch or xhr resource types, but classifies both as 'xmlhttprequest'. Because of this,
* we set X-Cypress-Is-XHR-Or-Fetch to true if the request is made with 'xhr' or 'fetch' so the
* middleware doesn't incorrectly assume which request type is being sent
* @see https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/webRequest/ResourceType
*/
...(details.type === 'xmlhttprequest' ? [{
name: 'X-Cypress-Request',
name: 'X-Cypress-Is-XHR-Or-Fetch',
value: 'true',
}] : []),
],
Expand Down
8 changes: 4 additions & 4 deletions packages/extension/test/integration/background_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,7 @@ describe('app/background', () => {
})
})

it('appends X-Cypress-Request header to request if the resourceType is "xmlhttprequest"', async function () {
it('appends X-Cypress-Is-XHR-Or-Fetch header to request if the resourceType is "xmlhttprequest"', async function () {
const details = {
parentFrameId: 0,
type: 'xmlhttprequest',
Expand All @@ -395,14 +395,14 @@ describe('app/background', () => {
value: 'Bar',
},
{
name: 'X-Cypress-Request',
name: 'X-Cypress-Is-XHR-Or-Fetch',
value: 'true',
},
],
})
})

it('does not append X-Cypress-Request header to request if the resourceType is not an "xmlhttprequest"', async function () {
it('does not append X-Cypress-Is-XHR-Or-Fetch header to request if the resourceType is not an "xmlhttprequest"', async function () {
const details = {
parentFrameId: 0,
type: 'sub_frame',
Expand All @@ -420,7 +420,7 @@ describe('app/background', () => {
expect(result).to.not.deep.equal({
requestHeaders: [
{
name: 'X-Cypress-Request',
name: 'X-Cypress-Is-XHR-Or-Fetch',
value: 'true',
},
],
Expand Down
13 changes: 7 additions & 6 deletions packages/proxy/lib/http/request-middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,21 +24,22 @@ const LogRequest: RequestMiddleware = function () {

const ExtractCypressMetadataHeaders: RequestMiddleware = function () {
this.req.isAUTFrame = !!this.req.headers['x-cypress-is-aut-frame']
const requestIsXhrOrFetch = this.req.headers['x-cypress-request']
const requestIsXhrOrFetch = this.req.headers['x-cypress-is-xhr-or-fetch']

if (this.req.headers['x-cypress-is-aut-frame']) {
delete this.req.headers['x-cypress-is-aut-frame']
}

if (this.req.headers['x-cypress-request']) {
this.debug(`found x-cypress-request header. Deleting x-cypress-request header.`)
delete this.req.headers['x-cypress-request']
if (this.req.headers['x-cypress-is-xhr-or-fetch']) {
this.debug(`found x-cypress-is-xhr-or-fetch header. Deleting x-cypress-is-xhr-or-fetch header.`)
delete this.req.headers['x-cypress-is-xhr-or-fetch']
}

if (!this.config.experimentalSessionAndOrigin ||
!doesTopNeedToBeSimulated(this) ||
// this should be unreachable, as the x-cypress-request header is only attached if the resource type is 'xhr'
// inside the extension or electron equivalent. This is only needed for defensive purposes.
// this should be unreachable, as the x-cypress-is-xhr-or-fetch header is only attached if
// the resource type is 'xhr' or 'fetch or 'true' (in the case of electron|extension).
// This is only needed for defensive purposes.
(requestIsXhrOrFetch !== 'true' && requestIsXhrOrFetch !== 'xhr' && requestIsXhrOrFetch !== 'fetch')) {
this.next()

Expand Down
24 changes: 12 additions & 12 deletions packages/proxy/test/unit/http/request-middleware.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,22 +61,22 @@ describe('http/request-middleware', () => {
})
})

it('removes x-cypress-request header when it exists', async () => {
it('removes x-cypress-is-xhr-or-fetch header when it exists', async () => {
const ctx = {
req: {
headers: {
'x-cypress-request': 'true',
'x-cypress-is-xhr-or-fetch': 'true',
},
} as Partial<CypressIncomingRequest>,
}

await testMiddleware([ExtractCypressMetadataHeaders], ctx)
.then(() => {
expect(ctx.req.headers['x-cypress-request']).not.to.exist
expect(ctx.req.headers['x-cypress-is-xhr-or-fetch']).not.to.exist
})
})

it('removes x-cypress-request header when it does not exist', async () => {
it('removes x-cypress-is-xhr-or-fetch header when it does not exist', async () => {
const ctx = {
req: {
headers: {},
Expand All @@ -85,7 +85,7 @@ describe('http/request-middleware', () => {

await testMiddleware([ExtractCypressMetadataHeaders], ctx)
.then(() => {
expect(ctx.req.headers['x-cypress-request']).not.to.exist
expect(ctx.req.headers['x-cypress-is-xhr-or-fetch']).not.to.exist
})
})

Expand All @@ -96,7 +96,7 @@ describe('http/request-middleware', () => {
},
req: {
headers: {
'x-cypress-request': 'true',
'x-cypress-is-xhr-or-fetch': 'true',
},
} as Partial<CypressIncomingRequest>,
}
Expand All @@ -116,7 +116,7 @@ describe('http/request-middleware', () => {
getAUTUrl: sinon.stub().returns(undefined),
req: {
headers: {
'x-cypress-request': 'true',
'x-cypress-is-xhr-or-fetch': 'true',
},
} as Partial<CypressIncomingRequest>,
}
Expand All @@ -128,7 +128,7 @@ describe('http/request-middleware', () => {
})
})

it('does not set requestedWith or credentialLevel on the request if x-cypress-request has invalid values', async () => {
it('does not set requestedWith or credentialLevel on the request if x-cypress-is-xhr-or-fetch has invalid values', async () => {
const ctx = {
config: {
experimentalSessionAndOrigin: true,
Expand All @@ -139,7 +139,7 @@ describe('http/request-middleware', () => {
},
req: {
headers: {
'x-cypress-request': 'sub_frame',
'x-cypress-is-xhr-or-fetch': 'sub_frame',
},
} as Partial<CypressIncomingRequest>,
}
Expand Down Expand Up @@ -167,7 +167,7 @@ describe('http/request-middleware', () => {
req: {
proxiedUrl: 'http://localhost:8080',
headers: {
'x-cypress-request': 'xhr',
'x-cypress-is-xhr-or-fetch': 'xhr',
},
} as Partial<CypressIncomingRequest>,
}
Expand All @@ -194,7 +194,7 @@ describe('http/request-middleware', () => {
req: {
proxiedUrl: 'http://localhost:8080',
headers: {
'x-cypress-request': 'fetch',
'x-cypress-is-xhr-or-fetch': 'fetch',
},
} as Partial<CypressIncomingRequest>,
}
Expand Down Expand Up @@ -223,7 +223,7 @@ describe('http/request-middleware', () => {
req: {
proxiedUrl: 'http://localhost:8080',
headers: {
'x-cypress-request': 'true',
'x-cypress-is-xhr-or-fetch': 'true',
},
} as Partial<CypressIncomingRequest>,
}
Expand Down
11 changes: 9 additions & 2 deletions packages/server/lib/browsers/chrome.ts
Original file line number Diff line number Diff line change
Expand Up @@ -408,10 +408,17 @@ const _handlePausedRequests = async (client) => {
value: string
}[] = []

/**
* Unlike the the web extension or Electrons's onBeforeSendHeaders, CDP can discern the difference
* between fetch or xhr resource types. Because of this, we set X-Cypress-Is-XHR-Or-Fetch to either
* 'xhr' or 'fetch' with CDP so the middleware can assume correct defaults in case credential/resourceTypes
* are not sent to the server.
* @see https://chromedevtools.github.io/devtools-protocol/tot/Network/#type-ResourceType
*/
if (params.resourceType === 'XHR' || params.resourceType === 'Fetch') {
debug('add X-Cypress-Request header to: %s', params.request.url)
debug('add X-Cypress-Is-XHR-Or-Fetch header to: %s', params.request.url)
addedHeaders.push({
name: 'X-Cypress-Request',
name: 'X-Cypress-Is-XHR-Or-Fetch',
value: params.resourceType.toLowerCase(),
})
}
Expand Down
9 changes: 8 additions & 1 deletion packages/server/lib/browsers/electron.ts
Original file line number Diff line number Diff line change
Expand Up @@ -387,8 +387,15 @@ export = {
const requestModifications = {
requestHeaders: {
...details.requestHeaders,
/**
* Unlike CDP, Electrons's onBeforeSendHeaders resourceType cannot discern the difference
* between fetch or xhr resource types, but classifies both as 'xhr'. Because of this,
* we set X-Cypress-Is-XHR-Or-Fetch to true if the request is made with 'xhr' or 'fetch' so the
* middleware doesn't incorrectly assume which request type is being sent
* @see https://www.electronjs.org/docs/latest/api/web-request#webrequestonbeforesendheadersfilter-listener
*/
...(details.resourceType === 'xhr') ? {
'X-Cypress-Request': 'true',
'X-Cypress-Is-XHR-Or-Fetch': 'true',
} : {},
},
}
Expand Down
1 change: 0 additions & 1 deletion packages/server/lib/socket-base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,6 @@ export class SocketBase {
case 'cross:origin:automation:cookies:received':
return this.localBus.emit('cross:origin:automation:cookies:received')
case 'request:sent:with:credentials':
// NOTE: this is currently a no-op until the server logic is implemented
return this.localBus.emit('request:sent:with:credentials', args[0])
default:
throw new Error(`You requested a backend event we cannot handle: ${eventName}`)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,5 +84,5 @@ class ResourceTypeAndCredentialManagerClass {
// export as a singleton
export const resourceTypeAndCredentialManager = new ResourceTypeAndCredentialManagerClass()

// export but only as a type
// export but only as a type. We do NOT want others to create instances of the class as it is intended to be a singleton
export type ResourceTypeAndCredentialManager = ResourceTypeAndCredentialManagerClass
8 changes: 4 additions & 4 deletions packages/server/test/unit/browsers/chrome_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,7 @@ describe('lib/browsers/chrome', () => {
})
})

it('appends X-Cypress-Request header to fetch request', async function () {
it('appends X-Cypress-Is-XHR-Or-Fetch header to fetch request', async function () {
await chrome.open('chrome', 'http://', withExperimentalFlagOn, this.automation)

this.pageCriClient.on.withArgs('Page.frameAttached').yield()
Expand All @@ -489,14 +489,14 @@ describe('lib/browsers/chrome', () => {
value: 'Bar',
},
{
name: 'X-Cypress-Request',
name: 'X-Cypress-Is-XHR-Or-Fetch',
value: 'fetch',
},
],
})
})

it('appends X-Cypress-Request header to xhr request', async function () {
it('appends X-Cypress-Is-XHR-Or-Fetch header to xhr request', async function () {
await chrome.open('chrome', 'http://', withExperimentalFlagOn, this.automation)

this.pageCriClient.on.withArgs('Page.frameAttached').yield()
Expand All @@ -521,7 +521,7 @@ describe('lib/browsers/chrome', () => {
value: 'Bar',
},
{
name: 'X-Cypress-Request',
name: 'X-Cypress-Is-XHR-Or-Fetch',
value: 'xhr',
},
],
Expand Down
4 changes: 2 additions & 2 deletions packages/server/test/unit/browsers/electron_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ describe('lib/browsers/electron', () => {
})
})

it('adds X-Cypress-Request header if xhr request (includes fetch)', function () {
it('adds X-Cypress-Is-XHR-Or-Fetch header if xhr request (includes fetch)', function () {
sinon.stub(this.win.webContents.session.webRequest, 'onBeforeSendHeaders')

return electron._launch(this.win, this.url, this.automation, this.options)
Expand All @@ -511,7 +511,7 @@ describe('lib/browsers/electron', () => {
expect(cb).to.be.calledWith({
requestHeaders: {
'X-Foo': 'Bar',
'X-Cypress-Request': 'true',
'X-Cypress-Is-XHR-Or-Fetch': 'true',
},
})
})
Expand Down

0 comments on commit a41b104

Please sign in to comment.