fix(security): TOCTOU race in webhook rate limiter — concurrent requests can bypass WEBHOOK_RATE_LIMIT

[AndriiPasternak31]

Summary

_check_webhook_rate_limit in src/backend/routers/webhooks.py performs the count-check and the count-increment as two separate Redis round-trips, not atomically. Concurrent requests at threshold-1 can all read the under-limit count, all skip the 429, and all increment — letting the actual call rate exceed WEBHOOK_RATE_LIMIT by N (N = concurrency).

The new regression test in tests/integration/test_webhook_rate_limit.py (added in PR #643) fires sequential requests and won't catch this race.

Context

Out-of-scope follow-up surfaced during review of PR #643 (Redis lock-down, #589). This race is pre-existing, not introduced by #589 — but PR #643's regression test gives a false sense of coverage for "rate limiter actually limits."

src/backend/routers/webhooks.py:209-221:

key = f"webhook_calls:{token}"
try:
    count = r.get(key)                              # T1: read 9
    if count and int(count) >= WEBHOOK_RATE_LIMIT:  # T2: 9 < 10 → skip 429
        ...
        raise HTTPException(429)
    pipe = r.pipeline()
    pipe.incr(key)                                  # T3: now 10/11/12
    pipe.expire(key, WEBHOOK_RATE_WINDOW)
    pipe.execute()

Three concurrent requests can all read 9, all skip the 429, all increment to {10, 11, 12} — so WEBHOOK_RATE_LIMIT=10 actually allows 12 calls in the window.

The existing INPROCESS_FALLBACK at INPROCESS_FALLBACK_LIMIT = WEBHOOK_RATE_LIMIT * 3 shields against this somewhat, but the primary Redis path still has the race.

Recommended fix

Make INCR-then-check atomic — INCR first (it's atomic), then compare:

key = f"webhook_calls:{token}"
try:
    pipe = r.pipeline()
    pipe.incr(key)
    pipe.expire(key, WEBHOOK_RATE_WINDOW)
    new_count, _ = pipe.execute()
    if int(new_count) > WEBHOOK_RATE_LIMIT:
        ttl = r.ttl(key)
        raise HTTPException(
            status_code=status.HTTP_429_TOO_MANY_REQUESTS,
            detail=f"Webhook rate limit exceeded. Try again in {ttl} seconds.",
            headers={"Retry-After": str(max(ttl, 1))},
        )

Trade-off: the pipeline always increments, so blocked requests still count toward the window. Acceptable — it slightly extends the cool-down for a token that's already over the limit. If preferred, use an EVAL'd Lua script for true CAS semantics (the ACL already grants +@scripting).

Acceptance criteria

• Race fixed (INCR-first or Lua CAS)
• Concurrent regression test added that fires WEBHOOK_RATE_LIMIT + 5 simultaneous requests via asyncio.gather and asserts at most WEBHOOK_RATE_LIMIT succeed
• Existing sequential test in tests/integration/test_webhook_rate_limit.py still passes

Out of scope

• The fail-open posture when Redis is unreachable (intentional, documented)
• The in-process fallback bucket (independent path, has its own thread-lock)

Related

• PR fix(security): lock down Redis — auth + ACL + network split (#589) #643 — Redis lock-down (parent context)
• Issue SEC: Redis has no requirepass — agent containers have unauthenticated full read/write access to platform task queue (AISEC-C1) #589 — Redis auth (parent issue)

[vybe]

Shipped in v0.6.0 (released via #998).