Fix: Safely parse OAuth refresh error responses#108
Conversation
|
Warning Review limit reached
More reviews will be available in 59 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ✨ 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
This PR hardens OAuthRefreshException to avoid crashing when the OAuth refresh error response is missing or cannot be decoded as JSON, improving reliability in common failure paths.
Changes:
- Add a specific
messageforOAuthRefreshException. - Replace direct
response.json()usage with_load_data()that safely returns{}for missing/invalid responses. - Add minimal validation to ensure parsed JSON is a
dictbefore storing it.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Handle missing, invalid, or non-dict JSON payloads - Avoid crashes when refresh failures return unexpected bodies - Add fallback type safe values for error, error_description Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: OpenCode (gpt-5.4) <noreply@openai.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| class OAuthRefreshException(OAuthException): | ||
| message = 'Unauthorized - OAuth token refresh failed' | ||
|
|
||
| def __init__(self, response=None): | ||
| super().__init__(response) | ||
| self.data = self.response.json() | ||
| self.data = self._error_data(self.response) | ||
|
|
||
| @property | ||
| def error(self): | ||
| return self.data["error"] | ||
|
|
||
| @property | ||
| def error_description(self): | ||
| return self.data["error_description"] |
There was a problem hiding this comment.
The current PR doesn't plan to fully implement. That may be a separate PR instead.
| def __init__(self, response=None): | ||
| super().__init__(response) | ||
| self.data = self.response.json() | ||
| self.data = self._error_data(self.response) | ||
|
|
||
| @property | ||
| def error(self): | ||
| return self.data["error"] | ||
|
|
||
| @property | ||
| def error_description(self): | ||
| return self.data["error_description"] | ||
|
|
||
| @classmethod | ||
| def _error_data(cls, response): | ||
| data = cls._response_json(response) | ||
|
|
||
| return { | ||
| "error": data.get("error", ""), | ||
| "error_description": data.get("error_description", ""), | ||
| } | ||
|
|
||
| @staticmethod | ||
| def _response_json(response): | ||
| if response is None: | ||
| return {} | ||
|
|
||
| try: | ||
| data = response.json() | ||
| except (ValueError, AttributeError): | ||
| return {} | ||
|
|
||
| if not isinstance(data, dict): | ||
| return {} | ||
|
|
||
| return data |
|
|
||
| def __init__(self, response=None): | ||
| super().__init__(response) |
There was a problem hiding this comment.
Implemented in commit d65434c by adding a class docstring to OAuthRefreshException.
Extracted from #104