Skip to content

test(platform): isolate the 2FA lifecycle check on its own user - #3196

Closed
Israeltheminer wants to merge 1 commit into
mainfrom
test/2fa-lifecycle-audit-harness
Closed

test(platform): isolate the 2FA lifecycle check on its own user#3196
Israeltheminer wants to merge 1 commit into
mainfrom
test/2fa-lifecycle-audit-harness

Conversation

@Israeltheminer

Copy link
Copy Markdown
Collaborator

The 2FA lifecycle audit check in integration-check.ts runs on the harness's
shared session. The block enables 2FA, regenerates backup codes, disables it,
and registers and removes a passkey — so it leaves that user's second-factor
state changed and their session invalidated.

Five later checks fail that way: the 2FA grace check, and four chat checks
that need a live session.

The fix

A dedicated throwaway user for the block. Audit rows are org-scoped, so the
user also needs a member row — without one their events have no org to land
under and the trail reads back empty, which would make the check pass for the
wrong reason.

The check already worked around the shared state with a note: "Leave 2FA
OFF: later lanes sign this account in with password alone."
With its own user
there is nothing to leave in any particular state, so the note goes.

+45 / −7, one file, no source changes.

Why this is a separate PR

This is what remained of #3181 after #3187 landed. #3181 also carried the five
audit-event calls in auth.ts and two_factor/service.ts, and those are now
byte-identical to main — I shipped that half twice by folding #3149's 2FA
third into #3187 without checking my own open PRs first. #3187 carried the
lifecycle check too, but the shared-session version; this is the isolation fix
that never made it across.

Gate

typecheck 0 errors, oxlint --type-aware clean, oxfmt --check clean.

I could not run the harness itself: the suite aborts early on the
knowledge/embedding step, which needs KNOWLEDGE_DATABASE_URL and an S3
endpoint this machine has no MinIO for. So the five failures this fixes are
from the earlier run that found them, not a fresh one.

The 2FA lifecycle audit check runs on the harness's shared session. The
block enables 2FA, regenerates backup codes, disables it, and registers
and removes a passkey — so it leaves that user's second-factor state
changed and their session invalidated.

Five later checks fail that way: the 2FA grace check, and four chat checks
that need a live session.

Give it a dedicated throwaway user instead. Audit rows are org-scoped, so
the user also needs a `member` row or their events have no org to land
under and the trail reads back empty.

The check already worked around the shared state with a "Leave 2FA OFF:
later lanes sign this account in with password alone" note. With its own
user there is nothing to leave in any particular state, so the note goes.
@Israeltheminer

Copy link
Copy Markdown
Collaborator Author

Closing — main has since solved this more generally, and better than this PR did.

The harness runner now asserts sharedSessionAlive after every lane, and its failure message names exactly this fix: "a probe that invalidates its own session must act as a throwaway user (signUpOrgMember)". That helper exists, so the next probe with this shape is caught by a guard rather than by someone noticing five unrelated failures.

The checkTwoFactor lane itself is also no longer leaving the shared session broken: authPost follows the session rotation with mergeCookieHeader the way a browser would, the lane ends with /two-factor/disable so 2FA is left off, and the passkey trio writes audit rows through recordTwoFactorLifecycleEvent rather than registering a real passkey.

So my earlier note that main carries the buggy version is out of date. Nothing here is worth forcing in against that design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant