Repository navigation
Conversation
build_access_token_legacy parsed the response inside its catch-all rescue, so a body that failed to parse looked the same as a token-less response. Parse before the rescue so parse errors propagate; errors raised by the extract proc are still rescued. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The focused implementation correctly preserves extractor handling while allowing parsing failures to propagate.
0 open findings
What changed in this PR
Ensures legacy access-token extraction propagates response parsing failures consistently with the standard token path.
Changes:
- Parses responses before rescuing legacy extractor errors.
- Updates invalid-JSON coverage.
- Documents the fix in the changelog.
| File | Description |
|---|---|
lib/oauth2/client.rb |
Separates parsing from legacy extraction error handling. |
spec/oauth2/client_spec.rb |
Verifies parse errors propagate through legacy extraction. |
CHANGELOG.md |
Records the corrected behavior. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
dpep
marked this pull request as ready for review
October 14, 2026 20:07
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This branch has not been deployed
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.
Client#build_access_token_legacycallsresponse.parsedinside its catch-allrescue, so a body that fails to parse looks the same as a response with no token. Withraise_errors: trueyou get a genericOAuth2::Error, and withraise_errors: falseyou getnil. Either way the parse error is lost.This parses before the
rescue, so parse errors propagate, which matches the default (non-legacy)get_tokenpath. Errors raised by the extract proc itself are still rescued, so nil and empty bodies behave the same as before.I know the inline comment says it isn't worth changing deprecated code. It's a small change, though, and anyone still on
extract_access_token(often a holdover from 1.4.x) hits exactly this: a broken dependency or malformed body shows up as an auth failure instead of a stack trace.