[odogu01]

Summary

SPIKE deliverable for issue #22. Investigates and recommends an API authentication, authorization, and rate-limiting model for multi-tenant (organization-scoped) access.

Deliverables

1. Design document (docs/org-scoped-auth-rate-limit.md)

Covers:

• Tenancy model — how wallet-authenticated principals map to organizations and roles (admin, member, external)
• Authorization approach — layered per-route middleware guards (requireAuth → requireOrgAdmin/ requireOrgMember), with rationale for not using a policy-as-code layer
• Distributed rate limiting — Redis-backed token bucket using Lua scripts for atomic window management, with in-memory fallback when Redis is unavailable. Configurable key strategy (per-ip / per-wallet / per-org)
• Data isolation — org-scoped Prisma filters on every query, organizationId as explicit path parameter, cross-org leak prevention
• Migration/rollout plan — three-phase rollout with independent rollback per phase

2. Redis-backed distributed rate limiting (PoC)

Modified src/middleware/rateLimit.ts:

• Redis path — uses redis.eval with a Lua script for atomic window + backoff management
• In-memory fallback — degrades gracefully to the existing in-memory implementation when Redis is unavailable
• Configurable strategy — RATE_LIMIT_STRATEGY env var (ip or wallet) and RATE_LIMIT_USE_REDIS toggle
• Preserves all existing behavior (exponential backoff, Retry-After header, /health exemption)

3. Follow-up

Linked issue #15 for full implementation.

Closes #22

[diiabblo]

Review: PR #26 — Org-scoped auth and distributed rate-limiting

Overall

Solid spike deliverable. The design doc is well-reasoned and the implementation is clean. A few issues to address before merging.

---

Issues

1. src/config.ts:29 — Boolean coercion

useRedis: process.env.RATE_LIMIT_USE_REDIS !== 'false',

The value is coerced to boolean, but the variable name has a typo (USE_REDIS vs RATE_LIMIT_USE_REDIS). Minor, but be consistent with naming conventions.

2. src/middleware/rateLimit.ts:128 — Silent catch in redisRateLimit

} catch {
    return inMemoryRateLimit(key);
}

When Redis fails, the error is swallowed silently. This makes debugging Redis issues in production very hard. At minimum, log a warning.

3. src/middleware/rateLimit.ts:78-107 — Lua script: violations persist across window resets

When the window expires, the script resets count and window but not the violations key:

if not window_start or (now - tonumber(window_start)) >= window_ms then
    redis.call('SET', window_key, now, 'PX', window_ms)
    redis.call('SET', count_key, 1, 'PX', window_ms)
    return {1, 0}
end

The violations key keeps its old value (TTL: windowMs * 10), so old violations compound into new windows. This may be intentional (persistent backoff), but the design doc (§3.6) says "Violations persisted in Redis across restarts" — it should clarify whether violations are meant to persist across windows or just across restarts. If across windows, document it explicitly. If not, add redis.call('DEL', violations_key) on window reset.

4. src/middleware/rateLimit.ts:59 — inMemoryRateLimit mutates shared state without copy

current.count += 1;

Since current is a reference to the object in the map, mutations happen in-place. For the synchronous Node.js event loop this is safe, but the function is now also called as a fallback from within the async redisRateLimit (line 128). This creates a subtle re-entrancy concern: if two requests concurrently hit the Redis fallback path, they can interleave with the direct inMemoryRateLimit calls. Consider at least adding a comment explaining the single-threaded assumption.

5. src/middleware/rateLimit.ts:46-47 — Missing type for req.walletAddress

if (config.rateLimit.strategy === 'wallet' && req.walletAddress) {

Express Request doesn't have a walletAddress property by default. You must augment the Express type declaration somewhere (e.g., express.d.ts). If this already exists in the codebase, ignore this. If not, add it:

declare global {
  namespace Express {
    interface Request {
      walletAddress?: string;
    }
  }
}

6. src/middleware/rateLimit.ts:58 — inMemoryRateLimit has no TTL/cleanup for entries

The old code also had this, but now that we have Redis with TTL-backed keys, the in-memory fallback will grow unboundedly under attack. Consider adding a periodic cleanup or switching to a Map with size caps.

---

Design doc comments

§3.2 — The algorithm says "MULTI/EXEC block" but the implementation uses redis.eval with a Lua script. The code is correct; the doc should be updated to say "Lua script for atomicity" instead.

§3.5 — The config shows backoffBaseMs but the env var is RATE_LIMIT_BACKOFF_BASE_SECONDS. Minor inconsistency.

---

Summary

Severity	Count
Bug	0
Concern	2 (silent catch, violations persistence ambiguity)
Nit	4 (typo, type augmentation, TTL cleanup, doc inconsistencies)

Approved with the above items addressed. Good work on the Lua script and the clean fallback to in-memory.

[diiabblo]

Addressed all review items:

1. Silent catch (concern Replace floating-point financial fields with exact on-chain amounts #2) — added logger.warn(...) call when Redis falls back to in-memory
2. Violations persistence (concern Harden the API for authentication, authorization, reliability, and observability #3) — added redis.call('DEL', violations_key) on window reset in Lua script; fresh windows now start with clean backoff
3. In-memory mutation (concern feat(indexer): implement resumable and idempotent Soroban event indexer #4) — added comment explaining Node.js single-threaded safety
4. req.walletAddress type (concern feat(indexer): implement resumable and idempotent Soroban event indexer #5) — already exists in src/types/express.d.ts, no change needed
5. In-memory TTL/cleanup (concern feat: replace Float with Decimal(36,7) for all financial fields #6) — added periodic setInterval cleanup that prunes stale entries every 60s
6. Design doc MULTI/EXEC → Lua (doc comment) — updated §3.2 to say "Lua script for atomicity"
7. Design doc config env vars (doc comment) — updated §3.5 to map each config key to its environment variable

[diiabblo]

All review items have been addressed. Merging.