feat: execute all steps and collect errors at end of chain

src/proxy/chain.ts

@@ -19,12 +19,12 @@ import { Request, Response } from 'express';
 import { PluginLoader } from '../plugin';
 import { Action, RequestType, PushType } from './actions';
 import * as proc from './processors';
-import { Processor } from './processors/types';
+import { ProcessorExec } from './processors/types';
 import { attemptAutoApproval, attemptAutoRejection } from './actions/autoActions';
 import { handleErrorAndLog } from '../utils/errors';
 import { createProgressWriter } from './sideband';
 
-const branchPushChain: Processor['exec'][] = [
+const branchPushChain: ProcessorExec[] = [
   proc.push.checkEmptyBranch,
   proc.push.checkRepoInAuthorisedList,
   proc.push.checkMessages,
@@ -41,7 +41,7 @@ const branchPushChain: Processor['exec'][] = [
   proc.push.blockForAuth,
 ];
 
-const tagPushChain: Processor['exec'][] = [
+const tagPushChain: ProcessorExec[] = [
   proc.push.checkRepoInAuthorisedList,
   proc.push.checkUserPushPermission,
   proc.push.checkIfWaitingAuth,
@@ -52,12 +52,34 @@ const tagPushChain: Processor['exec'][] = [
   proc.push.blockForAuth,
 ];
 
-const pullActionChain: Processor['exec'][] = [proc.push.checkRepoInAuthorisedList];
+const pullActionChain: ProcessorExec[] = [proc.push.checkRepoInAuthorisedList];
 
-const defaultActionChain: Processor['exec'][] = [proc.push.checkRepoInAuthorisedList];
+const defaultActionChain: ProcessorExec[] = [proc.push.checkRepoInAuthorisedList];
 
 let pluginsInserted = false;
 
+/**
+ * Compose a single error message from all failed steps, so that the git
+ * client displays every rejection reason for the push.
+ * @param {Action} action The action whose failed steps are reported.
+ * @return {string | undefined} The combined message, or undefined when there
+ * are fewer than two failed steps (the single step message is kept as-is).
+ */
+const composeErrorMessage = (action: Action): string | undefined => {
+  const messages = (action.steps ?? [])
+    .filter((step) => step.error && step.errorMessage)
+    .map((step) => (step.errorMessage as string).trim());
+
+  if (messages.length < 2) {
+    return undefined;
+  }
+
+  return (
+    `The following ${messages.length} checks failed:\n\n` +
+    messages.map((message, i) => `${i + 1}. ${message}`).join('\n\n')
+  );
+};
+
 const stepProgressLabels: Record<string, string> = {
   'checkEmptyBranch.exec': 'Checking for empty branch',
   'checkRepoInAuthorisedList.exec': 'Checking repository is authorised',
@@ -77,11 +99,11 @@ const stepProgressLabels: Record<string, string> = {
 
 /**
  * Obtain the message to display before a chain step.
- * @param {Processor['exec']} fn The chain step about to be executed.
+ * @param {ProcessorExec} fn The chain step about to be executed.
  * @return {string} The message to display.
  */
-const getProgressMessage = (fn: Processor['exec']): string => {
-  const displayName = (fn as { displayName?: string }).displayName;
+const getProgressMessage = (fn: ProcessorExec): string => {
+  const { displayName } = fn;
   if (displayName && stepProgressLabels[displayName]) {
     return stepProgressLabels[displayName];
   }
@@ -105,22 +127,57 @@ export const executeChain = async (req: Request, res: Response): Promise<Action>
     // 3) Select the correct chain now that action.actionType is set
     const actionFns = await getChain(action);
 
+    let collectedErrors = false;
     const progress = createProgressWriter(res, action);
 
     // 4) Execute each step in the selected chain
     for (const fn of actionFns) {
-      if (progress.active && action.continue()) {
-        progress.message(`${getProgressMessage(fn)}...`);
+      // a push that already failed checks must not be queued for approval
+      if (fn === proc.push.blockForAuth && !action.continue()) {
+        break;
       }
+
+      progress.message(`${getProgressMessage(fn)}...`);
+
+      const stepsBefore = action.steps?.length ?? 0;
       action = await fn(req, action);
-      if (!action.continue() || action.allowPush) {
+
+      if (action.allowPush) {
         break;
-      } else if (fn === proc.push.pullRemote) {
+      }
+
+      if (!action.continue()) {
+        if (action.blocked) {
+          break;
+        }
+
+        const failedNow = (action.steps ?? []).slice(stepsBefore).some((step) => step.error);
+        if (failedNow) {
+          // collectible steps have their failures can report all their
+          // rejection reasons at once, non-collectible steps fail immediately
+          if (!fn.isCollectible) {
+            break;
+          }
+          collectedErrors = true;
+        } else if (!collectedErrors) {
+          // error that predates the chain (e.g. produced while parsing the push)
+          break;
+        }
+      }
+
+      if (fn === proc.push.pullRemote) {
         //if the pull was successful then record the fact we need to clean it up again
         // pullRemote should cleanup unsuccessful clones itself
         checkoutCleanUpRequired = true;
       }
     }
+
+    if (collectedErrors) {
+      const combinedMessage = composeErrorMessage(action);
+      if (combinedMessage) {
+        action.errorMessage = combinedMessage;
+      }
+    }
   } catch (error: unknown) {
     const msg = handleErrorAndLog(error, 'An unexpected error occurred when executing the chain');
     action.error = true;
@@ -133,7 +190,8 @@ export const executeChain = async (req: Request, res: Response): Promise<Action>
 
     action = await proc.post.audit(req, action);
 
-    if (action.autoApproved) {
+    // a push that failed a later check must not be auto-approved
+    if (action.autoApproved && !action.error) {
       await attemptAutoApproval(action);
     } else if (action.autoRejected) {
       await attemptAutoRejection(action);
@@ -149,7 +207,7 @@ export const executeChain = async (req: Request, res: Response): Promise<Action>
  */
 let chainPluginLoader: PluginLoader;
 
-export const getChain = async (action: Action): Promise<Processor['exec'][]> => {
+export const getChain = async (action: Action): Promise<ProcessorExec[]> => {
   if (chainPluginLoader === undefined) {
     console.error(
       'Plugin loader was not initialized! This is an application error. Please report it to the GitProxy maintainers. Skipping plugins...',

src/proxy/processors/push-action/checkAuthorEmails.ts

@@ -73,5 +73,6 @@ const exec = async (_req: Request, action: Action): Promise<Action> => {
 };
 
 exec.displayName = 'checkAuthorEmails.exec';
+exec.isCollectible = true;
 
 export { exec };

src/proxy/processors/push-action/checkMessages.ts

@@ -92,5 +92,6 @@ const exec = async (_req: Request, action: Action): Promise<Action> => {
 };
 
 exec.displayName = 'checkMessages.exec';
+exec.isCollectible = true;
 
 export { exec };

src/proxy/processors/push-action/gitleaks.ts

@@ -197,5 +197,6 @@ const exec = async (_req: Request, action: Action): Promise<Action> => {
 };
 
 exec.displayName = 'gitleaks.exec';
+exec.isCollectible = true;
 
 export { exec };

src/proxy/processors/push-action/preReceive.ts

@@ -98,5 +98,6 @@ const exec = async (
 };
 
 exec.displayName = 'executeExternalPreReceiveHook.exec';
+exec.isCollectible = true;
 
 export { exec };

src/proxy/processors/push-action/scanDiff.ts

@@ -205,5 +205,6 @@ const exec = async (_req: Request, action: Action): Promise<Action> => {
 };
 
 exec.displayName = 'scanDiff.exec';
+exec.isCollectible = true;
 
 export { exec };

src/proxy/processors/types.ts

@@ -19,8 +19,20 @@ import { Request } from 'express';
 import { Question } from '../../config/generated/config';
 import { Action } from '../actions';
 
+export interface ProcessorExec {
+  (req: Request, action: Action): Promise<Action>;
+  /** Used for progress and step reporting (e.g. 'checkMessages.exec'). */
+  readonly displayName?: string;
+  /**
+   * Failures in collectible steps are recoverable by the user. Failures are
+   * recorded and all rejection reasons are reported at the end of the chain.
+   * When false or unset, a failure stops the chain immediately.
+   */
+  readonly isCollectible?: boolean;
+}
+
 export interface Processor {
-  exec(req: Request, action: Action): Promise<Action>;
+  exec: ProcessorExec;
   metadata: ProcessorMetadata;
 }
 

test/chain.test.ts

@@ -27,23 +27,26 @@ const mockLoader = {
   ],
 };
 
+const collectibleFn = () => Object.assign(vi.fn(), { isCollectible: true });
+const nonCollectibleFn = () => Object.assign(vi.fn(), { isCollectible: false });
+
 const initMockPushProcessors = () => {
   return {
-    checkEmptyBranch: vi.fn(),
-    checkRepoInAuthorisedList: vi.fn(),
-    checkMessages: vi.fn(),
-    checkAuthorEmails: vi.fn(),
-    checkUserPushPermission: vi.fn(),
-    checkIfWaitingAuth: vi.fn(),
-    checkHiddenCommits: vi.fn(),
-    pullRemote: vi.fn(),
-    writePack: vi.fn(),
-    preReceive: vi.fn(),
-    getDiff: vi.fn(),
-    gitleaks: vi.fn(),
-    clearBareClone: vi.fn(),
-    scanDiff: vi.fn(),
-    blockForAuth: vi.fn(),
+    checkEmptyBranch: nonCollectibleFn(),
+    checkRepoInAuthorisedList: nonCollectibleFn(),
+    checkMessages: collectibleFn(),
+    checkAuthorEmails: collectibleFn(),
+    checkUserPushPermission: nonCollectibleFn(),
+    checkIfWaitingAuth: nonCollectibleFn(),
+    checkHiddenCommits: nonCollectibleFn(),
+    pullRemote: nonCollectibleFn(),
+    writePack: nonCollectibleFn(),
+    preReceive: collectibleFn(),
+    getDiff: nonCollectibleFn(),
+    gitleaks: collectibleFn(),
+    clearBareClone: nonCollectibleFn(),
+    scanDiff: collectibleFn(),
+    blockForAuth: nonCollectibleFn(),
   };
 };
 
@@ -454,6 +457,96 @@ describe('proxy chain', function () {
     expect(consoleErrorSpy).toHaveBeenCalledWith('Error during auto-rejection: Database error');
   });
 
+  describe('error collection', () => {
+    // simulates a real processor failing
+    const failStep = (message: string) => async (req: any, action: any) => {
+      action.steps = [...(action.steps ?? []), { error: true, errorMessage: message }];
+      action.error = true;
+      action.continue = () => false;
+      return action;
+    };
+
+    const setupPushAction = () => {
+      const action = {
+        type: 'push',
+        steps: [],
+        continue: () => true,
+        allowPush: false,
+      };
+      mockPreProcessors.parseAction.mockResolvedValue(action);
+      mockPreProcessors.parsePush.mockResolvedValue(action);
+      return action;
+    };
+
+    it('should continue past recoverable failures and run the remaining checks', async () => {
+      setupPushAction();
+      mockPushProcessors.checkMessages.mockImplementation(failStep('bad commit message'));
+
+      const result = await chain.executeChain({});
+
+      // all later steps still ran
+      expect(mockPushProcessors.checkAuthorEmails).toHaveBeenCalled();
+      expect(mockPushProcessors.checkUserPushPermission).toHaveBeenCalled();
+      expect(mockPushProcessors.pullRemote).toHaveBeenCalled();
+      expect(mockPushProcessors.writePack).toHaveBeenCalled();
+      expect(mockPushProcessors.getDiff).toHaveBeenCalled();
+      expect(mockPushProcessors.gitleaks).toHaveBeenCalled();
+      expect(mockPushProcessors.scanDiff).toHaveBeenCalled();
+
+      // but a failing push is never queued for approval
+      expect(mockPushProcessors.blockForAuth).not.toHaveBeenCalled();
+      expect(result.error).toBe(true);
+    });
+
+    it('should report every collected failure in a single combined message', async () => {
+      setupPushAction();
+      mockPushProcessors.checkMessages.mockImplementation(failStep('bad commit message'));
+      mockPushProcessors.scanDiff.mockImplementation(failStep('secret detected in diff'));
+
+      const result = await chain.executeChain({});
+
+      expect(result.errorMessage).toContain('The following 2 checks failed:');
+      expect(result.errorMessage).toContain('bad commit message');
+      expect(result.errorMessage).toContain('secret detected in diff');
+    });
+
+    it('should keep the original message when only one collectible step fails', async () => {
+      setupPushAction();
+      mockPushProcessors.checkAuthorEmails.mockImplementation(failStep('illegal author email'));
+
+      const result = await chain.executeChain({});
+
+      expect(result.errorMessage).toBeUndefined(); // addStep is mocked so chain must not overwrite
+      expect(result.steps).toHaveLength(1);
+      expect(result.steps[0].errorMessage).toBe('illegal author email');
+    });
+
+    it('should still stop immediately when a fatal step fails', async () => {
+      setupPushAction();
+      mockPushProcessors.checkUserPushPermission.mockImplementation(failStep('no push permission'));
+
+      await chain.executeChain({});
+
+      expect(mockPushProcessors.pullRemote).not.toHaveBeenCalled();
+      expect(mockPushProcessors.scanDiff).not.toHaveBeenCalled();
+      expect(mockPushProcessors.blockForAuth).not.toHaveBeenCalled();
+    });
+
+    it('should not auto-approve a push that collected failures', async () => {
+      setupPushAction();
+      mockPushProcessors.checkMessages.mockImplementation(failStep('bad commit message'));
+      mockPushProcessors.preReceive.mockImplementation(async (req: any, action: any) => {
+        action.autoApproved = true;
+        return action;
+      });
+      const dbSpy = vi.spyOn(db, 'authorise');
+
+      await chain.executeChain({});
+
+      expect(dbSpy).not.toHaveBeenCalled();
+    });
+  });
+
   it('returns pullActionChain for pull actions', async () => {
     const action = new Action(
       '1',