Make MFA token redemption atomic to prevent multiple sessions

[dantecatalfamo]

Related issue: Resolves https://github.com/fleetdm/confidential/issues/16770 (Aikido-PT-30)

Summary

The MFA login token redemption path (POST /api/latest/fleet/sessions) read the one-time verification token with a non-locking SELECT on the read replica, then created a session and deleted the token in a separate transaction without verifying the token was still present. Concurrent requests carrying the same token each passed the SELECT and each minted a distinct session, breaking the single-use guarantee.

SessionByMFAToken now consumes the token and creates the session inside a single transaction:

• The token row is locked with SELECT ... FOR UPDATE, then deleted, and the delete's rows-affected count is confirmed non-zero before the session is created.
• Concurrent redemptions serialize on the row lock; the loser re-reads after the winner commits the delete, finds no row, and aborts before creating a session.
• The user is still loaded before the transaction, so a concurrently-deleted user or a transient read error leaves the token intact for retry (preserving the pre-fix atomicity behavior).

Checklist for submitter

• Changes file added for user-visible changes in changes/.

• Input data is properly validated, SELECT * is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.

Testing

• Added/updated automated tests

server/datastore/mysql/sessions_test.go gains a concurrency case in testMFA that fires 8 concurrent redemptions of one token and asserts exactly one session is created and the token is consumed. Verified this test fails (8 sessions created) against the pre-fix code and passes with the fix.

• QA'd all new/changed functionality manually

Manual test plan for reviewer

COMPOSE_PROJECT_NAME=fleet MYSQL_TEST=1 go test -run 'TestSessions/MFA' ./server/datastore/mysql/

To confirm it exercises the race, temporarily revert SessionByMFAToken to its previous body (create session, then delete token, no FOR UPDATE/rows-affected check) and re-run — the concurrency case fails with "exactly one concurrent redemption should succeed: expected 1, actual 8".

Summary by CodeRabbit

• Bug Fixes
  • Fixed a race condition during MFA sign-in that could allow a one-time token to create multiple sessions.
  • MFA tokens are now redeemed only once, even when used concurrently.
  • Attempts to reuse a consumed token now fail as expected.

[codecov]

Codecov Report

❌ Patch coverage is 63.63636% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.97%. Comparing base (0e78132) to head (2d84ef0).
⚠️ Report is 55 commits behind head on main.

Files with missing lines	Patch %	Lines
server/datastore/mysql/sessions.go	63.63%	4 Missing and 4 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #49359   +/-   ##
=======================================
  Coverage   67.97%   67.97%           
=======================================
  Files        3823     3823           
  Lines      241031   241120   +89     
  Branches    12702    12702           
=======================================
+ Hits       163831   163904   +73     
- Misses      62325    62338   +13     
- Partials    14875    14878    +3     

Flag	Coverage Δ
backend	69.54% <63.63%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Warning

• Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.

Pull request overview

This PR fixes a race in MFA one-time token redemption by making token consumption and session creation atomic, preventing multiple concurrent POST /api/latest/fleet/sessions requests from minting multiple sessions with the same token.

Changes:

• Update SessionByMFAToken to SELECT ... FOR UPDATE the verification token row inside a transaction, delete it, verify rows affected, then create the session.
• Add a concurrency regression test that attempts multiple concurrent redemptions of the same MFA token and asserts only one session is minted.
• Add a user-visible changelog entry (content excluded by policy).

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

File	Description
server/datastore/mysql/sessions.go	Make MFA token redemption atomic with row locking + delete rows-affected verification before session creation.
server/datastore/mysql/sessions_test.go	Add concurrency test ensuring only one redemption succeeds and the token is consumed.
changes/16770-mfa-token-race	User-visible changelog entry (content excluded; not reviewed).

Files excluded by content exclusion policy (1)

• changes/16770-mfa-token-race

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9346234d-38b8-480b-a2a1-a86ff66b0fe3

📥 Commits

Reviewing files that changed from the base of the PR and between 0e78132 and 2d84ef0.

📒 Files selected for processing (3)

• changes/16770-mfa-token-race
• server/datastore/mysql/sessions.go
• server/datastore/mysql/sessions_test.go

---

Walkthrough

MFA token redemption now locks and rechecks the verification token within the transaction, deletes it before session creation, verifies the delete affected a row, and creates the session using the locked token’s user ID. Tests concurrently redeem the same token and assert that exactly one request succeeds, one session is created, and subsequent reuse fails. The change note documents the atomic redemption behavior.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and accurately summarizes the main change: atomic MFA token redemption to prevent duplicate sessions.
Description check	✅ Passed	The description follows the template well, with a related issue, summary, checklist, automated tests, and manual verification plan.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch worktree-16770-mfa-token-race

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}