feat: SIWE-style nonce verification has a TOCTOU race

[Adeyemi-cmd]

PR: Fix SIWE nonce TOCTOU race — atomic claim + per-wallet throttling + merge domain-binding

🔖 Title

critical: SIWE-style nonce verification has a TOCTOU race — make nonce consumption atomic (UPDATE ... WHERE used_at IS NULL before verify, burn-on-failure) + per-wallet throttling on POST /auth/verify; merge upstream/main domain-binding envelope so message_hash binding survives

---

📝 Description

What was the problem?

AuthService.verifySignature() (src/modules/auth/auth.service.ts:94–142 old) used a read-then-write pattern:**

1. SELECT ... WHERE used_at IS NULL AND nonce = ? AND wallet = ? (src/modules/auth/auth.service.ts:96–102 old) — returns the same unused row to concurrent callers.
2. Expensive signature verification (src/modules/auth/auth.service.ts:109–136 old) — Keypair.verify over nonce and Stellar Signing Key: <nonce> fallback — sits between check and mark.
3. Only afterwards UPDATE nonces SET used_at = ... (src/modules/auth/auth.service.ts:141 old).

Two concurrent POST /auth/verify carrying the same (wallet, nonce, signature) both execute step 1 before either reaches step 3, both pass verification, both mint independent sessions (each with its own refresh token). One stolen/intercepted nonce+signature pair therefore creates unlimited sessions — textbook TOCTOU, window widened by the verify cost. Additionally there was no per-wallet rate limit on verify, enabling offline-style brute force of the SEP-0043 fallback space at network speed, and no guarantee that an expired nonce stays burned.

🔄 Changes Made

Core — src/modules/auth/auth.service.ts (src/modules/auth/auth.service.ts:94)

HEAD 5dd4772 atomic claim (now preserved on top of upstream envelope):

• Before: SELECT ... .is('used_at', null).single() → if (expires_at < now) → StrKey/Keypair.verify → UPDATE used_at (post-verify, not atomic).

• After (src/modules/auth/auth.service.ts:181–270): SELECT id, expires_at, issued_at, message_hash ... .is('used_at', null).single() → if (nonceError || !nonceRecord) → atomic claim UPDATE nonces SET used_at=claimedAt WHERE id=? AND used_at IS NULL with {count:'exact'} (src/modules/auth/auth.service.ts:207) → DATABASE_NONCE_CLAIM_FAILED on claimError, AUTH_NONCE_NOT_FOUND on claimedCount===0 (covers count and data.length fallback) → expiry after claim if (new Date(expires_at) < now) AUTH_NONCE_EXPIRED stays burned → StrKey → try { Keypair.fromPublicKey } branching on signatureType (see upstream envelope below). No trailing UPDATE — claim already burned the row.

  Security tradeoff documented in code comments (src/modules/auth/auth.service.ts:194–206): if verification subsequently fails (bad signature, bad StrKey, expired envelope.expirationTime), the nonce stays burned; caller must POST /auth/nonce again. This converts replay into DoS-on-self (one wasted challenge) vs unlimited sessions — correct vs allowing unlimited creations.

Tests

• test/unit/modules/auth/auth.service.spec.ts:1 — HEAD 5dd4772 TOCTOU suites (now merged, 325+ lines added):

  • should throw AUTH_NONCE_NOT_FOUND when atomic claim loses race (count===0) (test/unit/modules/auth/auth.service.spec.ts:388)
  • should throw AUTH_NONCE_EXPIRED when nonce is past expiry (nonce stays burned) (test/unit/modules/auth/auth.service.spec.ts:395)
  • should mark nonce as used via atomic claim before signature verification (test/unit/modules/auth/auth.service.spec.ts:610)
  • should burn the nonce even when signature verification fails (DoS-on-self tradeoff) — second call NOT_FOUND (test/unit/modules/auth/auth.service.spec.ts:617)
  • verifySignature — atomicity / concurrency (test/unit/modules/auth/auth.service.spec.ts:677): parallel double-verify yields exactly one success (atomic claim) via Promise.allSettled + count race, replay after success fails, replay during failure burns, expired nonce stays burned.
  • Plus upstream envelope suites (retained): generateNonce envelope fields/message_hash stored, envelope/sep0043 verify, AUTH_CHALLENGE_MISMATCH/DOMAIN_MISMATCH/NETWORK_MISMATCH, expirationTime expiry, missing message_hash, legacy flag/sunset.

• test/unit/modules/auth/auth-throttler.guard.spec.ts:1 (new) — getTracker returns wallet:<address> for body.wallet and user.wallet, falls back to IP.

• test/unit/modules/auth/auth.controller.spec.ts — updated to provide AuthWalletThrottlerGuard mock.

• test/e2e/modules/auth/auth.e2e-spec.ts (from upstream) — asserts POST /auth/nonce returns message and full envelope/sep0043 flows.

---

---

[EmeditWeb]

⚠️ Automated Audit: partial

@Adeyemi-cmd Good start — please look into the gaps identified below.

The PR correctly implements atomic nonce consumption by replacing the SELECT→verify→UPDATE sequence with a conditional UPDATE ... WHERE used_at IS NULL executed before signature verification, which directly eliminates the TOCTOU window described in issue #116. The burn-on-failure tradeoff is properly documented and implemented, per-wallet throttling is added via AuthWalletThrottlerGuard, and comprehensive tests cover parallel double-verify, replay after success/failure, and expired nonce scenarios. CI passes (build-test green). The PR description is terse (only ~3% keyword overlap with the issue) and leaves the template checkboxes unfilled, but the code changes substantively address every acceptance criterion in the issue.

⚖️ Adjusted by bot policy: gaps were still identified; the PR title/summary is terse — a descriptive title and a proper description of what/why/testing are required.

Gaps identified:

• PR description is a nearly-empty template — should explain what changed, why (linking #116), and how it was tested

---

CI checks: ✅ PASSED: build-test
Merge conflicts: ✅ none — but the PR is blocked (failing/missing required checks or reviews).

_{Audited by stepfi-audit-bot 🤖}

[EmeditWeb]

⚠️ @Adeyemi-cmd this PR now has merge conflicts with the base branch (likely because another PR was merged first).

Please rebase/merge the base branch into your branch and resolve the conflicts — a fresh audit will run automatically once new commits land.

[EmeditWeb]

⚠️ Automated Audit: partial

@Adeyemi-cmd Good start — please look into the gaps identified below.

The diff substantively fixes the TOCTOU root cause described in #116: the read-then-write pattern is replaced by an atomic conditional claim (UPDATE nonces SET used_at = ? WHERE id = ? AND used_at IS NULL with count check) executed BEFORE signature verification, with burned-on-failure semantics and the trailing post-verify UPDATE removed — so one (wallet, nonce) pair can produce at most one successful session. Per-wallet throttling is genuinely wired in via a new wallet-keyed AuthWalletThrottlerGuard applied to POST /auth/verify, and regression tests cover the atomic race, replay-after-success, replay-after-failure, and expired-nonce-burned paths. CI (build-test) passed on the actual changes and the PR's claimed test counts (38 suites, 425 tests) are consistent with the independent run, not contradicted by it.

⚖️ Adjusted by bot policy: confidence 85% is below the 90% threshold for a full approval; gaps were still identified.

Gaps identified:

• Atomicity is proven only via mocked unit tests simulating the count race; no real Postgres-level parallel e2e test exercises the conditional UPDATE under genuine concurrency (acceptable since Postgres UPDATE is natively atomic, but the issue explicitly asked for a concurrency proof).
• No e2e test asserts a 429 on POST /auth/verify past the per-wallet throttle limit - test coverage for the throttling wiring is unit-level only.
• Issue's secondary mention of expired-nonce cleanup that is 'tied to this flow' is not addressed (the expired row is now burned but no deletion/cleanup job was added).

---

CI checks: ✅ PASSED: build-test
Merge conflicts: ✅ none — but the PR is blocked (failing/missing required checks or reviews).

_{Audited by stepfi-audit-bot 🤖}

[EmeditWeb]

⚠️ Automated Audit: partial

@Adeyemi-cmd Good start — please look into the gaps identified below.

The diff genuinely replaces the SELECT→verify→UPDATE sequence with an atomic conditional claim (UPDATE ... WHERE id = ? AND used_at IS NULL with count === 0 rejected as AUTH_NONCE_NOT_FOUND) executed before signature verification and removes the trailing UPDATE, which eliminates the TOCTOU window; expired rows are burned after claim and the burn-on-failure tradeoff is documented in code. Per-wallet throttling is genuinely wired via the new AuthWalletThrottlerGuard (body/user wallet key with IP fallback) plus the existing IP throttle. Regression tests cover the claim race, burn-on-failure, replay-after-success, expired-nonce, and parallel double-verify via Promise.allSettled, and CI passed. Minor residual: atomicity is proven only at the unit-mock level (simulated count races), not a real-Postgres integration test, and the issue's secondary note about expired-nonce cleanup was not addressed beyond burning on claim.

⚖️ Adjusted by bot policy: confidence 85% is below the 90% threshold for a full approval; gaps were still identified.

Gaps identified:

• Concurrency/atomicity proven only via mocked unit tests (simulated count===0 races); no e2e/integration test against real Postgres proving parallel identical verify requests yield exactly one success
• No cleanup mechanism for expired nonces (secondary note in the issue) — rows are burned on claim but a scheduled cleanup job was not added

---

CI checks: ✅ PASSED: build-test
Merge conflicts: ✅ none — but the PR is blocked (failing/missing required checks or reviews).

_{Audited by stepfi-audit-bot 🤖}

[Adeyemi-cmd]

@EmeditWeb please review

[EmeditWeb]

@EmeditWeb please review

fix merge conflicts

[EmeditWeb]

⚠️ @Adeyemi-cmd this PR now has merge conflicts with the base branch (likely because another PR was merged first).

Please rebase/merge the base branch into your branch and resolve the conflicts — a fresh audit will run automatically once new commits land.

[EmeditWeb]

⚠️ Automated Audit: partial

@Adeyemi-cmd Good start — please look into the gaps identified below.

The code changes genuinely fix the TOCTOU nonce race by implementing atomic claim (UPDATE ... WHERE used_at IS NULL) before signature verification, and add per-wallet throttling via AuthWalletThrottlerGuard. Tests cover atomicity, expiry, burn-on-failure, and throttling. However, the PR has merge conflicts with the base branch that prevent it from being merged/approved.

---

CI checks: ✅ PASSED: build-test
Merge conflicts: ⚠️ YES — this PR has conflicts with the base branch and cannot be merged. Please resolve the conflicts before this PR can be approved.

_{Audited by stepfi-audit-bot 🤖}