smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(coding-agent): resolve the secret placeholder key lazily for builtin credential entries

Browse Source 
Appending builtinCredentialSecretEntries() unconditionally made every
secrets.enabled session carry a regex obfuscate entry, so
secretEntriesNeedPlaceholderKey was always true and startup always
created secret-placeholder.key — nullifying the replace-only/no-secret
key-avoidance path and failing headless runs on an unwritable config
root for a feature they never use.

Only configured entries now force startup key creation. The built-in
credential pattern matches dynamically, so SecretObfuscator accepts a
key provider resolved once on the first actual credential match, via
the new getSecretPlaceholderKeySync (never throws: degrades to a
process-ephemeral key with a warning when the key file is unwritable).

Also moves both CHANGELOG entries from the released 17.1.7 sections to
[Unreleased].
This commit is contained in:
iacore
2026-07-31 13:38:09 +08:00
parent 54008bce99
commit fcf6d65140
6 changed files with 169 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/src/secrets/index.ts
+57 -3
View File
@@ -1,5 +1,5 @@
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";
@@ -32,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) {
@@ -72,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<string | undefined> {
const attempts = retry ? 50 : 1;
packages/coding-agent/src/secrets/obfuscator.ts
+49 -15
View File
@@ -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;