From 98553d7c2c9b03d0d516c4595c515b80c03cf13d Mon Sep 17 00:00:00 2001
From: Markus Koller <mkoller@gitlab.com>
Date: Tue, 2 Nov 2021 18:14:40 +0100
Subject: [PATCH] Verify state before using errors received from provider

This avoids content spoofing attacks by crafting a URL with malicious
messages, because the `state` param is only present in the session after
a valid OAuth2 authentication flow.
---
 lib/omniauth/strategies/oauth2.rb       |  6 ++--
 spec/omniauth/strategies/oauth2_spec.rb | 41 ++++++++++++++++++++++---
 2 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/lib/omniauth/strategies/oauth2.rb b/lib/omniauth/strategies/oauth2.rb
index 0803402..e445214 100644
--- a/lib/omniauth/strategies/oauth2.rb
+++ b/lib/omniauth/strategies/oauth2.rb
@@ -83,10 +83,10 @@ def token_params
 
       def callback_phase # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/MethodLength, Metrics/PerceivedComplexity
         error = request.params["error_reason"] || request.params["error"]
-        if error
-          fail!(error, CallbackError.new(request.params["error"], request.params["error_description"] || request.params["error_reason"], request.params["error_uri"]))
-        elsif !options.provider_ignores_state && (request.params["state"].to_s.empty? || request.params["state"] != session.delete("omniauth.state"))
+        if !options.provider_ignores_state && (request.params["state"].to_s.empty? || request.params["state"] != session.delete("omniauth.state"))
           fail!(:csrf_detected, CallbackError.new(:csrf_detected, "CSRF detected"))
+        elsif error
+          fail!(error, CallbackError.new(request.params["error"], request.params["error_description"] || request.params["error_reason"], request.params["error_uri"]))
         else
           self.access_token = build_access_token
           self.access_token = access_token.refresh! if access_token.expired?
diff --git a/spec/omniauth/strategies/oauth2_spec.rb b/spec/omniauth/strategies/oauth2_spec.rb
index 3b988f8..68b5d62 100644
--- a/spec/omniauth/strategies/oauth2_spec.rb
+++ b/spec/omniauth/strategies/oauth2_spec.rb
@@ -97,14 +97,47 @@ def app
   end
 
   describe "#callback_phase" do
-    subject { fresh_strategy }
-    it "calls fail with the client error received" do
-      instance = subject.new("abc", "def")
+    subject(:instance) { fresh_strategy.new("abc", "def") }
+
+    let(:params) { {"error_reason" => "user_denied", "error" => "access_denied", "state" => state} }
+    let(:state) { "secret" }
+
+    before do
       allow(instance).to receive(:request) do
-        double("Request", :params => {"error_reason" => "user_denied", "error" => "access_denied"})
+        double("Request", :params => params)
+      end
+
+      allow(instance).to receive(:session) do
+        double("Session", :delete => state)
       end
+    end
+
+    it "calls fail with the error received" do
+      expect(instance).to receive(:fail!).with("user_denied", anything)
+
+      instance.callback_phase
+    end
+
+    it "calls fail with the error received if state is missing and CSRF verification is disabled" do
+      params["state"] = nil
+      instance.options.provider_ignores_state = true
 
       expect(instance).to receive(:fail!).with("user_denied", anything)
+
+      instance.callback_phase
+    end
+
+    it "calls fail with a CSRF error if the state is missing" do
+      params["state"] = nil
+
+      expect(instance).to receive(:fail!).with(:csrf_detected, anything)
+      instance.callback_phase
+    end
+
+    it "calls fail with a CSRF error if the state is invalid" do
+      params["state"] = "invalid"
+
+      expect(instance).to receive(:fail!).with(:csrf_detected, anything)
       instance.callback_phase
     end
   end