fix(docker): wrap docker CLI invocations in user shell to inherit PATH (#378)
* fix(docker): ensure Docker executions use correct PATH via shell wrapping
* fix(upstream): remove unused os/exec import in monitoring.go
After migrating monitoring.go to use c.newDockerCmd, the os/exec import
was left dangling and broke the build. Remove it.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* feat(shellwrap): add shared shell wrapping + docker path cache package
Introduces internal/shellwrap to centralize the login-shell wrapping
logic and docker binary resolution that were previously duplicated
(or reinvented) across internal/upstream/core and
internal/security/scanner.
Key helpers:
* Shellescape — POSIX single-quote / cmd.exe quoting.
* WrapWithUserShell — wraps a command in $SHELL -l -c for Unix
(and /c for Windows cmd), honoring $SHELL instead of hardcoding
sh so zsh/fish users get their real interactive PATH.
* ResolveDockerPath — sync.Once-cached absolute path for the docker
binary. Tries exec.LookPath first and only falls back to a
one-shot login-shell probe when the fast path fails. Callers can
then exec docker directly on every subsequent call, avoiding the
cost of respawning a login shell on hot paths (health check,
diagnostics, ~every 2-5s).
* MinimalEnv — returns an allow-listed PATH+HOME environment
for subprocesses that must not inherit ambient secrets.
Includes unit tests covering:
* Shellescape round-trips for spaces, single quotes, $, backticks,
and glob stars.
* WrapWithUserShell honors $SHELL overrides and falls back to
/bin/bash when unset.
* ResolveDockerPath is cached across calls (second call returns
the same value even after PATH is sabotaged).
* MinimalEnv strips AWS/GitHub/Anthropic/OpenAI credentials while
retaining PATH.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(scanner): resolve docker directly and drop ambient env inheritance
The previous implementation of the scanner's getDockerCmd had three
problems flagged in PR review:
1. It hardcoded \`sh -l -c\` on Unix, ignoring \$SHELL. Users whose login
shell is zsh/fish would get a different PATH than the one they see
in Terminal.
2. It hardcoded \`cmd /c\` on Windows, breaking Git Bash / WSL users,
and used strings.Trim(arg, "\"") which silently mangles legitimately
quoted arguments.
3. It inherited the full user environment via sh -l -c. Because the
scanner runs untrusted container images (vulnerability scanners,
SBOM generators, …), ambient secrets such as AWS_ACCESS_KEY_ID,
GITHUB_TOKEN, or ANTHROPIC_API_KEY could leak into scan sandboxes.
Fix:
* Use shellwrap.ResolveDockerPath to locate the docker binary once
(cached process-wide) and exec it directly — no shell wrapping.
* Set cmd.Env to shellwrap.MinimalEnv(), which retains only the
PATH + HOME (+ Windows equivalents) needed for docker to run.
* Drop the custom quoting logic entirely; exec.Command already
passes arguments verbatim when we skip the shell.
Added TestDockerRunnerDoesNotLeakAmbientSecrets which pollutes the
process env with fake AWS/GitHub/Anthropic credentials and then
asserts cmd.Env is non-nil and does NOT contain any of those keys
while still carrying PATH.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* perf(upstream): cache docker path and delegate shell wrapping to shellwrap
The Docker PATH fix in this PR made every newDockerCmd call re-spawn
sh -l -c so that docker would be discoverable when mcpproxy is
launched from Launchpad / a LaunchAgent. That works, but
checkDockerContainerHealth and GetConnectionDiagnostics fire every
few seconds each, which means we were re-reading .zshrc / .bashrc
dozens of times a minute per Docker upstream.
Fix:
* Resolve the docker binary once via shellwrap.ResolveDockerPath
(sync.Once) and exec it directly on subsequent calls. Retain a
fallback to the existing c.wrapWithUserShell path in case the
cache resolution fails (e.g. uncommon install layouts), so the
original "works when launched from Launchpad" guarantee stands.
Also deduplicate the shell wrapping implementation:
* c.wrapWithUserShell is now a thin wrapper around
shellwrap.WrapWithUserShell. It still emits the server-scoped
debug log line that existed before for log continuity.
* The package-local shellescape helper is kept as an alias of
shellwrap.Shellescape for backward compatibility with the
existing TestShellescape suite in shell_test.go.
No behavior change for the personal edition binary beyond the perf
improvement — docker commands are still launched with the correct
PATH and the secure environment built by c.envManager.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Code <noreply@anthropic.com> D
Dumbris committed
b6dc5ade72eefe1a237d87d38baff2c2956c20d2
Parent: 4dad6d1
Committed by GitHub <noreply@github.com>
on 4/11/2026, 2:45:06 PM