Defensive fixes: non-Hash raw_info, filter state/nonce

Two small hardening tweaks from the security review pass:

1. Controller guards against a non-Hash extra.raw_info from the
   OIDC strategy. A well-behaved omniauth_openid_connect run
   always gives us a Hash, but a custom or buggy strategy could
   set it to a String, nil, Array, etc — and the old code went
   straight to .to_h.transform_keys and crashed the callback
   action with a 500. Now we collapse anything non-Hash to {}
   and rebuild sub/email from the top-level auth hash, which is
   exactly the fallback path the next two lines were already
   doing for the normal case. Covered by a new request spec.

2. filter_parameters now also masks 'state' and 'nonce'. They
   aren't secrets in the bearer-token sense, but they are
   session-bound single-use values and leaking them into logs
   serves no purpose. Industry default is to treat them as
   opaque. Updated the security spec to assert on both keys.

The agent review also flagged the failure_message log line as
a potential PII leak, but closer reading of Devise's
OmniauthCallbacksController#failure_message shows it returns a
short humanized error code (error_reason || error ||
error.type) — not the raw OAuth error_description from the IdP.
Leaving that log alone.

Author: Fivell

app/controllers/active_admin/oidc/devise/omniauth_callbacks_controller.rb

@@ -18,9 +18,15 @@ module Devise
       # (`:oidc`, from ActiveAdmin::Oidc::Engine::PROVIDER_NAME).
       class OmniauthCallbacksController < ::Devise::OmniauthCallbacksController
         def oidc
-          auth   = request.env["omniauth.auth"] || {}
-          info   = auth["info"] || {}
-          extra  = auth.dig("extra", "raw_info") || {}
+          auth  = request.env["omniauth.auth"] || {}
+          info  = auth["info"] || {}
+          # Defensive: an OIDC strategy is supposed to put a Hash at
+          # extra.raw_info, but a misbehaving/custom strategy could
+          # set something else (String, nil, Array). We only trust a
+          # Hash-shaped value; anything else collapses to {} and we
+          # rebuild `sub`/`email` from the top-level auth hash below.
+          extra = auth.dig("extra", "raw_info")
+          extra = {} unless extra.is_a?(Hash)
 
           claims = extra.to_h.transform_keys(&:to_s)
           claims["sub"]   = auth["uid"] if claims["sub"].blank? && auth["uid"].present?

lib/activeadmin/oidc/engine.rb

@@ -53,13 +53,18 @@ def controllers
         end
       end
 
-      # Prevent OAuth codes and tokens from ever landing in Rails logs.
-      # If the host app crashes mid-callback Rails will dump the full
-      # params hash into the log; we merge the known-sensitive keys
-      # into `filter_parameters` so they come out as [FILTERED]. The
-      # host app's own filter_parameters are preserved — we only add.
+      # Prevent OAuth codes, tokens and CSRF/replay guards from ever
+      # landing in Rails logs. If the host app crashes mid-callback
+      # Rails will dump the full params hash into the log; we merge
+      # the known-sensitive keys into `filter_parameters` so they
+      # come out as [FILTERED]. The host app's own filter_parameters
+      # are preserved — we only add.
+      #
+      # `state` and `nonce` aren't "secret" in the strong sense, but
+      # they're single-use session-bound values and treating them as
+      # opaque in logs is the industry default.
       initializer "activeadmin_oidc.filter_parameters" do |app|
-        app.config.filter_parameters |= %i[code id_token access_token refresh_token]
+        app.config.filter_parameters |= %i[code id_token access_token refresh_token state nonce]
       end
     end
   end

spec/requests/omniauth_callback_spec.rb

@@ -127,6 +127,28 @@ def trigger_callback
     end
   end
 
+  context "misbehaving strategy returns non-Hash raw_info" do
+    # Defensive: a custom or buggy OIDC strategy could set
+    # auth.extra.raw_info to a String (or nil, Array, etc) instead of
+    # the expected Hash of claims. The controller must not crash with
+    # a NoMethodError/TypeError — it should fall back to building
+    # claims from auth.uid and auth.info["email"] and proceed normally.
+    it "falls back to top-level auth fields and still provisions the user" do
+      OmniAuth.config.mock_auth[:oidc] = OmniAuth::AuthHash.new(
+        provider: "oidc",
+        uid:      "sub-nohash",
+        info:     { "email" => "nohash@example.com", "name" => "No Hash" },
+        extra:    { "raw_info" => "this-should-be-a-hash-but-isnt" }
+      )
+
+      expect { trigger_callback }.to change(AdminUser, :count).by(1)
+
+      user = AdminUser.find_by(provider: "oidc", uid: "sub-nohash")
+      expect(user).not_to be_nil
+      expect(user.email).to eq("nohash@example.com")
+    end
+  end
+
   context "OmniAuth strategy failure (e.g. invalid_credentials)" do
     before do
       OmniAuth.config.mock_auth[:oidc] = :invalid_credentials

spec/security_spec.rb

@@ -19,10 +19,10 @@
     #   "(?-mix:(?i:code)|(?i:id_token)|(?i:access_token)|...)"
     # ), so we stringify the collection and look for each key in it
     # rather than asserting on discrete symbol entries.
-    it "includes the OIDC token/code keys so they never end up in Rails logs" do
+    it "includes the OIDC token/code/state/nonce keys so they never end up in Rails logs" do
       filters_str = Rails.application.config.filter_parameters.map(&:to_s).join(" ")
 
-      %w[code id_token access_token refresh_token].each do |key|
+      %w[code id_token access_token refresh_token state nonce].each do |key|
         expect(filters_str).to include(key),
           "expected filter_parameters to filter #{key.inspect}, got: #{filters_str}"
       end