From 41fe3e1c3149d22f2f928f9f1f2103e8f1fffc0c Mon Sep 17 00:00:00 2001 From: can1357 Date: Thu, 23 Jul 2026 17:12:22 +0200 Subject: [PATCH] fix(direnv): honored direnv's allow list instead of auto-allowing .envrc Removed the direnv-allow preflight so an .envrc the user never allowed is skipped silently (debug log) and never executed; only already-allowed files export. Updated the setting description, changelog, and rewrote the tests to allow explicitly per content change. --- packages/coding-agent/CHANGELOG.md | 1 + .../src/config/settings-schema.ts | 2 +- packages/coding-agent/src/exec/direnv.ts | 34 +++++++++------- packages/coding-agent/test/direnv.test.ts | 40 ++++++++++++++++++- 4 files changed, 60 insertions(+), 17 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index dbf2574e5..75f5b362e 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -1138,6 +1138,7 @@ ### Fixed - Extended the bash tool's direnv/devenv auto-loading to every backend: the ACP client terminal and the interactive PTY now receive the repo's direnv environment (variables set, and `unset -v` for variables the `.envrc` removes) — previously only the one-shot `executeBash` path did — via a shared preflight so all backends behave identically, with the caller's explicit env still winning. direnv loading also always re-runs `direnv export json` instead of serving a content-hashed cache, so a change to a `watch_file` target re-exports even when the `.envrc` text is unchanged (direnv's own watch invalidation is authoritative). ([#4455](https://github.com/can1357/oh-my-pi/issues/4455)) +- The bash tool's direnv auto-load now honors direnv's own allow list: an `.envrc` the user has not `direnv allow`ed is skipped silently and never executed or auto-allowed, keeping OMP's trust boundary identical to the user's shell. ([#4455](https://github.com/can1357/oh-my-pi/issues/4455)) ## [16.3.4] - 2026-07-03 diff --git a/packages/coding-agent/src/config/settings-schema.ts b/packages/coding-agent/src/config/settings-schema.ts index 2af91ddd7..4c643fb25 100644 --- a/packages/coding-agent/src/config/settings-schema.ts +++ b/packages/coding-agent/src/config/settings-schema.ts @@ -3356,7 +3356,7 @@ export const SETTINGS_SCHEMA = { group: "Bash", label: "direnv Auto-Load", description: - "Auto-load (and auto-allow) a repo's direnv/devenv `.envrc` into the bash session so devenv tools and env vars are present without manual `direnv exec`", + "Auto-load a repo's direnv/devenv `.envrc` into the bash session so devenv tools and env vars are present without manual `direnv exec`. Honors direnv's allow list: an `.envrc` you haven't `direnv allow`ed is never executed", }, }, "bash.direnvLoadTimeoutMs": { diff --git a/packages/coding-agent/src/exec/direnv.ts b/packages/coding-agent/src/exec/direnv.ts index bfbb8b662..d7442679b 100644 --- a/packages/coding-agent/src/exec/direnv.ts +++ b/packages/coding-agent/src/exec/direnv.ts @@ -73,7 +73,7 @@ async function runDirenv( timeoutMs: number, env: Record, signal?: AbortSignal, -): Promise<{ exitCode: number; stdout: string }> { +): Promise<{ exitCode: number; stdout: string; stderr: string }> { // Bail on the caller's cancellation as well as the per-invocation cap so a // cold `.envrc` load can't outlive an aborted / short-timeout bash call. const abortSignal = signal @@ -89,28 +89,29 @@ async function runDirenv( // Drain stdout AND stderr concurrently: a cold `use devenv`/Nix load emits // enough diagnostics to fill the stderr pipe and block the child forever if // only stdout is read (it would then wait out `timeoutMs`). - const [stdout] = await Promise.all([ + const [stdout, stderr] = await Promise.all([ new Response(proc.stdout as ReadableStream).text(), new Response(proc.stderr as ReadableStream).text(), ]); const exitCode = await proc.exited; - return { exitCode, stdout }; + return { exitCode, stdout, stderr }; } /** - * Resolve the nearest `.envrc` from `cwd`, auto-allow it, and return its - * `direnv export` diff (variables to set, and variables direnv removes). - * Returns `null` when there is no `.envrc`, `direnv` is not installed, or the - * export fails/times out. + * Resolve the nearest `.envrc` from `cwd` and return its `direnv export` diff + * (variables to set, and variables direnv removes). Returns `null` when there + * is no `.envrc`, `direnv` is not installed, the `.envrc` is not on direnv's + * allow list, or the export fails/times out. + * + * direnv's own allow list is honored — an `.envrc` the user has not + * `direnv allow`ed is NEVER executed or auto-allowed. This keeps OMP's trust + * boundary identical to the user's own shell: cloning a repo with a poisoned + * `.envrc` grants it nothing until the user explicitly allows it. * * Always re-invokes `direnv export json` rather than serving a cached diff: * direnv is fast once warm, and its own watch/mtime invalidation is the * authoritative freshness signal (a content-hash cache here would go stale when * a `watch_file` target changes without the `.envrc` text changing). - * - * Auto-allow is deliberate: OMP already runs the repository's own code, so its - * `.envrc` is trusted under the same model rather than forcing a manual - * `direnv allow`. */ export async function loadDirenvEnv( cwd: string, @@ -125,10 +126,15 @@ export async function loadDirenvEnv( const timeoutMs = opts?.timeoutMs ?? DEFAULT_DIRENV_TIMEOUT_MS; const env = cleanSpawnEnv(); try { - await runDirenv(bin, ["allow"], dir, timeoutMs, env, opts?.signal); - const { exitCode, stdout } = await runDirenv(bin, ["export", "json"], dir, timeoutMs, env, opts?.signal); + const { exitCode, stdout, stderr } = await runDirenv(bin, ["export", "json"], dir, timeoutMs, env, opts?.signal); if (exitCode !== 0) { - logger.warn("direnv export failed", { dir, exitCode }); + // A not-yet-allowed .envrc is an expected steady state (the user opted + // out by never running `direnv allow`), not an error worth warning on. + if (stderr.includes("is blocked")) { + logger.debug("direnv .envrc not allowed; skipping", { dir }); + } else { + logger.warn("direnv export failed", { dir, exitCode }); + } return null; } return parseDirenvExport(stdout); diff --git a/packages/coding-agent/test/direnv.test.ts b/packages/coding-agent/test/direnv.test.ts index 96cafcf70..1dd54d52f 100644 --- a/packages/coding-agent/test/direnv.test.ts +++ b/packages/coding-agent/test/direnv.test.ts @@ -20,6 +20,20 @@ afterEach(async () => { for (const dir of tmpDirs.splice(0)) await dir.remove(); }); +/** Explicitly `direnv allow` an `.envrc` (against the test-isolated HOME/XDG + * dirs). Loading now honors the allow list, so tests that expect an export + * must opt the file in the same way a user would. Re-run after every content + * change — direnv's allow entry is content-hashed. */ +async function allowEnvrc(dir: string): Promise { + const env: Record = {}; + for (const key in Bun.env) { + const value = Bun.env[key]; + if (value !== undefined && !key.startsWith("DIRENV_")) env[key] = value; + } + const proc = Bun.spawn(["direnv", "allow"], { cwd: dir, env, stdout: "ignore", stderr: "ignore" }); + if ((await proc.exited) !== 0) throw new Error(`direnv allow failed in ${dir}`); +} + describe("findEnvrc", () => { it("walks up to the nearest .envrc above the start dir", async () => { const root = tmp(); @@ -62,7 +76,7 @@ describe("parseDirenvExport", () => { }); }); -describe.skipIf(!hasDirenv)("loadDirenvEnv (real direnv, auto-allow)", () => { +describe.skipIf(!hasDirenv)("loadDirenvEnv (real direnv, allow-list honored)", () => { // `direnv allow` writes a trust entry into its data dir. Redirect HOME + the // XDG dirs to a throwaway tmp so real-direnv cases never leak allow state // into the dev/CI user's global direnv store. @@ -81,10 +95,21 @@ describe.skipIf(!hasDirenv)("loadDirenvEnv (real direnv, auto-allow)", () => { } }); - it("auto-allows an untrusted .envrc and returns its exported vars + PATH additions", async () => { + it("returns null for an .envrc the user has not allowed (never auto-allows)", async () => { + // The security contract: an untrusted `.envrc` is NEVER executed. A + // poisoned repo gets nothing until the user runs `direnv allow` — exactly + // the trust boundary direnv itself enforces in the user's shell. + const root = tmp(); + await Bun.write(path.join(root, ".envrc"), "export DIRENV_BLOCKED_TEST=leaked\n"); + + expect(await loadDirenvEnv(root)).toBeNull(); + }); + + it("returns exported vars + PATH additions for an allowed .envrc", async () => { const root = tmp(); await fs.mkdir(path.join(root, "bin"), { recursive: true }); await Bun.write(path.join(root, ".envrc"), "export DIRENV_FEATURE_TEST=loaded\nPATH_add bin\n"); + await allowEnvrc(root); const diff = await loadDirenvEnv(root); @@ -95,6 +120,7 @@ describe.skipIf(!hasDirenv)("loadDirenvEnv (real direnv, auto-allow)", () => { it("reports variables a .envrc unsets", async () => { const root = tmp(); await Bun.write(path.join(root, ".envrc"), "unset PI_DIRENV_UNSET_TEST\n"); + await allowEnvrc(root); // direnv emits a JSON null for a var only when it was present in the // spawn env and the `.envrc` removes it, so seed it in the parent env. // (Avoid a `DIRENV_`-prefixed name — the loader strips those before spawn.) @@ -113,11 +139,15 @@ describe.skipIf(!hasDirenv)("loadDirenvEnv (real direnv, auto-allow)", () => { }); it("re-exports when the .envrc content changes (no stale cache)", async () => { + // direnv's allow entry is content-hashed, so each rewrite is re-allowed — + // mirroring what a user does after editing their own .envrc. const root = tmp(); await Bun.write(path.join(root, ".envrc"), "export DIRENV_CACHE_TEST=one\n"); + await allowEnvrc(root); expect((await loadDirenvEnv(root))?.set.DIRENV_CACHE_TEST).toBe("one"); await Bun.write(path.join(root, ".envrc"), "export DIRENV_CACHE_TEST=two\n"); + await allowEnvrc(root); expect((await loadDirenvEnv(root))?.set.DIRENV_CACHE_TEST).toBe("two"); }); @@ -133,6 +163,7 @@ describe.skipIf(!hasDirenv)("loadDirenvEnv (real direnv, auto-allow)", () => { path.join(root, ".envrc"), 'watch_file watched.env\nexport DIRENV_WATCH_TEST="$(cat watched.env)"\n', ); + await allowEnvrc(root); expect((await loadDirenvEnv(root))?.set.DIRENV_WATCH_TEST).toBe("one"); await Bun.write(path.join(root, "watched.env"), "two\n"); @@ -159,6 +190,7 @@ describe.skipIf(!hasDirenv)("bash executor direnv wiring (end-to-end)", () => { it("exposes direnv-loaded vars to the command while per-call env still wins", async () => { const root = tmp(); await Bun.write(path.join(root, ".envrc"), "export DIRENV_WIRE_TEST=fromdirenv\nexport OVERRIDE_ME=fromdirenv\n"); + await allowEnvrc(root); const result = await executeBash('printf "%s|%s" "$DIRENV_WIRE_TEST" "$OVERRIDE_ME"', { cwd: root, @@ -171,6 +203,7 @@ describe.skipIf(!hasDirenv)("bash executor direnv wiring (end-to-end)", () => { it("removes variables the .envrc unsets from the command environment", async () => { const root = tmp(); await Bun.write(path.join(root, ".envrc"), "unset PI_DIRENV_UNSET_E2E\n"); + await allowEnvrc(root); // Inherited from the process env (as an OMP-provided var would be); the // caller does NOT re-supply it, so direnv's unset must strip it. `printenv` // exits non-zero and prints nothing when the name is genuinely absent. A @@ -214,6 +247,7 @@ describe.skipIf(!hasDirenv)("applyDirenvPreflight (shared all-backends preflight it("merges direnv set vars into env while the caller's env wins", async () => { const root = tmp(); await Bun.write(path.join(root, ".envrc"), "export DIRENV_PF_SET=fromdirenv\nexport OVERRIDE_ME=fromdirenv\n"); + await allowEnvrc(root); const { command, env } = await applyDirenvPreflight("echo hi", root, { callerEnv: { OVERRIDE_ME: "fromcaller" }, @@ -229,6 +263,7 @@ describe.skipIf(!hasDirenv)("applyDirenvPreflight (shared all-backends preflight it("prepends a regex-gated `unset -v` for vars the .envrc removes, skipping caller-resupplied names", async () => { const root = tmp(); await Bun.write(path.join(root, ".envrc"), "unset PI_PF_UNSET_A\nunset PI_PF_UNSET_B\n"); + await allowEnvrc(root); Bun.env.PI_PF_UNSET_A = "present"; Bun.env.PI_PF_UNSET_B = "present"; try { @@ -249,6 +284,7 @@ describe.skipIf(!hasDirenv)("applyDirenvPreflight (shared all-backends preflight it("applies the shell commandPrefix after the unset prefix", async () => { const root = tmp(); await Bun.write(path.join(root, ".envrc"), "unset PI_PF_ORDER\n"); + await allowEnvrc(root); Bun.env.PI_PF_ORDER = "present"; try { const { command } = await applyDirenvPreflight("payload", root, {