fix(bash): tightened snapshot perms and denied secret-shaped env vars
PR #3474 review: the new export pass writes referenced env-var values into a snapshot file under `os.tmpdir()/omp-shell-snapshots`. On Linux where `os.tmpdir()` is `/tmp` and the umask is the default 022, the file ended up world-readable (0644) until postmortem cleanup. A user rcfile defining `deploy(){ curl -H "Authorization: $GITHUB_TOKEN" ...; }` would have its token written verbatim to that file. Three-layer mitigation: - `umask 077` at the top of the snapshot script so the file is 0600 from the first byte (the shell creates it via redirection, not JS). - JS caller now passes `mode: 0o700` to `mkdirSync` and chmods the dir + file defensively after the script exits, covering pre-existing dirs and exotic shells where the umask call might not take. - Helper denylist gained the common secret-shaped name patterns (`*TOKEN*`, `*SECRET*`, `*API_KEY*`, `*PASSWORD*`, `*PASSWD*`, `*PRIVATE_KEY*`, `*ACCESS_KEY*`, `*CREDENTIAL*`, `*SESSION_KEY*`) so even when the file is locked down, we don't materialise tokens onto disk in the first place. Tests cover both: a new helper-level test asserts none of the secret names (or their values) appear in the export stream, and the e2e test now stats the snapshot file + dir and asserts `mode & 0o077 === 0`.
This commit is contained in:
roboomp
@@ -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
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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<Uint8Array> | 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);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user