From 648e49230546355feec269064628717e54107edd Mon Sep 17 00:00:00 2001 From: Juan Escalada Date: Thu, 9 Jul 2026 13:44:37 +0900 Subject: [PATCH] feat: implement disconnect detection, cancel pushes on disconnect --- src/proxy/chain.ts | 36 ++++++++++++++++- src/proxy/routes/index.ts | 14 +++++++ src/proxy/ssh/server.ts | 6 +++ src/proxy/ssh/types.ts | 2 + test/chain.test.ts | 84 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 141 insertions(+), 1 deletion(-) diff --git a/src/proxy/chain.ts b/src/proxy/chain.ts index ba58b3f4c..fdf2e23c6 100644 --- a/src/proxy/chain.ts +++ b/src/proxy/chain.ts @@ -17,7 +17,7 @@ import { Request, Response } from 'express'; import { PluginLoader } from '../plugin'; -import { Action, RequestType, PushType } from './actions'; +import { Action, Step, RequestType, PushType } from './actions'; import * as proc from './processors'; import { Processor } from './processors/types'; import { attemptAutoApproval, attemptAutoRejection } from './actions/autoActions'; @@ -57,6 +57,28 @@ const defaultActionChain: Processor['exec'][] = [proc.push.checkRepoInAuthorised let pluginsInserted = false; +const CLIENT_DISCONNECT_MESSAGE = + 'Client disconnected while the push was being processed. Push canceled.'; + +/** + * Detect whether the git client that initiated this request has disconnected + * + * For HTTPS, pack data is fully buffered before the chain runs, + * so we check for a destroyed socket (couldn't have been delivered anyway) + * + * For SSH we flag the ssh2 connection as disconnected by the SSH server + * on connection end/close events. + * @param {Request} req The request being processed by the chain. + * @return {boolean} true when the client is known to be gone. + */ +const isClientDisconnected = (req: Request): boolean => { + const sshReq = req as Request & { isSSH?: boolean; sshClient?: { disconnected?: boolean } }; + if (sshReq.isSSH) { + return sshReq.sshClient?.disconnected === true; + } + return req.socket?.destroyed === true; +}; + export const executeChain = async (req: Request, _res: Response): Promise => { let action: Action = {} as Action; let checkoutCleanUpRequired = false; @@ -73,6 +95,18 @@ export const executeChain = async (req: Request, _res: Response): Promise { return false; } + // the checks passed, but if the client vanished in the meantime (SIGINT, etc.) + // the push must not be forwarded on their behalf + if (req.socket?.destroyed) { + const message = 'Client disconnected before the push could be forwarded'; + logAction(req.url, req.headers.host, req.headers['user-agent'], ActionType.ERROR, message); + return false; + } + logAction(req.url, req.headers.host, req.headers['user-agent'], ActionType.ALLOWED); // this is the only case where we do not respond directly, instead we return true to proxy the request @@ -100,6 +108,12 @@ const proxyFilter: ProxyOptions['filter'] = async (req, res) => { }; const sendErrorResponse = (req: Request, res: Response, message: string): void => { + // nobody is listening on a destroyed socket (e.g. git push killed mid-flight) + if (req.socket?.destroyed) { + console.log('Client disconnected before the response could be sent, skipping response'); + return; + } + // GET requests to /info/refs (used to check refs for many git operations) must use Git protocol error packet format if (req.method === 'GET' && req.url.includes('/info/refs')) { res.set('content-type', 'application/x-git-upload-pack-advertisement'); diff --git a/src/proxy/ssh/server.ts b/src/proxy/ssh/server.ts index dc0b3691d..b00c37eeb 100644 --- a/src/proxy/ssh/server.ts +++ b/src/proxy/ssh/server.ts @@ -181,11 +181,13 @@ export class SSHServer { client.on('end', () => { console.log(`[SSH] Client disconnected from ${clientIp}`); + clientWithUser.disconnected = true; clearTimeout(connectionTimeout); }); client.on('close', () => { console.log(`[SSH] Client connection closed from ${clientIp}`); + clientWithUser.disconnected = true; clearTimeout(connectionTimeout); }); @@ -600,6 +602,10 @@ export class SSHServer { `[SSH] Chain execution failed for user ${client.authenticatedUser?.username}:`, chainError, ); + if (client.disconnected) { + console.log('[SSH] Client already disconnected, skipping error response'); + return; + } const errorMessage = chainError instanceof Error ? chainError.message : String(chainError); stream.stderr.write(`Access denied: ${errorMessage}\n`); stream.exit(1); diff --git a/src/proxy/ssh/types.ts b/src/proxy/ssh/types.ts index 70089a143..878ee9eca 100644 --- a/src/proxy/ssh/types.ts +++ b/src/proxy/ssh/types.ts @@ -70,4 +70,6 @@ export interface ClientWithUser extends ssh2.Connection, SSH2ConnectionInternals agentForwardingEnabled?: boolean; agentChannel?: ssh2.Channel; agentProxy?: SSHAgentProxy; + /** Set when the client connection ends/closes, so in-flight chains can cancel. */ + disconnected?: boolean; } diff --git a/test/chain.test.ts b/test/chain.test.ts index 215da7632..14706f19b 100644 --- a/test/chain.test.ts +++ b/test/chain.test.ts @@ -454,6 +454,90 @@ describe('proxy chain', function () { expect(consoleErrorSpy).toHaveBeenCalledWith('Error during auto-rejection: Database error'); }); + describe('client disconnect detection', () => { + const newPushAction = () => { + const action = new Action( + 'disconnect-test', + RequestType.PUSH, + 'POST', + Date.now(), + 'https://github.com/owner/repo.git', + ); + mockPreProcessors.parseAction.mockResolvedValue(action); + mockPreProcessors.parsePush.mockResolvedValue(action); + return action; + }; + + it('should cancel the push when the HTTP client has already disconnected', async () => { + newPushAction(); + const req = { socket: { destroyed: true } }; + + const result = await chain.executeChain(req); + + expect(result.canceled).toBe(true); + expect(result.error).toBe(true); + expect(result.errorMessage).toContain('Client disconnected'); + + // no checks ran and the push was not queued for approval + expect(mockPushProcessors.checkRepoInAuthorisedList).not.toHaveBeenCalled(); + expect(mockPushProcessors.blockForAuth).not.toHaveBeenCalled(); + + // the canceled push is still audited + expect(mockPostProcessors.audit).toHaveBeenCalled(); + }); + + it('should cancel the push when the HTTP client disconnects mid-chain', async () => { + newPushAction(); + const req: any = { socket: { destroyed: false } }; + + // simulate the client vanishing while a check is running + mockPushProcessors.checkUserPushPermission.mockImplementation(async (r: any, action: any) => { + r.socket.destroyed = true; + return action; + }); + + const result = await chain.executeChain(req); + + expect(result.canceled).toBe(true); + expect(result.error).toBe(true); + + // steps before the disconnect ran, later steps did not + expect(mockPushProcessors.checkUserPushPermission).toHaveBeenCalled(); + expect(mockPushProcessors.pullRemote).not.toHaveBeenCalled(); + expect(mockPushProcessors.scanDiff).not.toHaveBeenCalled(); + expect(mockPushProcessors.blockForAuth).not.toHaveBeenCalled(); + expect(mockPostProcessors.audit).toHaveBeenCalled(); + }); + + it('should cancel the push when the SSH client disconnects mid-chain', async () => { + newPushAction(); + const sshClient = { disconnected: false }; + const req: any = { isSSH: true, sshClient }; + + mockPushProcessors.checkMessages.mockImplementation(async (r: any, action: any) => { + sshClient.disconnected = true; + return action; + }); + + const result = await chain.executeChain(req); + + expect(result.canceled).toBe(true); + expect(result.error).toBe(true); + expect(mockPushProcessors.checkAuthorEmails).not.toHaveBeenCalled(); + expect(mockPushProcessors.blockForAuth).not.toHaveBeenCalled(); + }); + + it('should not cancel pushes for connected clients', async () => { + newPushAction(); + const req = { socket: { destroyed: false } }; + + const result = await chain.executeChain(req); + + expect(result.canceled).toBe(false); + expect(mockPushProcessors.blockForAuth).toHaveBeenCalled(); + }); + }); + it('returns pullActionChain for pull actions', async () => { const action = new Action( '1',