fix(auth): consume magic links atomically before issuing sessions

RM
3h ago
+610-108

Reviewed CI corrections — November 7

Current head: e7c4c3cec432854044ccdf4f110c3436b98d9f3e. Five advisory failure paths now have reviewed corrections: resolver setup supplies trusted bootstrap context; accounting tests load the existing mock plugin with autoload disabled; import inventory references are regenerated; logging test reload drains queued writes; PostgreSQL return completes rollback and session cleanup before returning ownership, preventing a reproduced shutdown leak. Offline guards and existing assertions are unchanged.

73 resolver tests including PostgreSQL 18; 63 accounting tests in each autoload mode; 10 inventory tests; 22 logging tests; 229 smoke tests before a regression-only synchronization refinement, then all 22 logging tests again; and 29 focused pool/reset/live tests passed. Independent reviewers ran another 23 PostgreSQL and 22 logging checks. All 10 changed Python files pass mypy 2.4.1 and Ruff. No new runtime Bandit findings; test assertions and an inherited synthetic-password fixture are documented scanner findings. The historical CI interleaving was not captured; the separate AuthNZ teardown/GC issue is not claimed fixed here. No restore or recovery operation was run.

Fresh required and advisory CI on this head is awaiting verification. Earlier CI reports below describe superseded heads. The requester's Change summary waiver remains in force. No merge or auto-merge has been requested.

MCP default-selection test synchronization — November 7

The superseded 7de63714 shared-UI coverage shard 1/16 passed 1,927 tests and failed the McpToolsStep missing-catalog case: Research appeared before its default checked state was applied. Catalog resolution and default-selection initialization occur in separate React updates.

The test now waits for the existing Research-checked assertion, preserving the load-once and Writing-unchecked assertions. A temporary probe using the actual component and removing only Testing Library's incidental async-wrapper timer drain reproduced the exact unchecked assertion before the change and passed afterward. All 11 tests in the original file, standard full WebUI TypeScript (8192 MiB, Node 20), scoped shared-UI ESLint, canonical task formatting and diff checks pass. Independent review found no actionable issue. Product code, dependencies, assertions, timeouts, retries and workflow gates are unchanged. ADR required: no; this synchronizes the existing initialization contract.

The complete hosted coverage shard was not rerun locally. Fresh required and advisory CI must validate this new head; previous successes do not transfer. Diagnostic wiring and temporary dependency links have been removed. Bandit is not applicable to this TypeScript-test-only correction; earlier Python security evidence remains.

Candidate-review test synchronization — November 7

The superseded 94c8bb7b shared-UI coverage shard 9/16 passed 1,504 tests, skipped one and failed the VisualPackEditor generation/review case: it observed one review request where two were expected. The test clicked Reject immediately after the accepted status appeared, while AntD's separate loading state could still suppress the click.

A controlled 50 ms delay of the test's review response reproduced the exact missing-second-request failure. The test now waits for the actual Reject button's loading class to clear before clicking once. The same controlled reproduction passes afterward, and all 66 tests in the original file pass. Standard full WebUI TypeScript (8192 MiB), scoped shared-UI lint using the repository config, task formatting and diff checks pass. Independent review found no actionable issue and verified the real AntD click guard. All existing accepted/rejected messages and request-count assertions remain; product behavior, dependencies, retries and timeouts are unchanged. ADR required: no; this restores the existing interaction test contract.

The complete hosted coverage shard was not rerun locally. Fresh required and advisory checks must validate this new head; earlier successes do not transfer. Temporary reproduction and dependency links were removed, preserving unrelated work.

Summary

Claim a verified magic link before changing the account or issuing a session. The existing blacklist's unique token ID decides which request wins, including when two requests have already passed JWT verification. The insert runs on the sign-in connection in statement-autocommit mode. No database migration is required.

A duplicate returns HTTP 400. A storage failure returns HTTP 503 without credentials. If the claim has committed and a later sign-in step fails, the user needs a fresh link.

Validation

  • Before the fix, two simultaneous SQLite HTTP submissions both returned 200. A rejected blacklist insert also returned 200. The new tests reproduce both failures.
  • 58 SQLite and adjacent AuthNZ tests passed, including rejection on missing redemption metadata and a consumed link remaining spent after a later failure.
  • All 7 PostgreSQL magic-link integration cases passed on PostgreSQL 18, including two concurrent first-time sign-ins with the same link and the existing verification, inactive-account, and rollback cases.
  • Ruff, Python compilation and whitespace checks passed. Bandit found no new issues in the changed runtime modules compared with the base commit.
  • ADR assessment: this enforces the existing one-time sign-in contract through the existing table and request connection.

CI follow-up

CI follow-up fixes the exact-type expiry narrowing and SQLite result typing, and regenerates the OpenAPI fingerprint for the documented redemption behavior. The changed-file gate also exposed inherited typing errors in the same auth module, which are corrected through existing exception tuples, validated fields, response defaults and FastAPI response injection. No typing suppressions were added. Exact CI mypy passes all four files, 91 focused auth tests and the legacy /auth/me HTTP test pass, and all 10 PostgreSQL magic-link/email-verification integration tests pass. Ruff and OpenAPI checks pass, and Bandit reports no new findings. Independent review found no actionable issue. At 15:17 UTC on November 7, all eight protected checks for this head report SUCCESS; advisory full-suite jobs are still running.

Advisory CI limitation (November 7)

The current-head core-server-smoke job reports 227 passed and one failure: test_log_file_lock_uses_runtime_timeout_from_env expected a nested fallback lock to time out, but it did not raise. The complete Logging source and test trees, shared test configuration and CI workflow are unchanged from the run's exact base 2f402bd7.

This failure is not proven inherited: the earlier baseline job, whose revision has the identical Git tree as that base, passed all 228 tests. One local run of the full system-log test file passes all 21 tests on both the exact archived base and this PR head. These macOS runs do not reproduce the full Ubuntu shard's ordering or environment, so the CI-only cause remains unclassified. No magic-link regression is demonstrated, and no unrelated logging changes, assertion changes or CI retry were made. This full-suite shard is advisory; required checks remain subject to their own results.

Advisory database fixture limitation (November 7)

The current-head admin-backup-core job reports 120 passed and one failure: test_official_restricted_fixture_retires_provisioner_before_offline_handoff. After the fixture closed its pool, the read-only offline-session check still detected another database client and correctly refused to proceed. The log does not identify that client's origin.

The complete Admin test, DB_Management and core AuthNZ trees, official PostgreSQL fixture, shared conftest, backup CLI/service, pyproject and CI workflow are unchanged from the run's exact base 2f402bd7. The baseline job at ddd87310 (identical entire Git tree to that base) passed all 121 tests, including this case. Therefore this failure is not proven inherited; its cause remains unclassified, and no magic-link regression is demonstrated. Investigation was limited to existing CI logs and source comparison. No recovery operation, local test replay, CI retry, guard change or unrelated fixture repair was performed. This is an advisory shard; all eight protected checks remain successful as of the timestamp above.

Inherited advisory auth failures (November 7)

The current-head auth-integration-b-z job reports 535 passed and six failures. All six are the org, team and user variants on SQLite and PostgreSQL of test_vn_outer_transaction_resolves_cold_and_warm_storage_limits in test_vn_generated_file_idempotency.py.

These six failures are inherited: the baseline job at ddd87310 reports exactly the same six failing cases and assertions, with 534 passed. That revision and this run's exact base 2f402bd7 have the identical entire Git tree 688e7bf3. The failing test and subprocess helper, AuthNZ fixtures, complete core AuthNZ/DB_Management/Storage/Usage/UserProfiles trees, storage quota service, shared configuration and CI workflow are unchanged between base and this PR head. The test's resolver setup omits the mandatory context argument to AuthnzOrgsTeamsRepo.add_org_member, before it reaches the storage-limit behavior.

No magic-link regression is demonstrated by this shard. This classification uses existing CI evidence and source comparison; no deterministic baseline tests were rerun or unrelated fixture changes made. It does not resolve the separate logging and database-fixture failures described above, whose causes remain unclassified.

Inherited advisory MCP setup errors (November 7)

The current-head platform-mcp-inapp shard reports 4,303 passed, three skipped and 36 setup errors. Every error is a parameter variant of test_invalid_handle_insertion_owner_has_no_repository_or_governor_effects in MCP_unified/tests/test_model_completion_accounting.py, where pytest cannot find the mocker fixture.

These 36 errors are proven inherited: the baseline shard at ddd87310 has the identical 36 case names and missing-fixture errors, with the same 4,303 passes and three skips. That revision and this run's exact base 2f402bd7 have the identical entire Git tree 688e7bf3. The entire MCP source/tests tree (be8c2c59), root/shared conftest, shared test plugins, pyproject, GitHub configuration and helper scripts are identical between base and this PR head.

No magic-link regression is demonstrated by this shard. No unrelated MCP or dependency repair, test rerun or CI retry was made. The separate logging and database-fixture causes above remain unclassified. Required checks pass; remaining advisory jobs are still pending, so this is not a claim that all CI passed.

Requester Change summary exception

The requester explicitly waived the human-written Change summary requirement for PRs #3428, #3429, #3430 and #3431 in the CI follow-up chat, answering “waived” on November 6, 2026 (Pacific). Required checks and review still apply.

Summary by cubic

Makes magic-link redemption a durable single-winner operation: the one-time token is claimed atomically on the sign-in connection before any account change or session is issued, so concurrent submissions with the same link can no longer both succeed. A consumed or malformed link returns 400; a failed or unconfirmed storage write returns 503 without credentials; and the link stays spent if a later step fails. Missing or malformed jti/exp claims are rejected, and the OpenAPI fingerprint documents the 400/503 behavior.

The CI follow-up also repairs unrelated infrastructure bugs uncovered by the changed-file gate.

  • PostgreSQL pool returns roll back unfinished work and scrub scope synchronously before handing the connection back, discarding it if cleanup fails.
  • Log buffer reload drains queued writes before resetting module state.
  • The MCP accounting test and plugin registration, the storage-resolver fixture (now supplies the trusted membership context), and the web-scraping import inventory line references are corrected.
  • UI tests now wait for actual readiness: the reject button's AntD loading class clears before clicking, the MCP onboarding test awaits the default checked selections, and the watchlist test awaits the rendered update row before selecting it.
  • The oversized AuthNZ advisory shard splits into b-p/q-z across all six platform jobs, covering every file exactly once.
  • Includes mechanical typing repairs in touched files so mypy passes without ignores.

Advisory AuthNZ runtime follow-up (November 7)

The current AuthNZ b-z advisory shard reached its existing 55-minute deadline after 534 passing outcomes, without an assertion failure (run log). Split that selection into b-p and q-z across all six platform/Python jobs. The two groups preserve all 55 files exactly once (33 + 22); all other workflow settings, test assertions, timeouts, retries and protected checks are unchanged. A successful reference run divides into roughly 28 and 17 minutes before common startup costs.

Validation: both final VN PostgreSQL regressions pass; all 61 workflow contracts, existing shard-coverage guard, exact mypy 2.4.1 and Ruff pass. The new contract fails against the old layout and rejects all 18 deliberate gap, overlap and expanded-range mutations. Bandit adds only three test-assertion B101 findings. Independent review found no actionable issue, independently checked current/future selection equivalence and matrix limits, and verified that recombining the parsed YAML exactly restores the baseline workflow. The owned synthetic PostgreSQL container was removed.

The full integration shards were not rerun locally. Runner-to-runner cost variation remains unexplained; this is a shard-capacity correction, and fresh hosted CI must confirm its timing. Earlier validation, limitations and the requester’s Change summary waiver remain in force. No merge or auto-merge is requested.

Follow-up: wait for Watchlists search results before selecting a row

The advisory shared UI coverage shard 14/16 failed because the scale-behavior test observed the debounced request starting and immediately looked for Item Two while the results list was still loading. The test now awaits the same accessible row before its single click. Debounce request counts, selected reader content and the zero-extra-fetch assertion are unchanged; product code, timeouts and retries are unchanged.

A controlled 50 ms delay of only the mocked search response reproduced the exact missing-button failure before the correction and passed afterward. The original target also passed. All three file tests pass with the committed V8 coverage configuration (55.23 seconds), full WebUI TypeScript passes on Node 20.20.2 with 8192 MiB and incremental=false, and scoped ESLint is clean. The initial typecheck lacked temporary local dependency links; that setup error is retained separately and the same check passed after setup was corrected. Independent review found no actionable issue after inspecting the connected load/render/selection path and reproduction evidence; the reviewer independently checked the baseline and one-line diff but did not rerun runtime suites, TypeScript or lint. Bandit is inapplicable to this TypeScript test/task-only correction.

The full hosted coverage shard and browser E2E were not rerun locally; fresh CI on cf0a78d36a remains necessary. Six task-owned dependency links and their empty directories were removed and verified absent. Existing waivers and earlier validation limits remain in effect. No merge, base update, required-gate dispatch or restore/recovery operation was performed for this repair.