From e8a75afe1baeb00a679b8b1abd24f9161c0270b2 Mon Sep 17 00:00:00 2001 From: Kris West Date: Fri, 6 Feb 2026 14:05:16 +0000 Subject: [PATCH 1/4] fix: clean-up the entire remote folder on startup --- src/proxy/index.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/proxy/index.ts b/src/proxy/index.ts index fcb77ec9f..9f24e284f 100644 --- a/src/proxy/index.ts +++ b/src/proxy/index.ts @@ -43,6 +43,16 @@ export class Proxy { constructor() {} private async proxyPreparations() { + // Clean-up the .remote folder in case anything was in-progress when we shut down + const remoteDir = './.remote'; + if (fs.existsSync(remoteDir)) { + console.log('Cleaning up the existing .remote dir...'); + // Recursively remove the contents of ./.remote and ignore exceptions + fs.rmSync(remoteDir, { recursive: true, force: true, retryDelay: 100 }); + } + console.log('Creating the .remote dir...'); + fs.mkdirSync(remoteDir); + const plugins = getPlugins(); const pluginLoader = new PluginLoader(plugins); await pluginLoader.load(); From 9a422dadc11bb60984b617fa2a66e4c9616b02ab Mon Sep 17 00:00:00 2001 From: Kris West Date: Fri, 6 Feb 2026 14:06:03 +0000 Subject: [PATCH 2/4] fix: clean up remote folders synchronously and specifically --- src/proxy/actions/Action.ts | 3 +- src/proxy/chain.ts | 18 +++-- .../processors/push-action/clearBareClone.ts | 14 +++- src/proxy/processors/push-action/parsePush.ts | 2 + .../processors/push-action/pullRemote.ts | 79 +++++++++++-------- 5 files changed, 71 insertions(+), 45 deletions(-) diff --git a/src/proxy/actions/Action.ts b/src/proxy/actions/Action.ts index d9ea96feb..eba4967e6 100644 --- a/src/proxy/actions/Action.ts +++ b/src/proxy/actions/Action.ts @@ -94,7 +94,8 @@ class Action { } /** - * Set the commit range for the action. + * Set the commit range for the action. Changes the action.id to be based on the + * the commit details. * @param {string} commitFrom the starting commit * @param {string} commitTo the ending commit */ diff --git a/src/proxy/chain.ts b/src/proxy/chain.ts index 5aeac2d96..87f04999d 100644 --- a/src/proxy/chain.ts +++ b/src/proxy/chain.ts @@ -10,15 +10,13 @@ const pushActionChain: ((req: any, action: Action) => Promise)[] = [ proc.push.checkCommitMessages, proc.push.checkAuthorEmails, proc.push.checkUserPushPermission, - proc.push.pullRemote, + proc.push.pullRemote, // cleanup is handled after chain execution if successful proc.push.writePack, proc.push.checkHiddenCommits, proc.push.checkIfWaitingAuth, proc.push.preReceive, proc.push.getDiff, - // run before clear remote proc.push.gitleaks, - proc.push.clearBareClone, proc.push.scanDiff, proc.push.blockForAuth, ]; @@ -32,6 +30,7 @@ const defaultActionChain: ((req: any, action: Action) => Promise)[] = [ ]; let pluginsInserted = false; +let checkoutCleanUpRequired = false; export const executeChain = async (req: any, res: any): Promise => { let action: Action = {} as Action; @@ -44,6 +43,10 @@ export const executeChain = async (req: any, res: any): Promise => { action = await fn(req, action); if (!action.continue() || action.allowPush) { break; + } else 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; } } } catch (e) { @@ -51,11 +54,16 @@ export const executeChain = async (req: any, res: any): Promise => { action.errorMessage = `An error occurred when executing the chain: ${e}`; console.error(action.errorMessage); } finally { + //clean up the clone created + if (checkoutCleanUpRequired) { + action = await proc.push.clearBareClone(req, action); + } + await proc.push.audit(req, action); if (action.autoApproved) { - attemptAutoApproval(action); + await attemptAutoApproval(action); } else if (action.autoRejected) { - attemptAutoRejection(action); + await attemptAutoRejection(action); } } diff --git a/src/proxy/processors/push-action/clearBareClone.ts b/src/proxy/processors/push-action/clearBareClone.ts index dd29f5612..2277a3d82 100644 --- a/src/proxy/processors/push-action/clearBareClone.ts +++ b/src/proxy/processors/push-action/clearBareClone.ts @@ -1,14 +1,20 @@ import { Action, Step } from '../../actions'; -import fs from 'node:fs/promises'; +import fs from 'fs'; const exec = async (req: any, action: Action): Promise => { const step = new Step('clearBareClone'); // Recursively remove the contents of ./.remote and ignore exceptions - await fs.rm('./.remote', { recursive: true, force: true }); - console.log(`.remote is deleted!`); - + if (action.proxyGitPath) { + fs.rmSync(action.proxyGitPath, { recursive: true, force: true }); + step.log(`.remote is deleted!`); + } else { + // This action should not be called unless a clone was made successfully as pullRemote cleans up after itself on failures + // Log an error as we couldn't delete the clone + step.setError(`action.proxyGitPath was not set and cannot be removed`); + } action.addStep(step); + return action; }; diff --git a/src/proxy/processors/push-action/parsePush.ts b/src/proxy/processors/push-action/parsePush.ts index ababdb751..307fe6286 100644 --- a/src/proxy/processors/push-action/parsePush.ts +++ b/src/proxy/processors/push-action/parsePush.ts @@ -63,6 +63,8 @@ async function exec(req: any, action: Action): Promise { // Strip everything after NUL, which is cap-list from // https://git-scm.com/docs/http-protocol#_smart_server_response action.branch = ref.replace(/\0.*/, '').trim(); + + // Note this will change the action.id to be based on the commits action.setCommit(oldCommit, newCommit); // Check if the offset is valid and if there's data after it diff --git a/src/proxy/processors/push-action/pullRemote.ts b/src/proxy/processors/push-action/pullRemote.ts index 9c8661166..4583cfdc7 100644 --- a/src/proxy/processors/push-action/pullRemote.ts +++ b/src/proxy/processors/push-action/pullRemote.ts @@ -7,45 +7,54 @@ const dir = './.remote'; const exec = async (req: any, action: Action): Promise => { const step = new Step('pullRemote'); + action.proxyGitPath = `${dir}/${action.id}`; - try { - action.proxyGitPath = `${dir}/${action.id}`; - - if (!fs.existsSync(dir)) { - fs.mkdirSync(dir); - } - - if (!fs.existsSync(action.proxyGitPath)) { + //the specific checkout folder should not exist + // - fail out if it does to avoid concurrent processing of conflicting requests + if (fs.existsSync(action.proxyGitPath)) { + const errMsg = + 'The checkout folder already exists - we may be processing a concurrent request for this push. If this issue persists the proxy may need to be restarted.'; + // do not delete the folder so that the other request can complete if its going to + step.setError(errMsg); + action.addStep(step); + throw new Error(errMsg); + } else { + try { step.log(`Creating folder ${action.proxyGitPath}`); fs.mkdirSync(action.proxyGitPath, 0o755); - } - const cmd = `git clone ${action.url}`; - step.log(`Executing ${cmd}`); - - const authHeader = req.headers?.authorization; - const [username, password] = Buffer.from(authHeader.split(' ')[1], 'base64') - .toString() - .split(':'); - - // Note: setting singleBranch to true will cause issues when pushing to - // a non-default branch as commits from those branches won't be fetched - await git.clone({ - fs, - http: gitHttpClient, - url: action.url, - dir: `${action.proxyGitPath}/${action.repoName}`, - onAuth: () => ({ username, password }), - depth: 1, - }); - - step.log(`Completed ${cmd}`); - step.setContent(`Completed ${cmd}`); - } catch (e: any) { - step.setError(e.toString('utf-8')); - throw e; - } finally { - action.addStep(step); + const cmd = `git clone ${action.url}`; + step.log(`Executing ${cmd}`); + + const authHeader = req.headers?.authorization; + const [username, password] = Buffer.from(authHeader.split(' ')[1], 'base64') + .toString() + .split(':'); + + // Note: setting singleBranch to true will cause issues when pushing to + // a non-default branch as commits from those branches won't be fetched + await git.clone({ + fs, + http: gitHttpClient, + url: action.url, + dir: `${action.proxyGitPath}/${action.repoName}`, + onAuth: () => ({ username, password }), + depth: 1, + }); + + step.log(`Completed ${cmd}`); + step.setContent(`Completed ${cmd}`); + } catch (e: any) { + step.setError(e.toString('utf-8')); + + //clean-up the check out folder so it doesn't block subsequent attempts + fs.rmSync(action.proxyGitPath, { recursive: true, force: true }); + step.log(`.remote is deleted!`); + + throw e; + } finally { + action.addStep(step); + } } return action; }; From 8bb0316958204f0446f5aa110d9fd21b1b8e8939 Mon Sep 17 00:00:00 2001 From: Kris West Date: Fri, 6 Feb 2026 17:45:47 +0000 Subject: [PATCH 3/4] chore: separate post-processors from push processors and clean up processor tests --- src/proxy/chain.ts | 5 +- src/proxy/processors/index.ts | 3 +- .../{push-action => post-processor}/audit.ts | 0 .../clearBareClone.ts | 0 src/proxy/processors/post-processor/index.ts | 4 + src/proxy/processors/push-action/index.ts | 6 +- test/chain.test.ts | 205 ++++++++---------- test/processors/clearBareClone.test.ts | 38 ++-- 8 files changed, 122 insertions(+), 139 deletions(-) rename src/proxy/processors/{push-action => post-processor}/audit.ts (100%) rename src/proxy/processors/{push-action => post-processor}/clearBareClone.ts (100%) create mode 100644 src/proxy/processors/post-processor/index.ts diff --git a/src/proxy/chain.ts b/src/proxy/chain.ts index 87f04999d..8b116f00e 100644 --- a/src/proxy/chain.ts +++ b/src/proxy/chain.ts @@ -56,10 +56,11 @@ export const executeChain = async (req: any, res: any): Promise => { } finally { //clean up the clone created if (checkoutCleanUpRequired) { - action = await proc.push.clearBareClone(req, action); + action = await proc.post.clearBareClone(req, action); } - await proc.push.audit(req, action); + action = await proc.post.audit(req, action); + if (action.autoApproved) { await attemptAutoApproval(action); } else if (action.autoRejected) { diff --git a/src/proxy/processors/index.ts b/src/proxy/processors/index.ts index 587a83e4f..3bac1fab7 100644 --- a/src/proxy/processors/index.ts +++ b/src/proxy/processors/index.ts @@ -1,4 +1,5 @@ import * as pre from './pre-processor'; import * as push from './push-action'; +import * as post from './post-processor'; -export { pre, push }; +export { pre, push, post }; diff --git a/src/proxy/processors/push-action/audit.ts b/src/proxy/processors/post-processor/audit.ts similarity index 100% rename from src/proxy/processors/push-action/audit.ts rename to src/proxy/processors/post-processor/audit.ts diff --git a/src/proxy/processors/push-action/clearBareClone.ts b/src/proxy/processors/post-processor/clearBareClone.ts similarity index 100% rename from src/proxy/processors/push-action/clearBareClone.ts rename to src/proxy/processors/post-processor/clearBareClone.ts diff --git a/src/proxy/processors/post-processor/index.ts b/src/proxy/processors/post-processor/index.ts new file mode 100644 index 000000000..10220d4dd --- /dev/null +++ b/src/proxy/processors/post-processor/index.ts @@ -0,0 +1,4 @@ +import { exec as audit } from '../post-processor/audit'; +import { exec as clearBareClone } from '../post-processor/clearBareClone'; + +export { audit, clearBareClone }; diff --git a/src/proxy/processors/push-action/index.ts b/src/proxy/processors/push-action/index.ts index 2947c788e..f7f1fbb21 100644 --- a/src/proxy/processors/push-action/index.ts +++ b/src/proxy/processors/push-action/index.ts @@ -1,7 +1,7 @@ import { exec as parsePush } from './parsePush'; import { exec as preReceive } from './preReceive'; import { exec as checkRepoInAuthorisedList } from './checkRepoInAuthorisedList'; -import { exec as audit } from './audit'; + import { exec as pullRemote } from './pullRemote'; import { exec as writePack } from './writePack'; import { exec as getDiff } from './getDiff'; @@ -13,14 +13,13 @@ import { exec as checkIfWaitingAuth } from './checkIfWaitingAuth'; import { exec as checkCommitMessages } from './checkCommitMessages'; import { exec as checkAuthorEmails } from './checkAuthorEmails'; import { exec as checkUserPushPermission } from './checkUserPushPermission'; -import { exec as clearBareClone } from './clearBareClone'; + import { exec as checkEmptyBranch } from './checkEmptyBranch'; export { parsePush, preReceive, checkRepoInAuthorisedList, - audit, pullRemote, writePack, getDiff, @@ -32,6 +31,5 @@ export { checkCommitMessages, checkAuthorEmails, checkUserPushPermission, - clearBareClone, checkEmptyBranch, }; diff --git a/test/chain.test.ts b/test/chain.test.ts index e9bc3fb0a..8ef00c6f5 100644 --- a/test/chain.test.ts +++ b/test/chain.test.ts @@ -1,5 +1,6 @@ import { describe, it, beforeEach, afterEach, expect, vi } from 'vitest'; import { PluginLoader } from '../src/plugin'; +import { Action } from '../src/proxy/actions'; const mockLoader = { pushPlugins: [ @@ -11,10 +12,9 @@ const mockLoader = { }; const initMockPushProcessors = () => { - const mockPushProcessors = { + return { parsePush: vi.fn(), checkEmptyBranch: vi.fn(), - audit: vi.fn(), checkRepoInAuthorisedList: vi.fn(), checkCommitMessages: vi.fn(), checkAuthorEmails: vi.fn(), @@ -30,7 +30,13 @@ const initMockPushProcessors = () => { scanDiff: vi.fn(), blockForAuth: vi.fn(), }; - return mockPushProcessors; +}; + +const initMockPostProcessors = () => { + return { + audit: vi.fn(), + clearBareClone: vi.fn(), + }; }; const mockPreProcessors = { @@ -42,17 +48,20 @@ describe('proxy chain', function () { let chain: any; let db: any; let mockPushProcessors: any; + let mockPostProcessors: any; beforeEach(async () => { vi.resetModules(); // Initialize the mocks mockPushProcessors = initMockPushProcessors(); + mockPostProcessors = initMockPostProcessors(); // Mock the processors module vi.doMock('../src/proxy/processors', async () => ({ pre: mockPreProcessors, push: mockPushProcessors, + post: mockPostProcessors, })); vi.doMock('../src/db', async () => ({ @@ -67,6 +76,17 @@ describe('proxy chain', function () { chain = chainModule.default; chain.chainPluginLoader = new PluginLoader([]); + + //mock all processors as pass-through by default + const passThroughImpl = (req: any, action: Action) => { + return action; + }; + Object.keys(mockPushProcessors).forEach((key) => { + mockPushProcessors[key].mockImplementation(passThroughImpl); + }); + Object.keys(mockPostProcessors).forEach((key) => { + mockPostProcessors[key].mockImplementation(passThroughImpl); + }); }); afterEach(() => { @@ -101,16 +121,11 @@ describe('proxy chain', function () { it('executeChain should stop executing if action has continue returns false', async () => { const req = {}; const continuingAction = { type: 'push', continue: () => true, allowPush: false }; - mockPreProcessors.parseAction.mockResolvedValue({ type: 'push' }); + const action = { type: 'push' } as Action; + mockPreProcessors.parseAction.mockResolvedValue(action); + mockPushProcessors.parsePush.mockResolvedValue(continuingAction); - mockPushProcessors.checkEmptyBranch.mockResolvedValue(continuingAction); - mockPushProcessors.checkRepoInAuthorisedList.mockResolvedValue(continuingAction); - mockPushProcessors.checkCommitMessages.mockResolvedValue(continuingAction); - mockPushProcessors.checkAuthorEmails.mockResolvedValue(continuingAction); - mockPushProcessors.checkUserPushPermission.mockResolvedValue(continuingAction); - mockPushProcessors.checkHiddenCommits.mockResolvedValue(continuingAction); - mockPushProcessors.pullRemote.mockResolvedValue(continuingAction); - mockPushProcessors.writePack.mockResolvedValue(continuingAction); + // this stops the chain from further execution mockPushProcessors.checkIfWaitingAuth.mockResolvedValue({ type: 'push', @@ -120,37 +135,42 @@ describe('proxy chain', function () { const result = await chain.executeChain(req); + //all processors upto checkIfWaitingAuth should have run + clearBareClone & audit expect(mockPreProcessors.parseAction).toHaveBeenCalled(); expect(mockPushProcessors.parsePush).toHaveBeenCalled(); + expect(mockPushProcessors.checkEmptyBranch).toHaveBeenCalled(); expect(mockPushProcessors.checkRepoInAuthorisedList).toHaveBeenCalled(); expect(mockPushProcessors.checkCommitMessages).toHaveBeenCalled(); expect(mockPushProcessors.checkAuthorEmails).toHaveBeenCalled(); expect(mockPushProcessors.checkUserPushPermission).toHaveBeenCalled(); - expect(mockPushProcessors.checkIfWaitingAuth).toHaveBeenCalled(); expect(mockPushProcessors.pullRemote).toHaveBeenCalled(); - expect(mockPushProcessors.checkHiddenCommits).toHaveBeenCalled(); expect(mockPushProcessors.writePack).toHaveBeenCalled(); - expect(mockPushProcessors.checkEmptyBranch).toHaveBeenCalled(); - expect(mockPushProcessors.audit).toHaveBeenCalled(); + expect(mockPushProcessors.checkHiddenCommits).toHaveBeenCalled(); + expect(mockPushProcessors.checkIfWaitingAuth).toHaveBeenCalled(); + + expect(mockPushProcessors.preReceive).not.toHaveBeenCalled(); + expect(mockPushProcessors.getDiff).not.toHaveBeenCalled(); + expect(mockPushProcessors.gitleaks).not.toHaveBeenCalled(); + expect(mockPushProcessors.scanDiff).not.toHaveBeenCalled(); + expect(mockPushProcessors.blockForAuth).not.toHaveBeenCalled(); + + expect(mockPostProcessors.audit).toHaveBeenCalled(); + expect(mockPostProcessors.clearBareClone).toHaveBeenCalled(); expect(result.type).toBe('push'); expect(result.allowPush).toBe(false); expect(result.continue).toBeTypeOf('function'); + expect(result.continue()).toBe(false); }); it('executeChain should stop executing if action has allowPush is set to true', async () => { const req = {}; const continuingAction = { type: 'push', continue: () => true, allowPush: false }; - mockPreProcessors.parseAction.mockResolvedValue({ type: 'push' }); + const action = { type: 'push' } as Action; + mockPreProcessors.parseAction.mockResolvedValue(action); + mockPushProcessors.parsePush.mockResolvedValue(continuingAction); - mockPushProcessors.checkEmptyBranch.mockResolvedValue(continuingAction); - mockPushProcessors.checkRepoInAuthorisedList.mockResolvedValue(continuingAction); - mockPushProcessors.checkCommitMessages.mockResolvedValue(continuingAction); - mockPushProcessors.checkAuthorEmails.mockResolvedValue(continuingAction); - mockPushProcessors.checkUserPushPermission.mockResolvedValue(continuingAction); - mockPushProcessors.checkHiddenCommits.mockResolvedValue(continuingAction); - mockPushProcessors.pullRemote.mockResolvedValue(continuingAction); - mockPushProcessors.writePack.mockResolvedValue(continuingAction); + // this stops the chain from further execution mockPushProcessors.checkIfWaitingAuth.mockResolvedValue({ type: 'push', @@ -160,6 +180,7 @@ describe('proxy chain', function () { const result = await chain.executeChain(req); + //all processors upto checkIfWaitingAuth should have run + clearBareClone & audit expect(mockPreProcessors.parseAction).toHaveBeenCalled(); expect(mockPushProcessors.parsePush).toHaveBeenCalled(); expect(mockPushProcessors.checkEmptyBranch).toHaveBeenCalled(); @@ -167,40 +188,37 @@ describe('proxy chain', function () { expect(mockPushProcessors.checkCommitMessages).toHaveBeenCalled(); expect(mockPushProcessors.checkAuthorEmails).toHaveBeenCalled(); expect(mockPushProcessors.checkUserPushPermission).toHaveBeenCalled(); - expect(mockPushProcessors.checkIfWaitingAuth).toHaveBeenCalled(); expect(mockPushProcessors.pullRemote).toHaveBeenCalled(); - expect(mockPushProcessors.checkHiddenCommits).toHaveBeenCalled(); expect(mockPushProcessors.writePack).toHaveBeenCalled(); - expect(mockPushProcessors.audit).toHaveBeenCalled(); + expect(mockPushProcessors.checkHiddenCommits).toHaveBeenCalled(); + expect(mockPushProcessors.checkIfWaitingAuth).toHaveBeenCalled(); + + expect(mockPushProcessors.preReceive).not.toHaveBeenCalled(); + expect(mockPushProcessors.getDiff).not.toHaveBeenCalled(); + expect(mockPushProcessors.gitleaks).not.toHaveBeenCalled(); + expect(mockPushProcessors.scanDiff).not.toHaveBeenCalled(); + expect(mockPushProcessors.blockForAuth).not.toHaveBeenCalled(); + + expect(mockPostProcessors.audit).toHaveBeenCalled(); + expect(mockPostProcessors.clearBareClone).toHaveBeenCalled(); expect(result.type).toBe('push'); expect(result.allowPush).toBe(true); expect(result.continue).toBeTypeOf('function'); + expect(result.continue()).toBe(true); }); it('executeChain should execute all steps if all actions succeed', async () => { const req = {}; const continuingAction = { type: 'push', continue: () => true, allowPush: false }; - mockPreProcessors.parseAction.mockResolvedValue({ type: 'push' }); + const action = { type: 'push' } as Action; + mockPreProcessors.parseAction.mockResolvedValue(action); + mockPushProcessors.parsePush.mockResolvedValue(continuingAction); - mockPushProcessors.checkEmptyBranch.mockResolvedValue(continuingAction); - mockPushProcessors.checkRepoInAuthorisedList.mockResolvedValue(continuingAction); - mockPushProcessors.checkCommitMessages.mockResolvedValue(continuingAction); - mockPushProcessors.checkAuthorEmails.mockResolvedValue(continuingAction); - mockPushProcessors.checkUserPushPermission.mockResolvedValue(continuingAction); - mockPushProcessors.checkIfWaitingAuth.mockResolvedValue(continuingAction); - mockPushProcessors.pullRemote.mockResolvedValue(continuingAction); - mockPushProcessors.writePack.mockResolvedValue(continuingAction); - mockPushProcessors.checkHiddenCommits.mockResolvedValue(continuingAction); - mockPushProcessors.preReceive.mockResolvedValue(continuingAction); - mockPushProcessors.getDiff.mockResolvedValue(continuingAction); - mockPushProcessors.gitleaks.mockResolvedValue(continuingAction); - mockPushProcessors.clearBareClone.mockResolvedValue(continuingAction); - mockPushProcessors.scanDiff.mockResolvedValue(continuingAction); - mockPushProcessors.blockForAuth.mockResolvedValue(continuingAction); const result = await chain.executeChain(req); + //all processors upto checkIfWaitingAuth should have run + clearBareClone & audit expect(mockPreProcessors.parseAction).toHaveBeenCalled(); expect(mockPushProcessors.parsePush).toHaveBeenCalled(); expect(mockPushProcessors.checkEmptyBranch).toHaveBeenCalled(); @@ -208,21 +226,23 @@ describe('proxy chain', function () { expect(mockPushProcessors.checkCommitMessages).toHaveBeenCalled(); expect(mockPushProcessors.checkAuthorEmails).toHaveBeenCalled(); expect(mockPushProcessors.checkUserPushPermission).toHaveBeenCalled(); - expect(mockPushProcessors.checkIfWaitingAuth).toHaveBeenCalled(); expect(mockPushProcessors.pullRemote).toHaveBeenCalled(); - expect(mockPushProcessors.checkHiddenCommits).toHaveBeenCalled(); expect(mockPushProcessors.writePack).toHaveBeenCalled(); + expect(mockPushProcessors.checkHiddenCommits).toHaveBeenCalled(); + expect(mockPushProcessors.checkIfWaitingAuth).toHaveBeenCalled(); expect(mockPushProcessors.preReceive).toHaveBeenCalled(); expect(mockPushProcessors.getDiff).toHaveBeenCalled(); expect(mockPushProcessors.gitleaks).toHaveBeenCalled(); - expect(mockPushProcessors.clearBareClone).toHaveBeenCalled(); expect(mockPushProcessors.scanDiff).toHaveBeenCalled(); expect(mockPushProcessors.blockForAuth).toHaveBeenCalled(); - expect(mockPushProcessors.audit).toHaveBeenCalled(); + + expect(mockPostProcessors.audit).toHaveBeenCalled(); + expect(mockPostProcessors.clearBareClone).toHaveBeenCalled(); expect(result.type).toBe('push'); expect(result.allowPush).toBe(false); expect(result.continue).toBeTypeOf('function'); + expect(result.continue()).toBe(true); }); it('executeChain should run the expected steps for pulls', async () => { @@ -235,12 +255,14 @@ describe('proxy chain', function () { expect(mockPushProcessors.checkRepoInAuthorisedList).toHaveBeenCalled(); expect(mockPushProcessors.parsePush).not.toHaveBeenCalled(); + expect(mockPostProcessors.audit).toHaveBeenCalled(); + expect(mockPostProcessors.clearBareClone).not.toHaveBeenCalled(); expect(result.type).toBe('pull'); }); it('executeChain should handle errors and still call audit', async () => { const req = {}; - const action = { type: 'push', continue: () => true, allowPush: true }; + const action = { type: 'push', continue: () => true, allowPush: false }; processors.pre.parseAction.mockResolvedValue(action); mockPushProcessors.parsePush.mockRejectedValue(new Error('Audit error')); @@ -251,10 +273,26 @@ describe('proxy chain', function () { // Ignore the error } - expect(mockPushProcessors.audit).toHaveBeenCalled(); + expect(mockPostProcessors.audit).toHaveBeenCalled(); + }); + + it('executeChain should handle errors after pullRemote and still call clearBareClone', async () => { + const req = {}; + const action = { type: 'push', continue: () => true, allowPush: false }; + + processors.pre.parseAction.mockResolvedValue(action); + mockPushProcessors.writePack.mockRejectedValue(new Error('writePack error')); + + try { + await chain.executeChain(req); + } catch { + // Ignore the error + } + + expect(mockPostProcessors.clearBareClone).toHaveBeenCalled(); }); - it('executeChain should always run at least checkRepoInAuthList', async () => { + it('executeChain should always run at least checkRepoInAuthList and audit', async () => { const req = {}; const action = { type: 'foo', continue: () => true, allowPush: true }; @@ -263,6 +301,7 @@ describe('proxy chain', function () { await chain.executeChain(req); expect(mockPushProcessors.checkRepoInAuthorisedList).toHaveBeenCalled(); + expect(mockPostProcessors.audit).toHaveBeenCalled(); }); it('should approve push automatically and record in the database', async () => { @@ -278,16 +317,6 @@ describe('proxy chain', function () { }; mockPreProcessors.parseAction.mockResolvedValue(action); - mockPushProcessors.parsePush.mockResolvedValue(action); - mockPushProcessors.checkEmptyBranch.mockResolvedValue(action); - mockPushProcessors.checkRepoInAuthorisedList.mockResolvedValue(action); - mockPushProcessors.checkCommitMessages.mockResolvedValue(action); - mockPushProcessors.checkAuthorEmails.mockResolvedValue(action); - mockPushProcessors.checkUserPushPermission.mockResolvedValue(action); - mockPushProcessors.checkIfWaitingAuth.mockResolvedValue(action); - mockPushProcessors.pullRemote.mockResolvedValue(action); - mockPushProcessors.writePack.mockResolvedValue(action); - mockPushProcessors.checkHiddenCommits.mockResolvedValue(action); mockPushProcessors.preReceive.mockResolvedValue({ ...action, @@ -296,12 +325,6 @@ describe('proxy chain', function () { autoApproved: true, }); - mockPushProcessors.getDiff.mockResolvedValue(action); - mockPushProcessors.gitleaks.mockResolvedValue(action); - mockPushProcessors.clearBareClone.mockResolvedValue(action); - mockPushProcessors.scanDiff.mockResolvedValue(action); - mockPushProcessors.blockForAuth.mockResolvedValue(action); - const dbSpy = vi.spyOn(db, 'authorise').mockResolvedValue({ message: `authorised ${action.id}`, }); @@ -327,16 +350,6 @@ describe('proxy chain', function () { }; mockPreProcessors.parseAction.mockResolvedValue(action); - mockPushProcessors.parsePush.mockResolvedValue(action); - mockPushProcessors.checkEmptyBranch.mockResolvedValue(action); - mockPushProcessors.checkRepoInAuthorisedList.mockResolvedValue(action); - mockPushProcessors.checkCommitMessages.mockResolvedValue(action); - mockPushProcessors.checkAuthorEmails.mockResolvedValue(action); - mockPushProcessors.checkUserPushPermission.mockResolvedValue(action); - mockPushProcessors.checkIfWaitingAuth.mockResolvedValue(action); - mockPushProcessors.pullRemote.mockResolvedValue(action); - mockPushProcessors.writePack.mockResolvedValue(action); - mockPushProcessors.checkHiddenCommits.mockResolvedValue(action); mockPushProcessors.preReceive.mockResolvedValue({ ...action, @@ -345,12 +358,6 @@ describe('proxy chain', function () { autoRejected: true, }); - mockPushProcessors.getDiff.mockResolvedValue(action); - mockPushProcessors.gitleaks.mockResolvedValue(action); - mockPushProcessors.clearBareClone.mockResolvedValue(action); - mockPushProcessors.scanDiff.mockResolvedValue(action); - mockPushProcessors.blockForAuth.mockResolvedValue(action); - const dbSpy = vi.spyOn(db, 'reject').mockResolvedValue({ message: `reject ${action.id}`, }); @@ -375,16 +382,6 @@ describe('proxy chain', function () { }; mockPreProcessors.parseAction.mockResolvedValue(action); - mockPushProcessors.parsePush.mockResolvedValue(action); - mockPushProcessors.checkEmptyBranch.mockResolvedValue(action); - mockPushProcessors.checkRepoInAuthorisedList.mockResolvedValue(action); - mockPushProcessors.checkCommitMessages.mockResolvedValue(action); - mockPushProcessors.checkAuthorEmails.mockResolvedValue(action); - mockPushProcessors.checkUserPushPermission.mockResolvedValue(action); - mockPushProcessors.checkIfWaitingAuth.mockResolvedValue(action); - mockPushProcessors.pullRemote.mockResolvedValue(action); - mockPushProcessors.writePack.mockResolvedValue(action); - mockPushProcessors.checkHiddenCommits.mockResolvedValue(action); mockPushProcessors.preReceive.mockResolvedValue({ ...action, @@ -393,12 +390,6 @@ describe('proxy chain', function () { autoApproved: true, }); - mockPushProcessors.getDiff.mockResolvedValue(action); - mockPushProcessors.gitleaks.mockResolvedValue(action); - mockPushProcessors.clearBareClone.mockResolvedValue(action); - mockPushProcessors.scanDiff.mockResolvedValue(action); - mockPushProcessors.blockForAuth.mockResolvedValue(action); - const error = new Error('Database error'); const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); vi.spyOn(db, 'authorise').mockRejectedValue(error); @@ -421,16 +412,6 @@ describe('proxy chain', function () { }; mockPreProcessors.parseAction.mockResolvedValue(action); - mockPushProcessors.parsePush.mockResolvedValue(action); - mockPushProcessors.checkEmptyBranch.mockResolvedValue(action); - mockPushProcessors.checkRepoInAuthorisedList.mockResolvedValue(action); - mockPushProcessors.checkCommitMessages.mockResolvedValue(action); - mockPushProcessors.checkAuthorEmails.mockResolvedValue(action); - mockPushProcessors.checkUserPushPermission.mockResolvedValue(action); - mockPushProcessors.checkIfWaitingAuth.mockResolvedValue(action); - mockPushProcessors.pullRemote.mockResolvedValue(action); - mockPushProcessors.writePack.mockResolvedValue(action); - mockPushProcessors.checkHiddenCommits.mockResolvedValue(action); mockPushProcessors.preReceive.mockResolvedValue({ ...action, @@ -439,12 +420,6 @@ describe('proxy chain', function () { autoRejected: true, }); - mockPushProcessors.getDiff.mockResolvedValue(action); - mockPushProcessors.gitleaks.mockResolvedValue(action); - mockPushProcessors.clearBareClone.mockResolvedValue(action); - mockPushProcessors.scanDiff.mockResolvedValue(action); - mockPushProcessors.blockForAuth.mockResolvedValue(action); - const error = new Error('Database error'); const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); vi.spyOn(db, 'reject').mockRejectedValue(error); diff --git a/test/processors/clearBareClone.test.ts b/test/processors/clearBareClone.test.ts index 60624196c..ab56d0703 100644 --- a/test/processors/clearBareClone.test.ts +++ b/test/processors/clearBareClone.test.ts @@ -1,19 +1,27 @@ -import { describe, it, expect, afterEach } from 'vitest'; +import { describe, it, expect, afterAll, beforeAll } from 'vitest'; import fs from 'fs'; -import { exec as clearBareClone } from '../../src/proxy/processors/push-action/clearBareClone'; +import { exec as clearBareClone } from '../../src/proxy/processors/post-processor/clearBareClone'; import { exec as pullRemote } from '../../src/proxy/processors/push-action/pullRemote'; import { Action } from '../../src/proxy/actions/Action'; const actionId = '123__456'; const timestamp = Date.now(); +const remoteFolder = `./.remote`; -describe('clear bare and local clones', () => { - it('pull remote generates a local .remote folder', async () => { - const action = new Action(actionId, 'type', 'get', timestamp, 'finos/git-proxy.git'); +describe('clear local clones', () => { + beforeAll(() => { + //make sure the remote folder exists (normally created on proxy startup) + if (!fs.existsSync(remoteFolder)) { + fs.mkdirSync(remoteFolder); + } + }); + + it('pullRemote generates a local .remote/* folder that clearBareClone purges afterwards', async () => { + let action = new Action(actionId, 'type', 'get', timestamp, 'finos/git-proxy.git'); action.url = 'https://github.com/finos/git-proxy.git'; const authorization = `Basic ${Buffer.from('JamieSlome:test').toString('base64')}`; - await pullRemote( + action = await pullRemote( { headers: { authorization, @@ -22,20 +30,16 @@ describe('clear bare and local clones', () => { action, ); - expect(fs.existsSync(`./.remote/${actionId}`)).toBe(true); - }, 20000); + expect(fs.existsSync(`${remoteFolder}/${actionId}`)).toBe(true); - it('clear bare clone function purges .remote folder and specific clone folder', async () => { - const action = new Action(actionId, 'type', 'get', timestamp, 'finos/git-proxy.git'); - await clearBareClone(null, action); + action = await clearBareClone(null, action); - expect(fs.existsSync(`./.remote`)).toBe(false); - expect(fs.existsSync(`./.remote/${actionId}`)).toBe(false); - }); + expect(fs.existsSync(`${remoteFolder}/${actionId}`)).toBe(false); + }, 20000); - afterEach(() => { - if (fs.existsSync(`./.remote`)) { - fs.rmSync(`./.remote`, { recursive: true }); + afterAll(() => { + if (fs.existsSync(remoteFolder)) { + fs.rmSync(remoteFolder, { recursive: true, force: true }); } }); }); From 9b7eab9c6146b3d0e8ccd7b34e67e49f349e4863 Mon Sep 17 00:00:00 2001 From: Kris West Date: Mon, 9 Feb 2026 11:34:01 +0000 Subject: [PATCH 4/4] fix: move checkoutCleanUpRequired into executeChain function and correct a comment --- src/proxy/actions/Action.ts | 2 +- src/proxy/chain.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/proxy/actions/Action.ts b/src/proxy/actions/Action.ts index eba4967e6..376c15c12 100644 --- a/src/proxy/actions/Action.ts +++ b/src/proxy/actions/Action.ts @@ -94,7 +94,7 @@ class Action { } /** - * Set the commit range for the action. Changes the action.id to be based on the + * Set the commit range for the action. Changes the action.id to be based on * the commit details. * @param {string} commitFrom the starting commit * @param {string} commitTo the ending commit diff --git a/src/proxy/chain.ts b/src/proxy/chain.ts index 8b116f00e..b6c1b7609 100644 --- a/src/proxy/chain.ts +++ b/src/proxy/chain.ts @@ -30,10 +30,10 @@ const defaultActionChain: ((req: any, action: Action) => Promise)[] = [ ]; let pluginsInserted = false; -let checkoutCleanUpRequired = false; export const executeChain = async (req: any, res: any): Promise => { let action: Action = {} as Action; + let checkoutCleanUpRequired = false; try { action = await proc.pre.parseAction(req);