From 453fe163469bdb081f065b8939f2313792679ec9 Mon Sep 17 00:00:00 2001 From: Daniel Pepper Date: Wed, 14 Oct 2026 13:03:06 -0700 Subject: [PATCH 1/3] =?UTF-8?q?=F0=9F=90=9B=20Raise=20parse=20errors=20on?= =?UTF-8?q?=20the=20legacy=20extract=5Faccess=5Ftoken=20path?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- CHANGELOG.md | 1 + lib/oauth2/client.rb | 15 +++++++++------ spec/oauth2/client_spec.rb | 6 +++--- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 715428bc..4d8ad30b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ Please file a bug if you notice a violation of semantic versioning. ### Fixed - `OAuth2::AccessToken` no longer raises `NoMethodError` when `expires_latency` is set but the token has no known expiry (no `expires_in`/`expires_at`, or `expires_in: 0`), e.g. `client.get_token(params, expires_latency: 30)` against a provider that omits `expires_in` by @iamibi +- `OAuth2::Client#get_token` raises response parse errors (e.g. `JSON::ParserError`) when using the deprecated `extract_access_token` option, rather than returning `nil` by @dpep ### Security diff --git a/lib/oauth2/client.rb b/lib/oauth2/client.rb index 37e90149..1f9f6298 100644 --- a/lib/oauth2/client.rb +++ b/lib/oauth2/client.rb @@ -600,12 +600,15 @@ def build_access_token(response, access_token_opts, access_token_class) # # @api private def build_access_token_legacy(response, access_token_opts, extract_access_token) - extract_access_token.call(self, response.parsed.merge(access_token_opts)) - rescue - # An error will be raised by the called if nil is returned and options[:raise_errors] is truthy, so this rescue is but temporary. - # Unfortunately, it does hide the real error, but this is deprecated legacy code, - # and this was effectively the long-standing pre-existing behavior, so there is little point in changing it. - nil + parsed = response.parsed + begin + extract_access_token.call(self, parsed.merge(access_token_opts)) + rescue + # An error will be raised by the called if nil is returned and options[:raise_errors] is truthy, so this rescue is but temporary. + # Unfortunately, it does hide the real error, but this is deprecated legacy code, + # and this was effectively the long-standing pre-existing behavior, so there is little point in changing it. + nil + end end def oauth_debug_logging(builder) diff --git a/spec/oauth2/client_spec.rb b/spec/oauth2/client_spec.rb index f1654779..a9ae3e7a 100644 --- a/spec/oauth2/client_spec.rb +++ b/spec/oauth2/client_spec.rb @@ -1208,12 +1208,12 @@ block_is_expected { get_token }.to raise_error(JSON::ParserError) end - context "when extract_access_token raises an exception" do + context "when using extract_access_token" do let(:status_code) { 200 } let(:extract_proc) { proc { |client, hash| raise ArgumentError } } - it "returns a nil :access_token" do - expect(client.get_token({}, {}, extract_proc)).to be_nil + it "raises the parse error" do + expect { client.get_token({}, {}, extract_proc) }.to raise_error(JSON::ParserError) end end end From 218d75b4acdaea6c5014d95cce696f913e98a96e Mon Sep 17 00:00:00 2001 From: Daniel Pepper Date: Wed, 14 Oct 2026 13:07:59 -0700 Subject: [PATCH 2/3] =?UTF-8?q?=E2=9C=85=20Cover=20both=20parse=20and=20ex?= =?UTF-8?q?tract=20errors=20on=20the=20legacy=20path?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5.5 (1M context) --- spec/oauth2/client_spec.rb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/spec/oauth2/client_spec.rb b/spec/oauth2/client_spec.rb index a9ae3e7a..61cea60d 100644 --- a/spec/oauth2/client_spec.rb +++ b/spec/oauth2/client_spec.rb @@ -1210,7 +1210,7 @@ context "when using extract_access_token" do let(:status_code) { 200 } - let(:extract_proc) { proc { |client, hash| raise ArgumentError } } + let(:extract_proc) { proc { |client, hash| OAuth2::AccessToken.from_hash(client, hash) } } it "raises the parse error" do expect { client.get_token({}, {}, extract_proc) }.to raise_error(JSON::ParserError) @@ -1229,6 +1229,14 @@ expect(token.response).to be_a OAuth2::Response expect(token.response.parsed).to eq("access_token" => "the-token") end + + context "when extract_access_token raises an exception" do + let(:extract_proc) { proc { |client, hash| raise ArgumentError } } + + it "returns a nil :access_token" do + expect(client.get_token({}, {}, extract_proc)).to be_nil + end + end end context "when Content-Type is not JSON" do From bffb54a739de4a94a0cfaf1bc0fd06bbd1fbb69a Mon Sep 17 00:00:00 2001 From: Daniel Pepper Date: Wed, 14 Oct 2026 13:10:44 -0700 Subject: [PATCH 3/3] =?UTF-8?q?=E2=9C=85=20Minimize=20legacy-path=20spec?= =?UTF-8?q?=20diff?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5.5 (1M context) --- spec/oauth2/client_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/oauth2/client_spec.rb b/spec/oauth2/client_spec.rb index 61cea60d..ca71844f 100644 --- a/spec/oauth2/client_spec.rb +++ b/spec/oauth2/client_spec.rb @@ -1208,9 +1208,9 @@ block_is_expected { get_token }.to raise_error(JSON::ParserError) end - context "when using extract_access_token" do + context "when extract_access_token raises an exception" do let(:status_code) { 200 } - let(:extract_proc) { proc { |client, hash| OAuth2::AccessToken.from_hash(client, hash) } } + let(:extract_proc) { proc { |client, hash| raise ArgumentError } } it "raises the parse error" do expect { client.get_token({}, {}, extract_proc) }.to raise_error(JSON::ParserError)