diff --git a/src/graph/graphWebviewProvider.ts b/src/graph/graphWebviewProvider.ts index 4aecc42..81291fc 100644 --- a/src/graph/graphWebviewProvider.ts +++ b/src/graph/graphWebviewProvider.ts @@ -13,6 +13,7 @@ import { ApiKeyManager } from '../mcp/apiKeyManager'; import { AdaptiveDebouncer } from '../git/branchWatcher'; import { readGitMessageTemplate } from '../git/conventionalCommits'; import { RemoteManager } from '../remotes/remoteManager'; +import { IpcSchemaValidator } from '../utils/ipcValidator'; import { CommitFlowSettings, DagQueryFilters, @@ -397,6 +398,12 @@ export class GraphWebviewProvider private setupMessageListener(webview: vscode.Webview): void { webview.onDidReceiveMessage(async (msg: WebviewToExtensionMessage) => { + const validation = IpcSchemaValidator.validateWebviewMessage(msg); + if (!validation.valid) { + console.warn(`[CommitFlow IPC] Rejected message: ${validation.reason}`); + return; + } + const root = await this.getEffectiveRepoRoot(); switch (msg.command) { diff --git a/src/mcp/commitflowHttpServer.ts b/src/mcp/commitflowHttpServer.ts index 83de560..37d2516 100644 --- a/src/mcp/commitflowHttpServer.ts +++ b/src/mcp/commitflowHttpServer.ts @@ -1,29 +1,68 @@ import * as http from 'http'; import { ApiKeyManager } from './apiKeyManager'; import { CommitFlowMcpServer } from './commitflowMcpServer'; +import { McpAuditLogEntry, sharedMcpAuditLog } from './mcpAuditLog'; export interface HttpServerOptions { port?: number; host?: string; repoRoot?: string; + rateLimitMax?: number; + rateLimitWindowMs?: number; } export class CommitFlowHttpServer { private server: http.Server | null = null; private readonly apiKeyManager = ApiKeyManager.getInstance(); - private readonly port: number; + private port: number; private readonly host: string = '127.0.0.1'; private readonly repoRoot: string; + private readonly rateLimitMax: number; + private readonly rateLimitWindowMs: number; + private readonly rateLimitBuckets = new Map(); private mcpEngine: CommitFlowMcpServer; private sseClients: Set = new Set(); constructor(options?: HttpServerOptions) { - this.port = options?.port || 47291; + this.port = options?.port ?? 47291; this.host = '127.0.0.1'; this.repoRoot = options?.repoRoot || process.cwd(); + this.rateLimitMax = options?.rateLimitMax && options.rateLimitMax > 0 ? options.rateLimitMax : 120; + this.rateLimitWindowMs = + options?.rateLimitWindowMs && options.rateLimitWindowMs > 0 ? options.rateLimitWindowMs : 60_000; this.mcpEngine = new CommitFlowMcpServer(this.repoRoot); } + public checkRateLimit( + clientKey: string, + now: number = Date.now() + ): { allowed: boolean; remaining: number; resetMs: number } { + const windowStart = now - this.rateLimitWindowMs; + const timestamps = (this.rateLimitBuckets.get(clientKey) || []).filter((ts) => ts > windowStart); + + if (timestamps.length >= this.rateLimitMax) { + this.rateLimitBuckets.set(clientKey, timestamps); + const oldest = timestamps[0] || now; + return { + allowed: false, + remaining: 0, + resetMs: Math.max(1000, oldest + this.rateLimitWindowMs - now), + }; + } + + timestamps.push(now); + this.rateLimitBuckets.set(clientKey, timestamps); + return { + allowed: true, + remaining: Math.max(0, this.rateLimitMax - timestamps.length), + resetMs: this.rateLimitWindowMs, + }; + } + + public getAuditLogs(): McpAuditLogEntry[] { + return sharedMcpAuditLog.getAll(); + } + public async start(): Promise<{ port: number; host: string }> { return new Promise((resolve, reject) => { this.server = http.createServer(async (req, res) => { @@ -42,6 +81,10 @@ export class CommitFlowHttpServer { }); this.server.listen(this.port, this.host, () => { + const addr = this.server?.address(); + if (addr && typeof addr === 'object') { + this.port = addr.port; + } resolve({ port: this.port, host: this.host }); }); }); @@ -80,6 +123,27 @@ export class CommitFlowHttpServer { } private async handleRequest(req: http.IncomingMessage, res: http.ServerResponse): Promise { + const startMs = Date.now(); + const clientIp = req.socket?.remoteAddress || '127.0.0.1'; + const method = req.method || 'GET'; + + const recordAudit = ( + pathName: string, + status: number, + extra?: Partial> + ) => { + sharedMcpAuditLog.record({ + timestamp: new Date().toISOString(), + transport: 'http', + method, + path: pathName, + status, + clientIp, + durationMs: Math.max(0, Date.now() - startMs), + ...extra, + }); + }; + const origin = req.headers['origin']; if (origin) { const allowed = @@ -87,6 +151,7 @@ export class CommitFlowHttpServer { origin.startsWith('http://localhost') || origin.startsWith('vscode-webview://'); if (!allowed) { + recordAudit('/unknown', 403, { reason: 'invalid_origin' }); res.writeHead(403, { 'Content-Type': 'application/json' }); res.end(JSON.stringify({ error: 'Forbidden: invalid origin' })); return; @@ -116,6 +181,7 @@ export class CommitFlowHttpServer { const token = authHeader || queryToken; if (!token) { + recordAudit(pathname, 401, { reason: 'missing_token' }); res.writeHead(401, { 'Content-Type': 'application/json' }); res.end(JSON.stringify({ error: 'Unauthorized: missing API key token' })); return; @@ -123,12 +189,30 @@ export class CommitFlowHttpServer { const authValidation = await this.apiKeyManager.validateKey(token, 'read', this.repoRoot); if (!authValidation.valid) { + recordAudit(pathname, 403, { reason: authValidation.reason || 'invalid_key' }); res.writeHead(403, { 'Content-Type': 'application/json' }); res.end(JSON.stringify({ error: 'Forbidden', reason: authValidation.reason })); return; } + const rateKey = authValidation.key?.id || clientIp; + const rate = this.checkRateLimit(rateKey); + res.setHeader('X-RateLimit-Limit', String(this.rateLimitMax)); + res.setHeader('X-RateLimit-Remaining', String(rate.remaining)); + if (!rate.allowed) { + res.setHeader('Retry-After', String(Math.ceil(rate.resetMs / 1000))); + recordAudit(pathname, 429, { + keyId: authValidation.key?.id, + keyName: authValidation.key?.name, + reason: 'rate_limit_exceeded', + }); + res.writeHead(429, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ error: 'Too Many Requests', retryAfterMs: rate.resetMs })); + return; + } + if (pathname === '/sse' && req.method === 'GET') { + recordAudit(pathname, 200, { keyId: authValidation.key?.id, keyName: authValidation.key?.name }); res.writeHead(200, { 'Content-Type': 'text/event-stream', 'Cache-Control': 'no-cache', @@ -155,9 +239,19 @@ export class CommitFlowHttpServer { try { const jsonRpcRequest = JSON.parse(body); const response = await this.mcpEngine.processDirectRequest(jsonRpcRequest, authValidation.key); + recordAudit(pathname, 200, { + keyId: authValidation.key?.id, + keyName: authValidation.key?.name, + rpcMethod: jsonRpcRequest?.method, + }); res.writeHead(200, { 'Content-Type': 'application/json' }); res.end(JSON.stringify(response)); } catch (err: any) { + recordAudit(pathname, 400, { + keyId: authValidation.key?.id, + keyName: authValidation.key?.name, + reason: err.message, + }); res.writeHead(400, { 'Content-Type': 'application/json' }); res.end( JSON.stringify({ @@ -171,6 +265,7 @@ export class CommitFlowHttpServer { return; } + recordAudit(pathname, 404, { keyId: authValidation.key?.id, keyName: authValidation.key?.name }); res.writeHead(404, { 'Content-Type': 'application/json' }); res.end(JSON.stringify({ error: 'Not found' })); } diff --git a/src/mcp/commitflowMcpServer.ts b/src/mcp/commitflowMcpServer.ts index d640436..e5f9249 100644 --- a/src/mcp/commitflowMcpServer.ts +++ b/src/mcp/commitflowMcpServer.ts @@ -6,6 +6,8 @@ import { RemoteManager } from '../remotes/remoteManager'; import { ApiKeyMetadata } from './apiKeyManager'; import { CloudPatch } from '../git/types'; +import { sharedMcpAuditLog } from './mcpAuditLog'; + export class CommitFlowMcpServer { private readonly gitClient = GitClient.getInstance(); private readonly issueService = IssueService.getInstance(); @@ -18,7 +20,20 @@ export class CommitFlowMcpServer { } public async processDirectRequest(req: any, clientKey?: ApiKeyMetadata): Promise { - return this.handleJsonRpc(req, clientKey); + const startMs = Date.now(); + const res = await this.handleJsonRpc(req, clientKey); + if (!clientKey) { + sharedMcpAuditLog.record({ + timestamp: new Date().toISOString(), + transport: 'stdio', + method: 'RPC', + path: 'stdio://json-rpc', + status: res?.error ? 400 : 200, + rpcMethod: req?.method, + durationMs: Math.max(0, Date.now() - startMs), + }); + } + return res; } public async start(): Promise { @@ -529,6 +544,12 @@ export class CommitFlowMcpServer { description: 'Real-time bisect status, current testing probe commit, remaining steps, and good/bad hashes.', mimeType: 'application/json', }, + { + uri: 'commitflow://mcp/audit-log', + name: 'MCP Security & Request Audit Log', + description: 'Structured ring-buffer audit trail of recent MCP HTTP and stdio requests including rate-limit and auth rejections.', + mimeType: 'application/json', + }, ], }, }; @@ -1050,6 +1071,16 @@ export class CommitFlowMcpServer { const bisectStatus = await this.gitClient.getBisectStatus(this.repoRoot); contentText = JSON.stringify(bisectStatus, null, 2); mimeType = 'application/json'; + } else if (uri === 'commitflow://mcp/audit-log') { + contentText = JSON.stringify( + { + count: sharedMcpAuditLog.getAll().length, + entries: sharedMcpAuditLog.getAll(), + }, + null, + 2 + ); + mimeType = 'application/json'; } else { return { jsonrpc: '2.0', diff --git a/src/mcp/mcpAuditLog.ts b/src/mcp/mcpAuditLog.ts new file mode 100644 index 0000000..6a55891 --- /dev/null +++ b/src/mcp/mcpAuditLog.ts @@ -0,0 +1,39 @@ +export interface McpAuditLogEntry { + timestamp: string; + transport: 'http' | 'stdio'; + method: string; + path: string; + status: number; + rpcMethod?: string; + keyId?: string; + keyName?: string; + clientIp?: string; + durationMs: number; + reason?: string; +} + +/** + * Bounded in-memory ring buffer storing recent MCP security and RPC audit events. + */ +export class McpAuditRingBuffer { + private readonly entries: McpAuditLogEntry[] = []; + + constructor(private readonly capacity: number = 500) {} + + public record(entry: McpAuditLogEntry): void { + if (this.entries.length >= this.capacity) { + this.entries.shift(); + } + this.entries.push(entry); + } + + public getAll(): McpAuditLogEntry[] { + return [...this.entries]; + } + + public clear(): void { + this.entries.length = 0; + } +} + +export const sharedMcpAuditLog = new McpAuditRingBuffer(500); diff --git a/src/utils/ipcValidator.ts b/src/utils/ipcValidator.ts new file mode 100644 index 0000000..7202cc5 --- /dev/null +++ b/src/utils/ipcValidator.ts @@ -0,0 +1,211 @@ +import { WebviewToExtensionMessage } from '../git/types'; + +export const ALLOWED_WEBVIEW_COMMANDS: ReadonlySet = new Set< + WebviewToExtensionMessage['command'] +>([ + 'ready', + 'requestData', + 'requestCommitFiles', + 'checkoutBranch', + 'checkoutCommit', + 'pullBranch', + 'rebaseBranch', + 'mergeBranch', + 'abortRebase', + 'stashSave', + 'stashPop', + 'stashApply', + 'stashDrop', + 'openFile', + 'saveSettings', + 'openSettings', + 'copyToClipboard', + 'openExternal', + 'openCommit', + 'openDiff', + 'openFullPanel', + 'filter', + 'requestWorktrees', + 'addWorktree', + 'removeWorktree', + 'openWorktree', + 'requestPullRequests', + 'checkoutPullRequest', + 'requestFileHistory', + 'openFileHistoryDiff', + 'createCloudPatch', + 'applyCloudPatch', + 'deleteCloudPatch', + 'requestCloudPatches', + 'setLayoutMode', + 'cherryPickCommit', + 'revertCommit', + 'createTag', + 'deleteTag', + 'undoLastCommit', + 'generateChangelog', + 'requestIssues', + 'startWorkOnIssue', + 'insertIssueRef', + 'requestIssueDetails', + 'createIssueComment', + 'setIssueState', + 'openInFork', + 'requestReflog', + 'restoreReflogCommit', + 'compareCommits', + 'requestRepoHealth', + 'runRepoGc', + 'startBisect', + 'stepBisect', + 'resetBisect', + 'requestBisectStatus', + 'previewCherryPick', + 'requestRepositories', + 'switchRepository', + 'requestRebaseCommits', + 'simulateRebase', + 'startInteractiveRebase', + 'continueRebase', + 'skipRebase', + 'requestAiKeys', + 'generateAiKey', + 'revokeAiKey', + 'applyDagFilters', + 'dndResetBranchToCommit', + 'dndCherryPickToBranch', + 'dndRebaseOntoCommit', + 'requestConflictedFiles', + 'resolveConflictFile', + 'openMergeEditor', + 'abortMerge', + 'requestCommitTemplate', + 'commitStagedChanges', + 'requestPrComments', + 'addPrComment', + 'approvePullRequest', +]); + +export interface IpcValidationResult { + valid: boolean; + reason?: string; + command?: WebviewToExtensionMessage['command']; +} + +const HASH_OR_REF_REGEX = /^[0-9a-fA-F]{4,64}$|^HEAD(?:~\d+)?$/; +const SAFE_BRANCH_OR_TAG_REGEX = /^(?!-)(?!.*\.\.)(?!.*@\{)(?!.*[\x00-\x1f\x7f ~^:?*\[\\]).{1,255}$/; +const PATH_KEYS = new Set(['filePath', 'worktreePath', 'repoPath', 'activeRepo', 'path', 'cwd']); +const HASH_KEYS = new Set(['commitHash', 'targetHash', 'baseHash', 'hash', 'badCommit', 'goodCommit']); + +function hasUnsafePathTraversal(value: string): boolean { + if (value.includes('\0')) return true; + const normalizedSegments = value.replace(/\\/g, '/').split('/'); + return normalizedSegments.some((segment) => segment === '..'); +} + +function scanPayloadSafety(value: unknown, keyName = '', depth = 0): string | null { + if (depth > 8 || value === undefined || value === null) return null; + + if (typeof value === 'string') { + if (value.includes('\0')) { + return `Null byte detected in ${keyName || 'payload'}`; + } + if (PATH_KEYS.has(keyName) && value.length > 0 && hasUnsafePathTraversal(value)) { + return `Path traversal detected in ${keyName}`; + } + if (HASH_KEYS.has(keyName) && value.length > 0 && !HASH_OR_REF_REGEX.test(value)) { + return `Invalid commit hash format in ${keyName}`; + } + return null; + } + + if (Array.isArray(value)) { + for (let i = 0; i < value.length; i++) { + const err = scanPayloadSafety(value[i], keyName, depth + 1); + if (err) return err; + } + return null; + } + + if (typeof value === 'object') { + const record = value as Record; + for (const key of Object.keys(record)) { + if (key === '__proto__' || key === 'constructor' || key === 'prototype') { + return `Prototype pollution key rejected: ${key}`; + } + const err = scanPayloadSafety(record[key], key, depth + 1); + if (err) return err; + } + } + + return null; +} + +/** + * Runtime schema & security validator for all messages received from the CommitFlow webview. + */ +export class IpcSchemaValidator { + public static validateWebviewMessage(raw: unknown): IpcValidationResult { + if (!raw || typeof raw !== 'object' || Array.isArray(raw)) { + return { valid: false, reason: 'Message must be a non-null plain object.' }; + } + + const msg = raw as Record; + const command = msg.command; + if (typeof command !== 'string' || !ALLOWED_WEBVIEW_COMMANDS.has(command as WebviewToExtensionMessage['command'])) { + return { valid: false, reason: `Unknown or disallowed IPC command: ${String(command)}` }; + } + + const safetyError = scanPayloadSafety(msg.payload); + if (safetyError) { + return { valid: false, reason: safetyError }; + } + + const payload = msg.payload as any; + + if (command === 'openExternal') { + if (typeof payload !== 'string' || !/^https?:\/\//i.test(payload.trim())) { + return { valid: false, reason: 'openExternal requires an http:// or https:// URL.' }; + } + } + + if (command === 'resolveConflictFile') { + if ( + !payload || + typeof payload !== 'object' || + typeof payload.filePath !== 'string' || + !['ours', 'theirs', 'mark-resolved'].includes(payload.strategy) + ) { + return { valid: false, reason: 'resolveConflictFile requires valid filePath and strategy.' }; + } + } + + if (command === 'dndResetBranchToCommit') { + if ( + !payload || + typeof payload !== 'object' || + typeof payload.branchName !== 'string' || + !SAFE_BRANCH_OR_TAG_REGEX.test(payload.branchName) || + typeof payload.targetHash !== 'string' || + !HASH_OR_REF_REGEX.test(payload.targetHash) + ) { + return { valid: false, reason: 'dndResetBranchToCommit requires valid branchName and targetHash.' }; + } + } + + if (command === 'checkoutBranch' && typeof payload === 'string' && !SAFE_BRANCH_OR_TAG_REGEX.test(payload)) { + return { valid: false, reason: 'checkoutBranch contains invalid ref characters.' }; + } + + if (command === 'commitStagedChanges') { + if (!payload || typeof payload !== 'object' || typeof payload.message !== 'string' || !payload.message.trim()) { + return { valid: false, reason: 'commitStagedChanges requires a non-empty message.' }; + } + } + + return { + valid: true, + command: command as WebviewToExtensionMessage['command'], + }; + } +} diff --git a/test/runTests.ts b/test/runTests.ts index debfecb..285fdb0 100644 --- a/test/runTests.ts +++ b/test/runTests.ts @@ -36,6 +36,8 @@ import { } from '../src/git/conventionalCommits'; import { GitHubProvider } from '../src/remotes/githubProvider'; import { GitLabProvider } from '../src/remotes/gitlabProvider'; +import { IpcSchemaValidator } from '../src/utils/ipcValidator'; +import { sharedMcpAuditLog } from '../src/mcp/mcpAuditLog'; console.log('Starting CommitFlow Extended Test Suite...\n'); @@ -1901,7 +1903,123 @@ Host *.internal.corp console.log(' [PASS] Conflict Studio, Conventional Commits & GitHub/GitLab Parity tests passed!'); - console.log('\n[PASS] ALL 48 COMMITFLOW TEST SUITES PASSED WITH 100% SUCCESS!\n'); + // 49. Test Security Hardening: IPC Schema Validator, MCP Rate Limiting & Audit Log Resource (Issue #17) + console.log('▶ Testing IPC Schema Validator, MCP Rate Limiter & Audit Log Resource...'); + assert.strictEqual(IpcSchemaValidator.validateWebviewMessage({ command: 'requestData' }).valid, true); + assert.strictEqual( + IpcSchemaValidator.validateWebviewMessage({ + command: 'resolveConflictFile', + payload: { filePath: 'src/index.ts', strategy: 'ours' }, + }).valid, + true + ); + assert.strictEqual( + IpcSchemaValidator.validateWebviewMessage({ + command: 'openExternal', + payload: 'https://github.com/Ignaciobrenas/CommitFlow', + }).valid, + true + ); + + // Disallowed / malicious IPC payloads + assert.strictEqual(IpcSchemaValidator.validateWebviewMessage(null).valid, false); + assert.strictEqual(IpcSchemaValidator.validateWebviewMessage(['array']).valid, false); + assert.strictEqual(IpcSchemaValidator.validateWebviewMessage({ command: 'execArbitraryCommand' }).valid, false); + assert.strictEqual( + IpcSchemaValidator.validateWebviewMessage({ + command: 'openFile', + payload: { filePath: '../../etc/passwd' }, + }).valid, + false + ); + assert.strictEqual( + IpcSchemaValidator.validateWebviewMessage({ + command: 'openFile', + payload: { filePath: 'src/file.ts\0.png' }, + }).valid, + false + ); + assert.strictEqual( + IpcSchemaValidator.validateWebviewMessage({ + command: 'openExternal', + payload: 'javascript:alert(1)', + }).valid, + false + ); + assert.strictEqual( + IpcSchemaValidator.validateWebviewMessage( + JSON.parse('{"command":"filter","payload":{"__proto__":{"isAdmin":true}}}') + ).valid, + false + ); + assert.strictEqual( + IpcSchemaValidator.validateWebviewMessage({ + command: 'dndResetBranchToCommit', + payload: { branchName: '--upload-pack=evil', targetHash: 'deadbeef' }, + }).valid, + false + ); + + // HTTP server rate limit (429) & audit log ring buffer + sharedMcpAuditLog.clear(); + const secKeyMgr = ApiKeyManager.getInstance(); + const { plaintextKey: secToken, metadata: secKeyMeta } = await secKeyMgr.generateKey('Rate Limit Audit Key', ['read']); + const secHttp = new CommitFlowHttpServer({ + port: 0, + repoRoot: process.cwd(), + rateLimitMax: 2, + rateLimitWindowMs: 10_000, + }); + + try { + const started = await secHttp.start(); + const baseUrl = `http://${started.host}:${started.port}`; + + // Request 1 & 2 succeed + const r1 = await fetch(`${baseUrl}/mcp`, { + method: 'POST', + headers: { Authorization: `Bearer ${secToken}`, 'Content-Type': 'application/json' }, + body: JSON.stringify({ jsonrpc: '2.0', id: 1, method: 'resources/list' }), + }); + assert.strictEqual(r1.status, 200); + const listJson: any = await r1.json(); + assert.ok( + listJson.result.resources.some((r: any) => r.uri === 'commitflow://mcp/audit-log'), + 'MCP server should list commitflow://mcp/audit-log resource' + ); + + const r2 = await fetch(`${baseUrl}/mcp`, { + method: 'POST', + headers: { Authorization: `Bearer ${secToken}`, 'Content-Type': 'application/json' }, + body: JSON.stringify({ + jsonrpc: '2.0', + id: 2, + method: 'resources/read', + params: { uri: 'commitflow://mcp/audit-log' }, + }), + }); + assert.strictEqual(r2.status, 200); + + // Request 3 hits sliding-window rate limit -> 429 + const r3 = await fetch(`${baseUrl}/mcp`, { + method: 'POST', + headers: { Authorization: `Bearer ${secToken}`, 'Content-Type': 'application/json' }, + body: JSON.stringify({ jsonrpc: '2.0', id: 3, method: 'ping' }), + }); + assert.strictEqual(r3.status, 429); + assert.ok(r3.headers.get('retry-after'), '429 response should include Retry-After header'); + + const logs = secHttp.getAuditLogs(); + assert.ok(logs.some((e) => e.status === 200 && e.rpcMethod === 'resources/list')); + assert.ok(logs.some((e) => e.status === 429 && e.reason === 'rate_limit_exceeded')); + } finally { + await secHttp.stop(); + await secKeyMgr.revokeKey(secKeyMeta.id); + } + + console.log(' [PASS] IPC Schema Validator, MCP Rate Limiter & Audit Log Resource tests passed!'); + + console.log('\n[PASS] ALL 49 COMMITFLOW TEST SUITES PASSED WITH 100% SUCCESS!\n'); })().catch((err) => { console.error('[CommitFlow] Test suite failed:', err); process.exit(1);