Security/audit 20260810 - #412
Merged
Merged
Conversation
Report #4020: the traversal guard matched literal '..', '?' and '#' in the raw path, but requests' requote_uri() decodes unreserved characters (%2e -> .) after validation, so %2e%2e/%2e%2e/... slipped through and the authenticated request went to a traversed path on the API host. Validate the urllib.parse.unquote() form instead, and reject backslashes. Also fixes call sites broken by the guard's '?' rejection, which embedded query strings in the path instead of using params: get_gear_activities, get_functional_threshold_power_range, and the lactate-threshold range URLs.
Report #4018: the CAS service ticket must travel in a query string on the ticket-consumption fallback (CAS protocol requirement), and requests embeds the full URL in its exception text. That text reached the WARNING log on strategy failure and the 'All login strategies exhausted' error handed to callers — a credential leak precisely when users turn logging up to file a bug report. New _sanitize_exception_text() redacts all query-string values; applied at the strategy-failure and 429 logs, the exhaustion raise, and the DI token exchange fallback log.
Report #4021: sanitize_request() only scrubbed key=value form bodies, but
the credential-posting login strategies send JSON ({"username":...,
"password":...}), so account e-mail and password were recorded verbatim
when re-recording cassettes with real credentials. Parse request bodies
by structure and reuse sanitize_json() so request and response share one
deny-list.
Also: add username, mfaVerificationCode, serviceTicketId, service_ticket,
captchaToken and customerGuid to SENSITIVE_FIELDS; add service_ticket to
SENSITIVE_FORM_PARAMS (form-encoded DI token exchange); scrub
ticket/service_ticket from request.uri (ticket-consumption fallback).
Report #4023: token_file_path() only inspected the last two path components, and O_NOFOLLOW on open() only covers the final component, so a symlink planted higher in the tree (e.g. <home>/cfg/store -> /attacker/dir with tokenstore <home>/cfg/store/sub/.garminconnect) redirected dump()/load()/logout() into an attacker-controlled directory, exposing di_refresh_token on write and allowing tokenstore substitution on read. Walk token_path.parents instead of only the immediate parent. Default ~/.garminconnect is unaffected (already fully covered).
Report #4024: the ROW regex combined re.S with an unanchored .*?, so an element carrying both data-* attributes but no bare <span> captured the next <span> anywhere later in the document — page chrome from the authenticated session (display name, tokens) included — and render() emitted it into the published exercises.py. Split into <li> elements first and match within each; attribute-only elements are now skipped instead of borrowing foreign text. Also: main() refuses to write output whose names look like session data (e-mail, JWT, URL, GUID), and the docstring now tells maintainers to copy only the picker's <ul> and notes the output is published.
Two TestUrlConstruction tests asserted the old inline-query URL form; the path-guard fix moves query parameters to the params kwarg. And the login redaction test must set the garminconnect logger level explicitly because demo.py sets it to CRITICAL at import time (via test_demo_security), which starved caplog in full-suite runs.
Contributor
|
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:
WalkthroughChangesThe pull request separates Garmin query parameters, strengthens path and sensitive-data sanitization, and hardens exercise catalog parsing against malformed or session-derived input. Garmin API request construction
Client security hardening
Exercise catalog input safety
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 196-204: Update every authentication DEBUG log at the cited
exception sites to pass the caught exception through _sanitize_exception_text()
instead of logging it directly, including the ticket-consumption request paths.
Preserve the existing log messages and exception handling while ensuring no raw
exception text is emitted.
- Around line 73-77: Replace the pre-validation loop around token_path with
descriptor-relative traversal that opens and retains each ancestor directory
using no-follow directory checks, then resolves the token file relative to the
final directory descriptor. Apply this protected traversal to both load() and
dump() (and logout() if it accesses the tokenstore), ensuring every directory
component is checked during the actual operation so replacement after validation
cannot redirect access.
In `@scripts/generate_exercises.py`:
- Around line 135-140: Update the suspect-entry guard in
scripts/generate_exercises.py:135-140 to report only non-sensitive diagnostics
such as the count or row indexes, never the extracted labels in suspect. Update
the related test in tests/test_generate_exercises.py:39-47 to capture the
SystemExit value and assert that “user@example.com” is absent from the error
text.
- Around line 32-34: Update the LI parsing in
scripts/generate_exercises.py:32-34 to use an HTML-aware parser or ensure
matches stop before a subsequent <li> start tag, preserving separate item
boundaries when </li> is omitted. Add the regression case in
tests/test_generate_exercises.py:32-36 with an attribute-only unclosed item
followed by an item containing user@example.com, and assert no combined row is
produced.
- Around line 45-50: Update the SUSPECT regular expression compilation to use
re.IGNORECASE so uppercase URL schemes and GUID characters are detected; add
regression cases covering HTTPS URLs and uppercase GUIDs while preserving the
existing safety-gate behavior.
🪄 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: 35d0c0f5-5eb5-4db2-86f5-0ce59773ce30
📒 Files selected for processing (7)
garminconnect/__init__.pygarminconnect/client.pyscripts/generate_exercises.pytests/conftest.pytests/test_cassette_sanitization.pytests/test_garmin_unit.pytests/test_generate_exercises.py
Comment on lines
+73
to
+77
| # Reject symlinks anywhere in the tokenstore ancestry (e.g. | ||
| # ~/.garminconnect -> /attacker/dir). O_NOFOLLOW on the final open() | ||
| # only covers the last component; an intermediate symlinked directory | ||
| # would still redirect load/dump/logout into an attacker-controlled tree. | ||
| for check_path in (token_path, *token_path.parents): |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Prevent ancestor symlink replacement after validation.
The symlink checks complete before dump() and load() open or create files. A local attacker can replace a checked ancestor with a symlink after this loop. O_NOFOLLOW protects only the final file component. It does not protect replaced parent directories.
Use descriptor-relative operations with no-follow checks for every directory component. Apply the same protected path traversal to both reads and writes.
🤖 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 73 - 77, Replace the pre-validation
loop around token_path with descriptor-relative traversal that opens and retains
each ancestor directory using no-follow directory checks, then resolves the
token file relative to the final directory descriptor. Apply this protected
traversal to both load() and dump() (and logout() if it accesses the
tokenstore), ensuring every directory component is checked during the actual
operation so replacement after validation cannot redirect access.
- Sanitize remaining auth-path exception logs (mobile/portal cffi 429 and failure debugs, token-validation, DI refresh, JWT_WEB refresh) through _sanitize_exception_text(). - generate_exercises: replace the element-splitting regex with an HTMLParser-based extractor (item boundaries hold even when </li> is omitted); SUSPECT regex is now case-insensitive (uppercase URL schemes and GUID hex); the refusal message reports row positions only, never the suspected labels. - ruff format. Skipped: descriptor-relative (dir_fd) tokenstore traversal. The ancestry walk plus O_NOFOLLOW on the final component covers the reported model; the residual TOCTOU window needs a local attacker racing a token write mid-operation, which is out of proportion for this library.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 1631-1636: Update the decoded_path validation in _run_request so
safely encoded display-name content remains valid: check raw path input for
literal ? and # before decoding, and replace the broad ".." substring rejection
with segment-based traversal validation that rejects only actual dot-directory
segments. Preserve rejection of genuine path traversal while allowing encoded
names such as name?fragment and first..last.
In `@scripts/generate_exercises.py`:
- Around line 58-64: Update handle_data and the selected-span assembly flow to
accumulate every text fragment, including nested markup, instead of overwriting
self._cur[2]. Reassemble the buffered fragments with "/" before applying SUSPECT
filtering and flushing each row, ensuring complete labels such as URL prefixes
are evaluated and sensitive fragments are not emitted.
- Around line 88-92: Update parse to flush any remaining item after
parser.close(), ensuring a final exercise label without a closing </li> is
included; add a regression test covering omitted final </li>.
🪄 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: e3f98ea6-3ac9-4d46-9461-c1634aa70b64
📒 Files selected for processing (3)
garminconnect/client.pyscripts/generate_exercises.pytests/test_generate_exercises.py
- handle_data() overwrote the captured span text on every call instead of accumulating it, so a label split across nested tags (e.g. a stray https:// URL with part of it inside a <b>) lost its leading fragment and could slip past the SUSPECT safety filter. - parser.close() does not synthesize a missing </li>, so the last item in the source HTML was silently dropped when its closing tag was omitted; flush explicitly after close(). Skipped: the client.py display-name path-validation false positive (display names with literal ?, #, or .. get percent-encoded, then rejected after decoding). Loosening that guard to check raw ?/# and exact-segment ".." would reopen the "foo/%2e%2e;/bar" matrix-parameter bypass the existing test suite guards against, for a false positive that requires characters Garmin display names don't contain in practice.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_generate_exercises.py (1)
53-64: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert that refusal leaves the output file untouched.
This test checks only the
SystemExitmessage. It would still pass ifOUT.write_text()ran before the refusal. Pointgen.OUTat a temporary path and assert that the path does not exist aftergen.main()raises. (raw.githubusercontent.com)🤖 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 `@tests/test_generate_exercises.py` around lines 53 - 64, Update test_main_refuses_suspect_names to redirect gen.OUT to a temporary output path before calling gen.main(), then assert that the path does not exist after SystemExit is raised, while preserving the existing message assertions.scripts/generate_exercises.py (1)
177-185: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport source row locations, not catalog indexes.
parse()deduplicates and sorts entries beforemain()computessuspect, sorows [...reports zero-based positions in the sorted catalog. If the message must point to input rows, preserve source indexes through parsing.🤖 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 `@scripts/generate_exercises.py` around lines 177 - 185, Update parse() and the main() suspect-reporting flow so each parsed exercise retains its original input row index through deduplication and sorting. Use those preserved source indexes in the refusal message’s rows list, while keeping the existing sensitive-name detection and deduplication behavior unchanged.
🤖 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 `@scripts/generate_exercises.py`:
- Around line 59-64: Update the HTML parser’s span tracking to maintain nested
`<span>` depth rather than a boolean `_in_span`; initialize/reset that depth in
`__init__` and adjust span start/end handling so outer-span text continues
accumulating after an inner span closes. Preserve the existing `_cur[2]`
accumulation behavior for all text within the outer span.
---
Outside diff comments:
In `@scripts/generate_exercises.py`:
- Around line 177-185: Update parse() and the main() suspect-reporting flow so
each parsed exercise retains its original input row index through deduplication
and sorting. Use those preserved source indexes in the refusal message’s rows
list, while keeping the existing sensitive-name detection and deduplication
behavior unchanged.
In `@tests/test_generate_exercises.py`:
- Around line 53-64: Update test_main_refuses_suspect_names to redirect gen.OUT
to a temporary output path before calling gen.main(), then assert that the path
does not exist after SystemExit is raised, while preserving the existing message
assertions.
🪄 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: 10afb421-80be-4062-95ca-9f7a6b293bfb
📒 Files selected for processing (2)
scripts/generate_exercises.pytests/test_generate_exercises.py
A plain in/out flag dropped back "out of span" on an inner </span>, so <span><span>Back</span> Squat</span> lost the " Squat" that followed the nested tag but was still inside the outer, selected span. Track nesting depth instead so text keeps accumulating until the matching outer </span>.
A quoted display name can legitimately contain a run of dots (e.g. "first..last"); the old substring match rejected any decoded path containing "..", even inside a single segment. Only reject a path segment that is exactly ".." or "..;<matrix-params>" (a known filter-bypass trick), so real traversal is still caught but a legitimate name is not. ?/#/backslash checks are unchanged: they still run against the decoded path, since callers only ever reach those characters raw or via percent-encoding by embedding a query string in the path, which the existing test suite already covers as a rejected case.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_garmin_unit.py (1)
241-242: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winVerify the clean path in the weekly aggregation test.
These assertions verify two parameter values, but they do not verify the path passed to
connectapi. A regression that embedsaggregation=weeklyin the path could still pass. Assert the call count and the exact path, then keep the parameter assertions.Proposed test update
+ mock.assert_called_once() + assert mock.call_args.args == ( + "/biometric-service/stats/functionalThresholdPower/range/2025-06-01/2025-06-30", + ) assert mock.call_args.kwargs["params"]["sport"] == "RUNNING" assert mock.call_args.kwargs["params"]["aggregation"] == "weekly"🤖 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 `@tests/test_garmin_unit.py` around lines 241 - 242, Update the weekly aggregation test around the existing mock call assertions to verify connectapi was called exactly once with the clean expected path, then retain the sport and aggregation parameter assertions. Use the path value already expected by the test’s request setup rather than allowing aggregation to be embedded in the path.garminconnect/client.py (1)
193-206: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSanitize all authentication failure outputs.
- Do not log the full MFA response at
client.py:1221.- Do not log raw DI response text at
client.py:1329.- Catch fallback request errors at
client.py:1258and raiseGarminConnectConnectionError(_sanitize_exception_text(err)) from None, becauseresume_login()can otherwise propagate a ticket-bearing exception.🤖 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 193 - 206, Sanitize every authentication failure output: in the MFA handling near the login flow, replace full-response logging with a non-sensitive summary; in the DI response handling, avoid logging raw response text; and in the fallback request path used by resume_login(), catch request exceptions and raise GarminConnectConnectionError using _sanitize_exception_text(err) from None so ticket-bearing details are redacted.
🤖 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.
Outside diff comments:
In `@garminconnect/client.py`:
- Around line 193-206: Sanitize every authentication failure output: in the MFA
handling near the login flow, replace full-response logging with a non-sensitive
summary; in the DI response handling, avoid logging raw response text; and in
the fallback request path used by resume_login(), catch request exceptions and
raise GarminConnectConnectionError using _sanitize_exception_text(err) from None
so ticket-bearing details are redacted.
In `@tests/test_garmin_unit.py`:
- Around line 241-242: Update the weekly aggregation test around the existing
mock call assertions to verify connectapi was called exactly once with the clean
expected path, then retain the sport and aggregation parameter assertions. Use
the path value already expected by the test’s request setup rather than allowing
aggregation to be embedded in the path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1991c350-f665-4373-8d53-6ab8e53d7337
📒 Files selected for processing (2)
garminconnect/client.pytests/test_garmin_unit.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Tests