fix: wire per-wallet throttling and add TOCTOU concurrency/cleanup proofs

Per-wallet throttling was DI-broken: AuthWalletThrottlerGuard and
WalletThrottlerGuard extended ThrottlerGuard but were used via
@UseGuards (instantiated with `new` without ThrottlerStorageService),
so the 5 req/60s limit on POST /auth/verify was never enforced.
Re-implement both guards as DI-free CanActivate with static in-memory
sliding window (limit 5/10, ttl 60s), wallet-prefixed tracker with IP
fallback, throwing ThrottlerException → 429. Keeps legacy constructor
signature for existing unit tests.

Add missing NonceCleanupService unit coverage (hourly cron deletes
expires_at < now-1h, including burned rows, idempotent, error swallow).

Add e2e proof for audit gaps: parallel double-verify yields exactly one
200 (atomic UPDATE ... WHERE used_at IS NULL via InMemoryStore),
burn-on-failure, and per-wallet 429 on 6th request with isolation.
Uses jest.requireActual("stellar-sdk") to bypass test/__mocks__
deterministic mock.

Co-authored-by: Muse Spark <muse-spark@opencode.ai>

Author: Adeyemi-cmd

src/modules/auth/auth-throttler.guard.ts

@@ -1,21 +1,34 @@
-import { Injectable } from '@nestjs/common';
-import { ThrottlerGuard } from '@nestjs/throttler';
+import { CanActivate, ExecutionContext, Injectable } from '@nestjs/common';
+import { ThrottlerException } from '@nestjs/throttler';
 
 /**
- * ThrottlerGuard variant for POST /auth/verify that keys rate limits on the
+ * Throttler guard variant for POST /auth/verify that keys rate limits on the
  * wallet address supplied in the request body (unauthenticated) or on the
  * authenticated wallet (if present). Prefers body wallet because verify is
  * unauthenticated — the wallet is not yet in req.user.
  *
- * Falls back to the default IP-based tracker when no wallet is present so
+ * Falls back to IP-based tracking when no wallet is present so
  * anonymous/probe traffic is still bounded per IP.
  *
  * Used alongside the global IP-based ThrottlerGuard so POST /auth/verify is
  * bounded per wallet AND per IP — preventing brute-force of the SEP-0043
  * fallback space at network speed and limiting stolen-nonce replay attempts.
+ *
+ * Implements its own in-memory sliding window so it works with
+ * `@UseGuards(AuthWalletThrottlerGuard)` without requiring Nest DI for
+ * ThrottlerGuard's storage service (which is not injected when a guard is
+ * instantiated via `@UseGuards`). Mirrors the semantics of the legacy
+ * `ThrottlerGuard extends` version but is DI-free and therefore testable
+ * with a plain `new` and usable in e2e without additional module wiring.
  */
 @Injectable()
-export class AuthWalletThrottlerGuard extends ThrottlerGuard {
+export class AuthWalletThrottlerGuard implements CanActivate {
+  private static readonly hits = new Map<string, { count: number; expiresAt: number }>();
+  private readonly limit = 5;
+  private readonly ttl = 60000;
+
+  constructor() {}
+
   protected async getTracker(req: Record<string, unknown>): Promise<string> {
     const body = (req as { body?: { wallet?: unknown } }).body;
     const user = (req as { user?: { wallet?: unknown } }).user;
@@ -26,6 +39,31 @@ export class AuthWalletThrottlerGuard extends ThrottlerGuard {
     if (wallet) {
       return `wallet:${wallet}`;
     }
-    return super.getTracker(req);
+    const ip = (req as unknown as { ip?: string }).ip;
+    if (typeof ip === 'string' && ip.length > 0) return ip;
+    const forwarded = (req as unknown as { headers?: Record<string, string> }).headers?.['x-forwarded-for'];
+    if (typeof forwarded === 'string' && forwarded.length > 0) return forwarded.split(',')[0].trim();
+    return 'unknown';
+  }
+
+  async canActivate(context: ExecutionContext): Promise<boolean> {
+    const req = context.switchToHttp().getRequest<Record<string, unknown>>();
+    const tracker = await this.getTracker(req);
+    const now = Date.now();
+    const entry = AuthWalletThrottlerGuard.hits.get(tracker);
+    if (!entry || now > entry.expiresAt) {
+      AuthWalletThrottlerGuard.hits.set(tracker, { count: 1, expiresAt: now + this.ttl });
+      return true;
+    }
+    entry.count += 1;
+    if (entry.count > this.limit) {
+      throw new ThrottlerException();
+    }
+    return true;
+  }
+
+  /** Test helper: reset in-memory throttle state between isolated tests. */
+  static clearStorage(): void {
+    AuthWalletThrottlerGuard.hits.clear();
   }
 }

src/modules/transactions/wallet-throttler.guard.ts

@@ -1,24 +1,59 @@
-import { Injectable } from '@nestjs/common';
-import { ThrottlerGuard } from '@nestjs/throttler';
+import { CanActivate, ExecutionContext, Injectable } from '@nestjs/common';
+import { ThrottlerException } from '@nestjs/throttler';
 
 /**
- * ThrottlerGuard variant that keys rate limits on the authenticated wallet
+ * Throttler guard variant that keys rate limits on the authenticated wallet
  * (from the JWT payload) instead of the client IP. Used alongside the global
  * IP-based guard so POST /transactions/submit is bounded per wallet AND per
  * IP, preventing a single wallet from being used as an open relay to Horizon.
  *
  * Also checks req.body.wallet so the same guard can be reused for
  * unauthenticated routes like POST /auth/verify where the wallet is in the
  * request body.
+ *
+ * DI-free implementation (see AuthWalletThrottlerGuard for rationale) so
+ * `@UseGuards(WalletThrottlerGuard)` works without Nest injecting
+ * `ThrottlerStorageService`.
  */
 @Injectable()
-export class WalletThrottlerGuard extends ThrottlerGuard {
+export class WalletThrottlerGuard implements CanActivate {
+  private static readonly hits = new Map<string, { count: number; expiresAt: number }>();
+  private readonly limit = 10;
+  private readonly ttl = 60000;
+
+  constructor() {}
+
   protected async getTracker(req: Record<string, unknown>): Promise<string> {
     const user = (req as { user?: { wallet?: unknown } }).user;
     const body = (req as { body?: { wallet?: unknown } }).body;
     const userWallet = typeof user?.wallet === 'string' ? user.wallet : undefined;
     const bodyWallet = typeof body?.wallet === 'string' ? body.wallet : undefined;
     const wallet = userWallet ?? bodyWallet;
-    return wallet ? `wallet:${wallet}` : super.getTracker(req as Record<string, unknown>);
+    if (wallet) return `wallet:${wallet}`;
+    const ip = (req as unknown as { ip?: string }).ip;
+    if (typeof ip === 'string' && ip.length > 0) return ip;
+    const forwarded = (req as unknown as { headers?: Record<string, string> }).headers?.['x-forwarded-for'];
+    if (typeof forwarded === 'string' && forwarded.length > 0) return forwarded.split(',')[0].trim();
+    return 'unknown';
+  }
+
+  async canActivate(context: ExecutionContext): Promise<boolean> {
+    const req = context.switchToHttp().getRequest<Record<string, unknown>>();
+    const tracker = await this.getTracker(req);
+    const now = Date.now();
+    const entry = WalletThrottlerGuard.hits.get(tracker);
+    if (!entry || now > entry.expiresAt) {
+      WalletThrottlerGuard.hits.set(tracker, { count: 1, expiresAt: now + this.ttl });
+      return true;
+    }
+    entry.count += 1;
+    if (entry.count > this.limit) {
+      throw new ThrottlerException();
+    }
+    return true;
+  }
+
+  static clearStorage(): void {
+    WalletThrottlerGuard.hits.clear();
   }
 }

test/e2e/modules/auth/auth-atomic-and-throttle.e2e-spec.ts

@@ -0,0 +1,169 @@
+import * as request from 'supertest';
+import { INestApplication } from '@nestjs/common';
+import { AuthWalletThrottlerGuard } from '../../../../src/modules/auth/auth-throttler.guard';
+import { buildTestApp, InMemoryStore } from '../../helpers/test-setup';
+
+// Use the real stellar-sdk implementation even when a manual mock exists at
+// test/__mocks__/stellar-sdk.js (which is auto-applied for `jest.mock` in
+// unit tests). `jest.requireActual` bypasses the mock and gives us a
+// Keypair that generates distinct random wallets per call.
+const { Keypair: RealKeypair } = jest.requireActual('stellar-sdk') as typeof import('stellar-sdk');
+type RealKeypairType = InstanceType<typeof RealKeypair>;
+function createTestKeypair(): RealKeypairType {
+  return RealKeypair.random() as unknown as RealKeypairType;
+}
+function signMessage(keypair: RealKeypairType, message: string): string {
+  return (keypair as unknown as { sign: (b: Buffer) => Buffer }).sign(Buffer.from(message)).toString('base64');
+}
+
+/**
+ * E2E coverage for the two gaps identified by the audit bot:
+ *  - atomic nonce consumption under genuine concurrency (parallel identical
+ *    POST /auth/verify must yield exactly one 200, the other 401)
+ *  - per-wallet throttling on POST /auth/verify (6 rapid requests from the
+ *    same wallet must yield 429 on the 6th)
+ *
+ * Uses the InMemoryStore mock (via buildTestApp) rather than a real Postgres
+ * instance. The store's UPDATE ... is('used_at', null) predicate is evaluated
+ * in-memory, so the second concurrent claim correctly sees count 0 / empty
+ * data and is rejected as AUTH_NONCE_NOT_FOUND. This is the user-visible
+ * contract even though the underlying atomicity is ultimately provided by
+ * Postgres `UPDATE ... WHERE used_at IS NULL` in production.
+ */
+describe('Auth verify — atomic claim & per-wallet throttling (e2e)', () => {
+  let app: INestApplication;
+  let mockDb: InMemoryStore;
+
+  beforeAll(async () => {
+    const built = await buildTestApp();
+    app = built.app;
+    mockDb = built.mockDb;
+  });
+
+  afterAll(async () => {
+    await app.close();
+  });
+
+  beforeEach(() => {
+    mockDb.clear();
+    AuthWalletThrottlerGuard.clearStorage();
+  });
+
+  it('parallel double-verify with same (wallet, nonce, signature) yields exactly one success (atomic claim)', async () => {
+    const keypair = createTestKeypair();
+    const wallet = keypair.publicKey();
+
+    const nonceRes = await request(app.getHttpServer())
+      .post('/auth/nonce')
+      .send({ wallet })
+      .expect(201);
+
+    const nonce: string = nonceRes.body.nonce;
+    expect(nonce).toHaveLength(64);
+
+    // Legacy raw scheme: signature over the nonce hex bytes.
+    const signature = signMessage(keypair, nonce);
+
+    const results = await Promise.allSettled([
+      request(app.getHttpServer()).post('/auth/verify').send({ wallet, nonce, signature }),
+      request(app.getHttpServer()).post('/auth/verify').send({ wallet, nonce, signature }),
+    ]);
+
+    // supertest always fulfills; inspect HTTP status directly
+    const statuses = results.map((r) =>
+      r.status === 'fulfilled' ? (r.value as request.Response).status : 0,
+    );
+
+    const successes = statuses.filter((s) => s === 200);
+    const notFounds = statuses.filter((s) => s === 401);
+
+    expect(successes).toHaveLength(1);
+    expect(notFounds).toHaveLength(1);
+
+    // A third sequential replay must also fail with 401 (nonce stays burned)
+    await request(app.getHttpServer())
+      .post('/auth/verify')
+      .send({ wallet, nonce, signature })
+      .expect(401);
+  });
+
+  it('expired nonce is rejected and stays burned — second attempt is NOT_FOUND, not success', async () => {
+    const keypair = createTestKeypair();
+    const wallet = keypair.publicKey();
+
+    const nonceRes = await request(app.getHttpServer())
+      .post('/auth/nonce')
+      .send({ wallet })
+      .expect(201);
+
+    const nonce: string = nonceRes.body.nonce;
+    const signature = signMessage(keypair, nonce);
+
+    // Manually expire the nonce row in the mock store (bulk update via store)
+    // The mock store holds rows in memory; find and mutate.
+    const rows = mockDb.dump('nonces');
+    const row = rows.find((r) => r.nonce === nonce);
+    if (row) {
+      row.expires_at = new Date(Date.now() - 1000).toISOString();
+    }
+
+    // First verify: claim succeeds but expiry check fails -> 401 AUTH_NONCE_EXPIRED
+    await request(app.getHttpServer())
+      .post('/auth/verify')
+      .send({ wallet, nonce, signature })
+      .expect(401);
+
+    // Second verify: nonce already claimed/burned -> 401 AUTH_NONCE_NOT_FOUND
+    // (burn-on-failure semantics)
+    await request(app.getHttpServer())
+      .post('/auth/verify')
+      .send({ wallet, nonce, signature })
+      .expect(401);
+  });
+
+  it('per-wallet throttling: 6 rapid POST /auth/verify from the same wallet yields 429 on the 6th', async () => {
+    const wallet = createTestKeypair().publicKey();
+    const fakeNonce = 'a'.repeat(64);
+    const fakeSig = Buffer.alloc(64).toString('base64');
+
+    // First 5 requests: throttler allows them (service returns 401 AUTH_NONCE_NOT_FOUND,
+    // but throttler does not block)
+    for (let i = 0; i < 5; i++) {
+      await request(app.getHttpServer())
+        .post('/auth/verify')
+        .send({ wallet, nonce: fakeNonce, signature: fakeSig })
+        .expect(401);
+    }
+
+    // 6th request from same wallet: per-wallet guard must reject with 429
+    await request(app.getHttpServer())
+      .post('/auth/verify')
+      .send({ wallet, nonce: fakeNonce, signature: fakeSig })
+      .expect(429);
+  });
+
+  it('per-wallet throttling is isolated — a different wallet is not throttled by the first wallet’s quota', async () => {
+    const walletA = createTestKeypair().publicKey();
+    const walletB = createTestKeypair().publicKey();
+    const fakeNonce = 'b'.repeat(64);
+    const fakeSig = Buffer.alloc(64).toString('base64');
+
+    for (let i = 0; i < 5; i++) {
+      await request(app.getHttpServer())
+        .post('/auth/verify')
+        .send({ wallet: walletA, nonce: fakeNonce, signature: fakeSig })
+        .expect(401);
+    }
+    // walletA exhausted
+    await request(app.getHttpServer())
+      .post('/auth/verify')
+      .send({ wallet: walletA, nonce: fakeNonce, signature: fakeSig })
+      .expect(429);
+
+    // walletB should still be allowed (gets 401, not 429)
+    await request(app.getHttpServer())
+      .post('/auth/verify')
+      .send({ wallet: walletB, nonce: fakeNonce, signature: fakeSig })
+      .expect(401);
+  });
+});