From 459eb207a375d90dc6a3466c6f3854f8dde4a967 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 8 Oct 2026 11:11:57 +0000 Subject: [PATCH 1/4] Initial plan From fe6973cb5a52b7a82bb73b6ace8b500c6b6dc26e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 8 Oct 2026 11:12:40 +0000 Subject: [PATCH 2/4] Apply remaining changes Co-authored-by: senid231 <8393857+senid231@users.noreply.github.com> --- CHANGELOG.md | 5 +++++ README.md | 4 ++-- lib/activeadmin/oidc/engine.rb | 4 +++- spec/security_spec.rb | 10 ++++++++++ 4 files changed, 20 insertions(+), 3 deletions(-) create mode 100644 CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..e278e46 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,5 @@ +# Changelog + +## Unreleased + +- Fix: OIDC `filter_parameters` entries are now anchored regexps matching the exact key, so host params such as `code_id`, `postal_code` or `state_eq` are no longer filtered from logs. diff --git a/README.md b/README.md index 019f924..88462fb 100644 --- a/README.md +++ b/README.md @@ -83,7 +83,7 @@ The gem's Rails engine handles several things so host apps don't have to: * **Login view override** — the engine prepends an SSO-only login page (no email/password fields) to the sessions controller's view path. If your host app ships its own `app/views/active_admin/devise/sessions/new.html.erb`, the gem detects it and backs off — your view wins. * **Session routes** — the engine mounts `GET /admin/login` (renders the SSO landing page) and `DELETE /admin/logout` under `devise_scope`, with the scope name derived from `config.admin_user_class`. Devise normally generates session routes as a side effect of `:database_authenticatable`; without that module the route helpers would not exist and ActiveAdmin's login redirect would 404. * **Path prefix** — the engine registers the strategy with `path_prefix: '/admin/auth'` so the middleware intercepts requests under ActiveAdmin's mount point, and sets `Devise.omniauth_path_prefix` to the prefix Devise declares its routes with. Compatible with Rails 7.2+ and Rails 8's lazy route loading. -* **Parameter filtering** — `code`, `id_token`, `access_token`, `refresh_token`, `state`, and `nonce` are added to `Rails.application.config.filter_parameters`. +* **Parameter filtering** — `code`, `id_token`, `access_token`, `refresh_token`, `state`, and `nonce` are added to `Rails.application.config.filter_parameters` as anchored regexps, so only those exact keys are matched (host params like `code_id` or `state_eq` stay visible). ## Configuration @@ -410,7 +410,7 @@ The gem also adds a unique `(provider, uid)` partial index in its own install mi ### What's filtered from logs -The engine merges `code`, `id_token`, `access_token`, `refresh_token`, `state`, and `nonce` into `Rails.application.config.filter_parameters` so a mid-callback crash can't dump them into production logs. Your own `filter_parameters` entries are preserved. +The engine merges `code`, `id_token`, `access_token`, `refresh_token`, `state`, and `nonce` into `Rails.application.config.filter_parameters` (matched exactly by key, so `code_id` or `state_eq` are not filtered) so a mid-callback crash can't dump them into production logs. Your own `filter_parameters` entries are preserved. ## Logger diff --git a/lib/activeadmin/oidc/engine.rb b/lib/activeadmin/oidc/engine.rb index 9d19178..03d0260 100644 --- a/lib/activeadmin/oidc/engine.rb +++ b/lib/activeadmin/oidc/engine.rb @@ -136,7 +136,9 @@ def controllers end initializer 'activeadmin_oidc.filter_parameters' do |app| - app.config.filter_parameters |= %i[code id_token access_token refresh_token state nonce] + # Anchored: a symbol matches as a substring, so :code would also hide a host's code_id. + oidc_params = %w[code state nonce id_token access_token refresh_token] + app.config.filter_parameters |= oidc_params.map { |key| /\A#{key}\z/i } end # The gem is OIDC-first: mount our SSO landing page at /admin/login diff --git a/spec/security_spec.rb b/spec/security_spec.rb index c9c3b99..0c5018c 100644 --- a/spec/security_spec.rb +++ b/spec/security_spec.rb @@ -27,6 +27,16 @@ "expected filter_parameters to filter #{key.inspect}, got: #{filters_str}" end end + + it "matches keys exactly, so host params like code_id and state_eq are not filtered" do + filter = ActiveSupport::ParameterFilter.new(Rails.application.config.filter_parameters) + result = filter.filter("code" => "a", "state" => "b", "code_id" => "1", "state_eq" => "2", + "nested" => { "code" => "c" }) + + expect(result).to include("code" => "[FILTERED]", "state" => "[FILTERED]", + "code_id" => "1", "state_eq" => "2") + expect(result["nested"]).to eq("code" => "[FILTERED]") + end end describe "oidc_raw_info persistence" do From ec7aca1f85784f0a28e0eac6c3de80ba4c1d311d Mon Sep 17 00:00:00 2001 From: Denis Talakevich Date: Thu, 8 Oct 2026 19:44:26 +0300 Subject: [PATCH 3/4] Filter OIDC params only at the top level, add code_verifier ActiveAdmin forms nest attributes (order[state], product[code]), so an exact-key match at any depth still hid host params. The PKCE code_verifier and session_state were covered by the old substring match and must stay filtered. Co-Authored-By: Clanker --- CHANGELOG.md | 2 +- README.md | 4 ++-- lib/activeadmin/oidc/engine.rb | 7 ++++--- spec/security_spec.rb | 34 ++++++++++++---------------------- 4 files changed, 19 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e278e46..da28333 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,4 +2,4 @@ ## Unreleased -- Fix: OIDC `filter_parameters` entries are now anchored regexps matching the exact key, so host params such as `code_id`, `postal_code` or `state_eq` are no longer filtered from logs. +- Fix: the OIDC `filter_parameters` entry now matches only the exact top-level keys `code`, `code_verifier`, `state`, `session_state`, `nonce`, `id_token`, `access_token` and `refresh_token`, so host params such as `code_id`, `state_eq` or `order[state]` are no longer filtered from logs. If you relied on the old substring match to hide params like `invite_code` or `reset_code`, add them to your own `filter_parameters`. diff --git a/README.md b/README.md index 88462fb..c04100e 100644 --- a/README.md +++ b/README.md @@ -83,7 +83,7 @@ The gem's Rails engine handles several things so host apps don't have to: * **Login view override** — the engine prepends an SSO-only login page (no email/password fields) to the sessions controller's view path. If your host app ships its own `app/views/active_admin/devise/sessions/new.html.erb`, the gem detects it and backs off — your view wins. * **Session routes** — the engine mounts `GET /admin/login` (renders the SSO landing page) and `DELETE /admin/logout` under `devise_scope`, with the scope name derived from `config.admin_user_class`. Devise normally generates session routes as a side effect of `:database_authenticatable`; without that module the route helpers would not exist and ActiveAdmin's login redirect would 404. * **Path prefix** — the engine registers the strategy with `path_prefix: '/admin/auth'` so the middleware intercepts requests under ActiveAdmin's mount point, and sets `Devise.omniauth_path_prefix` to the prefix Devise declares its routes with. Compatible with Rails 7.2+ and Rails 8's lazy route loading. -* **Parameter filtering** — `code`, `id_token`, `access_token`, `refresh_token`, `state`, and `nonce` are added to `Rails.application.config.filter_parameters` as anchored regexps, so only those exact keys are matched (host params like `code_id` or `state_eq` stay visible). +* **Parameter filtering** — top-level `code`, `code_verifier`, `state`, `session_state`, `nonce`, `id_token`, `access_token` and `refresh_token` params are added to `Rails.application.config.filter_parameters`. Only those exact top-level keys are matched, so host params like `code_id`, `state_eq` or `order[state]` stay visible. ## Configuration @@ -410,7 +410,7 @@ The gem also adds a unique `(provider, uid)` partial index in its own install mi ### What's filtered from logs -The engine merges `code`, `id_token`, `access_token`, `refresh_token`, `state`, and `nonce` into `Rails.application.config.filter_parameters` (matched exactly by key, so `code_id` or `state_eq` are not filtered) so a mid-callback crash can't dump them into production logs. Your own `filter_parameters` entries are preserved. +The engine adds the top-level OIDC callback keys (`code`, `code_verifier`, `state`, `session_state`, `nonce`, `id_token`, `access_token`, `refresh_token`) to `Rails.application.config.filter_parameters`, so a mid-callback crash can't dump them into production logs. Only those exact top-level keys are matched: `code_id`, `state_eq` and nested params like `order[state]` are not filtered. Your own `filter_parameters` entries are preserved. ## Logger diff --git a/lib/activeadmin/oidc/engine.rb b/lib/activeadmin/oidc/engine.rb index 03d0260..477c7e0 100644 --- a/lib/activeadmin/oidc/engine.rb +++ b/lib/activeadmin/oidc/engine.rb @@ -136,9 +136,10 @@ def controllers end initializer 'activeadmin_oidc.filter_parameters' do |app| - # Anchored: a symbol matches as a substring, so :code would also hide a host's code_id. - oidc_params = %w[code state nonce id_token access_token refresh_token] - app.config.filter_parameters |= oidc_params.map { |key| /\A#{key}\z/i } + # ActiveSupport::ParameterFilter matches a regexp containing "\." against the dotted + # full key (order.state), so this hides only top-level keys, where the IdP callback puts them. + oidc_params = %w[code code_verifier state session_state nonce id_token access_token refresh_token] + app.config.filter_parameters |= [/\A(?!.*\.)(?:#{Regexp.union(oidc_params).source})\z/i] end # The gem is OIDC-first: mount our SSO landing page at /admin/login diff --git a/spec/security_spec.rb b/spec/security_spec.rb index 0c5018c..1021e6e 100644 --- a/spec/security_spec.rb +++ b/spec/security_spec.rb @@ -13,30 +13,20 @@ # out of the Rails log is a liability — a crash mid-callback will # dump the whole params hash (including `code`, `id_token`, # `access_token`, `refresh_token`) straight into production logs. - # - # Note: once Rails has served a request, filter_parameters is - # compiled into a single combined regex (e.g. - # "(?-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/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 state nonce].each do |key| - expect(filters_str).to include(key), - "expected filter_parameters to filter #{key.inspect}, got: #{filters_str}" + let(:oidc_keys) { %w[code code_verifier state session_state nonce id_token access_token refresh_token] } + let(:host_params) { { "code_id" => "1", "state_eq" => "2", "order" => { "state" => "shipped", "code" => "X" } } } + + [ + ["raw", ->(filters) { filters }], + ["precompiled", ->(filters) { ActiveSupport::ParameterFilter.precompile_filters(filters) }] + ].each do |form, compile| + it "filters the top-level OIDC keys and leaves host params visible (#{form} filters)" do + filter = ActiveSupport::ParameterFilter.new(compile.call(Rails.application.config.filter_parameters)) + result = filter.filter(oidc_keys.index_with("secret").merge(host_params)) + + expect(result).to eq(oidc_keys.index_with("[FILTERED]").merge(host_params)) end end - - it "matches keys exactly, so host params like code_id and state_eq are not filtered" do - filter = ActiveSupport::ParameterFilter.new(Rails.application.config.filter_parameters) - result = filter.filter("code" => "a", "state" => "b", "code_id" => "1", "state_eq" => "2", - "nested" => { "code" => "c" }) - - expect(result).to include("code" => "[FILTERED]", "state" => "[FILTERED]", - "code_id" => "1", "state_eq" => "2") - expect(result["nested"]).to eq("code" => "[FILTERED]") - end end describe "oidc_raw_info persistence" do From a2c78aefde02eed456b4f5b0ac331ab3cfa3233e Mon Sep 17 00:00:00 2001 From: Denis Talakevich Date: Fri, 9 Oct 2026 12:14:14 +0300 Subject: [PATCH 4/4] Filter OIDC token params at any depth again A nested id_token or refresh_token is always a secret, so only the generic callback names (code, state, session_state, nonce) need the top-level limit that keeps host attributes like data.attributes.state visible. Co-Authored-By: Clanker --- CHANGELOG.md | 2 +- README.md | 4 ++-- lib/activeadmin/oidc/engine.rb | 10 +++++++--- spec/security_spec.rb | 13 +++++++++---- 4 files changed, 19 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index da28333..85492a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,4 +2,4 @@ ## Unreleased -- Fix: the OIDC `filter_parameters` entry now matches only the exact top-level keys `code`, `code_verifier`, `state`, `session_state`, `nonce`, `id_token`, `access_token` and `refresh_token`, so host params such as `code_id`, `state_eq` or `order[state]` are no longer filtered from logs. If you relied on the old substring match to hide params like `invite_code` or `reset_code`, add them to your own `filter_parameters`. +- Fix: the OIDC `filter_parameters` entries now match exact keys instead of substrings. `code_verifier`, `id_token`, `access_token` and `refresh_token` are filtered at any depth; `code`, `state`, `session_state` and `nonce` only at the top level. Host params such as `code_id`, `state_eq` or `order[state]` are no longer filtered from logs. If you relied on the old substring match to hide params like `invite_code` or `reset_code`, add them to your own `filter_parameters`. diff --git a/README.md b/README.md index c04100e..46ed360 100644 --- a/README.md +++ b/README.md @@ -83,7 +83,7 @@ The gem's Rails engine handles several things so host apps don't have to: * **Login view override** — the engine prepends an SSO-only login page (no email/password fields) to the sessions controller's view path. If your host app ships its own `app/views/active_admin/devise/sessions/new.html.erb`, the gem detects it and backs off — your view wins. * **Session routes** — the engine mounts `GET /admin/login` (renders the SSO landing page) and `DELETE /admin/logout` under `devise_scope`, with the scope name derived from `config.admin_user_class`. Devise normally generates session routes as a side effect of `:database_authenticatable`; without that module the route helpers would not exist and ActiveAdmin's login redirect would 404. * **Path prefix** — the engine registers the strategy with `path_prefix: '/admin/auth'` so the middleware intercepts requests under ActiveAdmin's mount point, and sets `Devise.omniauth_path_prefix` to the prefix Devise declares its routes with. Compatible with Rails 7.2+ and Rails 8's lazy route loading. -* **Parameter filtering** — top-level `code`, `code_verifier`, `state`, `session_state`, `nonce`, `id_token`, `access_token` and `refresh_token` params are added to `Rails.application.config.filter_parameters`. Only those exact top-level keys are matched, so host params like `code_id`, `state_eq` or `order[state]` stay visible. +* **Parameter filtering** — the OIDC keys are added to `Rails.application.config.filter_parameters` as exact-key matches. `code_verifier`, `id_token`, `access_token` and `refresh_token` are filtered at any depth. The generic `code`, `state`, `session_state` and `nonce` are filtered only as top-level params, where the callback sends them, so host params like `code_id`, `state_eq` or `order[state]` stay visible. ## Configuration @@ -410,7 +410,7 @@ The gem also adds a unique `(provider, uid)` partial index in its own install mi ### What's filtered from logs -The engine adds the top-level OIDC callback keys (`code`, `code_verifier`, `state`, `session_state`, `nonce`, `id_token`, `access_token`, `refresh_token`) to `Rails.application.config.filter_parameters`, so a mid-callback crash can't dump them into production logs. Only those exact top-level keys are matched: `code_id`, `state_eq` and nested params like `order[state]` are not filtered. Your own `filter_parameters` entries are preserved. +The engine adds the OIDC keys to `Rails.application.config.filter_parameters`, so a mid-callback crash can't dump them into production logs. Keys are matched exactly. `code_verifier`, `id_token`, `access_token` and `refresh_token` are filtered at any depth; `code`, `state`, `session_state` and `nonce` only as top-level params. So `code_id`, `state_eq` and nested params like `order[state]` are not filtered. Your own `filter_parameters` entries are preserved. ## Logger diff --git a/lib/activeadmin/oidc/engine.rb b/lib/activeadmin/oidc/engine.rb index 477c7e0..368cb48 100644 --- a/lib/activeadmin/oidc/engine.rb +++ b/lib/activeadmin/oidc/engine.rb @@ -137,9 +137,13 @@ def controllers initializer 'activeadmin_oidc.filter_parameters' do |app| # ActiveSupport::ParameterFilter matches a regexp containing "\." against the dotted - # full key (order.state), so this hides only top-level keys, where the IdP callback puts them. - oidc_params = %w[code code_verifier state session_state nonce id_token access_token refresh_token] - app.config.filter_parameters |= [/\A(?!.*\.)(?:#{Regexp.union(oidc_params).source})\z/i] + # full key (data.attributes.state), so the generic callback names stay visible when nested. + callback_params = %w[code state session_state nonce] + token_params = %w[code_verifier id_token access_token refresh_token] + app.config.filter_parameters |= [ + /\A(?!.*\.)(?:#{Regexp.union(callback_params).source})\z/i, + /\A(?:#{Regexp.union(token_params).source})\z/i + ] end # The gem is OIDC-first: mount our SSO landing page at /admin/login diff --git a/spec/security_spec.rb b/spec/security_spec.rb index 1021e6e..b562324 100644 --- a/spec/security_spec.rb +++ b/spec/security_spec.rb @@ -14,17 +14,22 @@ # dump the whole params hash (including `code`, `id_token`, # `access_token`, `refresh_token`) straight into production logs. let(:oidc_keys) { %w[code code_verifier state session_state nonce id_token access_token refresh_token] } - let(:host_params) { { "code_id" => "1", "state_eq" => "2", "order" => { "state" => "shipped", "code" => "X" } } } + let(:host_params) do + { "code_id" => "1", "state_eq" => "2", "data" => { "attributes" => { "state" => "shipped", "code" => "X" } } } + end + let(:nested_tokens) { %w[code_verifier id_token access_token refresh_token] } [ ["raw", ->(filters) { filters }], ["precompiled", ->(filters) { ActiveSupport::ParameterFilter.precompile_filters(filters) }] ].each do |form, compile| - it "filters the top-level OIDC keys and leaves host params visible (#{form} filters)" do + it "filters OIDC keys and nested tokens, and leaves host params visible (#{form} filters)" do filter = ActiveSupport::ParameterFilter.new(compile.call(Rails.application.config.filter_parameters)) - result = filter.filter(oidc_keys.index_with("secret").merge(host_params)) + params = oidc_keys.index_with("secret").merge(host_params, "auth" => nested_tokens.index_with("secret")) + result = filter.filter(params) - expect(result).to eq(oidc_keys.index_with("[FILTERED]").merge(host_params)) + expect(result).to eq(oidc_keys.index_with("[FILTERED]") + .merge(host_params, "auth" => nested_tokens.index_with("[FILTERED]"))) end end end