Api key guard hammers

[Adeyemi-cmd]

Closes #121

PR: Harden ApiKeyGuard hot path + fix SIWE nonce TOCTOU race

🔖 Title

hard: ApiKeyGuard hammers Supabase per request — cache key records by hash, collapse write amplification, normalize errors, per-key rate limit, revocation invalidation; plus SIWE nonce TOCTOU atomic claim + wallet throttling (5dd4772)

---

📝 Description

What was the problem?

ApiKeyGuard.canActivate() (src/auth/guards/api-key.guard.ts:35–114) was the scalability ceiling and a DoS amplifier:**

1. 2 DB round-trips per vendor request, zero caching — sha256(key) → .select('*').eq('key_hash', hash).single() plus an unconditional UPDATE last_used_at (src/auth/guards/api-key.guard.ts:104–113 old). Under vendor traffic this is pure hot-path load; flooding random keys saturates the Supabase connection pool.
2. Enumeration via distinct error codes — API_KEY_INVALID vs API_KEY_INACTIVE vs API_KEY_EXPIRED (src/auth/guards/api-key.guard.ts:59–80 old) let an attacker distinguish revoked vs expired vs nonexistent keys. Low severity alone, sloppy combined with (1).
3. No per-key rate limiting / lockout / anomaly tracking — a leaked key can be driven at line rate indefinitely. permissions check used .some(includes) with no wildcard/hierarchy, pushing consumers toward over-permissioned keys.
4. Write amplification 1:1 with reads — updateLastUsed fires on every request, no throttling.

Incidental hardening in same branch — AuthService.verifySignature() TOCTOU (5dd4772):

• Nonce verification was SELECT is('used_at', null).single() → verify → UPDATE used_at. Two concurrent POST /auth/verify with the same (wallet, nonce, signature) both observed used_at IS NULL, both verified, both succeeded — a classic TOCTOU replay that could mint unlimited sessions from one intercepted pair. The trailing UPDATE at old src/modules/auth/auth.service.ts:158 was not atomic with the read.

🔄 Changes Made

Core — src/auth/guards/api-key.guard.ts (src/auth/guards/api-key.guard.ts:1)

• Added CACHE_MANAGER (@Inject(CACHE_MANAGER) private readonly cacheManager: Cache) — same cache-manager + ioredis pattern already used in src/modules/liquidity/liquidity.service.ts:54 and src/modules/transactions/transactions.service.ts:121. No new infra.
• New constants (src/auth/guards/api-key.guard.ts:40):

  const API_KEY_CACHE_TTL_SECONDS = 60;
  const API_KEY_LAST_USED_TTL_SECONDS = 300;
  const API_KEY_RATE_LIMIT_WINDOW_SECONDS = 60;
  const API_KEY_RATE_LIMIT_MAX_REQUESTS = 60;

  and cache keys (src/auth/guards/api-key.guard.ts:45):

  getRecordCacheKey(keyHash) // apikey:record:<keyHash>
  getLastUsedCacheKey(keyId) // apikey:last_used:<keyId>
  getRateLimitCacheKey(keyId) // apikey:rate:<keyId>

• canActivate() (src/auth/guards/api-key.guard.ts:67):
  • Reads x-api-key, hashes with createHash('sha256'), tries cacheManager.get<ApiKeyRecord>(recordCacheKey) first (src/auth/guards/api-key.guard.ts:88). On miss, does the single DB SELECT (src/auth/guards/api-key.guard.ts:96); negative lookups are not cached to avoid polluting the store.
  • Unified validation (src/auth/guards/api-key.guard.ts:111): !is_active and expired expires_at both throw one UnauthorizedException({ code: 'API_KEY_UNAUTHORIZED', message: 'Invalid API key.' }). Distinct reasons only in Logger.warn (8-char hash prefix / keyId) — blocks enumeration of revoked vs expired vs nonexistent. Missing/malformed header also maps to same code (src/auth/guards/api-key.guard.ts:75).
  • Caches validated records only (src/auth/guards/api-key.guard.ts:130): double-checks cacheManager.get before set(recordCacheKey, keyRecord, 60) so inactive/expired never enter cache; new validated path explicitly avoids caching earlier. Revocation explicitly invalidates via VendorsService.
  • Calls enforceRateLimit(keyRecord.id, keyHash) (src/auth/guards/api-key.guard.ts:143) before permission checks.
  • Permission check unchanged (src/auth/guards/api-key.guard.ts:145 some(includes)) but now gated behind rate limit.
  • Fire-and-forget void this.maybeUpdateLastUsed(keyRecord.id) (src/auth/guards/api-key.guard.ts:164) instead of await + unconditional write.
  • Preserves request.apiKey contract (src/auth/guards/api-key.guard.ts:166 request.apiKey = keyRecord).
• enforceRateLimit() (src/auth/guards/api-key.guard.ts:170): cacheManager.get<number>(rateKey) ?? 0, if >=60 throws HttpException({ code:'API_KEY_RATE_LIMITED' }, 429). Otherwise set(rateKey, next, 60) — sliding window (each request resets TTL to full window). Cache failures are logged and fail-open (src/auth/guards/api-key.guard.ts:191 catches and re-throws only HttpException).
• maybeUpdateLastUsed() (src/auth/guards/api-key.guard.ts:198): checks cacheManager.get<boolean>(lastUsedKey); if flagged, returns. Otherwise update({ last_used_at }) + set(lastUsedKey, true, 300). Errors logged at warn, never throw. Collapses writes to at-most-once-per-5-min per key.

Invalidation — src/modules/vendors/vendors.service.ts (src/modules/vendors/vendors.service.ts:1)

• Injects CACHE_MANAGER (src/modules/vendors/vendors.service.ts:111 @Inject(CACHE_MANAGER) private readonly cacheManager: Cache).
• revokeApiKey() (src/modules/vendors/vendors.service.ts:636) now select('id, key_hash') (was just id check), updates is_active=false, then:

  await this.cacheManager.del(`apikey:record:${keyHash}`);
  await this.cacheManager.del(`apikey:rate:${keyId}`);
  await this.cacheManager.del(`apikey:last_used:${keyId}`);

  (src/modules/vendors/vendors.service.ts:676) — guarantees revoked key is rejected within one TTL (60s) even if it was cached, and resets its rate/last-used state. Failures are Logger.warn-only, so revocation still succeeds if Redis is down.

Wiring — src/app.module.ts:13

• Registers global CacheModule (CacheModule.registerAsync({ isGlobal: true, useFactory: getRedisConfig, inject: [ConfigService] })) so ApiKeyGuard and VendorsService share the same Redis/in-memory store. Required for the per-key apikey:* keys to be visible across modules. Uses existing getRedisConfig (src/config/redis.config.ts) — no new dependency.

Auth nonce TOCTOU — src/modules/auth/auth.service.ts (5dd4772)

• verifySignature() (src/modules/auth/auth.service.ts:120): replaced SELECT → verify → UPDATE with atomic claim before verification:

  const claimResult: any = await (client.from('nonces') as any)
    .update({ used_at: claimedAt }, { count: 'exact' })
    .eq('id', nonceRecord.id).is('used_at', null).select('id');

  Only winner gets count===1; loser count===0 → AUTH_NONCE_NOT_FOUND (src/modules/auth/auth.service.ts:207–225). Expiry check moved after claim so expired rows stay burned (src/modules/auth/auth.service.ts:230). Trailing UPDATE removed. Burn-on-failure tradeoff documented in code: invalid signature / bad StrKey / expired nonce still consumes the challenge (caller must POST /auth/nonce again).
• src/modules/auth/auth-throttler.guard.ts:1 (new): AuthWalletThrottlerGuard extends ThrottlerGuard keys on req.body.wallet (unauthenticated verify) fallback to req.user.wallet / IP (src/modules/auth/auth-throttler.guard.ts:14). Used alongside global IP guard so POST /auth/verify is bounded per wallet AND per IP (5 req/60s).
• src/modules/auth/auth.controller.ts:13 — adds @UseGuards(AuthWalletThrottlerGuard) to POST /auth/verify (src/modules/auth/auth.controller.ts:78), keeps @Throttle({ default: { limit:5, ttl:60000 } }), adds 429 Swagger response.
• src/modules/transactions/wallet-throttler.guard.ts:1 hardened to accept body.wallet (src/modules/transactions/wallet-throttler.guard.ts:12) and type-check strings, so same infrastructure is reused.

Tests

• test/unit/modules/auth/api-key.guard.spec.ts:1 (rewritten, ~500 lines) — asserts:
  • Cache hit avoids DB — mock cacheManager.get returns ApiKeyRecord → select mock not called; asserts never stores raw key (key_hash only, prefix apikey:record:<hash>).
  • Revocation invalidates within one TTL — VendorsService.revokeApiKey deletes three keys, manual del then DB re-check.
  • Rate limit trips and resets — 60 canActivate calls → 61st throws 429 API_KEY_RATE_LIMITED (HttpException 429); TTL expiry resets counter.
  • Enumeration uniformity — missing/malformed header, nonexistent hash, inactive, expired all throw API_KEY_UNAUTHORIZED (same code/message); only API_KEY_INSUFFICIENT_PERMISSIONS (403) and API_KEY_RATE_LIMITED (429) remain distinct.
  • Write amplification bounding — maybeUpdateLastUsed dirty-flag TTL skip path covered (second call within 300s does not issue UPDATE; updateEqFn call counts and last_used cache flag mocked). Explicitly addresses audit gap [12] Add unit tests for AuthService #4.
• test/unit/modules/vendors/vendors.service.spec.ts:14 — provides CACHE_MANAGER mock, verifies revokeApiKey deletes apikey:record:<hash>, apikey:rate:<keyId>, apikey:last_used:<keyId> and tolerates cache failures.
• test/unit/modules/auth/auth.service.spec.ts:1 (extended) — atomicity coverage:
  • should throw AUTH_NONCE_NOT_FOUND when atomic claim loses race (count===0), expiry stays burned, should mark nonce as used via atomic claim before signature verification, burn the nonce even when signature verification fails (second call NOT_FOUND), verifySignature — atomicity / concurrency: parallel double-verify yields exactly one success, replay after success fails, replay during failure burns, expired nonce stays burned.
• test/unit/modules/auth/auth-throttler.guard.spec.ts:1 (new) and test/unit/modules/auth/auth.controller.spec.ts — wallet-keyed throttler tracker (wallet:… vs fallback IP), controller guards mocked.
• context/progress-tracker.md:11 updated per Ground Rules.

---

---

[EmeditWeb]

⚠️ Automated Audit: partial

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

The code diffs genuinely implement all five requirements from issue #121: cache-backed key records keyed by hash (never full keys) with a 60s TTL, throttled last_used_at writes guarded by a 300s cache flag, normalized single API_KEY_UNAUTHORIZED error code, per-key sliding-window rate limiting via cache-manager with a structured 429, and cache invalidation on revocation in VendorsService. The api-key.guard.spec.ts tests were substantively rewritten to assert cache-hit-avoids-DB, revocation invalidation, rate-limit trip/reset, and error-code uniformity, and vendors.service.spec.ts verifies the cache deletes. However, the PR description is a copy-pasted empty template (Title/Description/Changes Made/Screenshots/Notes all empty or placeholder-only) and it mismatches the issue: it never mentions #121's API-key caching work, instead describing a different TOCTOU nonce/verify fix, so the claimed linkage is unsubstantiated by the description and the coverage of the intended root cause is unverified by any written explanation. Independent CI passed (build-test) and there are no merge conflicts, but because the description is placeholder-only and the actual issue-specific behavior is only evidenced via code/diffs rather than a coherent explanation, and per the rules vague/placeholder descriptions are author-fixable gaps that preclude full approval, this is partial rather than solves.

Gaps identified:

• PR description is an unfilled copy-pasted template (Title, Description, Changes Made, Screenshots, Additional Notes all blank) — must explain WHAT was fixed, WHY it links to #121, and HOW it was tested
• Description content describes a different change (AuthService nonce TOCTOU / verify throttling) and never actually documents the ApiKeyGuard caching/rate-limit/error-normalization work that is the core of #121 — issue linkage and root-cause explanation are missing
• Confirm negative (non-existent key) lookups are not cached is acceptable, but the rate-limit increment resets the full sliding TTL on every request, meaning a steady high-volume key never actually resets/windows correctly — verify window semantics match spec intent
• Regression tests are present and updated, but no explicit test asserts write-amplification bounding (the maybeUpdateLastUsed dirty-flag TTL skip path) — the cache-guard logic is untested directly

---

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 implements every root-cause item from #121: key records are cached by sha256 hash prefix behind cache-manager with a 60s TTL and never store raw keys, last_used_at writes are collapsed to at-most-once-per-5-min via a cache dirty flag and fired without blocking, invalid/inactive/expired/missing all normalize to a single API_KEY_UNAUTHORIZED (details only in server-side Logger.warn), and a per-key sliding-window counter returns a structured 429. revokeApiKey() now invalidates apikey:record/rate/last_used keys, and a global CacheModule plus VendorsService CACHE_MANAGER injection wires it together. Tests were added/rewritten for cache-hit-no-DB, enumeration uniformity, rate-limit trip/reset, and revocation deletion, CI (build-test) passed, CI config is untouched, and title/description are substantive and consistent with the diffs. Residual notes: unknown/random keys are intentionally not cached and have no keyId so bare key-flood still hits the DB once per request (rate limiting only guards known keys), revocation invalidation is asserted at the service layer rather than an end-to-end guard regression proving a cached key is rejected within one TTL, and the cached-store guarantees depend on getRedisConfig resolving to a shared Redis (not in-memory) instance.

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

Gaps identified:

• Random/unknown-key floods still hit the DB once per request (negative lookups intentionally uncached, and per-key rate limits are keyed on known keyId only); a per-IP/burst limit at the guard would fully close the DoS amplifier from invalid keys.
• No end-to-end regression test that a previously cached key is actually rejected after revokeApiKey() within one TTL — the new tests assert the three cache del calls, not the guard+service outcome.
• Correctness of shared invalidation and rate limiting depends on getRedisConfig returning a real shared Redis store; if it ever falls back to in-memory cache-manager in some environment, revocation and 429 counters become per-instance.

---

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

_{Audited by stepfi-audit-bot 🤖}