mirror/cypress
1
0
Fork 0
mirror of https://github.com/cypress-io/cypress.git synced 2026-05-01 12:30:01 -05:00
Code Issues Packages Projects Releases Wiki Activity

fix: Improve error handling around calls to this.next in middleware (#25702)

Browse Source
This commit is contained in:
Mike Plummer
2023-02-08 10:17:28 -06:00
committed by GitHub
parent 2644028678
commit efc195896a
10 changed files with 223 additions and 55 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.circleci/workflows.yml
+4 -4
View File
@@ -28,7 +28,7 @@ mainBuildFilters: &mainBuildFilters
only:
- develop
- /^release\/\d+\.\d+\.\d+$/
- 'emily/next-version'
- 'mikep/22825-next-middleware'
# usually we don't build Mac app - it takes a long time
# but sometimes we want to really confirm we are doing the right thing
@@ -37,7 +37,7 @@ macWorkflowFilters: &darwin-workflow-filters
when:
or:
- equal: [ develop, << pipeline.git.branch >> ]
- equal: [ 'mschile/chrome_memory_fix', << pipeline.git.branch >> ]
- equal: [ 'mikep/22825-next-middleware', << pipeline.git.branch >> ]
- matches:
pattern: /^release\/\d+\.\d+\.\d+$/
value: << pipeline.git.branch >>
@@ -46,7 +46,7 @@ linuxArm64WorkflowFilters: &linux-arm64-workflow-filters
when:
or:
- equal: [ develop, << pipeline.git.branch >> ]
- equal: [ 'mschile/chrome_memory_fix', << pipeline.git.branch >> ]
- equal: [ 'mikep/22825-next-middleware', << pipeline.git.branch >> ]
- matches:
pattern: /^release\/\d+\.\d+\.\d+$/
value: << pipeline.git.branch >>
@@ -130,7 +130,7 @@ commands:
- run:
name: Check current branch to persist artifacts
command: |
if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "emily/next-version" ]]; then
if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "mikep/22825-next-middleware" ]]; then
echo "Not uploading artifacts or posting install comment for this branch."
circleci-agent step halt
fi
cli/CHANGELOG.md
+2 -1
View File
@@ -5,7 +5,8 @@ _Released 02/14/2023 (PENDING)_
**Bugfixes:**
- Fixed an issue with the Cloud project selection modal not showing the correct prompts. Fixes [#25520](https://github.com/cypress-io/cypress/issues/25520).
- Fixed an issue with the Cloud project selection modal not showing the correct prompts. Fixes [#25520](https://github.com/cypress-io/cypress/issues/25520).
- Fixed an issue in middleware where error-handling code could itself generate an error and fail to report the original issue. Fixes [#22825](https://github.com/cypress-io/cypress/issues/22825).
**Features:**
packages/errors/src/errorUtils.ts
+8 -1
View File
@@ -50,9 +50,10 @@ type AllowedChalkColors = 'red' | 'blue' | 'green' | 'magenta' | 'yellow'
*
* @param err
* @param color
* @param causeDepth If error has a `cause` limits the maximum depth of causes to log. Set to `0` to not log any `cause`
* @returns
*/
export const logError = function (err: CypressError | ErrorLike, color: AllowedChalkColors = 'red') {
export const logError = function (err: CypressError | ErrorLike, color: AllowedChalkColors = 'red', causeDepth: number = 3) {
console.log(chalk[color](err.message))
if (err.details) {
@@ -67,5 +68,11 @@ export const logError = function (err: CypressError | ErrorLike, color: AllowedC
console.log(chalk[color](err.stack ?? ''))
if (causeDepth > 0 && err['cause']) {
// Limit the recursions on `cause` in case there is a loop
console.log(chalk[color]('Caused by:'))
logError(err['cause'], color, causeDepth - 1)
}
return err
}
packages/errors/test/unit/errors_spec.ts
+56 -7
View File
@@ -3,7 +3,7 @@ import style from 'ansi-styles'
import chai, { expect } from 'chai'
/* eslint-disable no-console */
import chalk from 'chalk'
import sinon from 'sinon'
import sinon, { SinonSpy } from 'sinon'
import * as errors from '../../src'
import { parseResolvedPattern } from '../../src/errorUtils'
@@ -72,16 +72,65 @@ describe('lib/errors', () => {
expect(console.log).to.be.calledWithMatch(chalk.magenta(userError.stack ?? ''))
})
it('logs err.stack in development', () => {
process.env.CYPRESS_INTERNAL_ENV = 'development'
describe('err.stack', () => {
it('is logged if not a known Cypress error', () => {
const err = new Error('foo')
const err = new Error('foo')
const ret = errors.log(err)
const ret = errors.log(err)
expect(ret).to.eq(err)
expect(ret).to.eq(err)
expect(console.log).to.be.calledWith(chalk.red(err.stack ?? ''))
})
expect(console.log).to.be.calledWith(chalk.red(err.stack ?? ''))
it('is not logged if a known Cypress error', () => {
const err = new Error('foo')
err['isCypressErr'] = true
const ret = errors.log(err)
expect(ret).to.be.undefined
expect(console.log).not.to.be.calledWith(chalk.red(err.stack ?? ''))
})
})
context('err.cause', () => {
let err
beforeEach(() => {
err = new Error('foo')
err['cause'] = err
})
it('is not logged if a known Cypress error', () => {
err['isCypressErr'] = true
const ret = errors.log(err)
expect(ret).to.be.undefined
expect(console.log).not.to.be.calledWith(chalk.red('Caused by:'))
})
it('is not logged if max cause depth === 0', () => {
const ret = errors.log(err, 'red', 0)
expect(ret).to.eq(ret)
expect(console.log).not.to.be.calledWith(chalk.red('Caused by:'))
})
it('is logged to a specified max depth', () => {
const ret = errors.log(err, 'red', 5)
expect(ret).to.eq(err)
const causeLogs = (console.log as SinonSpy).getCalls().filter((call) => call.args[0] === chalk.red('Caused by:'))
expect(causeLogs).to.have.length(5)
})
})
})
packages/proxy/lib/http/index.ts
+13 -3
View File
@@ -23,6 +23,7 @@ import type { RemoteStates } from '@packages/server/lib/remote_states'
import type { CookieJar } from '@packages/server/lib/util/cookies'
import type { RequestedWithAndCredentialManager } from '@packages/server/lib/util/requestedWithAndCredentialManager'
import type { AutomationCookie } from '@packages/server/lib/automation/cookies'
import { errorUtils } from '@packages/errors'
function getRandomColorFn () {
return chalk.hex(`#${Number(
@@ -70,7 +71,7 @@ export const defaultMiddleware = {
export type ServerCtx = Readonly<{
config: CyServer.Config & Cypress.Config
shouldCorrelatePreRequests?: () => boolean
getFileServerToken: () => string
getFileServerToken: () => string | undefined
getCookieJar: () => CookieJar
remoteStates: RemoteStates
requestedWithAndCredentialManager: RequestedWithAndCredentialManager
@@ -182,7 +183,14 @@ export function _runStage (type: HttpStages, ctx: any, onError: Function) {
const fullCtx = {
next: () => {
fullCtx.next = () => {
throw new Error('Error running proxy middleware: Cannot call this.next() more than once in the same middleware function. Doing so can cause unintended issues.')
const error = new Error('Error running proxy middleware: Detected `this.next()` was called more than once in the same middleware function, but a middleware can only be completed once.')
if (ctx.error) {
error.message = error.message += '\nThis middleware invocation previously encountered an error which may be related, see `error.cause`'
error['cause'] = ctx.error
}
throw error
}
copyChangedCtx()
@@ -207,6 +215,8 @@ export function _runStage (type: HttpStages, ctx: any, onError: Function) {
try {
middleware.call(fullCtx)
} catch (err) {
err.message = `Internal error while proxying "${ctx.req.method} ${ctx.req.proxiedUrl}" in ${middlewareName}:\n${err.message}`
errorUtils.logError(err)
fullCtx.onError(err)
}
})
@@ -230,7 +240,7 @@ export class Http {
config: CyServer.Config
shouldCorrelatePreRequests: () => boolean
deferredSourceMapCache: DeferredSourceMapCache
getFileServerToken: () => string
getFileServerToken: () => string | undefined
remoteStates: RemoteStates
middleware: HttpMiddlewareStacks
netStubbingState: NetStubbingState
packages/proxy/test/unit/http/index.spec.ts
+6 -6
View File
@@ -63,13 +63,13 @@ describe('http', function () {
incomingRequest.throws(new Error('oops'))
error.callsFake(function () {
expect(this.error.message).to.eq('oops')
expect(this.error.message).to.eq('Internal error while proxying "GET url" in 0:\noops')
this.end()
})
return new Http(httpOpts)
// @ts-ignore
.handle({}, { on, off })
.handle({ method: 'GET', proxiedUrl: 'url' }, { on, off })
.then(function () {
expect(incomingRequest).to.be.calledOnce
expect(incomingResponse).to.not.be.called
@@ -88,13 +88,13 @@ describe('http', function () {
incomingResponse.throws(new Error('oops'))
error.callsFake(function () {
expect(this.error.message).to.eq('oops')
expect(this.error.message).to.eq('Internal error while proxying "GET url" in 0:\noops')
this.end()
})
return new Http(httpOpts)
// @ts-ignore
.handle({}, { on, off })
.handle({ method: 'GET', proxiedUrl: 'url' }, { on, off })
.then(function () {
expect(incomingRequest).to.be.calledOnce
expect(incomingResponse).to.be.calledOnce
@@ -141,7 +141,7 @@ describe('http', function () {
})
error.callsFake(function () {
expect(this.error.message).to.eq('goto error stack')
expect(this.error.message).to.eq('Internal error while proxying "GET url" in 1:\ngoto error stack')
expect(this).to.include.keys(expectedKeys)
this.errorAdded = errorAdded
this.next()
@@ -159,7 +159,7 @@ describe('http', function () {
return new Http(httpOpts)
// @ts-ignore
.handle({}, { on, off })
.handle({ method: 'GET', proxiedUrl: 'url' }, { on, off })
.then(function () {
[
incomingRequest, incomingRequest2,
packages/proxy/test/unit/http/request-middleware.spec.ts
+66
View File
@@ -770,4 +770,70 @@ describe('http/request-middleware', () => {
})
})
})
describe('SendRequestOutgoing', () => {
const { SendRequestOutgoing } = RequestMiddleware
let ctx
beforeEach(() => {
const headers = {}
const remoteStates = new RemoteStates(() => {})
ctx = {
onError: sinon.stub(),
request: {
create: (opts) => {
return {
inputArgs: opts,
on: (event, callback) => {
if (event === 'response') {
callback()
}
},
}
},
},
req: {
body: '{}',
headers,
socket: {
on: () => {},
},
},
res: {
on: (event, listener) => {},
off: (event, listener) => {},
} as Partial<CypressOutgoingResponse>,
remoteStates,
}
})
context('same-origin file request', () => {
beforeEach(() => {
ctx.getFileServerToken = () => 'abcd1234'
ctx.req.proxiedUrl = 'https://www.cypress.io/file'
ctx.remoteStates.set({
origin: 'https://www.cypress.io',
strategy: 'file',
} as any)
})
it('adds `x-cypress-authorization` header', async () => {
await testMiddleware([SendRequestOutgoing], ctx)
.then(() => {
expect(ctx.req.headers['x-cypress-authorization']).to.equal('abcd1234')
})
})
it('handles nil fileServer token', async () => {
ctx.getFileServerToken = () => undefined
await testMiddleware([SendRequestOutgoing], ctx)
.then(() => {
expect(ctx.req.headers['x-cypress-authorization']).to.be.undefined
})
})
})
})
})
packages/proxy/test/unit/http/response-middleware.spec.ts
+59 -30
View File
@@ -31,41 +31,70 @@ describe('http/response-middleware', function () {
])
})
it('errors if this.next() is called more than once in the same middleware', function (done) {
const middleware = function () {
this.next()
this.next()
}
describe('multiple this.next invocations', () => {
context('within the same middleware', () => {
it('throws an error', function (done) {
const middleware = function () {
this.next()
this.next()
}
testMiddleware([middleware], {
res: {
on: (event, listener) => {},
off: (event, listener) => {},
},
onError (err) {
expect(err.message).to.equal('Error running proxy middleware: Cannot call this.next() more than once in the same middleware function. Doing so can cause unintended issues.')
testMiddleware([middleware], {
res: {
on: (event, listener) => {},
off: (event, listener) => {},
},
onError (err) {
expect(err.message).to.equal('Internal error while proxying "undefined undefined" in 0:\nError running proxy middleware: Detected `this.next()` was called more than once in the same middleware function, but a middleware can only be completed once.')
done()
},
done()
},
})
})
it('includes a previous context error in error message if one exists', (done) => {
const middleware = function () {
this.next()
this.next()
}
const error = new Error('previous error')
testMiddleware([middleware], {
error,
res: {
on: (event, listener) => {},
off: (event, listener) => {},
},
onError (err) {
expect(err.message).to.contain('This middleware invocation previously encountered an error which may be related, see `error.cause`')
expect(err['cause']).to.equal(error)
done()
},
method: 'GET',
proxiedUrl: 'url',
})
})
})
})
it('does not error if this.next() is called more than once in different middleware', function () {
const middleware1 = function () {
this.next()
}
const middleware2 = function () {
this.next()
}
context('across different middleware', () => {
it('does not throw an error', function () {
const middleware1 = function () {
this.next()
}
const middleware2 = function () {
this.next()
}
return testMiddleware([middleware1, middleware2], {
res: {
on: (event, listener) => {},
off: (event, listener) => {},
},
onError () {
throw new Error('onError should not be called')
},
return testMiddleware([middleware1, middleware2], {
res: {
on: (event, listener) => {},
off: (event, listener) => {},
},
onError () {
throw new Error('onError should not be called')
},
})
})
})
})
packages/server/lib/server-base.ts
+8 -2
View File
@@ -95,6 +95,12 @@ const notSSE = (req, res) => {
export type WarningErr = Record<string, any>
type FileServer = {
token: string
port: () => number
close: () => void
}
export interface OpenServerOptions {
SocketCtor: typeof SocketE2E | typeof SocketCt
testingType: Cypress.TestingType
@@ -112,7 +118,7 @@ export abstract class ServerBase<TSocket extends SocketE2E | SocketCt> {
protected isListening: boolean
protected socketAllowed: SocketAllowed
protected requestedWithAndCredentialManager: RequestedWithAndCredentialManager
protected _fileServer
protected _fileServer: FileServer | null
protected _baseUrl: string | null
protected _server?: DestroyableHttpServer
protected _socket?: TSocket
@@ -320,7 +326,7 @@ export abstract class ServerBase<TSocket extends SocketE2E | SocketCt> {
createNetworkProxy ({ config, remoteStates, requestedWithAndCredentialManager, shouldCorrelatePreRequests }) {
const getFileServerToken = () => {
return this._fileServer.token
return this._fileServer?.token
}
this._netStubbingState = netStubbingState()
packages/server/lib/server-e2e.ts
+1 -1
View File
@@ -221,7 +221,7 @@ export class ServerE2E extends ServerBase<SocketE2E> {
if (!fullyQualifiedRe.test(urlStr)) {
handlingLocalFile = true
options.headers['x-cypress-authorization'] = this._fileServer.token
options.headers['x-cypress-authorization'] = this._fileServer?.token
const state = this._remoteStates.set(urlStr, options)