Merge commit from fork

fix: write token store with owner-only permissions (GHSA-wjhr-76vg-2hvc)

Author: cyberjunky

garminconnect/client.py

@@ -13,6 +13,7 @@
 import http.cookiejar
 import json
 import logging
+import os
 import random
 import re
 import time
@@ -1191,12 +1192,33 @@ def dumps(self) -> str:
         return json.dumps(data)
 
     def dump(self, path: str) -> None:
-        """Write tokens safely to disk."""
+        """Write tokens safely to disk with owner-only permissions.
+
+        The token file contains the DI refresh token, which grants persistent
+        account access. It is written as 0o600 inside a 0o700 directory so a
+        permissive process umask can't leave it world-readable on a shared host
+        (GHSA-wjhr-76vg-2hvc).
+        """
         p = Path(path).expanduser()
         if p.is_dir() or not p.name.endswith(".json"):
             p = p / "garmin_tokens.json"
-        p.parent.mkdir(parents=True, exist_ok=True)
-        p.write_text(self.dumps())
+        p.parent.mkdir(mode=0o700, parents=True, exist_ok=True)
+        # mkdir's mode is subject to umask and a no-op if the dir already
+        # exists; chmod enforces 0o700 unconditionally.
+        with contextlib.suppress(OSError):
+            p.parent.chmod(0o700)
+        # Open with O_CREAT mode 0o600 (and O_NOFOLLOW where available so a
+        # pre-planted symlink can't redirect the write) instead of write_text,
+        # which would create the file under the umask first.
+        flags = os.O_WRONLY | os.O_CREAT | os.O_TRUNC
+        if hasattr(os, "O_NOFOLLOW"):
+            flags |= os.O_NOFOLLOW
+        fd = os.open(p, flags, 0o600)
+        with os.fdopen(fd, "w", encoding="utf-8") as token_file:
+            token_file.write(self.dumps())
+        # Enforce 0o600 even if the file pre-existed with looser permissions.
+        with contextlib.suppress(OSError):
+            p.chmod(0o600)
 
     def load(self, path: str) -> None:
         try:

pyproject.toml

@@ -56,6 +56,7 @@ module = [
     "test_retry_decorator",
     "test_workout_constants",
     "test_login_recovery",
+    "test_token_permissions",
     "conftest",
 ]
 disallow_untyped_defs = false
@@ -195,6 +196,7 @@ unfixable = []  # Allow all fixes, including unsafe ones
 "demo.py" = ["T20", "S101", "ERA", "RUF001", "PTH", "PERF401", "PERF203", "PLR0911", "D103"]  # Demo script - various user-facing patterns
 "example.py" = ["S110", "PLR0911", "T20", "PERF203"]  # Example script - simple error handling is intentional
 "test_strategy.py" = ["T20", "D103"]  # Interactive login-strategy diagnostic script
+"tests/test_token_permissions.py" = ["S105"]  # synthetic token literals in fixtures
 
 [tool.coverage.run]
 source = ["garminconnect"]

tests/test_token_permissions.py

@@ -0,0 +1,78 @@
+"""Security regression tests for token-store file permissions.
+
+Guards against GHSA-wjhr-76vg-2hvc: ``Client.dump()`` must write the token
+file (which holds the DI refresh token) owner-only (0o600) inside an
+owner-only directory (0o700), regardless of the process umask, so it can't be
+left world-readable on a shared host.
+
+POSIX-only — file mode bits are not meaningful on Windows.
+"""
+
+import os
+import stat
+import sys
+from pathlib import Path
+
+import pytest
+
+from garminconnect.client import Client
+
+pytestmark = pytest.mark.skipif(sys.platform == "win32", reason="POSIX file modes only")
+
+
+def _make_client() -> Client:
+    c = Client()
+    c.di_token = "ACCESS_TOKEN_EXAMPLE"
+    c.di_refresh_token = "REFRESH_TOKEN_EXAMPLE"
+    c.di_client_id = "CLIENT_ID_EXAMPLE"
+    return c
+
+
+def _mode(path) -> int:
+    return stat.S_IMODE(Path(path).stat().st_mode)
+
+
+def test_dump_creates_owner_only_token_file(tmp_path):
+    """Under a permissive umask the token file must still be 0o600."""
+    old_umask = os.umask(0o022)
+    try:
+        token_dir = tmp_path / "tokens"
+        _make_client().dump(str(token_dir))
+        token_file = token_dir / "garmin_tokens.json"
+
+        assert _mode(token_file) == 0o600, oct(_mode(token_file))
+        assert _mode(token_dir) == 0o700, oct(_mode(token_dir))
+        # World/group bits must be clear
+        assert not (_mode(token_file) & (stat.S_IRWXG | stat.S_IRWXO))
+        assert not (_mode(token_dir) & (stat.S_IRWXG | stat.S_IRWXO))
+    finally:
+        os.umask(old_umask)
+
+
+def test_dump_tightens_preexisting_loose_permissions(tmp_path):
+    """A pre-existing world-readable dir/file is tightened on dump."""
+    old_umask = os.umask(0o022)
+    try:
+        token_dir = tmp_path / "tokens"
+        token_dir.mkdir(mode=0o755)
+        token_file = token_dir / "garmin_tokens.json"
+        token_file.write_text("{}")
+        token_file.chmod(0o644)
+
+        _make_client().dump(str(token_dir))
+
+        assert _mode(token_file) == 0o600, oct(_mode(token_file))
+        assert _mode(token_dir) == 0o700, oct(_mode(token_dir))
+    finally:
+        os.umask(old_umask)
+
+
+def test_dump_explicit_json_path(tmp_path):
+    """A direct ``*.json`` path is also written owner-only."""
+    old_umask = os.umask(0o022)
+    try:
+        token_file = tmp_path / "store" / "garmin_tokens.json"
+        _make_client().dump(str(token_file))
+        assert _mode(token_file) == 0o600, oct(_mode(token_file))
+    finally:
+        os.umask(old_umask)