From d369d4c841e15d5d55b1aafc3f2f768959d844e7 Mon Sep 17 00:00:00 2001 From: Codex Date: Fri, 7 Aug 2026 01:14:24 +1000 Subject: [PATCH 1/3] Validate OAuth state before token exchange --- lib/xero-ruby/api_client.rb | 3 +- spec/api_client_spec.rb | 83 +++++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/lib/xero-ruby/api_client.rb b/lib/xero-ruby/api_client.rb index dc9d679a..4f79e673 100644 --- a/lib/xero-ruby/api_client.rb +++ b/lib/xero-ruby/api_client.rb @@ -164,6 +164,8 @@ def get_client_credentials_token end def get_token_set_from_callback(params) + validate_state(params) + data = { grant_type: @grant_type, code: params['code'], @@ -172,7 +174,6 @@ def get_token_set_from_callback(params) token_set = token_request(data, '/token') validate_tokens(token_set) - validate_state(params) return token_set end diff --git a/spec/api_client_spec.rb b/spec/api_client_spec.rb index 617433b7..9253d56b 100644 --- a/spec/api_client_spec.rb +++ b/spec/api_client_spec.rb @@ -100,6 +100,89 @@ end end + describe '#get_token_set_from_callback' do + let(:credentials) do + { + client_id: 'abc', + client_secret: '123', + redirect_uri: 'https://mydomain.com/callback', + scopes: 'openid profile email', + state: 'expected-state' + } + end + let(:api_client) { XeroRuby::ApiClient.new(credentials: credentials) } + let(:existing_token_set) do + { + 'access_token' => 'existing-access-token', + 'id_token' => 'existing-id-token' + } + end + let(:new_token_set) do + { + 'access_token' => 'new-access-token', + 'id_token' => 'new-id-token' + } + end + + it 'rejects a mismatched state before requesting or mutating tokens' do + api_client.set_token_set(existing_token_set) + + expect(api_client).not_to receive(:token_request) + expect(api_client).not_to receive(:set_token_set) + + expect { + api_client.get_token_set_from_callback( + 'code' => 'callback-code', + 'state' => 'attacker-state' + ) + }.to raise_error( + StandardError, + 'WARNING: @config.state: expected-state and OAuth callback state: attacker-state do not match!' + ) + + expect(api_client.token_set).to eq(existing_token_set.with_indifferent_access) + expect(api_client.access_token).to eq('existing-access-token') + expect(api_client.id_token).to eq('existing-id-token') + end + + it 'exchanges and stores tokens when a supplied state matches' do + callback_params = { + 'code' => 'callback-code', + 'state' => 'expected-state' + } + + expect(api_client).to receive(:token_request).with( + { + grant_type: 'authorization_code', + code: 'callback-code', + redirect_uri: 'https://mydomain.com/callback' + }, + '/token' + ) do + api_client.set_token_set(new_token_set) + new_token_set + end + expect(api_client).to receive(:validate_tokens).with(new_token_set).and_return(true) + + expect(api_client.get_token_set_from_callback(callback_params)).to eq(new_token_set) + expect(api_client.token_set).to eq(new_token_set.with_indifferent_access) + end + + it 'continues to allow callbacks without state when no state was configured' do + client_without_state = XeroRuby::ApiClient.new(credentials: { + client_id: 'abc', + client_secret: '123', + redirect_uri: 'https://mydomain.com/callback', + scopes: 'openid profile email' + }) + + expect(client_without_state).to receive(:token_request).and_return(new_token_set) + expect(client_without_state).to receive(:validate_tokens).with(new_token_set).and_return(true) + + expect(client_without_state.get_token_set_from_callback('code' => 'callback-code')).to eq(new_token_set) + end + end + describe 'api_client helper functions' do let(:api_client) { XeroRuby::ApiClient.new } let(:token_set) { { 'access_token': 'eyx.authorization.data', 'id_token': 'eyx.authentication.data', 'refresh_token': 'REFRESHMENTS' } } From b43b77bc5ee79ed1747b1e0f36ecb45531fa3519 Mon Sep 17 00:00:00 2001 From: Ryan Duguid <152749594+ryanduguid@users.noreply.github.com> Date: Thu, 13 Aug 2026 03:05:49 +1000 Subject: [PATCH 2/3] fix: keep OAuth state values out of errors --- lib/xero-ruby/api_client.rb | 2 +- spec/api_client_spec.rb | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/xero-ruby/api_client.rb b/lib/xero-ruby/api_client.rb index 4f79e673..df80928a 100644 --- a/lib/xero-ruby/api_client.rb +++ b/lib/xero-ruby/api_client.rb @@ -190,7 +190,7 @@ def validate_tokens(token_set) def validate_state(params) if params['state'] != @state - raise StandardError.new "WARNING: @config.state: #{@state} and OAuth callback state: #{params['state']} do not match!" + raise StandardError.new 'WARNING: OAuth callback state does not match!' end return true end diff --git a/spec/api_client_spec.rb b/spec/api_client_spec.rb index 9253d56b..3387923d 100644 --- a/spec/api_client_spec.rb +++ b/spec/api_client_spec.rb @@ -71,7 +71,7 @@ } api_client = XeroRuby::ApiClient.new(credentials: creds) altered_state = { 'state': 'not-original-state' } - expect { api_client.validate_state(altered_state) }.to raise_error(StandardError, 'WARNING: @config.state: custom-state and OAuth callback state: do not match!') + expect { api_client.validate_state(altered_state) }.to raise_error(StandardError, 'WARNING: OAuth callback state does not match!') end end @@ -137,8 +137,10 @@ ) }.to raise_error( StandardError, - 'WARNING: @config.state: expected-state and OAuth callback state: attacker-state do not match!' - ) + 'WARNING: OAuth callback state does not match!' + ) { |error| + expect(error.message).not_to include('expected-state', 'attacker-state') + } expect(api_client.token_set).to eq(existing_token_set.with_indifferent_access) expect(api_client.access_token).to eq('existing-access-token') From de2bd30e12bd68f76bac8cb8d5d2a29e6bfac0a1 Mon Sep 17 00:00:00 2001 From: Ryan Duguid <152749594+ryanduguid@users.noreply.github.com> Date: Wed, 19 Aug 2026 15:07:21 +1000 Subject: [PATCH 3/3] fix(oauth): fail closed when a configured state is missing from the callback validate_state only rejected a callback when params['state'] differed from @state. @state is nil unless the consumer passes state: in credentials, so the pre-exchange check was inert in the default configuration and a callback with no state parameter passed straight through to token_request and set_token_set. - Reject a missing or blank callback state whenever a state is configured, and raise before the authorization code is exchanged. - Compare the nonce in constant time over SHA-256 digests instead of String#==, which short circuits on length and then memcmps. - Keep the no-state-configured path permissive to avoid a breaking API change, and document the CSRF risk of omitting state in the README. Specs: the mismatched-state test used the symbol key 'state': so params['state'] was nil and it actually exercised the missing-state branch. Use the string key and cover matching, mismatched, absent and blank states separately, plus an end-to-end case asserting no token request or mutation on an absent state. --- README.md | 10 +++++++++ lib/xero-ruby/api_client.rb | 30 ++++++++++++++++++++++++- spec/api_client_spec.rb | 45 +++++++++++++++++++++++++++++++++---- 3 files changed, 80 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index d443aeb8..fbcd830a 100644 --- a/README.md +++ b/README.md @@ -82,6 +82,16 @@ creds = { xero_client ||= XeroRuby::ApiClient.new(credentials: creds) ``` +> **Security note on `state`.** `state` is optional, but omitting it leaves the +> authorization code flow with no CSRF protection. Without it the SDK has nothing +> to compare the callback against, so an attacker who can send a victim to +> `/your-callback?code=` can bind the victim's session to the +> attacker's Xero tokens. Set `state` to an unguessable, per-authorization value +> that you persist in the user's session, and pass the same client credentials +> when you handle the callback. When `state` is set, `xero-ruby` rejects any +> callback whose `state` is missing, blank, or does not match, and it does so +> before the authorization code is exchanged. + For additional [config](/lib/xero-ruby/configuration.rb) options you can pass an optional named parameter `config: {}` ```ruby config = { timeout: 30, debugging: true } diff --git a/lib/xero-ruby/api_client.rb b/lib/xero-ruby/api_client.rb index df80928a..c64214c9 100644 --- a/lib/xero-ruby/api_client.rb +++ b/lib/xero-ruby/api_client.rb @@ -17,6 +17,7 @@ require 'faraday' require 'base64' require 'cgi' +require 'openssl' require 'json/jwt' module XeroRuby @@ -189,12 +190,39 @@ def validate_tokens(token_set) end def validate_state(params) - if params['state'] != @state + # No state was configured on the client, so there is nothing to compare the + # callback against. This is the SDK default and it leaves the authorization + # code flow without CSRF protection - see the `state` note in the README. + return true if blank_state?(@state) + + callback_state = params['state'] + if blank_state?(callback_state) + raise StandardError.new 'WARNING: OAuth callback is missing the state parameter!' + end + + unless secure_compare(callback_state.to_s, @state.to_s) raise StandardError.new 'WARNING: OAuth callback state does not match!' end return true end + def blank_state?(value) + value.nil? || value.to_s.empty? + end + + # Constant time comparison so the CSRF nonce is not recoverable from response + # timing. Both sides are digested first, which gives the byte loop a fixed + # length regardless of how long the supplied values are. + def secure_compare(a, b) + a_digest = OpenSSL::Digest::SHA256.digest(a) + b_digest = OpenSSL::Digest::SHA256.digest(b) + result = 0 + a_digest.bytesize.times do |i| + result |= a_digest.getbyte(i) ^ b_digest.getbyte(i) + end + result.zero? + end + def decode_jwt(tkn, verify = true) if verify == true diff --git a/spec/api_client_spec.rb b/spec/api_client_spec.rb index 3387923d..401f045b 100644 --- a/spec/api_client_spec.rb +++ b/spec/api_client_spec.rb @@ -61,18 +61,37 @@ expect(api_client.authorization_url).to eq('https://login.xero.com/identity/connect/authorize?response_type=code&client_id=abc&redirect_uri=https%3A%2F%2Fmydomain.com%2Fcallback&scope=openid+profile+email+accounting.transactions+accounting.settings') end - it "Validates state on callback matches @config.state" do - creds = { + let(:stateful_creds) do + { client_id: 'abc', client_secret: '123', redirect_uri: 'https://mydomain.com/callback', scopes: 'openid profile email accounting.transactions accounting.settings', state: "custom-state" } - api_client = XeroRuby::ApiClient.new(credentials: creds) - altered_state = { 'state': 'not-original-state' } + end + + it "Accepts a callback state that matches @config.state" do + api_client = XeroRuby::ApiClient.new(credentials: stateful_creds) + matching_state = { 'state' => 'custom-state' } + expect(api_client.validate_state(matching_state)).to eq(true) + end + + it "Validates state on callback matches @config.state" do + api_client = XeroRuby::ApiClient.new(credentials: stateful_creds) + altered_state = { 'state' => 'not-original-state' } expect { api_client.validate_state(altered_state) }.to raise_error(StandardError, 'WARNING: OAuth callback state does not match!') end + + it "Rejects a callback that omits state when @config.state is set" do + api_client = XeroRuby::ApiClient.new(credentials: stateful_creds) + expect { api_client.validate_state({}) }.to raise_error(StandardError, 'WARNING: OAuth callback is missing the state parameter!') + end + + it "Rejects a callback with a blank state when @config.state is set" do + api_client = XeroRuby::ApiClient.new(credentials: stateful_creds) + expect { api_client.validate_state({ 'state' => '' }) }.to raise_error(StandardError, 'WARNING: OAuth callback is missing the state parameter!') + end end context "Creates a valid client_credentials client" do @@ -147,6 +166,24 @@ expect(api_client.id_token).to eq('existing-id-token') end + it 'rejects a callback that omits state before requesting or mutating tokens' do + api_client.set_token_set(existing_token_set) + + expect(api_client).not_to receive(:token_request) + expect(api_client).not_to receive(:set_token_set) + + expect { + api_client.get_token_set_from_callback('code' => 'attacker-code') + }.to raise_error( + StandardError, + 'WARNING: OAuth callback is missing the state parameter!' + ) + + expect(api_client.token_set).to eq(existing_token_set.with_indifferent_access) + expect(api_client.access_token).to eq('existing-access-token') + expect(api_client.id_token).to eq('existing-id-token') + end + it 'exchanges and stores tokens when a supplied state matches' do callback_params = { 'code' => 'callback-code',