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..4634c391d --- /dev/null +++ b/src/proxy/processors/push-action/checkEmptyBranch.ts @@ -0,0 +1,42 @@ +import { Action, Step } from '../../actions'; +import simpleGit from 'simple-git'; +import { EMPTY_COMMIT_HASH } from '../constants'; + +const isEmptyBranch = async (action: Action) => { + 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) { + 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..144eb53d3 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; } @@ -17,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 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/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'); diff --git a/test/processors/checkCommitMessages.test.js b/test/processors/checkCommitMessages.test.js index 75156e0ae..5eeb0fff2 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 handled in the checkEmptyBranch.exec action action.commitData = []; const result = await exec(req, action); diff --git a/test/processors/checkEmptyBranch.test.js b/test/processors/checkEmptyBranch.test.js new file mode 100644 index 000000000..3aba831d0 --- /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 } = require('../../src/proxy/actions'); + +chai.should(); +const expect = chai.expect; + +describe('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.false; + + 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'); + }); + }); +}); 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.'); + }); }); }); 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', () => {