[james-crowley]

Description

Second of the two PRs for #1706 (Spec 111 FR-030), on top of #1716 (merged). Closes #1706. The path allow-list is the separate follow-up #1721.

Adds the file provider next to env and keyring. It is registered in secret.NewResolver, so ${file:/absolute/path} works everywhere the existing references do: an upstream server's env, args and headers (resolved on a copy, so the stored config keeps the reference), data_dir, and the proxy's own api_key.

• Read rules are secret.ReadFile's from feat(config): read the API key from MCPPROXY_API_KEY_FILE #1716: absolute path, symlinks followed but the target must be a regular file, 64 KiB cap, one trailing LF or CRLF removed (an LF file and a CRLF file resolve to the identical value), NUL rejected, errors name the reference and reason and never the content. An empty file is an error, as an empty env var is for ${env:}.
• Read each resolve, nothing cached, so a server picks up a rotated secret the next time it connects.
• Servers keep the existing behaviour: a missing file is a missing-secret error naming the reference and the proxy stays up (US7 scenario 2).
• api_key: a reference is resolved at load as a process-only override, so the process runs with the secret while every save, including the one a PATCH /api/v1/config triggers, writes the reference back and never the secret. One that does not resolve fails the load and never falls back to a generated key. A key from the environment overrides the file's, and the reference is then not read. Editing api_key itself is still persisted as the operator's edit.
• Spec 111 FR-030, docs (keyring-integration.md, upstream-servers.md, environment-variables.md) and CHANGELOG updated.

@Dumbris, one security question. ${file:} reads any file the proxy process can read, so a server added through the upstream_servers tool with env: {X: "${file:/data/mcp_config.json}"} could hand that file to the server's process. ${env:} already has the same exposure (it can name MCPPROXY_API_KEY), servers added that way are quarantined, and allow_server_add gates the tool, so I treated it as the same trust boundary and documented it. Would you like a path allow-list (for example a secret_file_dirs setting that defaults to /run/secrets, /run/credentials and the data directory's secrets folder) in this PR, or as a follow-up?

Testing

• I have tested these changes locally
• I have added/updated tests that prove my fix is effective or my feature works
• All existing tests pass

go test ./internal/config ./internal/secret pass. New tests cover the provider, LF/CRLF equality, error text without content, api_key resolving as an override and surviving a save and a PersistableConfig round trip, an edit being persisted, an unresolvable reference failing the load with the file untouched, an env key winning without reading the reference, and server env/headers/args expanding on a copy. By hand with a built binary: a reference in api_key authenticates, PATCH {"tools_limit": 21} is saved with api_key still the reference, a rotated file works after restart, and a missing file refuses to start.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 91.83673% with 4 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/config/api_key_env.go	83.33%	2 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

[Dumbris]

Good call treating it as the same trust boundary as ${env:} for now, and thanks for documenting it. Please keep the path allow-list out of this PR. I would rather do it as a follow-up that covers both providers at once, something like a secret_file_dirs setting defaulting to /run/secrets, /run/credentials and a secrets folder under the data dir, plus the equivalent guard for ${env:} naming MCPPROXY_API_KEY. If you want to file that issue with your proposed defaults I will take it from there. I will review this one once #1716 is in and you have rebased.

[james-crowley]

Rebased onto the updated #1716 (28730ef, both-set is an error; an env key still wins over a ${file:} api_key reference, unchanged). Windows fix: the api_key reference tests now build mcp_config.json with encoding/json instead of formatting a C:\... path into a string, and the symlink test skips on Windows if the account cannot create one. Docs state what an absolute path is on Windows. All checks green on 09a7161. Left as draft.

[james-crowley]

Rebased onto main now that #1716 is in (6061e50): only the five ${file:} commits remain, and the only conflict was the CHANGELOG. All checks green, golangci-lint v2.9.0 clean on the touched packages. Ready for review. The allow-list follow-up is #1721.

[Dumbris]

Thank you, James. This is careful work: the read rules are shared with #1716, the api_key override reuses the existing persisted-vs-effective machinery instead of adding a new path, and the docs state what an absolute path is on Windows. Approving, with one fix I pushed to your branch.

What I found and fixed (a97dd95)

After the key file is rotated, any load that does not become the live config re-registered the api_key override with the new value. The server restart does this: refreshServerFromDiskForRestart re-reads the whole file. The running key then no longer equalled the override, so the next save treated it as an operator edit and wrote it in clear over the reference.

Reproduced with a built binary:

1. start with api_key: "${file:/…/key}" (key A)
2. overwrite the file with key B
3. POST /api/v1/servers/s1/restart
4. PATCH /api/v1/config {"tools_limit":22}
5. mcp_config.json now contains "api_key": "KEY-A-…"

The fix: once a reference has been resolved, later loads in the same process carry the key the process runs with. That matches the running config, because api_key is restart-gated and a reload pins the live value anyway. The file is still read, so an unusable reference still fails the load. A rotated key takes effect on the next start. I corrected the doc line that promised "restart or config reload". A regression test (TestPersistableConfig_APIKeyReferenceSurvivesARotationAndAnotherLoad) fails without the fix. The same steps now leave the reference on disk.

What I verified (on the branch, 6061e50 plus the fix)

• go test -race ./internal/secret/... ./internal/config/... passes.
• ./scripts/test-api-e2e.sh passes (70/70, with the server-edition binary built).
• Both golangci-lint v2 runs (bare and --build-tags server) report nothing in your files. Three reflect.Ptr govet notes in secret/resolver.go and one in config/zero_value_preservation_test.go are on lines this PR does not touch and are also on main.
• A real mcpproxy serve with api_key: "${file:…}" and a CRLF key file:
  • the right key gets 200 and a wrong key gets 401
  • PATCH {"tools_limit":21} is saved and api_key stays the reference
  • the secret appears neither in mcp_config.json nor in the log (only the masked prefix)
• Server env and args with ${file:} reach the child as tok-XYZ (trailing LF removed). A relative path and a missing file are refused, with an error that names the reference and no content.
• No Gemini or other bot review comments exist on this PR (only the Codecov note).
• One cross-model round with opencode github-copilot/gpt-6.1-sol, three findings, each checked by hand:
  • the P1 above (confirmed by the repro)
  • the docs claim about reload (confirmed, fixed)
  • a note (below)
• It merges cleanly with current main.

Not blocking, for @Dumbris or a follow-up

• api_key is only resolved in Validate(), not in ValidateDetailed(). PATCH {"api_key":"${file:/nonexistent}"} is accepted and saved, and the next start then refuses to run. That is the intended hard-fail, but a write-time check would surface the typo earlier. It fits the #1721 follow-up better than this PR.
• This is unchanged behaviour: a server whose ${file:} (or ${env:}) cannot be resolved is still started with the literal placeholder as the value, and the error is only logged. It matches US7 scenario 2 as written.

Merge is Algis's call; I have not merged.

[Dumbris]

Merged. Thank you, @james-crowley! ${file:/absolute/path} means a container or systemd deployment can keep its API key and upstream secrets in mounted files instead of the environment or the config on the volume, and the resolver refuses relative paths and anything outside the allow-list. The rotation case you hit during review is fixed on your branch as well.

Contributions like this are what keep MCPProxy moving, and they matter a lot to the project.

One small ask, only if you feel like it: a short public note about your change (a tweet, a LinkedIn post, a line in a community you are part of) helps more people discover MCPProxy. No pressure at all, and I would be glad to repost it.