feat: add SSO embed widget login strategy to bypass 429 rate limiting#345
Open
diegoscarabelli wants to merge 6 commits intocyberjunky:masterfrom
Open
feat: add SSO embed widget login strategy to bypass 429 rate limiting#345diegoscarabelli wants to merge 6 commits intocyberjunky:masterfrom
diegoscarabelli wants to merge 6 commits intocyberjunky:masterfrom
Conversation
The portal and mobile JSON API endpoints are subject to aggressive per-clientId rate limiting. This adds a fifth login strategy using the SSO embed widget HTML form flow (/sso/embed + /sso/signin) with curl_cffi TLS impersonation. This flow does not use a clientId parameter, so it is not subject to the same rate limiting. The strategy is tried first when curl_cffi is available, before falling back to the existing portal and mobile strategies. Closes cyberjunky#344
Contributor
WalkthroughAdded a new SSO web widget login strategy that performs an embed→signin HTML form flow with CSRF handling and widget-specific MFA completion; adjusted strategy ordering, inserted a randomized sleep in the portal web flow, and updated resume logic to prioritize completing widget MFA and establishing the session. Changes
Sequence DiagramsequenceDiagram
participant Client
participant SSO_Embed as SSO /embed
participant SSO_Signin as SSO /signin
participant SSO_MFA as SSO /verifyMFA
participant DI as DI Token Exchange
Client->>SSO_Embed: GET /sso/embed (establish cookies)
SSO_Embed-->>Client: Set-Cookie
Client->>SSO_Signin: GET /sso/signin (fetch sign-in form)
SSO_Signin-->>Client: HTML + CSRF token
Client->>Client: Extract CSRF from hidden input
Client->>SSO_Signin: POST /sso/signin (email, password, _csrf, embed)
SSO_Signin-->>Client: Response page (may indicate MFA)
alt MFA required
Client->>SSO_MFA: POST /sso/verifyMFA/loginEnterMfaCode (mfa-code)
SSO_MFA-->>Client: MFA success page (contains ticket)
end
Client->>Client: Extract service ticket from response
Client->>DI: Exchange ticket for DI token
DI-->>Client: DI Token
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Pull request overview
Adds a new widget+cffi login strategy that uses the Garmin SSO embed widget HTML form flow (/sso/embed + /sso/signin) with curl_cffi TLS impersonation, intended to bypass per-clientId 429 rate limiting affecting the existing portal/mobile strategies.
Changes:
- Introduces
widget+cffias the first attempted login strategy whencurl_cffiis available. - Implements widget-flow MFA handling via
_complete_mfa_widgetand wires it intoresume_login. - Adds regex-based HTML parsing for CSRF/title/ticket extraction.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Prevents stale _widget_session from being picked up by a subsequent resume_login call if a different login strategy is used next.
![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 the current code and only fix it if needed.
Inline comments:
In `@garminconnect/client.py`:
- Around line 364-370: The current check uses title_match/_TITLE_RE and raises
GarminConnectAuthenticationError with a generic message when title != "Success";
instead, examine the extracted title string for known failure values (e.g.,
"Invalid code", "Code expired", "Too many attempts") and map them to clearer,
specific errors or messages before raising—either raise distinct exceptions
(e.g., GarminConnectMFAInvalidCodeError) or include a descriptive message that
differentiates "invalid MFA code" vs. "unexpected server response"; update the
block that references title_match, title, and the
GarminConnectAuthenticationError to perform this lookup and raise the
appropriate, more informative exception.
- Around line 352-362: The POST to "/sso/verifyMFA/loginEnterMfaCode" (the
sess.post call in garminconnect/client.py that sends the MFA code) is missing a
timeout; add a timeout keyword argument to that sess.post invocation so the
request cannot hang indefinitely (use an existing attribute like
self._request_timeout if one exists, otherwise add a sensible default such as
timeout=30 and surface it as a configurable client option). Make sure to update
any constructor or config to allow setting the timeout if you introduce a new
attribute and keep the same parameter name when used elsewhere.
- Around line 285-295: The POST that submits credentials (the sess.post call
that posts to f"{sso_base}/signin" with signin_params and data including
"_csrf") must include an explicit timeout argument so it cannot hang
indefinitely; update that sess.post invocation to pass a numeric timeout (or
reuse a configured value like self.timeout or a module-level DEFAULT_TIMEOUT)
and ensure any related callers expect a requests timeout exception
(requests.exceptions.Timeout) if needed.
- Around line 269-277: Check the response and handle errors for the initial
cookie-establishment call sess.get(sso_embed, params=embed_params) and the
subsequent CSRF fetch r = sess.get(f"{sso_base}/signin", ...): add an explicit
timeout argument to both calls, wrap them in try/except to catch
requests.exceptions.RequestException, verify r.status_code (treat non-2xx as
failure) and surface a clear error or raise an exception that includes the URL,
status_code and the underlying exception message before proceeding to CSRF
extraction; update any logging or error text so failures point to
sess.get(sso_embed, ...) or the signin GET rather than a generic "could not find
CSRF token."
- Around line 325-328: The strict title equality check (if title != "Success")
can false-negative if the success page is localized or changed; update the logic
in the same block (where title and GarminConnectConnectionError are referenced)
to try a fallback detection: if title isn't exactly "Success", attempt to locate
the service ticket (e.g., check for a parsed variable like service_ticket, look
for a "ticket" query param in the redirect URL, or search the page HTML for a
service ticket token) and only raise GarminConnectConnectionError when neither
the title matches nor a valid service ticket is found; keep the original
exception message but include the fallback branch so presence of a service
ticket allows success to proceed.
🪄 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: f7389bd9-ccec-4f2a-826b-0ade911cb6cc
📒 Files selected for processing (1)
garminconnect/client.py
Address Copilot review feedback: - Check status codes on embed and sign-in GET requests - Add explicit timeout=30 to credentials POST and MFA POST
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
garminconnect/client.py:1151
resume_login(self, _client_state, mfa_code)still ignores the_client_stateargument and relies on internal attributes (_widget_session,_widget_signin_params,_widget_last_resp) to complete MFA. This makes the "state" value returned fromlogin(return_on_mfa=True)effectively unused for the widget flow and prevents resuming MFA on a fresh Client instance. Consider using_client_stateas the source of truth (or documenting/removing the parameter) so MFA resumption behaves consistently.
def put(self, _domain: str, path: str, **kwargs: Any) -> Any:
api = kwargs.pop("api", False)
resp = self._run_request("PUT", path, **kwargs)
if api:
return resp.json() if hasattr(resp, "json") else None
return resp
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
garminconnect/client.py (1)
280-294:⚠️ Potential issue | 🟡 MinorRaise on
/sso/signinGET failures before parsing CSRF.This branch still skips
r.okhandling, so a 403/5xx is reported as “could not find CSRF token” instead of the actual HTTP failure.Suggested fix
if r.status_code == 429: raise GarminConnectTooManyRequestsError( "Widget login returned 429 on sign-in page" ) + if not r.ok: + raise GarminConnectConnectionError( + f"Widget login: sign-in page returned HTTP {r.status_code}" + ) csrf_match = self._CSRF_RE.search(r.text) if not csrf_match: raise GarminConnectConnectionError( "Widget login: could not find CSRF token in sign-in page" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@garminconnect/client.py` around lines 280 - 294, The GET to /sso/signin currently parses the response body before verifying HTTP success, causing 403/5xx to be misreported as a missing CSRF; update the block that performs sess.get(...) to first check r.ok (or r.status_code) and raise a GarminConnectConnectionError (including r.status_code and a short message) for non-2xx responses (handle 429 as before using GarminConnectTooManyRequestsError), then proceed to run self._CSRF_RE.search(r.text) and raise the existing CSRF-related GarminConnectConnectionError only if the token is absent.
🤖 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 329-331: The widget login call sites pass service_url=sso_embed to
Client._establish_session but _establish_session still redeems the fallback
ticket against the hard-coded MOBILE_SSO_SERVICE_URL, breaking the JWT fallback;
update _establish_session to use the passed service_url parameter when
performing the fallback/JWT redemption (instead of the constant
MOBILE_SSO_SERVICE_URL), ensure any internal variable/branch that builds the
DI/JWT redemption endpoint uses service_url, and leave callers like the ones
that call _complete_mfa_widget(...)/_establish_session(ticket, sess=sess,
service_url=sso_embed) unchanged so widget logins can properly fall back to
JWT_WEB.
- Around line 315-341: The widget branch currently treats any non-"Success"
title as a connection error which causes Client.login() to continue trying other
strategies; modify the post-MFA check in the widget handling (around
title_match, title, _widget_session, _widget_signin_params, _widget_last_resp,
and _complete_mfa_widget) so that known widget credential-rejection titles
(e.g., titles containing "Invalid", "Incorrect", "Bad credentials", or a
specific rejection phrase your tests show) raise
GarminConnectAuthenticationError immediately, while leaving the generic
GarminConnectConnectionError for other unexpected titles; ensure return_on_mfa
and prompt_mfa flows remain unchanged.
- Around line 327-333: After successful inline MFA completion in the prompt_mfa
path, clear the stored widget state to avoid reusing stale widget sessions:
after calling self._establish_session(ticket, sess=sess, service_url=sso_embed)
(inside the prompt_mfa branch) reset self._widget_session,
self._widget_signin_params, and self._widget_last_resp to None so subsequent
resume_login() calls won't pick up old widget state from the same Client
instance.
- Around line 365-384: The _complete_mfa_widget() flow parses the HTML title
without checking the HTTP response code, so transport errors (429/5xx) get
misclassified as invalid MFA; update the POST handling to first inspect
r.status_code (and treat non-2xx as transport failures) and raise a distinct
error or re-raise an HTTP/transport exception before attempting to parse the
title; ensure the change references the POST response variable r, the method
_complete_mfa_widget, and use GarminConnectAuthenticationError (or a new
Transport/HTTP error) to signal network/server failures rather than treating
them as "Widget MFA verification failed".
---
Duplicate comments:
In `@garminconnect/client.py`:
- Around line 280-294: The GET to /sso/signin currently parses the response body
before verifying HTTP success, causing 403/5xx to be misreported as a missing
CSRF; update the block that performs sess.get(...) to first check r.ok (or
r.status_code) and raise a GarminConnectConnectionError (including r.status_code
and a short message) for non-2xx responses (handle 429 as before using
GarminConnectTooManyRequestsError), then proceed to run
self._CSRF_RE.search(r.text) and raise the existing CSRF-related
GarminConnectConnectionError only if the token is absent.
🪄 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: 2c2d4463-46d6-471b-805f-ef60e8cbb4d0
📒 Files selected for processing (1)
garminconnect/client.py
- Detect credential failures in widget title and raise AuthenticationError instead of ConnectionError to prevent falling through to other strategies with bad credentials - Clear widget MFA state in the prompt_mfa inline path (not just resume_login) to prevent stale session reuse - Check HTTP status on MFA POST before parsing HTML to correctly classify 429/5xx as transport failures
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 169-180: The widget MFA state (_widget_session and related
_widget_* attributes) must be cleared before starting a fresh login so
resume_login() won't incorrectly prefer the stale widget path; update the
beginning of login() (or just before resume_login() selection) to reset/clear
_widget_session, _widget_challenge, _widget_result (or any _widget_* attributes
used) to None/empty so that _complete_mfa_widget() is not chosen for new
portal/mobile MFA flows; ensure you reference and clear all widget-related
fields present on the Client instance so the new login proceeds through the
correct strategy selection.
- Around line 297-354: The response body title-based authentication heuristic is
being applied even for transport errors; before inspecting title/title_lower
(the block using title_match/title_lower), check r.ok (HTTP 2xx) and if not ok
raise a GarminConnectConnectionError (or re-raise appropriate transport error)
so server-side 4xx/5xx pages don't get misclassified as credential failures;
mirror the guard used in _complete_mfa_widget(), and keep the existing MFA
handling (references: _complete_mfa_widget, _establish_session, _widget_session,
_widget_signin_params, _widget_last_resp) intact.
🪄 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: 56cac49c-1646-4031-8d37-728a243cc166
📒 Files selected for processing (1)
garminconnect/client.py
Author
|
@cyberjunky what do you think? |
Addresses CodeRabbit review feedback on PR cyberjunky#345: 1. Clear leftover _widget_* attributes at the start of login() so resume_login() does not incorrectly route a fresh portal/mobile MFA flow through _complete_mfa_widget() after an abandoned widget challenge. 2. Guard the credential POST with r.ok check after the 429 handler, so server-side 4xx/5xx error pages raise GarminConnectConnectionError instead of being misclassified as credential failures via the title-based heuristic.
Author
|
@coderabbitai full review |
|
hello! f2f403b worked for me, for whatever that's worth. thank you :) |
|
I got this on this branch :( |
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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
garminconnect/client.py (1)
293-306:⚠️ Potential issue | 🟡 MinorCheck
/sso/signinfor non-2xx before parsing the CSRF token.Line 298 only special-cases 429. A 403/500 response here still falls through to
_CSRF_REand gets reported as “could not find CSRF token”, which hides the real failure and makes the fallback logs misleading. Mirror thenot r.okguard already used for the embed GET and credential POST.🔧 Proposed fix
if r.status_code == 429: raise GarminConnectTooManyRequestsError( "Widget login returned 429 on sign-in page" ) + if not r.ok: + raise GarminConnectConnectionError( + f"Widget login: sign-in page returned HTTP {r.status_code}" + ) csrf_match = self._CSRF_RE.search(r.text)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@garminconnect/client.py` around lines 293 - 306, The GET to f"{sso_base}/signin" only special-cases 429 and then proceeds to parse the CSRF token, which hides other HTTP errors; update the block around sess.get(...) so it checks r.ok (mirror the earlier embed GET and credential POST checks) before attempting to parse self._CSRF_RE: if not r.ok raise a GarminConnectConnectionError (include r.status_code and a short context message) except preserve the existing GarminConnectTooManyRequestsError for 429; this ensures failures like 403/500 are surfaced instead of producing a misleading "could not find CSRF token" error.
🤖 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 516-522: Only sleep before the credential POST when the preceding
sign-in GET actually succeeded: capture the GET response (do not discard it),
check that it returned a successful status and the expected sign-in page
content, and only then compute delay_s and call _LOGGER.debug/time.sleep before
issuing the POST; if the GET returned a 429/5xx or unexpected content, skip the
30–45s sleep and handle or retry the GET error path instead. Ensure you modify
the code around the sign-in GET, delay_s, _LOGGER.debug and time.sleep usage so
the delay is conditional on a successful GET.
---
Duplicate comments:
In `@garminconnect/client.py`:
- Around line 293-306: The GET to f"{sso_base}/signin" only special-cases 429
and then proceeds to parse the CSRF token, which hides other HTTP errors; update
the block around sess.get(...) so it checks r.ok (mirror the earlier embed GET
and credential POST checks) before attempting to parse self._CSRF_RE: if not
r.ok raise a GarminConnectConnectionError (include r.status_code and a short
context message) except preserve the existing GarminConnectTooManyRequestsError
for 429; this ensures failures like 403/500 are surfaced instead of producing a
misleading "could not find CSRF token" error.
🪄 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: 27744529-7440-427d-a94b-5a0478553f75
📒 Files selected for processing (1)
garminconnect/client.py
Comment on lines
+516
to
+522
| # 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. (Adapted from upstream PR #346.) | ||
| delay_s = random.uniform(30, 45) | ||
| _LOGGER.debug("Portal login: sleeping %.1fs before credential POST", delay_s) | ||
| time.sleep(delay_s) |
Contributor
There was a problem hiding this comment.
Only sleep after the sign-in GET has actually succeeded.
Because the GET on Line 506 still discards its response, a 429/5xx sign-in page now incurs the full 30–45s delay and then still sends the credential POST. With both portal strategies in the fallback chain, that can add 60–90s of avoidable latency before mobile auth even starts.
🔧 Proposed fix
- sess.get(
+ r = sess.get(
signin_url,
params={
"clientId": PORTAL_SSO_CLIENT_ID,
"service": PORTAL_SSO_SERVICE_URL,
},
headers=get_headers,
timeout=30,
)
+ if r.status_code == 429:
+ raise GarminConnectTooManyRequestsError(
+ "Portal sign-in page returned 429. Cloudflare is blocking this request."
+ )
+ if not r.ok:
+ raise GarminConnectConnectionError(
+ f"Portal sign-in page returned HTTP {r.status_code}"
+ )
# Garmin's Cloudflare WAF rate-limits requests that go directly from
# the SSO page GET to the login POST without intervening activity.
delay_s = random.uniform(30, 45)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@garminconnect/client.py` around lines 516 - 522, Only sleep before the
credential POST when the preceding sign-in GET actually succeeded: capture the
GET response (do not discard it), check that it returned a successful status and
the expected sign-in page content, and only then compute delay_s and call
_LOGGER.debug/time.sleep before issuing the POST; if the GET returned a 429/5xx
or unexpected content, skip the 30–45s sleep and handle or retry the GET error
path instead. Ensure you modify the code around the sign-in GET, delay_s,
_LOGGER.debug and time.sleep usage so the delay is conditional on a successful
GET.
Author
|
Looks like widget+cffi → 429 now... |
izaak-frost
approved these changes
Apr 8, 2026
There was a problem hiding this comment.
Found that new tokens couldn't be created with garminconnect 0.3.1. Tested the proposed changes from @diegoscarabelli and was able to generate new tokens, perfectly as expected.
(Thank you for the fix by the way!)
|
I just tested and it's working properly :) |
|
wow.... it works..... |
|
Confirmed to be working! |
|
This PR fixed the 429 issue for me — thanks @diegoscarabelli! The widget strategy itself still got 429'd (presumably because my account/IP was already deep in the rate-limit hole), but the portal fallback with the 30-45s delay succeeded. One UX note: the 30-45 second Thank you again!!! |
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
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
Adds a fifth login strategy (
widget+cffi) that uses the SSO embed widget HTML form flow (/sso/embed+/sso/signin) withcurl_cffiTLS impersonation. This bypasses the per-clientId 429 rate limiting that affects all four existing strategies.Closes #344
Problem
The portal and mobile login endpoints require a
clientIdparameter (GarminConnect,GCM_ANDROID_DARK). Garmin rate-limits these perclientId + account email. Once triggered, switching IP or User-Agent does not help. The official Garmin Connect app is unaffected because it uses a different rate limit bucket.How this works
The SSO embed widget flow is an HTML form-based login (CSRF token + POST with username/password). It does not use a
clientIdparameter, so it sits in a completely different rate limit bucket. Combined withcurl_cffiChrome TLS impersonation to pass Cloudflare, it reliably authenticates even when all other strategies return 429.The flow:
GET /sso/embed(establish cookies)GET /sso/signin(obtain CSRF token from HTML)POST /sso/signin(submit credentials via form data)POST /sso/verifyMFA/loginEnterMfaCodeThe service ticket feeds into the existing
_exchange_service_ticketfor DI token exchange. No changes needed downstream.Changes
import refor HTML parsing (CSRF token, title, ticket extraction)_widget_login_cffias a new login strategy (tried first whencurl_cffiis available)_complete_mfa_widgetfor MFA handling in the widget flowresume_loginto support widget MFA completioncurl_cffi)Testing
Tested manually against two Garmin accounts (one with MFA enabled, one without):
resume_loginpath verified for thereturn_on_mfa=TruecaseTrade-offs
The widget flow parses HTML (CSRF token from a hidden input, success/MFA status from
<title>tag, ticket from a URL in the response). This is more fragile than JSON APIs. However:Resolves #344
Summary by CodeRabbit
New Features
Improvements