diff --git a/src/proxy/chain.ts b/src/proxy/chain.ts index f504476be..5aeac2d96 100644 --- a/src/proxy/chain.ts +++ b/src/proxy/chain.ts @@ -27,24 +27,29 @@ const pullActionChain: ((req: any, action: Action) => Promise)[] = [ proc.push.checkRepoInAuthorisedList, ]; +const defaultActionChain: ((req: any, action: Action) => Promise)[] = [ + proc.push.checkRepoInAuthorisedList, +]; + let pluginsInserted = false; export const executeChain = async (req: any, res: any): Promise => { let action: Action = {} as Action; + try { action = await proc.pre.parseAction(req); const actionFns = await getChain(action); for (const fn of actionFns) { action = await fn(req, action); - if (!action.continue()) { - return action; - } - - if (action.allowPush) { - return action; + if (!action.continue() || action.allowPush) { + break; } } + } catch (e) { + action.error = true; + action.errorMessage = `An error occurred when executing the chain: ${e}`; + console.error(action.errorMessage); } finally { await proc.push.audit(req, action); if (action.autoApproved) { @@ -89,9 +94,13 @@ export const getChain = async ( // This is set to true so that we don't re-insert the plugins into the chain pluginsInserted = true; } - if (action.type === 'pull') return pullActionChain; - if (action.type === 'push') return pushActionChain; - return []; + if (action.type === 'pull') { + return pullActionChain; + } else if (action.type === 'push') { + return pushActionChain; + } else { + return defaultActionChain; + } }; export default { @@ -110,6 +119,9 @@ export default { get pullActionChain() { return pullActionChain; }, + get defaultActionChain() { + return defaultActionChain; + }, executeChain, getChain, }; diff --git a/src/proxy/processors/pre-processor/parseAction.ts b/src/proxy/processors/pre-processor/parseAction.ts index ddf854caf..0707d9240 100644 --- a/src/proxy/processors/pre-processor/parseAction.ts +++ b/src/proxy/processors/pre-processor/parseAction.ts @@ -14,8 +14,7 @@ const exec = async (req: { if (pathBreakdown) { if (pathBreakdown.gitPath.endsWith('git-upload-pack') && req.method === 'GET') { type = 'pull'; - } - if ( + } else if ( pathBreakdown.gitPath.includes('git-receive-pack') && req.method === 'POST' && req.headers['content-type'] === 'application/x-git-receive-pack-request' diff --git a/src/proxy/routes/index.ts b/src/proxy/routes/index.ts index 211542851..997e9a2d2 100644 --- a/src/proxy/routes/index.ts +++ b/src/proxy/routes/index.ts @@ -29,14 +29,19 @@ const logAction = ( const proxyFilter: ProxyOptions['filter'] = async (req, res) => { try { const urlComponents = processUrlPath(req.url); - if ( !urlComponents || urlComponents.gitPath === undefined || !validGitRequest(urlComponents.gitPath, req.headers) ) { - res.status(400).send('Invalid request received'); - console.log('action blocked'); + logAction( + req.url, + req.headers.host, + req.headers['user-agent'], + 'Invalid request received', + null, + ); + res.status(400).send(handleMessage('Invalid request received')); return false; } @@ -51,17 +56,7 @@ const proxyFilter: ProxyOptions['filter'] = async (req, res) => { res.set('x-frame-options', 'DENY'); res.set('connection', 'close'); - let message = ''; - - if (action.error) { - message = action.errorMessage!; - console.error(message); - } - if (action.blocked) { - message = action.blockedMessage!; - } - - const packetMessage = handleMessage(message); + const packetMessage = handleMessage(action.errorMessage ?? action.blockedMessage ?? ''); logAction( req.url, @@ -70,9 +65,7 @@ const proxyFilter: ProxyOptions['filter'] = async (req, res) => { action.errorMessage, action.blockedMessage, ); - - res.status(200).send(packetMessage); - + res.status(403).send(packetMessage); return false; } @@ -84,9 +77,11 @@ const proxyFilter: ProxyOptions['filter'] = async (req, res) => { action.blockedMessage, ); + // this is the only case where we do not respond directly, instead we return true to proxy the request return true; } catch (e) { - console.error('Error occurred in proxy filter function ', e); + const packetMessage = handleMessage(`Error occurred in proxy filter function ${e}`); + logAction( req.url, req.headers.host, @@ -94,6 +89,8 @@ const proxyFilter: ProxyOptions['filter'] = async (req, res) => { 'Error occurred in proxy filter function: ' + ((e as Error).message ?? e), null, ); + + res.status(500).send(packetMessage); return false; } }; @@ -140,7 +137,9 @@ const isPackPost = (req: Request) => /^(?:\/[^/]+)*\/[^/]+\.git\/(?:git-upload-pack|git-receive-pack)$/.test(req.url); const teeAndValidate = async (req: Request, res: Response, next: NextFunction) => { - if (!isPackPost(req)) return next(); + if (!isPackPost(req)) { + return next(); + } const proxyStream = new PassThrough(); const pluginStream = new PassThrough(); @@ -152,17 +151,16 @@ const teeAndValidate = async (req: Request, res: Response, next: NextFunction) = const buf = await getRawBody(pluginStream, { limit: '1gb' }); (req as any).body = buf; const verdict = await executeChain(req, res); - console.log('action processed'); if (verdict.error || verdict.blocked) { - let msg = ''; + const msg = verdict.errorMessage ?? verdict.blockedMessage ?? ''; - if (verdict.error) { - msg = verdict.errorMessage!; - console.error(msg); - } - if (verdict.blocked) { - msg = verdict.blockedMessage!; - } + logAction( + req.url, + req.headers?.host, + req.headers?.['user-agent'], + verdict.errorMessage, + verdict.blockedMessage, + ); res .set({ @@ -174,7 +172,7 @@ const teeAndValidate = async (req: Request, res: Response, next: NextFunction) = 'x-frame-options': 'DENY', connection: 'close', }) - .status(200) + .status(403) .send(handleMessage(msg)); return; } @@ -233,8 +231,9 @@ const getRouter = async () => { console.log('proxy keys registered: ', JSON.stringify(proxyKeys)); router.use('/', (req, res, next) => { - console.log(`processing request URL: '${req.url}'`); - console.log('proxy keys registered: ', JSON.stringify(proxyKeys)); + console.log( + `processing request URL: '${req.url}' against registered proxy keys: ${JSON.stringify(proxyKeys)}`, + ); for (let i = 0; i < proxyKeys.length; i++) { if (req.url.startsWith(proxyKeys[i])) { diff --git a/test/chain.test.js b/test/chain.test.js index 1d00d1b7f..8f4b180d1 100644 --- a/test/chain.test.js +++ b/test/chain.test.js @@ -64,7 +64,7 @@ let mockPushProcessors; const clearCache = (sandbox) => { delete require.cache[require.resolve('../src/proxy/processors')]; delete require.cache[require.resolve('../src/proxy/chain')]; - sandbox.reset(); + sandbox.restore(); }; describe('proxy chain', function () { @@ -275,17 +275,15 @@ describe('proxy chain', function () { expect(mockPushProcessors.audit.called).to.be.true; }); - it('executeChain should run no actions if not a push or pull', async function () { + it('executeChain should always run at least checkRepoInAuthList', async function () { const req = {}; const action = { type: 'foo', continue: () => true, allowPush: true }; - processors.pre.parseAction.resolves(action); - - const result = await chain.executeChain(req); + mockPreProcessors.parseAction.resolves(action); + mockPushProcessors.checkRepoInAuthorisedList.resolves(action); - expect(mockPushProcessors.checkRepoInAuthorisedList.called).to.be.false; - expect(mockPushProcessors.parsePush.called).to.be.false; - expect(result).to.deep.equal(action); + await chain.executeChain(req); + expect(mockPushProcessors.checkRepoInAuthorisedList.called).to.be.true; }); it('should approve push automatically and record in the database', async function () { diff --git a/test/teeAndValidation.test.js b/test/teeAndValidation.test.js index 3c5bf5da7..aa75b59c6 100644 --- a/test/teeAndValidation.test.js +++ b/test/teeAndValidation.test.js @@ -56,7 +56,7 @@ describe('teeAndValidate middleware', () => { expect(next.called).to.be.false; expect(res.set.called).to.be.true; - expect(res.status.calledWith(200)).to.be.true; + expect(res.status.calledWith(403)).to.be.true; expect(res.send.calledWith(handleMessage('denied!'))).to.be.true; }); diff --git a/test/testProxyRoute.test.js b/test/testProxyRoute.test.js index a4768e21b..d0e1a6361 100644 --- a/test/testProxyRoute.test.js +++ b/test/testProxyRoute.test.js @@ -18,8 +18,9 @@ import Proxy from '../src/proxy'; const TEST_DEFAULT_REPO = { url: 'https://github.com/finos/git-proxy.git', name: 'git-proxy', - project: 'finos/gitproxy', + project: 'finos/git-proxy', host: 'github.com', + proxyUrlPrefix: '/github.com/finos/git-proxy.git', }; const TEST_GITLAB_REPO = { @@ -27,7 +28,16 @@ const TEST_GITLAB_REPO = { name: 'gitlab', project: 'gitlab-community/meta', host: 'gitlab.com', - proxyUrlPrefix: 'gitlab.com/gitlab-community/meta.git', + proxyUrlPrefix: '/gitlab.com/gitlab-community/meta.git', +}; + +const TEST_UNKNOWN_REPO = { + url: 'https://github.com/finos/fdc3.git', + name: 'fdc3', + project: 'finos/fdc3', + host: 'github.com', + proxyUrlPrefix: '/github.com/finos/fdc3.git', + fallbackUrlPrefix: '/finos/fdc3.git', }; describe('proxy route filter middleware', () => { @@ -42,6 +52,10 @@ describe('proxy route filter middleware', () => { sinon.restore(); }); + after(() => { + sinon.restore(); + }); + it('should reject invalid git requests with 400', async () => { const res = await chai .request(app) @@ -50,7 +64,7 @@ describe('proxy route filter middleware', () => { .set('accept', 'application/x-git-upload-pack-request'); expect(res).to.have.status(400); - expect(res.text).to.equal('Invalid request received'); + expect(res.text).to.contain('Invalid request received'); }); it('should handle blocked requests and return custom packet message', async () => { @@ -68,7 +82,7 @@ describe('proxy route filter middleware', () => { .send(Buffer.from('0000')) .buffer(); - expect(res.status).to.equal(200); + expect(res.status).to.equal(403); expect(res.text).to.contain('You shall not push!'); expect(res.headers['content-type']).to.include('application/x-git-receive-pack-result'); expect(res.headers['x-frame-options']).to.equal('DENY'); @@ -321,13 +335,6 @@ describe('proxy express application', async () => { }; before(async () => { - // pass through requests - sinon.stub(chain, 'executeChain').resolves({ - blocked: false, - blockedMessage: '', - error: false, - }); - // start the API and proxy proxy = new Proxy(); apiApp = await service.start(proxy); @@ -364,7 +371,7 @@ describe('proxy express application', async () => { // proxy a fetch request const res = await chai .request(proxy.getExpressApp()) - .get('/github.com/finos/git-proxy.git/info/refs?service=git-upload-pack') + .get(`${TEST_DEFAULT_REPO.proxyUrlPrefix}/info/refs?service=git-upload-pack`) .set('user-agent', 'git/2.42.0') .set('accept', 'application/x-git-upload-pack-request') .buffer(); @@ -373,11 +380,11 @@ describe('proxy express application', async () => { expect(res.text).to.contain('git-upload-pack'); }); - it('should proxy requests for the default GitHub repository using the backwards compatibility URL', async function () { + it('should proxy requests for the default GitHub repository using the fallback URL', async function () { // proxy a fetch request using a fallback URL const res = await chai .request(proxy.getExpressApp()) - .get('/finos/git-proxy.git/info/refs?service=git-upload-pack') + .get(`${TEST_DEFAULT_REPO.proxyUrlPrefix}/info/refs?service=git-upload-pack`) .set('user-agent', 'git/2.42.0') .set('accept', 'application/x-git-upload-pack-request') .buffer(); @@ -415,7 +422,7 @@ describe('proxy express application', async () => { // proxy a request to the new repo const res2 = await chai .request(proxy.getExpressApp()) - .get(`/${TEST_GITLAB_REPO.proxyUrlPrefix}/info/refs?service=git-upload-pack`) + .get(`${TEST_GITLAB_REPO.proxyUrlPrefix}/info/refs?service=git-upload-pack`) .set('user-agent', 'git/2.42.0') .set('accept', 'application/x-git-upload-pack-request') .buffer(); @@ -442,11 +449,11 @@ describe('proxy express application', async () => { res.should.have.status(200); // confirm that its gone from the DB - repo = await db.getRepoByUrl( - TEST_GITLAB_REPO.url, + repo = await db.getRepoByUrl(TEST_GITLAB_REPO.url); + expect( + repo, 'The GitLab repo still existed in the database after it should have been deleted...', - ); - expect(repo).to.be.null; + ).to.be.null; // give the proxy half a second to restart await new Promise((resolve) => setTimeout(resolve, 500)); @@ -454,11 +461,41 @@ describe('proxy express application', async () => { // try (and fail) to proxy a request to gitlab.com const res2 = await chai .request(proxy.getExpressApp()) - .get(`/${TEST_GITLAB_REPO.proxyUrlPrefix}/info/refs?service=git-upload-pack`) + .get(`${TEST_GITLAB_REPO.proxyUrlPrefix}/info/refs?service=git-upload-pack`) .set('user-agent', 'git/2.42.0') .set('accept', 'application/x-git-upload-pack-request') .buffer(); - res2.should.have.status(404); + res2.should.have.status(403); + }).timeout(5000); + + it('should not proxy requests for an unknown project', async function () { + // We are testing that the proxy stops proxying requests for a particular origin + // The chain is stubbed and will always passthrough requests, hence, we are only checking what hosts are proxied. + + // the gitlab test repo should already exist + const repo = await db.getRepoByUrl(TEST_UNKNOWN_REPO.url); + expect( + repo, + 'The unknown (but real) repo existed in the database which is not expected for this test', + ).to.be.null; + + // try (and fail) to proxy a request to the repo directly + const res = await chai + .request(proxy.getExpressApp()) + .get(`${TEST_UNKNOWN_REPO.proxyUrlPrefix}/info/refs?service=git-upload-pack`) + .set('user-agent', 'git/2.42.0') + .set('accept', 'application/x-git-upload-pack-request') + .buffer(); + res.should.have.status(403); + + // try (and fail) to proxy a request to the repo via the fallback URL directly + const res2 = await chai + .request(proxy.getExpressApp()) + .get(`${TEST_UNKNOWN_REPO.fallbackUrlPrefix}/info/refs?service=git-upload-pack`) + .set('user-agent', 'git/2.42.0') + .set('accept', 'application/x-git-upload-pack-request') + .buffer(); + res2.should.have.status(403); }).timeout(5000); });