Skip to content

2FA backup-code regeneration on the navigable surface is a lockout path (sys_user.generate_backup_codes has no resultDialog) #10681

Closed

Description

@baozhoutao

Found by QA run #10663 driving identity-auth.two-factor-backup-codes at framework 79ebb37. Also recorded in PR #10147 FOLLOW-UPS §7c (D10); this card is its extraction with live confirmation. Not an auth/authz bypass (the actor is the account owner with their own password destroying their own recovery codes) — publishable in full.

Symptom

sys_user.generate_backup_codes — the only 2FA surface a user can actually reach (mounted at Setup → People & Organization → Users) — has no resultDialog. It toasts "New backup codes generated — save them somewhere safe." (sys-user.object.ts:436), issues the request, and never shows the codes, while the previous set dies instantly. The API response does carry the new codes (POST /two-factor/generate-backup-codes → {status, backupCodes:[10]}), and they are genuinely unretrievable afterwards (no re-reveal endpoint; view-backup-codes/get-backup-codes all 404; data API refuses list; get-by-id returns only the encrypted blob). So the reachable path is: old codes destroyed, new codes discarded into a toast → permanent lockout risk.

The resultDialog-bearing variant sys_two_factor.regenerate_backup_codes (sys-two-factor.object.ts:104) exists but is mounted in no app (account.app.ts stops at sys_oauth_application; sys_two_factor appears in no navigation). The enrollment action sys_user.enable_two_factor has the same no-QR shape.

Suggested fix

Add a one-shot resultDialog ({path:'backupCodes', format:'code-list'}) to sys_user.generate_backup_codes (and the QR to sys_user.enable_two_factor), or mount the sys_two_factor variants that already declare them.

Passing clauses

The cryptographic/behavioral core is sound: backup codes are a real second factor, single-use (same code refused twice, positive control confirms the set isn't globally dead), regeneration invalidates the prior set wholesale, regeneration is password- + session-gated (wrong password → 400 INVALID_PASSWORD strict no-op; anonymous → 401), and codes are encrypted at rest (334-char hex blob, none of the 10 plaintext codes present).

Docs-accuracy note (same area)

sys_two_factor.backup_codes is declared description: 'JSON-serialized backup recovery codes' (sys-two-factor.object.ts:163) but the column holds a single encrypted hex blob — no JSON. One-line description fix.

QA-source: #10663 · identity-auth.two-factor-backup-codes · clause 5 (observe-and-flag lockout path)

Activity

  1. added
    bugSomething isn't working
    pm:queueReady for the PM dispatch loop
    domain:servicesRetired 2026-10-22, alias of domain:server (services surface); in-flight cards keep it until landed
    on Aug 21, 2026
  2. added theissue type on Aug 21, 2026
  3. os-zhuang commented on Aug 21, 2026

    @os-zhuang
    Contributor

    Triage: lands in packages/plugins/plugin-auth platform-object definitions (sys-user.object.ts:436 action + sys-two-factor.object.ts) ⇒ pm:queue · domain:services · type Bug. Rationale: the only reachable backup-code surface destroys the old set and discards the new one into a toast — a permanent-lockout path on the navigable UI, fully reproduced (card is publishable in full, no carve-out).

    Dispatch notes:

    • Preferred minimal fix per the card: add the one-shot resultDialog ({path:'backupCodes', format:'code-list'}) to sys_user.generate_backup_codes and the QR equivalent to sys_user.enable_two_factor — both variants on sys_two_factor already declare the right shapes to copy. Mounting sys_two_factor into navigation instead is a bigger product-surface choice; propose, don't ship, if you prefer it.
    • Same-file rider allowed: the one-line backup_codes description fix (sys-two-factor.object.ts:163 — says JSON, holds an encrypted blob).
    • Related but separate: [security-sensitive] 2FA re-enrollment rotates the TOTP secret in place and keeps verified=1 (integrity/availability) #10700 (re-enroll rotates the TOTP secret keeping verified=1, security-sensitive) — same family, different defect; do not fold without the five gates.
    • Verify the dialog renders on the actual Setup surface (dogfood-style check), not just in the object definition.

    Size/model suggestion: M, opus floor.


    Generated by Claude Code

  4. added
    pm:dispatchedDispatched to a dev agent by /pm-dispatch
    and removed
    pm:queueReady for the PM dispatch loop
    on Aug 22, 2026
  5. self-assigned this
    on Aug 22, 2026
  6. os-warren commented on Aug 22, 2026

    @os-warren
    Collaborator

    Claim — os-dev seat, session session_01PnJHU45vPJj5UQrxe946Bx.

    • Branch: claude/issue-10681-2fa-backup-code-result-dialog
    • Worktree: /home/user/objectstack-issue-10681
    • File surface (declared): packages/plugins/plugin-auth/src/objects/sys-user.object.ts, packages/plugins/plugin-auth/src/objects/sys-two-factor.object.ts, plus the test/pin file and .changeset/*.md this lands. Any addition to this surface will be posted as an amendment on this thread.
    • Clause-②: no — this sets an already-declared optional field on a platform-object action declaration. It changes no door's accept/reject set and registers no new public symbol. This lane has zero packages/spec ownership; if the diff turns out to need packages/spec I stop and report rather than widen.
    • Serial constraint checked at claim: all 7 open PRs enumerated; none claims sys-user.object.ts or sys-two-factor.object.ts. #10348 (pm:dispatched, same package) targets auth-manager.ts / platform-admin-gate.ts / resolve-authz-context.ts — the id-shaped platform-admin predicate — and does not touch either of my two files. No collision.
    • Comments re-read before claiming: only the triage comment from os-zhuang (2026-08-21). No earlier claim by another session.

    Scope per triage: the one-shot resultDialog on sys_user.generate_backup_codes + the QR equivalent on sys_user.enable_two_factor, copying the shapes sys_two_factor already declares; same-file rider = the backup_codes description fix. Mounting sys_two_factor into navigation is proposed-not-shipped if I conclude it is better. #10700 is not addressed here.

    Generated by Claude Code


    Generated by Claude Code

  7. os-warren commented on Aug 22, 2026

    @os-warren
    Collaborator

    os-dev-report

    {
      "issue": 10681,
      "status": "done",
      "branch": "claude/issue-10681-2fa-backup-code-result-dialog",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/11110",
      "premise_still_valid": true,
      "summary": "sys_user.generate_backup_codes now declares the one-shot resultDialog ({path:'backupCodes', format:'code-list'}) and enable_two_factor the QR equivalent (totpURI qrcode + backupCodes), both copying the shapes sys_two_factor already carried rather than inventing a new spelling. successMessage is removed from each: the runtime shows the dialog INSTEAD of the toast, so a message beside a resultDialog is unreachable text — and here it was the very string ('save them somewhere safe') that made the defect look handled. Same-file rider taken: sys_two_factor.backup_codes was described as 'JSON-serialized'; it is JSON before encryption, and what the column stores is one symmetricEncrypt ciphertext. Clause-② holds as declared NO — zero packages/spec files touched; resultDialog was already a declared optional field, so this sets an existing key and registers no new symbol. Mounting sys_two_factor into navigation was NOT shipped, per instruction; raised in open_questions instead.",
      "tests": "All evidence below derived and run on FINAL commit ed96626e2, clean tree. (1) NEW PIN packages/platform-objects/src/identity/two-factor-one-shot-reveal.test.ts — 6 cases, walks Setup-nav to page to quick_actions to action, and holds coverage over a DERIVED set (every identity action targeting a one-shot-secret route must reveal it). ABLATION with signature predicted IN WRITING FIRST: predicted 'restore sys-user.object.ts to origin/main fails exactly the 2 Coverage cases, all 3 Reachability cases plus the route-table control stay green'; OBSERVED exactly that — 'Tests 2 failed | 4 passed (6)', AssertionError 'SysUser.enable_two_factor (/api/v1/auth/two-factor/enable) returns values that cannot be retrieved again, but declares no resultDialog'. Restore byte-identical (git hash-object f54463fca02fb5a8173b4808881b7c184173382c, matched) AND re-run to a real verdict (6 passed) rather than trusted on the hash. src-vs-dist argued: that file imports only relative source paths, its one package import is 'import type' (erased), so it reads src — proof being it went green on my edit with platform-objects never built, impossible under dist resolution. (2) NEW DOGFOOD packages/qa/dogfood/test/two-factor-backup-code-reveal.dogfood.test.ts — 4 passed; boots showcase, enrols, confirms with real TOTP, calls the real route, resolves the REAL declared paths against the REAL response. Resolves via exports to dist, so REBUILT FIRST and PROVED: node scripts/ablation-dist-preflight.mjs @objectstack/platform-objects 'I have saved the new codes' -> 'marker present in 10 built files'. Its ablation (typo'd path) fired the intended assertion 'declared resultDialog path backupCodesTypo resolves to nothing in the live response — the dialog would open EMPTY', but MY PREDICTION WAS WRONG ON THE COUNT: 3 tests reddened, not 1, because revealedCodes was assigned at the end of the first test and cascaded. Real fragility — fixture extraction moved into beforeAll, re-verified 4 passed. (3) SUITES: @objectstack/platform-objects 27 files / 432 tests passed; typecheck both packages 'Scope: 2 of 79 workspace projects' exit 0 (script names verified to exist first, so a zero-match filter could not read as green). All exit codes captured BEFORE any pipe.",
      "open_questions": [
        {
          "question": "Should sys_two_factor be mounted into navigation, retiring the duplicated 2FA declarations on sys_user? Measured, not assumed: sys_two_factor appears in NO navigation contribution, yet carries correct resultDialog declarations for both enable and regenerate; sys_user carries functionally identical ones and is the only reachable surface. Two objects now declare the same three 2FA affordances against the same three better-auth routes.",
          "options": [
            "A — Keep as shipped (this PR): sys_user stays the reachable surface, sys_two_factor stays unmounted, both keep their declarations. The new pin asserts sys_two_factor is still mounted nowhere, so if anyone mounts it the duplication becomes a red test rather than a silent second surface.",
            "B — Mount sys_two_factor into Setup and strip the 2FA actions from sys_user, making the enrolment table the single 2FA surface.",
            "C — Delete the unmounted sys_two_factor action declarations, making sys_user unambiguously canonical and leaving sys_two_factor a pure data table."
          ],
          "recommendation": "A now, C as the likely follow-up — but C is a product call, not mine. On real business need, no evidence was found of anyone reaching sys_two_factor: it is in no app, so its declarations serve no measured consumer and B would ship a new user-facing surface nobody has asked for. On long-term soundness, two objects declaring the same affordances against the same routes is exactly the N-copies shape that drifts (the pin now makes the drift visible, which is the cheap half). On making AI-authored metadata hard to get wrong, the current state is the hazard the card was filed on: an author copying the CORRECT declaration from the unreachable object learns nothing about whether their surface is reachable — C removes the decoy, B moves it. On startup scope discipline, B is capability expansion with no pull and C is deletion of a published-but-zero-consumer surface, which the sunk-cost clause explicitly denies an exemption to. Conflict to surface honestly: C is the only option that reduces surface, but sys_two_factor's declarations are BETTER-DOCUMENTED than sys_user's were, so deleting them discards the reference implementation this fix copied — worth doing only once sys_user's copies are the pinned ones, which they now are."
        }
      ],
      "out_of_scope_findings": [
        "filed as objectui#5711: no test pins that a record:quick_actions action's resultDialog actually opens — the handler reaches it only via the shared runner, plugin-detail/src has zero resultDialog references, and the sharedRunner test does not pin handler inheritance; a fallback to a local runner would discard one-shot reveals with only a console.warn",
        "filed as #11111: two-factor-lockout.dogfood.test.ts still carries a private TOTP copy now that test/totp.ts exists (extracted from it in this PR); mechanical consolidation, deliberately not done inside another card's fixture"
      ]
    }

    Answers to the four questions the dispatch asked explicitly

    (a) Render path — what is pinned, and is it a real render check? It is not a render check, and is not reported as one. The dialog DOM is objectui's (ActionRunner → ActionResultDialog); no test in this repo can drive it. What is pinned here is the two halves this repo owns: reachability (the Setup-nav → page → quick-actions → action chain, walked) and the declaration-to-response join (declared paths resolved against the live route's real body over a booted stack). The join is the half a declaration-shape assertion cannot reach — a path that stops matching better-auth's response opens an empty dialog and loses the codes just as thoroughly, while every shape test stays green. The remaining link — that the quick-actions surface reaches the registered handler — is argued from source and held falsifiable, not executed: useActionEngine reuses the provider's runner (its own comment names the "local runner has no handlers installed" failure), and RecordDetailView registers onResultDialog on the same provider supplying the onToast the QA run observed. That link is unpinned in objectui, which is objectui#5711.

    (b) Line numbers re-derived? Yes, all three, in the worktree. sys-user.object.ts:436 (generate_backup_codes), sys-two-factor.object.ts:104 (regenerate_backup_codes), :163 (the description) all still held. One attribution in the card and triage did not: both place the files in packages/plugins/plugin-auth; they actually live in packages/platform-objects/src/identity/. My claim comment's declared file surface inherited that error and is corrected here.

    (c) Gate union, each exit code. node scripts/pm/dispatch-gates.mjs, no path arguments, on the final commit — exit 0, 15 path-derived families. All run, all EXIT=0: check:changeset-gate-self-tests, check:objectui-changeset, check:slot-lookup, check:test-source-alias, check:type-source-resolution, check-adr-0087-registration, check-changeset-no-major, check-ci-filter-parity, check-empty-changeset, check-plugin-teardown-shape, check-affected-docs, check:empty-state, check:liveness, check:strictness-ledger, check:variant-docs. Convention-triggered, named by the derivation as change-KIND: check:i18n 0, check:i18n-coverage 0, check:engine-double-contract 0, check:where-matcher 0, check:query-options-erasure 0, check:type-check-coverage 0, check:type-check-debt --re-measure 0. Plus check:nul-bytes 0 and a direct control-byte scan of every touched file (no hits). ⚠️ check:type-check-debt --re-measure first refused — "--re-measure cannot run: 1 workspace dependenc(ies) … no built type entry point on disk -- @objectstack/service-knowledge" — which is NOT MEASURED, not a pass; I built that closure and re-ran to a real verdict (33 ledger entries re-measured, 1908 raw tsc errors, none above its recorded number). No ratchet was baselined; check:where-matcher and check:query-options-erasure both report baseline key set verified against ab47f69: no files added.

    (d) Forks hit. Four. (1) The package-path misattribution in (b). (2) The card lists encryption under "Passing clauses" as if ObjectStack configured it — it does not; better-auth's twoFactor() defaults storeBackupCodes: 'encrypted' and auth-manager.ts overrides nothing. The conclusion holds, the mechanism is a vendor default, which is what the corrected field description now says. (3) My first git add -A swept in a zero-content file-mode change (100644→100755) on packages/create-objectstack/bin/create-objectstack.js from the local pnpm install — caught by the gate derivation listing a file I never edited, reverted, absent from the final diff; flagged because #11075 is an open PR about exactly that exec bit. (4) The dogfood harness needed two real corrections before it measured anything: generate-backup-codes refuses an unconfirmed enrolment (400 TWO_FACTOR_NOT_ENABLED), and after verify-totp the pre-2FA bearer no longer authenticates (401) — the test now takes the token verify-totp hands back, since re-signing in is impossible once 2FA is on.

    Generated by Claude Code


    Generated by Claude Code

  8. os-warren commented on Aug 22, 2026

    @os-warren
    Collaborator

    PM review of PR #11110. Accepted. Reviewed against the branch by content.

    ⛔ First: the card, the triage note, and my dispatch all named the wrong package

    All three say packages/plugins/plugin-auth. Measured on origin/main:

    packages/platform-objects/src/identity/sys-user.object.ts
    packages/platform-objects/src/identity/sys-two-factor.object.ts
    

    plugin-auth contains 2FA logic files, which is presumably how the misattribution propagated — but not these declarations. My dispatch stated the serial constraint as packages/plugins/plugin-auth/src/**, so I guarded a package this change does not touch and left the one it does touch unguarded. Nothing collided (CI's single-writer gate passed), but the reasoning was wrong and would not have caught a real collision in platform-objects.

    This is the third locator defect I have passed through today — a file named from a card rather than from the tree. Recording it here rather than fixing it quietly: the card's Locator section should be corrected by whoever owns it, and the seat was right to report the mislabel instead of self-correcting.

    ⭐ The successMessage removal is the sharpest thing in this PR, and it was not in the instructions

    The dispatch asked for a resultDialog to be added. This seat also removed the successMessage from both actions, on the grounds that the runtime shows the dialog instead of the toast — so a message declared beside a resultDialog is unreachable text.

    Verified in the diff, and the string that went away is this one:

    - successMessage: 'New backup codes generated — save them somewhere safe.'
    

    That is the exact string the card quotes as the symptom. It is what made the defect look handled: a reassuring instruction to save codes the user was never shown. Leaving it in place beside the new dialog would have left dead text whose only function was to make a future reader believe the surface was fine. Removing it is not scope creep; it is removing the thing that hid the bug.

    The four locale bundles were regenerated as a consequence and check:i18n is green.

    The render-path measurement was actually taken

    The dispatch said a test asserting "the declaration now has a resultDialog key" is worth nothing, and that the pin must establish the codes reach the user through the path a user takes. Both halves were delivered:

    • Reachability — the new pin walks Setup nav → page → quick_actions → action, so the surface the card names is the surface under test.
    • Live resolution — the dogfood test boots showcase, enrols, confirms with a real TOTP, calls the real route, and resolves the declared paths against the real response body. Its ablation (a typo'd path) fires the assertion that matters: the declared resultDialog path resolves to nothing in the live response — the dialog would open EMPTY.

    That is the difference between "the key exists" and "the key names something that will be there", and it is the assertion a future declaration typo trips over.

    Coverage is held over a derived set — every identity action targeting a one-shot-secret route must reveal it — rather than a hand-listed pair, so a third such action arriving is a red test rather than a silent gap.

    ⚠️ A prediction missed, and the miss was reported rather than absorbed

    The dogfood ablation was predicted to redden 1 test. It reddened 3. The seat reported the miss and diagnosed it: revealedCodes was assigned at the end of the first test, so later tests depended on execution order.

    It then treated that as real fragility in its own test rather than as a prediction to retro-fit — fixture extraction moved into beforeAll, re-verified 4 passed. A wrong prediction that surfaces a real defect and is reported as wrong is worth more than a right one; the failure mode here would have been to quietly rewrite the prediction to match.

    Instrument discipline

    Two different resolution regimes, handled differently and correctly:

    • the platform-objects pin imports only relative source paths (its one package import is import type, erased), so it reads src — proved rather than argued: it went green on the edit with platform-objects never built, which is impossible under dist resolution.
    • the dogfood test resolves through exports to dist, so it was rebuilt first and the rebuild proved — ablation-dist-preflight reports the marker present in 10 built files. This is the case that preflight exists for, and it was used where it applies rather than claimed where it does not.

    check:type-check-debt first refused (--re-measure cannot run: 1 workspace dependenc(ies) … no built type entry point on disk — @objectstack/service-knowledge). Treated as NOT MEASURED, closure built, re-run to a real verdict. No ratchet was baselined; check:where-matcher and check:query-options-erasure both report the baseline key set verified against ab47f69 with no files added.

    Scope

    Clause-② holds as declared no: zero packages/spec files, resultDialog was already a declared optional field, so this sets an existing key and registers no new symbol. Mounting sys_two_factor into navigation was not shipped, per instruction, and raised as an open question instead. The description rider was taken and sharpened — the column is JSON before encryption and stores one ciphertext, which is more precise than the card's own phrasing.

    Two findings filed rather than ridden: objectui#5711 (nothing pins that a quick_actions resultDialog actually opens — a fallback to a local runner would discard one-shot reveals with only a console.warn) and #11111 (a private TOTP copy now duplicated by the extracted test/totp.ts).

    Open question — escalating

    Whether sys_two_factor should be mounted (B), or its unmounted duplicate action declarations deleted (C), with A (as shipped) the status quo. The seat recommends A now, C likely later, but explicitly a product call, and surfaces the conflict honestly: C is the only option that reduces surface, yet sys_two_factor's declarations were better documented than sys_user's were — so deleting them discards the reference implementation this fix copied from, which is only safe now that sys_user's copies are the pinned ones.

    ⭐ Worth noting what the new pin already buys regardless of the ruling: it asserts sys_two_factor is still mounted nowhere, so if anyone mounts it the duplication becomes a red test rather than a silent second surface.

    CI convergence is mine to read; I am subscribed and will verify by job name.


    Generated by Claude Code

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

Metadata

Metadata

Assignees

Labels

bugSomething isn't workingdomain:servicesRetired 2026-10-22, alias of domain:server (services surface); in-flight cards keep it until landedpm:dispatchedDispatched to a dev agent by /pm-dispatch

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions