fix: neutralize terminal control characters in human-readable output by alerizzo · Pull Request #30 · codacy/codacy-cloud-cli

alerizzo · 2026-07-24T10:02:36Z

Summary

Repository-derived values shown by the CLI (PR/finding titles, author names, branches, file paths, diff/file content, issue messages, package names) were written to the terminal without neutralizing control bytes — a crafted pull request could inject ANSI/OSC escape sequences to repaint or hide findings, spoof gate status, or trigger terminal side effects like clipboard writes (CWE-150, HackerOne, OD-309).
New sanitizeText() (src/utils/sanitize.ts) neutralizes C0 (0x00–0x1F except TAB/LF), DEL (0x7F) and C1 (0x80–0x9F) bytes — CR included — replacing each with visible caret / \xNN notation.
Applied before the CLI's own ansis styling, centrally in the shared render helpers (utils/formatting.ts) and at the render points of every command that prints untrusted values (pull-request, findings/finding, issues, issue, repository, ls/directories). Sanitizing at the console.log boundary isn't viable — it would strip the CLI's own legitimate colours, and allow-listing SGR would still pass an attacker's SGR through (the originally reported vector).
--output json is deliberately left untouched — JSON encoding already escapes control bytes, so consumers still get the original values.

Test plan

npm test — 490 pass (12 new: 9 sanitize unit tests + 3 pull-request regression tests covering the About table, the annotated diff view, and JSON output staying intact)
npx tsc -p tsconfig.build.json --noEmit — clean
Manual: run codacy pull-request <a PR whose title/diff contains a raw ESC byte> and confirm the escape renders as ^[… instead of being interpreted; --output json still shows the raw value.

🤖 Generated with Claude Code

Repository-derived values (PR/finding titles, author names, branches, file paths, diff/file content, issue messages, package names) were written to the terminal without neutralizing control bytes, so a crafted pull request could inject ANSI/OSC escape sequences to repaint or hide findings, spoof gate status, or trigger terminal side effects (CWE-150). Add sanitizeText() and apply it to every untrusted value *before* the CLI wraps it in its own ansis styling — the console boundary can't be used because the CLI legitimately emits colour codes there. JSON output is left untouched (JSON encoding already escapes control bytes). Fixes OD-309 (HackerOne). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

codacy-production · 2026-07-24T10:05:12Z

Not up to standards ⛔

🔴 Issues 1 medium · 2 minor

Alerts:
⚠ 3 issues (≤ 0 issues of at least minor severity)

Results:
3 new issues

Category Results

BestPractice 1 medium (1 false positive)

Comprehensibility 2 minor

View in Codacy

🟢 Metrics 19 complexity · 0 duplication

Metric Results

Complexity 19

Duplication 0

View in Codacy

AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.

_{TIP This summary will be updated as you push new changes.}

codacy-production

Pull Request Overview

This PR implements critical protections against terminal control character attacks (CWE-150) by neutralizing potentially dangerous sequences in repository-derived data. While the core sanitization logic is sound, the PR is currently not up to standards due to a visual regression and documentation gaps.

Specifically, the findings command fails to handle optional scanType fields correctly, which will result in the string 'undefined' appearing in the terminal. Additionally, the security instructions in AGENTS.md need to be more granular to ensure consistent enforcement across the codebase. Addressing these will ensure the fix is both secure and maintains high output quality.

Test suggestions

Found recommended test scenario: Unit tests for sanitizeText verify correct neutralization of ESC, BEL, DEL, CR, and C1 bytes.
Found recommended test scenario: Unit tests for sanitizeText verify that TAB and LF characters are preserved.
Found recommended test scenario: Regression test ensures PR metadata (title, author, branch) containing escape sequences is neutralized in terminal output.
Found recommended test scenario: Regression test ensures annotated diff content containing escape sequences is neutralized in terminal output.
Found recommended test scenario: Regression test confirms that --output json preserves original escape sequences (escaped via JSON encoding) for downstream tools.
Found recommended test scenario: Verify sanitization is applied to SCA dependency package names and version strings.

_{TIP Improve review quality by adding custom instructions}
_{TIP How was this review? Give us feedback}

codacy-production · 2026-07-24T10:06:20Z

  - `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):** Any repository-derived value written to the human-readable output (PR/finding titles, author names, branches, file paths, diff/file content, issue messages, package names, etc.) 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. Do **not** sanitize at the `console.log` boundary (that would strip the CLI's own legitimate colors) and do **not** sanitize the `--output json` path (JSON encoding already escapes control bytes). When adding a new command or render helper, sanitize each untrusted field at the point the raw value enters the string.


_{🟡 MEDIUM RISK}

The security policy for CWE-150 is currently defined in a complex paragraph using vague references ('etc.'). This should be broken into specific, actionable instructions to ensure consistent application.

Suggested change

- **Untrusted output (CWE-150):** Any repository-derived value written to the human-readable output (PR/finding titles, author names, branches, file paths, diff/file content, issue messages, package names, etc.) 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. Do **not** sanitize at the `console.log` boundary (that would strip the CLI's own legitimate colors) and do **not** sanitize the `--output json` path (JSON encoding already escapes control bytes). When adding a new command or render helper, sanitize each untrusted field at the point the raw value enters the string.

- **Untrusted output (CWE-150: Common Weakness Enumeration for Control Characters):** 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.

- **Sinks to sanitize:** PR/finding titles, author names, branches, file paths, diff/file content, issue messages, and package names.

- **Where NOT to sanitize:** Do not sanitize at the `console.log` boundary (this strips legitimate CLI colors). Do not sanitize for `--output json` (JSON encoding handles escapes).

- **Requirement:** When adding a new command or render helper, sanitize each untrusted field at the point the raw value enters the string.

See Issue in Codacy
See Issue in Codacy
See Issue in Codacy

Done in 3324ac2 — split the CWE-150 guidance into granular bullets (sinks / where-not-to-sanitize / when-adding-a-command).

codacy-production · 2026-07-24T10:06:20Z

+  const catParts = [sanitizeText(item.securityCategory), ansis.dim(sanitizeText(item.scanType))]
    .filter(Boolean)
    .join(" ");


_{🟡 MEDIUM RISK}

The item.scanType field is optional. Passing the result of sanitizeText(item.scanType) directly to ansis.dim without checking for existence will cause the string "undefined" or "null" to be printed in the output when the field is missing, as the styling function will stringify its argument.

Fixed in 3324ac2. Note the stated failure mode isn't reproducible on this ansis version — ansis.dim(undefined) returns "" (falsy), which .filter(Boolean) drops, so no "undefined" was printed. But you're right that the list view didn't guard scanType while the detail view (finding.ts) does, so I've added the item.scanType ? … : undefined guard to make them consistent and future-proof.

codacy-production · 2026-07-24T10:06:20Z

            endLine,
          ).catch((e) => {
-            console.log("File path: ", issue.filePath);
+            console.log("File path: ", sanitizeText(issue.filePath));


_{⚪ LOW RISK}

Suggestion: For better consistency in error reporting, use console.error and apply the same red styling as the adjacent error message.

Suggested change

console.log("File path: ", sanitizeText(issue.filePath));

console.error(ansis.red(`File path: ${sanitizeText(issue.filePath)}`));

Done in 3324ac2 — the file-path diagnostic now uses console.error(ansis.red(…)), consistent with the adjacent error line.

codacy-production · 2026-07-24T10:06:20Z


-  if (item.application) line3Parts.push(ansis.dim(item.application));
+  if (item.application) line3Parts.push(ansis.dim(sanitizeText(item.application)));
  //if (item.affectedTargets) line3Parts.push(ansis.dim(item.affectedTargets));


_{⚪ LOW RISK}

Nitpick: This commented-out line is redundant as affectedTargets is now handled on line 105. Removing it will help keep the code clean.

Done in 3324ac2 — removed the stale commented-out affectedTargets line.

- findings.ts: guard optional scanType before ansis.dim, matching the finding detail view (defensive; avoids relying on ansis coercing undefined to "") and drop a stale commented-out affectedTargets line. - issue.ts: route the file-path diagnostic through console.error + red, consistent with the adjacent error line. - AGENTS.md: break the CWE-150 output guidance into granular bullets (sinks / where-not-to-sanitize / when-adding-a-command). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

codacy-production Bot reviewed Jul 24, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: neutralize terminal control characters in human-readable output#30

fix: neutralize terminal control characters in human-readable output#30
alerizzo wants to merge 2 commits into
mainfrom
fix/neutralize-control-chars

alerizzo commented Jul 24, 2026

Uh oh!

codacy-production Bot commented Jul 24, 2026 •

edited

Loading

Uh oh!

codacy-production Bot left a comment

Uh oh!

codacy-production Bot Jul 24, 2026

Uh oh!

alerizzo Jul 24, 2026

Uh oh!

codacy-production Bot Jul 24, 2026

Uh oh!

alerizzo Jul 24, 2026

Uh oh!

codacy-production Bot Jul 24, 2026

Uh oh!

alerizzo Jul 24, 2026

Uh oh!

codacy-production Bot Jul 24, 2026

Uh oh!

alerizzo Jul 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

-- **Untrusted output (CWE-150):** Any repository-derived value written to the human-readable output (PR/finding titles, author names, branches, file paths, diff/file content, issue messages, package names, etc.) 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. Do **not** sanitize at the `console.log` boundary (that would strip the CLI's own legitimate colors) and do **not** sanitize the `--output json` path (JSON encoding already escapes control bytes). When adding a new command or render helper, sanitize each untrusted field at the point the raw value enters the string.
+- **Untrusted output (CWE-150: Common Weakness Enumeration for Control Characters):** 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.
+  - **Sinks to sanitize:** PR/finding titles, author names, branches, file paths, diff/file content, issue messages, and package names.
+  - **Where NOT to sanitize:** Do not sanitize at the `console.log` boundary (this strips legitimate CLI colors). Do not sanitize for `--output json` (JSON encoding handles escapes).
+  - **Requirement:** When adding a new command or render helper, sanitize each untrusted field at the point the raw value enters the string.

	console.log("File path: ", sanitizeText(issue.filePath));
	console.error(ansis.red(`File path: ${sanitizeText(issue.filePath)}`));

Uh oh!

Conversation

alerizzo commented Jul 24, 2026

Summary

Test plan

Uh oh!

codacy-production Bot commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Not up to standards ⛔

Uh oh!

codacy-production Bot left a comment

Choose a reason for hiding this comment

Pull Request Overview

Test suggestions

Uh oh!

codacy-production Bot Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

alerizzo Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

alerizzo Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

alerizzo Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

alerizzo Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

codacy-production Bot commented Jul 24, 2026 •

edited

Loading