astronomer / astro-cli / 39062578433

45%

Build Type

push

github

Committed by web-flow

Commit Message

Pass registry passwords over stdin instead of through a shell string (#2221)

* Stop shelling registry passwords through bash on login

Docker/podman login built its command as a bash -c string with the
password spliced in directly. A password containing a quote, a dollar
sign, or backticks could break the login or get executed by the shell.
It also meant login only worked on hosts with bash installed.

Login now runs the container runtime binary directly with exec.Command
and writes the password to its stdin pipe for --password-stdin, so the
shell never sees it. Same change in airflow/docker_image.go (Pull and
pushWithBash) and airflow/docker_registry.go (dockerLogin).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Stub cmdExecWithStdin in the 403 push test so it stops invoking the real runtime

The bash-less login fallback added for the password-stdin fix runs through
cmdExecWithStdin, which this subtest never stubbed, so CI executed a real
docker login against a dummy registry and failed on connection refused.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P6DEdUpFBFaAiPKEu81Wti

* Test the real cmdExecWithStdin helper directly

Every existing test stubs cmdExecWithStdin, so the helper body itself
never ran under coverage and coveralls flagged a drop. Add a direct
test mirroring TestExecCmd: stdin reaches the child's stdout, a
missing binary errors with "failed to find", and a nonzero exit
errors with "failed to execute cmd".

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01P6DEdUpFBFaAiPKEu81Wti

* Log in through one helper: no "" registry, "--" before it, no password in the debug log

From a code review of this PR, which already keeps the password out of a
shell string (and so out of the process list, where `bash -c` put it):

- The three CLI login sites each built their own argv. They now share
  cliRegistryLogin: the pas... (continued)

Coverage Stats

28 of 29 new or added lines in 2 files covered. (96.55%)

2 existing lines in 1 file now uncovered.

27275 of 60405 relevant lines covered (45.15%)

9.85 hits per line