fix: add delay between SSO page load and login POST to avoid Cloudfla…#346
Open
cyberjunky wants to merge 8 commits intocyberjunky:masterfrom
Open
fix: add delay between SSO page load and login POST to avoid Cloudfla…#346cyberjunky wants to merge 8 commits intocyberjunky:masterfrom
cyberjunky wants to merge 8 commits intocyberjunky:masterfrom
Conversation
…re 429 Garmin's Cloudflare WAF rate-limits requests that go directly from the SSO page GET to the login POST without intervening activity. A random 30-45s delay mimics natural browser behavior and consistently avoids the 429 block. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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:
WalkthroughAdds an optional headless browser login flow (browser-based MFA support), randomizes a 30–45s pause in portal web login, persists Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client
participant Browser
participant GarminAPI
participant TokenStore
User->>Client: initiate login
alt browser strategy available
Client->>Browser: launch headless browser, navigate SSO
Browser->>GarminAPI: GET sign-in (handle Cloudflare)
Browser->>Browser: in-page POST credentials
alt MFA required
Browser->>User: prompt for MFA
User->>Browser: provide MFA code
Browser->>GarminAPI: submit MFA
end
GarminAPI-->>Browser: return web tokens (JWT_WEB, csrf_token)
Browser-->>Client: deliver tokens
Client->>TokenStore: dumps(jwt_web, csrf_token, other tokens)
Client->>Browser: close
else HTTP portal flow
Client->>GarminAPI: GET sign-in
Note right of Client: sleep(random.uniform(30,45))
Client->>GarminAPI: POST credentials
alt MFA required
Client->>User: prompt for MFA
User->>Client: provide MFA code
Client->>GarminAPI: submit MFA
end
GarminAPI-->>Client: return web tokens (JWT_WEB, csrf_token)
Client->>TokenStore: dumps(jwt_web, csrf_token, other tokens)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@garminconnect/client.py`:
- Line 306: The S311 warning flags use of random.uniform for cryptographic
purposes but this sleep jitter is non-cryptographic pacing; suppress it by
adding an explicit noqa for S311 on the line that calls sleep(random.uniform(30,
45)) so that the linter knows this use is intentional (refer to the call to
sleep and random.uniform in the login pacing logic) and include a short inline
comment stating it's non-cryptographic jitter.
- Around line 2-3: Reorder the standard-library imports in
garminconnect/client.py to satisfy Ruff I001: ensure the stdlib group is sorted
alphabetically and properly grouped (place the imports for the random module and
the time module in alphabetical order and separate groups with a single blank
line); specifically adjust the import lines that reference random and sleep (the
import random and from time import sleep statements) so they follow PEP 8 import
ordering.
🪄 Autofix (Beta)
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
Run ID: 0c454442-2ee1-4b0f-b028-2a4ce1e8bfb6
📒 Files selected for processing (1)
garminconnect/client.py
|
This random delay is the only fix that has solved the 429 issue for me and has been consistantly working. |
… session import Extend Client.dumps() and Client.loads() to serialize and restore jwt_web and csrf_token fields alongside DI tokens. This enables token persistence for JWT_WEB-based auth sessions. Add Client.load_givemydata_session() to import JWT_WEB directly from garmin-givemydata's garmin_session.json cookie format. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@garminconnect/client.py`:
- Around line 956-959: The loads() method currently only assigns self.jwt_web
and self.csrf_token when those keys exist, which leaves prior values intact;
update loads() to explicitly set these attributes to the new state (e.g.,
self.jwt_web = data.get("jwt_web") and self.csrf_token = data.get("csrf_token")
or use "jwt_web" in data ? data["jwt_web"] : None) so missing keys clear
existing values instead of leaving stale tokens; modify the assignments for
self.jwt_web and self.csrf_token in loads() accordingly.
- Around line 967-985: The load_givemydata_session method doesn't clear existing
DI auth, causing get_api_headers to still prefer self.di_token; update
load_givemydata_session (in class where self.jwt_web is set) to clear any DI
auth state before assigning jwt_web—specifically set self.di_token = None (and
clear related DI flags/fields if present) so get_api_headers will use the new
JWT_WEB token instead of stale DI Bearer auth.
🪄 Autofix (Beta)
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
Run ID: 96e3e916-4ada-4a38-b155-937d85bf843c
📒 Files selected for processing (1)
garminconnect/client.py
garminconnect/client.py
Outdated
Comment on lines
+956
to
+959
| if data.get("jwt_web"): | ||
| self.jwt_web = data["jwt_web"] | ||
| if data.get("csrf_token"): | ||
| self.csrf_token = data["csrf_token"] |
Contributor
There was a problem hiding this comment.
loads() may keep stale jwt_web/csrf_token from previous state.
Because assignment is conditional, missing keys do not clear existing values. A prior in-memory token can survive a new load unintentionally.
Suggested fix
def loads(self, tokenstore: str) -> None:
try:
data = json.loads(tokenstore)
self.di_token = data.get("di_token")
self.di_refresh_token = data.get("di_refresh_token")
self.di_client_id = data.get("di_client_id")
- if data.get("jwt_web"):
- self.jwt_web = data["jwt_web"]
- if data.get("csrf_token"):
- self.csrf_token = data["csrf_token"]
+ self.jwt_web = data.get("jwt_web")
+ self.csrf_token = data.get("csrf_token")
if not self.is_authenticated:
raise GarminConnectAuthenticationError("Missing tokens from dict load")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if data.get("jwt_web"): | |
| self.jwt_web = data["jwt_web"] | |
| if data.get("csrf_token"): | |
| self.csrf_token = data["csrf_token"] | |
| def loads(self, tokenstore: str) -> None: | |
| try: | |
| data = json.loads(tokenstore) | |
| self.di_token = data.get("di_token") | |
| self.di_refresh_token = data.get("di_refresh_token") | |
| self.di_client_id = data.get("di_client_id") | |
| self.jwt_web = data.get("jwt_web") | |
| self.csrf_token = data.get("csrf_token") | |
| if not self.is_authenticated: | |
| raise GarminConnectAuthenticationError("Missing tokens from dict load") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@garminconnect/client.py` around lines 956 - 959, The loads() method currently
only assigns self.jwt_web and self.csrf_token when those keys exist, which
leaves prior values intact; update loads() to explicitly set these attributes to
the new state (e.g., self.jwt_web = data.get("jwt_web") and self.csrf_token =
data.get("csrf_token") or use "jwt_web" in data ? data["jwt_web"] : None) so
missing keys clear existing values instead of leaving stale tokens; modify the
assignments for self.jwt_web and self.csrf_token in loads() accordingly.
garminconnect/client.py
Outdated
Comment on lines
+967
to
+985
| def load_givemydata_session(self, session_json: str) -> None: | ||
| """Import auth from a garmin-givemydata session JSON string. | ||
|
|
||
| Extracts the JWT_WEB cookie from the garmin-givemydata | ||
| garmin_session.json format and sets it as the active auth token. | ||
| """ | ||
| data = json.loads(session_json) | ||
| cookies = data.get("cookies", []) | ||
| jwt_web = None | ||
| for cookie in cookies: | ||
| if cookie.get("name") == "JWT_WEB": | ||
| jwt_web = cookie["value"] | ||
| break | ||
| if not jwt_web: | ||
| raise GarminConnectAuthenticationError( | ||
| "No JWT_WEB cookie found in garmin-givemydata session" | ||
| ) | ||
| self.jwt_web = jwt_web | ||
|
|
Contributor
There was a problem hiding this comment.
load_givemydata_session() should reset DI auth state before switching to JWT_WEB.
If self.di_token is already set, get_api_headers() prefers DI Bearer auth and may ignore the imported JWT_WEB, producing mixed/stale auth behavior.
Suggested fix
def load_givemydata_session(self, session_json: str) -> None:
@@
if not jwt_web:
raise GarminConnectAuthenticationError(
"No JWT_WEB cookie found in garmin-givemydata session"
)
+ # Switch auth mode explicitly to JWT_WEB-based session.
+ self.di_token = None
+ self.di_refresh_token = None
+ self.di_client_id = None
+ self.csrf_token = None
self.jwt_web = jwt_web🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@garminconnect/client.py` around lines 967 - 985, The load_givemydata_session
method doesn't clear existing DI auth, causing get_api_headers to still prefer
self.di_token; update load_givemydata_session (in class where self.jwt_web is
set) to clear any DI auth state before assigning jwt_web—specifically set
self.di_token = None (and clear related DI flags/fields if present) so
get_api_headers will use the new JWT_WEB token instead of stale DI Bearer auth.
Add browser+selenium as the first login strategy when garmin-givemydata is installed. Uses SeleniumBase UC Chrome to bypass Cloudflare bot detection, falling through to HTTP-based strategies if unavailable. The browser strategy: - Launches headless Chrome, fills SSO credentials, handles redirects - Supports return_on_mfa for two-step MFA flows (config flow UIs) - Extracts JWT_WEB + CSRF tokens from the browser session - Closes the browser after token extraction resume_login() now handles browser-based MFA completion by submitting the code to the live browser and extracting tokens on success. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
garminconnect/client.py (3)
1027-1036:⚠️ Potential issue | 🟠 MajorReset web-auth fields when the token blob omits them.
The conditional assignments at Lines 1033-1036 keep any previous
jwt_web/csrf_tokenin memory. Loading a DI-only or partial token store can silently reuse stale web auth.Suggested diff
- if data.get("jwt_web"): - self.jwt_web = data["jwt_web"] - if data.get("csrf_token"): - self.csrf_token = data["csrf_token"] + self.jwt_web = data.get("jwt_web") + self.csrf_token = data.get("csrf_token")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@garminconnect/client.py` around lines 1027 - 1036, In the loads method, missing keys for web-auth are left unchanged causing stale jwt_web/csrf_token reuse; update garminconnect.client.Client.loads to explicitly clear/reset self.jwt_web and self.csrf_token when the loaded JSON does not contain "jwt_web" or "csrf_token" (e.g., set to None or an empty value) instead of only assigning them conditionally, while keeping existing assignments for di_token/di_refresh_token/di_client_id as-is.
288-289:⚠️ Potential issue | 🟠 MajorClear DI auth before switching the client to
JWT_WEBmode.These paths only overwrite
jwt_web/csrf_token. If a staledi_tokenis still present,get_api_headers()keeps preferring Bearer auth and ignores the new browser/imported session;load_givemydata_session()can also keep an unrelated oldcsrf_token.Suggested direction
+ def _set_jwt_web_auth( + self, jwt_web: str, csrf_token: str | None = None + ) -> None: + self.di_token = None + self.di_refresh_token = None + self.di_client_id = None + self.jwt_web = jwt_web + self.csrf_token = csrf_tokenUse that helper from
_browser_login(),load_givemydata_session(), and the browser branch ofresume_login().Also applies to: 1044-1062, 1103-1106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@garminconnect/client.py` around lines 288 - 289, The code is only overwriting jwt_web/csrf_token but not clearing DI auth, causing get_api_headers() to still prefer Bearer (di_token); update the blocks that set self.jwt_web and self.csrf_token (including the ones in _browser_login(), load_givemydata_session(), and the browser branch of resume_login()) to also clear DI-related state (e.g., remove/None-out self.di_token and any DI auth flags) by calling the existing helper used in _browser_login() for clearing DI auth before assigning the new JWT_WEB values so get_api_headers() will switch to JWT_WEB/broswer session correctly.
383-383:⚠️ Potential issue | 🟠 MajorSuppress Ruff
S311on this intentional pacing jitter.Ruff will keep flagging Line 383 until the non-cryptographic delay is marked as intentional.
Suggested diff
- sleep(random.uniform(30, 45)) + sleep(random.uniform(30, 45)) # noqa: S311 - non-cryptographic pacing jitter🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@garminconnect/client.py` at line 383, Add a Ruff suppression for the intentional non-cryptographic jitter by appending a noqa disable for S311 to the sleep call (i.e., change the call "sleep(random.uniform(30, 45))" to include " # noqa: S311"); ensure the comment sits on the same line as the sleep invocation so Ruff ignores this S311 warning for the intentional pacing jitter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@garminconnect/client.py`:
- Around line 284-286: The browser client (gc) is closed only after calling
gc.get_auth_tokens(), which can raise and leak the headless browser; wrap the
token extraction and close in a try/finally so gc.close() always runs. Locate
the block around gc.get_auth_tokens() and gc.close() and change it so you call
gc.get_auth_tokens() inside a try and invoke gc.close() in the finally (or use a
context manager if available on the client) to guarantee cleanup even on
exceptions.
- Around line 1093-1102: The code clears self._browser_client before attempting
MFA which makes browser MFA single-shot; instead, keep self._browser_client
intact until the MFA attempt succeeds or you decide to fully abandon the browser
session. Change the flow in the block that references self._browser_client and
submit_mfa to: capture gc = self._browser_client into a local variable without
setting self._browser_client = None up front, call result =
gc.submit_mfa(mfa_code), and only set self._browser_client = None when you
explicitly close the browser (e.g., after gc.close() on a definitive failure) or
after a successful verification; ensure you still raise
GarminConnectAuthenticationError on permanent failure and close gc before
clearing the attribute so transient failures allow retry.
---
Duplicate comments:
In `@garminconnect/client.py`:
- Around line 1027-1036: In the loads method, missing keys for web-auth are left
unchanged causing stale jwt_web/csrf_token reuse; update
garminconnect.client.Client.loads to explicitly clear/reset self.jwt_web and
self.csrf_token when the loaded JSON does not contain "jwt_web" or "csrf_token"
(e.g., set to None or an empty value) instead of only assigning them
conditionally, while keeping existing assignments for
di_token/di_refresh_token/di_client_id as-is.
- Around line 288-289: The code is only overwriting jwt_web/csrf_token but not
clearing DI auth, causing get_api_headers() to still prefer Bearer (di_token);
update the blocks that set self.jwt_web and self.csrf_token (including the ones
in _browser_login(), load_givemydata_session(), and the browser branch of
resume_login()) to also clear DI-related state (e.g., remove/None-out
self.di_token and any DI auth flags) by calling the existing helper used in
_browser_login() for clearing DI auth before assigning the new JWT_WEB values so
get_api_headers() will switch to JWT_WEB/broswer session correctly.
- Line 383: Add a Ruff suppression for the intentional non-cryptographic jitter
by appending a noqa disable for S311 to the sleep call (i.e., change the call
"sleep(random.uniform(30, 45))" to include " # noqa: S311"); ensure the comment
sits on the same line as the sleep invocation so Ruff ignores this S311 warning
for the intentional pacing jitter.
🪄 Autofix (Beta)
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
Run ID: 843909b6-8416-4a05-9a06-ec4747c77c85
📒 Files selected for processing (1)
garminconnect/client.py
garminconnect/client.py
Outdated
Comment on lines
+284
to
+286
| # Extract tokens from the browser session | ||
| tokens = gc.get_auth_tokens() | ||
| gc.close() |
Contributor
There was a problem hiding this comment.
Close the browser client in a finally block.
If Line 285 raises, Line 286 is skipped and the headless browser process leaks.
Suggested diff
- tokens = gc.get_auth_tokens()
- gc.close()
+ try:
+ tokens = gc.get_auth_tokens()
+ finally:
+ gc.close()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Extract tokens from the browser session | |
| tokens = gc.get_auth_tokens() | |
| gc.close() | |
| # Extract tokens from the browser session | |
| try: | |
| tokens = gc.get_auth_tokens() | |
| finally: | |
| gc.close() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@garminconnect/client.py` around lines 284 - 286, The browser client (gc) is
closed only after calling gc.get_auth_tokens(), which can raise and leak the
headless browser; wrap the token extraction and close in a try/finally so
gc.close() always runs. Locate the block around gc.get_auth_tokens() and
gc.close() and change it so you call gc.get_auth_tokens() inside a try and
invoke gc.close() in the finally (or use a context manager if available on the
client) to guarantee cleanup even on exceptions.
The len() > 512 heuristic failed for short JWT tokens (~300 chars),
causing dumps() output to be treated as a file path instead of a JSON
string. Now checks if the string starts with '{' to distinguish JSON
from file paths.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@garmin_auth_test.py`:
- Around line 256-263: The try/except around the csrf_token extraction (the
block calling page.evaluate and logging via log.info(" CSRF token: %s",
csrf_token)) is using a bare except which can hide real errors; change it to
catch and handle specific exceptions (e.g., PlaywrightError or the
library-specific exception) or catch Exception as e and call log.debug with the
exception info (including e) so failures are visible in debug output rather than
silently swallowed—update the try/except that wraps page.evaluate and the
csrf_token handling accordingly.
- Around line 492-504: The token dump block only writes di_token,
di_refresh_token and di_client_id, but Client.dumps()/Client.loads() expect
jwt_web and csrf_token as well; update the token_data created when
args.dump_tokens is set to include "jwt_web": jwt_web and "csrf_token":
csrf_token (or their local variable names used in this module) so the dumped
JSON matches the five-field shape produced by Client.dumps() and can be reloaded
by Client.loads().
🪄 Autofix (Beta)
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
Run ID: 74791c9f-ff5b-4940-865f-a19755a1afa6
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
garmin_auth_test.pygarminconnect/__init__.py
Comment on lines
+256
to
+263
| try: | ||
| csrf_token = page.evaluate( | ||
| '() => document.querySelector(\'meta[name="csrf-token"]\')?.content' | ||
| ) | ||
| if csrf_token: | ||
| log.info(" CSRF token: %s", csrf_token) | ||
| except Exception: | ||
| pass |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider narrowing the exception catch for easier debugging.
The bare except Exception: pass could silently swallow unexpected errors (e.g., Playwright connection issues), making debugging harder. Since this is a test script, consider logging the exception at debug level.
🔧 Suggested improvement
try:
csrf_token = page.evaluate(
'() => document.querySelector(\'meta[name="csrf-token"]\')?.content'
)
if csrf_token:
log.info(" CSRF token: %s", csrf_token)
- except Exception:
- pass
+ except Exception as e:
+ log.debug(" CSRF extraction failed: %s", e)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try: | |
| csrf_token = page.evaluate( | |
| '() => document.querySelector(\'meta[name="csrf-token"]\')?.content' | |
| ) | |
| if csrf_token: | |
| log.info(" CSRF token: %s", csrf_token) | |
| except Exception: | |
| pass | |
| try: | |
| csrf_token = page.evaluate( | |
| '() => document.querySelector(\'meta[name="csrf-token"]\')?.content' | |
| ) | |
| if csrf_token: | |
| log.info(" CSRF token: %s", csrf_token) | |
| except Exception as e: | |
| log.debug(" CSRF extraction failed: %s", e) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@garmin_auth_test.py` around lines 256 - 263, The try/except around the
csrf_token extraction (the block calling page.evaluate and logging via
log.info(" CSRF token: %s", csrf_token)) is using a bare except which can hide
real errors; change it to catch and handle specific exceptions (e.g.,
PlaywrightError or the library-specific exception) or catch Exception as e and
call log.debug with the exception info (including e) so failures are visible in
debug output rather than silently swallowed—update the try/except that wraps
page.evaluate and the csrf_token handling accordingly.
Comment on lines
+492
to
+504
| if args.dump_tokens and di_token: | ||
| token_data = { | ||
| "di_token": di_token, | ||
| "di_refresh_token": di_refresh, | ||
| "di_client_id": di_client_id, | ||
| } | ||
| with open(args.dump_tokens, "w") as f: | ||
| json.dump(token_data, f, indent=2) | ||
| log.info("Tokens written to %s", args.dump_tokens) | ||
| log.info( | ||
| "Load in garminconnect: api.client.loads(open('%s').read())", | ||
| args.dump_tokens, | ||
| ) |
Contributor
There was a problem hiding this comment.
Token dump is missing jwt_web and csrf_token fields.
The dumped JSON only includes 3 fields, but Client.dumps() (see garminconnect/client.py:996-1005) writes 5 fields including jwt_web and csrf_token. If users rely on --dump-tokens to persist session state for later use with Client.loads(), the web session tokens will be lost.
🔧 Proposed fix to include all token fields
if args.dump_tokens and di_token:
token_data = {
"di_token": di_token,
"di_refresh_token": di_refresh,
"di_client_id": di_client_id,
+ "jwt_web": jwt_web,
+ "csrf_token": csrf_token,
}
with open(args.dump_tokens, "w") as f:
json.dump(token_data, f, indent=2)
log.info("Tokens written to %s", args.dump_tokens)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if args.dump_tokens and di_token: | |
| token_data = { | |
| "di_token": di_token, | |
| "di_refresh_token": di_refresh, | |
| "di_client_id": di_client_id, | |
| } | |
| with open(args.dump_tokens, "w") as f: | |
| json.dump(token_data, f, indent=2) | |
| log.info("Tokens written to %s", args.dump_tokens) | |
| log.info( | |
| "Load in garminconnect: api.client.loads(open('%s').read())", | |
| args.dump_tokens, | |
| ) | |
| if args.dump_tokens and di_token: | |
| token_data = { | |
| "di_token": di_token, | |
| "di_refresh_token": di_refresh, | |
| "di_client_id": di_client_id, | |
| "jwt_web": jwt_web, | |
| "csrf_token": csrf_token, | |
| } | |
| with open(args.dump_tokens, "w") as f: | |
| json.dump(token_data, f, indent=2) | |
| log.info("Tokens written to %s", args.dump_tokens) | |
| log.info( | |
| "Load in garminconnect: api.client.loads(open('%s').read())", | |
| args.dump_tokens, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@garmin_auth_test.py` around lines 492 - 504, The token dump block only writes
di_token, di_refresh_token and di_client_id, but Client.dumps()/Client.loads()
expect jwt_web and csrf_token as well; update the token_data created when
args.dump_tokens is set to include "jwt_web": jwt_web and "csrf_token":
csrf_token (or their local variable names used in this module) so the dumped
JSON matches the five-field shape produced by Client.dumps() and can be reloaded
by Client.loads().
When proxy_url is set, all API calls are routed through the garmin-givemydata add-on's /api/fetch endpoint, which executes requests via Chrome's JavaScript fetch() context to bypass Cloudflare protection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
garminconnect/client.py (5)
1046-1063:⚠️ Potential issue | 🟠 MajorClear DI auth state before switching to JWT_WEB mode.
If
self.di_tokenis already set,get_api_headers()prefers DI Bearer auth and may ignore the importedJWT_WEB, producing mixed/stale auth behavior.Suggested fix
if not jwt_web: raise GarminConnectAuthenticationError( "No JWT_WEB cookie found in garmin-givemydata session" ) + # Switch auth mode explicitly to JWT_WEB-based session + self.di_token = None + self.di_refresh_token = None + self.di_client_id = None + self.csrf_token = None self.jwt_web = jwt_web🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@garminconnect/client.py` around lines 1046 - 1063, The load_givemydata_session method must clear any existing DI auth so JWT_WEB becomes the active auth; update load_givemydata_session (which currently sets self.jwt_web) to also unset/clear self.di_token (and any related DI auth flags) before returning so get_api_headers will not prefer stale DI Bearer credentials over the imported JWT_WEB token.
1094-1104:⚠️ Potential issue | 🟠 MajorDon't clear
_browser_clientbefore MFA verification.Line 1097 clears
_browser_clientbefore the first verification attempt. A typo or transient failure now forces a full re-login, while the HTTP MFA paths keep their session state for retries.Suggested fix
if self._browser_client is not None: gc = self._browser_client - self._browser_client = None try: result = gc.submit_mfa(mfa_code) if not result: + self._browser_client = None gc.close() raise GarminConnectAuthenticationError( "Browser MFA verification failed" ) tokens = gc.get_auth_tokens() + self._browser_client = None gc.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@garminconnect/client.py` around lines 1094 - 1104, The code currently sets self._browser_client = None before attempting browser MFA, which loses the session on a transient failure; instead, preserve self._browser_client until MFA succeeds: fetch gc = self._browser_client without clearing it, call gc.submit_mfa(mfa_code), and only clear self._browser_client (or close gc) after a successful verification or when explicitly aborting; on a failed submit_mfa keep the browser client intact and call gc.close() only if you intend to discard the session, and raise GarminConnectAuthenticationError as before if verification fails.
385-385:⚠️ Potential issue | 🟡 MinorAdd
noqa: S311comment for non-cryptographic pacing jitter.This line triggers Ruff S311 (pseudo-random unsuitable for cryptographic purposes). Since this is behavioral login delay pacing, not security-related randomness, add an explicit suppression comment.
Suggested fix
- sleep(random.uniform(30, 45)) + sleep(random.uniform(30, 45)) # noqa: S311 - non-cryptographic pacing jitter🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@garminconnect/client.py` at line 385, Add an explicit Ruff suppression for S311 on the non-cryptographic jitter call: update the line calling sleep(random.uniform(30, 45)) to append a trailing comment like "# noqa: S311" (optionally with a short rationale, e.g., "# noqa: S311 # non-cryptographic pacing jitter") so the linter knows this pseudo-random usage is intentional; the affected expression is the call to sleep(random.uniform(30, 45)) in garminconnect.client.py.
286-288:⚠️ Potential issue | 🟠 MajorWrap token extraction in try/finally to prevent browser process leak.
If
gc.get_auth_tokens()raises an exception,gc.close()is skipped and the headless browser process leaks.Suggested fix
# Extract tokens from the browser session - tokens = gc.get_auth_tokens() - gc.close() + try: + tokens = gc.get_auth_tokens() + finally: + gc.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@garminconnect/client.py` around lines 286 - 288, The call to gc.get_auth_tokens() can raise and currently prevents gc.close() from running, leaking the headless browser; wrap the extraction in a try/finally: call tokens = gc.get_auth_tokens() inside a try block and ensure gc.close() is invoked in the finally block (re-raise or return tokens after the finally), so gc.close() always runs even on exceptions.
1035-1038:⚠️ Potential issue | 🟠 Major
loads()may preserve stalejwt_web/csrf_tokenfrom prior state.Conditional assignment means missing keys don't clear existing values. If a prior in-memory token exists and the new tokenstore omits these keys, stale tokens survive unintentionally.
Suggested fix
- if data.get("jwt_web"): - self.jwt_web = data["jwt_web"] - if data.get("csrf_token"): - self.csrf_token = data["csrf_token"] + self.jwt_web = data.get("jwt_web") + self.csrf_token = data.get("csrf_token")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@garminconnect/client.py` around lines 1035 - 1038, In loads(), avoid preserving stale tokens by explicitly clearing jwt_web and csrf_token when they're absent from the provided data: update the logic in the loads method so that instead of conditionally setting self.jwt_web and self.csrf_token only when data.get(...) is truthy, assign self.jwt_web = data.get("jwt_web") and self.csrf_token = data.get("csrf_token") (which will set them to None when missing) so stale in-memory values are not retained.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@garminconnect/client.py`:
- Around line 1248-1261: Extract the duplicated empty-response implementation
into a shared helper class (e.g., _EmptyJSONResponse) and replace both
EmptyProxyResp and the local EmptyJSONResp in _run_request() with instantiations
of that single helper; ensure the new class exposes status_code, content,
json(), __repr__ and __str__ with the same behavior so callers of _run_request
and the proxy-returning code continue to receive a 204 empty JSON response
without changing call sites.
- Around line 1263-1274: The proxy response error path currently always raises
GarminConnectConnectionError; modify the block that inspects resp (the shown if
resp.status_code >= 400 branch) to detect status_code == 429 and raise
GarminConnectTooManyRequestsError instead, mirroring the behavior in
_run_request(); preserve the existing logic that extracts JSON message
(error_data.get("message") or error_data.get("content")) or falls back to
resp.text for the exception message and include that message when constructing
GarminConnectTooManyRequestsError so callers receive the same rate-limit details
as when not using the proxy.
---
Duplicate comments:
In `@garminconnect/client.py`:
- Around line 1046-1063: The load_givemydata_session method must clear any
existing DI auth so JWT_WEB becomes the active auth; update
load_givemydata_session (which currently sets self.jwt_web) to also unset/clear
self.di_token (and any related DI auth flags) before returning so
get_api_headers will not prefer stale DI Bearer credentials over the imported
JWT_WEB token.
- Around line 1094-1104: The code currently sets self._browser_client = None
before attempting browser MFA, which loses the session on a transient failure;
instead, preserve self._browser_client until MFA succeeds: fetch gc =
self._browser_client without clearing it, call gc.submit_mfa(mfa_code), and only
clear self._browser_client (or close gc) after a successful verification or when
explicitly aborting; on a failed submit_mfa keep the browser client intact and
call gc.close() only if you intend to discard the session, and raise
GarminConnectAuthenticationError as before if verification fails.
- Line 385: Add an explicit Ruff suppression for S311 on the non-cryptographic
jitter call: update the line calling sleep(random.uniform(30, 45)) to append a
trailing comment like "# noqa: S311" (optionally with a short rationale, e.g.,
"# noqa: S311 # non-cryptographic pacing jitter") so the linter knows this
pseudo-random usage is intentional; the affected expression is the call to
sleep(random.uniform(30, 45)) in garminconnect.client.py.
- Around line 286-288: The call to gc.get_auth_tokens() can raise and currently
prevents gc.close() from running, leaking the headless browser; wrap the
extraction in a try/finally: call tokens = gc.get_auth_tokens() inside a try
block and ensure gc.close() is invoked in the finally block (re-raise or return
tokens after the finally), so gc.close() always runs even on exceptions.
- Around line 1035-1038: In loads(), avoid preserving stale tokens by explicitly
clearing jwt_web and csrf_token when they're absent from the provided data:
update the logic in the loads method so that instead of conditionally setting
self.jwt_web and self.csrf_token only when data.get(...) is truthy, assign
self.jwt_web = data.get("jwt_web") and self.csrf_token = data.get("csrf_token")
(which will set them to None when missing) so stale in-memory values are not
retained.
🪄 Autofix (Beta)
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
Run ID: 3358ae79-d4d8-4482-8782-f9d41b37f482
📒 Files selected for processing (2)
garminconnect/__init__.pygarminconnect/client.py
garminconnect/client.py
Outdated
Comment on lines
+1248
to
+1261
| class EmptyProxyResp: | ||
| status_code = 204 | ||
| content = b"" | ||
|
|
||
| def json(self_) -> Any: | ||
| return {} | ||
|
|
||
| def __repr__(self_) -> str: | ||
| return "{}" | ||
|
|
||
| def __str__(self_) -> str: | ||
| return "{}" | ||
|
|
||
| return EmptyProxyResp() |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting the empty response class to reduce duplication.
EmptyProxyResp duplicates EmptyJSONResp from _run_request(). While not critical, extracting to a shared class would reduce maintenance burden.
Suggested approach
Define a module-level or class-level helper:
class _EmptyJSONResponse:
"""Represents a 204 No Content response with empty JSON body."""
status_code = 204
content = b""
def json(self) -> Any:
return {}
def __repr__(self) -> str:
return "{}"
def __str__(self) -> str:
return "{}"Then use return _EmptyJSONResponse() in both methods.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@garminconnect/client.py` around lines 1248 - 1261, Extract the duplicated
empty-response implementation into a shared helper class (e.g.,
_EmptyJSONResponse) and replace both EmptyProxyResp and the local EmptyJSONResp
in _run_request() with instantiations of that single helper; ensure the new
class exposes status_code, content, json(), __repr__ and __str__ with the same
behavior so callers of _run_request and the proxy-returning code continue to
receive a 204 empty JSON response without changing call sites.
garminconnect/client.py
Outdated
Comment on lines
+1263
to
+1274
| if resp.status_code >= 400: | ||
| error_msg = f"API Error {resp.status_code}" | ||
| try: | ||
| error_data = resp.json() | ||
| if isinstance(error_data, dict): | ||
| msg = error_data.get("message") or error_data.get("content") | ||
| if msg: | ||
| error_msg += f" - {msg}" | ||
| except Exception: | ||
| if len(resp.text) < 500: | ||
| error_msg += f" - {resp.text}" | ||
| raise GarminConnectConnectionError(error_msg) |
Contributor
There was a problem hiding this comment.
Add explicit 429 handling for consistency with direct requests.
The proxy response handling doesn't map HTTP 429 to GarminConnectTooManyRequestsError, unlike _run_request(). This inconsistency could cause different exception types for the same underlying rate-limit condition depending on whether proxy is enabled.
Suggested fix
+ if resp.status_code == 429:
+ raise GarminConnectTooManyRequestsError(
+ "Rate limit exceeded via browser proxy"
+ )
+
if resp.status_code >= 400:
error_msg = f"API Error {resp.status_code}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@garminconnect/client.py` around lines 1263 - 1274, The proxy response error
path currently always raises GarminConnectConnectionError; modify the block that
inspects resp (the shown if resp.status_code >= 400 branch) to detect
status_code == 429 and raise GarminConnectTooManyRequestsError instead,
mirroring the behavior in _run_request(); preserve the existing logic that
extracts JSON message (error_data.get("message") or error_data.get("content"))
or falls back to resp.text for the exception message and include that message
when constructing GarminConnectTooManyRequestsError so callers receive the same
rate-limit details as when not using the proxy.
API calls like get_body_composition pass params={startDate, endDate}
which were silently dropped by _run_proxy_request, causing the
upstream to return different data shapes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When proxy_email is set, the /api/fetch JSON body includes an 'email' field so the garmin-givemydata add-on can auto-switch to the correct user's browser session. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reverts all garmin-givemydata integration changes. This matches the known-working-20260405 / pre-givemydata-integration tagged state. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
diegoscarabelli
added a commit
to diegoscarabelli/python-garminconnect
that referenced
this pull request
Apr 7, 2026
Garmin's Cloudflare WAF rate-limits requests that go directly from the SSO page GET to the login POST without intervening activity. A random 30-45s delay mimics natural browser behavior and helps avoid the 429 block. Adapted from upstream PR cyberjunky#346. The delay only applies to the portal+cffi / portal+requests fallback strategies. The widget+cffi primary strategy uses a different (HTML form, no clientId) endpoint that is not subject to per-clientId rate limiting and does not need this delay.
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
3 participants
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.
…re 429
Garmin's Cloudflare WAF rate-limits requests that go directly from the SSO page GET to the login POST without intervening activity. A random 30-45s delay mimics natural browser behavior and consistently avoids the 429 block.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests