quote remote job paths in posix ssh command builders - #70091
Conversation
potiuk
left a comment
There was a problem hiding this comment.
Thanks — this is a proper hardening fix and the test is the best part of it.
The bug is real: job_dir='{paths.job_dir}' closes and reopens on any single quote in the path, so a remote_base_dir containing ' stops being data and becomes shell syntax. shlex.quote() is the correct tool, applied consistently across all six POSIX builders.
The test actually proves it. Rather than asserting on generated string content, TestPosixPathQuoting builds a real payload — /tmp/airflow-ssh-jobs/x'; touch {marker}; :' — executes the generated command through sh -c, and asserts the marker file was never created, parametrised across all six builders. That's an executable proof of non-injection rather than a proxy for one, and it would catch a future builder that forgets to quote. Updating cat '/tmp/pid' -> cat /tmp/pid in the existing assertion is right too, since shlex.quote leaves simple paths bare.
Two notes, neither blocking:
The Windows builders are untouched — build_windows_log_tail_command, build_windows_file_size_command, build_windows_completion_check_command and build_windows_cleanup_command still interpolate directly. That matches your title's scoping, and shlex.quote would be actively wrong there (it emits POSIX quoting, not cmd.exe/PowerShell), so this isn't something to bolt on here. Worth a follow-up issue so the asymmetry is recorded rather than forgotten.
Framing: under Airflow's security model the Dag author is a trusted party who can already execute arbitrary code, so remote_base_dir isn't an untrusted input boundary — this is a robustness/correctness fix, not a vulnerability. Worth saying so in the description so nobody routes it through the security process.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
The POSIX builders in
remote_job.pyinterpolateRemoteJobPathsvalues into single-quoted shell words without escaping, so a single quote anywhere inremote_base_dircloses the quote and the rest of the value is parsed as commands that run on the remote host as the SSH connection user.remote_base_diris a template field, but_validate_base_dirruns in__init__against the un-rendered Jinja literal and never looks for shell metacharacters, so a value like{{ dag_run.conf['dir'] }}reaches the shell unchecked and someone who can only trigger a Dag run with config gets a shell on the target host. Quoting withshlex.quoteat each site matches what the Windows builders already do viaps_escape.All six POSIX builders are affected. The
_validate_job_dirprefix check on cleanup does not help, since a base dir that starts with/tmp/airflow-ssh-jobs/and then contains a quote still passes it:Added a parametrized regression that runs each builder's output through
shand asserts the injected marker is never created; it fails on all six before this change.Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 4.8) following the guidelines