diff --git a/.changeset/neutralize-control-chars.md b/.changeset/neutralize-control-chars.md new file mode 100644 index 0000000..cc689f6 --- /dev/null +++ b/.changeset/neutralize-control-chars.md @@ -0,0 +1,13 @@ +--- +"@codacy/codacy-cloud-cli": patch +--- + +Neutralize terminal control characters in human-readable output (CWE-150). +Repository-derived values shown by the CLI — PR and finding titles, author +names, branches, file paths, diff and file content, issue messages, and package +names — are now stripped of ANSI/OSC escape and other control bytes before being +printed, so a crafted pull request can no longer repaint or hide findings, spoof +gate status, or trigger terminal side effects (e.g. clipboard writes) when you +run the CLI against it. Offending bytes are shown in visible caret notation +(e.g. `^[`) instead of being interpreted. `--output json` is unaffected — it +still returns the original values, escaped by JSON encoding. diff --git a/AGENTS.md b/AGENTS.md index 699e131..fe96d40 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -75,6 +75,10 @@ codacy-cloud-cli/ - `ora` for loading spinners - `dayjs` for date formatting — for "last updated" style dates, use `formatFriendlyDate()` from `utils/output.ts` (relative for today, "Yesterday", otherwise YYYY-MM-DD) - **Output:** Default output is human readable with tables and colors, but can be overridden with the `--output json` flag. +- **Untrusted output (CWE-150 — improper neutralization of control sequences):** Any repository-derived value written to the human-readable output must be passed through `sanitizeText()` from `utils/sanitize.ts` **before** the CLI wraps it in `ansis` styling. This neutralizes terminal control characters (C0/C1/DEL) so a crafted repo can't inject ANSI/OSC escape sequences. + - **Sinks to sanitize:** PR/finding titles, author names, branches, file paths, diff/file content, issue messages, and package/version names. + - **Where NOT to sanitize:** do **not** sanitize at the `console.log` boundary (that strips the CLI's own legitimate colors), and do **not** sanitize the `--output json` path (JSON encoding already escapes control bytes). + - **When adding a command or render helper:** sanitize each untrusted field at the point the raw value enters the output string. - **Pagination:** All commands calling paginated APIs must call `printPaginationWarning(response.pagination, hint)` from `utils/output.ts` after displaying results. The hint should suggest command-specific filtering options. - **Polling / waiting:** Commands that wait on a remote operation (e.g. `--reanalyze-and-wait`) use the shared helpers in `utils/reanalyze-wait.ts`. - Route polling delays through the exported `timers.sleep` so tests can stub it (`vi.spyOn(timers, "sleep").mockResolvedValue()`). diff --git a/SPECS/README.md b/SPECS/README.md index 89e0f01..8ba0195 100644 --- a/SPECS/README.md +++ b/SPECS/README.md @@ -77,3 +77,4 @@ _No pending tasks._ All commands implemented. | 2026-07-16 | `issues --ignored` (`-i`): new read-only mode listing issues marked as ignored on Codacy, via the dedicated `searchRepositoryIgnoredIssues` endpoint. Boolean flag modeled on `--false-positives` for consistency (not a `--state ` selector). Reuses `buildFilterBody` (all existing filters + `--false-positives` pass through) and the paginate-to-`--limit` loop; errors when combined with `--overview`/`--ignore`. New `printIgnoredIssueCard` in `utils/formatting.ts` renders the ignore metadata line (`Ignored as by · ` + optional comment) and the string `issueId` (ignored issues have no numeric `resultDataId`). Omitting the flag keeps existing behavior unchanged (10 new tests, 475 total) | | 2026-07-08 | `issues --overview` noise suggestions tuned to stop firing on low-volume repos: added a `NOISE_MIN_TOTAL` (200) floor on the repo's total issues that suppresses the whole "reduce noise" section below it, and a `NOISE_MIN_PATTERN` (100) absolute floor on each pattern's own count (AND-gated with the relative rules) so a long tail of tiny patterns can't drag the median down and make a ~9-issue pattern look noisy — the total floor is kept above the per-pattern floor so it isn't dead code; the ≥10% share rule now only applies with ≥11 distinct patterns (`NOISE_MIN_PATTERNS_FOR_SHARE` — an even split only drops below 10% once N > 10, so 8–10 balanced patterns would otherwise all be flagged); and the ≥3× multiple rule (`NOISE_MEDIAN_MULTIPLE`) now measures against the **median** (via new `medianOf()`) instead of the mean, so a single huge pattern can no longer inflate the baseline and mask smaller disproportionate patterns (5 new tests, 465 total) | | 2026-07-17 | `issues --ignore` now confirms before bulk-ignoring: `executeBulkIgnore` prints the match count then prompts via shared `confirmAction` (`utils/prompt.ts`), proceeding only on `y`. New `--skip-confirmation` (`-y`) bypasses the prompt for CI/scripts (same short flag as `tools --import --skip-approval`); non-TTY without the flag aborts rather than ignoring by accident. Confirmation runs after the fetch (count is shown) but before any `bulkIgnoreIssues` call (3 new tests, 478 total) | +| 2026-07-24 | Security (CWE-150, HackerOne): neutralize terminal control characters in human-readable output. New `src/utils/sanitize.ts` (`sanitizeText`) strips C0 (0x00–0x1F except TAB/LF), DEL (0x7F) and C1 (0x80–0x9F) — CR included — replacing each with visible caret/`\xNN` notation so a crafted PR can't inject ANSI/OSC sequences to repaint or hide findings, spoof gate status, or drive terminal side effects (OSC 52 clipboard, OSC 8 hyperlinks). Applied *before* the CLI's own `ansis` styling (can't sanitize at the console boundary — that would strip the CLI's legitimate colours, and allow-listing SGR would still pass attacker SGR through). Covers every render path: shared helpers in `utils/formatting.ts` (issue cards/detail, code context, CVE block, dependency chains, version segments) plus `pull-request` (About table, Files, diff-coverage, annotated diff line/hunk/path), `findings`/`finding`, `issues` (overview tables, noise suggestions), `issue`, `repository` (About, PRs, overview), `ls`/`directories`. JSON output left intact (JSON encoding already escapes control bytes). New `src/utils/sanitize.test.ts` + pull-request table/diff regression tests (12 new tests, 490 total) | diff --git a/src/commands/AGENTS.md b/src/commands/AGENTS.md index 6288333..9379ade 100644 --- a/src/commands/AGENTS.md +++ b/src/commands/AGENTS.md @@ -4,6 +4,32 @@ Each command is a single file that exports a `registerCommand(program: Command)` function. Commands are registered in `src/index.ts`. +## Neutralizing untrusted output (CWE-150) + +Repository-derived values (attacker may control them via a PR) are neutralized +with `sanitizeText()` (`utils/sanitize.ts`) before being written to the +human-readable output, so embedded ESC/control bytes can't be interpreted as +ANSI/OSC control sequences (repaint/hide findings, spoof gate status, OSC 52 +clipboard writes, OSC 8 hyperlink spoofing). + +- **Apply before styling, not at the boundary.** The CLI itself emits legitimate + `ansis` SGR sequences, so sanitizing at `console.log` would strip its own + colors, and merely allow-listing SGR would still pass an attacker's SGR + through (the originally reported vector). Sanitize each raw untrusted value + *before* it is wrapped in `ansis`. +- **JSON output is left untouched** — JSON encoding already escapes control + bytes, so `--output json` consumers get the original values. +- **Where it lives.** Most sinks are covered centrally in the shared render + helpers (`utils/formatting.ts`: issue cards/detail, code context, CVE block, + dependency chains, version segments). Commands that render untrusted values + outside those helpers sanitize at the render point: `pull-request` (About + table, Files, diff-coverage, annotated diff line/hunk/path), `findings`/ + `finding`, `issues` (overview tables + noise suggestions), `issue`, + `repository` (About, Open PRs, overview), `ls`/`directories` (dir/file names). +- **New commands/helpers** must sanitize each untrusted field (titles, author + names, branches, file paths, diff/file content, issue messages, package + names, etc.) at the point the raw value enters the output string. + ## Command Aliases Every command must declare a short alias via `.alias()`. Keep aliases short (2–4 characters) and intuitive: diff --git a/src/commands/directories.ts b/src/commands/directories.ts index 5f136de..756cb88 100644 --- a/src/commands/directories.ts +++ b/src/commands/directories.ts @@ -5,6 +5,7 @@ import pluralize from "pluralize"; import { checkApiToken } from "../utils/auth"; import { handleError } from "../utils/error"; import { createTable, getOutputFormat, printJson } from "../utils/output"; +import { sanitizeText } from "../utils/sanitize"; import { resolveRepoArgs } from "../utils/resolve-repo-args"; import { fetchAllDirectories, @@ -152,10 +153,10 @@ function renderDir(ctx: DirContext, dirs: DirectoryNode[]): void { console.log(ansis.bold(`\n${dirHeader(ctx, dirs)}\n`)); const table = createTable({ head: TABLE_HEAD }); for (const d of dirs) { - table.push([`${FOLDER_GLYPH} ${d.name}`, ...metricCells(d)]); + table.push([`${FOLDER_GLYPH} ${sanitizeText(d.name)}`, ...metricCells(d)]); for (const child of d.children ?? []) { table.push([ - `${ansis.dim(CHILD_CONNECTOR)}${child.name}`, + `${ansis.dim(CHILD_CONNECTOR)}${sanitizeText(child.name)}`, ...metricCells(child), ]); } diff --git a/src/commands/finding.ts b/src/commands/finding.ts index 0f0e2de..6847ca1 100644 --- a/src/commands/finding.ts +++ b/src/commands/finding.ts @@ -13,6 +13,7 @@ import { printCveBlock, printIssueCodeContext, } from "../utils/formatting"; +import { sanitizeText } from "../utils/sanitize"; import { SecurityService } from "../api/client/services/SecurityService"; import { IgnoreSRMItemBody } from "../api/client/models/IgnoreSRMItemBody"; import { AnalysisService } from "../api/client/services/AnalysisService"; @@ -49,8 +50,8 @@ function printFindingDetail( const line1Parts: string[] = [colorPriority(item.priority)]; const catParts = [ - item.securityCategory, - item.scanType ? ansis.dim(item.scanType) : undefined, + sanitizeText(item.securityCategory), + item.scanType ? ansis.dim(sanitizeText(item.scanType)) : undefined, ] .filter(Boolean) .join(" "); @@ -61,13 +62,13 @@ function printFindingDetail( ) as string[]; if (penTestParts.length > 0) line1Parts.push(penTestParts.join(" ")); - if (item.repository) line1Parts.push(ansis.dim(item.repository)); + if (item.repository) line1Parts.push(ansis.dim(sanitizeText(item.repository))); const idLabel = ansis.hex("#555555")(item.id); console.log(line1Parts.join(pipe) + ` ${idLabel}`); // Title - console.log(item.title); + console.log(sanitizeText(item.title)); console.log(); // Line 2: Status DueAt | CVE/CWE | AffectedVersion → FixedVersion | Application | AffectedTargets @@ -89,8 +90,8 @@ function printFindingDetail( if (versionSegment) line2Parts.push(ansis.dim(versionSegment)); } - if (item.application) line2Parts.push(ansis.dim(item.application)); - if (item.affectedTargets) line2Parts.push(ansis.dim(item.affectedTargets)); + if (item.application) line2Parts.push(ansis.dim(sanitizeText(item.application))); + if (item.affectedTargets) line2Parts.push(ansis.dim(sanitizeText(item.affectedTargets))); console.log(line2Parts.join(pipe)); @@ -108,9 +109,9 @@ function printFindingDetail( const ig = item.ignored; console.log(); console.log( - ansis.dim(`Ignored by ${ig.authorName} on ${formatDueDate(ig.at)}`), + ansis.dim(`Ignored by ${sanitizeText(ig.authorName)} on ${formatDueDate(ig.at)}`), ); - if (ig.reason) console.log(ansis.dim(ig.reason)); + if (ig.reason) console.log(ansis.dim(sanitizeText(ig.reason))); } // CVE block: shown here only when there is no linked Codacy issue. @@ -123,18 +124,18 @@ function printFindingDetail( // Optional prose fields if (item.summary) { console.log(); - console.log(item.summary); + console.log(sanitizeText(item.summary)); } if (item.additionalInfo) { console.log(); - console.log(item.additionalInfo); + console.log(sanitizeText(item.additionalInfo)); } if (item.remediation) { console.log(); console.log(ansis.bold("Remediation:")); - console.log(item.remediation); + console.log(sanitizeText(item.remediation)); } // Codacy source: show linked quality issue context + pattern info. diff --git a/src/commands/findings.ts b/src/commands/findings.ts index 32ea180..357c8fa 100644 --- a/src/commands/findings.ts +++ b/src/commands/findings.ts @@ -18,6 +18,7 @@ import { formatVersionSegment, formatDependencyChainsLine, } from "../utils/formatting"; +import { sanitizeText } from "../utils/sanitize"; import { SecurityService } from "../api/client/services/SecurityService"; import { SrmItem } from "../api/client/models/SrmItem"; import { SearchSRMItems } from "../api/client/models/SearchSRMItems"; @@ -84,7 +85,10 @@ function printFindingCard(item: SrmItem, showRepo: boolean): void { // Line 1: Priority | SecurityCategory ScanType | Likelihood EffortToFix | Repository const line1Parts: string[] = [colorPriority(item.priority)]; - const catParts = [item.securityCategory, ansis.dim(item.scanType)] + const catParts = [ + sanitizeText(item.securityCategory), + item.scanType ? ansis.dim(sanitizeText(item.scanType)) : undefined, + ] .filter(Boolean) .join(" "); if (catParts) line1Parts.push(catParts); @@ -94,14 +98,14 @@ function printFindingCard(item: SrmItem, showRepo: boolean): void { ) as string[]; if (penTestParts.length > 0) line1Parts.push(penTestParts.join(" ")); - if (showRepo && item.repository) line1Parts.push(ansis.dim(item.repository)); + if (showRepo && item.repository) line1Parts.push(ansis.dim(sanitizeText(item.repository))); const idLabel = ansis.hex("#555555")(item.id); console.log(line1Parts.join(pipe) + ` ${idLabel}`); // Line 2: Title - console.log(item.title); - if (item.affectedTargets) console.log(ansis.dim(item.affectedTargets)); + console.log(sanitizeText(item.title)); + if (item.affectedTargets) console.log(ansis.dim(sanitizeText(item.affectedTargets))); console.log(); // Line 3: Status DueAt | CVE/CWE | AffectedVersion → FixedVersion | Application | AffectedTargets @@ -124,8 +128,7 @@ function printFindingCard(item: SrmItem, showRepo: boolean): void { if (versionSegment) line3Parts.push(ansis.dim(versionSegment)); } - if (item.application) line3Parts.push(ansis.dim(item.application)); - //if (item.affectedTargets) line3Parts.push(ansis.dim(item.affectedTargets)); + if (item.application) line3Parts.push(ansis.dim(sanitizeText(item.application))); console.log(line3Parts.join(pipe)); diff --git a/src/commands/issue.ts b/src/commands/issue.ts index d412f43..fdfe5b6 100644 --- a/src/commands/issue.ts +++ b/src/commands/issue.ts @@ -6,6 +6,7 @@ import { handleError } from "../utils/error"; import { resolveRepoArgs } from "../utils/resolve-repo-args"; import { getOutputFormat, pickDeep, printJson } from "../utils/output"; import { printIssueDetail } from "../utils/formatting"; +import { sanitizeText } from "../utils/sanitize"; import { AnalysisService } from "../api/client/services/AnalysisService"; import { ToolsService } from "../api/client/services/ToolsService"; import { FileService } from "../api/client/services/FileService"; @@ -96,7 +97,7 @@ Examples: startLine, endLine, ).catch((e) => { - console.log("File path: ", issue.filePath); + console.error(ansis.red(`File path: ${sanitizeText(issue.filePath)}`)); console.error(ansis.red(`Error fetching file content: ${e}`)); return null; }), diff --git a/src/commands/issues.ts b/src/commands/issues.ts index 53bb027..397fe34 100644 --- a/src/commands/issues.ts +++ b/src/commands/issues.ts @@ -19,6 +19,7 @@ import { resolveToolUuids, formatCount, } from "../utils/formatting"; +import { sanitizeText } from "../utils/sanitize"; import { AnalysisService } from "../api/client/services/AnalysisService"; import { ToolsService } from "../api/client/services/ToolsService"; import { Tool } from "../api/client/models/Tool"; @@ -176,7 +177,7 @@ function printCountTable(title: string, counts: Count[]): void { const sorted = [...counts].sort((a, b) => b.total - a.total); const table = createTable({ head: [title, "Count"] }); for (const c of sorted) { - table.push([c.name, String(c.total)]); + table.push([sanitizeText(c.name), String(c.total)]); } console.log(table.toString()); } @@ -186,7 +187,7 @@ function printPatternsTable(patterns: PatternsCount[]): void { const sorted = [...patterns].sort((a, b) => b.total - a.total); const table = createTable({ head: ["Pattern", "Count"] }); for (const p of sorted) { - table.push([`${p.title} ${ansis.dim(p.id)}`, String(p.total)]); + table.push([`${sanitizeText(p.title)} ${ansis.dim(p.id)}`, String(p.total)]); } console.log(table.toString()); } @@ -436,7 +437,7 @@ function printNoiseSuggestions( for (const s of suggestions) { const label = s.total === 1 ? "issue" : "issues"; const reduction = ansis.green(`(-${formatCount(s.total)} ${label})`); - console.log(` Disable ${ansis.bold(`"${s.title}"`)} ${reduction}`); + console.log(` Disable ${ansis.bold(`"${sanitizeText(s.title)}"`)} ${reduction}`); if (s.command) { console.log(` ${ansis.dim(">")} ${s.command}`); } else if (s.action) { diff --git a/src/commands/ls.ts b/src/commands/ls.ts index f7ddcf1..bfd8834 100644 --- a/src/commands/ls.ts +++ b/src/commands/ls.ts @@ -5,6 +5,7 @@ import pluralize from "pluralize"; import { checkApiToken } from "../utils/auth"; import { handleError } from "../utils/error"; import { createTable, getOutputFormat, printJson } from "../utils/output"; +import { sanitizeText } from "../utils/sanitize"; import { fetchAllDirectories, fetchAllFiles, @@ -184,11 +185,11 @@ function renderLs( console.log(ansis.bold(`\n${lsHeader(ctx, dirs.length, files.length)}\n`)); const table = createTable({ head: TABLE_HEAD }); for (const d of dirs) { - table.push([`${FOLDER_GLYPH} ${d.name}`, ...metricCells(d)]); + table.push([`${FOLDER_GLYPH} ${sanitizeText(d.name)}`, ...metricCells(d)]); } for (const f of files) { table.push([ - `${ansis.dim(FILE_GLYPH)} ${fileLabel(ctx, f.path)}`, + `${ansis.dim(FILE_GLYPH)} ${sanitizeText(fileLabel(ctx, f.path))}`, ...metricCells(f), ]); } diff --git a/src/commands/pull-request.test.ts b/src/commands/pull-request.test.ts index a3f9564..5042046 100644 --- a/src/commands/pull-request.test.ts +++ b/src/commands/pull-request.test.ts @@ -1700,6 +1700,131 @@ describe("pull-request command", () => { expect(allOutput).not.toContain("Head Commit"); }); + // ─── Control-character neutralization (CWE-150) ────────────────────────── + + describe("neutralizes terminal control characters in untrusted output", () => { + const ESC = "\u001b"; + const BEL = "\u0007"; + + it("neutralizes ESC bytes in the About table (title, author, branch)", async () => { + const maliciousPr = { + ...mockPrData, + pullRequest: { + ...mockPrData.pullRequest, + title: `Fix bug ${ESC}[31mPWNED-TITLE`, + owner: { id: 1, name: `evil${ESC}]0;PWNED-AUTHOR${BEL}` }, + originBranch: `feat${ESC}[2K-PWNED-BRANCH`, + }, + }; + + vi.mocked(AnalysisService.getRepositoryPullRequest).mockResolvedValue( + maliciousPr as any, + ); + vi.mocked(AnalysisService.listPullRequestIssues) + .mockResolvedValueOnce({ analyzed: true, data: [] } as any) + .mockResolvedValueOnce({ analyzed: true, data: [] } as any); + vi.mocked(AnalysisService.listPullRequestFiles).mockResolvedValue({ + data: [], + } as any); + + const program = createProgram(); + await program.parseAsync([ + "node", "test", "pull-request", "gh", "test-org", "test-repo", "42", + ]); + + const output = getAllOutput(); + + // Visible text survives, so the value isn't silently dropped. + expect(output).toContain("PWNED-TITLE"); + expect(output).toContain("PWNED-AUTHOR"); + expect(output).toContain("PWNED-BRANCH"); + + // The raw attacker sequences must be gone (the CLI's own colour codes + // are separate and never precede these unique markers). + expect(output).not.toContain(`${ESC}[31mPWNED-TITLE`); + expect(output).not.toContain(`${ESC}]0;PWNED-AUTHOR`); + expect(output).not.toContain(BEL); + expect(output).not.toContain(`${ESC}[2K-PWNED-BRANCH`); + }); + + it("neutralizes ESC bytes in the annotated diff view (--diff)", async () => { + const maliciousDiff = [ + "diff --git a/src/index.ts b/src/index.ts", + "index abc1234..def5678 100644", + "--- a/src/index.ts", + "+++ b/src/index.ts", + "@@ -1,5 +1,6 @@", + " function foo() {", + " const x = 1;", + `+ const y = 2; ${ESC}[31mINJECTED-DIFF`, + " return x;", + " }", + ].join("\n"); + + vi.mocked(RepositoryService.getPullRequestDiff).mockResolvedValue({ + diff: maliciousDiff, + } as any); + vi.mocked( + CoverageService.getRepositoryPullRequestFilesCoverage, + ).mockResolvedValue({ + data: [ + { + fileName: "src/index.ts", + coverage: 100, + diffLineHits: [{ lineNumber: "3", hits: 2 }], // marks the injected line + }, + ], + } as any); + vi.mocked(AnalysisService.listPullRequestIssues) + .mockResolvedValueOnce({ data: [], pagination: undefined } as any) + .mockResolvedValueOnce({ data: [], pagination: undefined } as any); + + const program = createProgram(); + await program.parseAsync([ + "node", "test", "pull-request", "gh", "test-org", "test-repo", "42", "--diff", + ]); + + const output = getAllOutput(); + + // The diff line content is still shown... + expect(output).toContain("INJECTED-DIFF"); + // ...but the raw ESC control sequence is neutralized. + expect(output).not.toContain(`${ESC}[31mINJECTED-DIFF`); + expect(output).toContain("^[[31mINJECTED-DIFF"); + }); + + it("leaves JSON output untouched so consumers get the original bytes", async () => { + const maliciousPr = { + ...mockPrData, + pullRequest: { + ...mockPrData.pullRequest, + title: `Fix bug ${ESC}[31mPWNED-TITLE`, + }, + }; + + vi.mocked(AnalysisService.getRepositoryPullRequest).mockResolvedValue( + maliciousPr as any, + ); + vi.mocked(AnalysisService.listPullRequestIssues) + .mockResolvedValueOnce({ analyzed: true, data: [] } as any) + .mockResolvedValueOnce({ analyzed: true, data: [] } as any); + vi.mocked(AnalysisService.listPullRequestFiles).mockResolvedValue({ + data: [], + } as any); + + const program = createProgram(); + await program.parseAsync([ + "node", "test", "--output", "json", + "pull-request", "gh", "test-org", "test-repo", "42", + ]); + + // JSON.stringify escapes the ESC byte, preserving the original value — + // it is NOT neutralized to caret notation like the human-readable output. + const output = getAllOutput(); + expect(output).toContain("\\u001b[31mPWNED-TITLE"); + }); + }); + describe("auto-detect from git remote", () => { it("should auto-detect provider/org/repo when only prNumber is provided", async () => { vi.mocked(AnalysisService.getRepositoryPullRequest).mockResolvedValue( diff --git a/src/commands/pull-request.ts b/src/commands/pull-request.ts index 3f9a10e..ec77c92 100644 --- a/src/commands/pull-request.ts +++ b/src/commands/pull-request.ts @@ -49,6 +49,7 @@ import { FileDiffCoverage } from "../api/client/models/FileDiffCoverage"; import { PullRequestIssuesResponse } from "../api/client/models/PullRequestIssuesResponse"; import { FileAnalysisListResponse } from "../api/client/models/FileAnalysisListResponse"; import { parseDiff } from "../utils/diff"; +import { sanitizeText } from "../utils/sanitize"; import { RepositoryService } from "../api/client/services/RepositoryService"; import { IssueStateBody } from "../api/client/models/IssueStateBody"; @@ -172,13 +173,13 @@ function printAbout( const p = pr.pullRequest; const table = createTable(); table.push({ - Repository: `${providerDisplayName(provider)} / ${organization} / ${p.repository}`, + Repository: `${providerDisplayName(provider)} / ${organization} / ${sanitizeText(p.repository)}`, }); - table.push({ "Pull Request": `#${p.number} — ${p.title}` }); + table.push({ "Pull Request": `#${p.number} — ${sanitizeText(p.title)}` }); table.push({ Status: p.status }); - table.push({ Author: p.owner?.name || ansis.dim("N/A") }); + table.push({ Author: sanitizeText(p.owner?.name) || ansis.dim("N/A") }); table.push({ - Branches: `${p.originBranch || "N/A"} → ${p.targetBranch || "N/A"}`, + Branches: `${sanitizeText(p.originBranch) || "N/A"} → ${sanitizeText(p.targetBranch) || "N/A"}`, }); table.push({ Updated: formatFriendlyDate(p.updated) }); @@ -354,7 +355,7 @@ function printFilesList(files: FileDeltaAnalysis[]): void { : ansis.dim("0"); table.push([ - truncate(f.file.path, 50), + truncate(sanitizeText(f.file.path), 50), `${newI} / ${fixI}`, formatFileDelta(c?.deltaCoverage, true, true), formatFileDelta(q?.deltaComplexity), @@ -445,7 +446,7 @@ function printDiffCoverageSummary(fileCoverageList: FileDiffCoverage[]): void { ? ` | Uncovered lines: ${ansis.red(compressRanges(uncoveredNums))}` : ""; - console.log(`${fc.fileName} | ${pctColored}${uncoveredPart}`); + console.log(`${sanitizeText(fc.fileName)} | ${pctColored}${uncoveredPart}`); } } @@ -527,8 +528,10 @@ function printDiffChange( ? (s) => s : (s) => ansis.dim(s); + // Diff line content is the strongest untrusted sink — arbitrary bytes, + // unbounded length — so neutralize control characters before printing. console.log( - `${leftSymbol} ${numPipeColor(`${numStr} ${changeChar}`)} ${contentColor(change.content)}`, + `${leftSymbol} ${numPipeColor(`${numStr} ${changeChar}`)} ${contentColor(sanitizeText(change.content))}`, ); // Issue annotations below the code line (severity-colored ┃) @@ -537,12 +540,12 @@ function printDiffChange( const cf = severityColorFn(issue.patternInfo.severityLevel); const pipe = cf("┃"); const subCat = issue.patternInfo.subCategory - ? ` ${issue.patternInfo.subCategory}` + ? ` ${sanitizeText(issue.patternInfo.subCategory)}` : ""; const potentialTag = tagged.isPotential ? " Potential false positive" : ""; - const header = `${colorSeverity(issue.patternInfo.severityLevel)} | ${issue.patternInfo.category}${subCat}${potentialTag} #${issue.resultDataId}`; + const header = `${colorSeverity(issue.patternInfo.severityLevel)} | ${sanitizeText(issue.patternInfo.category)}${subCat}${potentialTag} #${issue.resultDataId}`; console.log(`${pipe} ↳ ${header}`); - console.log(`${pipe} ${issue.message}`); + console.log(`${pipe} ${sanitizeText(issue.message)}`); } } @@ -598,7 +601,7 @@ function printAnnotatedDiff( if (!hasRelevant) continue; console.log(sep); - console.log(ansis.bold(file.path)); + console.log(ansis.bold(sanitizeText(file.path))); for (const hunk of file.hunks) { const changes = hunk.changes; @@ -641,7 +644,9 @@ function printAnnotatedDiff( if (interestingIdx.size === 0) continue; - console.log(ansis.dim(hunk.content)); // @@ -x,y +a,b @@ context line + // The @@ header trails a git "section heading" (surrounding code) that is + // attacker-controllable, so neutralize it too. + console.log(ansis.dim(sanitizeText(hunk.content))); // @@ -x,y +a,b @@ context line // Print changes with "..." for skipped stretches let skipped = false; diff --git a/src/commands/repository.ts b/src/commands/repository.ts index 08f6228..e3d2172 100644 --- a/src/commands/repository.ts +++ b/src/commands/repository.ts @@ -25,6 +25,7 @@ import { formatPrIssues, formatAnalysisStatus, } from "../utils/formatting"; +import { sanitizeText } from "../utils/sanitize"; import { AnalysisStatus, pollForAnalysis, @@ -53,10 +54,10 @@ function printAbout( const table = createTable(); table.push( { - Repository: `${providerDisplayName(repo.provider)} / ${repo.owner} / ${repo.name}`, + Repository: `${providerDisplayName(repo.provider)} / ${sanitizeText(repo.owner)} / ${sanitizeText(repo.name)}`, }, { Visibility: repo.visibility }, - { "Default Branch": repo.defaultBranch?.name || "N/A" }, + { "Default Branch": sanitizeText(repo.defaultBranch?.name) || "N/A" }, { "Last Updated": repo.lastUpdated ? formatFriendlyDate(repo.lastUpdated) @@ -174,8 +175,8 @@ function printPullRequests(pullRequests: PullRequestWithAnalysis[]): void { const gates = buildGateStatus(pr); table.push([ String(pr.pullRequest.number), - truncate(pr.pullRequest.title, 40), - truncate(pr.pullRequest.originBranch || "N/A", 20), + truncate(sanitizeText(pr.pullRequest.title), 40), + truncate(sanitizeText(pr.pullRequest.originBranch) || "N/A", 20), formatStandards(pr), formatPrIssues(pr, gates.issues), formatPrCoverage(pr, gates.coverage), @@ -192,7 +193,7 @@ function printCountTable(title: string, counts: Count[]): void { const sorted = [...counts].sort((a, b) => b.total - a.total); const table = createTable({ head: [title, "Count"] }); for (const c of sorted) { - table.push([c.name, String(c.total)]); + table.push([sanitizeText(c.name), String(c.total)]); } console.log(table.toString()); } diff --git a/src/utils/formatting.ts b/src/utils/formatting.ts index 7003387..febc28b 100644 --- a/src/utils/formatting.ts +++ b/src/utils/formatting.ts @@ -14,6 +14,7 @@ import { CveRecord } from "./cve"; import { AnalysisTool } from "../api/client/models/AnalysisTool"; import { Tool } from "../api/client/models/Tool"; import { formatFriendlyDate } from "./output"; +import { sanitizeText } from "./sanitize"; export const SEVERITY_DISPLAY: Record = { Error: "Critical", @@ -101,9 +102,11 @@ const CHAIN_FULL_MAX = 3; * `a@1 → ... 2 more ... → d@4`). Chains with ≤ 3 packages are shown in full. */ export function formatDependencyChain(chain: string[]): string { - if (chain.length <= CHAIN_FULL_MAX) return chain.join(" → "); - const hidden = chain.length - 2; - return `${chain[0]} → ... ${hidden} more ... → ${chain[chain.length - 1]}`; + // Package names come from the repository's manifest — sanitize each segment. + const safe = chain.map((pkg) => sanitizeText(pkg)); + if (safe.length <= CHAIN_FULL_MAX) return safe.join(" → "); + const hidden = safe.length - 2; + return `${safe[0]} → ... ${hidden} more ... → ${safe[safe.length - 1]}`; } /** @@ -112,10 +115,13 @@ export function formatDependencyChain(chain: string[]): string { * longer chains show the (possibly collapsed) import path plus the fixed version. */ function dependencyChainBody(chain: string[], fixedVersion?: string[]): string { - const fixed = fixedVersion?.length ? fixedVersion.join(", ") : ""; + const fixed = fixedVersion?.length + ? fixedVersion.map((v) => sanitizeText(v)).join(", ") + : ""; if (chain.length === 1) { // Direct dependency: the package is imported directly, so show how to fix it. - return fixed ? `Update ${chain[0]} to ${fixed}` : `Update ${chain[0]}`; + const pkg = sanitizeText(chain[0]); + return fixed ? `Update ${pkg} to ${fixed}` : `Update ${pkg}`; } const suffix = fixed ? ` (Fixed in ${fixed})` : ""; return `${formatDependencyChain(chain)}${suffix}`; @@ -133,9 +139,12 @@ export function formatVersionSegment( options?: { includeUpdatePrefix?: boolean }, ): string | null { if (!affectedVersion) return null; - const fixed = fixedVersion?.length ? ` → ${fixedVersion.join(", ")}` : ""; + // Version strings come from the repository's manifest — sanitize them. + const fixed = fixedVersion?.length + ? ` → ${fixedVersion.map((v) => sanitizeText(v)).join(", ")}` + : ""; const prefix = options?.includeUpdatePrefix ? "Update " : ""; - return `${prefix}${affectedVersion}${fixed}`; + return `${prefix}${sanitizeText(affectedVersion)}${fixed}`; } /** @@ -205,21 +214,23 @@ function printIssueCardBody(fields: { console.log(); const severity = colorSeverity(fields.severityLevel); - const subCat = fields.subCategory ? ` ${fields.subCategory}` : ""; + const subCat = fields.subCategory + ? ` ${sanitizeText(fields.subCategory)}` + : ""; const potentialTag = fields.isPotential ? ` ${ansis.dim("|")} ${ansis.dim("POTENTIAL")}` : ""; const id = ansis.hex("#555555")(fields.idText); console.log( - `${severity} ${ansis.dim("|")} ${fields.category}${subCat}${potentialTag} ${id}`, + `${severity} ${ansis.dim("|")} ${sanitizeText(fields.category)}${subCat}${potentialTag} ${id}`, ); - console.log(fields.message); + console.log(sanitizeText(fields.message)); console.log(); - console.log(ansis.dim(`${fields.filePath}:${fields.lineNumber}`)); + console.log(ansis.dim(`${sanitizeText(fields.filePath)}:${fields.lineNumber}`)); if (fields.lineText) { - console.log(ansis.dim(fields.lineText.trim())); + console.log(ansis.dim(sanitizeText(fields.lineText.trim()))); } } @@ -277,15 +288,17 @@ export function printIgnoredIssueCard(issue: IgnoredIssue): void { // Ignore metadata: "Ignored as by · " console.log(); - const reason = issue.reason ? ` as ${issue.reason}` : ""; - const by = issue.ignoredByName ? ` by ${issue.ignoredByName}` : ""; + const reason = issue.reason ? ` as ${sanitizeText(issue.reason)}` : ""; + const by = issue.ignoredByName + ? ` by ${sanitizeText(issue.ignoredByName)}` + : ""; const parts = [`Ignored${reason}${by}`]; if (issue.ignoredTimestamp) { parts.push(formatFriendlyDate(issue.ignoredTimestamp)); } console.log(ansis.dim(parts.join(" · "))); if (issue.comment) { - console.log(ansis.dim(`Comment: ${issue.comment}`)); + console.log(ansis.dim(`Comment: ${sanitizeText(issue.comment)}`)); } console.log(); @@ -567,14 +580,14 @@ export function printCveBlock(cve: CveRecord): void { cna.problemTypes?.[0]?.descriptions?.find((d) => d.lang === "en")?.description; if (title) { console.log(); - console.log(title); + console.log(sanitizeText(title)); } // English description const desc = cna.descriptions?.find((d) => d.lang === "en")?.value; if (desc) { console.log(); - console.log(desc); + console.log(sanitizeText(desc)); } // Deduplicated references from cna and all adp containers @@ -592,7 +605,7 @@ export function printCveBlock(cve: CveRecord): void { console.log(); console.log(ansis.bold("References:")); for (const ref of uniqueRefs) { - console.log(ansis.dim(` ${ref.url}`)); + console.log(ansis.dim(` ${sanitizeText(ref.url)}`)); } } } @@ -612,11 +625,12 @@ export function printFileContext( for (const line of lines) { const num = String(line.number).padStart(width, " "); - const content = line.content; + // File content is attacker-controllable — neutralize before printing. + const content = sanitizeText(line.content); if (line.number === issueLine) { console.log(ansis.bold(`${num} | ${content}`)); if (suggestion) { - console.log(ansis.bold(ansis.green(`${num} | ${suggestion}`))); + console.log(ansis.bold(ansis.green(`${num} | ${sanitizeText(suggestion)}`))); } } else { console.log(ansis.dim(`${num} | ${content}`)); @@ -639,7 +653,7 @@ export function printIssueCodeContext( console.log(); // File path : line - console.log(ansis.dim(`${issue.filePath}:${issue.lineNumber}`)); + console.log(ansis.dim(`${sanitizeText(issue.filePath)}:${issue.lineNumber}`)); console.log(); // Extended code context (or fall back to single line from issue) @@ -648,9 +662,9 @@ export function printIssueCodeContext( } else { // Fallback: just show the lineText we already have const num = String(issue.lineNumber).padStart(4, " "); - console.log(ansis.bold(`${num} | ${issue.lineText}`)); + console.log(ansis.bold(`${num} | ${sanitizeText(issue.lineText)}`)); if (issue.suggestion) { - console.log(ansis.bold(ansis.green(`${num} | ${issue.suggestion}`))); + console.log(ansis.bold(ansis.green(`${num} | ${sanitizeText(issue.suggestion)}`))); } } @@ -677,35 +691,35 @@ export function printIssueCodeContext( if (pattern.description) { console.log(); console.log(ansis.bold("About this pattern")); - console.log(pattern.description); + console.log(sanitizeText(pattern.description)); } // Rationale if (pattern.rationale) { console.log(); console.log(ansis.bold("Why is this a problem?")); - console.log(pattern.rationale); + console.log(sanitizeText(pattern.rationale)); } // Solution if (pattern.solution) { console.log(); console.log(ansis.bold("How to fix it?")); - console.log(pattern.solution); + console.log(sanitizeText(pattern.solution)); } // Tags if (pattern.tags && pattern.tags.length > 0) { console.log(); - console.log(ansis.dim(`Tags: ${pattern.tags.join(", ")}`)); + console.log(ansis.dim(`Tags: ${pattern.tags.map((t) => sanitizeText(t)).join(", ")}`)); } // Detected by console.log(); - const toolName = issue.toolInfo.name; + const toolName = sanitizeText(issue.toolInfo.name); const patternRef = pattern.title - ? `${pattern.title} (${pattern.id})` - : pattern.id; + ? `${sanitizeText(pattern.title)} (${sanitizeText(pattern.id)})` + : sanitizeText(pattern.id); console.log(ansis.dim(`Detected by: ${toolName}`)); console.log(ansis.dim(patternRef)); } @@ -726,11 +740,11 @@ export function printIssueDetail( // Header: Severity | Category SubCategory const severity = colorSeverity(p.severityLevel); - const subCat = p.subCategory ? ` ${ansis.dim(p.subCategory)}` : ""; - console.log(`${severity} ${ansis.dim("|")} ${p.category}${subCat}`); + const subCat = p.subCategory ? ` ${ansis.dim(sanitizeText(p.subCategory))}` : ""; + console.log(`${severity} ${ansis.dim("|")} ${sanitizeText(p.category)}${subCat}`); // Message - console.log(issue.message); + console.log(sanitizeText(issue.message)); // Code context + pattern info (shared with finding command for Codacy-source findings) printIssueCodeContext(issue, pattern, lines); diff --git a/src/utils/sanitize.test.ts b/src/utils/sanitize.test.ts new file mode 100644 index 0000000..c206239 --- /dev/null +++ b/src/utils/sanitize.test.ts @@ -0,0 +1,61 @@ +import { describe, it, expect } from "vitest"; +import { sanitizeText } from "./sanitize"; + +describe("sanitizeText", () => { + it("neutralizes a bare ESC byte (0x1B) with caret notation", () => { + expect(sanitizeText("\x1b")).toBe("^["); + // The raw ESC byte must be gone. + expect(sanitizeText("\x1b")).not.toContain("\x1b"); + }); + + it("neutralizes an SGR colour sequence so it can't repaint output", () => { + // The originally reported vector: a value carrying its own colour codes. + const payload = "\x1b[31mfake error\x1b[0m"; + const out = sanitizeText(payload); + expect(out).toBe("^[[31mfake error^[[0m"); + expect(out).not.toContain("\x1b"); + // The visible text survives. + expect(out).toContain("fake error"); + }); + + it("neutralizes OSC sequences (e.g. clipboard/hyperlink) — no raw ESC or BEL", () => { + const payload = "\x1b]52;c;cGF5bG9hZA==\x07"; + const out = sanitizeText(payload); + expect(out).not.toContain("\x1b"); + expect(out).not.toContain("\x07"); + expect(out).toContain("^["); // ESC (0x1B) → ^[ (the ] is a literal char, kept) + expect(out).toContain("^G"); // BEL (0x07) → ^G + }); + + it("neutralizes DEL (0x7F) and carriage return (0x0D)", () => { + expect(sanitizeText("a\x7fb")).toBe("a^?b"); + // CR can overwrite already-printed output, so it is neutralized too. + expect(sanitizeText("visible\rHIDDEN")).toBe("visible^MHIDDEN"); + }); + + it("neutralizes C1 control bytes (0x80–0x9F) with a hex escape", () => { + // 0x9B is the single-byte CSI introducer. + expect(sanitizeText("x\x9by")).toBe("x\\x9By"); + expect(sanitizeText("\x80")).toBe("\\x80"); + }); + + it("keeps TAB and LF so layout and multi-line text survive", () => { + expect(sanitizeText("a\tb\nc")).toBe("a\tb\nc"); + }); + + it("leaves ordinary text and non-ASCII printable characters unchanged", () => { + expect(sanitizeText("Add new feature → done ✓")).toBe( + "Add new feature → done ✓", + ); + expect(sanitizeText("src/index.ts:10")).toBe("src/index.ts:10"); + }); + + it("passes undefined and null through unchanged", () => { + expect(sanitizeText(undefined)).toBeUndefined(); + expect(sanitizeText(null)).toBeNull(); + }); + + it("returns an empty string for empty input", () => { + expect(sanitizeText("")).toBe(""); + }); +}); diff --git a/src/utils/sanitize.ts b/src/utils/sanitize.ts new file mode 100644 index 0000000..7076fc9 --- /dev/null +++ b/src/utils/sanitize.ts @@ -0,0 +1,61 @@ +/** + * Neutralize terminal control characters in untrusted, repository-derived text + * before it is written to the terminal (CWE-150). + * + * Why this exists: values such as PR titles, author names, file paths, diff and + * file content, finding titles and issue messages come straight from a + * repository an attacker may control. Written raw to a terminal, embedded escape + * bytes (ESC 0x1B and other C0/C1/DEL controls) are interpreted as ANSI/OSC + * control sequences — letting a crafted pull request repaint or hide findings, + * spoof gate status or counts, or drive terminal side effects (OSC 52 clipboard + * writes, OSC 8 hyperlink spoofing). + * + * Why not neutralize at the console boundary: the CLI itself emits legitimate + * ANSI SGR sequences (via `ansis`) for its own colours. Stripping every escape + * at the boundary would break that output, and merely allow-listing the SGR + * family would still let an attacker's own SGR sequences through — the exact + * vector in the original report. So untrusted values are neutralized *before* + * the CLI wraps them in its own styling, centrally in the render helpers. + * + * Neutralized ranges (keeping TAB and LF so layout survives): + * C0: 0x00–0x1F except 0x09 (TAB) and 0x0A (LF) + * DEL: 0x7F + * C1: 0x80–0x9F + * CR (0x0D) is intentionally neutralized too — it can return the cursor to the + * start of the line and overwrite already-printed output. + * + * Each offending byte is replaced with a visible, non-interpretable token + * (caret notation for C0/DEL, `\xNN` for C1) rather than silently dropped, so + * tampering stays evident. Structured JSON output is deliberately left + * untouched — JSON encoding already escapes control bytes. + */ +const CONTROL_CHAR_RE = /[\x00-\x08\x0B-\x1F\x7F-\x9F]/g; + +function neutralizeChar(ch: string): string { + const code = ch.charCodeAt(0); + // C0 controls (0x00–0x1F) → caret notation, e.g. ESC (0x1B) → "^[". + if (code <= 0x1f) return "^" + String.fromCharCode(code + 0x40); + // DEL (0x7F) → "^?". + if (code === 0x7f) return "^?"; + // C1 controls (0x80–0x9F) → hex escape, e.g. 0x9B (CSI) → "\x9B". + return "\\x" + code.toString(16).toUpperCase().padStart(2, "0"); +} + +/** + * Return `value` with terminal control characters neutralized. `undefined` and + * `null` pass through unchanged so callers can wrap optional fields inline + * without disturbing their truthiness checks. + */ +export function sanitizeText(value: string): string; +export function sanitizeText(value: undefined): undefined; +export function sanitizeText(value: null): null; +export function sanitizeText( + value: string | null | undefined, +): string | null | undefined; +export function sanitizeText( + value: string | null | undefined, +): string | null | undefined { + return typeof value === "string" + ? value.replace(CONTROL_CHAR_RE, neutralizeChar) + : value; +}