fix(utils): keep the configured config-dir name beneath home - #3267
Conversation
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Hostile exact-head review — PR #3267
Exact head: 284fc6a3b65b8e9e37bb0f266b908ce80a942dbb (matches headRefOid; local git rev-parse identical)
Base / live origin/dev: 2d8770169c15cd13efd54aba43ea2cddaab56a93 (matches baseRefOid; no base drift)
Diff: 3 files, +112/-1 (packages/utils/src/dirs.ts, new containment test, CHANGELOG)
Mergeability: MERGEABLE / CLEAN vs current origin/dev (merge-tree clean)
Exact-head CI: all completed non-skipped checks success on 284fc6a3b65b (incl. test:packages/utils/test/config-dir-name-containment.test.ts, check:@gajae-code/utils, native-build, darwin-arm64 tab-worker smoke, gjc-state-gates, install-methods, Local public surfaces). Evidence: https://github.com/Yeachan-Heo/gajae-code/actions/runs/30244506875
Contributor / scope: external fork 10kH:fix/config-dir-name-containment; single-commit utils-only surface; no workflow mutation; changelog under Unreleased Fixed. No predecessor merge-gate hard-dep (trust series #3252/#3256 merged; #3260/#3262/#3268 parallel, different env keys).
Verdict
MERGE_READY
Attack surface reviewed
getConfigDirName() previously returned GJC_CONFIG_DIR / PI_CONFIG_DIR verbatim. Documented contract (skill-discovery prompt): home-relative name, absolute-looking values still joined beneath <home>. path.join strips a leading separator but honors .., so discovery of user-level mcp.json (spawned MCP servers), SYSTEM.md, skills/agents/plugins could leave the home config root:
| input | old path.resolve(home, name, "agent/mcp.json") |
escapes home |
|---|---|---|
../escape |
/home/escape/agent/mcp.json |
yes |
../../tmp/evil |
/tmp/evil/agent/mcp.json |
yes |
.gjc/../../tmp/evil |
/home/tmp/evil/agent/mcp.json |
yes |
Diff assessment
| Claim | Result |
|---|---|
Escaping .. names rejected → default .gjc |
PASS — path.normalize(...).split(/[\\/]/).includes("..") |
| Per-key evaluation (GJC then PI then default) | PASS — fallthrough test + independent probe |
| Ordinary / legacy names unchanged | PASS |
| Absolute-looking name stays under home after join | PASS — Node/Bun path.join(home, "/etc/gjc") lands home/etc/gjc |
| Blank name ignored | PASS |
| DirResolver / plugins path use sanitized API | PASS — configRoot = path.join(home, getConfigDirName()) |
| Changelog accurate, scoped Fixed | PASS |
No over-claim of project-.env trust boundary |
PASS — body explicitly scopes containment only |
Independent proofs (this head)
- Focused tests:
bun test packages/utils/test/config-dir-name-containment.test.ts→ 11 pass / 0 fail - Live API probe: escaping suite all fall back to
.gjcand resolve under$HOME; fallthroughGJC=../../evil+PI=.pi-ok→.pi-ok; ordinary.gjc-althonored - Revert non-vacuity (old semantics reimplemented): same four escaping shapes all leave home (
/home/escape,/tmp/evil, …)
Residual notes (non-blocking for this claim)
- Parallel raw readers still reimplement resolution and can escape — pre-existing, out of this diff’s API surface, but same env keys:
packages/coding-agent/src/hooks/native-skill-hook.ts—path.join(homedir(), process.env.GJC_CONFIG_DIR ?? …, "agent")still escapes on../../tmp/evilgjc-runtime/{deep-interview,ralplan,ultragoal}-runtime.tstreatGJC_CONFIG_DIRas a path root (not home-joined name) for settings/config.yml- Follow-up: route those through
getConfigDirName()/ home-join contract
- Not a project-
.envtrust boundary — Bun preloadscwd/.envintoprocess.env; a repo can still set a non-escaping home-relative name (e.g..ssh). Author correctly defers$credentialEnv-class treatment (see #3252 family). Documented honesty; not a hole in the claimed fix. - Sanitize returns original (possibly internally
..-bearing) text when normalize collapses — e.g.foo/../baraccepted as-is; afterpath.join(home, …)still under home. Cosmetic inconsistency only. - origin/dev at
2d877016…has unrelated coding-agent shard failures on push run30244279756. Not caused by this PR; exact-head PR CI is green. Prior policy: technical MERGE_READY stands; do not treat that red as a defect in this diff.
Merge policy note
Technical content of this PR is correct and proof-backed at exact head 284fc6a3b65b. Residual raw readers and trusted-env promotion are separate follow-ups, not regressions introduced here.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
stale exact-head review refresh: re-affirming APPROVED at current head (no diff since last review).
Documented as a pre-existing platform gap in the #3273 review: Bun loads cwd/.env into process.env before any gjc module runs, so a planted GJC_CODING_AGENT_DIR redirects getAgentDir() (dirs.ts:251). The agent directory selects the agent's own .env, which is one of the trusted sources $credentialEnv consults — so a repository could ship a directory, point the agent at it, and have its .env treated as trusted. That recovers every endpoint and credential redirect the boundary is meant to reject. Measured on this source, with repo/.env setting the agent dir and repo/.evil-agent/.env setting a provider base URL: before: $credentialEnv("ANTHROPIC_BASE_URL") = "https://attacker.example" after: undefined dirs.ts cannot use $credentialEnv — env.ts imports dirs.ts — so the .env parsing primitives move to a leaf env-file module that both can import, and dirs.ts applies the same conservative ambiguity rule directly: an override that matches what the project .env sets is not honoured. Their public surface is unchanged (env.ts re-exports them). An operator exporting GJC_CODING_AGENT_DIR from their shell is unaffected; verified both directions. Regression: a child process with a controlled cwd proves an inherited agent dir still supplies its .env, a planted one does not, and a planted one cannot redirect an inherited one. Reverting the check fails the planted case. GJC_CONFIG_DIR has the same shape and is deliberately untouched here: #3267 is already approved against that function, and binding its trust belongs in a successor rather than a conflicting edit.
getConfigDirName() returned GJC_CONFIG_DIR / PI_CONFIG_DIR verbatim. Consumers join that name with <home> to locate user-level discovery inputs: discovery/builtin.ts:235 path.join(ctx.home, userAgentDir, "mcp.json") discovery/builtin.ts:273 path.join(ctx.home, userAgentDir, "SYSTEM.md") discovery/builtin.ts:328 path.join(ctx.home, userAgentDir, "skills") runtime-skill-discovery.ts:66 path.join(home, canonicalUserDir, "skills") The documented contract is that the value names a directory beneath <home> — "even an absolute-looking configured name is joined beneath <home>" — which path.join delivers for a leading separator but not for ".." segments. A name such as ../../tmp/evil resolved to /tmp/evil/agent, moving user-level MCP server definitions, the system prompt and skill discovery outside the config root. Reject names with a ".." segment and fall back to the default, per key, so a legacy PI_CONFIG_DIR still applies when the primary one is rejected. Note: this restores the documented containment, not the trust boundary. Bun loads cwd/.env into process.env before any JS runs, so the value can still originate from the project .env; dirs.ts is imported by env.ts, so it cannot use $credentialEnv without a cycle. Making the name trusted needs a separate decision about where that primitive should live.
|
Rebased onto The conflict was with #3295, which merged in the meantime and replaced the direct return (
sanitizeConfigDirName(trustedConfigDirName("GJC_CONFIG_DIR")) ??
sanitizeConfigDirName(trustedConfigDirName("PI_CONFIG_DIR")) ??
CONFIG_DIR_NAME
);
Neither subsumes the other: a trusted value can still contain Verification after the rebase: |
284fc6a to
a9fe219
Compare
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Verdict: APPROVE — exact head a9fe2195c2b04ec1f30f2194cd1131ee4b24a08f
Rebase-only refresh of the prior exact-head review at 284fc6a3b65b8e9e37bb0f266b908ce80a942dbb (APPROVE).
Checks performed:
- Rebase adaptation verified, not assumed identical: unlike the other 21 PRs in this batch, this PR's own commit is not byte-identical between
284fc6a3b65b8e9e37bb0f266b908ce80a942dbbanda9fe2195c2b04ec1f30f2194cd1131ee4b24a08f—diff <(git show 284fc6a3 --format= -- packages/utils/src/dirs.ts packages/utils/test/config-dir-name-containment.test.ts) <(git show a9fe2195 --format= -- packages/utils/src/dirs.ts packages/utils/test/config-dir-name-containment.test.ts)shows a real textual difference. Root cause traced:git merge-base 284fc6a3b65b8e9e37bb0f266b908ce80a942dbb a9fe2195c2b04ec1f30f2194cd1131ee4b24a08fshows two independent PRs (#3285, #3295) landed ondevbetween the two reviews and added atrustedConfigDirName()/trustedAgentDirOverride()wrapper around the sameprocess.envreads this PR'ssanitizeConfigDirName()also wraps. This PR's commit necessarily rebased its hunk to composesanitizeConfigDirName(trustedConfigDirName(...))instead ofsanitizeConfigDirName(process.env...)directly. - Functional equivalence confirmed by direct read of
dirs.tsata9fe2195c2b04ec1f30f2194cd1131ee4b24a08f:sanitizeConfigDirName()'s own body (reject any..path segment, else fall back toCONFIG_DIR_NAME) is unchanged from the prior review; it now composes correctly on top of the already-merged trust guard (comment at the call site: "Both guards apply: the value must come from a trusted source ... and it must still be a single name that stays beneath home"). This is mechanical conflict resolution required by upstream rebase, not a new claim or behavior change by this PR. - Test file untouched:
packages/utils/test/config-dir-name-containment.test.tsis byte-identical between284fc6a3b65b8e9e37bb0f266b908ce80a942dbbanda9fe2195c2b04ec1f30f2194cd1131ee4b24a08f. - Exact-head terminal CI: 17 success / 0 failure / 0 pending / 4 skipped (total 21). No pending/queued checks.
Prior technical finding (path.join delivers containment for a leading separator but not .. segments; consumers join the name with <home>) still holds verbatim.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
- verify-gjc-sdk-canonicalization: allowlist the second sanctioned tmux send-keys fallback in relaunchWorkerPaneForMemoryGuard. The exact-match scan counted the memory-guard dispatch as a duplicate of the startTmuxSession loop dispatch and rejected all 12 occurrences. Loop counts are now scoped to startTmuxSession; the memory-guard block gets its own exact-shape matcher. Verified tamper-rejecting: payload swap, appended unsanctioned send-keys, and widened fallback predicate all still fail closed. - stats tests: TempDir.createSync used os.tmpdir(), so path.relative(os.homedir(), tmp) produced ../.. segments that sanitizeConfigDirName rejects (by design, #3267). PI_CONFIG_DIR then silently fell back to the real ~/.gjc and four suites read/wrote the live stats.db instead of an isolated root. Create the temp root under $HOME so the relative name stays beneath home.
2 participants
Problem
getConfigDirName()returnedGJC_CONFIG_DIR/PI_CONFIG_DIRverbatim. Consumers join that name with<home>to locate user-level discovery inputs:discovery/builtin.ts:235path.join(ctx.home, userAgentDir, "mcp.json")discovery/builtin.ts:273path.join(ctx.home, userAgentDir, "SYSTEM.md")discovery/builtin.ts:328path.join(ctx.home, userAgentDir, "skills")runtime-skill-discovery.ts:66path.join(home, canonicalUserDir, "skills")The contract is documented in
prompts/tools/skill-discovery.md: the value is the home-relative directory name, and “even an absolute-looking configured name is joined beneath<home>”.path.joindelivers that for a leading separator, but not for..segments.Measured against this source:
So an escaping name moves user-level MCP server definitions (which are spawned as processes), the system prompt, and skill/agent/plugin discovery outside the config root entirely.
Fix
Reject a configured name containing a
..segment and fall back to the default — evaluated per key, so a legacyPI_CONFIG_DIRstill applies when the primary one is rejected. Ordinary names, and the documented absolute-looking case, are unchanged.Tests
config-dir-name-containment.test.ts: default when unset, ordinary name honored, legacy key honored, absolute-looking name still lands beneath home, four escaping shapes rejected (../escape,../../tmp/evil,.gjc/../../tmp/evil,a/b/../../../../tmp/evil), escaping legacy key rejected, fall-through from an escaping primary to a valid legacy key, and a blank value ignored.Proof-first: reverting the guard turns seven of the eleven red.
bun run checkinpackages/utils(whole-package biome + tsc): exit 0.packages/utils/test+coding-agent/test/discovery+marketplace/discovery: 366 pass / 0 fail.Scope note
This restores the documented containment, not a trust boundary. Bun loads
cwd/.envintoprocess.envbefore any JS runs, so the value can still originate from a project.env;dirs.tsis imported byenv.ts, so it cannot reach$credentialEnvwithout a cycle. Making this name trusted the way #3252 / #3262 do needs a separate decision about where that primitive should live — happy to follow your preference if you want it moved.