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 dc9d679a..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 @@ -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 diff --git a/spec/api_client_spec.rb b/spec/api_client_spec.rb index 617433b7..401f045b 100644 --- a/spec/api_client_spec.rb +++ b/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' } }