security: rewind file bodies before 401 retry to prevent empty uploads - #409
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe client captures upload stream positions before requests, restores them after token refresh, and retries only when restoration succeeds. Unit tests cover seekable and unseekable bodies, nonzero positions, consumed streams, and custom headers. ChangesRequest Body Retry Handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant _run_request
participant UploadStreams
participant TokenRefresh
Caller->>_run_request: Send request with upload streams
_run_request->>UploadStreams: Capture stream positions
_run_request->>TokenRefresh: Refresh token after 401
_run_request->>UploadStreams: Restore stream positions
UploadStreams-->>_run_request: Return restoration result
_run_request->>Caller: Retry request or return original 401
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1ae7bff to
1f793bc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@garminconnect/client.py`:
- Around line 189-204: Update _iter_file_objects to recognize files values from
collections.abc.Mapping, not only dict, and classify streamed data iterables
such as generators even when they lack read(). Ensure _capture_file_positions
returns None for any non-rewindable streamed body so _run_request skips the 401
retry rather than resending consumed content; preserve rewindable file handling.
Add regression coverage for mapping-like files and generator data.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5fe70fae-3906-44ca-beea-33f7a10a59a4
📒 Files selected for processing (2)
garminconnect/client.pytests/test_garmin_unit.py
07f327d to
4cf7b57
Compare
4cf7b57 to
e5ce1e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@garminconnect/client.py`:
- Around line 187-188: Update _iter_file_objects to annotate its generator
return type as Iterator[Any] instead of Any, and add the required Iterator
import using the file’s existing typing-import style.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2e07e2f9-bc90-4658-a47e-c766c89ec35d
📒 Files selected for processing (1)
garminconnect/client.py
e5ce1e6 to
33208e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@garminconnect/client.py`:
- Around line 228-235: Update _restore_file_positions to treat a missing seek
method like other restoration failures by catching AttributeError alongside
OSError and ValueError. Ensure it returns False so retry logic does not resend a
consumed body when restoration cannot be performed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3ced7298-f09a-4d97-aba2-8b8d9e7d3fe3
📒 Files selected for processing (2)
garminconnect/client.pytests/test_garmin_unit.py
33208e3 to
22b6f13
Compare
_run_request retried a 401 by re-sending the same **kwargs. For uploads, kwargs['files'] holds an open file handle that requests reads to EOF while encoding the multipart body on the first attempt; the retry re-read from EOF and uploaded an empty file part while reporting success. Any 401 during an upload (routine token expiry or a hostile/ MITM response) caused deterministic silent data loss (report 3995). Snapshot the stream position of every file-like body before the first attempt and rewind before retrying. If a body is unseekable, skip the retry and let the 401 raise GarminConnectConnectionError instead of silently sending an empty part. The retry now also re-merges the caller's custom headers (NK/origin/User-Agent), which were previously dropped in favor of bare get_api_headers().
22b6f13 to
1dc6d20
Compare
Addresses CodeRabbit review on PR #409: - _iter_file_objects: recognize any Mapping for files (requests accepts them via to_key_val_list), not only dict; annotate as Iterator[Any]. - _capture_file_positions: return None for iterator data bodies (e.g. generators) that attempt #1 consumes and cannot be rewound. - _restore_file_positions: catch AttributeError (missing seek) like other restoration failures so the retry is skipped instead of crashing. Adds regression tests for Mapping files, generator data, and seek-less duck-typed streams.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@garminconnect/client.py`:
- Around line 226-230: The streamed-body check in _capture_file_positions
currently misses one-shot Iterable objects that implement __iter__ without
__next__. Update it to reject non-file Iterable/.iter bodies before retrying,
while excluding materialized str, bytes, list, tuple, and Mapping values;
preserve existing Iterator handling and add a regression test alongside
test_generator_data_skips_retry_and_raises.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cdc42d74-3ed8-4e31-8be3-2802482b33b5
📒 Files selected for processing (2)
garminconnect/client.pytests/test_garmin_unit.py
| 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 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from collections.abc import Iterable, Iterator
class OneShotBody:
def __init__(self):
self._body = iter((b"chunk1", b"chunk2"))
def __iter__(self):
return self._body
body = OneShotBody()
assert isinstance(body, Iterable)
assert not isinstance(body, Iterator)
assert list(body) == [b"chunk1", b"chunk2"]
assert list(body) == []
print("One-shot Iterable is missed by an Iterator-only check")
PY
python3 - <<'PY'
import inspect
import requests
from requests.models import PreparedRequest
print("requests:", requests.__version__)
print(inspect.getsource(PreparedRequest.prepare_body))
PYRepository: cyberjunky/python-garminconnect
Length of output: 344
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files =="
git ls-files | sed -n '1,120p'
echo "== locate client.py and test files =="
fd -a 'client.py|test_garmin_unit.py' . | sed -n '1,80p'
echo "== inspect client.py relevant section =="
client=$(fd 'client.py' . | head -1)
if [ -n "$client" ]; then
wc -l "$client"
sed -n '180,260p' "$client" | nl -ba -v180
fi
echo "== search retry/capture helpers and tests =="
rg -n "_capture_file_positions|_restore_file_positions|_run_request|streamed|generator_data_skips|requests" -S .gitignore README.md tests garminconnect 2>/dev/null | sed -n '1,220p'Repository: cyberjunky/python-garminconnect
Length of output: 1614
🌐 Web query:
Requests PreparedRequest prepare_body source streamed body callable iterator read
💡 Result:
In the Python requests library, the PreparedRequest.prepare_body method handles the preparation of request bodies, including detecting whether the provided data is a streamed body [1][2]. When data is passed to prepare_body, the library identifies it as a stream if it is iterable (typically checked via collections.abc.Iterable or the presence of a iter attribute) and is not a standard type like a string, bytes, list, tuple, or Mapping [1][3][2]. Key mechanisms for streamed bodies include: 1. Stream Detection: Modern versions of requests use a check similar to isinstance(data, Iterable) or hasattr(data, "iter") to identify streams [3][2]. Using both ensures compatibility with objects that rely on getattr delegation for their interface, such as file-like wrappers [4][3]. 2. Rewind Capability: If the streamed object has a tell method, requests records the current position in the _body_position attribute [2][5]. This allows the library to attempt to rewind the stream if the request needs to be resent, such as following a 307 or 308 redirect [4][2]. 3. Content Length: If a length can be determined for the stream (using super_len), it is set in the Content-Length header; otherwise, the library uses Transfer-Encoding: chunked [1][2]. A recent regression in version 2.34.0 (addressed in PR #7433) highlighted that relying solely on isinstance(data, Iterable) can break file-like objects that dynamically proxy their interface, leading to issues where the body cannot be properly rewound during redirects [4][3]. The fix reinstated the explicit check for the iter attribute to ensure robust support for these patterns [3].
Citations:
- 1: https://docs.python-requests.org/en/latest/_modules/requests/models/
- 2: https://github.com/psf/requests/blob/23953c0c/src/requests/models.py
- 3: Fix
prepare_bodystream detection for__getattr__-based file wrappers psf/requests#7433 - 4:
prepare_bodystream detection regression psf/requests#7432 - 5: https://github.com/psf/requests/blob/4c800e9a/src/requests/models.py
🏁 Script executed:
#!/bin/bash
set -euo pipefail
client="$(fd 'client.py' . | head -1)"
test_file="$(fd 'test_garmin_unit.py' . | head -1)"
echo "== client imports and retry helpers =="
sed -n '1,40p' "$client"
sed -n '190,245p' "$client"
echo "== test generator/stream body context =="
sed -n '1,80p' "$test_file"
rg -n "def test_generator_data_skips_retry_and_raises|_capture_file_positions|_run_request|Iterab|Iterator|OneShot|Generator|stream" "$test_file" "$client"
echo "== Python standard library probe for current Iterator check =="
python3 - <<'PY'
from collections.abc import Iterable, Iterator, Mapping
class OneShotBody:
def __init__(self):
self._body = iter((b"chunk1", b"chunk2"))
def __iter__(self):
return self._body
# Mirrors the current request body classification before retry decisions
class Current:
pass
Current.__bases__ = ()
def current_is_iterable_file(data):
return data is not None and hasattr(data, "read") and Current.__bases__ == ()
def current_is_stream(data):
return data is not None and not hasattr(data, "read") and isinstance(data, Iterator)
body = OneShotBody()
print("object", type(body).__name__)
print("is_iterable_file()", current_is_iterable_file(body))
print("is_stream()", current_is_stream(body))
print("first list()", list(body))
print("second list()", list(body))
# Modern equivalent with materialized container exclusion
def fixed_is_stream(data):
return (data is not None
and not hasattr(data, "read")
and (isinstance(data, Iterable) or hasattr(data, "iter"))
and not isinstance(data, (str, bytes, list, tuple, Mapping)))
print("fixed_is_stream()", fixed_is_stream(body))
print("fixed first list()", list(body))
print("fixed second list()", list(body))
PYRepository: cyberjunky/python-garminconnect
Length of output: 9409
🌐 Web query:
site:github.com/psf/requests PreparedRequest.prepare_body is Iterable or hasattr "iter"
💡 Result:
In the requests library, the PreparedRequest.prepare_body method determines if the provided body data should be treated as a stream (iterable) by checking if the data is an instance of collections.abc.Iterable or has the iter attribute [1][2]. For many years, the library exclusively used hasattr(data, "iter") to detect streams [3][2]. However, a change was introduced to use isinstance(data, Iterable) for better adherence to modern Python practices [3][2]. This change caused a regression for objects that use getattr-based delegation (such as file-like wrappers like tqdm.wrapattr), because isinstance only checks the class MRO and ignores attributes resolved via getattr [3][2]. To address this, as of May 2026, the implementation in the main branch of the requests library was updated to use both checks in conjunction to ensure compatibility: if (isinstance(data, Iterable) or hasattr(data, "iter")) and not isinstance(data, (str, bytes, list, tuple, Mapping)): # logic for handling streamed bodies [1][2] This ensures that the library correctly identifies both standard iterables and objects that proxy their iterator interface through dynamic attribute delegation [2][4].
Citations:
- 1: https://github.com/psf/requests/blob/main/src/requests/models.py
- 2: Fix
prepare_bodystream detection for__getattr__-based file wrappers psf/requests#7433 - 3:
prepare_bodystream detection regression psf/requests#7432 - 4: psf/requests@6404f34
🏁 Script executed:
#!/bin/bash
set -euo pipefail
client="$(fd 'client.py' . | head -1)"
test_file="$(fd 'test_garmin_unit.py' . | head -1)"
echo "== full _capture/_restore helper =="
sed -n '189,229p' "$client"
echo "== _run_request relevant section =="
sed -n '1560,1660p' "$client"
echo "== generator test context =="
sed -n '1728,1770p' "$test_file"
echo "== deterministic probe mirroring current helper predicate =="
python3 - <<'PY'
from collections.abc import Iterator, Mapping, Iterable
class OneShotData:
def __iter__(self):
return iter([b"x", b"y"])
def current_capture_returns_none(data):
return data is not None and not hasattr(data, "read") and isinstance(data, Iterator)
print("current_capture_returns_none(generator_function_called)", current_capture_returns_none((lambda: (x for x in []))()))
print("current_capture_returns_none(one_shot_iterable)", current_capture_returns_none(OneShotData()))
print("current_capture_returns_none(generator_function_called)", current_capture_returns_none((lambda: (x for x in []))()))
print("current_capture_returns_none(one_shot_iterable)", current_capture_returns_none(OneShotData()))
class ModernPredicate:
pass
ModernPredicate.__bases__ = ()
def current_like(data):
return data is not None and not hasattr(data, "read") and isinstance(data, Iterator)
def fixed_like(data):
return (
data is not None
and not hasattr(data, "read")
and (isinstance(data, Iterable) or hasattr(data, "iter"))
and not isinstance(data, (str, bytes, list, tuple, Mapping))
)
for body in [(lambda: (x for x in []))(), OneShotData(), iter((b"one", b"two"))]:
print(type(body).__name__, "current=", current_like(body), "fixed=", fixed_like(body))
print(_iter_file_objects.__self__)
PYRepository: cyberjunky/python-garminconnect
Length of output: 7514
Reject one-shot Iterable bodies before retrying.
_capture_file_positions only treats Iterator bodies as non-retryable, but Requests also streams objects that define __iter__ without __next__. Such bodies can be consumed by the first request, allowing the 401 path to retry with an empty iterable. Match the stream check with Iterable/.iter, excluding materialized str, bytes, list, tuple, and Mapping bodies, and add a regression test near test_generator_data_skips_retry_and_raises.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@garminconnect/client.py` around lines 226 - 230, The streamed-body check in
_capture_file_positions currently misses one-shot Iterable objects that
implement __iter__ without __next__. Update it to reject non-file Iterable/.iter
bodies before retrying, while excluding materialized str, bytes, list, tuple,
and Mapping values; preserve existing Iterator handling and add a regression
test alongside test_generator_data_skips_retry_and_raises.
1 participant
Summary
Fixes an externally reported data-integrity flaw (CVSS 4.0 8.2 / High) in the 401 retry path of
_run_request.After a 401,
_run_requestrefreshed the session and retried with the same**kwargs. For uploads (upload_activity,import_activity),kwargs["files"]holds an open file handle thatrequestsreads to EOF while encoding the multipart body on attempt #1. The retry re-read from EOF and sent an empty file part — and the client reported success.Impact
get_api_headers(), dropping the caller's custom headers (NK,origin,User-Agent: GCM-iOS…) thatimport_activitydepends on.Fix
filesdict/list forms, tuple variants, file-likedata) before attempt added Garmin Connect heart rate endpoint #1; rewind them to the caller's original positions (not blindly to 0) before the retry.GarminConnectConnectionError— fail loudly instead of silently uploading an empty body.New helpers:
_iter_file_objects,_capture_file_positions,_restore_file_positions.Tests
tests/test_garmin_unit.py::Test401RetryFileRewind(4 new, all pass):test_retry_rewinds_file_body— both attempts read the full payloadtest_retry_restores_initial_position_not_zero— respects caller's initial offsettest_unseekable_body_skips_retry_and_raises— no blind retry; raisestest_retry_keeps_custom_headers— NK/User-Agent survive the retryNo regressions in the unit suite.
Summary by CodeRabbit
Bug Fixes
Tests