diff --git a/README.md b/README.md index f5d8eea..3abf896 100644 --- a/README.md +++ b/README.md @@ -42,6 +42,10 @@ The global mode sets `git config --global core.hooksPath ~/.config/git/hooks`. S its own local `core.hooksPath` overrides the global hook — install per-repo there. See memory `reference_global_security_review_hook`. +If `review.sh` is missing or not executable at the configured path, the pre-push hook **fails closed** +(nonzero exit) instead of skipping the gate. Repair by setting `SH_REVIEW_SH` to the scanner path, or +reinstall with `./install-hooks.sh --global`. + - `--global` also creates the machine-level suppressions dir (`${SH_SECURITY_SUPPRESSIONS_DIR:-~/.config/sea-haven/security-review}`): the per-repo file `//suppressions.json` is preferred by the hooks over repo-local diff --git a/hooks/pre-push b/hooks/pre-push index b638ccf..aec7e96 100755 --- a/hooks/pre-push +++ b/hooks/pre-push @@ -8,26 +8,28 @@ set -uo pipefail REPO_ROOT="$(git rev-parse --show-toplevel 2>/dev/null)" || exit 0 [ -f "$REPO_ROOT/.security-review-skip" ] && exit 0 REVIEW_SH="${SH_REVIEW_SH:-$HOME/Documents/repositories/seahaven/security-review/review.sh}" -if [ -f "$REVIEW_SH" ]; then - SUP=() - # Suppressions: prefer a MACHINE-LEVEL file kept out of repo history - # (//suppressions.json), else fall back to a repo-local - # .security-review/suppressions.json. Keyed by repo basename — adequate for the - # current single-namespace layout under ~/Documents/repositories. - MACHINE_SUP="${SH_SECURITY_SUPPRESSIONS_DIR:-$HOME/.config/sea-haven/security-review}/$(basename "$REPO_ROOT")/suppressions.json" - if [ -f "$MACHINE_SUP" ]; then - SUP=(--suppressions "$MACHINE_SUP") - elif [ -f "$REPO_ROOT/.security-review/suppressions.json" ]; then - SUP=(--suppressions "$REPO_ROOT/.security-review/suppressions.json") - fi - echo "security-review: scanning $REPO_ROOT (scanners-only) before push..." >&2 - # ${SUP[@]+"${SUP[@]}"} = bash-3.2-safe expansion of a possibly-empty array under set -u. - if ! bash "$REVIEW_SH" --scanners-only ${SUP[@]+"${SUP[@]}"} "$REPO_ROOT"; then - echo "security-review: BLOCKED (confirmed crit/high). Fix it, suppress with justification, or 'git push --no-verify' to override." >&2 - exit 1 - fi -else - echo "security-review: review.sh not found at $REVIEW_SH (set SH_REVIEW_SH) — skipping gate" >&2 +if [ ! -x "$REVIEW_SH" ]; then + echo "security-review: review.sh is missing or not executable at $REVIEW_SH" >&2 + echo "Repair: set SH_REVIEW_SH to the scanner path, or reinstall with:" >&2 + echo " ${HOME}/Documents/repositories/seahaven/security-review/install-hooks.sh --global" >&2 + exit 1 +fi +SUP=() +# Suppressions: prefer a MACHINE-LEVEL file kept out of repo history +# (//suppressions.json), else fall back to a repo-local +# .security-review/suppressions.json. Keyed by repo basename — adequate for the +# current single-namespace layout under ~/Documents/repositories. +MACHINE_SUP="${SH_SECURITY_SUPPRESSIONS_DIR:-$HOME/.config/sea-haven/security-review}/$(basename "$REPO_ROOT")/suppressions.json" +if [ -f "$MACHINE_SUP" ]; then + SUP=(--suppressions "$MACHINE_SUP") +elif [ -f "$REPO_ROOT/.security-review/suppressions.json" ]; then + SUP=(--suppressions "$REPO_ROOT/.security-review/suppressions.json") +fi +echo "security-review: scanning $REPO_ROOT (scanners-only) before push..." >&2 +# ${SUP[@]+"${SUP[@]}"} = bash-3.2-safe expansion of a possibly-empty array under set -u. +if ! bash "$REVIEW_SH" --scanners-only ${SUP[@]+"${SUP[@]}"} "$REPO_ROOT"; then + echo "security-review: BLOCKED (confirmed crit/high). Fix it, suppress with justification, or 'git push --no-verify' to override." >&2 + exit 1 fi # Don't silently disable a repo-local pre-push hook: chain to it if present. LOCAL_HOOK="$REPO_ROOT/.git/hooks/pre-push" diff --git a/install-hooks.sh b/install-hooks.sh index 42ed933..5766903 100755 --- a/install-hooks.sh +++ b/install-hooks.sh @@ -64,6 +64,7 @@ case "${1:-}" in echo echo "Done. Every repo on this machine is now gated by review.sh --scanners-only before push." + echo "A missing or non-executable review.sh fails the push closed; repair with this installer or SH_REVIEW_SH." echo "Caveats: a repo that sets its OWN local core.hooksPath (e.g. husky) overrides this global hook" echo " — run 'install-hooks.sh ' to gate it per-repo. Skip a repo with a" echo " .security-review-skip file at its root; bypass once with 'git push --no-verify'." diff --git a/tests/test_pre_push_hook.py b/tests/test_pre_push_hook.py new file mode 100644 index 0000000..e3a23d2 --- /dev/null +++ b/tests/test_pre_push_hook.py @@ -0,0 +1,99 @@ +"""Regression tests for the global pre-push security hook fail-closed behavior.""" + +from __future__ import annotations + +import os +import subprocess +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[1] +PRE_PUSH = REPO_ROOT / "hooks" / "pre-push" + + +@pytest.fixture +def temp_git_repo(tmp_path: Path) -> Path: + """Minimal git repo so the hook's git rev-parse succeeds.""" + repo = tmp_path / "repo" + repo.mkdir() + subprocess.run(["git", "init"], cwd=repo, check=True, capture_output=True) + subprocess.run( + ["git", "config", "user.email", "test@example.com"], + cwd=repo, + check=True, + capture_output=True, + ) + subprocess.run( + ["git", "config", "user.name", "Test"], + cwd=repo, + check=True, + capture_output=True, + ) + (repo / "README").write_text("x\n", encoding="utf-8") + subprocess.run(["git", "add", "README"], cwd=repo, check=True, capture_output=True) + subprocess.run( + ["git", "commit", "-m", "init"], + cwd=repo, + check=True, + capture_output=True, + ) + return repo + + +def _run_hook( + repo: Path, review_sh: str | Path, *, env_extra: dict[str, str] | None = None +) -> subprocess.CompletedProcess[str]: + env = os.environ.copy() + env["SH_REVIEW_SH"] = str(review_sh) + if env_extra: + env.update(env_extra) + return subprocess.run( + ["bash", str(PRE_PUSH)], + cwd=repo, + env=env, + capture_output=True, + text=True, + check=False, + ) + + +def test_missing_scanner_fails_closed(temp_git_repo: Path, tmp_path: Path) -> None: + missing = tmp_path / "no-such-review.sh" + result = _run_hook(temp_git_repo, missing) + assert result.returncode == 1 + assert str(missing) in result.stderr + assert "missing or not executable" in result.stderr + assert "install-hooks.sh --global" in result.stderr + + +def test_non_executable_scanner_fails_closed( + temp_git_repo: Path, tmp_path: Path +) -> None: + stub = tmp_path / "review.sh" + stub.write_text("#!/usr/bin/env bash\nexit 0\n", encoding="utf-8") + stub.chmod(0o644) + result = _run_hook(temp_git_repo, stub) + assert result.returncode == 1 + assert str(stub) in result.stderr + assert "missing or not executable" in result.stderr + assert "install-hooks.sh --global" in result.stderr + + +def test_scanner_success_allows_push(temp_git_repo: Path, tmp_path: Path) -> None: + stub = tmp_path / "review.sh" + stub.write_text("#!/usr/bin/env bash\nexit 0\n", encoding="utf-8") + stub.chmod(0o755) + result = _run_hook(temp_git_repo, stub) + assert result.returncode == 0 + assert "BLOCKED" not in result.stderr + assert "missing or not executable" not in result.stderr + + +def test_scanner_block_blocks_push(temp_git_repo: Path, tmp_path: Path) -> None: + stub = tmp_path / "review.sh" + stub.write_text("#!/usr/bin/env bash\nexit 1\n", encoding="utf-8") + stub.chmod(0o755) + result = _run_hook(temp_git_repo, stub) + assert result.returncode == 1 + assert "BLOCKED" in result.stderr