Merge pull request finos#1164 from kriswest/1163-dont-forward-unknown-repos

feat: don't forward requests for unknown repos

Author: jescalada

src/proxy/chain.ts

@@ -27,24 +27,29 @@ const pullActionChain: ((req: any, action: Action) => Promise<Action>)[] = [
   proc.push.checkRepoInAuthorisedList,
 ];
 
+const defaultActionChain: ((req: any, action: Action) => Promise<Action>)[] = [
+  proc.push.checkRepoInAuthorisedList,
+];
+
 let pluginsInserted = false;
 
 export const executeChain = async (req: any, res: any): Promise<Action> => {
   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,
 };

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'

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,16 +77,20 @@ 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,
       req.headers['user-agent'],
       '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])) {

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 () {

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;
   });