fix(utils): reject a project-.env config directory override - #3295
Conversation
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Verdict: APPROVE — MERGE_READY_WITH_ORDER (parent-first hold) — exact head 162127f3a4a43db93de146419cdf18b7759ebe73
This PR is not an independent clean-base change; it is a two-commit stack (#3285's exact head e88771a26cd4bbc3215239973a81fdeba6641244 + one new commit) and must be evaluated and merged as such.
Parent/head decomposition — verified directly against git, not the PR description:
git log --oneline pr3295
162127f3a fix(utils): reject a project-.env config directory override <- head, this PR's new commit
e88771a26 fix(utils): reject a project-.env agent directory override <- == #3285 exact head, byte-identical
18f6935b7 fix(coding-agent): repair dev CI 30244279756 after #3257 (#3275)
pr3295^1 == e88771a26cd4bbc3215239973a81fdeba6641244 exactly (confirmed via git rev-parse), and git merge-base pr3295 origin/dev == 18f6935b7, i.e. this stack branches from the same pre-#3281 base as #3285 with no drift introduced by the second commit. The incremental diff (e88771a26..162127f3a) touches only packages/utils/{CHANGELOG.md,src/dirs.ts,test/agent-dir-trust.test.ts} — no re-touch of anything #3285 already changed, confirming this really is "#3285 plus one commit" and not a rebase-and-diverge.
Checks performed on the incremental commit only (the parent portion was already reviewed and approved under #3285):
- Source-trust check for
GJC_CONFIG_DIR/PI_CONFIG_DIR:getConfigDirName()moved from rawprocess.env.GJC_CONFIG_DIR ?? process.env.PI_CONFIG_DIR ?? CONFIG_DIR_NAMEtotrustedConfigDirName("GJC_CONFIG_DIR") ?? trustedConfigDirName("PI_CONFIG_DIR") ?? CONFIG_DIR_NAME. Both names are now rejected when their value exactly matches what the project.envsets, mirroringtrustedAgentDirOverride's rule from the parent commit exactly — same conservative ambiguity trade-off (operator whose environment happens to carry the identical value loses the override), consistently applied. - Per-key fallback: verified
trustedConfigDirNameis called independently per name (GJC_CONFIG_DIRfirst, thenPI_CONFIG_DIR, then theCONFIG_DIR_NAMEdefault) — undefined-propagation semantics of the original??chain are preserved, and a legacyPI_CONFIG_DIRcorrectly still applies ifGJC_CONFIG_DIRspecifically gets rejected as project-planted (not both names collapsed together). - Interaction with #3267 shape containment: confirmed complementary, not overlapping, by reasoning through the two independent failure modes — #3267 constrains the shape of the config-dir name (no
..escaping home) but has no source check, so a value that is well-formed and even lives entirely under$HOMEwould still pass #3267 unchecked. This PR constrains the source (project.envcannot supply the value at all) regardless of shape, so it also closes the case #3267 alone cannot: a malicious directory the shape check would accept because it never needs..to begin with. Both are genuinely necessary; this is not a duplicate of #3267's concern. - cwd-under-HOME attack: the source-equality check in
trustedConfigDirNamehas no path-shape dependency — it rejects purely on project-.envprovenance, so it equally covers a malicious config directory placed inside$HOME(no..needed) as it does one placed outside. Confirmed this is not merely incidental: the check never inspects the resolved path's relationship to$HOME, only whether the raw value's source is the project.env. - Cycle avoidance via
env-fileleaf module:trustedConfigDirNamereuses theparseEnvFileimport already added todirs.tsby the parent commit (./env-file, not./env) — no new import needed, no cycle reintroduced. The PR body's claim that importing fromenv.tsdirectly would silently never fire (becausedirs.tsruns whileenv.tsis still initializing) is consistent with the same reasoning already validated in #3285's review. - Tests: extends the same
agent-dir-trust.test.tssuite (now 5 cases) rather than creating a parallel file — correct, since it is testing the same trust boundary. New cases: a config directory planted by the project.envis ignored (probed via a.envinside the config dir, matching the parent's probe pattern), and a config directory inherited from the launching shell (GJC_CONFIG_DIR=.customrelative to a controlledHOME) still resolves. Test hermeticity extended correctly:GJC_CONFIG_DIR/PI_CONFIG_DIRare now also deleted from the child env alongside the existingGJC_CODING_AGENT_DIRcleanup. - CI: all 23 checks pass on the exact head (
test:packages/utils/test/agent-dir-trust.test.tsexplicitly green; native-build, darwin-arm64 smoke, install-methods, and both ts-build jobs all completed pass after an initial pending window). mergeStateStatus: reportedUNSTABLEby the GitHub API at time of review — consistent with this being a stacked/dependent branch on an unmerged parent, not a defect in the diff itself.- No automated reviewer comments present.
Required merge order: #3285 MUST land first. This PR's base commit is #3285's exact head verbatim; merging #3295 before #3285 is not meaningful (there is no independent diff to land) and merging in the other order requires no rebase at all since the stack is already linear against #3285. If #3285 is rebased or its commit hash changes before merge, this stack needs a rebase to match.
No merge action taken by me on either PR — dev CI on the current base is nonterminal (per the standing instruction not to merge while post-#3250 dev CI runs), and this is a signed review verdict only.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
Same escalation as the agent-directory override in the parent commit, through
the other name. getConfigDirName() read GJC_CONFIG_DIR / PI_CONFIG_DIR straight
from process.env, and DirResolver joins that name with the home directory to
build configRoot (dirs.ts:185), with the agent directory beneath it. Both of
those supply .env files that $credentialEnv treats as trusted.
Measured on this source, with repo/.env setting GJC_CONFIG_DIR=../repo/.evil and
repo/.evil/.env setting a provider base URL:
before: configRoot = /tmp/_cfg2/repo/.evil
$credentialEnv("ANTHROPIC_BASE_URL") = "https://attacker.example"
after: configRoot = /tmp/_cfg2/home/.gjc
$credentialEnv("ANTHROPIC_BASE_URL") = undefined
An operator setting GJC_CONFIG_DIR from their shell still works, verified in the
same shape: configRoot lands at <home>/.custom and that directory's .env
resolves.
Applies the same conservative ambiguity rule as the agent-dir override, using
the leaf env-file module the parent commit extracted so there is no cycle.
162127f to
2a4c2e8
Compare
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Exact-head refresh review at 2a4c2e8bbb021961aa917361895b862d489cba84 (rebased past merged #3285, base 69faa20d confirmed ancestor of dev fa2ca8a).
Diff scope verified against prior approved head 162127f: single fix commit 2a4c2e8b extends the existing GJC_CODING_AGENT_DIR project-.env trust-boundary fix to also cover GJC_CONFIG_DIR/PI_CONFIG_DIR via a new trustedConfigDirName() helper in packages/utils/src/dirs.ts, plus matching CHANGELOG and 2 new regression tests in packages/utils/test/agent-dir-trust.test.ts. No unrelated changes.
Trust-boundary re-verification: ran bun test packages/utils/test/agent-dir-trust.test.ts locally against this exact head — 5/5 pass (8 expect calls), including the two new cases (rejects project-.env-planted GJC_CONFIG_DIR; honors shell-inherited GJC_CONFIG_DIR). No regression on the existing agent-dir cases.
CI: all applicable checks terminal SUCCESS (Affected path validation, gjc-state-gates ×4, Local public surfaces, Telegram daemon generation guard); Windows/Python/darwin-arm64 legs SKIPPED as path-gated, not failing.
Verdict: APPROVE.
Reviewed by gaebal-gajae (automated red-team review agent).
|
Follow-up at exact head — |
resolveConfigPaths (native-skill-hook.ts:156) built its config locations from raw process.env: GJC_CONFIG_DIR ?? PI_CONFIG_DIR ?? ".gjc" GJC_CODING_AGENT_DIR ?? join(os.homedir(), configDirName, "agent") Bun loads cwd/.env into process.env before any module runs, so both values are repository-controlled. The config.yml these paths select carries skills.customDirectories — the directories the agent then loads skills from — so this is the same escalation trustedAgentDirOverride (Yeachan-Heo#3285) and trustedConfigDirName (Yeachan-Heo#3295) were added to close, reached through a call site that never went through them. Measured on a scenario repo whose .env sets GJC_CODING_AGENT_DIR to a planted directory: getAgentDir() correctly resolved to /home/woody/.gjc/agent, while the hook loaded the planted config.yml and returned its customDirectories. After the change the injected entry is gone and a config in the trusted agent dir is still honored. getAgentDir()/getConfigDirName() resolve to the same locations this built by hand — dirs.agentDir is join(os.homedir(), getConfigDirName(), "agent") absent a trusted override — so only the untrusted case changes. Regression drives a child process, since the planted value is only visible to a process started with that cwd. Restoring the raw reads fails the two bypass cases while the trusted-config case keeps passing.
resolveConfigPaths (native-skill-hook.ts:156) built its config locations from raw process.env: GJC_CONFIG_DIR ?? PI_CONFIG_DIR ?? ".gjc" GJC_CODING_AGENT_DIR ?? join(os.homedir(), configDirName, "agent") Bun loads cwd/.env into process.env before any module runs, so both values are repository-controlled. The config.yml these paths select carries skills.customDirectories — the directories the agent then loads skills from — so this is the same escalation trustedAgentDirOverride (Yeachan-Heo#3285) and trustedConfigDirName (Yeachan-Heo#3295) were added to close, reached through a call site that never went through them. Measured on a scenario repo whose .env sets GJC_CODING_AGENT_DIR to a planted directory: getAgentDir() correctly resolved to /home/woody/.gjc/agent, while the hook loaded the planted config.yml and returned its customDirectories. After the change the injected entry is gone and a config in the trusted agent dir is still honored. getAgentDir()/getConfigDirName() resolve to the same locations this built by hand — dirs.agentDir is join(os.homedir(), getConfigDirName(), "agent") absent a trusted override — so only the untrusted case changes. Regression drives a child process, since the planted value is only visible to a process started with that cwd. Restoring the raw reads fails the two bypass cases while the trusted-config case keeps passing.
…3324) resolveConfigPaths (native-skill-hook.ts:156) built its config locations from raw process.env: GJC_CONFIG_DIR ?? PI_CONFIG_DIR ?? ".gjc" GJC_CODING_AGENT_DIR ?? join(os.homedir(), configDirName, "agent") Bun loads cwd/.env into process.env before any module runs, so both values are repository-controlled. The config.yml these paths select carries skills.customDirectories — the directories the agent then loads skills from — so this is the same escalation trustedAgentDirOverride (#3285) and trustedConfigDirName (#3295) were added to close, reached through a call site that never went through them. Measured on a scenario repo whose .env sets GJC_CODING_AGENT_DIR to a planted directory: getAgentDir() correctly resolved to /home/woody/.gjc/agent, while the hook loaded the planted config.yml and returned its customDirectories. After the change the injected entry is gone and a config in the trusted agent dir is still honored. getAgentDir()/getConfigDirName() resolve to the same locations this built by hand — dirs.agentDir is join(os.homedir(), getConfigDirName(), "agent") absent a trusted override — so only the untrusted case changes. Regression drives a child process, since the planted value is only visible to a process started with that cwd. Restoring the raw reads fails the two bypass cases while the trusted-config case keeps passing.
Stacked on #3285 — it contains the parent commit and the leaf
env-filemodule this needs. Review/merge #3285 first; this branch is #3285 plus one commit.The other half of the same escalation
#3285 closed
GJC_CODING_AGENT_DIR. The same escalation is reachable through the config-directory name, which I deferred there to avoid conflicting with the already-approved #3267.getConfigDirName()readGJC_CONFIG_DIR/PI_CONFIG_DIRstraight fromprocess.env, andDirResolverjoins that name with the home directory to buildconfigRoot(dirs.ts:185), with the agent directory beneath it (:187). Both of those supply.envfiles that$credentialEnvtreats as trusted — so this one name was also enough to make a repository's own.envtrusted, recovering every endpoint and credential redirect the boundary rejects.Measured on this source —
repo/.envsetsGJC_CONFIG_DIR=../repo/.evil,repo/.evil/.envsets a provider base URL:Operator path verified in the same shape:
GJC_CONFIG_DIR=.customfrom the shell still landsconfigRootat<home>/.customand that directory's.envresolves.Fix
The same conservative ambiguity rule the parent commit applies to
GJC_CODING_AGENT_DIR, evaluated per key so a legacyPI_CONFIG_DIRstill applies when the primary one is rejected. It uses the leafenv-filemodule from #3285 — importingparseEnvFilefromenv.tsdirectly would be a cycle, and I confirmed that empirically: the check silently never fires becausedirs.tsruns whileenv.tsis still initialising.Relationship to #3267
Complementary, not overlapping. #3267 constrains the shape of the name (no
..escaping home); this constrains its source. Both are needed — shape alone still allows a repository that lives under$HOMEto point the config root at itself without any.., and source alone still allows a trusted-but-malformed value. They touch the same function, so whichever lands second will need a trivial rebase; happy to do that.Tests
Extends
agent-dir-trust.test.ts(now 5 cases): a planted config directory is ignored, and a config directory inherited from the launching shell still supplies its.env.Proof-first: reverting the config-dir check fails exactly the planted case (4 pass / 1 fail).
Verification
bun run checkinpackages/utils: exit 0.packages/utils/test: 218 pass / 0 fail.coding-agentdiscovery suites: 130 pass / 0 fail.