From ef1650b472b9f10d59a339d413d6be1a7e6692b6 Mon Sep 17 00:00:00 2001 From: Jeff Detmer <649578+jeffdetmer@users.noreply.github.com> Date: Thu, 12 Mar 2026 23:36:35 -0400 Subject: [PATCH] fix(git-service): strip ANSI color codes and control characters from diff output Add --no-color and --no-ext-diff flags to all git diff/show calls via centralized getPlainDiff() and getPlainShowDiff() helpers, and strip any remaining VT control characters using Node's stripVTControlCharacters. This prevents ANSI escape sequences from leaking into diff responses when users have color.diff=always or external diff tools configured. Also improves error logging in getUnstagedDiff for unreadable untracked files, and adds tests for getStagedDiff, getStagedFileDiff, and getBranchDiff with terminal control character assertions. --- package-lock.json | 82 +++++++++++++++++------ src/server/services/git.service.ts | 41 ++++++++---- tests/server/services/git.service.test.ts | 65 ++++++++++++++++++ 3 files changed, 155 insertions(+), 33 deletions(-) diff --git a/package-lock.json b/package-lock.json index 1a861ee..9cf7665 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "githuman", - "version": "0.3.0", + "version": "0.6.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "githuman", - "version": "0.3.0", + "version": "0.6.0", "license": "MIT", "dependencies": { "@fastify/cors": "^10.0.0", @@ -180,7 +180,6 @@ "integrity": "sha512-H3mcG6ZDLTlYfaSNi0iOKkigqMFvkTKlGUYlD8GW7nNOYRrevuA46iTypPyv+06V3fEmvvazfntkBU34L0azAw==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@babel/code-frame": "^7.28.6", "@babel/generator": "^7.28.6", @@ -540,7 +539,6 @@ } ], "license": "MIT", - "peer": true, "engines": { "node": ">=18" }, @@ -584,7 +582,6 @@ } ], "license": "MIT", - "peer": true, "engines": { "node": ">=18" } @@ -2738,6 +2735,66 @@ "node": ">=14.0.0" } }, + "node_modules/@tailwindcss/oxide-wasm32-wasi/node_modules/@emnapi/core": { + "version": "1.7.1", + "dev": true, + "inBundle": true, + "license": "MIT", + "optional": true, + "dependencies": { + "@emnapi/wasi-threads": "1.1.0", + "tslib": "^2.4.0" + } + }, + "node_modules/@tailwindcss/oxide-wasm32-wasi/node_modules/@emnapi/runtime": { + "version": "1.7.1", + "dev": true, + "inBundle": true, + "license": "MIT", + "optional": true, + "dependencies": { + "tslib": "^2.4.0" + } + }, + "node_modules/@tailwindcss/oxide-wasm32-wasi/node_modules/@emnapi/wasi-threads": { + "version": "1.1.0", + "dev": true, + "inBundle": true, + "license": "MIT", + "optional": true, + "dependencies": { + "tslib": "^2.4.0" + } + }, + "node_modules/@tailwindcss/oxide-wasm32-wasi/node_modules/@napi-rs/wasm-runtime": { + "version": "1.1.0", + "dev": true, + "inBundle": true, + "license": "MIT", + "optional": true, + "dependencies": { + "@emnapi/core": "^1.7.1", + "@emnapi/runtime": "^1.7.1", + "@tybys/wasm-util": "^0.10.1" + } + }, + "node_modules/@tailwindcss/oxide-wasm32-wasi/node_modules/@tybys/wasm-util": { + "version": "0.10.1", + "dev": true, + "inBundle": true, + "license": "MIT", + "optional": true, + "dependencies": { + "tslib": "^2.4.0" + } + }, + "node_modules/@tailwindcss/oxide-wasm32-wasi/node_modules/tslib": { + "version": "2.8.1", + "dev": true, + "inBundle": true, + "license": "0BSD", + "optional": true + }, "node_modules/@tailwindcss/oxide-win32-arm64-msvc": { "version": "4.1.18", "resolved": "https://registry.npmjs.org/@tailwindcss/oxide-win32-arm64-msvc/-/oxide-win32-arm64-msvc-4.1.18.tgz", @@ -2792,7 +2849,6 @@ "integrity": "sha512-o4PXJQidqJl82ckFaXUeoAW+XysPLauYI43Abki5hABd853iMhitooc6znOnczgbTYmEP6U6/y1ZyKAIsvMKGg==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@babel/code-frame": "^7.10.4", "@babel/runtime": "^7.12.5", @@ -2998,7 +3054,6 @@ "integrity": "sha512-MciR4AKGHWl7xwxkBa6xUGxQJ4VBOmPTF7sL+iGzuahOFaO0jHCsuEfS80pan1ef4gWId1oWOweIhrDEYLuaOw==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "undici-types": "~6.21.0" } @@ -3008,7 +3063,6 @@ "resolved": "https://registry.npmjs.org/@types/react/-/react-19.2.8.tgz", "integrity": "sha512-3MbSL37jEchWZz2p2mjntRZtPt837ij10ApxKfgmXCTuHWagYg7iA5bqPw6C8BMPfwidlvfPI/fxOc42HLhcyg==", "license": "MIT", - "peer": true, "dependencies": { "csstype": "^3.2.2" } @@ -3019,7 +3073,6 @@ "integrity": "sha512-jp2L/eY6fn+KgVVQAOqYItbF0VY/YApe5Mz2F0aykSO8gx31bYCZyvSeYxCHKvzHG5eZjc+zyaS5BrBWya2+kQ==", "dev": true, "license": "MIT", - "peer": true, "peerDependencies": { "@types/react": "^19.2.0" } @@ -3079,7 +3132,6 @@ "integrity": "sha512-nm3cvFN9SqZGXjmw5bZ6cGmvJSyJPn0wU9gHAZZHDnZl2wF9PhHv78Xf06E0MaNk4zLVHL8hb2/c32XvyJOLQg==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@typescript-eslint/scope-manager": "8.53.1", "@typescript-eslint/types": "8.53.1", @@ -3760,7 +3812,6 @@ "integrity": "sha512-NZyJarBfL7nWwIq+FDL6Zp/yHEhePMNnnJ0y3qfieCrmNvYct8uvtiV41UvlSe6apAfk0fY1FbWx+NwfmpvtTg==", "dev": true, "license": "MIT", - "peer": true, "bin": { "acorn": "bin/acorn" }, @@ -4216,7 +4267,6 @@ } ], "license": "MIT", - "peer": true, "dependencies": { "baseline-browser-mapping": "^2.9.0", "caniuse-lite": "^1.0.30001759", @@ -5406,7 +5456,6 @@ "integrity": "sha512-vPZZsiOKaBAIATpFE2uMI4w5IRwdv/FpQ+qZZMR4E+PeOcM4OeoEbqxRMnywdxP19TyB/3h6QBB0EWon7letSQ==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@typescript-eslint/types": "^8.35.0", "comment-parser": "^1.4.1", @@ -5846,7 +5895,6 @@ } ], "license": "MIT", - "peer": true, "dependencies": { "@fastify/ajv-compiler": "^4.0.5", "@fastify/error": "^4.0.0", @@ -7417,7 +7465,6 @@ "integrity": "sha512-mjzqwWRD9Y1J1KUi7W97Gja1bwOOM5Ug0EZ6UDK3xS7j7mndrkwozHtSblfomlzyB4NepioNt+B2sOSzczVgtQ==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@acemir/cssom": "^0.9.28", "@asamuzakjp/dom-selector": "^6.7.6", @@ -9594,7 +9641,6 @@ } ], "license": "MIT", - "peer": true, "dependencies": { "nanoid": "^3.3.11", "picocolors": "^1.1.1", @@ -9752,7 +9798,6 @@ "resolved": "https://registry.npmjs.org/react/-/react-19.2.3.tgz", "integrity": "sha512-Ku/hhYbVjOQnXDZFv2+RibmLFGwFdeeKHFcOTlrt7xplBnya5OGn/hIRDsqDiSUcfORsDC7MPxwork8jBwsIWA==", "license": "MIT", - "peer": true, "engines": { "node": ">=0.10.0" } @@ -9762,7 +9807,6 @@ "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-19.2.3.tgz", "integrity": "sha512-yELu4WmLPw5Mr/lmeEpox5rw3RETacE++JgHqQzd2dg+YbJuat3jH4ingc+WPZhxaoFzdv9y33G+F7Nl5O0GBg==", "license": "MIT", - "peer": true, "dependencies": { "scheduler": "^0.27.0" }, @@ -11517,7 +11561,6 @@ "integrity": "sha512-w+N7Hifpc3gRjZ63vYBXA56dvvRlNWRczTdmCBBa+CotUzAPf5b7YMdMR/8CQoeYE5LX3W4wj6RYTgonm1b9DA==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "esbuild": "^0.27.0", "fdir": "^6.5.0", @@ -11608,7 +11651,6 @@ "integrity": "sha512-FQMeF0DJdWY0iOnbv466n/0BudNdKj1l5jYgl5JVTwjSsZSlqyXFt/9+1sEyhR6CLowbZpV7O1sCHrzBhucKKg==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@vitest/expect": "4.0.17", "@vitest/mocker": "4.0.17", diff --git a/src/server/services/git.service.ts b/src/server/services/git.service.ts index ed2f8a2..7968425 100644 --- a/src/server/services/git.service.ts +++ b/src/server/services/git.service.ts @@ -4,6 +4,7 @@ import { simpleGit, type SimpleGit } from 'simple-git' import { execFileSync } from 'node:child_process' import { readFile } from 'node:fs/promises' +import { stripVTControlCharacters } from 'node:util' import { join } from 'node:path' import { requestContext } from '../app.ts' import type { RepositoryInfo } from '../../shared/types.ts' @@ -22,6 +23,20 @@ export class GitService { return requestContext.get('log') } + private sanitizeGitTextOutput (output: string): string { + return stripVTControlCharacters(output) + } + + private async getPlainDiff (args: string[] = []): Promise { + const output = await this.git.diff(['--no-color', '--no-ext-diff', ...args]) + return this.sanitizeGitTextOutput(output) + } + + private async getPlainShowDiff (args: string[] = []): Promise { + const output = await this.git.show(['--no-color', '--no-ext-diff', ...args]) + return this.sanitizeGitTextOutput(output) + } + /** * Check if the path is a valid git repository */ @@ -146,7 +161,7 @@ export class GitService { */ private async isFileStaged (filePath: string): Promise { try { - const result = await this.git.diff(['--cached', '--name-only', '--', filePath]) + const result = await this.getPlainDiff(['--cached', '--name-only', '--', filePath]) return result.trim().length > 0 } catch (err) { this.log?.debug({ err, filePath }, 'isFileStaged check failed') @@ -158,7 +173,7 @@ export class GitService { * Get unified diff for all staged changes */ async getStagedDiff (): Promise { - const diff = await this.git.diff(['--cached']) + const diff = await this.getPlainDiff(['--cached']) return diff } @@ -166,7 +181,7 @@ export class GitService { * Get unified diff for a specific staged file */ async getStagedFileDiff (filePath: string): Promise { - const diff = await this.git.diff(['--cached', '--', filePath]) + const diff = await this.getPlainDiff(['--cached', '--', filePath]) return diff } @@ -174,7 +189,7 @@ export class GitService { * Get diff statistics for staged changes */ async getStagedDiffStats (): Promise { - const numstat = await this.git.diff(['--cached', '--numstat']) + const numstat = await this.getPlainDiff(['--cached', '--numstat']) const files: FileDiffStats[] = [] const lines = numstat.trim().split('\n').filter(Boolean) @@ -203,7 +218,7 @@ export class GitService { * Check if there are any staged changes */ async hasStagedChanges (): Promise { - const diff = await this.git.diff(['--cached', '--name-only']) + const diff = await this.getPlainDiff(['--cached', '--name-only']) return diff.trim().length > 0 } @@ -264,7 +279,7 @@ export class GitService { */ async getUnstagedDiff (): Promise { // Get regular diff for tracked files - const diff = await this.git.diff() + const diff = await this.getPlainDiff() // Get untracked files and generate diff for them const status = await this.git.status() @@ -287,8 +302,8 @@ export class GitService { const diffLines = lines.map(line => `+${line}`) untrackedDiffs.push([...diffHeader, ...diffLines].join('\n')) - } catch { - // Skip files that can't be read (binary, permission issues, etc.) + } catch (err) { + this.log?.debug({ err, filePath }, 'getUnstagedDiff skipped unreadable untracked file') } } @@ -491,7 +506,7 @@ export class GitService { */ async getBranchDiff (targetBranch: string): Promise { // Get diff from target branch to HEAD (shows what's in HEAD, not in target branch) - const diff = await this.git.diff([`${targetBranch}...HEAD`]) + const diff = await this.getPlainDiff([`${targetBranch}...HEAD`]) return diff } @@ -500,7 +515,7 @@ export class GitService { */ async getBranchFileDiff (targetBranch: string, filePath: string): Promise { try { - const diff = await this.git.diff([`${targetBranch}...HEAD`, '--', filePath]) + const diff = await this.getPlainDiff([`${targetBranch}...HEAD`, '--', filePath]) return diff } catch (err) { this.log?.debug({ err, targetBranch, filePath }, 'getBranchFileDiff failed') @@ -518,7 +533,7 @@ export class GitService { if (commits.length === 1) { // Single commit - show that commit's diff - const diff = await this.git.show([commits[0], '--format=']) + const diff = await this.getPlainShowDiff([commits[0], '--format=']) return diff } @@ -526,7 +541,7 @@ export class GitService { // This works regardless of commit order or whether they're contiguous const diffs: string[] = [] for (const commit of commits) { - const diff = await this.git.show([commit, '--format=']) + const diff = await this.getPlainShowDiff([commit, '--format=']) if (diff.trim()) { diffs.push(diff) } @@ -546,7 +561,7 @@ export class GitService { const diffs: string[] = [] for (const commit of commits) { try { - const diff = await this.git.show([commit, '--format=', '--', filePath]) + const diff = await this.getPlainShowDiff([commit, '--format=', '--', filePath]) if (diff.trim()) { diffs.push(diff) } diff --git a/tests/server/services/git.service.test.ts b/tests/server/services/git.service.test.ts index 94a3d58..d6b44fb 100644 --- a/tests/server/services/git.service.test.ts +++ b/tests/server/services/git.service.test.ts @@ -58,6 +58,15 @@ function createTestRepoWithMultipleCommits (t: TestContext): string { return tempDir } +function assertHasNoTerminalControlCharacters (value: string, message: string): void { + const hasTerminalControlCharacter = Array.from(value).some(char => { + const charCode = char.charCodeAt(0) + return charCode === 0x1B || charCode === 0x9B + }) + + assert.ok(!hasTerminalControlCharacter, message) +} + describe('git.service', () => { describe('getCommits', () => { it('should return an array of commits', async (t) => { @@ -159,6 +168,7 @@ describe('git.service', () => { // Diff should be a string with content (our test repo has real changes) assert.ok(typeof diff === 'string') assert.ok(diff.length > 0, 'Diff should have content') + assertHasNoTerminalControlCharacters(diff, 'commit diff should not include terminal control characters') }) it('should return combined diff for multiple commits', async (t) => { @@ -173,6 +183,7 @@ describe('git.service', () => { // Should return a string with diff content assert.ok(typeof diff === 'string') assert.ok(diff.length > 0, 'Combined diff should have content') + assertHasNoTerminalControlCharacters(diff, 'combined commit diff should not include terminal control characters') }) it('should handle commits in any order', async (t) => { @@ -217,6 +228,7 @@ describe('git.service', () => { const diff = await testGit.getCommitsFileDiff([sha], 'src.ts') assert.ok(diff.includes('src.ts')) + assertHasNoTerminalControlCharacters(diff, 'commit diff should not include terminal control characters') assert.ok(diff.includes('+const x = 1;')) }) @@ -270,6 +282,7 @@ describe('git.service', () => { // Stay on feature branch and compare against main (shows what's in feature, not in main) const diff = await testGit.getBranchFileDiff(mainBranch, 'feature.ts') assert.ok(diff.includes('feature.ts')) + assertHasNoTerminalControlCharacters(diff, 'branch diff should not include terminal control characters') assert.ok(diff.includes('+const y = 1;')) }) @@ -299,6 +312,25 @@ describe('git.service', () => { }) }) + describe('getBranchDiff', () => { + it('should return diff between the current branch and a target branch', async (t) => { + const tempDir = createTestRepoWithCommit(t) + const testGit = new GitService(tempDir) + + const mainBranch = execSync('git rev-parse --abbrev-ref HEAD', { cwd: tempDir }).toString().trim() + + execSync('git checkout -b feature', { cwd: tempDir, stdio: 'ignore' }) + writeFileSync(join(tempDir, 'feature.ts'), 'const y = 1;\n') + execSync('git add feature.ts && git commit -m "Add feature.ts"', { cwd: tempDir, stdio: 'ignore' }) + + const diff = await testGit.getBranchDiff(mainBranch) + + assert.ok(diff.includes('feature.ts')) + assertHasNoTerminalControlCharacters(diff, 'branch diff should not include terminal control characters') + assert.ok(diff.includes('+const y = 1;')) + }) + }) + describe('getBranches', () => { it('should return an array of branches', async (t) => { const tempDir = createTestRepoWithCommit(t) @@ -462,6 +494,7 @@ describe('git.service', () => { const diff = await testGit.getUnstagedDiff() assert.ok(diff.includes('README.md')) assert.ok(diff.includes('-# Test')) + assertHasNoTerminalControlCharacters(diff, 'unstaged diff should not include terminal control characters') assert.ok(diff.includes('+# Updated content')) }) @@ -480,6 +513,38 @@ describe('git.service', () => { }) }) + describe('getStagedDiff', () => { + it('should return diff for staged files', async (t) => { + const tempDir = createTestRepoWithCommit(t) + const testGit = new GitService(tempDir) + + writeFileSync(join(tempDir, 'README.md'), '# Updated content\n') + execSync('git add README.md', { cwd: tempDir, stdio: 'ignore' }) + + const diff = await testGit.getStagedDiff() + assert.ok(diff.includes('README.md')) + assert.ok(diff.includes('-# Test')) + assertHasNoTerminalControlCharacters(diff, 'staged diff should not include terminal control characters') + assert.ok(diff.includes('+# Updated content')) + }) + }) + + describe('getStagedFileDiff', () => { + it('should return diff for a specific staged file', async (t) => { + const tempDir = createTestRepoWithCommit(t) + const testGit = new GitService(tempDir) + + writeFileSync(join(tempDir, 'README.md'), '# Updated content\n') + execSync('git add README.md', { cwd: tempDir, stdio: 'ignore' }) + + const diff = await testGit.getStagedFileDiff('README.md') + assert.ok(diff.includes('README.md')) + assert.ok(diff.includes('-# Test')) + assertHasNoTerminalControlCharacters(diff, 'staged file diff should not include terminal control characters') + assert.ok(diff.includes('+# Updated content')) + }) + }) + describe('stageFile', () => { it('should stage a single file', async (t) => { const tempDir = createTestRepoWithCommit(t)