security: validate JWT exp claim to prevent client DoS - #408
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe JWT expiry check validates ChangesJWT expiry validation
Estimated code review effort: 2 (Simple) | ~10 minutes 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 |
![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: 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 1316-1323: Update _token_expires_soon() to reject boolean exp
values before conversion, catch OverflowError alongside TypeError and ValueError
from float(), and return False for non-finite results using an appropriate
finiteness check. Preserve the existing expiry comparison for valid finite
numeric values.
🪄 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: 2b9fe587-70c3-467b-aa0b-235524e1528d
📒 Files selected for processing (2)
garminconnect/client.pytests/test_garmin_unit.py
cyberjunky
changed the title
cyberjunky
force-pushed
the
security/jwt-exp-validation
branch
3 times, most recently
from
August 10, 2026 12:22
0f39ab1 to
ac24eb4
Compare
cyberjunky
force-pushed
the
security/jwt-exp-validation
branch
from
August 10, 2026 12:27
ac24eb4 to
5475ea2
Compare
Contributor
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 `@tests/test_garmin_unit.py`:
- Around line 1750-1769: Extend test_token_expires_soon_survives_malformed_exp
with boolean values and non-finite numeric strings such as "-inf", asserting
_token_expires_soon() remains False; alternatively, update
Client._token_expires_soon to reject booleans and non-finite expiry values
before comparison.
🪄 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: 03a875bb-34b5-4b5e-82c0-857679fc1d77
📒 Files selected for processing (1)
tests/test_garmin_unit.py
cyberjunky
force-pushed
the
security/jwt-exp-validation
branch
from
August 10, 2026 13:06
5475ea2 to
5528e38
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
garminconnect/client.py (2)
1317-1324:⚠️ Potential issue | 🟠 MajorReapply the unresolved non-finite and overflow guards.
float()still acceptsTrueandFalse, accepts values such as"-inf", and raisesOverflowErrorfor a very large JSON integer. These values can force repeated refresh attempts or abort_run_request(). Reject booleans, catchOverflowError, and requiremath.isfinite(exp). This repeats the previous review finding.Proposed fix
+import math + + raw_exp = payload.get("exp") + if isinstance(raw_exp, bool): + return False try: - exp = float(payload.get("exp")) # type: ignore[arg-type] - except (TypeError, ValueError): + exp = float(raw_exp) + except (TypeError, ValueError, OverflowError): return False + if not math.isfinite(exp): + return False#!/bin/bash set -euo pipefail python - <<'PY' import math for value in (True, False, "-inf", "inf", "nan", 10**400): try: parsed = float(value) except Exception as exc: print(repr(value), type(exc).__name__) else: print(repr(value), parsed, math.isfinite(parsed)) PY🤖 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 1317 - 1324, Update the expiration parsing in the JWT validation logic around payload.get("exp") to reject boolean values, catch OverflowError alongside TypeError and ValueError, and return False for non-finite results using math.isfinite(exp). Preserve the existing refresh-threshold comparison only for finite numeric expiration values.
1317-1324:⚠️ Potential issue | 🟠 MajorKeep malformed
exphandling and regression coverage consistent.float()still accepts booleans and non-finite strings and can overflow on very large JSON integers.
garminconnect/client.py#L1317-L1324: reject booleans, catchOverflowError, and reject non-finite values withmath.isfinite.tests/test_garmin_unit.py#L1752-L1771: addTrue,False,"-inf", and10**400cases.🤖 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 1317 - 1324, Update the JWT expiration parsing in garminconnect/client.py lines 1317-1324 to reject boolean values before conversion, catch OverflowError alongside TypeError and ValueError, and return False for non-finite results using math.isfinite. Extend the expiration regression tests in tests/test_garmin_unit.py lines 1752-1771 with True, False, "-inf", and 10**400 cases, verifying each malformed value is rejected without raising.tests/test_garmin_unit.py (1)
1752-1771:⚠️ Potential issue | 🟡 MinorAdd regression cases for values that
float()accepts or overflows on.The current parameter list covers
TypeErrorandValueError, but not boolean coercion, non-finite values, orOverflowError. AddTrue,False,"-inf", and10**400so the unresolved paths ingarminconnect/client.pyremain covered. This repeats the previous review finding.Proposed test cases
[ + True, + False, + "-inf", + 10**400, "not-a-number",🤖 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 1752 - 1771, Extend the parameter list in test_token_expires_soon_survives_malformed_exp with True, False, "-inf", and 10**400, preserving the existing assertion that _token_expires_soon() returns False for each malformed expiration value.
🤖 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.
Duplicate comments:
In `@garminconnect/client.py`:
- Around line 1317-1324: Update the expiration parsing in the JWT validation
logic around payload.get("exp") to reject boolean values, catch OverflowError
alongside TypeError and ValueError, and return False for non-finite results
using math.isfinite(exp). Preserve the existing refresh-threshold comparison
only for finite numeric expiration values.
- Around line 1317-1324: Update the JWT expiration parsing in
garminconnect/client.py lines 1317-1324 to reject boolean values before
conversion, catch OverflowError alongside TypeError and ValueError, and return
False for non-finite results using math.isfinite. Extend the expiration
regression tests in tests/test_garmin_unit.py lines 1752-1771 with True, False,
"-inf", and 10**400 cases, verifying each malformed value is rejected without
raising.
In `@tests/test_garmin_unit.py`:
- Around line 1752-1771: Extend the parameter list in
test_token_expires_soon_survives_malformed_exp with True, False, "-inf", and
10**400, preserving the existing assertion that _token_expires_soon() returns
False for each malformed expiration value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0ce0c394-bdcc-46e3-a36b-97cf39bdb77d
📒 Files selected for processing (2)
garminconnect/client.pytests/test_garmin_unit.py
_token_expires_soon() called int(exp) on the 'exp' claim from an unverified, server-controlled JWT payload with no type guard. A hostile or MITM server returning a non-numeric exp (string -> ValueError, or a non-empty container -> TypeError) crashed every _run_request call before any HTTP request. The raw exception is not a GarminConnect* subclass, and the poisoned token is persisted by dump()/load(), so a single crafted response makes the client unusable across restarts (report 3997). Coerce exp with float() inside try/except, returning False (treat as not expiring soon) when it cannot be parsed. This also supports legitimate numeric-string exp values. Adds parametrized tests covering all PoC payloads.
cyberjunky
force-pushed
the
security/jwt-exp-validation
branch
from
August 10, 2026 13:18
5528e38 to
c8c83d0
Compare
Addresses CodeRabbit review on PR #408: float() coercion still accepted booleans (True -> 1.0, always 'expiring soon'), non-finite strings like 'inf' (repeated refresh attempts), and raised uncaught OverflowError for huge JSON integers -- the same crash-before-every-request DoS class the original fix targeted.
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
Fixes a client-side denial-of-service reported externally (CVSS 4.0 8.2 / High).
_token_expires_soon()calledint(exp)on theexpclaim taken directly from_decode_jwt_payload()— an unverified, server-controlled JWT payload — with no type guard anywhere in the chain.int(exp)raises:ValueErrorfor a non-numeric string (e.g."not-a-number")TypeErrorfor a non-empty container (e.g.{"a": 1},[1])Impact
access_tokenpoisonsself.di_token. Every public method routed through_run_request(get/post/download/connectapi/…) then raised the raw exception before any HTTP request.GarminConnect*subclass, so callers catching by library type don't handle it — can crash the host app.dump()/ restored byload(), so a single crafted response leaves the client unusable across restarts until the token file is manually deleted.Fix
Coerce
expwithfloat()inside atry/except (TypeError, ValueError), returningFalse(treat as "not expiring soon", client stays usable) when it can't be parsed. This also transparently supports legitimate numeric-stringexpvalues. A genuinely expired token is still caught by the normal 401 path.Tests
Added to
tests/test_garmin_unit.py::TestJwtHandling:test_token_expires_soon_accepts_numeric_string_exptest_token_expires_soon_survives_malformed_exp— parametrized over"not-a-number",{"a":1},[1],None,"",{},[]15/15 JWT tests pass; no regressions in the unit suite.
Summary by CodeRabbit