Repository navigation
fix(docker): pass env values to docker via environment, not -e KEY=VALUE argv - #1811
Merged
Merged
Conversation
…LUE argv
For a stdio server whose command is `docker run …`, and for a server under
Docker isolation, every entry of the server's `env` map was turned into a
`-e KEY=VALUE` argument of the docker process. Resolved secrets (`${keyring:…}`,
`${env:…}`, `${file:…}`) therefore sat in the process table, readable by every
local user via `ps aux` or `/proc/<pid>/cmdline`.
Both spawn paths already merge `config.Env` into the docker client's
environment, so emit the key-only `-e KEY` form: the docker CLI takes the
value from its own environment, which only the owner and root can read.
Keys are emitted in sorted order so argv and its log line are deterministic.
An empty value still reaches the container as set-but-empty.
Closes #1809
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Deploying mcpproxy-docs with
|
| Latest commit: |
2eef47e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://641aa0b4.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://claude-issue-1809-fix-review.mcpproxy-docs.pages.dev |
…shell edge Review round 1 (Sol 6.1): prependDockerDirToPath rewrites the docker client's PATH after the config.Env merge, so a key-only `-e PATH` would forward the host docker dir into a container whose PATH the user configured. PATH is never a secret, so it stays `-e PATH=<value>`. The login-shell fallback (docker not resolvable) lets a profile that exports the same name win; this is documented in the helper and in docs/docker-isolation.md. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Review round 2 (Sol 6.1): Windows spells the variable `Path`, so the inline exception now matches case-insensitively and keeps the configured spelling. The login-shell caveat also covers a resolved docker on Linux without DOCKER_HOST/DOCKER_CONTEXT exported, not only an unresolvable docker. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 41296613339 --repo smart-mcp-proxy/mcpproxy-go
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Summary
For a stdio server whose command is
docker run …, and for a server under Docker isolation, every entry of the server'senvmap was turned into a-e KEY=VALUEargument of thedockerprocess. Resolved secrets (${keyring:…},${env:…},${file:…}) therefore sat in the process table, readable by every local user viaps auxor/proc/<pid>/cmdline.Both spawn paths (
connection_stdio.go,connection_launcher.go) already mergeconfig.Envinto the docker client's environment, so this change emits the key-only-e KEYform ininjectEnvVarsIntoDockerArgsand the isolationBuildDockerArgs. The docker CLI takes the value from its own environment, which only the process owner and root can read. Keys are emitted in sorted order so argv and its log line are deterministic. An empty value still reaches the container as set-but-empty.Closes #1809
Verification
internal/upstream/core/docker_env_argv_test.gocover the helper, the isolation builder, and the launcher direct-exec and shell-wrap paths (secret incmd.Env, never incmd.Args); existing assertions flipped from-e KEY=VALUEto-e KEY.go test -race ./internal/upstream/core/green; CI golangci-lint config clean apart from the pre-existingreflect.Ptrbaseline finding.npxserver and one user-supplieddocker runserver, both with a secret inenv:ps axwwshows only-e ISO_SECRET_1809/-e USR_SECRET_1809, no value anywhere in argv;docker inspecton both containers shows the values (andISO_EMPTY=) arrived.Docs
docs/docker-isolation.md(Environment Variables) anddocs/features/keyring-integration.md(file references) now state where anenvvalue is and is not visible.