diff --git a/README.md b/README.md index 0905e18..b50f9f9 100644 --- a/README.md +++ b/README.md @@ -104,6 +104,17 @@ _other Sign In with Apple guides:_ pem: ENV['APPLE_P8_FILE_CONTENT_WITH_EXTRA_NEWLINE'] } ``` + +## Nonce handling + +As Apple returns to the omniauth callback with a **POST** request, the session values that previously were set are not +available for `SameSite` cookie strategies other than `:none`. When `:strict` or `:lax` are used (recommended settings) +the returning nonce validation, that uses the default `nonce` setting `nonce: :session` will fail. To mitigate this, +there are two options: +* setting `nonce: :local` which will set a short-lived, encrypted cookie with `SameSite: :none` policy to enable nonce + validation without compromising session security. +* setting `nonce: :ignore` will completely skip `nonce` validation, however this isn't recommended as it opens CSRF + attack possibilities ## Contributing diff --git a/lib/omniauth/strategies/apple.rb b/lib/omniauth/strategies/apple.rb index 5ad3a40..576f5ce 100644 --- a/lib/omniauth/strategies/apple.rb +++ b/lib/omniauth/strategies/apple.rb @@ -19,6 +19,8 @@ class Apple < OmniAuth::Strategies::OAuth2 response_mode: 'form_post', scope: 'email name' option :authorized_client_ids, [] + # one of :session (default), :local, :ignore + option :nonce, :session uid { id_info[:sub] } @@ -56,7 +58,8 @@ def is_private_email end def authorize_params - super.merge(nonce: new_nonce) + params = super + options[:nonce] != :ignore ? params.merge(nonce: new_nonce) : params end def callback_url @@ -66,11 +69,26 @@ def callback_url private def new_nonce - session['omniauth.nonce'] = SecureRandom.urlsafe_base64(16) + nonce = SecureRandom.urlsafe_base64(16) + if options[:nonce] == :local + cookies.encrypted[:omniauth_apple_store] = + { same_site: :none, expires: 1.hour.from_now, secure: true, value: nonce } + else + session['omniauth.nonce'] = nonce + end + nonce end def stored_nonce - session.delete('omniauth.nonce') + return session.delete("omniauth.nonce") unless options[:nonce] == :local + + nonce = cookies.encrypted[:omniauth_apple_store] + cookies.delete :omniauth_apple_store + nonce + end + + def cookies + request.env["action_dispatch.cookies"] end def id_info @@ -105,7 +123,7 @@ def verify_claims!(id_token) verify_aud!(id_token) verify_iat!(id_token) verify_exp!(id_token) - verify_nonce!(id_token) if id_token[:nonce_supported] + verify_nonce!(id_token) if id_token[:nonce_supported] && options[:nonce] != :ignore end def verify_iss!(id_token) diff --git a/omniauth-apple.gemspec b/omniauth-apple.gemspec index e6ec42f..fd3f26b 100644 --- a/omniauth-apple.gemspec +++ b/omniauth-apple.gemspec @@ -43,4 +43,5 @@ Gem::Specification.new do |spec| spec.add_development_dependency "rspec", "~> 3.9" spec.add_development_dependency "webmock", "~> 3.8" spec.add_development_dependency "simplecov", "~> 0.18" + spec.add_development_dependency "actionpack", ">= 4.2" end diff --git a/spec/omniauth/apple/vesion_spec.rb b/spec/omniauth/apple/version_spec.rb similarity index 100% rename from spec/omniauth/apple/vesion_spec.rb rename to spec/omniauth/apple/version_spec.rb diff --git a/spec/omniauth/strategies/apple_spec.rb b/spec/omniauth/strategies/apple_spec.rb index 2709d1b..95ea807 100644 --- a/spec/omniauth/strategies/apple_spec.rb +++ b/spec/omniauth/strategies/apple_spec.rb @@ -290,6 +290,78 @@ end end + context 'nonce with local store' do + let(:cookies) { ActionDispatch::Cookies::CookieJar.new(request) } + + before do + subject.options.nonce = :local + allow(cookies).to receive(:encrypted).and_return(cookies) + allow(cookies).to receive(:handle_options) + request.env["action_dispatch.cookies"] = cookies + # required to proof that we actually fail on the local store + strategy.authorize_params + subject.session['omniauth.nonce'] = id_token_payload['nonce'] + end + + context 'when in store succeeds' do + before do + cookies.encrypted[:omniauth_apple_store] = + { same_site: :none, expires: 1.hour.from_now, secure: true, value: id_token_payload['nonce'] } + end + + it do + expect { subject.info }.not_to raise_error + end + end + + context 'when differs from store fails' do + before do + cookies.encrypted[:omniauth_apple_store] = + { same_site: :none, expires: 1.hour.from_now, secure: true, value: 'abd' } + end + + it do + expect { subject.info }.to raise_error( + OmniAuth::Strategies::OAuth2::CallbackError, 'id_token_claims_invalid | nonce invalid' + ) + end + end + + context 'when missing from store fails' do + before do + cookies.encrypted[:omniauth_apple_store] = nil + end + + it do + expect { subject.info }.to raise_error( + OmniAuth::Strategies::OAuth2::CallbackError, 'id_token_claims_invalid | nonce invalid' + ) + end + end + end + + context 'ignores nonce' do + context 'when differs from session' do + before do + subject.options.nonce = :ignore + subject.session['omniauth.nonce'] = 'abc' + end + it do + expect { subject.info }.not_to raise_error + end + end + + context 'when missing from session' do + before do + subject.options.nonce = :ignore + subject.session.delete('omniauth.nonce') + end + it do + expect { subject.info }.not_to raise_error + end + end + end + context 'with a spoofed email in the user payload' do before do request.params['user'] = { diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index b869d3c..d3c6862 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -2,6 +2,7 @@ require File.join('bundler', 'setup') require 'rspec' +require 'action_dispatch' require 'simplecov' SimpleCov.start('test_frameworks')