Repository navigation
Anchor OIDC filter_parameters so host params like code_id and state_eq stay visible - #27
Conversation
Co-authored-by: senid231 <8393857+senid231@users.noreply.github.com>
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
There was a problem hiding this comment.
🟡 Changes recommended
The implementation exposes nested exact OIDC keys, contradicting the stated requirement to filter them at every nesting depth.
4 open findings
What changed in this PR
Narrows OIDC parameter filtering to avoid hiding similarly named host parameters.
Changes:
- Replaces symbol filters with anchored regex filtering.
- Adds raw and precompiled filter tests.
- Updates documentation and changelog.
| File | Description |
|---|---|
lib/activeadmin/oidc/engine.rb |
Introduces exact, top-level OIDC filters. |
spec/security_spec.rb |
Tests filtering and host parameter visibility. |
README.md |
Documents filtering behavior. |
CHANGELOG.md |
Records the behavior change. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # 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] |
There was a problem hiding this comment.
Partly fixed in a2c78ae. Nested code_verifier, id_token, access_token and refresh_token are filtered again at any depth, since a nested token is always a secret.
code, state, session_state and nonce stay top-level only on purpose. #26 does not ask for nested filtering: "per key at any depth" there only explains why the callback params stay hidden. Its example is JSON:API, where a host resource with a code or state attribute shows up as data.attributes.state. Hiding that is the bug #26 reports. The IdP callback sends these keys only at the top level.
| 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)) |
There was a problem hiding this comment.
Fixed in a2c78ae for the token keys: the spec now nests code_verifier, id_token, access_token and refresh_token under auth and expects them filtered, in both raw and precompiled form. Nested code and state are still expected visible, for the reason in the engine thread.
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
Release 3.0.1 Ships the exact-key filter_parameters fix from #27. Params whose names only contain code, state, nonce or a token name, and nested code, state, session_state and nonce, are no longer hidden in logs. The CHANGELOG tells hosts how to filter any that carry secrets. Co-Authored-By: Clanker
The engine added its OIDC keys to
filter_parametersas symbols, whichActiveSupport::ParameterFiltermatches as case-insensitive substrings. So:codeand:statealso hid host params such ascode_id,postal_codeandstate_eq, and a host couldn't override it from its own initializers.The keys are now exact-key regexps in two groups:
code_verifier,id_token,access_token,refresh_token: filtered at any depth. Nested, they are always secrets.code,state,session_state,nonce: filtered only as top-level params, where the IdP callback sends them. Nested, these names are usually host attributes (data.attributes.state,order[code]), which is the case filter_parameters symbols hide host params like code_id and state_eq #26 reports. The regexp contains\., so ParameterFilter matches it against the dotted full key.code_verifier(read from params by omniauth_openid_connect with PKCE) andsession_statewere covered by the old substring match, so they are now listed explicitly.Behavior change: hosts that relied on the substring match to hide params like
invite_codemust add their own filters. The CHANGELOG says so.Verified with a spec that runs the real filter, raw and precompiled, plus
rake spec:all.Fixes #26