security: rewind file bodies before 401 retry to prevent empty uploads

garminconnect/client.py

@@ -18,6 +18,7 @@
 import re
 import threading
 import time
+from collections.abc import Iterator, Mapping
 from pathlib import Path
 from typing import Any, cast
 
@@ -185,6 +186,61 @@ def _build_basic_auth(client_id: str) -> str:
     return "Basic " + base64.b64encode(f"{client_id}:".encode()).decode()
 
 
+def _iter_file_objects(kwargs: dict[str, Any]) -> Iterator[Any]:
+    """Yield file-like objects referenced by request kwargs (files/data)."""
+    files = kwargs.get("files")
+    if isinstance(files, Mapping):
+        values = list(files.values())
+    elif isinstance(files, (list, tuple)):
+        values = [v for _, v in files]
+    else:
+        values = []
+    for value in values:
+        # requests accepts fileobj or (name, fileobj[, content_type[, headers]])
+        fileobj = (
+            value[1] if isinstance(value, (tuple, list)) and len(value) >= 2 else value
+        )
+        if hasattr(fileobj, "read"):
+            yield fileobj
+    data = kwargs.get("data")
+    if hasattr(data, "read"):
+        yield data
+
+
+def _capture_file_positions(
+    kwargs: dict[str, Any],
+) -> list[tuple[Any, int]] | None:
+    """Record the stream position of each file-like body before a request.
+
+    Returns None when any body cannot be repositioned, meaning a retry
+    would re-read from EOF and silently send empty/truncated content.
+    """
+    positions: list[tuple[Any, int]] = []
+    for fileobj in _iter_file_objects(kwargs):
+        try:
+            if not fileobj.seekable():
+                return None
+            positions.append((fileobj, fileobj.tell()))
+        except (OSError, ValueError, AttributeError):
+            return None
+    data = kwargs.get("data")
+    if data is not None and not hasattr(data, "read") and isinstance(data, Iterator):
+        # Streamed iterable body (e.g. a generator): attempt #1 consumes it
+        # and it cannot be rewound, so a retry must not be attempted.
+        return None
+    return positions
+
+
+def _restore_file_positions(positions: list[tuple[Any, int]]) -> bool:
+    """Rewind file-like bodies to their pre-request positions. False on failure."""
+    try:
+        for fileobj, pos in positions:
+            fileobj.seek(pos)
+    except (OSError, ValueError, AttributeError):
+        return False
+    return True
+
+
 def _decode_jwt_payload(token: str) -> dict[str, Any] | None:
     """Decode a JWT payload without verifying the signature.
 
@@ -1532,12 +1588,23 @@ def _run_request(self, method: str, path: str, **kwargs: Any) -> Any:
         headers.update(custom_headers)
 
         sess = self._api_session
+        # Snapshot stream positions of any file bodies so a 401 retry can
+        # rewind them; attempt #1 reads file handles to EOF, and re-sending
+        # the same kwargs would otherwise upload an empty/truncated body.
+        file_positions = _capture_file_positions(kwargs)
         resp = sess.request(method, url, headers=headers, **kwargs)
 
         if resp.status_code == 401:
             with self._token_lock:
                 self._refresh_session()
-            resp = sess.request(method, url, headers=self.get_api_headers(), **kwargs)
+            if file_positions is not None and _restore_file_positions(file_positions):
+                headers = self.get_api_headers()
+                headers.update(custom_headers)
+                resp = sess.request(method, url, headers=headers, **kwargs)
+            else:
+                # Unseekable/unrewindable file body: retrying would silently
+                # send an empty part. Fall through so the 401 raises below.
+                _LOGGER.debug("Skipping 401 retry: request body is not rewindable")
 
         if resp.status_code == 204:
 

tests/test_garmin_unit.py

@@ -1639,6 +1639,151 @@ def test_rejects_path_with_traversal_or_query(
             c._run_request("GET", bad_path)
 
 
+# ---------------------------------------------------------------------------
+# _run_request: 401 retry must rewind file bodies and keep custom headers
+# ---------------------------------------------------------------------------
+
+
+class Test401RetryFileRewind:
+    """A 401 retry must not re-send a consumed file handle (empty upload)."""
+
+    def _client(self, monkeypatch, request_fn):
+        g = garminconnect.Garmin()
+        c = g.client
+        monkeypatch.setattr(c, "get_api_headers", dict)
+        monkeypatch.setattr(c, "_refresh_session", lambda: None)
+        monkeypatch.setattr(c._api_session, "request", request_fn)
+        return c
+
+    def _consuming_request(self, responses, read_sizes):
+        """Fake session.request that drains file bodies like requests does."""
+
+        def request(method, url, **kwargs):
+            files = kwargs.get("files") or {}
+            for value in files.values():
+                fileobj = value[1] if isinstance(value, tuple) else value
+                read_sizes.append(len(fileobj.read()))
+            return responses.pop(0)
+
+        return request
+
+    def test_retry_rewinds_file_body(self, monkeypatch):
+        read_sizes: list[int] = []
+        responses = [_FakeResp(401, {}), _FakeResp(200, {"ok": True})]
+        c = self._client(
+            monkeypatch, self._consuming_request(responses, read_sizes)
+        )
+        payload = b"FITDATA" * 100
+        resp = c._run_request(
+            "POST", "upload", files={"file": ("a.fit", io.BytesIO(payload))}
+        )
+        assert resp.status_code == 200
+        # Attempt #1 and the retry must both read the full payload.
+        assert read_sizes == [len(payload), len(payload)]
+
+    def test_retry_restores_initial_position_not_zero(self, monkeypatch):
+        read_sizes: list[int] = []
+        responses = [_FakeResp(401, {}), _FakeResp(200, {"ok": True})]
+        c = self._client(
+            monkeypatch, self._consuming_request(responses, read_sizes)
+        )
+        stream = io.BytesIO(b"HEADER" + b"BODY" * 10)
+        stream.seek(6)  # caller intentionally skips a prefix
+        c._run_request("POST", "upload", files={"file": ("a.fit", stream)})
+        assert read_sizes[0] == read_sizes[1] == 40
+
+    def test_unseekable_body_skips_retry_and_raises(self, monkeypatch):
+        class UnseekableStream(io.BytesIO):
+            def seekable(self):
+                return False
+
+        calls = []
+
+        def request(method, url, **kwargs):
+            calls.append(url)
+            return _FakeResp(401, {})
+
+        c = self._client(monkeypatch, request)
+        with pytest.raises(garminconnect.GarminConnectConnectionError):
+            c._run_request(
+                "POST",
+                "upload",
+                files={"file": ("a.fit", UnseekableStream(b"DATA"))},
+            )
+        # No blind retry that would have sent an empty body.
+        assert len(calls) == 1
+
+    def test_retry_rewinds_mapping_files(self, monkeypatch):
+        # requests accepts any Mapping for files (via to_key_val_list), so
+        # position capture must not be limited to plain dicts.
+        from types import MappingProxyType
+
+        read_sizes: list[int] = []
+        responses = [_FakeResp(401, {}), _FakeResp(200, {"ok": True})]
+        c = self._client(
+            monkeypatch, self._consuming_request(responses, read_sizes)
+        )
+        payload = b"FITDATA" * 100
+        files = MappingProxyType({"file": ("a.fit", io.BytesIO(payload))})
+        resp = c._run_request("POST", "upload", files=files)
+        assert resp.status_code == 200
+        assert read_sizes == [len(payload), len(payload)]
+
+    def test_generator_data_skips_retry_and_raises(self, monkeypatch):
+        # A streamed (generator) body is consumed by attempt #1 and cannot be
+        # rewound; the 401 must raise instead of resending the remainder.
+        calls = []
+
+        def request(method, url, **kwargs):
+            calls.append(url)
+            # Drain the streamed body like requests does.
+            for _ in kwargs.get("data") or ():
+                pass
+            return _FakeResp(401, {})
+
+        c = self._client(monkeypatch, request)
+
+        def body():
+            yield b"chunk1"
+            yield b"chunk2"
+
+        with pytest.raises(garminconnect.GarminConnectConnectionError):
+            c._run_request("POST", "upload", data=body())
+        assert len(calls) == 1
+
+    def test_restore_file_positions_false_when_seek_missing(self):
+        # A duck-typed stream with seekable()/tell() but no seek() must make
+        # restoration fail (AttributeError caught) rather than raise.
+        class NoSeekStream:
+            def read(self, n: int = -1) -> bytes:
+                return b""
+
+            def seekable(self) -> bool:
+                return True
+
+            def tell(self) -> int:
+                return 0
+
+        assert client_mod._restore_file_positions([(NoSeekStream(), 0)]) is False
+
+    def test_retry_keeps_custom_headers(self, monkeypatch):
+        seen_headers = []
+        responses = [_FakeResp(401, {}), _FakeResp(200, {"ok": True})]
+
+        def request(method, url, **kwargs):
+            seen_headers.append(kwargs.get("headers") or {})
+            return responses.pop(0)
+
+        c = self._client(monkeypatch, request)
+        c._run_request(
+            "POST",
+            "upload",
+            headers={"NK": "NT", "User-Agent": "GCM-iOS-5.7.2.1"},
+        )
+        assert seen_headers[1].get("NK") == "NT"
+        assert seen_headers[1].get("User-Agent") == "GCM-iOS-5.7.2.1"
+
+
 # ---------------------------------------------------------------------------
 # Error-message sanitization
 # ---------------------------------------------------------------------------