Merge pull request #387 from GitGuardian/agateau/fix-pre-receive-empt…

…y-branch

Do not crash if pushed branch contains no new commits

ggshield/cmd/secret/scan/prepush.py

@@ -116,9 +116,10 @@ def prepush_cmd(ctx: click.Context, prepush_args: List[str]) -> int:
         return handle_exception(error, config.verbose)
 
 
-def find_branch_start(commit: str, remote: str) -> str:
+def find_branch_start(commit: str, remote: str) -> Optional[str]:
     """
-    Returns the first local-only commit of the branch
+    Returns the first local-only commit of the branch.
+    Returns None if the branch does not contain any new commit.
     """
     # List all ancestors of `commit` which are not in `remote`
     # Based on _pre_push_ns() from pre-commit
@@ -136,7 +137,10 @@ def find_branch_start(commit: str, remote: str) -> str:
         ]
     )
     ancestors = output.splitlines()
-    return ancestors[0]
+
+    if ancestors:
+        return ancestors[0]
+    return None
 
 
 def collect_from_stdin(remote_name: str) -> Tuple[str, str]:
@@ -160,6 +164,8 @@ def collect_from_stdin(remote_name: str) -> Tuple[str, str]:
 
     # Pushing to a new branch
     start_commit = find_branch_start(local_commit, remote_name)
+    if start_commit is None:
+        return local_commit, local_commit
     return (local_commit, f"{start_commit}~1")
 
 

ggshield/cmd/secret/scan/prereceive.py

@@ -72,16 +72,20 @@ def get_breakglass_option() -> bool:
     return False
 
 
-def find_branch_start(commit: str) -> str:
+def find_branch_start(commit: str) -> Optional[str]:
     """
-    Returns the first local-only commit of the branch
+    Returns the first local-only commit of the branch.
+    Returns None if the branch does not contain any new commit.
     """
     # List all ancestors of `commit` which are not in any branches
     output = git(
         ["rev-list", commit, "--topo-order", "--reverse", "--not", "--branches"]
     )
     ancestors = output.splitlines()
-    return ancestors[0]
+
+    if ancestors:
+        return ancestors[0]
+    return None
 
 
 def parse_stdin() -> Tuple[str, str]:
@@ -100,7 +104,11 @@ def parse_stdin() -> Tuple[str, str]:
     if old_commit == EMPTY_SHA:
         # Pushing to a new branch
         start_commit = find_branch_start(new_commit)
-        old_commit = f"{start_commit}~1"
+        if start_commit is None:
+            # branch does not contain any new commit
+            old_commit = new_commit
+        else:
+            old_commit = f"{start_commit}~1"
 
     return (old_commit, new_commit)
 
@@ -136,6 +144,12 @@ def prereceive_cmd(ctx: click.Context, web: bool, prereceive_args: List[str]) ->
         return 0
 
     before, after = parse_stdin()
+    if before == after:
+        click.echo(
+            "Pushed branch does not contain any new commit.",
+            err=True,
+        )
+        return 0
 
     if after == EMPTY_SHA:
         click.echo("Deletion event or nothing to scan.", err=True)

tests/functional/secret/test_scan_prepush.py

@@ -33,3 +33,27 @@ def test_scan_prepush(tmp_path: Path) -> None:
     # AND the error message contains the leaked secret
     stdout = exc.value.stdout.decode()
     assert recreate_censored_content(secret_content, GG_VALID_TOKEN) in stdout
+
+
+def test_scan_prepush_branch_without_new_commits(tmp_path: Path) -> None:
+    # GIVEN a remote repository
+    remote_repo = Repository.create(tmp_path / "remote", bare=True)
+
+    # AND a local clone
+    local_repo = Repository.clone(remote_repo.path, tmp_path / "local")
+
+    # Add a commit to the remote repository, otherwise git complains the branch does not
+    # contain anything
+    local_repo.create_commit()
+    local_repo.git("push")
+
+    # AND ggshield installed as a pre-push hook
+    run_ggshield("install", "-m", "local", "-t", "pre-push", cwd=local_repo.path)
+
+    # AND a branch without new commits
+    branch_name = "topic"
+    local_repo.create_branch(branch_name)
+
+    # WHEN I try to push the branch
+    # THEN the hook does not crash
+    local_repo.git("push", "-u", "origin", branch_name)

tests/functional/secret/test_scan_prereceive.py

@@ -10,7 +10,6 @@
 
 HOOK_CONTENT = """#!/bin/sh
 set -e
-echo "Hello from hook"
 ggshield secret scan pre-receive
 """
 
@@ -42,3 +41,29 @@ def test_scan_prereceive(tmp_path: Path) -> None:
     # AND the error message contains the leaked secret
     stderr = exc.value.stderr.decode()
     assert recreate_censored_content(secret_content, GG_VALID_TOKEN) in stderr
+
+
+def test_scan_prereceive_branch_without_new_commits(tmp_path: Path) -> None:
+    # GIVEN a remote repository
+    remote_repo = Repository.create(tmp_path / "remote", bare=True)
+
+    # AND a local clone
+    local_repo = Repository.clone(remote_repo.path, tmp_path / "local")
+
+    # Add a commit to the remote repository, otherwise git complains the branch does not
+    # contain anything
+    local_repo.create_commit()
+    local_repo.git("push")
+
+    # AND ggshield installed as a pre-receive hook
+    hook_path = remote_repo.path / "hooks" / "pre-receive"
+    hook_path.write_text(HOOK_CONTENT)
+    hook_path.chmod(0o755)
+
+    # AND a branch without new commits
+    branch_name = "topic"
+    local_repo.create_branch(branch_name)
+
+    # WHEN I try to push the branch
+    # THEN the hook does not crash
+    local_repo.git("push", "-u", "origin", branch_name)

tests/unit/cmd/scan/test_prepush.py

@@ -31,7 +31,7 @@ def create_local_repo_with_remote(work_dir: Path) -> Repository:
 
 class TestPrepush:
     @patch("ggshield.cmd.secret.scan.prepush.get_list_commit_SHA")
-    def test_pre_push_no_commits(self, get_list_mock: Mock, cli_fs_runner: CliRunner):
+    def test_prepush_no_commits(self, get_list_mock: Mock, cli_fs_runner: CliRunner):
         """
         GIVEN a prepush range with 0 commits
         WHEN the command is run
@@ -46,6 +46,37 @@ def test_pre_push_no_commits(self, get_list_mock: Mock, cli_fs_runner: CliRunner
         assert_invoke_ok(result)
         assert "Unable to get commit range." in result.output
 
+    @patch("ggshield.cmd.secret.scan.prepush.scan_commit_range")
+    def test_prepush_no_commits_stdin(
+        self,
+        scan_commit_range_mock: Mock,
+        tmp_path,
+        cli_fs_runner: CliRunner,
+    ):
+        """
+        GIVEN a repository
+        AND a new branch pushed from another repository, without any commit
+        WHEN the pre-push command is run on the commits from the push
+        THEN it scans nothing
+        """
+        local_repo = create_local_repo_with_remote(tmp_path)
+
+        branch = "topic"
+        local_repo.create_branch(branch)
+
+        sha = local_repo.get_top_sha()
+
+        with cd(str(local_repo.path)):
+            result = cli_fs_runner.invoke(
+                cli,
+                ["-v", "secret", "scan", "pre-push", "origin", local_repo.remote_url],
+                input=f"refs/heads/master {sha} refs/heads/master {EMPTY_SHA}\n",
+            )
+
+        assert_invoke_ok(result)
+        scan_commit_range_mock.assert_not_called()
+        assert "Unable to get commit range." in result.output
+
     @patch("ggshield.cmd.secret.scan.prepush.get_list_commit_SHA")
     @patch("ggshield.cmd.secret.scan.prepush.scan_commit_range")
     @patch("ggshield.cmd.secret.scan.prepush.check_git_dir")

tests/unit/cmd/scan/test_prereceive.py

@@ -289,6 +289,35 @@ def test_new_branch(
             ignored_detectors=set(),
         )
 
+    @patch("ggshield.cmd.secret.scan.prereceive.scan_commit_range")
+    def test_new_branch_without_commits(
+        self,
+        scan_commit_range_mock: Mock,
+        tmp_path,
+        cli_fs_runner: CliRunner,
+    ):
+        """
+        GIVEN a repository
+        AND a new branch pushed from another repository, without any commit
+        WHEN the pre-receive command is run on the commits from the push
+        THEN it scans nothing
+        """
+        repo = Repository.create(tmp_path)
+        sha = repo.create_commit("initial commit")
+        branch_name = "topic"
+        repo.create_branch(branch_name)
+
+        with cd(str(repo.path)):
+            result = cli_fs_runner.invoke(
+                cli,
+                ["-v", "secret", "scan", "pre-receive"],
+                input=f"{EMPTY_SHA} {sha} refs/heads/{branch_name}",
+            )
+
+        assert_invoke_ok(result)
+        scan_commit_range_mock.assert_not_called()
+        assert "Pushed branch does not contain any new commit" in result.output
+
     def test_stdin_input_deletion(
         self,
         cli_fs_runner: CliRunner,