fix: harden coding agent runtime settings

[moose-lab]

Rebuilds the post-PR #30 runtime/settings hardening on top of current main, excluding docs/plans changes.

Fixes:

• Scope browser BYOK keys by provider protocol and base URL so keys do not leak across providers.
• Reject invalid custom Local CLI binary overrides during launch preflight.
• Recheck API session status before saving provider output after an awaited request.
• Require local CLI connection smoke tests to exit 0 before reporting success.

Verification:

• node --test --import tsx --test-reporter spec src/core/tests/agent-settings.test.ts src/core/tests/process-manager-health.test.ts src/core/tests/api-session-runtime.test.ts src/core/tests/agent-connection-test.test.ts
• bun run test
• bun run typecheck
• bun run build
• git diff --check

Docs/plans: intentionally excluded.

[claude]

Claude finished @moose-lab's task in 3m 54s —— View job

---

PR Review: fix/coding-agent-runtime-settings

• Gather context and read CLAUDE.md
• Review core changes: agent-settings.ts, session-runtime-auth.ts, process-manager.ts
• Review new modules: api-provider-runtime.ts, api-session-runtime.ts, agent-connection-test.ts, local-cli-agents.ts
• Review API routes and UI changes
• Review test coverage
• Post final review findings

---

Findings

• [must-fix] isBlockedExternalHost does not block IPv4-mapped IPv6 addresses (::ffff:192.168.1.1). parseIpv4 returns null for them (the host starts with ::ffff: so the colons break the 4-octet split), and they match none of the IPv6 range regexes, so validateAgentConnectionBaseUrl returns { url } — allowed. A server-side fetch to https://[::ffff:192.168.1.1] on a Linux host resolves via the OS IPv4-mapped stack directly to 192.168.1.1. Fix: after parseIpv4 returns null, add a check for the /^::ffff:/i prefix, extract the trailing IPv4 literal, and run it through the existing parseIpv4 / range guards. — src/core/api-provider-runtime.ts:86-101 Fix this →

• [should-fix] classifyProviderFailure maps HTTP 403 to auth_failed (src/core/api-provider-runtime.ts:481). A 403 typically means the key is valid but lacks permission (wrong tier, model access restriction), not that it's missing or invalid. This gives users confusing guidance. Fix: add a separate "forbidden" kind (already exists in AgentConnectionTestKind) or at minimum return "forbidden" for 403 and "auth_failed" only for 401.

• [should-fix] In runProviderSessionTurn, the user message is written to session_messages and the session status is set to 'running' before await runProviderChatCompletion (api-session-runtime.ts:95-102). If the session is killed while the fetch is in-flight, the post-await status check (postRequestStatus !== "running") correctly prevents the assistant message from being saved, but the user message is left orphaned — no assistant reply, session shows killed. The session history is then inconsistent (odd number of turns). Consider either: (a) not persisting the user message until after the status check, or (b) deleting it on kill-detection.

• [nit] appendVersionedApiPath appends /v1 to any base path that does not already end with /v1 (api-provider-runtime.ts:358-365). A proxy serving at https://my-proxy.com/v2 would produce …/v2/v1/messages. Only the exact suffix /v1 is detected; alternatives like /v2, /v1beta, or /openai/v1 silently get an extra /v1 segment inserted. Fix this →

• [nit] The scope-key separator in getBrowserApiKeyScope is a literal \n character (agent-settings.ts:566). It's correct (neither protocol nor normalized URL can contain a newline), but it reads like a formatting mistake. A brief comment would prevent future readers from "fixing" it.

What's well done:

• getBrowserApiKeyScope / setScopedBrowserApiKey cleanly isolate BYOK keys per (protocol, baseUrl) pair — the core ask of the PR.
• resolveConfiguredLocalCliBinary using fs.existsSync to reject invalid CLI binary overrides before spawn is exactly the right preflight.
• The post-await getStoredSessionStatus check in runProviderSessionTurn is the correct approach for the kill-race problem (and well-tested).
• redactSecrets in error paths protects API keys from leaking into logs.
• isBlockedExternalHost IPv4 range coverage (RFC1918 + CGNAT + link-local + multicast) is comprehensive for non-mapped addresses.
• Test coverage is strong: the new test files all test the named failure modes with injected fakes rather than relying on real network/process calls.

Verdict: REQUEST_CHANGES — the IPv4-mapped IPv6 SSRF bypass is a must-fix before merge given that runProviderChatCompletion issues server-side fetch calls to the user-provided URL.

[claude]

Claude finished @moose-lab's task in 1m 59s —— View job

---

Re-review: fix/coding-agent-runtime-settings

• Read CLAUDE.md for repo guidelines
• Check if the must-fix IPv4-mapped IPv6 SSRF bypass was addressed
• Check if the should-fix issues were addressed
• Post updated review findings

---

The branch was synchronized but the code is unchanged from the previous review — there is still only one commit (789308e). All prior findings remain open.

Findings

• [must-fix] isBlockedExternalHost still does not block IPv4-mapped IPv6 addresses (::ffff:192.168.1.1). parseIpv4 returns null (the ::ffff: prefix breaks the 4-octet split), and the trailing IPv6 regexes only cover fc00::/7 and fe80::/10 — neither matches ::ffff:*. A server-side fetch to https://[::ffff:192.168.1.1] on Linux resolves to the private host via the IPv4-mapped stack. Not addressed. — src/core/api-provider-runtime.ts:91-107 Fix this →

• [should-fix] classifyProviderFailure maps HTTP 403 → auth_failed (api-provider-runtime.ts:540). 403 means the key is valid but lacks permission (wrong tier, model restriction), not invalid — the user gets confusing "authentication failed" guidance. Fix: return "forbidden" for 403, reserve "auth_failed" for 401 only. Not addressed.

• [should-fix] In runProviderSessionTurn, the user message is written to session_messages and status set to 'running' before await runProviderChatCompletion (api-session-runtime.ts:94-102). On kill-detection the assistant reply is correctly dropped, but the user message is left orphaned — odd number of turns, inconsistent history. Consider deferring the user message insert until after the status check, or deleting it on kill-detection. Not addressed.

• [nit] appendVersionedApiPath only detects /v1 at the end of a base path before appending /v1 (api-provider-runtime.ts:421-423). A proxy at /v2 gets …/v2/v1/messages. Not addressed.

• [nit] getBrowserApiKeyScope uses a literal \n as the key scope separator (agent-settings.ts:566). Correct, but reads like a formatting mistake. A brief inline comment would prevent future readers from "fixing" it. Not addressed.

What's well done (unchanged from prior review):

• getBrowserApiKeyScope / setScopedBrowserApiKey cleanly isolate BYOK keys per (protocol, baseUrl) pair.
• resolveConfiguredLocalCliBinary preflight with fs.existsSync before spawn.
• Post-await getStoredSessionStatus kill-race check and its test coverage.
• redactSecrets in error paths.
• Strong test coverage overall using injected fakes.

Verdict: REQUEST_CHANGES — the IPv4-mapped IPv6 SSRF bypass remains a must-fix before merge.

[claude]

Claude finished @moose-lab's task in 1m 17s —— View job

---

Re-review: fix/coding-agent-runtime-settings

• Read CLAUDE.md for repo guidelines
• Check if must-fix IPv4-mapped IPv6 SSRF bypass was addressed (api-provider-runtime.ts)
• Check if should-fix issues were addressed (403 mapping, orphaned user message)
• Check if nit issues were addressed
• Post updated review findings

---

The branch was rebased (commit hash changed from 789308e → 30eb04e) but code is unchanged. All prior findings remain open.

Findings

• [must-fix] isBlockedExternalHost still does not block IPv4-mapped IPv6 addresses (::ffff:192.168.1.1). Line 106 returns false for any host that isn't ::, FC/FD, or FE80 — ::ffff:* addresses fall through and are allowed. A server-side fetch to https://[::ffff:192.168.1.1] on Linux resolves directly to the private host via the IPv4-mapped stack. Not addressed. — src/core/api-provider-runtime.ts:106 Fix this →

• [should-fix] classifyProviderFailure still maps HTTP 403 → auth_failed (api-provider-runtime.ts:541). A 403 means the key is valid but lacks permission (tier/model restriction), not that it's invalid. Return "forbidden" for 403, keep "auth_failed" for 401 only. Not addressed.

• [should-fix] runProviderSessionTurn still inserts the user message and sets status to 'running' before await runProviderChatCompletion (api-session-runtime.ts:94-101). On kill-detection the assistant reply is correctly dropped but the user message is left orphaned, producing inconsistent history (odd number of turns). Not addressed.

• [nit] appendVersionedApiPath still only checks basePath.endsWith("/v1") before appending /v1 (api-provider-runtime.ts:422). A proxy at /v2 gets …/v2/v1/messages. Note: googleGenerateContentUrl at line 431 already uses the correct /\/v\d+(?:beta)?$/ regex — appendVersionedApiPath should do the same. Not addressed.

• [nit] getBrowserApiKeyScope uses a literal \n as the key scope separator with no explanation (agent-settings.ts:566). A brief inline comment prevents future readers from treating it as a bug. Not addressed.

Verdict: REQUEST_CHANGES — the IPv4-mapped IPv6 SSRF bypass (must-fix) remains unaddressed and is a real attack surface given server-side fetch is called with the user-supplied URL.