Merge pull request #1209 from fabiovincenzi/duplicate-executechain

refactor: eliminate duplicate executeChain calls in push approval flow

Author: kriswest

src/proxy/routes/index.ts

@@ -48,6 +48,13 @@ const proxyFilter: ProxyOptions['filter'] = async (req, res) => {
       return false;
     }
 
+    // For POST pack requests, use the raw body extracted by extractRawBody middleware
+    if (isPackPost(req) && (req as any).bodyRaw) {
+      (req as any).body = (req as any).bodyRaw;
+      // Clean up the bodyRaw property before forwarding the request
+      delete (req as any).bodyRaw;
+    }
+
     const action = await executeChain(req, res);
 
     if (action.error || action.blocked) {
@@ -131,7 +138,7 @@ const proxyReqBodyDecorator: ProxyOptions['proxyReqBodyDecorator'] = (bodyConten
   return bodyContent;
 };
 
-const proxyErrorHandler: ProxyOptions['proxyErrorHandler'] = (err, res, next) => {
+const proxyErrorHandler: ProxyOptions['proxyErrorHandler'] = (err, _res, next) => {
   console.log(`ERROR=${err}`);
   next(err);
 };
@@ -140,42 +147,24 @@ const isPackPost = (req: Request) =>
   req.method === 'POST' &&
   /^(?:\/[^/]+)*\/[^/]+\.git\/(?:git-upload-pack|git-receive-pack)$/.test(req.url);
 
-const teeAndValidate = async (req: Request, res: Response, next: NextFunction) => {
+const extractRawBody = async (req: Request, res: Response, next: NextFunction) => {
   if (!isPackPost(req)) {
     return next();
   }
 
-  const proxyStream = new PassThrough();
-  const pluginStream = new PassThrough();
+  const proxyStream = new PassThrough({
+    highWaterMark: 4 * 1024 * 1024,
+  });
+  const pluginStream = new PassThrough({
+    highWaterMark: 4 * 1024 * 1024,
+  });
 
   req.pipe(proxyStream);
   req.pipe(pluginStream);
 
   try {
     const buf = await getRawBody(pluginStream, { limit: '1gb' });
-    (req as any).body = buf;
-    const verdict = await executeChain(req, res);
-    if (verdict.error || verdict.blocked) {
-      const message = verdict.errorMessage ?? verdict.blockedMessage ?? 'Unknown error';
-      const type = verdict.error ? ActionType.ERROR : ActionType.BLOCKED;
-
-      logAction(req.url, req.headers?.host, req.headers?.['user-agent'], type, message);
-
-      res
-        .set({
-          'content-type': 'application/x-git-receive-pack-result',
-          expires: 'Fri, 01 Jan 1980 00:00:00 GMT',
-          pragma: 'no-cache',
-          'cache-control': 'no-cache, max-age=0, must-revalidate',
-          vary: 'Accept-Encoding',
-          'x-frame-options': 'DENY',
-          connection: 'close',
-        })
-        .status(200) // return status 200 to ensure that the error message is rendered by the git client
-        .send(handleMessage(message));
-      return;
-    }
-
+    (req as any).bodyRaw = buf;
     (req as any).pipe = (dest: any, opts: any) => proxyStream.pipe(dest, opts);
     next();
   } catch (e) {
@@ -187,7 +176,7 @@ const teeAndValidate = async (req: Request, res: Response, next: NextFunction) =
 
 const getRouter = async () => {
   const router = Router();
-  router.use(teeAndValidate);
+  router.use(extractRawBody);
 
   const originsToProxy = await getAllProxiedHosts();
   const proxyKeys: string[] = [];
@@ -262,6 +251,6 @@ export {
   handleMessage,
   handleRefsErrorMessage,
   isPackPost,
-  teeAndValidate,
+  extractRawBody,
   validGitRequest,
 };

test/teeAndValidation.test.js renamed to test/extractRawBody.test.js

@@ -9,12 +9,12 @@ const fakeChain = {
   executeChain: sinon.stub(),
 };
 
-const { teeAndValidate, isPackPost, handleMessage } = proxyquire('../src/proxy/routes', {
+const { extractRawBody, isPackPost } = proxyquire('../src/proxy/routes', {
   'raw-body': fakeRawBody,
   '../chain': fakeChain,
 });
 
-describe('teeAndValidate middleware', () => {
+describe('extractRawBody middleware', () => {
   let req;
   let res;
   let next;
@@ -38,39 +38,21 @@ describe('teeAndValidate middleware', () => {
 
   it('skips non-pack posts', async () => {
     req.method = 'GET';
-    await teeAndValidate(req, res, next);
+    await extractRawBody(req, res, next);
     expect(next.calledOnce).to.be.true;
     expect(fakeRawBody.called).to.be.false;
   });
 
-  it('when the chain blocks it sends a packet and does NOT call next()', async () => {
-    fakeChain.executeChain.resolves({ blocked: true, blockedMessage: 'denied!' });
-
-    req.write('abcd');
-    req.end();
-
-    await teeAndValidate(req, res, next);
-
-    expect(fakeRawBody.calledOnce).to.be.true;
-    expect(fakeChain.executeChain.calledOnce).to.be.true;
-    expect(next.called).to.be.false;
-
-    expect(res.set.called).to.be.true;
-    expect(res.status.calledWith(200)).to.be.true; // status 200 is used to ensure error message is rendered by git client
-    expect(res.send.calledWith(handleMessage('denied!'))).to.be.true;
-  });
-
-  it('when the chain allow it calls next() and overrides req.pipe', async () => {
-    fakeChain.executeChain.resolves({ blocked: false, error: false });
-
+  it('extracts raw body and sets bodyRaw property', async () => {
     req.write('abcd');
     req.end();
 
-    await teeAndValidate(req, res, next);
+    await extractRawBody(req, res, next);
 
     expect(fakeRawBody.calledOnce).to.be.true;
-    expect(fakeChain.executeChain.calledOnce).to.be.true;
+    expect(fakeChain.executeChain.called).to.be.false;
     expect(next.calledOnce).to.be.true;
+    expect(req.bodyRaw).to.exist;
     expect(typeof req.pipe).to.equal('function');
   });
 });