From 4aa0e1b4ab047f002784e5587156f0e6cb0959f8 Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Mon, 9 Aug 2021 13:25:10 +0100
Subject: [PATCH] FIX: Ensure nonce mismatch causes auth to fail correctly
 (#15)

---
 lib/omniauth_open_id_connect.rb           | 12 +++++++++---
 spec/lib/omniauth_open_id_connect_spec.rb |  7 +++++++
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/lib/omniauth_open_id_connect.rb b/lib/omniauth_open_id_connect.rb
index cdb4f20..6d67150 100644
--- a/lib/omniauth_open_id_connect.rb
+++ b/lib/omniauth_open_id_connect.rb
@@ -9,6 +9,8 @@ module ::OmniAuth
 
   module Strategies
     class OpenIDConnect < OmniAuth::Strategies::OAuth2
+      class NonceVerifyError < StandardError; end
+
       option :scope, "openid"
       option :discovery, true
       option :discovery_document, nil
@@ -95,14 +97,13 @@ module ::OmniAuth
           oauth2_callback_phase = super
           return oauth2_callback_phase if env['omniauth.error']
 
-          if id_token_info["nonce"].nil? || id_token_info["nonce"].empty? || id_token_info["nonce"] != session.delete("omniauth.nonce")
-            return fail!(:csrf_detected, CallbackError.new(:csrf_detected, "CSRF detected"))
-          end
           oauth2_callback_phase
         rescue ::OmniAuth::OpenIDConnect::DiscoveryError => e
           fail!(:openid_connect_discovery_error, e)
         rescue ::JWT::DecodeError => e
           fail!(:jwt_decode_failed, e)
+        rescue NonceVerifyError => e
+          fail!(:jwt_nonce_verify_failed, e)
         end
       end
 
@@ -124,6 +125,11 @@ module ::OmniAuth
             verify_iat: false,
             verify_jti: false
           )
+
+          if decoded["nonce"].nil? || decoded["nonce"].empty? || decoded["nonce"] != session.delete("omniauth.nonce")
+            raise NonceVerifyError.new "JWT nonce is missing, or does not match"
+          end
+
           verbose_log("Verified JWT\n\n#{decoded.to_yaml}")
 
           decoded
diff --git a/spec/lib/omniauth_open_id_connect_spec.rb b/spec/lib/omniauth_open_id_connect_spec.rb
index 7c42bed..0e72a94 100644
--- a/spec/lib/omniauth_open_id_connect_spec.rb
+++ b/spec/lib/omniauth_open_id_connect_spec.rb
@@ -5,7 +5,9 @@ require 'rails_helper'
 
 describe OmniAuth::Strategies::OpenIDConnect do
   let(:app) do
+    @app_called = false
     lambda do |*args|
+      @app_called = true
       [200, {}, ['Hello.']]
     end
   end
@@ -109,6 +111,7 @@ describe OmniAuth::Strategies::OpenIDConnect do
         subject.options.error_handler = lambda { |error, message| return "https://example.com/error_redirect" if message.include?("forgot password") }
         expect(subject.callback_phase[0]).to eq(302)
         expect(subject.callback_phase[1]["Location"]).to eq("https://example.com/error_redirect")
+        expect(@app_called).to eq(false)
       end
 
       context "with userinfo disabled" do
@@ -128,16 +131,19 @@ describe OmniAuth::Strategies::OpenIDConnect do
           expect(subject.info[:name]).to eq("My Auth Token Name")
           expect(subject.info[:email]).to eq("tokenemail@example.com")
           expect(subject.extra[:id_token]).to eq(@token)
+          expect(@app_called).to eq(true)
         end
 
         it "checks the nonce" do
           subject.session["omniauth.nonce"] = "overriddenNonce"
           expect(subject.callback_phase[0]).to eq(302)
+          expect(@app_called).to eq(false)
         end
 
         it "checks the issuer" do
           subject.options.client_id = "overriddenclientid"
           expect(subject.callback_phase[0]).to eq(302)
+          expect(@app_called).to eq(false)
         end
       end
 
@@ -165,6 +171,7 @@ describe OmniAuth::Strategies::OpenIDConnect do
           expect(subject.uid).to eq("someuserid")
           expect(subject.info[:name]).to eq("My Userinfo Name")
           expect(subject.info[:email]).to eq("userinfoemail@example.com")
+          expect(@app_called).to eq(true)
         end
       end
     end