diff --git a/packages/ai/CHANGELOG.md b/packages/ai/CHANGELOG.md index 9cc1dbae5..267eb8e7c 100644 --- a/packages/ai/CHANGELOG.md +++ b/packages/ai/CHANGELOG.md @@ -24,6 +24,7 @@ ### Changed - Anthropic OAuth requests now reproduce Cowork's current `claude-desktop` request profile, including client/runtime metadata, beta selection, system and billing attestation, the 64K output cap, and stable HTTP/1.1 header ordering. +- Exported `SENSITIVE_TOKEN_RE` from `providers/transform-messages` so hosts can route the same credential shapes through reversible obfuscation instead of the irreversible redaction fallback ([#6968](https://github.com/can1357/oh-my-pi/issues/6968)). ## [17.2.0] - 2026-07-30 diff --git a/packages/ai/src/providers/transform-messages.ts b/packages/ai/src/providers/transform-messages.ts index dba8dcf8d..25fc5acc0 100644 --- a/packages/ai/src/providers/transform-messages.ts +++ b/packages/ai/src/providers/transform-messages.ts @@ -295,7 +295,15 @@ function normalizeAnthropicTargetToolCallId( * - Preserves tool call structure (unlike converting to text summaries) * - Injects synthetic "aborted" tool results */ -const SENSITIVE_TOKEN_RE = +/** + * Credential-shaped token patterns scrubbed from outbound provider traffic when + * credential redaction is enabled. Exported so hosts can route the same shapes + * through reversible obfuscation (keyed placeholders restored before local tool + * execution) instead of the irreversible `[*_token_redacted]` rewrite below — + * an irreversible placeholder echoed back in edit-tool `old_text` can never + * match the real bytes on disk. + */ +export const SENSITIVE_TOKEN_RE = /(? 0) { - // The persisted placeholder key — and creating its key file under the - // configured agentDir — is only needed for reversible obfuscate-mode - // placeholders, or for a default (no custom `replacement`) replace-mode - // regex whose key-derived idempotent fallback marker needs a stable key - // across restarts (see `secretEntryNeedsPlaceholderKey`). A replace-only - // secrets set with no such regex must not require the key; otherwise a - // headless run with an unwritable default config root fails startup for a - // feature it does not use. - obfuscator = new SecretObfuscator(allEntries, placeholderKey); + obfuscator = new SecretObfuscator(allEntries, placeholderKey ?? (() => getSecretPlaceholderKeySync(agentDir))); } if (obfuscator?.hasSecrets() !== true && placeholderKey !== undefined) { // No configured entry produced an active secret (e.g. only ignored short diff --git a/packages/coding-agent/src/secrets/index.ts b/packages/coding-agent/src/secrets/index.ts index 6b53be2f5..e94eba24e 100644 --- a/packages/coding-agent/src/secrets/index.ts +++ b/packages/coding-agent/src/secrets/index.ts @@ -1,6 +1,7 @@ import * as crypto from "node:crypto"; -import * as fs from "node:fs/promises"; +import * as fs from "node:fs"; import * as path from "node:path"; +import { SENSITIVE_TOKEN_RE } from "@oh-my-pi/pi-ai/providers/transform-messages"; import { getAgentDir, isEnoent, logger } from "@oh-my-pi/pi-utils"; import { YAML } from "bun"; import { regexHasUnresolvableShortMatchFallback, type SecretEntry, sanitizeSecretFriendlyName } from "./obfuscator"; @@ -31,9 +32,9 @@ export async function getSecretPlaceholderKey(keyDir: string = getAgentDir()): P } const generated = crypto.randomBytes(32).toString("base64url"); - await fs.mkdir(keyDir, { recursive: true }); + await fs.promises.mkdir(keyDir, { recursive: true }); try { - await fs.writeFile(keyPath, generated, { flag: "wx", mode: 0o600 }); + await fs.promises.writeFile(keyPath, generated, { flag: "wx", mode: 0o600 }); cachedPlaceholderKeys.set(keyPath, generated); return generated; } catch (err) { @@ -71,6 +72,60 @@ export async function getExistingSecretPlaceholderKey(keyDir: string = getAgentD return existing; } +// Process-stable fallback for the sync lazy path when the key file cannot be +// persisted (e.g. unwritable config root on a headless run). Memoized so +// re-obfuscation within the process stays idempotent; placeholders simply lose +// cross-session stability, matching `defaultPlaceholderKey()` in obfuscator.ts. +let ephemeralSyncPlaceholderKey: string | undefined; + +/** + * Synchronous variant of `getSecretPlaceholderKey` for the lazy key provider + * `SecretObfuscator` invokes inside its synchronous `obfuscate()` path when a + * built-in credential-pattern entry first matches session content. Never + * throws: an unreadable or unwritable key file degrades to a process-ephemeral + * key (with a warning) instead of breaking the session. + */ +export function getSecretPlaceholderKeySync(keyDir: string = getAgentDir()): string { + const keyPath = path.join(keyDir, "secret-placeholder.key"); + const cached = cachedPlaceholderKeys.get(keyPath); + if (cached !== undefined) return cached; + try { + const existing = fs.readFileSync(keyPath, "utf8").trim(); + if (PLACEHOLDER_KEY_RE.test(existing)) { + cachedPlaceholderKeys.set(keyPath, existing); + return existing; + } + } catch { + // Missing or unreadable — attempt creation below. + } + const generated = crypto.randomBytes(32).toString("base64url"); + try { + fs.mkdirSync(keyDir, { recursive: true }); + fs.writeFileSync(keyPath, generated, { flag: "wx", mode: 0o600 }); + cachedPlaceholderKeys.set(keyPath, generated); + return generated; + } catch (err) { + if ((err as NodeJS.ErrnoException).code === "EEXIST") { + // Another process won the create race; accept its key if valid. + try { + const winner = fs.readFileSync(keyPath, "utf8").trim(); + if (PLACEHOLDER_KEY_RE.test(winner)) { + cachedPlaceholderKeys.set(keyPath, winner); + return winner; + } + } catch { + // Fall through to the ephemeral key. + } + } + logger.warn("Could not persist secret placeholder key, using a process-ephemeral key", { + path: keyPath, + error: String(err), + }); + ephemeralSyncPlaceholderKey ??= crypto.randomBytes(32).toString("base64url"); + return ephemeralSyncPlaceholderKey; + } +} + /** Read and validate the key file, optionally retrying briefly until a valid key lands. */ async function readPlaceholderKeyFile(keyPath: string, retry: boolean): Promise { const attempts = retry ? 50 : 1; @@ -145,6 +200,31 @@ export function collectEnvSecrets(): SecretEntry[] { return entries; } +/** + * Built-in entries covering credential-shaped tokens (GitHub/GitLab/OpenAI-style + * API keys) that are NOT configured via secrets.yml or the environment. Without + * these, such a token in a tool result falls through to pi-ai's irreversible + * provider-boundary redaction (`[openai_token_redacted]`); the model then echoes + * that placeholder into edit-tool `old_text`, which can never match the real + * bytes on disk (issue #6968). Routing the same shapes through the obfuscator + * mints reversible keyed placeholders that `deobfuscateToolArguments` restores + * before tool execution, keeping exact-match edits working while the credential + * bytes still never reach the provider. Unlike the pi-ai redaction there is no + * entropy gate here — a false positive only over-obfuscates, which stays + * transparent because the round trip is lossless. + */ +export function builtinCredentialSecretEntries(): SecretEntry[] { + return [ + { + type: "regex", + content: SENSITIVE_TOKEN_RE.source, + flags: "i", + mode: "obfuscate", + friendlyName: "Credential", + }, + ]; +} + async function loadSecretsFile(filePath: string): Promise { try { const text = await Bun.file(filePath).text(); diff --git a/packages/coding-agent/src/secrets/obfuscator.ts b/packages/coding-agent/src/secrets/obfuscator.ts index 57c488e61..c422f7078 100644 --- a/packages/coding-agent/src/secrets/obfuscator.ts +++ b/packages/coding-agent/src/secrets/obfuscator.ts @@ -594,18 +594,22 @@ export class SecretObfuscator { /** Whether any secrets were configured */ #hasAny: boolean; - /** Private per-install (or per-process) key for the keyed placeholder digest. */ - readonly #key: string; + /** + * Private per-install (or per-process) key for the keyed placeholder digest. + * Resolved lazily when the constructor received a key PROVIDER: the first + * placeholder mint or keyed fallback marker triggers resolution, so callers + * can defer persisting `secret-placeholder.key` until a dynamic regex entry + * actually matches session content. + */ + #key: string | undefined; + #keyProvider: (() => string) | undefined; - constructor(entries: SecretEntry[], key: string = defaultPlaceholderKey()) { - this.#key = key; - // The keyed-hash key makes obfuscate-mode placeholder bases un-dictionaryable, - // but it can be persisted in a user-readable file (`secret-placeholder.key`). - // A prompt-injected tool read (read/bash) could otherwise surface it to the - // provider verbatim and undo that protection, so redact the key itself from - // obfuscated (provider-visible) output as a one-way secret. - this.#replaceMappings.set(key, this.#generateSecretReplacement(key)); - this.#configuredSecretValues.add(key); + constructor(entries: SecretEntry[], key: string | (() => string) = defaultPlaceholderKey()) { + if (typeof key === "function") { + this.#keyProvider = key; + } else { + this.#setPlaceholderKey(key); + } // Collect every configured plain-secret literal AND compile every regex // entry BEFORE minting any placeholder below, so a placeholder's friendly // name (checked against both in `#createPlaceholder`) can never embed a @@ -671,6 +675,30 @@ export class SecretObfuscator { this.#hasAny = hasRealSec; } + /** + * The keyed-hash key makes obfuscate-mode placeholder bases un-dictionaryable, + * but it can be persisted in a user-readable file (`secret-placeholder.key`). + * A prompt-injected tool read (read/bash) could otherwise surface it to the + * provider verbatim and undo that protection, so redact the key itself from + * obfuscated (provider-visible) output as a one-way secret. + */ + #setPlaceholderKey(key: string): void { + this.#key = key; + this.#replaceMappings.set(key, this.#generateSecretReplacement(key)); + this.#configuredSecretValues.add(key); + } + + /** Resolve the placeholder key once, minting its self-redaction on first use. */ + #getKey(): string { + let key = this.#key; + if (key === undefined) { + key = this.#keyProvider?.() ?? defaultPlaceholderKey(); + this.#keyProvider = undefined; + this.#setPlaceholderKey(key); + } + return key; + } + hasSecrets(): boolean { return this.#hasAny; } @@ -937,7 +965,9 @@ export class SecretObfuscator { // remainder bytes that merely look sentinel-shaped (e.g. `ZZZZ`) cannot equal // the marker and are still redacted instead of passed through. const replacement = - chunk.length <= 2 ? "Z".repeat(chunk.length) : `ZZ${buildKeyedReplacementRun(this.#key, chunk.length - 2)}`; + chunk.length <= 2 + ? "Z".repeat(chunk.length) + : `ZZ${buildKeyedReplacementRun(this.#getKey(), chunk.length - 2)}`; this.#generatedReplaceChunks.add(replacement); return replacement; } @@ -1023,7 +1053,9 @@ export class SecretObfuscator { // the ordinary keyed-run fallback #generateReplacement already uses. replacement = stable ?? - (value.length <= 2 ? buildKeyedReplacementRun(this.#key, value.length) : this.#generateReplacement(value)); + (value.length <= 2 + ? buildKeyedReplacementRun(this.#getKey(), value.length) + : this.#generateReplacement(value)); regex.lastIndex = 0; } this.#generatedReplaceChunks.add(replacement); @@ -1183,7 +1215,9 @@ export class SecretObfuscator { for (let attempt = 0; ; attempt++) { const base = - attempt === 0 ? buildHashBase(this.#key, baseKey) : buildHashBase(this.#key, `${baseKey}\0${attempt}`); + attempt === 0 + ? buildHashBase(this.#getKey(), baseKey) + : buildHashBase(this.#getKey(), `${baseKey}\0${attempt}`); const owner = this.#placeholderBaseOwners.get(base); if (owner !== undefined && owner !== baseKey) continue; this.#placeholderBaseOwners.set(base, baseKey); @@ -1195,7 +1229,7 @@ export class SecretObfuscator { #reserveFallbackPlaceholderBase(baseKey: string, startAttempt: number): string { for (let attempt = startAttempt; ; attempt++) { const owner = `${baseKey}\0collision\0${attempt}`; - const base = buildHashBase(this.#key, `${baseKey}\0collision\0${attempt}`); + const base = buildHashBase(this.#getKey(), `${baseKey}\0collision\0${attempt}`); if (this.#placeholderBaseOwners.has(base)) continue; this.#placeholderBaseOwners.set(base, owner); return base; diff --git a/packages/coding-agent/test/secrets-obfuscator.test.ts b/packages/coding-agent/test/secrets-obfuscator.test.ts index 7e9b7e1da..01e11c2e5 100644 --- a/packages/coding-agent/test/secrets-obfuscator.test.ts +++ b/packages/coding-agent/test/secrets-obfuscator.test.ts @@ -3,14 +3,17 @@ */ import { describe, expect, it, spyOn } from "bun:test"; +import * as crypto from "node:crypto"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; import type { AgentMessage } from "@oh-my-pi/pi-agent-core"; import type { AssistantMessage, Context, Message, TextContent } from "@oh-my-pi/pi-ai"; import { + builtinCredentialSecretEntries, getExistingSecretPlaceholderKey, getSecretPlaceholderKey, + getSecretPlaceholderKeySync, loadSecrets, } from "@oh-my-pi/pi-coding-agent/secrets"; import { @@ -51,6 +54,69 @@ describe("compileSecretRegex", () => { }); }); +describe("builtinCredentialSecretEntries", () => { + // Issue #6968: an unconfigured credential-shaped token in a tool result used + // to fall through to pi-ai's irreversible `[*_token_redacted]` rewrite, so an + // edit-tool `old_text` echoing that placeholder could never match the file. + // The contract: the token is hidden from provider-visible text AND restored + // byte-exact in tool-call arguments before tool execution. + it("hides unconfigured credential-shaped tokens and restores them in tool-call arguments", () => { + const obfuscator = new SecretObfuscator(builtinCredentialSecretEntries()); + expect(obfuscator.hasSecrets()).toBe(true); + + const tokens = [`sk-${"a1B-c2D".repeat(7)}e3F`, `ghp_${"aB1".repeat(12)}`, `glpat-${"xY2-".repeat(5)}`]; + for (const token of tokens) { + const fileLine = `MOONSHOT_API_KEY=${token}`; + const providerView = obfuscator.obfuscate(fileLine); + expect(providerView).not.toContain(token); + // Re-obfuscation is a fixed point: the placeholder itself is never re-matched. + expect(obfuscator.obfuscate(providerView)).toBe(providerView); + // The model echoes the placeholder into edit-tool old_text verbatim. + const args = deobfuscateToolArguments(obfuscator, { old_text: providerView }); + expect(args.old_text).toBe(fileLine); + } + }); +}); + +describe("lazy placeholder key", () => { + // The built-in credential entries match dynamically, so they must not force + // `secret-placeholder.key` creation for every secrets.enabled session: the + // key provider is only invoked when a credential-shaped token is actually + // obfuscated, and exactly once across the obfuscator's lifetime. + it("resolves the key provider only on the first real credential match", () => { + let resolutions = 0; + const obfuscator = new SecretObfuscator(builtinCredentialSecretEntries(), () => { + resolutions++; + return crypto.randomBytes(32).toString("base64url"); + }); + expect(resolutions).toBe(0); + obfuscator.obfuscate("MOONSHOT_API_KEY=huntsville"); + expect(resolutions).toBe(0); + + const token = `sk-${"a1B-c2D".repeat(7)}e3F`; + const providerView = obfuscator.obfuscate(`MOONSHOT_API_KEY=${token}`); + expect(resolutions).toBe(1); + expect(providerView).not.toContain(token); + obfuscator.obfuscate(`repeat: ${token}`); + expect(resolutions).toBe(1); + const args = deobfuscateToolArguments(obfuscator, { old_text: providerView }); + expect(args.old_text).toBe(`MOONSHOT_API_KEY=${token}`); + }); + + it("getSecretPlaceholderKeySync creates the key file on demand and shares it with the async readers", async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-lazy-placeholder-key-")); + try { + expect(await getExistingSecretPlaceholderKey(dir)).toBeUndefined(); + const key = getSecretPlaceholderKeySync(dir); + expect(key).toMatch(/^[A-Za-z0-9_-]{43}$/); + expect(await getExistingSecretPlaceholderKey(dir)).toBe(key); + expect(await getSecretPlaceholderKey(dir)).toBe(key); + } finally { + await fs.rm(dir, { recursive: true, force: true }); + } + }); +}); + describe("SecretObfuscator regex behavior", () => { it("obfuscates and deobfuscates regex matches with flags", () => { const obfuscator = new SecretObfuscator([{ type: "regex", content: "api[_-]?key\\s*=\\s*\\w+", flags: "i" }]);