Repository: gitguardian/ggshield
Issue: Config Error Messages Replace "-" With "_"

## Environment

- ggshield version: 1.14.3
- Operating system (Linux, macOS, Windows): macOS
- Operating system version: Ventura 13.1
- Python version: 3.10.10

## Describe the bug

The "Unrecognized key in config" error message displays an underscore instead of hyphen when an invalid configuration is present.

Steps to reproduce:

Create an invalid configuration file such as (note `ignored-matches` misspelled):

**`.gitguardian.yml`**
```yaml
version: 2
secret:
  ignore-matches:
    - name: some-secret-i-want-to-ignore
      match: 4f2f11652a0<snip>5ead42463
```

Execute a `git commit` that includes the secret you wish to ignore (with GGShield configured as a pre-commit hook).

Actual result:

Error message:

> Unrecognized key in config: ignore_matches

Note the underscore when the configuration has a hyphen.

Expected result:

Error message:

> Unrecognized key in config: ignore-matches

(hyphen instead of underscore)


Changed files:
tests/unit/core/test_check_updates.py, ggshield/core/check_updates.py

diff --git a/ggshield/core/check_updates.py b/ggshield/core/check_updates.py
index 16506e5..f36ca4f 100644
--- a/ggshield/core/check_updates.py
+++ b/ggshield/core/check_updates.py
@@ -17,6 +17,8 @@ CACHE_FILE = os.path.join(
     "update_check.yaml",
 )
 
+CHECK_AT_KEY = "check-at"
+
 
 # Use a short timeout to prevent blocking
 CHECK_TIMEOUT = 5
@@ -26,25 +28,32 @@ def _split_version(version: str) -> Tuple[int, ...]:
     return tuple([int(x) for x in version.split(".")])
 
 
+def load_last_check_time() -> Optional[float]:
+    """Returns the last time we checked, or None if not available"""
+    try:
+        cached_data = load_yaml_dict(CACHE_FILE)
+        if cached_data is None:
+            # File does not exist, do not log any warning
+            return None
+        return float(cached_data[CHECK_AT_KEY])
+    except Exception as e:
+        logger.warning("Could not load cached latest version: %s", repr(e))
+        return None
+
+
+def save_last_check_time(check_at: float) -> None:
+    """Store the last time we checked"""
+    save_yaml_dict({CHECK_AT_KEY: check_at}, CACHE_FILE)
+
+
 def check_for_updates() -> Optional[str]:
     """
     Check for ggshield updates on GitHub. Return the latest version if available.
     Query GitHub API at most once per day and save locally the latest version in a file.
     """
-    check_at = -1.0
-    # Load the last time we checked
-    try:
-        cached_data = load_yaml_dict(CACHE_FILE)
-    except ValueError:
-        # Swallow the error
-        cached_data = None
-    if cached_data is not None:
-        try:
-            check_at = cached_data["check_at"]
-        except Exception as e:
-            logger.warning("Could not load cached latest version: %s", e)
-
-    if check_at > 0 and (time.time() - check_at < 24 * 60 * 60):
+    check_at = load_last_check_time()
+
+    if check_at is not None and (time.time() - check_at < 24 * 60 * 60):
         # We checked today, no need to check again
         return None
 
@@ -53,9 +62,9 @@ def check_for_updates() -> Optional[str]:
     # Save check time now so that it is saved even if the check fails. This ensures we
     # don't try for every command if the user does not have network access.
     try:
-        save_yaml_dict({"check_at": time.time()}, CACHE_FILE)
+        save_last_check_time(time.time())
     except Exception as e:
-        logger.warning("Could not save time of version check to cache: %s", e)
+        logger.warning("Could not save time of version check to cache: %s", repr(e))
         # Do not continue if we can't save check time. If we continue we are going to
         # send requests to api.github.com every time ggshield is called.
         return None
@@ -71,7 +80,7 @@ def check_for_updates() -> Optional[str]:
             timeout=CHECK_TIMEOUT,
         )
     except Exception as e:
-        logger.warning("Failed to connect to api.github.com: %s", e)
+        logger.warning("Failed to connect to api.github.com: %s", repr(e))
         return None
 
     if resp.status_code != 200:
@@ -89,9 +98,11 @@ def check_for_updates() -> Optional[str]:
                 return None
 
             try:
-                save_yaml_dict({"check_at": check_at}, CACHE_FILE)
+                save_last_check_time(check_at)
             except Exception as e:
-                logger.warning("Could not save time of version check to cache: %s", e)
+                logger.warning(
+                    "Could not save time of version check to cache: %s", repr(e)
+                )
 
             return None
 
@@ -105,7 +116,7 @@ def check_for_updates() -> Optional[str]:
         current_version_split = _split_version(__version__)
         latest_version_split = _split_version(latest_version)
     except Exception as e:
-        logger.warning("Failed to parse response: %s", e)
+        logger.warning("Failed to parse response: %s", repr(e))
         return None
 
     if current_version_split < latest_version_split:
diff --git a/tests/unit/core/test_check_updates.py b/tests/unit/core/test_check_updates.py
index 17a8a56..0c3f87c 100644
--- a/tests/unit/core/test_check_updates.py
+++ b/tests/unit/core/test_check_updates.py
@@ -7,7 +7,11 @@ from _pytest.monkeypatch import MonkeyPatch
 from pyfakefs.fake_filesystem import FakeFilesystem
 
 import ggshield.core
-from ggshield.core.check_updates import CACHE_FILE, check_for_updates
+from ggshield.core.check_updates import (
+    CACHE_FILE,
+    check_for_updates,
+    load_last_check_time,
+)
 
 
 @patch("requests.get")
@@ -103,3 +107,52 @@ def test_check_for_updates_does_nothing_if_cache_cant_be_saved(
     latest_version = check_for_updates()
     assert latest_version is None
     request_get_mock.assert_not_called()
+
+
+def test_load_last_check_time_ok(fs: FakeFilesystem):
+    """
+    GIVEN an update cache file
+    WHEN load_last_check_time() is called
+    THEN it correctly reads the last check time
+    """
+    Path(CACHE_FILE).parent.mkdir(parents=True, exist_ok=False)
+    Path(CACHE_FILE).write_text("check-at: 2")
+
+    assert load_last_check_time() == 2
+
+
+def test_load_last_check_time_no_cache_ok(caplog, fs: FakeFilesystem):
+    """
+    GIVEN no update cache file
+    WHEN load_last_check_time() is called
+    THEN it returns None
+    AND no warning messages are logged
+    """
+    assert load_last_check_time() is None
+    assert len(caplog.records) == 0
+
+
+@pytest.mark.parametrize(
+    "content",
+    [
+        "this-is-not-yaml",
+        "no-cache-key: 1",
+        "check-at: not_a_float",
+    ],
+)
+def test_load_last_check_time_swallows_errors(caplog, fs: FakeFilesystem, content: str):
+    """
+    GIVEN invalid content for the cache file
+    WHEN load_last_check_time() is called
+    THEN it does not raise any exception
+    AND it returns None
+    AND 1 warning message is logged
+    """
+    Path(CACHE_FILE).parent.mkdir(parents=True, exist_ok=False)
+    Path(CACHE_FILE).write_text(content)
+
+    assert load_last_check_time() is None
+    log_record = caplog.records[0]
+    assert log_record.levelname == "WARNING"
+    assert "Could not load cached latest version" in log_record.message
+    assert len(caplog.records) == 1