Pass proxy/verify settings when fetching OAuth consumer#155
Conversation
When behind a proxy, the initial OAuth consumer fetch would fail because proxy and SSL verification settings from the parent session weren't being passed to the requests.get() call. Fixes #66 Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughThe change modifies Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #155 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 58 58
Lines 2599 2603 +4
=========================================
+ Hits 2599 2603 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/garth/sso.py:
- Around line 36-42: The requests.get call that loads OAUTH_CONSUMER (using
OAUTH_CONSUMER_URL and request_kwargs) needs a timeout to avoid blocking; update
the invocation of requests.get to pass a timeout parameter (use parent.timeout
or parent.adapters/session timeout if available, otherwise a sensible default
like 5–10s) and ensure request_kwargs still propagates proxies and verify before
calling requests.get(..., timeout=...).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/garth/sso.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Usemake formatto auto-format Python source files using ruff
Usemake lintto lint Python source files (ruff format check, ruff check, mypy)
Files:
src/garth/sso.py
**/sso.py
📄 CodeRabbit inference engine (CLAUDE.md)
Single Sign-On authentication logic for Garmin services should be implemented in
sso.py
Files:
src/garth/sso.py
🧠 Learnings (1)
📚 Learning: 2026-01-08T00:25:14.763Z
Learnt from: CR
Repo: matin/garth PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-08T00:25:14.763Z
Learning: Applies to **/sso.py : Single Sign-On authentication logic for Garmin services should be implemented in `sso.py`
Applied to files:
src/garth/sso.py
🪛 Ruff (0.14.10)
src/garth/sso.py
40-40: Probable use of requests call without timeout
(S113)
| request_kwargs: dict[str, Any] = {} | ||
| if parent is not None: | ||
| request_kwargs["proxies"] = parent.proxies | ||
| request_kwargs["verify"] = parent.verify | ||
| OAUTH_CONSUMER = requests.get( | ||
| OAUTH_CONSUMER_URL, **request_kwargs | ||
| ).json() |
There was a problem hiding this comment.
Proxy and verify settings correctly propagated; add timeout parameter to prevent hangs.
The conditional propagation of proxies and verify settings correctly addresses issue #66 and ensures the library works for users behind proxies. However, the requests.get() call lacks a timeout parameter, which can cause the application to hang indefinitely if the network is slow or the server is unresponsive. This is particularly problematic since it blocks object initialization.
⏱️ Proposed fix to add timeout parameter
request_kwargs: dict[str, Any] = {}
if parent is not None:
request_kwargs["proxies"] = parent.proxies
request_kwargs["verify"] = parent.verify
+ # Use a reasonable default timeout to prevent indefinite hangs
+ request_kwargs["timeout"] = 30
OAUTH_CONSUMER = requests.get(
OAUTH_CONSUMER_URL, **request_kwargs
).json()Alternatively, if a timeout attribute is available from the parent session context, you could use that instead of a hardcoded value.
Based on static analysis hints (Ruff S113).
📝 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.
| request_kwargs: dict[str, Any] = {} | |
| if parent is not None: | |
| request_kwargs["proxies"] = parent.proxies | |
| request_kwargs["verify"] = parent.verify | |
| OAUTH_CONSUMER = requests.get( | |
| OAUTH_CONSUMER_URL, **request_kwargs | |
| ).json() | |
| request_kwargs: dict[str, Any] = {} | |
| if parent is not None: | |
| request_kwargs["proxies"] = parent.proxies | |
| request_kwargs["verify"] = parent.verify | |
| # Use a reasonable default timeout to prevent indefinite hangs | |
| request_kwargs["timeout"] = 30 | |
| OAUTH_CONSUMER = requests.get( | |
| OAUTH_CONSUMER_URL, **request_kwargs | |
| ).json() |
🧰 Tools
🪛 Ruff (0.14.10)
40-40: Probable use of requests call without timeout
(S113)
🤖 Prompt for AI Agents
In @src/garth/sso.py around lines 36 - 42, The requests.get call that loads
OAUTH_CONSUMER (using OAUTH_CONSUMER_URL and request_kwargs) needs a timeout to
avoid blocking; update the invocation of requests.get to pass a timeout
parameter (use parent.timeout or parent.adapters/session timeout if available,
otherwise a sensible default like 5–10s) and ensure request_kwargs still
propagates proxies and verify before calling requests.get(..., timeout=...).
1 participant
Summary
When behind a proxy, the initial OAuth consumer fetch would fail because proxy and SSL verification settings from the parent session weren't being passed to the
requests.get()call.This fix passes the parent session's
proxiesandverifysettings when fetching the OAuth consumer JSON.Closes #66
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.