Repository navigation
fix(config): refuse project provider keys that move the API key - #613
Merged
Merged
Conversation
Problem: a project .harness.json could set base_url, responses_path, extra_headers, api_key_env, and permission_mode on a provider that the user config defines, so an untrusted checkout could send the API key or any environment variable to its own endpoint, or set bypassPermissions. Design: MergeProject checks the project providers in one rule, rejectProjectProviderKeys. A project file cannot set auth, and cannot set those five keys on a provider that the user config defines or that is native (nativeProviderKeys). A provider that only the project defines may set them. The rule replaces the earlier drop-and-warn auth rule. Semantic change: the load fails with an error that names the key, the provider, and both files. binary_path and extra_args stay allowed. Verification: config table test, CLI contract row, go test -race ./..., HARNESS_E2E_RUNTIME=1 go test -race ./e2e. The codex row that sent a project base_url is removed: the new CLI row covers the refusal.
andybons
force-pushed
the
fix/project-config-provider-keys
branch
from
October 14, 2026 02:23
5bf48c5 to
4c9b8e8
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The changelog currently makes a materially inaccurate security guarantee about project-defined providers.
1 open finding
What changed in this PR
Restricts project configuration from redirecting credentials through existing or built-in providers.
Changes:
- Rejects protected project provider keys with explicit errors.
- Adds unit and CLI contract coverage.
- Updates architecture documentation and removes obsolete Codex coverage.
| File | Description |
|---|---|
CHANGELOG.md |
Documents the security fix. |
config/load.go |
Adds provider-key rejection. |
config/merge.go |
Removes drop-and-warn behavior. |
config/project_keys_test.go |
Tests protected and permitted keys. |
config/config_test.go |
Updates merge expectations. |
config/auth_test.go |
Removes superseded authentication test. |
docs/architecture.md |
Defines the new trust boundary. |
e2e/contract_cli_test.go |
Tests CLI rejection and no request. |
e2e/contract_codex_test.go |
Removes project-file setup. |
e2e/contract_codex_auth_test.go |
Removes obsolete Codex scenario. |
e2e/runtime_rows_test.go |
Removes the obsolete runtime row. |
e2e/testdata/contract/codex_login_project_file_cannot_move_the_login_to_another_url.golden.json |
Deletes the obsolete golden. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… over ACP Problem: a provider that only the project file defined could still name api_key_env, or fall back to OPENAI_API_KEY, and send that key to the base_url of the project. The ACP error for a project file that does not load said only "harness: invalid request". Design: rejectProjectProviderKeys fails the load when a project file sets api_key_env on any provider. The default key variable moves into the native provider table of config (NativeKeyEnv), and the router reads the variable only for a native entry, so an openai-type entry under another name reads no default variable. The router lists the native providers from the same table. rpcError sends the text of an invalid_request error in data.reason. Semantic change: a project-only provider sends no API key. A project file that sets api_key_env fails to load. A session/new for such a project answers -32602 with the config error in data.reason. Verification: config table test, CLI rows (codex login moved by a project file, api_key_env in a project file, no OPENAI_API_KEY sent to a project provider), ACP row, go test -race ./..., HARNESS_E2E_RUNTIME=1 e2e.
…the wasm gzip budget The wasm module grew by 860 bytes of gzip over its budget with the new config check and the data.reason of an invalid_request error. The budget rises by 50,000 bytes as the file asks for a PR that grows the module. The Node wasm host names its user config 'the config of harnessConnect' in an error, so its golden is aligned with the native hosts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A host tool named like a built-in tool, for example
bash, failsNew. An sdk/embed page tool or a GoOptions.Toolstool with that name makessession/newfail withinternal, also when the allowed list leaves the built-in out. The browser default set excludesbash, so a page that wants its ownbashtool cannot have one.Design
Spec: docs/architecture.md
Tool names(Four rules, AllowedTools bullet),Runtime(New),Built-in tools, and the wasmToolsrow. Design: docs/design/acp-wasm.md section 5.1, 5.6.Newno longer rejects anOptions.Toolsname fromread_file,write_file,edit_file,glob,grep,ls,bash(Runtime.shadowable, the one owner of the rule).checkModelfailsCreatewithinvalid_request(ErrInvalidRequest, ACP invalid params) and names the tool whenAllowedToolsis nil, because the built-in tool is then allowed too.read_tool_result,session_info,process,task,modelkeep their rules. The spec is silent on plugin and MCP tools, so only host tools change.Semantic change
A violation fails the load with an error that names the key, the provider, and both files, in place of a warning and a dropped key. A project file cannot set
api_key_envon any provider, and a provider that only the project file defines sends no API key: the default key variable (ANTHROPIC_API_KEY,OPENAI_API_KEY) moved into the native provider table ofconfig(NativeKeyEnv,NativeProviders), and the router reads it only for a native entry. Anopenaitype entry under another name now needsapi_key_envin the user config. A provider with no key fails its request with "no API key" before any request is sent (an existing behavior of the adapters), so a project-only provider reaches no endpoint with a key; a keyless local model needs anHTTPClienttransport that supplies the credentials.harness acpsends the text of aninvalid_requesterror indata.reason, so an editor shows which key, provider, and file failed. The codex contract row that sent a projectbase_urlis replaced by a CLI row that checks the load fails and that neither the endpoint, the evil URL, nor the issuer gets a request. The WebAssembly gzip budget rises by 50,000 bytes: the module was 860 bytes over after this change.Boxes:
cmd/boxinit/config.gowritesapi_key_envon every provider it writes (bifrost, codex, and the native entries), so no boxes user-config entry relied on theOPENAI_API_KEYfallback. No in-repo fixture did either.Verification
Table test in
config/project_keys_test.go. CLI contract rows: a projectanthropic.base_url, a project file that moves the codex login, a projectapi_key_env, and noOPENAI_API_KEYsent to a project-only provider. One ACPsession/newrow for a bad project file that checksdata.reason.go build,go vet,go test -race ./...,HARNESS_E2E_RUNTIME=1 go test -race ./e2e, gofmt pass.CI job times, this PR vs main (seconds): Static 42 vs 40, Unit 54 vs 56, Contract 84 vs 89, Runtime contract 53 vs 61, Gates 33 vs 35, Browser 115 vs 120, WebAssembly 22 vs 27, Node wasm 72 vs 65. Node wasm grew 7 s: it runs the new ACP row on the wasm module (about 2.5 s) and the rest is runner noise. No other job grew by more than 5 s.