From 63bb7dd9d3d3a86b14da7b5bc15c4e7bf8634f53 Mon Sep 17 00:00:00 2001 From: Qing Wang Date: Wed, 15 Jul 2026 00:30:08 +0000 Subject: [PATCH] Validate the SDK release version and stop interpolating it into shell The version handed to the release flow was written verbatim into two source files as a string literal and spliced into a bash command, with no validation anywhere in between. Two separate injection shapes: 1. scripts/update_version.py took the version from argv unchecked and f-stringed it into re.sub *replacement* strings that rewrite pyproject.toml and src/claude_agent_sdk/_version.py. A string replacement also undergoes backslash-escape processing, so a "\1" or "\g<0>" in the value is expanded against the match; and a value containing a double quote escapes the literal it is written into, turning _version.py into a source file that executes on import. 2. .github/workflows/build-and-publish.yml spliced ${{ inputs.version }} directly into the command text of a run: block, so a crafted workflow input became shell. Fixes: - Validate the version against an allowlist before it is used anywhere. Every version this project has released -- all 121 git tags, and all 121 releases on PyPI -- is a plain MAJOR.MINOR.PATCH; the pattern also admits the PEP 440 pre/post/dev suffixes PyPI would accept, so a future 0.3.0rc1 is not rejected on release day. The admissible character set is exactly [0-9a-z.]: no quote, backslash, whitespace, or metacharacter. Rejection is a clean non-zero exit with a one-line stderr message and no traceback, and leaves both target files untouched. - Substitute with a *callable* replacement rather than a string, so re.sub inserts the value literally with no escape pass, and emit the literal with json.dumps(). Both substitutions are computed before either file is written, so a missing anchor cannot leave the first file half-rewritten; a missing anchor now raises instead of silently reporting success. - Pass the version to the build step through env: and reference it as "$VERSION", so bash expands it as a single word instead of GitHub splicing it into the command text. Adds tests/test_update_version.py: quote-breakout, metacharacter, control character and whitespace rejection; backslash/backreference payloads pinning that the callable replacement is literal (including a test that demonstrates the old string-replacement form expanding "\g<0>"); that no file is modified on rejection; and that every version the project has actually released still validates, read from the real git tag list. :house: Remote-Dev: homespace --- .github/workflows/build-and-publish.yml | 8 +- scripts/update_version.py | 168 ++++++++++-- tests/test_update_version.py | 351 ++++++++++++++++++++++++ 3 files changed, 500 insertions(+), 27 deletions(-) create mode 100644 tests/test_update_version.py diff --git a/.github/workflows/build-and-publish.yml b/.github/workflows/build-and-publish.yml index d1c102d03..9dff889cc 100644 --- a/.github/workflows/build-and-publish.yml +++ b/.github/workflows/build-and-publish.yml @@ -31,7 +31,13 @@ jobs: shell: bash - name: Build wheel with bundled CLI - run: python scripts/build_wheel.py --version "${{ inputs.version }}" --skip-sdist --clean + # Passed through the environment rather than interpolated into the + # command text: a "${{ }}" expansion is spliced in before bash ever + # sees the line, so a crafted `version` input would become shell. + # "$VERSION" is expanded by bash, as one word, and cannot be. + env: + VERSION: ${{ inputs.version }} + run: python scripts/build_wheel.py --version "$VERSION" --skip-sdist --clean shell: bash - uses: actions/upload-artifact@v4 diff --git a/scripts/update_version.py b/scripts/update_version.py index b980d5224..f26ae19a2 100755 --- a/scripts/update_version.py +++ b/scripts/update_version.py @@ -1,49 +1,165 @@ #!/usr/bin/env python3 -"""Update version in pyproject.toml and __init__.py files.""" +r"""Update the SDK version in pyproject.toml and src/claude_agent_sdk/_version.py. +This script is the release flow's writer: the version it is handed (ultimately +from a workflow_dispatch input, see .github/workflows/build-and-publish.yml) +is written verbatim into two source files as a *string literal* -- a TOML one +in pyproject.toml and a Python one in _version.py. That makes the value code, +not data, so it is validated against an allowlist before it is used anywhere, +and substituted with a callable replacement so it can never be reinterpreted. + +Two distinct hazards, both real, both closed here: + + 1. **Quote breakout.** An unvalidated value written as ``version = "{v}"`` + escapes its literal the moment it contains a double quote: a ``v`` of + ``0.0.0"\nimport os; os.system("...")\n#`` turns _version.py into a + source file that executes on import. VERSION_PATTERN admits no quote, no + backslash, and no newline; json.dumps() then emits the literal, so the + quoting is done by a serializer rather than by an f-string. + + 2. **Backslash expansion in the replacement.** ``re.sub(pat, repl, s)`` with + a *string* ``repl`` does not insert it literally -- it runs it through + escape processing first, so a ``\1`` or ``\g<0>`` in the version is + expanded against the match, and a lone ``\`` raises. The replacements + below are therefore **callables**: re.sub() inserts a callable's return + value literally, with no escape pass. This is belt-and-braces given the + pattern already excludes backslashes, and it is the property the tests + pin. + +The allowlist is deliberately narrow. Every version this project has ever +released -- all 121 git tags, and all 121 releases on PyPI -- is a plain +``MAJOR.MINOR.PATCH`` with no suffix. The pattern additionally admits the +PEP 440 pre-release / post-release / dev-release suffixes that PyPI would +accept for a *future* ``0.3.0rc1``-style release, so release day is not the +day someone discovers this script rejects a legitimate version. Every +character it can admit is drawn from ``[0-9a-z.]``: there is no metacharacter, +quote, backslash, or space in the admissible set, which is what makes both +hazards above unreachable rather than merely unlikely. + +Note: scripts/_cli_version_validation.py (added by #1117) validates the +bundled *CLI* version and enforces the same two properties for the same +reasons. The two allowlists differ -- a CLI version has a ``-dev.…`` suffix +and dist-tags, an SDK version has PEP 440 suffixes and must satisfy PyPI -- +but the validate-then-substitute-with-a-callable shape is common to both, and +they should be unified behind one shared validator once #1117 lands. +""" + +import json import re import sys from pathlib import Path +# MAJOR.MINOR.PATCH, plus the optional PEP 440 suffixes PyPI would accept. +# Admissible characters are exactly [0-9a-z.] -- see the module docstring for +# why that, and not the numeric arity, is the security boundary. Never widen +# this to admit a quote, a backslash, whitespace, or a shell metacharacter. +# +# Deliberately unanchored, and matched with fullmatch() rather than match(): +# with "^...$" a later swap to match() would silently accept a trailing +# newline ("0.2.119\n"); unanchored, the same swap accepts an obvious prefix +# like "0.2.119; id" and fails loudly in the tests. +VERSION_PATTERN = re.compile( + r"[0-9]+\.[0-9]+\.[0-9]+" # 0.2.119 -- every version ever released + r"(?:(?:a|b|rc)[0-9]+)?" # 0.3.0rc1 (PEP 440 pre-release) + r"(?:\.post[0-9]+)?" # 0.3.0.post1 (PEP 440 post-release) + r"(?:\.dev[0-9]+)?" # 0.3.0.dev1 (PEP 440 dev release) +) -def update_version(new_version: str) -> None: - """Update version in project files.""" - # Update pyproject.toml - pyproject_path = Path("pyproject.toml") - content = pyproject_path.read_text() - # Only update the version field in [project] section - content = re.sub( - r'^version = "[^"]*"', - f'version = "{new_version}"', +def validate_version(version: str, *, source: str = "version") -> str: + """Return ``version`` if it is a version this project could publish, else raise. + + Surrounding whitespace is stripped first: a trailing "\\n" from a file read + or a stray space from YAML is unambiguous in intent. Interior whitespace is + not stripped and is rejected by the pattern. + + Args: + version: The candidate version string. + source: Where the value came from, used in the error message. + + Returns: + The stripped, validated version. + + Raises: + ValueError: If ``version`` is not a fullmatch of VERSION_PATTERN. + """ + candidate = version.strip() + + if VERSION_PATTERN.fullmatch(candidate): + return candidate + + # "v0.2.119" is the single most likely typo -- the tags carry a leading + # "v" but the version does not. Say so, rather than printing a regex and + # leaving the reader to spot it. Not normalized away: publishing a + # different string than the caller asked for is worse than failing. + if candidate[:1] in ("v", "V") and VERSION_PATTERN.fullmatch(candidate[1:]): + raise ValueError( + f"Invalid {source}: {version!r}. " + f"Did you mean {candidate[1:]!r}? (no leading 'v')" + ) + + raise ValueError( + f"Invalid {source}: {version!r}. " + f"Expected a version matching {VERSION_PATTERN.pattern}" + ) + + +def _substitute(path: Path, pattern: str, assignment: str, version: str) -> str: + """Return ``path``'s text with ``assignment``'s version literal set to ``version``. + + The replacement is a **callable**, so re.sub() inserts its return value + literally instead of running it through backslash-escape processing -- a + "\\1" or "\\g<0>" in ``version`` stays those four characters. json.dumps() + emits the quoted literal, which is valid in both TOML and Python. + + Raises: + ValueError: If ``pattern`` does not match, which would otherwise write + the file back unchanged and report success. + """ + content = path.read_text() + literal = f"{assignment} = {json.dumps(version)}" + new_content, count = re.subn( + pattern, + lambda _match: literal, content, count=1, flags=re.MULTILINE, ) + if count != 1: + raise ValueError(f"Could not find a {assignment!r} assignment in {path}") + return new_content + - pyproject_path.write_text(content) - print(f"Updated pyproject.toml to version {new_version}") +def update_version(new_version: str) -> None: + """Validate ``new_version`` and write it into pyproject.toml and _version.py.""" + version = validate_version(new_version) - # Update _version.py + pyproject_path = Path("pyproject.toml") version_path = Path("src/claude_agent_sdk/_version.py") - content = version_path.read_text() - # Only update __version__ assignment - content = re.sub( - r'^__version__ = "[^"]*"', - f'__version__ = "{new_version}"', - content, - count=1, - flags=re.MULTILINE, + # Compute both substitutions before writing either, so a missing anchor in + # the second file cannot leave the first one already rewritten. + pyproject_content = _substitute( + pyproject_path, r'^version = "[^"]*"', "version", version + ) + version_content = _substitute( + version_path, r'^__version__ = "[^"]*"', "__version__", version ) - version_path.write_text(content) - print(f"Updated _version.py to version {new_version}") + pyproject_path.write_text(pyproject_content) + print(f"Updated pyproject.toml to version {version}") + + version_path.write_text(version_content) + print(f"Updated _version.py to version {version}") if __name__ == "__main__": if len(sys.argv) != 2: - print("Usage: python scripts/update_version.py ") - sys.exit(1) + sys.exit("Usage: python scripts/update_version.py ") - update_version(sys.argv[1]) + try: + update_version(sys.argv[1]) + except ValueError as exc: + # A clean one-line message, not a traceback: the caller is a release + # workflow and the reader is whoever typed the bad version. + sys.exit(str(exc)) diff --git a/tests/test_update_version.py b/tests/test_update_version.py new file mode 100644 index 000000000..6939ddb18 --- /dev/null +++ b/tests/test_update_version.py @@ -0,0 +1,351 @@ +"""Tests for scripts/update_version.py version validation and substitution. + +The version handed to this script is written into two source files as a string +literal -- a TOML one in pyproject.toml, a Python one in _version.py -- so it +is code, not data. These tests pin the two properties that keep it from being +reinterpreted as such: + + * the allowlist admits no quote, backslash, newline, or shell metacharacter, + so a value cannot break out of the literal it is written into; and + * the re.sub() replacement is a *callable*, so a backslash escape in the + value ("\\1", "\\g<0>") is inserted literally rather than expanded against + the match. + +They also pin the converse, which is the one that bites on release day: every +version this project has actually released must still validate. +""" + +import importlib.util +import json +import re +import subprocess +import sys +from pathlib import Path + +import pytest + +_REPO_ROOT = Path(__file__).parent.parent +_SCRIPT = _REPO_ROOT / "scripts" / "update_version.py" + +# scripts/ is not a package, so load update_version.py by path (same approach +# as tests/test_build_wheel.py). +_spec = importlib.util.spec_from_file_location("update_version", _SCRIPT) +assert _spec is not None and _spec.loader is not None +update_version_module = importlib.util.module_from_spec(_spec) +sys.modules["update_version"] = update_version_module +_spec.loader.exec_module(update_version_module) + +validate_version = update_version_module.validate_version +update_version = update_version_module.update_version +VERSION_PATTERN = update_version_module.VERSION_PATTERN + + +# Every version this project has ever published: `git tag` and the PyPI +# release list agree, 121 of them, all plain MAJOR.MINOR.PATCH. A sample +# spanning the whole history, plus the PEP 440 suffixes a future release could +# legitimately use. Rejecting any of these would break a real release. +REAL_VERSIONS = [ + "0.0.16", + "0.0.25", + "0.1.0", + "0.1.9", + "0.1.81", + "0.2.82", + "0.2.100", + "0.2.117", + "0.2.118", + "0.2.119", +] + +FUTURE_VALID_VERSIONS = [ + "1.0.0", + "10.20.30", + "0.3.0a1", + "0.3.0b2", + "0.3.0rc1", + "0.3.0.post1", + "0.3.0.dev1", + "0.3.0rc1.post2.dev3", +] + + +class TestAcceptsRealVersions: + """The allowlist must not reject a version this project could publish.""" + + @pytest.mark.parametrize("version", REAL_VERSIONS) + def test_every_released_version_validates(self, version: str) -> None: + assert validate_version(version) == version + + @pytest.mark.parametrize("version", FUTURE_VALID_VERSIONS) + def test_pep440_suffixed_versions_validate(self, version: str) -> None: + assert validate_version(version) == version + + def test_every_git_tag_validates(self) -> None: + """The real tag list, read from git -- not a hand-copied sample. + + Skipped where git history is unavailable (e.g. a shallow checkout or + an sdist), since the parametrized sample above still covers the shapes. + """ + try: + result = subprocess.run( + ["git", "tag", "--list", "v*"], + cwd=_REPO_ROOT, + capture_output=True, + text=True, + check=True, + ) + except (subprocess.CalledProcessError, FileNotFoundError): # pragma: no cover + pytest.skip("git tags unavailable") + + tags = [t.strip() for t in result.stdout.splitlines() if t.strip()] + if not tags: # pragma: no cover + pytest.skip("no tags in this checkout") + + for tag in tags: + version = tag[1:] # strip the leading "v" + assert validate_version(version) == version, f"rejected released {tag}" + + def test_surrounding_whitespace_is_stripped(self) -> None: + """A trailing newline from a file read is unambiguous in intent.""" + assert validate_version(" 0.2.119\n") == "0.2.119" + + +class TestRejectsInjection: + """The allowlist must admit nothing that could escape a string literal.""" + + @pytest.mark.parametrize( + "version", + [ + # Quote breakout -- the hazard that makes _version.py executable. + '0.2.119"', + '0.2.119" + __import__("os").system("id") + "', + '0.2.119"\nimport os\nos.system("id")\n#', + "0.2.119'", + # Backslashes: rejected by the pattern *and* neutered by the + # callable replacement (see TestCallableReplacementIsLiteral). + "0.2.119\\", + "0.2.119\\n", + # Shell metacharacters -- this value also reaches a shell in CI. + "0.2.119; id", + "0.2.119 && id", + "0.2.119 | id", + "0.2.119`id`", + "0.2.119$(id)", + "0.2.119${IFS}id", + "0.2.119 #comment", + # Control characters and interior newlines. (A *trailing* newline + # is stripped and accepted -- see test_surrounding_whitespace.) + "0.2.119\n0.0.0", + "0.2.119\r\n0.0.0", + "0.2.119\x00", + "0.2.119\x1b[31m", + "0.2.119\t0.0.0", + # Interior whitespace is not stripped, and is not a version. + "0.2. 119", + "0 .2.119", + # Not a version at all. + "", + " ", + "latest", + "stable", + "0.2", + "0.2.119.4.5", + "0.2.x", + "-0.2.119", + "../../etc/passwd", + "0.2.119/../../evil", + ], + ) + def test_rejected(self, version: str) -> None: + with pytest.raises(ValueError): + validate_version(version) + + def test_leading_v_is_rejected_with_a_useful_message(self) -> None: + """The tags carry a "v"; the version must not. Say so.""" + with pytest.raises(ValueError, match=r"no leading 'v'"): + validate_version("v0.2.119") + + def test_error_message_names_the_source(self) -> None: + with pytest.raises(ValueError, match="CUSTOM_SOURCE"): + validate_version("nope", source="CUSTOM_SOURCE") + + def test_pattern_is_fullmatched_not_prefix_matched(self) -> None: + """A valid prefix must not carry an arbitrary suffix along with it.""" + assert VERSION_PATTERN.match("0.2.119; id") is not None # prefix matches + assert VERSION_PATTERN.fullmatch("0.2.119; id") is None # but not the whole + + +@pytest.fixture +def repo(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """A minimal checkout: the two files update_version() rewrites.""" + pyproject = tmp_path / "pyproject.toml" + pyproject.write_text( + '[project]\nname = "claude-agent-sdk"\nversion = "0.2.119"\n' + '\n[tool.other]\nversion = "9.9.9"\n' # must not be touched (count=1) + ) + version_dir = tmp_path / "src" / "claude_agent_sdk" + version_dir.mkdir(parents=True) + (version_dir / "_version.py").write_text('__version__ = "0.2.119"\n') + monkeypatch.chdir(tmp_path) + return tmp_path + + +class TestUpdateVersion: + """End-to-end: what actually lands in the two files.""" + + def test_writes_both_files(self, repo: Path) -> None: + update_version("0.2.120") + assert 'version = "0.2.120"' in (repo / "pyproject.toml").read_text() + assert ( + repo / "src" / "claude_agent_sdk" / "_version.py" + ).read_text() == '__version__ = "0.2.120"\n' + + def test_only_the_first_version_field_is_rewritten(self, repo: Path) -> None: + """count=1: the [tool.other] version stays put.""" + update_version("0.2.120") + assert 'version = "9.9.9"' in (repo / "pyproject.toml").read_text() + + def test_written_version_py_is_importable_and_correct(self, repo: Path) -> None: + """The whole point: _version.py must still be a sane Python module.""" + update_version("0.3.0rc1") + namespace: dict[str, object] = {} + exec( # noqa: S102 - executing the file we just generated is the test + (repo / "src" / "claude_agent_sdk" / "_version.py").read_text(), + namespace, + ) + assert namespace["__version__"] == "0.3.0rc1" + + +class TestRejectionLeavesFilesUntouched: + """A rejected version must not modify anything.""" + + @pytest.mark.parametrize( + "version", + ['0.2.119"; import os', "0.2.119; id", "\\1", "latest", "v0.2.119"], + ) + def test_no_file_is_modified(self, repo: Path, version: str) -> None: + pyproject = repo / "pyproject.toml" + version_py = repo / "src" / "claude_agent_sdk" / "_version.py" + before = (pyproject.read_text(), version_py.read_text()) + + with pytest.raises(ValueError): + update_version(version) + + assert (pyproject.read_text(), version_py.read_text()) == before + + +class TestCallableReplacementIsLiteral: + """re.sub() with a *string* replacement runs it through backslash-escape + processing; with a callable it does not. + + The allowlist already rejects every payload below, so these are defense in + depth -- they pin the substitution layer independently, so that widening + the pattern (or reusing _substitute elsewhere) cannot silently reintroduce + backreference expansion. + """ + + @pytest.mark.parametrize( + "payload", + [ + r"\1", + r"\g<0>", + r"\g<1>", + "\\", + r"\n", + r"0.2.119\1", + ], + ) + def test_payload_is_inserted_literally(self, tmp_path: Path, payload: str) -> None: + target = tmp_path / "pyproject.toml" + target.write_text('version = "0.0.0"\n') + + result = update_version_module._substitute( + target, r'^version = "[^"]*"', "version", payload + ) + + # The value survives verbatim, JSON-quoted -- no group expansion, no + # "bad escape" error, no bare backslash mangling. + assert result == f"version = {json.dumps(payload)}\n" + assert json.loads(result.split(" = ", 1)[1].strip()) == payload + + def test_string_replacement_would_have_expanded_it(self) -> None: + """The bug this guards against, demonstrated on the old code shape. + + With a string replacement, "\\g<0>" in the version is expanded to the + whole match instead of being written out -- proof the callable is + load-bearing and not decoration. + """ + content = 'version = "0.0.0"\n' + payload = r"\g<0>" + + # The old shape: an f-string spliced into a *string* replacement. + expanded = re.sub( + r'^version = "[^"]*"', + f'version = "{payload}"', + content, + count=1, + flags=re.MULTILINE, + ) + assert expanded == 'version = "version = "0.0.0""\n' # group expanded! + assert payload not in expanded + + # The new shape keeps it literal. + kept = re.sub( + r'^version = "[^"]*"', + lambda _m: f'version = "{payload}"', + content, + count=1, + flags=re.MULTILINE, + ) + assert kept == 'version = "\\g<0>"\n' + + def test_missing_anchor_raises_instead_of_silently_succeeding( + self, tmp_path: Path + ) -> None: + target = tmp_path / "pyproject.toml" + target.write_text("name = 'no version here'\n") + + with pytest.raises(ValueError, match="Could not find"): + update_version_module._substitute( + target, r'^version = "[^"]*"', "version", "0.2.120" + ) + + +class TestCommandLine: + """The script must fail cleanly -- non-zero, one line on stderr, no traceback.""" + + def _run(self, arg: str, cwd: Path) -> subprocess.CompletedProcess[str]: + return subprocess.run( + [sys.executable, str(_SCRIPT), arg], + cwd=cwd, + capture_output=True, + text=True, + ) + + def test_rejects_injection_with_clean_exit(self, repo: Path) -> None: + result = self._run('0.2.119"; import os', repo) + + assert result.returncode != 0 + assert "Traceback" not in result.stderr + assert "Invalid version" in result.stderr + assert result.stderr.strip().count("\n") == 0 # one line + # And nothing was written. + assert 'version = "0.2.119"' in (repo / "pyproject.toml").read_text() + + def test_accepts_a_real_version(self, repo: Path) -> None: + result = self._run("0.2.120", repo) + + assert result.returncode == 0, result.stderr + assert 'version = "0.2.120"' in (repo / "pyproject.toml").read_text() + + def test_usage_error_without_traceback(self, repo: Path) -> None: + result = subprocess.run( + [sys.executable, str(_SCRIPT)], + cwd=repo, + capture_output=True, + text=True, + ) + + assert result.returncode != 0 + assert "Traceback" not in result.stderr + assert "Usage:" in result.stderr