diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 4d92d21e0..56860d863 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -4,7 +4,7 @@ ### Fixed -- Fixed the bash tool's snapshotted `mise()` shell function dying with `command: command not found:` because `$__MISE_EXE` was empty in the replay shell. `generateSnapshotScript` captured the function via `declare -f`/`typeset -f` but only ever re-exported `PATH`, so every other env var the rc file set (notably the `*_EXE` sidecar `mise activate` exports) was lost; the function body then expanded `command "$__MISE_EXE" "$@"` to `command "" …` and died with exit 127. The snapshot script now scans captured function bodies for `$VAR` / `${VAR…}` references and re-emits `export NAME='value'` for each referenced var that is currently set (with a denylist for shell-internal names like `PATH`/`HOME`/`BASH_*`/`LC_*`), fixing mise, asdf shims, direnv-style helpers, and other activation idioms that pair a function with a helper env var. `getShellConfigFile` now also honours `env.HOME` (falling back to `os.homedir()`) so sandboxed callers can target a non-default rc. ([#3470](https://github.com/can1357/oh-my-pi/issues/3470)) +- Fixed the bash tool's snapshotted `mise()` shell function dying with `command: command not found:` because `$__MISE_EXE` was empty in the replay shell. `generateSnapshotScript` captured the function via `declare -f`/`typeset -f` but only ever re-exported `PATH`, so every other env var the rc file set (notably the `*_EXE` sidecar `mise activate` exports) was lost; the function body then expanded `command "$__MISE_EXE" "$@"` to `command "" …` and died with exit 127. The snapshot script now scans captured function bodies for `$VAR` / `${VAR…}` references and re-emits `export NAME='value'` for each referenced var that is currently set (with a denylist for shell-internal names like `PATH`/`HOME`/`BASH_*`/`LC_*` plus a likely-secret denylist for `*TOKEN*`/`*SECRET*`/`*API_KEY*`/`*PASSWORD*`/`*PRIVATE_KEY*`/`*ACCESS_KEY*`/`*CREDENTIAL*`/`*SESSION_KEY*`), the snapshot script `umask 077`s itself and the JS caller chmods the snapshot file/dir to `0600`/`0700` so the new export pass can't leak secrets into a shared tmp dir. Fixes mise, asdf shims, direnv-style helpers, and other activation idioms that pair a function with a helper env var. `getShellConfigFile` now also honours `env.HOME` (falling back to `os.homedir()`) so sandboxed callers can target a non-default rc. ([#3470](https://github.com/can1357/oh-my-pi/issues/3470)) ## [16.1.19] - 2026-06-25 diff --git a/packages/coding-agent/src/utils/shell-snapshot-fn-env.sh b/packages/coding-agent/src/utils/shell-snapshot-fn-env.sh index 1ed243b6b..493b3da75 100644 --- a/packages/coding-agent/src/utils/shell-snapshot-fn-env.sh +++ b/packages/coding-agent/src/utils/shell-snapshot-fn-env.sh @@ -30,11 +30,19 @@ __omp_sq_quote() { } # Emit `export NAME='value'` for $1 unless the name is a shell-internal we -# must never overwrite or the var is unset. +# must never overwrite, a likely secret (token / key / password / credential +# patterns — kept conservative, since `__MISE_EXE` and `FOO_DIR` style helper +# vars never carry secrets), or the var is unset. POSIX `case` patterns are +# byte-exact so we list common uppercase variants; lowercase secret vars are +# rare and out of scope. __omp_emit_export_for() { case "$1" in _|PATH|HOME|USER|LOGNAME|PWD|OLDPWD|SHELL|SHLVL|TERM|TERMINFO|TERMCAP|IFS|TMPDIR|TMOUT|LANG|RANDOM|LINENO|SECONDS|FUNCNAME|HISTFILE|HISTSIZE|HISTFILESIZE|HISTCMD|PS1|PS2|PS3|PS4|UID|EUID|GROUPS|HOSTNAME|HOSTTYPE|OSTYPE|MACHTYPE|PIPESTATUS|BASH|ZSH|argv|PROMPT|RPROMPT|RPS1|RPS2|status|pipestatus|COLUMNS|LINES|COLORTERM|FUNCNEST) return ;; LC_*|BASH_*|ZSH_*) return ;; + # Common secret-name patterns — never materialise these into the + # snapshot file even though it's now created 0600 (defence in depth + # against the file ending up in a backup, tarball, or NFS share). + *TOKEN*|*SECRET*|*PASSWORD*|*PASSWD*|*API_KEY*|*PRIVATE_KEY*|*ACCESS_KEY*|*CREDENTIAL*|*SESSION_KEY*) return ;; esac eval "[ \"\${$1+x}\" = x ]" 2>/dev/null || return eval "__omp_xv=\"\${$1}\"" 2>/dev/null || return diff --git a/packages/coding-agent/src/utils/shell-snapshot.ts b/packages/coding-agent/src/utils/shell-snapshot.ts index f8e56c735..c3975cd7f 100644 --- a/packages/coding-agent/src/utils/shell-snapshot.ts +++ b/packages/coding-agent/src/utils/shell-snapshot.ts @@ -143,6 +143,13 @@ echo "shopt -s expand_aliases" >> "$SNAPSHOT_FILE" return ` SNAPSHOT_FILE='${escapedPath}' +# Snapshot may inline env-var values referenced by captured functions (#3470). +# Force 0600/0700 perms so a multi-user box can't read tokens out of the tmp +# file. The JS caller also chmods the file/dir defensively after the script +# exits, but the umask catches the file at first write so secrets never live +# at 0644 even briefly. +umask 077 + # Source user's rc file if it exists ${hasRcFile ? `source "${rcFile}" < /dev/null 2>/dev/null` : "# No user config file to source"} @@ -218,9 +225,18 @@ export async function getOrCreateSnapshot( const rcFile = getShellConfigFile(shell, env); - // Create snapshot directory + // Create snapshot directory with owner-only perms — the script may inline + // env vars referenced by captured functions (#3470) and `os.tmpdir()` is + // shared on Linux. `mode: 0o700` applies to a fresh mkdir; an existing dir + // keeps its mode, so chmod it defensively. Ignore EPERM (dir owned by + // another user on a shared box). const snapshotDir = path.join(os.tmpdir(), "omp-shell-snapshots"); - fs.mkdirSync(snapshotDir, { recursive: true }); + fs.mkdirSync(snapshotDir, { recursive: true, mode: 0o700 }); + try { + fs.chmodSync(snapshotDir, 0o700); + } catch { + // best-effort + } // Generate unique snapshot path const shellName = shell.includes("zsh") ? "zsh" : shell.includes("bash") ? "bash" : "sh"; @@ -248,6 +264,14 @@ export async function getOrCreateSnapshot( await child.exited; if (child.exitCode === 0 && fs.existsSync(snapshotPath)) { + // Defence-in-depth: the script's `umask 077` already locks the file at + // first write, but chmod again in case the umask didn't take (exotic + // shells) or a postmortem-restored file ended up looser. + try { + fs.chmodSync(snapshotPath, 0o600); + } catch { + // best-effort + } scrubSnapshotInPlace(snapshotPath); cachedSnapshotPaths.set(cacheKey, snapshotPath); return snapshotPath; diff --git a/packages/coding-agent/test/shell-snapshot.test.ts b/packages/coding-agent/test/shell-snapshot.test.ts index 87349ee13..b6961020d 100644 --- a/packages/coding-agent/test/shell-snapshot.test.ts +++ b/packages/coding-agent/test/shell-snapshot.test.ts @@ -132,6 +132,65 @@ describe("shell-snapshot fn-env helper", () => { expect(out).not.toContain("NEVER_SET_TEST_VAR"); }); + it("never emits export lines for likely-secret env var names", async () => { + const funcs = [ + `deploy () { curl -H "Authorization: $GITHUB_TOKEN" .; }`, + `call_openai () { curl -H "Authorization: Bearer $OPENAI_API_KEY" .; }`, + `aws_sign () { echo "$AWS_SECRET_ACCESS_KEY"; }`, + `db () { mysql --password="$DB_PASSWORD" -u root; }`, + `legacy () { echo "$LDAP_PASSWD"; }`, + `vault () { echo "$VAULT_PRIVATE_KEY"; }`, + `session () { echo "$REDIS_SESSION_KEY"; }`, + `creds () { echo "$AZURE_CREDENTIAL"; }`, + // Control: non-secret-shaped var must still be emitted. + `mise () { command "$__MISE_EXE" "$@"; }`, + ``, + ].join("\n"); + + const child = Bun.spawn(["bash", "-c", `${fnEnvHelper}\n__omp_emit_referenced_exports`], { + env: { + PATH: process.env.PATH ?? "/usr/bin:/bin", + GITHUB_TOKEN: "ghp_REDACTED", + OPENAI_API_KEY: "sk-REDACTED", + AWS_SECRET_ACCESS_KEY: "REDACTED", + DB_PASSWORD: "hunter2", + LDAP_PASSWD: "hunter2", + VAULT_PRIVATE_KEY: "-----BEGIN-----", + REDIS_SESSION_KEY: "abc", + AZURE_CREDENTIAL: "xyz", + __MISE_EXE: "/opt/echo", + }, + stdin: "pipe", + stdout: "pipe", + stderr: "ignore", + }); + child.stdin.write(funcs); + await child.stdin.end(); + const out = await readStream(child.stdout as ReadableStream | null); + await child.exited; + expect(child.exitCode).toBe(0); + + for (const secret of [ + "GITHUB_TOKEN", + "OPENAI_API_KEY", + "AWS_SECRET_ACCESS_KEY", + "DB_PASSWORD", + "LDAP_PASSWD", + "VAULT_PRIVATE_KEY", + "REDIS_SESSION_KEY", + "AZURE_CREDENTIAL", + ]) { + expect(out).not.toContain(secret); + } + // And the secret VALUES — make sure nothing leaked through a different + // quoting path. + for (const value of ["ghp_REDACTED", "sk-REDACTED", "hunter2", "-----BEGIN-----"]) { + expect(out).not.toContain(value); + } + // The non-secret helper var still goes through. + expect(out).toContain("export __MISE_EXE='/opt/echo'"); + }); + it("single-quote-escapes values containing apostrophes and preserves newlines", async () => { const funcs = `shout () { echo "$TRICKY_VAL $NL_VAL"; }\n`; const child = Bun.spawn(["bash", "-c", `${fnEnvHelper}\n__omp_emit_referenced_exports`], { @@ -206,5 +265,13 @@ describe("getOrCreateSnapshot", () => { await replay.exited; expect({ exitCode: replay.exitCode, stderr }).toEqual({ exitCode: 0, stderr: "" }); expect(stdout).toBe("hello world\n/opt/foo\n"); + + // PR-review hardening: snapshot file must be group/world-unreadable since + // it now inlines env-var values. Directory must be 0700 for the same + // reason — UUID filenames shouldn't leak via `ls /tmp/omp-shell-snapshots`. + const fileStat = await fs.stat(snapshotPath!); + expect(fileStat.mode & 0o077).toBe(0); + const dirStat = await fs.stat(path.dirname(snapshotPath!)); + expect(dirStat.mode & 0o077).toBe(0); }); });