fix: Improve error handling around calls to this.next in middleware

.circleci/workflows.yml

@@ -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

@@ -7,6 +7,10 @@ _Released 02/14/2023 (PENDING)_
 
 - Added the "Open in IDE" feature for failed tests reported from the Debug page. Addressed in [#25691](https://github.com/cypress-io/cypress/pull/25691).
 
+**Bugfixes:**
+
+- 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).
+
 **Misc:**
 
 - Improved the layout of the Debug Page on smaller viewports when there is a pending run. Addresses [#25664](https://github.com/cypress-io/cypress/issues/25664).

packages/proxy/lib/http/index.ts

@@ -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,13 @@ 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. This can cause unintended issues and may indicate an error in your middleware logic or configuration.')
+
+            if (ctx.error) {
+              error.message = error.message += `\nThis middleware invocation previously encountered an error which may be related: ${ctx.error}`
+            }
+
+            throw error
           }
 
           copyChangedCtx()
@@ -207,6 +214,7 @@ export function _runStage (type: HttpStages, ctx: any, onError: Function) {
       try {
         middleware.call(fullCtx)
       } catch (err) {
+        errorUtils.logError(err)
         fullCtx.onError(err)
       }
     })
@@ -230,7 +238,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/request-middleware.spec.ts

@@ -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

@@ -31,41 +31,67 @@ 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: Detected `this.next()` was called more than once in the same middleware function. This can cause unintended issues and may indicate an error in your middleware logic or configuration.')
 
-    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.')
+            done()
+          },
+        })
+      })
 
-        done()
-      },
+      it('includes a previous context error in error message if one exists', (done) => {
+        const middleware = function () {
+          this.next()
+          this.next()
+        }
+
+        testMiddleware([middleware], {
+          error: new Error('previous 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: Error: previous error')
+
+            done()
+          },
+        })
+      })
     })
-  })
 
-  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

@@ -95,6 +95,12 @@ const notSSE = (req, res) => {
 
 export type WarningErr = Record<string, any>
 
+export 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

@@ -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)