Repository: gitguardian/ggshield
Issue: pre-receive does not print filenames with spaces correctly

**GitGuardian Shield Version**

- 1.12.0

**Command executed**

`ggshield secret scan pre-receive`

**Describe the bug**

- Unpack this [zip file][zip], it contains a git repository with this content:
    - `biggerpython_server copie 11.py`: a file with spaces in its name. Contains a secret.
    - `startscan.sh`: a script to reproduce the bug. It calls `ggshield secret scan pre-receive` with the right stdin content.
- Run `startscan.sh`

[zip]: https://github.com/GitGuardian/ggshield/files/8957755/prereceive-filename-bug-example.zip

The output looks like this:

```
$ ./startscan.sh 
Scanning Commits  [####################################]  100%

commit 9e0b342414884fc68e8939504545c479524c38dd
Author: Aurelien Gateau <aurelien.gateau@gitguardian.com>
Date: Wed Jun 22 14:52:35 2022 +0200

🛡️  ⚔️  🛡️  1 incident has been found in file pie

(...)
```

As you can see the file name is wrong.

**Expected behavior**

File name should be printed correctly, like `ggshield secret scan path 'biggerpython_server copie 11.py'` does:

```
secrets-engine-version: 2.69.0

🛡️  ⚔️  🛡️  1 incident has been found in file /home/agateau/tmp/prereceive-filename-bug-example/biggerpython_server copie 11.py
(...)
```



Changed files:
tests/scan/test_scannable.py, ggshield/scan/scannable.py, tests/scan/test_commit.py

diff --git a/ggshield/scan/scannable.py b/ggshield/scan/scannable.py
index 6c45ed1..9156b65 100644
--- a/ggshield/scan/scannable.py
+++ b/ggshield/scan/scannable.py
@@ -2,7 +2,7 @@ import concurrent.futures
 import logging
 import re
 from pathlib import Path
-from typing import Any, Callable, Dict, Iterable, List, NamedTuple, Optional, Set
+from typing import Any, Callable, Dict, Iterable, List, NamedTuple, Optional, Set, Tuple
 
 import click
 from pygitguardian import GGClient
@@ -30,6 +30,17 @@ from .scannable_errors import handle_scan_chunk_error
 logger = logging.getLogger(__name__)
 
 
+_MATCH_FILENAME_RX = re.compile(r"^a/(.*) b/\\1$")
+
+
+class PatchParseError(Exception):
+    """
+    Raised by Commit.get_files() if it fails to parse its patch.
+    """
+
+    pass
+
+
 class Result(NamedTuple):
     """
     Return model for a scan which zips the information
@@ -270,16 +281,7 @@ class Commit(Files):
         return self._files
 
     @staticmethod
-    def get_filename(line: str) -> str:
-        """
-        Get the file path from the line patch
-
-        Example: line = "a/filename.txt b/filename.txt"
-        """
-        return line.split(" ")[1][2:]
-
-    @staticmethod
-    def get_filemode(line: str) -> Filemode:
+    def _get_filemode(line: str) -> Filemode:
         """
         Get the file mode from the line patch (new, modified or deleted)
 
@@ -296,7 +298,53 @@ class Commit(Files):
         if line.startswith("old"):
             return Filemode.PERMISSION_CHANGE
 
-        raise click.ClickException(f"Filemode is not detected:{line}")
+        raise PatchParseError("Filemode is not detected")
+
+    @staticmethod
+    def _parse_diff_header_lines(lines: List[str]) -> Tuple[str, Filemode]:
+        """
+        Parses the part of the patch after a "diff --git " string to extract the
+        filename and filemode.
+        """
+
+        # This code is tricky because the first line of a diff looks like this:
+        #
+        #     diff --git a/path/to/filename b/path/to/filename
+        #
+        # But spaces are not escaped, so if a filename contains a space the line looks
+        # like this:
+        #
+        #     diff --git a/path/to/file name b/path/to/file name
+        #
+        # For all cases but RENAMEs, the old and the new file are the same. We take
+        # advantage of this in the _MATCH_FILENAME_RX regular expression.
+        #
+        # For RENAMEs we look at the next lines. A rename header looks like this:
+        #
+        #     diff --git a/old_name b/new_name
+        #     similarity index 90%
+        #     rename from old_name
+        #     rename to new_name
+        #
+        # We look for the "rename to " and get the new file name from it.
+        #
+        # Note: we can't use the "--- old", "+++ new" header lines because
+        # PERMISSION_CHANGE and RENAME without changes do not have these lines.
+        filemode = Commit._get_filemode(lines[1])
+        if filemode == Filemode.RENAME:
+            prefix = "rename to "
+            for line in lines[2:]:
+                if line.startswith(prefix):
+                    filename = line[len(prefix) :]
+                    break
+            else:
+                raise PatchParseError("Could not extract filename, no rename prefix")
+        else:
+            match = _MATCH_FILENAME_RX.match(lines[0])
+            if not match:
+                raise PatchParseError("Could not extract filename")
+            filename = match.group(1)
+        return filename, filemode
 
     def get_files(self) -> Iterable[CommitFile]:
         """
@@ -316,11 +364,16 @@ class Commit(Files):
         for diff in list_diff:
             lines = diff.split("\\n")
 
-            filename = self.get_filename(lines[0])
+            try:
+                filename, filemode = self._parse_diff_header_lines(lines)
+            except PatchParseError as e:
+                if self.sha:
+                    raise click.ClickException(f"Error parsing commit {self.sha}: {e}")
+                else:
+                    raise click.ClickException(f"Error parsing diff: {e}")
+
             if is_filepath_excluded(filename, self.exclusion_regexes):
                 continue
-
-            filemode = self.get_filemode(lines[1])
             document = "\\n".join(lines[filemode.start :])
             file_size = len(document.encode("utf-8"))
             if file_size > MAX_FILE_SIZE * 0.90:
diff --git a/tests/scan/test_commit.py b/tests/scan/test_commit.py
index 1146051..d2d1c5d 100644
--- a/tests/scan/test_commit.py
+++ b/tests/scan/test_commit.py
@@ -1,27 +1,109 @@
+import click
+import pytest
+
 from ggshield.core.utils import Filemode
 from ggshield.scan import Commit
 
 
-def test_get_filename():
-    line = "a/test.txt b/test.txt"
-    assert Commit().get_filename(line) == "test.txt"
+NEW_FILE = """
+a/test.txt b/test.txt
+new file mode 100644
+"""
+
+DELETED_FILE = """
+a/test.txt b/test.txt
+deleted file mode 100644
+"""
+
+MODIFIED_FILE = """
+a/test.txt b/test.txt
+index e965047..802992c 100644
+"""
+
+MODIFIED_FILE_WITH_SPACES = """
+a/some spaces.txt b/some spaces.txt
+index e965047..802992c 100644
+"""
+
+RENAMED_FILE = """
+a/old.txt b/new.txt
+similarity index 71%
+rename from old.txt
+rename to new.txt
+"""
+
+PERMISSION_CHANGE = """
+a/script.py b/script.py
+old mode 100644
+new mode 100755
+"""
+
+
+@pytest.mark.parametrize(
+    ("header", "expected_name", "expected_mode"),
+    (
+        (NEW_FILE, "test.txt", Filemode.NEW),
+        (
+            DELETED_FILE,
+            "test.txt",
+            Filemode.DELETE,
+        ),
+        (
+            MODIFIED_FILE,
+            "test.txt",
+            Filemode.MODIFY,
+        ),
+        (
+            MODIFIED_FILE_WITH_SPACES,
+            "some spaces.txt",
+            Filemode.MODIFY,
+        ),
+        (RENAMED_FILE, "new.txt", Filemode.RENAME),
+        (PERMISSION_CHANGE, "script.py", Filemode.PERMISSION_CHANGE),
+    ),
+)
+def test_parse_diff_header_lines(header, expected_name, expected_mode):
+    lines = header.strip().split("\\n")
+    name, mode = Commit._parse_diff_header_lines(lines)
+    assert (name, mode) == (expected_name, expected_mode)
+
 
+INVALID_FILE_MODE = """
+diff --git a/old.txt b/new.txt
+similarity index 71%
+indx 0000000..b80e3df
+"""
 
-def test_get_filemode_new():
-    line = "new file mode 100644\\n"
-    assert Commit().get_filemode(line) == Filemode.NEW
 
+INVALID_RENAMED_FILE = """
+diff --git a/old.txt b/new.txt
+similarity index 71%
+index 0000000..b80e3df
+"""
 
-def test_get_filemode_delete():
-    line = "deleted file mode 100644\\n"
-    assert Commit().get_filemode(line) == Filemode.DELETE
 
+INVALID_MODIFIED_FILE = """
+diff --git a/test1.txt b/test2.txt
+index e965047..802992c 100644
+"""
 
-def test_get_filemode_modify():
-    line = "index 3d47bfe..ee93988 100644\\n"
-    assert Commit().get_filemode(line) == Filemode.MODIFY
 
+@pytest.mark.parametrize(
+    ("patch",),
+    (
+        pytest.param(INVALID_FILE_MODE, id="invalid-filemode"),
+        pytest.param(INVALID_RENAMED_FILE, id="invalid-renamed"),
+        pytest.param(INVALID_MODIFIED_FILE, id="invalid-modified"),
+    ),
+)
+def test_parsing_invalid_patch_fails(patch: str):
+    commit = Commit()
+    commit._patch = patch.lstrip()
+    with pytest.raises(click.ClickException) as info:
+        list(commit.get_files())
+    assert "Error parsing diff:" in info.value.message
 
-def test_get_filemode_rename():
-    line = "similarity index 99%\\n"
-    assert Commit().get_filemode(line) == Filemode.RENAME
+    commit.sha = "1234567"
+    with pytest.raises(click.ClickException) as info:
+        list(commit.get_files())
+    assert f"Error parsing commit {commit.sha}:" in info.value.message
diff --git a/tests/scan/test_scannable.py b/tests/scan/test_scannable.py
index 6d79de6..64d9ff3 100644
--- a/tests/scan/test_scannable.py
+++ b/tests/scan/test_scannable.py
@@ -114,7 +114,7 @@ diff --git a/bin/shutdown.sh b/bin/shutdown.sh
 old mode 100644
 new mode 100755
 
-diff --git a/tests/test_scannable.py b/.env
+diff --git a/.env b/.env
 new file mode 100644
 index 0000000..0000000
 --- /dev/null
@@ -166,7 +166,7 @@ def test_patch_separation_ignore():
 def test_patch_max_size():
     c = Commit()
     c._patch = """
-diff --git a/tests/test_scannable.py b/.env
+diff --git a/.env b/.env
 new file mode 100644
 index 0000000..0000000
 --- /dev/null