fix(core,ci): harden git diff validation, make segment digests canonical, and align CI policy

orenlab · orenlab · commit f9be59124c94 · 2026-04-08T22:22:51.000+05:00
diff --git a/.github/workflows/codeclone.yml b/.github/workflows/codeclone.yml
@@ -19,7 +19,7 @@ jobs:
     runs-on: ubuntu-latest
     steps:
       - name: Checkout
-        uses: actions/checkout@v4
+        uses: actions/checkout@v6.0.2
         with:
           fetch-depth: 0
 
diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml
@@ -40,7 +40,7 @@ jobs:
       - name: Run tests
         # Smoke CLI tests intentionally disable subprocess coverage collection
         # to avoid runner-specific flakiness while keeping parent-process coverage strict.
-        run: uv run pytest --cov=codeclone --cov-report=term-missing --cov-fail-under=98
+        run: uv run pytest --cov=codeclone --cov-report=term-missing --cov-fail-under=99
 
       - name: Verify baseline exists
         if: ${{ matrix.python-version == '3.13' }}
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -16,6 +16,12 @@
 - Add `workspace_root` user-config field to the Claude Desktop bundle: setting it to the project directory forces the
   launcher to prefer `.venv` inside that path even when Claude Desktop starts with a different working directory
   (fixes python-tag mismatch caused by system-wide interpreter fallback).
+- Validate `git_diff_ref` inputs as safe single revision expressions in both
+  CLI and MCP before invoking `git diff`.
+- Replace the segment-group raw digest `repr()` payload with canonical JSON
+  bytes for cross-version-safe determinism.
+- Align the tests workflow coverage gate with the canonical `fail_under = 99`
+  policy and refresh the remaining `actions/checkout` pin in `codeclone.yml`.
 - Refresh branch metadata and client docs for the `2.0.0b5` line.
 - Update the README repository health badge to `87 (B)`.
 
diff --git a/codeclone/_git_diff.py b/codeclone/_git_diff.py
@@ -0,0 +1,44 @@
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at https://mozilla.org/MPL/2.0/.
+# SPDX-License-Identifier: MPL-2.0
+# Copyright (c) 2026 Den Rozhnovskiy
+
+from __future__ import annotations
+
+import re
+from typing import Final
+
+_SAFE_GIT_DIFF_REF_RE: Final[re.Pattern[str]] = re.compile(
+    r"^(?![-./])[A-Za-z0-9._/@{}^~+-]+$"
+)
+
+
+def validate_git_diff_ref(git_diff_ref: str) -> str:
+    """Validate a safe, single git revision expression for `git diff`.
+
+    CodeClone intentionally accepts a conservative subset of git revision
+    syntax here: common branch names, tags, revision operators (`~`, `^`),
+    reflog selectors (`@{...}`), and dotted range expressions. Whitespace,
+    control characters, option-like prefixes, and unsupported punctuation are
+    rejected before any subprocess call.
+    """
+
+    if git_diff_ref != git_diff_ref.strip():
+        raise ValueError(
+            "Invalid git diff ref "
+            f"{git_diff_ref!r}: surrounding whitespace is not allowed."
+        )
+    if not git_diff_ref:
+        raise ValueError("Invalid git diff ref '': value must not be empty.")
+    if any(ch.isspace() or ord(ch) < 32 or ord(ch) == 127 for ch in git_diff_ref):
+        raise ValueError(
+            "Invalid git diff ref "
+            f"{git_diff_ref!r}: whitespace and control characters are not allowed."
+        )
+    if not _SAFE_GIT_DIFF_REF_RE.fullmatch(git_diff_ref):
+        raise ValueError(
+            "Invalid git diff ref "
+            f"{git_diff_ref!r}: expected a safe revision expression."
+        )
+    return git_diff_ref
diff --git a/codeclone/cli.py b/codeclone/cli.py
@@ -92,6 +92,7 @@
     _print_metrics,
     _print_summary,
 )
+from ._git_diff import validate_git_diff_ref
 from .baseline import Baseline
 from .cache import Cache, CacheStatus, build_segment_report_projection
 from .contracts import (
@@ -270,16 +271,14 @@ def _normalize_changed_paths(
 
 
 def _git_diff_changed_paths(*, root_path: Path, git_diff_ref: str) -> tuple[str, ...]:
-    if git_diff_ref.startswith("-"):
-        console.print(
-            ui.fmt_contract_error(
-                f"Invalid git diff ref '{git_diff_ref}': must not start with '-'."
-            )
-        )
+    try:
+        validated_ref = validate_git_diff_ref(git_diff_ref)
+    except ValueError as exc:
+        console.print(ui.fmt_contract_error(str(exc)))
         sys.exit(ExitCode.CONTRACT_ERROR)
     try:
         completed = subprocess.run(
-            ["git", "diff", "--name-only", git_diff_ref, "--"],
+            ["git", "diff", "--name-only", validated_ref, "--"],
             cwd=str(root_path),
             check=True,
             capture_output=True,
@@ -294,7 +293,7 @@ def _git_diff_changed_paths(*, root_path: Path, git_diff_ref: str) -> tuple[str,
         console.print(
             ui.fmt_contract_error(
                 "Unable to resolve changed files from git diff ref "
-                f"'{git_diff_ref}': {exc}"
+                f"'{validated_ref}': {exc}"
             )
         )
         sys.exit(ExitCode.CONTRACT_ERROR)
diff --git a/codeclone/mcp_service.py b/codeclone/mcp_service.py
@@ -47,6 +47,7 @@
 )
 from ._coerce import as_float as _as_float
 from ._coerce import as_int as _as_int
+from ._git_diff import validate_git_diff_ref
 from .baseline import Baseline
 from .cache import Cache, CacheStatus
 from .contracts import (
@@ -793,13 +794,13 @@ def _git_diff_lines_payload(
     root_path: Path,
     git_diff_ref: str,
 ) -> tuple[str, ...]:
-    if git_diff_ref.startswith("-"):
-        raise MCPGitDiffError(
-            f"Invalid git diff ref '{git_diff_ref}': must not start with '-'."
-        )
+    try:
+        validated_ref = validate_git_diff_ref(git_diff_ref)
+    except ValueError as exc:
+        raise MCPGitDiffError(str(exc)) from exc
     try:
         completed = subprocess.run(
-            ["git", "diff", "--name-only", git_diff_ref, "--"],
+            ["git", "diff", "--name-only", validated_ref, "--"],
             cwd=root_path,
             check=True,
             capture_output=True,
@@ -808,7 +809,7 @@ def _git_diff_lines_payload(
         )
     except (OSError, subprocess.CalledProcessError, subprocess.TimeoutExpired) as exc:
         raise MCPGitDiffError(
-            f"Unable to resolve changed paths from git diff ref '{git_diff_ref}'."
+            f"Unable to resolve changed paths from git diff ref '{validated_ref}'."
         ) from exc
     return tuple(
         sorted({line.strip() for line in completed.stdout.splitlines() if line.strip()})
diff --git a/codeclone/pipeline.py b/codeclone/pipeline.py
@@ -13,6 +13,8 @@
 from pathlib import Path
 from typing import TYPE_CHECKING, Literal, cast
 
+import orjson
+
 from ._coerce import as_int, as_str
 from .cache import (
     Cache,
@@ -257,7 +259,7 @@ def _segment_groups_digest(segment_groups: GroupMap) -> str:
             for item in items
         ]
         normalized_rows.append((group_key, tuple(normalized_items)))
-    payload = repr(tuple(normalized_rows)).encode("utf-8")
+    payload = orjson.dumps(tuple(normalized_rows), option=orjson.OPT_SORT_KEYS)
     return sha256(payload).hexdigest()
 
 
diff --git a/docs/architecture.md b/docs/architecture.md
@@ -224,7 +224,8 @@ Security boundaries:
 - `cache_policy=refresh` rejected to preserve read-only semantics.
 - Review markers are session-local in-memory state, never persisted.
 - Run history bounded by `--history-limit` to prevent unbounded memory growth.
-- `git_diff_ref` validated against strict regex to prevent injection.
+- `git_diff_ref` validated as a safe single revision expression before any
+  `git diff` subprocess call.
 
 ---
 
diff --git a/docs/book/09-cli.md b/docs/book/09-cli.md
@@ -75,6 +75,8 @@ Refs:
 - `--changed-only` requires either `--diff-against` or `--paths-from-git-diff`.
 - `--diff-against` requires `--changed-only`.
 - `--diff-against` and `--paths-from-git-diff` are mutually exclusive.
+- Git diff refs are validated as safe single revision expressions before
+  subprocess execution.
 - Browser-open failure after a successful HTML write is warning-only and does not change the process exit code.
 - Baseline update write failure is contract error.
 - In gating mode, unreadable source files are contract errors with higher priority than clone gating failure.
diff --git a/docs/book/11-security-model.md b/docs/book/11-security-model.md
@@ -33,8 +33,9 @@ Security-relevant input classes:
 - `cache_policy=refresh` is rejected — MCP cannot trigger cache invalidation.
 - Review markers (`mark_finding_reviewed`) are session-local in-memory state;
   they are never persisted to disk or leaked into baselines/reports.
-- `git_diff_ref` parameter is validated against a strict regex to prevent
-  command injection via shell-interpreted git arguments.
+- `git_diff_ref` is validated as a safe single revision expression before any
+  `git diff` subprocess call. Leading option-like prefixes, whitespace/control
+  characters, and unsupported punctuation are rejected.
 - Run history is bounded by `--history-limit` (default 10) to prevent
   unbounded memory growth.
 
@@ -68,7 +69,7 @@ Refs:
 | HTML-injected payload in metadata/source | Escaped output     |
 | `--allow-remote` not passed for HTTP     | Transport rejected |
 | `cache_policy=refresh` requested         | Policy rejected    |
-| `git_diff_ref` fails regex               | Parameter rejected |
+| `git_diff_ref` fails validation          | Parameter rejected |
 
 ## Determinism / canonicalization
 
diff --git a/docs/book/20-mcp-interface.md b/docs/book/20-mcp-interface.md
@@ -228,6 +228,8 @@ state behind `codeclone://latest/...`.
 - `analyze_changed_paths` requires `changed_paths` or `git_diff_ref`.
 - `analyze_repository` and `analyze_changed_paths` require an absolute `root`;
   relative roots like `.` are rejected.
+- `git_diff_ref` is validated as a safe single revision expression before
+  invoking `git diff`.
 - `changed_paths` is a structured `list[str]` of repo-relative paths, not a
   comma-separated string payload.
 - `analyze_changed_paths` may return the same `run_id` as a previous run when
diff --git a/docs/mcp.md b/docs/mcp.md
@@ -236,6 +236,8 @@ Separate accepted baseline debt from new regressions.
 - Prefer `list_hotspots` or narrow `check_*` tools before broad `list_findings`.
 - Use `get_finding` / `get_remediation` for one finding instead of raising
   `detail_level` on larger lists.
+- Keep `git_diff_ref` to a safe single revision expression; option-like,
+  whitespace-containing, and punctuated shell-style inputs are rejected.
 - Pass an absolute `root` — MCP rejects relative roots like `.`.
 - Use `"production-only"` / `source_kind` filters to cut test/fixture noise.
 - Use `mark_finding_reviewed` + `exclude_reviewed=true` in long sessions.
diff --git a/tests/test_cli_unit.py b/tests/test_cli_unit.py
@@ -498,6 +498,29 @@ def test_git_diff_changed_paths_rejects_option_like_ref(tmp_path: Path) -> None:
     assert exc.value.code == 2
 
 
+@pytest.mark.parametrize(
+    "git_diff_ref",
+    [
+        "HEAD~1;rm",
+        "HEAD path",
+        "HEAD:path",
+        "../HEAD",
+        " HEAD",
+    ],
+)
+def test_git_diff_changed_paths_rejects_unsafe_ref_syntax(
+    tmp_path: Path,
+    git_diff_ref: str,
+) -> None:
+    cli.console = cli._make_console(no_color=True)
+    with pytest.raises(SystemExit) as exc:
+        cli._git_diff_changed_paths(
+            root_path=tmp_path.resolve(),
+            git_diff_ref=git_diff_ref,
+        )
+    assert exc.value.code == 2
+
+
 def test_report_path_origins_ignores_unrelated_equals_tokens() -> None:
     assert cli._report_path_origins(("--unknown=value", "--json=out.json")) == {
         "html": None,
diff --git a/tests/test_core_branch_coverage.py b/tests/test_core_branch_coverage.py
@@ -7,9 +7,11 @@
 from __future__ import annotations
 
 from argparse import Namespace
+from hashlib import sha256
 from pathlib import Path
 from typing import cast
 
+import orjson
 import pytest
 
 import codeclone.cli as cli
@@ -501,6 +503,20 @@ def test_pipeline_analyze_uses_cached_segment_projection(
         },
     }
 
+    expected_payload = orjson.dumps(
+        (
+            (
+                "seg-hash|pkg.a:f",
+                (
+                    ("/tmp/a.py", "pkg.a:f", 10, 15, 6, "seg-hash", "seg-sig"),
+                    ("/tmp/a.py", "pkg.a:f", 20, 25, 6, "seg-hash", "seg-sig"),
+                ),
+            ),
+        ),
+        option=orjson.OPT_SORT_KEYS,
+    )
+    assert digest == sha256(expected_payload).hexdigest()
+
     def _must_not_run(
         _segment_groups: object,
     ) -> tuple[dict[str, list[dict[str, object]]], int]:
diff --git a/tests/test_mcp_service.py b/tests/test_mcp_service.py
@@ -1415,12 +1415,18 @@ def test_mcp_service_git_diff_and_helper_branch_edges(
 ) -> None:
     service = CodeCloneMCPService(history_limit=4)
 
-    with pytest.raises(MCPGitDiffError, match="must not start with '-'"):
+    with pytest.raises(MCPGitDiffError, match="Invalid git diff ref"):
         mcp_service_mod._git_diff_lines_payload(
             root_path=tmp_path,
             git_diff_ref="--cached",
         )
 
+    with pytest.raises(MCPGitDiffError, match="safe revision expression"):
+        mcp_service_mod._git_diff_lines_payload(
+            root_path=tmp_path,
+            git_diff_ref="HEAD:path",
+        )
+
     assert service._normalize_relative_path("./.github/workflows/docs.yml") == (
         ".github/workflows/docs.yml"
     )
