Failing spec: login view hardcodes /admin/auth/oidc form action#10
Merged
Merged
Conversation
Fivell
force-pushed
the
medium-risk-failing-tests
branch
2 times, most recently
from
ba46688 to
ccf2acf
Compare
Fivell
force-pushed
the
high-risk-failing-tests
branch
from
bd2234b to
b40e24b
Compare
Fivell
force-pushed
the
medium-risk-failing-tests
branch
from
ccf2acf to
f548fdf
Compare
The login view hardcoded /admin/auth/oidc. Hosts that change Devise.omniauth_path_prefix (mounting Devise at a different scope, say) ended up with a button POSTing to a dead URL. Source the path from OmniAuth.config.path_prefix instead - it's the single Rack-level value of where OmniAuth actually listens, and Devise keeps it in sync with its own omniauth_path_prefix. Both view branches (AA v3 form_tag and AA v4 button_to) updated. Why not omniauth_authorize_path? Tried that first - it breaks in the isolated engine case: Devise's helper resolves through engine routes and the engine mount path gets prepended, producing /admin/admin/auth/oidc. OmniAuth.config.path_prefix is plain Rack and sidesteps the issue. Regression spec stubs OmniAuth.config.path_prefix to a sentinel and asserts the form action follows it. The spec's before block force- loads routes first - on Rails 8 with lazy route loading, the stub would otherwise still be active when devise_for runs, tripping Devise's "path_prefix must match Devise.omniauth_path_prefix" guard.
Fivell
force-pushed
the
medium-risk-failing-tests
branch
from
f548fdf to
62831f1
Compare
This was referenced Jun 2, 2026
Fivell
added a commit
that referenced
this pull request
Jun 2, 2026
Follow-ups to PRs #9 and #10 surfaced by the code review. - Apply the same OmniAuth.config.path_prefix substitution to both install-generator templates. New installs were still shipping the hardcoded path PR #10 fixed in the live view. - Default the InactiveError key to :inactive when the model's inactive_message returns nil/blank. The empty key used to render an empty flash; now the user always sees a reason. - Stop leaking custom inactive_message symbols. If the host returns a symbol like :locked_by_admin with no devise.failure translation, the raw symbol used to land in a public flash. Fall back to devise.failure.inactive instead. The disabled-user request spec is consolidated via let helpers (auth_hash, disabling_hook, post_callback) and now uses new_admin_user_session_path / OmniAuth.config.path_prefix instead of hardcoded URLs. Adds a regression spec for the retry leg + an inactive winner row, documenting the expected redirect-and-no- warden-session behaviour.
Fivell
added a commit
that referenced
this pull request
Jun 2, 2026
…nfig.path_prefix (#12) * Add failing tests for three security risks in UserProvisioner Each spec is RED today and documents one production bug. The next commit implements the fixes and turns them green. 1. on_login runs twice when two first sign-ins race. The loser hits RecordNotUnique, the provisioner retries, and the host's hook double-fires (audit log, webhook, welcome email all duplicated). 2. A disabled-by-hook user still gets a row in admin_users. The active_for_authentication? check runs in the controller AFTER the provisioner has already persisted. Hostile attempts grow the table. 3. A pre-seeded admin row (provider/uid still nil) gets adopted by anyone the IdP says owns that email - even when the IdP marks the email as unverified. Attacker registers ceo@example.com at a lax IdP, signs in, walks off with the CEO account. * Harden UserProvisioner against the three security risks Fix the three bugs covered by the failing specs in the previous commit. 1. Don't re-run on_login on the RecordNotUnique retry path. The flag set by save!'s rescue is now read in #call to short-circuit after the row is found: return the winner's record without re-invoking the host hook. Side effects fire exactly once. 2. Check active_for_authentication? inside the provisioner, between on_login and save!. If the hook flipped an inactivity flag, raise ProvisioningError BEFORE writing the row. 3. Refuse to adopt a legacy admin row when email_verified is explicitly false. IdPs that don't ship the claim keep the old behaviour - many never emit it, so a strict "must be true" check would break legitimate hosts. * Preserve the model's inactive_message in the disabled-user flash The previous commit moved the active_for_authentication? guard into the provisioner, but the controller's generic ProvisioningError rescue replaced the model's I18n-translated reason ("Your account is not activated yet.") with the catch-all access_denied_message. Disabled users lost the specific feedback. Add InactiveError (a ProvisioningError subclass that carries the inactive_message symbol) and rescue it separately in the callbacks controller, translating via I18n.t("devise.failure.<symbol>"). The generic rescue stays in place for every other denial path. Also drop the now-dead active_for_authentication? check in the controller - the provisioner runs the same guard earlier and never returns an inactive user. * Use OmniAuth.config.path_prefix for the SSO login form action The login view hardcoded /admin/auth/oidc. Hosts that change Devise.omniauth_path_prefix (mounting Devise at a different scope, say) ended up with a button POSTing to a dead URL. Source the path from OmniAuth.config.path_prefix instead - it's the single Rack-level value of where OmniAuth actually listens, and Devise keeps it in sync with its own omniauth_path_prefix. Both view branches (AA v3 form_tag and AA v4 button_to) updated. Why not omniauth_authorize_path? Tried that first - it breaks in the isolated engine case: Devise's helper resolves through engine routes and the engine mount path gets prepended, producing /admin/admin/auth/oidc. OmniAuth.config.path_prefix is plain Rack and sidesteps the issue. Regression spec stubs OmniAuth.config.path_prefix to a sentinel and asserts the form action follows it. The spec's before block force- loads routes first - on Rails 8 with lazy route loading, the stub would otherwise still be active when devise_for runs, tripping Devise's "path_prefix must match Devise.omniauth_path_prefix" guard.
Fivell
added a commit
that referenced
this pull request
Jun 2, 2026
#11) * Code-review followups: generator templates, inactive_message hardening Follow-ups to PRs #9 and #10 surfaced by the code review. - Apply the same OmniAuth.config.path_prefix substitution to both install-generator templates. New installs were still shipping the hardcoded path PR #10 fixed in the live view. - Default the InactiveError key to :inactive when the model's inactive_message returns nil/blank. The empty key used to render an empty flash; now the user always sees a reason. - Stop leaking custom inactive_message symbols. If the host returns a symbol like :locked_by_admin with no devise.failure translation, the raw symbol used to land in a public flash. Fall back to devise.failure.inactive instead. The disabled-user request spec is consolidated via let helpers (auth_hash, disabling_hook, post_callback) and now uses new_admin_user_session_path / OmniAuth.config.path_prefix instead of hardcoded URLs. Adds a regression spec for the retry leg + an inactive winner row, documenting the expected redirect-and-no- warden-session behaviour. * Drop review-process references from disabled-user spec comments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #9 — please review/merge #9 first.
The SSO login view hardcoded
/admin/auth/oidc. If a host customisesDevise.omniauth_path_prefix(different mount path, different SSO sub-prefix), the button POSTs to a dead URL — OmniAuth listens at the configured prefix, not the literal one.The fix
Both view branches (AA v3
form_tag, AA v4button_to) now source the form action fromOmniAuth.config.path_prefix— the Rack-level path where the OmniAuth middleware actually listens, which Devise keeps in sync with its ownomniauth_path_prefix.Why not
omniauth_authorize_path(resource_name, :oidc)?That was the first attempt. It works for the default setup but breaks when Devise lives inside a mounted engine: Devise's helper resolves through the engine's route set and the engine mount path gets prepended on top of the already-correct prefix, producing
/admin/admin/auth/oidc. Sourcing the path fromOmniAuth.configis plain Rack and avoids the engine routing dance.Spec note
The regression spec stubs
OmniAuth.config.path_prefixto a sentinel. It also force-loads routes inbefore— on Rails 8 with lazy route loading, the firstgetevaluatesconfig/routes.rbwhile the stub is still active, and Devise's "path_prefixmust matchDevise.omniauth_path_prefix" guard raises. Compat shim covers Rails 7.x (reload_routes!) and 8.x (execute_unless_loaded).Test plan
bundle exec rspec spec/requests/login_path_helper_spec.rb— GREENbundle exec rake spec:isolated— 5/0 (covers the engine case the first attempt broke)bundle exec rake spec:engine— 7/0bundle exec rspec— 103/0