From 3463f0985af64e003a0aab18d5eb03ef961e26ee Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Fri, 4 Sep 2026 11:49:13 -0700 Subject: [PATCH] =?UTF-8?q?fix:=20ledger=20redaction=20=E2=80=94=20preserv?= =?UTF-8?q?e=20write=20paths=20and=20close=20a=20curl=20-u=20credential=20?= =?UTF-8?q?leak=20introduced=20by=20#1117?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SessionCompaction.redactLedgerDetail` called `Telemetry.maskString` before its own curl-credential redaction. #1117 added a filesystem-path masking pass to `maskString` that collapses any 2+ segment path — including `/usr/bin/curl` — to the literal `` before the curl-context lookback ran. Two consequences: - Every write path in the ledger collapsed to ``, so the ledger could no longer report which file was written (4 failing tests). - With the `curl` token gone, the curl-context lookback found nothing, so a path-qualified curl invocation's space-separated `-u user password` (non-colon-shaped, so not otherwise flagged as credential-shaped) passed through `redactLedgerDetail` unredacted — a real credential leaking into ledger text that is later persisted into a model prompt across compaction. Fix: - `Telemetry.maskString` gains an opt-out for its path-masking pass (`maskPaths`, default `true`, unchanged for every existing caller). Every other mask (api keys, bearer tokens, emails, internal hosts, quote collapsing) still applies regardless. - `redactLedgerDetail` calls `maskString(value, { maskPaths: false })`, restoring write-path fidelity and, because the `curl` token survives, restoring the curl-context lookback. - Belt-and-suspenders: `redactLedgerDetail`'s curl-context detection is also derived independently from the pre-mask raw value (correlated ordinally against the masked-string matches), so it no longer depends solely on `maskPaths:false` — a future masking rule that happens to eat the command name can't quietly reopen this leak. Adds an explicit adversarial test reproducing the leak (`/usr/bin/curl -u alice hunter2 ...` and the `curl.exe` path-qualified variant); confirmed it fails against pre-fix code via `git stash` and passes after. #1117's own telemetry path-masking is untouched for its other callers (mcp/index.ts, sql-execute.ts, tool.ts, prompt.ts, register.ts, dispatcher.ts, registry.ts, warehouse-add.ts, project-scan.ts) — confirmed via test/telemetry/mask-file-paths.test.ts still green. Fixes broken main: origin/main's TypeScript CI job has been red since ~2026-09-04 07:28Z on these 5 tests, blocking all PRs. The leak itself is main-only — not present in v0.10.0 or any released version. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- .../opencode/src/altimate/telemetry/index.ts | 25 +++++++-- packages/opencode/src/session/compaction.ts | 56 +++++++++++++++++-- .../test/session/compaction-ledger.test.ts | 19 +++++++ 3 files changed, 88 insertions(+), 12 deletions(-) diff --git a/packages/opencode/src/altimate/telemetry/index.ts b/packages/opencode/src/altimate/telemetry/index.ts index 39c992b5d..08ca60459 100644 --- a/packages/opencode/src/altimate/telemetry/index.ts +++ b/packages/opencode/src/altimate/telemetry/index.ts @@ -1558,14 +1558,20 @@ export namespace Telemetry { // Each match replaces with a fixed redaction so length-based fingerprinting // can't reconstruct the original token. - export function maskString(s: string): string { + // altimate_change start — maskPaths opt-out for callers that redact on their + // own terms (see compaction.ts::redactLedgerDetail). Default true keeps + // every existing caller's behavior byte-for-byte unchanged; only a caller + // that explicitly passes maskPaths:false skips the #1117 path-masking pass, + // and every other rule (api keys, bearer tokens, emails, internal hosts, + // quote collapsing) still applies. + export function maskString(s: string, opts?: { maskPaths?: boolean }): string { // Consumers truncate masked output to <= 2000 chars; masking beyond 8 KB // buys nothing, and unbounded input is what turns any super-linear rule // into a stall. Input is cut FIRST so every rule — the linear credential/ // quote passes included — does bounded work, and the cut must never fail // a rule open across the boundary. No floor gates any of this: a floor is // a leak past the floor. - if (s.length <= PM_CAP) return pmMask(s) + if (s.length <= PM_CAP) return pmMask(s, opts) // the head ends at whitespace of any kind, so no token straddles it (a // head with no whitespace at all is one token: nothing is emitted) const ws = s.slice(0, PM_CAP).search(/\s\S*$/) @@ -1583,17 +1589,18 @@ export namespace Telemetry { // masked the same way with and without the continuation, cut back to // whitespace. Whatever the continuation changes is dropped, never // emitted half-proven. - const alone = pmMask(head) - const seen = pmMask(s.slice(0, at + PM_LOOKAHEAD)) + const alone = pmMask(head, opts) + const seen = pmMask(s.slice(0, at + PM_LOOKAHEAD), opts) let n = 0 while (n < alone.length && alone[n] === seen[n]) n++ if (n === alone.length) return alone const back = alone.slice(0, n).search(/\s\S*$/) return back >= 0 ? alone.slice(0, back).trimEnd() : "" } + // altimate_change end // the masking chain proper, on bounded input (see maskString) - function pmMask(s: string): string { + function pmMask(s: string, opts?: { maskPaths?: boolean }): string { let out = s // ANSI CSI sequences (colored subprocess stderr) would otherwise split // tokens so neither credential nor path rules can see them @@ -1604,7 +1611,13 @@ export namespace Telemetry { .replace(/"(?:[^"\\]|\\.)*"/g, "?") // Fast path: a string with no separator cannot contain a path — skip the // whole path stack (most telemetry strings carry no path at all). - if (out.includes("/") || out.includes("\\") || /(?` before the curlContext lookback below ever runs, which both + // destroys the write paths this ledger exists to report AND — the actual + // security bug — erases the `curl` token the lookback needs, so a + // path-qualified `curl -u user password` credential slips through + // unredacted. Every other telemetry mask (api keys, bearer tokens, + // emails, internal hosts, quote collapsing) still applies. + let masked = Telemetry.maskString(value, { maskPaths: false }) + + // altimate_change start — belt-and-suspenders curl detection. Derive + // curlContext for each `-u`/`--user` occurrence from the RAW, pre-mask + // `value` as well as from `masked`. maskPaths:false above is what keeps + // the `curl` token intact today; this makes detection structurally + // independent of that single flag, so a future masking rule that happens + // to eat the command-name token can't quietly reopen this leak. The two + // occurrence lists are correlated by ordinal position (same regex, same + // match order); if a prior mask ever changes how many times the pattern + // matches, this safely falls back to the masked-only signal — no worse + // than before this change. + const rawCurlByOrdinal = [...value.matchAll(USER_FLAG_RE)].map((m) => + isCurlContext(shellSegmentBefore(value, m.index + m[1].length)), + ) + const maskedOccurrenceCount = [...masked.matchAll(USER_FLAG_RE)].length + const ordinalsAligned = maskedOccurrenceCount === rawCurlByOrdinal.length + let ordinal = 0 + // altimate_change end // `-u` is also a benign flag for commands such as `git push -u` and // `python -u`. Redact it as authentication only in the current curl shell @@ -677,7 +717,7 @@ export namespace SessionCompaction { // `--user` follows the same rule so task literals are not discarded merely // because an unrelated CLI chose that option name. masked = masked.replace( - /(^|\s)(--user|-u)(?:(=|\s+)("[^"]*"|'[^']*'|[^\s,;]+)|([^\s,;]+))(?:(\s+)("[^"]*"|'[^']*'|[^\s,;]+))?/gi, + USER_FLAG_RE, ( match, lead: string, @@ -690,13 +730,17 @@ export namespace SessionCompaction { offset: number, whole: string, ) => { + // altimate_change — capture and advance the ordinal before any early + // return so it always tracks this callback's position in `masked`'s + // match sequence, matching how rawCurlByOrdinal was built. + const currentOrdinal = ordinal++ // Attached values are valid only for short `-u` (`-ualice:pass`). if (flag.toLowerCase() === "--user" && separator === undefined) return match const rawValue = (separatedValue ?? attachedValue ?? "").replace(/^["']|["']$/g, "") - // Windows invokes curl as `curl.exe`, and either platform may reach it - // through a path such as /usr/bin/curl or a Windows System32 path. - // Missing those spellings left the `-u` VALUE unredacted. - const curlContext = /(?:^|[\s/\\])curl(?:\.exe)?(?=\s|$)/i.test(shellSegmentBefore(whole, offset + lead.length)) + const maskedCurlContext = isCurlContext(shellSegmentBefore(whole, offset + lead.length)) + // altimate_change — OR in the raw-value signal (see block above). + const curlContext = + maskedCurlContext || (ordinalsAligned && (rawCurlByOrdinal[currentOrdinal] ?? false)) // Outside a curl context a colon-shaped value is treated as // user:password. The ONE exemption is an explicitly recognized // all-numeric UID:GID pair (`docker run --user 1000:1000`), which is a diff --git a/packages/opencode/test/session/compaction-ledger.test.ts b/packages/opencode/test/session/compaction-ledger.test.ts index afe0afc0c..bb724360f 100644 --- a/packages/opencode/test/session/compaction-ledger.test.ts +++ b/packages/opencode/test/session/compaction-ledger.test.ts @@ -439,6 +439,25 @@ describe("SessionCompaction.renderLedger", () => { expect(SessionCompaction.redactLedgerDetail("curlywurly -u alice script.py")).toBe("curlywurly -u alice script.py") }) + test("does not leak credentials via a path-qualified curl and the space-separated -u idiom (regression, #1117)", () => { + // #1117 added a filesystem-path masking rule to Telemetry.maskString. + // redactLedgerDetail used to call maskString BEFORE its own curl-context + // lookback, so `/usr/bin/curl` collapsed to `` first. The `curl` + // token was gone by the time the lookback ran: curlContext came back + // false, `-u alice hunter2` (space-separated, non-colon-shaped) isn't + // credentialShaped either, and the credential passed through untouched + // into ledger text that later gets persisted into a model prompt across + // compaction. This is the exact adversarial shape of that leak. + for (const command of [ + "/usr/bin/curl -u alice hunter2 https://example.com", + "/usr/local/bin/curl.exe -u alice hunter2 https://example.com", + ]) { + const detail = SessionCompaction.redactLedgerDetail(command) + expect(detail).not.toContain("alice") + expect(detail).not.toContain("hunter2") + } + }) + test("keeps non-credential colon-shaped values outside a curl context", () => { // `1000:1000` has no alphabetic character before the colon, so it is a // UID:GID pair rather than user:password and must survive in the ledger.