security: auth bypasses, constant-time comparisons, MFA password re-auth, OAuth revocation#1519
security: auth bypasses, constant-time comparisons, MFA password re-auth, OAuth revocation#1519dgilperez wants to merge 10 commits intowe-promise:mainfrom
Conversation
Revocation is meaningless if we still accept revoked tokens on subsequent requests. Add `!access_token.revoked?` to the validity check in `authenticate_oauth`. Ported from origin/security/pentest-2026-03-02 (8c4d0f9).
String equality (==) on secrets is susceptible to timing side-channel attacks. Switch `ApiKey#key_matches?` to `ActiveSupport::SecurityUtils.secure_compare` so the comparison takes constant time regardless of where the first differing byte is. Ported from origin/security/pentest-2026-03-02 (0b04682).
…-15) `Array#index(code)` uses plain equality which short-circuits on the first differing byte, leaking information about backup codes via timing. Replace with a block form that uses `ActiveSupport::SecurityUtils.secure_compare` against every stored code. Ported from origin/security/pentest-2026-03-02 (0b04682).
API login only checked password — deactivated users could still obtain fresh OAuth tokens. Mirror the web session guard by checking `user.active?` after authentication and returning 401 "Account has been deactivated". Ported from origin/security/pentest-2026-03-02 (0b04682) + the corresponding controller test from 0c2dd99.
Web registrations honor `Setting.onboarding_state == \"closed\"` via `RegistrationsController#ensure_signup_open`, but the mobile/API signup endpoint bypassed that check. Add an equivalent guard for self-hosted deployments so closing registrations actually closes them. Ported from origin/security/pentest-2026-03-02 (0b04682) + test from 0c2dd99.
The refresh endpoint issued a new access token before checking whether the underlying user was still active, letting deactivated users extend their session indefinitely. Move the active check after token issuance and also revoke the newly minted token on failure so all outstanding tokens for a deactivated user are dead. Ported from origin/security/pentest-2026-03-02 (0b04682) + test from 0c2dd99. Slight deviation from source: also explicitly revoke the newly issued token (the old token is already revoked earlier in the flow), which matches the test's "all tokens revoked" assertion.
When an instance is configured for SSO-only authentication, the web flow rejects password logins but the API accepted them, providing a bypass for the SSO-only policy. Mirror the web behavior by returning 403 when `AuthConfig.local_login_enabled?` is false. Ported from origin/security/pentest-2026-03-02 (8bb8c99) + test from 0c2dd99. OTP rate-limiting additions in d053f61 are PR-5 scope and intentionally not included here.
An attacker with a hijacked session could silently enable MFA on a victim's account (locking them out) or disable an existing MFA protection. Require the user to re-enter their password on both `POST /mfa` (enable) and `DELETE /mfa` (disable), matching the sensitive-operation re-auth pattern. In addition to the controller change ported from origin/security/pentest-2026-03-02 (8bb8c99), also wire the views so the forms actually submit a `password` parameter: - `app/views/mfa/new.html.erb` adds a password field to the enable form. - `app/views/settings/securities/show.html.erb` replaces the simple DELETE link with a disclosure that reveals a password-gated delete form. - Locale keys for the new form labels and `invalid_password` flash. Backup-code unit tests (reuse prevention, invalid-code rejection, blank input) are added here because they exercise the constant-time comparison change introduced in FIX-15; the tests originate in the source branch alongside these MFA fixes.
brin-security-scanner
Bot
added
the
contributor:verified
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds server-side gates and active-user checks across API auth and MFA: registration closure, local-login enforcement, deactivated-account handling with token revocation, revoked-token rejection, password re-auth for MFA enable/disable, and constant-time comparisons for keys/backup codes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant ApiAuth as Api::V1::AuthController
participant OAuth as Doorkeeper::AccessToken
participant DB as User/Device Store
Client->>ApiAuth: POST /api/v1/login (credentials)
ApiAuth->>ApiAuth: check AuthConfig.local_login_enabled?
alt local login disabled
ApiAuth-->>Client: 403 {"error":"Local login is disabled. Please use SSO."}
else local login enabled
ApiAuth->>DB: verify credentials
alt credentials invalid
ApiAuth-->>Client: 401 {"error":"Invalid credentials"}
else credentials valid
ApiAuth->>DB: check user.active?
alt user deactivated
ApiAuth->>OAuth: revoke all outstanding tokens for resource_owner_id
ApiAuth-->>Client: 401 {"error":"Account has been deactivated"}
else user active
ApiAuth->>OAuth: validate/issue tokens, update device last_seen
ApiAuth-->>Client: 200 { access_token, refresh_token, ... }
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17b36fcc43
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/controllers/api/v1/auth_controller.rb (1)
279-299:⚠️ Potential issue | 🟠 MajorCheck
user.active?before issuing the new token, and consider revoking all of the user's tokens.Two concerns with the current ordering:
- Race / wasted work:
new_tokenis created at line 280 and only revoked at line 296 after loading the user. Between those points, a concurrent request presentingnew_tokenwould pass the updated OAuth gate inbase_controller.rb(since it isn't revoked yet). For a denied request you also perform 3 DB writes (create + 2 revokes) instead of 1.- Scope of revocation: the test at
test/controllers/api/v1/auth_controller_test.rb:874asserts all of the deactivated user's tokens are revoked, but the controller only revokes the current pair. The test passes only because the test user has exactly one prior token; a deactivated user with tokens on other devices would retain valid access. Given the intent ("no access for deactivated users"), revoking all outstandingDoorkeeper::AccessTokens for the user would match the test's assertion and the security goal.Also note:
User.find(access_token.resource_owner_id)will raiseActiveRecord::RecordNotFoundfor a deleted user and surface as 404 viahandle_not_found, which is inconsistent with the 401 returned by the analogous path inbase_controller.rb#authenticate_oauth(lines 72–76). Preferfind_by+ explicit 401.🔒 Proposed refactor
- # Create new access token - new_token = Doorkeeper::AccessToken.create!( - application: access_token.application, - resource_owner_id: access_token.resource_owner_id, - mobile_device_id: access_token.mobile_device_id, - expires_in: 30.days.to_i, - scopes: access_token.scopes, - use_refresh_token: true - ) - - # Revoke old access token - access_token.revoke - - # Reject deactivated users on token refresh - user = User.find(access_token.resource_owner_id) - unless user.active? - # Revoke the newly issued token as well — no access for deactivated users - new_token.revoke - render json: { error: "Account has been deactivated" }, status: :unauthorized - return - end + # Reject deactivated/deleted users BEFORE issuing a new token. + user = User.find_by(id: access_token.resource_owner_id) + unless user&.active? + # Revoke the presented refresh token and all outstanding access tokens + # for this user — no access for deactivated accounts. + Doorkeeper::AccessToken + .where(resource_owner_id: access_token.resource_owner_id, revoked_at: nil) + .find_each(&:revoke) + render json: { error: "Account has been deactivated" }, status: :unauthorized + return + end + + # Create new access token + new_token = Doorkeeper::AccessToken.create!( + application: access_token.application, + resource_owner_id: access_token.resource_owner_id, + mobile_device_id: access_token.mobile_device_id, + expires_in: 30.days.to_i, + scopes: access_token.scopes, + use_refresh_token: true + ) + + # Revoke old access token + access_token.revoke🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/api/v1/auth_controller.rb` around lines 279 - 299, Check the user's existence and active? status before creating a new token: replace User.find(access_token.resource_owner_id) with User.find_by(id: access_token.resource_owner_id) and if user.nil? or !user.active? then revoke all of that user's Doorkeeper::AccessToken records (e.g. Doorkeeper::AccessToken.where(resource_owner_id: access_token.resource_owner_id).find_each { |t| t.revoke }) and render json error with status :unauthorized; only create new_token via Doorkeeper::AccessToken.create!(...) after the active check passes, and remove the later new_token.revoke since revocation of all tokens covers it.app/views/settings/securities/show.html.erb (1)
22-43:⚠️ Potential issue | 🟠 MajorGate the MFA UI on
password_digest.present?for SSO-only users.Elsewhere in this very file (line 69 and the warning block at lines 87–94) the SSO section already special-cases users with blank
password_digest. The MFA block does not, so SSO-only users see an "Enable 2FA" link leading to a form they cannot submit, and — if MFA is somehow enabled — a disable form whose password field they cannot satisfy. Either hide this UI or route them through an SSO re-auth for these sensitive operations. See the controller-side comment onapp/controllers/mfa_controller.rbfor the root cause.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/settings/securities/show.html.erb` around lines 22 - 43, Wrap the existing MFA UI in app/views/settings/securities/show.html.erb with an additional check for whether the current user has a local password (e.g. Current.user.password_digest.present?): only render the enable/disable MFA branch (the if Current.user.otp_required? block, the styled_form_with that posts to disable_mfa_path, and the DS::Link to new_mfa_path) when password_digest.present?; otherwise hide those controls (or render the existing SSO warning block) so SSO-only users cannot see or submit the enable/disable MFA forms.app/controllers/mfa_controller.rb (1)
10-57:⚠️ Potential issue | 🟠 MajorSSO-only users cannot enable or disable MFA due to missing password digest checks.
The
createanddisableactions inapp/controllers/mfa_controller.rbcallCurrent.user.authenticate(params[:password])without verifying thatpassword_digestexists. Per the User model, SSO-only users havenilpassword digest and authenticate exclusively via OIDC;authenticate()will unconditionally fail for such users, blocking both MFA setup and removal flows.Additionally,
app/views/settings/securities/show.html.erbdisplays the "Enable 2FA" link and disable form without gating onCurrent.user.password_digest.present?, unlike the SSO disconnect section in the same file (lines 69 and 87). SSO-only users will see a password field they cannot satisfy.Consider one of:
- Skip password re-auth for users without a
password_digest(optionally require SSO provider re-confirmation instead).- Hide the MFA UI for SSO-only users and redirect to an SSO re-auth flow.
- At minimum, check
password_digest.present?before invokingauthenticate().🛡️ Sketch of a guarded re-auth helper
def create - unless Current.user.authenticate(params[:password]) + unless password_reauth_ok? Current.user.disable_mfa! redirect_to new_mfa_path, alert: t(".invalid_password") return end ... end def disable - unless Current.user.authenticate(params[:password]) + unless password_reauth_ok? redirect_to settings_security_path, alert: t(".invalid_password") return end ... end private + def password_reauth_ok? + # SSO-only users cannot re-auth via password; gate the UI elsewhere + # or wire an SSO re-auth flow before invoking this action. + return false if Current.user.password_digest.blank? + Current.user.authenticate(params[:password]).present? + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/mfa_controller.rb` around lines 10 - 57, The create and disable actions call Current.user.authenticate(params[:password]) without checking whether the user has a password_digest, which breaks SSO-only users; update both actions (create and disable in mfa_controller.rb) to first check Current.user.password_digest.present? and only call authenticate when true, otherwise either skip the password re-auth branch (e.g. redirect to SSO re-auth or surface an appropriate error) or block/show alternate UI; also update the settings/securities view to gate the "Enable 2FA" link and the disable form behind password_digest.present? (matching the SSO disconnect gating) so SSO-only users don’t see a password field they can’t satisfy.
🧹 Nitpick comments (3)
app/controllers/mfa_controller.rb (1)
11-15: Minor: typo in password forces a full MFA re-setup.Calling
Current.user.disable_mfa!on a failed password clearsotp_secret(andotp_backup_codes), which means a single password typo forces the user to re-scan the QR code on the next attempt. This mirrors the existinginvalid_codebranch, so it is consistent — but for the password path the verification code was never even evaluated, so there is no reason to invalidate the in-progress setup. Consider leaving the setup state intact for the password-only failure and only resetting on actual code mismatch / session expiry.♻️ Suggested change
def create unless Current.user.authenticate(params[:password]) - Current.user.disable_mfa! redirect_to new_mfa_path, alert: t(".invalid_password") return end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/mfa_controller.rb` around lines 11 - 15, The password-failure branch in the MFA controller is incorrectly calling Current.user.disable_mfa!, which clears otp_secret and backup codes and forces a full re-setup; remove (or relocate) the call to Current.user.disable_mfa! from the password check so that a bad password only redirects to new_mfa_path with the alert (leave the in-progress setup intact), and keep disable_mfa! only in the branches that actually represent code mismatch or session expiry (the existing invalid_code/session-expiry handling) so only real verification failures reset otp_secret/otp_backup_codes.app/views/settings/securities/show.html.erb (1)
24-36: Minor UX: consider keeping the disclosure open on validation errors.When the password is wrong, the
disableaction redirects back here with a flash alert, but the<details>element will re-render collapsed, hiding the form the user just interacted with and the alert's context. Two lightweight options:
- Pass
open: truewhen a relevant flash is present:<details class="group" <%= "open" if flash[:alert].present? %>>.- Or conditionally anchor/scroll to the section via a
flash.now-driven controller render path.Not a blocker, just friction in the error path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/settings/securities/show.html.erb` around lines 24 - 36, The details element collapses after a failed disable_mfa submission; update the <details> rendering in show.html.erb to include the open attribute when a validation error/alert exists (e.g. use flash[:alert].present? or another flash key) so the disclosure remains expanded after the redirect from the disable action; locate the <details class="group"> element around the styled_form_with (which posts to disable_mfa_path) and conditionally render open (or alternatively implement a flash-driven anchor/scroll path using flash.now and the controller render) to keep the form visible on errors.test/controllers/mfa_controller_test.rb (1)
54-62: Consider asserting the resulting secret/backup-codes state.The new test verifies that
otp_required?remains false and the redirect target, but doesn't pin down the side effect of the wrong-password branch (currentlydisable_mfa!is called, which nilsotp_secretandotp_backup_codes). Asserting on that state would document the intended behavior and catch regressions — especially relevant if the controller is changed to preserve the setup state on password-only failures (see related comment onmfa_controller.rb).post mfa_path, params: { code: totp.now, password: "wrongpassword" } assert_not `@user.reload.otp_required`? assert_redirected_to new_mfa_path + assert_empty `@user.otp_backup_codes` + # assert_nil `@user.otp_secret` # depending on desired semantics🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/controllers/mfa_controller_test.rb` around lines 54 - 62, The test "enables MFA only with correct password" should also assert the post-failure state of the user's secret and backup codes to document the wrong-password branch: after the POST and reload, assert that `@user.otp_secret` is nil (or cleared) and that `@user.otp_backup_codes` is nil or empty to confirm disable_mfa! ran and the setup state was wiped; update the test to call `@user.reload` and add assertions on otp_secret and otp_backup_codes alongside the existing otp_required? and redirect assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/locales/views/settings/en.yml`:
- Around line 141-142: Add the two new keys disable_mfa_password_label and
disable_mfa_password_placeholder under the existing securities.show section in
every locale file that already contains a securities translation (ca, de, es,
fr, nb, nl, pl, pt-BR, ro, tr, zh-CN, zh-TW) so those languages display the
localized password-confirmation label and placeholder instead of English; open
each locale's YAML, locate the securities.show mapping and add the two keys with
appropriate translations (or placeholders if translators will fill them later)
mirroring the structure used in config/locales/views/settings/en.yml.
---
Outside diff comments:
In `@app/controllers/api/v1/auth_controller.rb`:
- Around line 279-299: Check the user's existence and active? status before
creating a new token: replace User.find(access_token.resource_owner_id) with
User.find_by(id: access_token.resource_owner_id) and if user.nil? or
!user.active? then revoke all of that user's Doorkeeper::AccessToken records
(e.g. Doorkeeper::AccessToken.where(resource_owner_id:
access_token.resource_owner_id).find_each { |t| t.revoke }) and render json
error with status :unauthorized; only create new_token via
Doorkeeper::AccessToken.create!(...) after the active check passes, and remove
the later new_token.revoke since revocation of all tokens covers it.
In `@app/controllers/mfa_controller.rb`:
- Around line 10-57: The create and disable actions call
Current.user.authenticate(params[:password]) without checking whether the user
has a password_digest, which breaks SSO-only users; update both actions (create
and disable in mfa_controller.rb) to first check
Current.user.password_digest.present? and only call authenticate when true,
otherwise either skip the password re-auth branch (e.g. redirect to SSO re-auth
or surface an appropriate error) or block/show alternate UI; also update the
settings/securities view to gate the "Enable 2FA" link and the disable form
behind password_digest.present? (matching the SSO disconnect gating) so SSO-only
users don’t see a password field they can’t satisfy.
In `@app/views/settings/securities/show.html.erb`:
- Around line 22-43: Wrap the existing MFA UI in
app/views/settings/securities/show.html.erb with an additional check for whether
the current user has a local password (e.g.
Current.user.password_digest.present?): only render the enable/disable MFA
branch (the if Current.user.otp_required? block, the styled_form_with that posts
to disable_mfa_path, and the DS::Link to new_mfa_path) when
password_digest.present?; otherwise hide those controls (or render the existing
SSO warning block) so SSO-only users cannot see or submit the enable/disable MFA
forms.
---
Nitpick comments:
In `@app/controllers/mfa_controller.rb`:
- Around line 11-15: The password-failure branch in the MFA controller is
incorrectly calling Current.user.disable_mfa!, which clears otp_secret and
backup codes and forces a full re-setup; remove (or relocate) the call to
Current.user.disable_mfa! from the password check so that a bad password only
redirects to new_mfa_path with the alert (leave the in-progress setup intact),
and keep disable_mfa! only in the branches that actually represent code mismatch
or session expiry (the existing invalid_code/session-expiry handling) so only
real verification failures reset otp_secret/otp_backup_codes.
In `@app/views/settings/securities/show.html.erb`:
- Around line 24-36: The details element collapses after a failed disable_mfa
submission; update the <details> rendering in show.html.erb to include the open
attribute when a validation error/alert exists (e.g. use flash[:alert].present?
or another flash key) so the disclosure remains expanded after the redirect from
the disable action; locate the <details class="group"> element around the
styled_form_with (which posts to disable_mfa_path) and conditionally render open
(or alternatively implement a flash-driven anchor/scroll path using flash.now
and the controller render) to keep the form visible on errors.
In `@test/controllers/mfa_controller_test.rb`:
- Around line 54-62: The test "enables MFA only with correct password" should
also assert the post-failure state of the user's secret and backup codes to
document the wrong-password branch: after the POST and reload, assert that
`@user.otp_secret` is nil (or cleared) and that `@user.otp_backup_codes` is nil or
empty to confirm disable_mfa! ran and the setup state was wiped; update the test
to call `@user.reload` and add assertions on otp_secret and otp_backup_codes
alongside the existing otp_required? and redirect assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf609027-bea8-445c-bdd1-a12aca0ea64e
📒 Files selected for processing (12)
app/controllers/api/v1/auth_controller.rbapp/controllers/api/v1/base_controller.rbapp/controllers/mfa_controller.rbapp/models/api_key.rbapp/models/user.rbapp/views/mfa/new.html.erbapp/views/settings/securities/show.html.erbconfig/locales/views/mfa/en.ymlconfig/locales/views/settings/en.ymltest/controllers/api/v1/auth_controller_test.rbtest/controllers/mfa_controller_test.rbtest/models/user_test.rb
| disable_mfa_password_label: Confirm with your password | ||
| disable_mfa_password_placeholder: Enter your password |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find sibling locale files for the settings view and check which are missing the new keys.
fd -t f . config/locales/views/settings
for f in $(fd -t f . config/locales/views/settings); do
echo "=== $f ==="
rg -n 'disable_mfa_password_label|disable_mfa_password_placeholder' "$f" || echo " MISSING"
doneRepository: we-promise/sure
Length of output: 6401
🏁 Script executed:
#!/bin/bash
# Check the YAML structure around lines 141-142
echo "=== Structure of en.yml around lines 141-142 ==="
sed -n '130,150p' config/locales/views/settings/en.yml
echo -e "\n=== Looking for disable_mfa references in views ==="
fd -e erb -e html.erb . app/views | xargs rg -l 'disable.mfa|disable_mfa' 2>/dev/null || echo "No matches found"Repository: we-promise/sure
Length of output: 1195
🏁 Script executed:
#!/bin/bash
# Check if other securities keys are translated in non-English locales
echo "=== Checking fr.yml for other securities keys ==="
rg -A 5 'securities:' config/locales/views/settings/fr.yml | head -20
echo -e "\n=== Checking es.yml for other securities keys ==="
rg -A 5 'securities:' config/locales/views/settings/es.yml | head -20
echo -e "\n=== Git log to understand PR type ==="
git log --oneline --all -20 | grep -i 'mfa\|security\|disable' || echo "No recent commits with those keywords"Repository: we-promise/sure
Length of output: 972
Add parallel keys to all supported locale files for the securities section.
The two new keys disable_mfa_password_label and disable_mfa_password_placeholder under securities.show in config/locales/views/settings/en.yml must be added to the corresponding sections in all other locale files (ca, de, es, fr, nb, nl, pl, pt-BR, ro, tr, zh-CN, zh-TW) to maintain consistency. The securities section is already translated in these locales, so missing these keys will cause the password confirmation fields to display in English when viewed in other languages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/locales/views/settings/en.yml` around lines 141 - 142, Add the two new
keys disable_mfa_password_label and disable_mfa_password_placeholder under the
existing securities.show section in every locale file that already contains a
securities translation (ca, de, es, fr, nb, nl, pl, pt-BR, ro, tr, zh-CN, zh-TW)
so those languages display the localized password-confirmation label and
placeholder instead of English; open each locale's YAML, locate the
securities.show mapping and add the two keys with appropriate translations (or
placeholders if translators will fill them later) mirroring the structure used
in config/locales/views/settings/en.yml.
Major: - Token refresh ordering: check user.active? BEFORE issuing new_token (not after), and revoke ALL of the user's outstanding access tokens when inactive (not just the current pair). Also switch to User.find_by + 401 for missing users to match base_controller#authenticate_oauth, instead of User.find raising RecordNotFound → 404. - MFA + SSO-only users: Current.user.authenticate always fails when password_digest is nil, which previously let SSO-only users see an enable/disable UI they could never submit. Added password_reauth_ok? helper in mfa_controller that short-circuits when password_digest is blank; gated the MFA block in settings/securities/show.html.erb the same way (with a short explanatory line for affected users). Refinements: - Wrong password during MFA enable no longer calls disable_mfa!; a single typo would otherwise wipe otp_secret + backup codes and force a QR re-scan. Only real code mismatch / session expiry invalidates setup. - Disable-MFA <details> disclosure now renders with `open` when a flash alert is present, so the user sees the form (and the error) after a failed password attempt instead of a collapsed summary. Tests: - Existing enable-with-wrong-password test now asserts otp_secret survives. - New tests: SSO-only user cannot enable MFA, SSO-only user cannot disable MFA via password prompt. i18n: - Backfilled disable_mfa_password_label, disable_mfa_password_placeholder, mfa_sso_only in de/es/fr/nl/pl/pt-BR (the locales that already localize disable_mfa). Other locale files (ca, nb, ro, tr, zh-CN, zh-TW) don't yet localize the MFA block at all and are out of scope.
|
Thanks — addressed all seven points. Major — token refresh ordering + scope ( Major — MFA flow broken for SSO-only users ( Major — view needs same gating ( Nit — wrong password wipes MFA setup ( Nit — details collapses on error ( Nit — test should assert post-failure otp state ( i18n — new keys missing in non-English locales: Backfilled All changes in commit — wolfgang 🤖 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/mfa_controller.rb`:
- Around line 63-70: The controller allows SSO-only users to reach the MFA setup
flow by visiting new_mfa_path even though password_reauth_ok? prevents
submission; add a server-side guard in the new action (before calling
setup_mfa!) that checks password_reauth_ok? (or
Current.user.password_digest.present?) and redirects or renders an appropriate
403/redirect to settings if the check fails so SSO-only users cannot create a
setup state they cannot complete; update the new action to early-return when the
guard fails and ensure any helper methods (setup_mfa!) are not invoked for
guarded users.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae85a19a-4960-4c05-a7e4-ad637e7ae70e
📒 Files selected for processing (11)
app/controllers/api/v1/auth_controller.rbapp/controllers/mfa_controller.rbapp/views/settings/securities/show.html.erbconfig/locales/views/settings/de.ymlconfig/locales/views/settings/en.ymlconfig/locales/views/settings/es.ymlconfig/locales/views/settings/fr.ymlconfig/locales/views/settings/nl.ymlconfig/locales/views/settings/pl.ymlconfig/locales/views/settings/pt-BR.ymltest/controllers/mfa_controller_test.rb
✅ Files skipped from review due to trivial changes (6)
- config/locales/views/settings/pt-BR.yml
- config/locales/views/settings/de.yml
- config/locales/views/settings/nl.yml
- config/locales/views/settings/fr.yml
- config/locales/views/settings/en.yml
- config/locales/views/settings/pl.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- test/controllers/mfa_controller_test.rb
- app/controllers/api/v1/auth_controller.rb
CodeRabbit follow-up: password_reauth_ok? only gated the `create` and `disable` actions. GET /mfa/new was still reachable and called Current.user.setup_mfa!, which writes an otp_secret the SSO-only user could never complete (every submit path requires the password they don't have). Incomplete setup stayed in the DB. Added `block_sso_only_users` before_action on new/create/disable. SSO-only users now redirect to settings_security_path with a helpful message before any state mutation. Test: direct GET /mfa/new asserts no change to otp_secret.
|
Good catch — you were right that gating only
Added a Commit — wolfgang 🤖 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
Fixes several auth bypasses on the v1 API, adds constant-time comparisons for API keys and MFA backup codes, adds password re-auth to MFA enable/disable, and makes the v1 base controller honor OAuth token revocation.
Part of the security hardening originally bundled in #1104 (closed), now split by functional area per @jjmata's feedback. Companion PRs: #1516 #1517 #1518 #1520 #1521.
Findings addressed (8)
api/v1/auth_controller.rbSetting.registration_closed?api_key.rb—ActiveSupport::SecurityUtils.secure_compareuser.rb— constant-time verifyAuthConfig.local_login_enabled?(SSO-only mode) — check addedMigration notes
UI change: MFA enable and disable forms now include a password field. Any fork that customized these views needs to add the field.
Tests
test/controllers/api/v1/auth_controller_test.rb(deactivated users, signup closure, F-01, token refresh),test/controllers/mfa_controller_test.rb(password re-auth, disable flow),test/models/user_test.rb(backup code constant-time).bin/rubocopclean,bin/brakeman0 warnings.Out of scope
e.messageleakage → security: sanitize exception messages in v1 API responses (FIX-11) #1521Related
Summary by CodeRabbit
New Features
Bug Fixes
Documentation