From 654d5354ff1ed945312255bff2a5f4e7b20e93d0 Mon Sep 17 00:00:00 2001 From: Juan Escalada Date: Sun, 3 Aug 2025 16:41:39 +0900 Subject: [PATCH 01/11] refactor: replace getMissingData with checkEmptyBranch and move action order --- src/proxy/chain.ts | 2 +- .../push-action/checkEmptyBranch.ts | 45 +++++++++++ .../push-action/checkUserPushPermission.ts | 4 +- .../processors/push-action/getMissingData.ts | 76 ------------------- src/proxy/processors/push-action/index.ts | 4 +- test/processors/checkCommitMessages.test.js | 5 +- 6 files changed, 54 insertions(+), 82 deletions(-) create mode 100644 src/proxy/processors/push-action/checkEmptyBranch.ts delete mode 100644 src/proxy/processors/push-action/getMissingData.ts diff --git a/src/proxy/chain.ts b/src/proxy/chain.ts index 2130519cf..f504476be 100644 --- a/src/proxy/chain.ts +++ b/src/proxy/chain.ts @@ -5,6 +5,7 @@ import { attemptAutoApproval, attemptAutoRejection } from './actions/autoActions const pushActionChain: ((req: any, action: Action) => Promise)[] = [ proc.push.parsePush, + proc.push.checkEmptyBranch, proc.push.checkRepoInAuthorisedList, proc.push.checkCommitMessages, proc.push.checkAuthorEmails, @@ -13,7 +14,6 @@ const pushActionChain: ((req: any, action: Action) => Promise)[] = [ proc.push.writePack, proc.push.checkHiddenCommits, proc.push.checkIfWaitingAuth, - proc.push.getMissingData, proc.push.preReceive, proc.push.getDiff, // run before clear remote diff --git a/src/proxy/processors/push-action/checkEmptyBranch.ts b/src/proxy/processors/push-action/checkEmptyBranch.ts new file mode 100644 index 000000000..61895db15 --- /dev/null +++ b/src/proxy/processors/push-action/checkEmptyBranch.ts @@ -0,0 +1,45 @@ +import { Action, Step } from '../../actions'; +import simpleGit from 'simple-git'; +import { EMPTY_COMMIT_HASH } from '../constants'; + +const isEmptyBranch = async (action: Action) => { + const git = simpleGit(`${action.proxyGitPath}/${action.repoName}`); + + if (action.commitFrom === EMPTY_COMMIT_HASH) { + try { + const type = await git.raw(['cat-file', '-t', action.commitTo || '']); + const known = type.trim() === 'commit'; + if (known) { + return true; + } + } catch (err) { + console.log(`Commit ${action.commitTo} not found: ${err}`); + } + } + + return false; +}; + +const exec = async (req: any, action: Action): Promise => { + const step = new Step('checkEmptyBranch'); + + if (action.commitData && action.commitData.length > 0) { + return action; + } + + if (await isEmptyBranch(action)) { + step.setError('Push blocked: Empty branch. Please make a commit before pushing a new branch.'); + action.addStep(step); + step.error = true; + return action; + } else { + step.setError('Push blocked: Commit data not found. Please contact an administrator for support.'); + action.addStep(step); + step.error = true; + return action; + } +}; + +exec.displayName = 'checkEmptyBranch.exec'; + +export { exec }; diff --git a/src/proxy/processors/push-action/checkUserPushPermission.ts b/src/proxy/processors/push-action/checkUserPushPermission.ts index 6243ddb55..a1961bcd6 100644 --- a/src/proxy/processors/push-action/checkUserPushPermission.ts +++ b/src/proxy/processors/push-action/checkUserPushPermission.ts @@ -8,7 +8,9 @@ const exec = async (req: any, action: Action): Promise => { const user = action.user; if (!user) { - console.log('Action has no user set. This may be due to a fast-forward ref update. Deferring to getMissingData action.'); + step.setError('Push blocked: User not found. Please contact an administrator for support.'); + action.addStep(step); + step.error = true; return action; } diff --git a/src/proxy/processors/push-action/getMissingData.ts b/src/proxy/processors/push-action/getMissingData.ts deleted file mode 100644 index e256da3f6..000000000 --- a/src/proxy/processors/push-action/getMissingData.ts +++ /dev/null @@ -1,76 +0,0 @@ -import { Action, Step } from '../../actions'; -import { validateUser } from './checkUserPushPermission'; -import simpleGit from 'simple-git'; -import { EMPTY_COMMIT_HASH } from '../constants'; - -const isEmptyBranch = async (action: Action) => { - const git = simpleGit(`${action.proxyGitPath}/${action.repoName}`); - - if (action.commitFrom === EMPTY_COMMIT_HASH) { - try { - const type = await git.raw(['cat-file', '-t', action.commitTo || '']); - const known = type.trim() === 'commit'; - if (known) { - return true; - } - } catch (err) { - console.log(`Commit ${action.commitTo} not found: ${err}`); - } - } - - return false; -}; - -const exec = async (req: any, action: Action): Promise => { - const step = new Step('getMissingData'); - - if (action.commitData && action.commitData.length > 0) { - console.log('getMissingData', action); - return action; - } - - if (await isEmptyBranch(action)) { - step.setError('Push blocked: Empty branch. Please make a commit before pushing a new branch.'); - action.addStep(step); - step.error = true; - return action; - } - console.log(`commitData not found, fetching missing commits from git...`); - - try { - const path = `${action.proxyGitPath}/${action.repoName}`; - const git = simpleGit(path); - const log = await git.log({ from: action.commitFrom, to: action.commitTo }); - - action.commitData = [...log.all].reverse().map((entry, i, array) => { - const parent = i === 0 ? action.commitFrom : array[i - 1].hash; - const timestamp = Math.floor(new Date(entry.date).getTime() / 1000).toString(); - return { - message: entry.message || '', - committer: entry.author_name || '', - tree: entry.hash || '', - parent: parent || EMPTY_COMMIT_HASH, - author: entry.author_name || '', - authorEmail: entry.author_email || '', - commitTimestamp: timestamp, - } - }); - console.log(`Updated commitData:`, { commitData: action.commitData }); - - if (action.commitFrom === EMPTY_COMMIT_HASH) { - action.commitFrom = action.commitData[action.commitData.length - 1].parent; - } - - const user = action.commitData[action.commitData.length - 1].committer; - action.user = user; - } catch (e: any) { - step.setError(e.toString('utf-8')); - } finally { - action.addStep(step); - } - return await validateUser(action.user || '', action, step); -}; - -exec.displayName = 'getMissingData.exec'; - -export { exec }; diff --git a/src/proxy/processors/push-action/index.ts b/src/proxy/processors/push-action/index.ts index 48f9759c7..2947c788e 100644 --- a/src/proxy/processors/push-action/index.ts +++ b/src/proxy/processors/push-action/index.ts @@ -14,7 +14,7 @@ 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 getMissingData } from './getMissingData'; +import { exec as checkEmptyBranch } from './checkEmptyBranch'; export { parsePush, @@ -33,5 +33,5 @@ export { checkAuthorEmails, checkUserPushPermission, clearBareClone, - getMissingData, + checkEmptyBranch, }; diff --git a/test/processors/checkCommitMessages.test.js b/test/processors/checkCommitMessages.test.js index 75156e0ae..1c6526590 100644 --- a/test/processors/checkCommitMessages.test.js +++ b/test/processors/checkCommitMessages.test.js @@ -97,8 +97,9 @@ describe('checkCommitMessages', () => { }); it('should not error when commit data is empty', async () => { - // Empty commit data is a valid scenario that happens when making a branch from an unapproved commit - // This is remedied in the getMissingData.exec action + // Empty commit data happens when making a branch from an unapproved commit + // or when pushing an empty branch or deleting a branch + // This is remedied in the checkEmptyBranch.exec action action.commitData = []; const result = await exec(req, action); From 05d19b04161ae49e9c4a33f959e7c78e74a30cad Mon Sep 17 00:00:00 2001 From: Juan Escalada Date: Sun, 3 Aug 2025 16:42:06 +0900 Subject: [PATCH 02/11] test: add empty commitData test case for parsePush --- test/testParsePush.test.js | 38 +++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/test/testParsePush.test.js b/test/testParsePush.test.js index 88bdedd06..65c745ec1 100644 --- a/test/testParsePush.test.js +++ b/test/testParsePush.test.js @@ -10,7 +10,7 @@ const { unpack } = require('../src/proxy/processors/push-action/parsePush'); -import { FLUSH_PACKET, PACK_SIGNATURE } from '../src/proxy/processors/constants'; +import { EMPTY_COMMIT_HASH, FLUSH_PACKET, PACK_SIGNATURE } from '../src/proxy/processors/constants'; /** * Creates a simplified sample PACK buffer for testing. @@ -64,6 +64,20 @@ function createPacketLineBuffer(lines) { return buffer; } +/** + * Creates an empty PACK buffer for testing. + * @return {Buffer} - The generated buffer containing the PACK header and checksum. + */ +function createEmptyPackBuffer() { + const header = Buffer.alloc(12); + header.write(PACK_SIGNATURE, 0, 4, 'utf-8'); // signature + header.writeUInt32BE(2, 4); // version + header.writeUInt32BE(0, 8); // number of entries + + const checksum = Buffer.alloc(20); // fake checksum (all zeros) + return Buffer.concat([header, checksum]); +} + describe('parsePackFile', () => { let action; let req; @@ -442,6 +456,28 @@ describe('parsePackFile', () => { expect(step.error).to.be.true; expect(step.errorMessage).to.include('Invalid PACK data structure'); }); + + it('should return empty commitData on empty branch push', async () => { + const emptyPackBuffer = createEmptyPackBuffer(); + + const newCommit = 'b'.repeat(40); + const ref = 'refs/heads/feature/emptybranch'; + const packetLine = `${EMPTY_COMMIT_HASH} ${newCommit} ${ref}\0capabilities\n`; + + req.body = Buffer.concat([createPacketLineBuffer([packetLine]), emptyPackBuffer]); + + const result = await exec(req, action); + + expect(result).to.equal(action); + + const step = action.steps.find(s => s.stepName === 'parsePackFile'); + expect(step).to.exist; + expect(step.error).to.be.false; + expect(action.branch).to.equal(ref); + expect(action.setCommit.calledOnceWith(EMPTY_COMMIT_HASH, newCommit)).to.be.true; + + expect(action.commitData).to.be.an('array').with.lengthOf(0); + }); }); describe('getPackMeta', () => { From 087fb0bcd77f322c4de448d4d52710746fac26ed Mon Sep 17 00:00:00 2001 From: Juan Escalada Date: Sun, 3 Aug 2025 16:42:20 +0900 Subject: [PATCH 03/11] test: fix failing chain tests --- test/chain.test.js | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/test/chain.test.js b/test/chain.test.js index 07bf1b341..1d00d1b7f 100644 --- a/test/chain.test.js +++ b/test/chain.test.js @@ -18,6 +18,7 @@ const mockLoader = { const initMockPushProcessors = (sinon) => { const mockPushProcessors = { parsePush: sinon.stub(), + checkEmptyBranch: sinon.stub(), audit: sinon.stub(), checkRepoInAuthorisedList: sinon.stub(), checkCommitMessages: sinon.stub(), @@ -33,9 +34,9 @@ const initMockPushProcessors = (sinon) => { clearBareClone: sinon.stub(), scanDiff: sinon.stub(), blockForAuth: sinon.stub(), - getMissingData: sinon.stub(), }; mockPushProcessors.parsePush.displayName = 'parsePush'; + mockPushProcessors.checkEmptyBranch.displayName = 'checkEmptyBranch'; mockPushProcessors.audit.displayName = 'audit'; mockPushProcessors.checkRepoInAuthorisedList.displayName = 'checkRepoInAuthorisedList'; mockPushProcessors.checkCommitMessages.displayName = 'checkCommitMessages'; @@ -51,7 +52,6 @@ const initMockPushProcessors = (sinon) => { mockPushProcessors.clearBareClone.displayName = 'clearBareClone'; mockPushProcessors.scanDiff.displayName = 'scanDiff'; mockPushProcessors.blockForAuth.displayName = 'blockForAuth'; - mockPushProcessors.getMissingData.displayName = 'getMissingData'; return mockPushProcessors; }; const mockPreProcessors = { @@ -127,6 +127,7 @@ describe('proxy chain', function () { const continuingAction = { type: 'push', continue: () => true, allowPush: false }; mockPreProcessors.parseAction.resolves({ type: 'push' }); mockPushProcessors.parsePush.resolves(continuingAction); + mockPushProcessors.checkEmptyBranch.resolves(continuingAction); mockPushProcessors.checkRepoInAuthorisedList.resolves(continuingAction); mockPushProcessors.checkCommitMessages.resolves(continuingAction); mockPushProcessors.checkAuthorEmails.resolves(continuingAction); @@ -152,7 +153,7 @@ describe('proxy chain', function () { expect(mockPushProcessors.pullRemote.called).to.be.true; expect(mockPushProcessors.checkHiddenCommits.called).to.be.true; expect(mockPushProcessors.writePack.called).to.be.true; - expect(mockPushProcessors.getMissingData.called).to.be.false; + expect(mockPushProcessors.checkEmptyBranch.called).to.be.true; expect(mockPushProcessors.audit.called).to.be.true; expect(result.type).to.equal('push'); @@ -165,6 +166,7 @@ describe('proxy chain', function () { const continuingAction = { type: 'push', continue: () => true, allowPush: false }; mockPreProcessors.parseAction.resolves({ type: 'push' }); mockPushProcessors.parsePush.resolves(continuingAction); + mockPushProcessors.checkEmptyBranch.resolves(continuingAction); mockPushProcessors.checkRepoInAuthorisedList.resolves(continuingAction); mockPushProcessors.checkCommitMessages.resolves(continuingAction); mockPushProcessors.checkAuthorEmails.resolves(continuingAction); @@ -182,6 +184,7 @@ describe('proxy chain', function () { expect(mockPreProcessors.parseAction.called).to.be.true; expect(mockPushProcessors.parsePush.called).to.be.true; + expect(mockPushProcessors.checkEmptyBranch.called).to.be.true; expect(mockPushProcessors.checkRepoInAuthorisedList.called).to.be.true; expect(mockPushProcessors.checkCommitMessages.called).to.be.true; expect(mockPushProcessors.checkAuthorEmails.called).to.be.true; @@ -190,7 +193,6 @@ describe('proxy chain', function () { expect(mockPushProcessors.pullRemote.called).to.be.true; expect(mockPushProcessors.checkHiddenCommits.called).to.be.true; expect(mockPushProcessors.writePack.called).to.be.true; - expect(mockPushProcessors.getMissingData.called).to.be.false; expect(mockPushProcessors.audit.called).to.be.true; expect(result.type).to.equal('push'); @@ -203,6 +205,7 @@ describe('proxy chain', function () { const continuingAction = { type: 'push', continue: () => true, allowPush: false }; mockPreProcessors.parseAction.resolves({ type: 'push' }); mockPushProcessors.parsePush.resolves(continuingAction); + mockPushProcessors.checkEmptyBranch.resolves(continuingAction); mockPushProcessors.checkRepoInAuthorisedList.resolves(continuingAction); mockPushProcessors.checkCommitMessages.resolves(continuingAction); mockPushProcessors.checkAuthorEmails.resolves(continuingAction); @@ -217,12 +220,12 @@ describe('proxy chain', function () { mockPushProcessors.clearBareClone.resolves(continuingAction); mockPushProcessors.scanDiff.resolves(continuingAction); mockPushProcessors.blockForAuth.resolves(continuingAction); - mockPushProcessors.getMissingData.resolves(continuingAction); const result = await chain.executeChain(req); expect(mockPreProcessors.parseAction.called).to.be.true; expect(mockPushProcessors.parsePush.called).to.be.true; + expect(mockPushProcessors.checkEmptyBranch.called).to.be.true; expect(mockPushProcessors.checkRepoInAuthorisedList.called).to.be.true; expect(mockPushProcessors.checkCommitMessages.called).to.be.true; expect(mockPushProcessors.checkAuthorEmails.called).to.be.true; @@ -238,7 +241,6 @@ describe('proxy chain', function () { expect(mockPushProcessors.scanDiff.called).to.be.true; expect(mockPushProcessors.blockForAuth.called).to.be.true; expect(mockPushProcessors.audit.called).to.be.true; - expect(mockPushProcessors.getMissingData.called).to.be.true; expect(result.type).to.equal('push'); expect(result.allowPush).to.be.false; @@ -299,6 +301,7 @@ describe('proxy chain', function () { mockPreProcessors.parseAction.resolves(action); mockPushProcessors.parsePush.resolves(action); + mockPushProcessors.checkEmptyBranch.resolves(action); mockPushProcessors.checkRepoInAuthorisedList.resolves(action); mockPushProcessors.checkCommitMessages.resolves(action); mockPushProcessors.checkAuthorEmails.resolves(action); @@ -320,7 +323,6 @@ describe('proxy chain', function () { mockPushProcessors.clearBareClone.resolves(action); mockPushProcessors.scanDiff.resolves(action); mockPushProcessors.blockForAuth.resolves(action); - mockPushProcessors.getMissingData.resolves(action); const dbStub = sinon.stub(db, 'authorise').resolves(true); const result = await chain.executeChain(req); @@ -347,6 +349,7 @@ describe('proxy chain', function () { mockPreProcessors.parseAction.resolves(action); mockPushProcessors.parsePush.resolves(action); + mockPushProcessors.checkEmptyBranch.resolves(action); mockPushProcessors.checkRepoInAuthorisedList.resolves(action); mockPushProcessors.checkCommitMessages.resolves(action); mockPushProcessors.checkAuthorEmails.resolves(action); @@ -368,7 +371,6 @@ describe('proxy chain', function () { mockPushProcessors.clearBareClone.resolves(action); mockPushProcessors.scanDiff.resolves(action); mockPushProcessors.blockForAuth.resolves(action); - mockPushProcessors.getMissingData.resolves(action); const dbStub = sinon.stub(db, 'reject').resolves(true); @@ -396,6 +398,7 @@ describe('proxy chain', function () { mockPreProcessors.parseAction.resolves(action); mockPushProcessors.parsePush.resolves(action); + mockPushProcessors.checkEmptyBranch.resolves(action); mockPushProcessors.checkRepoInAuthorisedList.resolves(action); mockPushProcessors.checkCommitMessages.resolves(action); mockPushProcessors.checkAuthorEmails.resolves(action); @@ -417,7 +420,6 @@ describe('proxy chain', function () { mockPushProcessors.clearBareClone.resolves(action); mockPushProcessors.scanDiff.resolves(action); mockPushProcessors.blockForAuth.resolves(action); - mockPushProcessors.getMissingData.resolves(action); const error = new Error('Database error'); @@ -444,6 +446,7 @@ describe('proxy chain', function () { mockPreProcessors.parseAction.resolves(action); mockPushProcessors.parsePush.resolves(action); + mockPushProcessors.checkEmptyBranch.resolves(action); mockPushProcessors.checkRepoInAuthorisedList.resolves(action); mockPushProcessors.checkCommitMessages.resolves(action); mockPushProcessors.checkAuthorEmails.resolves(action); @@ -465,7 +468,6 @@ describe('proxy chain', function () { mockPushProcessors.clearBareClone.resolves(action); mockPushProcessors.scanDiff.resolves(action); mockPushProcessors.blockForAuth.resolves(action); - mockPushProcessors.getMissingData.resolves(action); const error = new Error('Database error'); From 381f00f83c584f3e1ffd93692a16a952b5775d01 Mon Sep 17 00:00:00 2001 From: Juan Escalada Date: Sun, 3 Aug 2025 16:45:34 +0900 Subject: [PATCH 04/11] chore: update inaccurate docstring --- src/proxy/processors/push-action/checkUserPushPermission.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/proxy/processors/push-action/checkUserPushPermission.ts b/src/proxy/processors/push-action/checkUserPushPermission.ts index a1961bcd6..144eb53d3 100644 --- a/src/proxy/processors/push-action/checkUserPushPermission.ts +++ b/src/proxy/processors/push-action/checkUserPushPermission.ts @@ -19,8 +19,7 @@ const exec = async (req: any, action: Action): Promise => { /** * Helper that validates the user's push permission. - * This can be used by other actions that need it. For example, when the user is missing from the commit data, - * validation is deferred to getMissingData, but the logic is the same. + * This can be used by other actions that need it. * @param {string} user The user to validate * @param {Action} action The action object * @param {Step} step The step object From e7951c01bf8e9c63bb83d67b3d73e7b6ac8dda2c Mon Sep 17 00:00:00 2001 From: Juan Escalada Date: Sun, 3 Aug 2025 22:01:03 +0900 Subject: [PATCH 05/11] test: checkEmptyBranch cases --- test/processors/checkEmptyBranch.test.js | 117 +++++++++++++++++++++++ 1 file changed, 117 insertions(+) create mode 100644 test/processors/checkEmptyBranch.test.js diff --git a/test/processors/checkEmptyBranch.test.js b/test/processors/checkEmptyBranch.test.js new file mode 100644 index 000000000..fc63d94aa --- /dev/null +++ b/test/processors/checkEmptyBranch.test.js @@ -0,0 +1,117 @@ +const chai = require('chai'); +const sinon = require('sinon'); +const proxyquire = require('proxyquire'); +const { Action, Step } = require('../../src/proxy/actions'); + +chai.should(); +const expect = chai.expect; + +describe.only('checkEmptyBranch', () => { + let exec; + let simpleGitStub; + let gitRawStub; + + beforeEach(() => { + gitRawStub = sinon.stub(); + simpleGitStub = sinon.stub().callsFake((workingDir) => { + return { + raw: gitRawStub, + cwd: workingDir + }; + }); + + const checkEmptyBranch = proxyquire('../../src/proxy/processors/push-action/checkEmptyBranch', { + 'simple-git': { + default: simpleGitStub, + __esModule: true, + '@global': true, + '@noCallThru': true + }, + // deeply mocking fs to prevent simple-git from validating directories (which fails) + 'fs': { + existsSync: sinon.stub().returns(true), + lstatSync: sinon.stub().returns({ + isDirectory: () => true, + isFile: () => false + }), + '@global': true + } + }); + + exec = checkEmptyBranch.exec; + }); + + afterEach(() => { + sinon.restore(); + }); + + describe('exec', () => { + let action; + let req; + + beforeEach(() => { + req = {}; + action = new Action( + '1234567890', + 'push', + 'POST', + 1234567890, + 'test/repo' + ); + action.proxyGitPath = '/tmp/gitproxy'; + action.repoName = 'test-repo'; + action.commitFrom = '0000000000000000000000000000000000000000'; + action.commitTo = 'abcdef1234567890abcdef1234567890abcdef12'; + action.commitData = []; + }); + + it('should pass through if commitData is already populated', async () => { + action.commitData = [{ message: 'Existing commit' }]; + + const result = await exec(req, action); + + expect(result.steps).to.have.lengthOf(0); + expect(simpleGitStub.called).to.be.false; + }); + + it('should block empty branch pushes with a commit that exists', async () => { + gitRawStub.resolves('commit\n'); + + const result = await exec(req, action); + + expect(simpleGitStub.calledWith('/tmp/gitproxy/test-repo')).to.be.true; + expect(gitRawStub.calledWith(['cat-file', '-t', action.commitTo])).to.be.true; + + const step = result.steps.find(s => s.stepName === 'checkEmptyBranch'); + expect(step).to.exist; + expect(step.error).to.be.true; + expect(step.errorMessage).to.include('Push blocked: Empty branch'); + }); + + it('should block pushes if commitTo does not resolve', async () => { + gitRawStub.rejects(new Error('fatal: Not a valid object name')); + + const result = await exec(req, action); + + expect(gitRawStub.calledWith(['cat-file', '-t', action.commitTo])).to.be.true; + + const step = result.steps.find(s => s.stepName === 'checkEmptyBranch'); + expect(step).to.exist; + expect(step.error).to.be.true; + expect(step.errorMessage).to.include('Push blocked: Commit data not found'); + }); + + it('should block non-empty branch pushes with empty commitData', async () => { + action.commitFrom = 'abcdef1234567890abcdef1234567890abcdef12'; + + const result = await exec(req, action); + + expect(simpleGitStub.called).to.be.true; + + const step = result.steps.find(s => s.stepName === 'checkEmptyBranch'); + expect(step).to.exist; + expect(step.error).to.be.true; + expect(step.errorMessage).to.include('Push blocked: Commit data not found'); + }); + }); +}); From 3a5f600ea8b97b61ae92507501bf64185e63b6bb Mon Sep 17 00:00:00 2001 From: Juan Escalada Date: Sun, 3 Aug 2025 22:04:07 +0900 Subject: [PATCH 06/11] chore: fix linter error --- test/processors/checkEmptyBranch.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/processors/checkEmptyBranch.test.js b/test/processors/checkEmptyBranch.test.js index fc63d94aa..90900d05b 100644 --- a/test/processors/checkEmptyBranch.test.js +++ b/test/processors/checkEmptyBranch.test.js @@ -1,7 +1,7 @@ const chai = require('chai'); const sinon = require('sinon'); const proxyquire = require('proxyquire'); -const { Action, Step } = require('../../src/proxy/actions'); +const { Action } = require('../../src/proxy/actions'); chai.should(); const expect = chai.expect; From f851983ba96f8962dccdf92693b900e918e4125e Mon Sep 17 00:00:00 2001 From: Juan Escalada Date: Sun, 3 Aug 2025 22:08:23 +0900 Subject: [PATCH 07/11] test: add missing user test case for checkUserPushPermission --- test/processors/checkEmptyBranch.test.js | 2 +- test/processors/checkUserPushPermission.test.js | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/test/processors/checkEmptyBranch.test.js b/test/processors/checkEmptyBranch.test.js index 90900d05b..f9a5a8b83 100644 --- a/test/processors/checkEmptyBranch.test.js +++ b/test/processors/checkEmptyBranch.test.js @@ -6,7 +6,7 @@ const { Action } = require('../../src/proxy/actions'); chai.should(); const expect = chai.expect; -describe.only('checkEmptyBranch', () => { +describe('checkEmptyBranch', () => { let exec; let simpleGitStub; let gitRawStub; diff --git a/test/processors/checkUserPushPermission.test.js b/test/processors/checkUserPushPermission.test.js index b140d383b..2aa241023 100644 --- a/test/processors/checkUserPushPermission.test.js +++ b/test/processors/checkUserPushPermission.test.js @@ -98,5 +98,13 @@ describe('checkUserPushPermission', () => { expect(result.steps[0].error).to.be.true; expect(logStub.calledWith('Users for this git account: [{"username":"user1","gitAccount":"git-user"},{"username":"user2","gitAccount":"git-user"}]')).to.be.true; }); + + it('should return error when no user is set in the action', async () => { + action.user = null; + const result = await exec(req, action); + expect(result.steps).to.have.lengthOf(1); + expect(result.steps[0].error).to.be.true; + expect(result.steps[0].errorMessage).to.include('Push blocked: User not found. Please contact an administrator for support.'); + }); }); }); From 7d67818d6277f199613ebed649fe259628da8882 Mon Sep 17 00:00:00 2001 From: Juan Escalada <97265671+jescalada@users.noreply.github.com> Date: Wed, 6 Aug 2025 01:06:40 +0000 Subject: [PATCH 08/11] Update test/processors/checkCommitMessages.test.js Co-authored-by: Kris West Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com> --- test/processors/checkCommitMessages.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/processors/checkCommitMessages.test.js b/test/processors/checkCommitMessages.test.js index 1c6526590..5eeb0fff2 100644 --- a/test/processors/checkCommitMessages.test.js +++ b/test/processors/checkCommitMessages.test.js @@ -99,7 +99,7 @@ describe('checkCommitMessages', () => { it('should not error when commit data is empty', async () => { // Empty commit data happens when making a branch from an unapproved commit // or when pushing an empty branch or deleting a branch - // This is remedied in the checkEmptyBranch.exec action + // This is handled in the checkEmptyBranch.exec action action.commitData = []; const result = await exec(req, action); From 028436d90aa868cc93e69c63e22b834593b12068 Mon Sep 17 00:00:00 2001 From: Juan Escalada <97265671+jescalada@users.noreply.github.com> Date: Wed, 6 Aug 2025 01:06:55 +0000 Subject: [PATCH 09/11] Update src/proxy/processors/push-action/checkEmptyBranch.ts Co-authored-by: Thomas Cooper <57812123+coopernetes@users.noreply.github.com> Signed-off-by: Juan Escalada <97265671+jescalada@users.noreply.github.com> --- src/proxy/processors/push-action/checkEmptyBranch.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/proxy/processors/push-action/checkEmptyBranch.ts b/src/proxy/processors/push-action/checkEmptyBranch.ts index 61895db15..211e9c591 100644 --- a/src/proxy/processors/push-action/checkEmptyBranch.ts +++ b/src/proxy/processors/push-action/checkEmptyBranch.ts @@ -8,10 +8,7 @@ const isEmptyBranch = async (action: Action) => { if (action.commitFrom === EMPTY_COMMIT_HASH) { try { const type = await git.raw(['cat-file', '-t', action.commitTo || '']); - const known = type.trim() === 'commit'; - if (known) { - return true; - } + return type.trim() === 'commit'; } catch (err) { console.log(`Commit ${action.commitTo} not found: ${err}`); } From ab8cf319641a7de054e515a559b40af840b1a2a8 Mon Sep 17 00:00:00 2001 From: Juan Escalada Date: Wed, 6 Aug 2025 11:29:57 +0900 Subject: [PATCH 10/11] chore: optimize simpleGit call --- src/proxy/processors/push-action/checkEmptyBranch.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/proxy/processors/push-action/checkEmptyBranch.ts b/src/proxy/processors/push-action/checkEmptyBranch.ts index 211e9c591..4634c391d 100644 --- a/src/proxy/processors/push-action/checkEmptyBranch.ts +++ b/src/proxy/processors/push-action/checkEmptyBranch.ts @@ -3,10 +3,10 @@ import simpleGit from 'simple-git'; import { EMPTY_COMMIT_HASH } from '../constants'; const isEmptyBranch = async (action: Action) => { - const git = simpleGit(`${action.proxyGitPath}/${action.repoName}`); - if (action.commitFrom === EMPTY_COMMIT_HASH) { try { + const git = simpleGit(`${action.proxyGitPath}/${action.repoName}`); + const type = await git.raw(['cat-file', '-t', action.commitTo || '']); return type.trim() === 'commit'; } catch (err) { From 0d5b5aaa155194542af4985f1443b4be460294d0 Mon Sep 17 00:00:00 2001 From: Juan Escalada Date: Wed, 6 Aug 2025 11:53:37 +0900 Subject: [PATCH 11/11] chore: fix failing test --- test/processors/checkEmptyBranch.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/processors/checkEmptyBranch.test.js b/test/processors/checkEmptyBranch.test.js index f9a5a8b83..3aba831d0 100644 --- a/test/processors/checkEmptyBranch.test.js +++ b/test/processors/checkEmptyBranch.test.js @@ -106,7 +106,7 @@ describe('checkEmptyBranch', () => { const result = await exec(req, action); - expect(simpleGitStub.called).to.be.true; + expect(simpleGitStub.called).to.be.false; const step = result.steps.find(s => s.stepName === 'checkEmptyBranch'); expect(step).to.exist;