fix(coding-agent): resolve auth-broker config from trusted env only

[10kH]

New site in the same trust-boundary class as #3252 / #3262 / #3270 — and the widest one so far, because it replaces the credential store rather than one provider's endpoint.

Problem

// session/auth-broker-config.ts
const envUrl = process.env.GJC_AUTH_BROKER_URL;
const envToken = process.env.GJC_AUTH_BROKER_TOKEN;

Bun.env === process.env, and the env module folds the caller's cwd/.env into it.

discoverAuthStorage() (sdk/session.ts:548, reached from main.ts:1239) turns this result into an AuthBrokerClient + RemoteAuthCredentialStore and uses it as the AuthStorage for every provider. So a repository could plant two lines in .env and replace the agent's whole credential store — serving the credentials it authenticates with, and receiving the ones it writes back on login/refresh.

Measured against this source, with a project .env as the only input:

before: resolveAuthBrokerConfig() = {"url":"https://attacker.example","token":"attacker-token"}
after:  null

A planted URL without a token is also relevant: the function throws when a URL is set with no token available, so an unfixed planted URL could also fail the agent at startup.

Fix

Resolve both through $credentialEnv, the resolver this codebase already designates for material that must not come from the caller's project .env. Everything an operator actually uses is untouched: the launching shell, the auth.broker.url / auth.broker.token config entries (with !command resolution), and the ~/.gjc/auth-broker.token file.

Tests

auth-broker-env-trust.test.ts spawns a probe with a controlled cwd (the env module parses projectEnv at load time, so the boundary is only observable across processes) and points GJC_CODING_AGENT_DIR at a temp dir so the probe never reads a developer's real config.yml: no broker by default, a planted URL+token is ignored, a planted URL alone does not reach the throw path, an inherited broker still applies, and a planted .env cannot override an inherited one.

Proof-first: reverting the two reads fails two of the five.

Verification

bun run check in packages/coding-agent (whole-package biome + tsc): exit 0. New suite: 5 pass / 0 fail. Broker-related suites across the package: 514 pass / 0 fail.

[Yeachan-Heo]

Adversarial review — exact head ce195a0648626b1128063ba6473eda97908b8b70 vs origin/dev 3649db42e4f31cb373c0cee5703fa4c0b0996680

Verdict: MERGE_READY (do not merge yet — see Merge gating.)

Reviewed the exact head, not the branch tip. Merge-base 2d8770169c15cd13efd54aba43ea2cddaab56a93. Diff sha256 4994d639c3636aa75a290be803efaeb7f6982c6bf7ca01efeac83e62534fa8dc.

Trust-boundary claim — independently verified, not taken from the PR body

I did not reuse your probe. I wrote my own against packages/utils/src/env.ts directly, with a fresh HOME and GJC_CODING_AGENT_DIR per scenario:

#	Scenario	$credentialEnv	$env (merged)
A	hostile project .env, nothing inherited	null	fully attacker-controlled
B	empty / whitespace / no-= / bare-key lines	null	"", " ", "", null
C	shell override vs hostile .env	operator value wins	operator value
D	inherited value identical to project .env value	null	value present
E	GJC agent-owned .env + hostile project .env	agent-dir value	attacker value

Scenario E is the one that actually matters here, and it is the one a weaker fix would fail: the trusted file sources still work while the project .env is rejected. Scenario D is the conservative ambiguity exclusion at env.ts:167-174 — correct, and worth knowing about since it means an operator whose shell value coincides with the repo's value resolves to undefined.

Surface closure is total, which I checked mechanically rather than by reading the diff: process.env. reads in auth-broker-config.ts before = 2 (:75, :76), after = 0.

Empty / malformed values

$credentialEnv → resolveFileEnvValue (env.ts:115-121) trims and returns undefined for zero-length, so the downstream envUrl && envUrl.length > 0 and envToken && envToken.length > 0 guards are now redundant rather than load-bearing. Harmless. Confirmed by scenario B: no crash, no empty-string URL reaching the client.

The "planted URL without a token" case deserves the separate test you gave it — an unfixed planted URL reaches the throw and becomes a startup DoS, not just a redirect. That test earns its place.

Credential leakage

Checked, since this PR moves a token. The throw at :102-107 interpolates ${url} but never ${token}, and that URL echo is pre-existing on dev — unchanged by this PR. No new logging of either value. Clean.

Caller overrides and precedence

config.yml (auth.broker.url / auth.broker.token, incl. !command) and ~/.gjc/auth-broker.token are untouched, and env still wins over config — matching the documented precedence in docs/environment-variables.md:97.

Residual, correctly out of scope

readConfigYaml() → resolveConfigValue() reads raw process.env[config] (resolve-config-value.ts:24), i.e. the merged view. Not repo-controlled: it only triggers for a var the operator's own ~/.gjc/agent/config.yml explicitly names. Right call to leave it; flagging it so it is on the record.

Overlap and ordering with the open env-trust set

Compared file sets across #3262 / #3268 / #3270 / #3272 / #3273 / this PR. The only shared source file in the whole batch is packages/ai/src/providers/azure-openai-responses.ts, between #3268 and #3272 — neither is this PR. This PR shares only packages/coding-agent/CHANGELOG.md with #3270/#3273, and the three entries occupy distinct lines in the same Unreleased/Fixed block. Textual adjacency only; no semantic ordering dependency, mergeable in any order.

Changelog placement verified: correct ## Unreleased → ### Fixed block, directly alongside the already-merged browser-launch entry it is a sibling of.

Automated review comments

Zero. pulls/3276/reviews = 0, pulls/3276/comments = 0, issues/3276/comments = 0. Nothing to triage or dismiss.

CI

Terminal green, including the dedicated test:packages/coding-agent/test/auth-broker-env-trust.test.ts job and whole-package check:@gajae-code/coding-agent.

Test hermeticity — noted, not blocking

env.GJC_CODING_AGENT_DIR = tempDir() is the right instinct and is more than the merged precedent does. It is still not fully hermetic: $credentialEnv also reads homeEnv and homeShellEnv (env.ts:220-229), which inherit the real HOME. I verified that a developer with GJC_AUTH_BROKER_URL in ~/.env resolves https://home-broker.example where the suite asserts config === null. Latent only — it does not reproduce on a clean machine or in CI, and the production change is unaffected. Worth an env.HOME = tempDir() in a follow-up.

Pre-existing platform gap (explicitly not charged to this PR)

Bun loads cwd/.env into process.env before any gjc module runs, so a planted GJC_CODING_AGENT_DIR redirects getAgentDir() (dirs.ts:251) and makes the "trusted" agent .env repo-internal — recovering the full attack. I confirmed this is not a regression here: neither PR touches dirs.ts or env.ts, dirs.ts:251 is byte-identical across dev/pr3273/pr3276, and the same escalation defeats the already-merged trustedBrowserEnv on dev today (PUPPETEER_EXECUTABLE_PATH → /tmp/evil-browser). Separate follow-up against the shared resolver. This PR is a strict improvement over its base.

Merge gating

MERGE_READY is a verdict on this head, not an instruction to merge. Current dev CI is red on unrelated work; hold until it is green.

—
[repo owner's gaebal-gajae (clawdbot) 🦞]

[Yeachan-Heo]

Legacy/open batch triage: closing without merge in this lane. Reopen with an updated review request if this should still land.

[Yeachan-Heo]

Closed as part of legacy/open PR batch triage (external REQUEST_CHANGES close, no-merge lane).