fix: validate OAuth state before token exchange

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=<attacker_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 }

lib/xero-ruby/api_client.rb

@@ -17,6 +17,7 @@
 require 'faraday'
 require 'base64'
 require 'cgi'
+require 'openssl'
 require 'json/jwt'
 
 module XeroRuby
@@ -164,6 +165,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 +175,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
 
@@ -188,12 +190,39 @@ def validate_tokens(token_set)
     end
 
     def validate_state(params)
-      if params['state'] != @state
-        raise StandardError.new "WARNING: @config.state: #{@state} and OAuth callback state: #{params['state']} do not match!"
+      # 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
 

spec/api_client_spec.rb

@@ -61,17 +61,36 @@
           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' }
-          expect { api_client.validate_state(altered_state) }.to raise_error(StandardError, 'WARNING: @config.state: custom-state and OAuth callback state:  do not match!')
+        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
 
@@ -100,6 +119,109 @@
     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: 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')
+      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',
+        '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' } }