Skip to content

fix(security): avoid logging raw AI CLI stderr - #1608

Closed
JSONbored wants to merge 1 commit into
mainfrom
codex/propose-fix-for-cli-stderr-vulnerability
Closed

fix(security): avoid logging raw AI CLI stderr#1608
JSONbored wants to merge 1 commit into
mainfrom
codex/propose-fix-for-cli-stderr-vulnerability

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • Subprocess claude/codex stderr was being appended into thrown errors and emitted at error level, and the existing redactSecrets() only covered a small set of known shapes, leaving a risk of free-text credential leakage into logs/Sentry.
  • The goal is to preserve diagnosability (class of failure) while preventing arbitrary untrusted stderr from being copied into error fields that Sentry will forward.

Description

  • Add summarizeCliStderr() to convert untrusted CLI stderr into a bounded, allowlisted summary (e.g. auth_error, rate_limit, model_not_supported, timeout, permission_denied, stderr_captured, no_stderr) while applying redactSecrets() first. (file: src/selfhost/ai.ts).
  • Replace raw/redacted stderr embedding in non-zero-exit errors for Claude and Codex with summarizeCliStderr() so thrown errors carry a safe summary instead of arbitrary text. (file: src/selfhost/ai.ts).
  • Add/modify unit tests to assert the new safe summaries, that known secrets are not present, that key-shaped tokens are suppressed, and that arbitrary/proxy/prompt-like stderr is withheld. (file: test/unit/selfhost-ai.test.ts).

Testing

  • Ran npx vitest run test/unit/selfhost-ai.test.ts, and the modified unit suite passed (tests covering summarizeCliStderr and the updated error behavior succeeded).
  • Ran npm run typecheck and git diff --check, both succeeded locally.
  • Attempted full coverage with npm run test:coverage, but the run failed in this environment due to unrelated long-running/timeouts in other suites and a Vitest coverage provider error (TypeError: jsTokens is not a function); the failure appears unrelated to these changes.

Codex Task

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jun 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.51%. Comparing base (427eb35) to head (1440e06).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1608   +/-   ##
=======================================
  Coverage   95.51%   95.51%           
=======================================
  Files         204      204           
  Lines       22042    22050    +8     
  Branches     7964     7971    +7     
=======================================
+ Hits        21053    21061    +8     
  Misses        413      413           
  Partials      576      576           
Files with missing lines Coverage Δ
src/selfhost/ai.ts 99.47% <100.00%> (+0.02%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@loopover-orb

loopover-orb Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Caution

🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥

🛑 Gittensory review — blocked

2 files · 1 AI reviewers · no blockers · readiness 66/100 · CI green · dirty

🛑 Blocked

Review summary
Clean, well-scoped security fix that replaces arbitrary-length redacted stderr with a bounded allowlisted classifier. The core approach is correct: `redactSecrets` runs before pattern matching so the regex never sees raw credentials; the five-category plus two-fallback design preserves operational diagnosability (#26) without copying untrusted text into Sentry-forwarded error fields. All updated test assertions trace correctly to real code paths — no fabricated scenarios, and the integration test using a real script stub still exercises the `defaultSpawn` path end-to-end.

Signal Result Evidence
Code review ✅ No blockers 1 reviewers, synthesized
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Review load ✅ 20/20 Readiness component derived from cached public PR metadata and labels; size label size:S.
Validation evidence ❌ 5/25 Cached preflight status is hold.
Open PR queue ❌ 3/10 24 open PR(s), 10 likely reviewable, 14 unlinked.
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 80 PR(s), 293 issue(s).
Gate result ✅ Passing No configured blocker found.
Nits — 5 non-blocking
  • src/selfhost/ai.ts:280 — `token` has no word boundary in the auth_error regex, so 'unexpected token in JSON', 'tokenize failed', or 'token count exceeded' all misclassify as auth_error instead of stderr_captured, undermining the diagnosability goal the PR calls out; add `\b`: `|\btoken\b|`.
  • src/selfhost/ai.ts:283 — `model.*invalid` matches 'model response is invalid' (an output-parse error), and `model.*unknown` matches 'model state unknown' (a runtime error) — both classify as model_not_supported when they aren't; tighten the suffix set to only unambiguous qualifiers like `not supported|unsupported|not found`.
  • src/selfhost/ai.ts:279 — the JSDoc says nothing about what `stderr_captured` signals vs `no_stderr`; callers reading logs or Sentry will have to reverse-engineer the contract; one sentence like 'Returns stderr_captured for non-empty unclassified stderr to signal that diagnostics exist but were withheld' makes it explicit.
  • test/unit/selfhost-ai.test.ts:552 — there is no test for the edge where stderr is entirely a known secret (e.g. just the raw token value); after `redactSecrets` it becomes '[redacted]' which is truthy and returns stderr_captured rather than no_stderr — a single assertion would pin that intended behavior and guard against future drift.
  • src/selfhost/ai.ts:280 — Add `\b` word boundaries around `token` and `credential` to cut false positives: replace the auth fragment with `\bauth(?:entication)?\b|api[- ]key|\boauth\b|\btoken\b|\bcredential\b|unauthorized|forbidden`.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: not available
  • Official Gittensor activity: 80 PR(s), 293 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Fix blocker.
  • Expect slower review.
  • Refresh registry data or choose a registered active repo.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Review load = cached public PR metadata such as size labels, changed paths, and preflight status.
  • Open PR queue = repo-wide review pressure; it is not a PR quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
Review details

Generated from public PR metadata and the diff. Advisory only; deterministic signals remain authoritative.

Clean, well-scoped security fix that replaces arbitrary-length redacted stderr with a bounded allowlisted classifier. The core approach is correct: `redactSecrets` runs before pattern matching so the regex never sees raw credentials; the five-category plus two-fallback design preserves operational diagnosability (#26) without copying untrusted text into Sentry-forwarded error fields. All updated test assertions trace correctly to real code paths — no fabricated scenarios, and the integration test using a real script stub still exercises the `defaultSpawn` path end-to-end.

Nits (5)

  • src/selfhost/ai.ts:280 — `token` has no word boundary in the auth_error regex, so 'unexpected token in JSON', 'tokenize failed', or 'token count exceeded' all misclassify as auth_error instead of stderr_captured, undermining the diagnosability goal the PR calls out; add `\b`: `|\btoken\b|`.
  • src/selfhost/ai.ts:283 — `model.*invalid` matches 'model response is invalid' (an output-parse error), and `model.*unknown` matches 'model state unknown' (a runtime error) — both classify as model_not_supported when they aren't; tighten the suffix set to only unambiguous qualifiers like `not supported|unsupported|not found`.
  • src/selfhost/ai.ts:279 — the JSDoc says nothing about what `stderr_captured` signals vs `no_stderr`; callers reading logs or Sentry will have to reverse-engineer the contract; one sentence like 'Returns stderr_captured for non-empty unclassified stderr to signal that diagnostics exist but were withheld' makes it explicit.
  • test/unit/selfhost-ai.test.ts:552 — there is no test for the edge where stderr is entirely a known secret (e.g. just the raw token value); after `redactSecrets` it becomes '[redacted]' which is truthy and returns stderr_captured rather than no_stderr — a single assertion would pin that intended behavior and guard against future drift.
  • src/selfhost/ai.ts:280 — Add `\b` word boundaries around `token` and `credential` to cut false positives: replace the auth fragment with `\bauth(?:entication)?\b|api[- ]key|\boauth\b|\btoken\b|\bcredential\b|unauthorized|forbidden`.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot added gittensor gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. labels Jun 27, 2026
@JSONbored

Copy link
Copy Markdown
Owner Author

Closing: the raw-stderr leak this addresses is already fixed on main — createClaudeCodeAi/createCodexAi wrap CLI stderr in redactSecrets(stderr, [token]) before it enters the thrown error message (shipped in #1605). This branch also conflicts with main (it predates the #1605/#1613 CLI error-handling rework). No action needed.

@JSONbored JSONbored closed this Jun 28, 2026
@JSONbored
JSONbored deleted the codex/propose-fix-for-cli-stderr-vulnerability branch June 28, 2026 05:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant