[BUG] Sliding-window limiter bypass via non-unique Redis sorted-set members

[YashIIT0909]

Describe the bug
There is a rate-limiter bypass vulnerability in the SlidingWindowRateLimiter. Attackers can easily bypass the configured rate limits by sending a burst of concurrent requests in the exact same millisecond.

This happens due to two intertwined bugs:

1. TOCTOU Race Condition: The hit method uses Promise.all to concurrently read (getRangeFromSortedSet) and write (addToSortedSet) to Redis. When hundreds of requests arrive at the exact same millisecond, the Node.js event loop executes the "read" operation for all of them before any "write" operations finish. Redis answers 0 to all initial requests, allowing the entire burst to bypass the limit.
2. Redis Key Collision (Data Loss): The rate limiter uses the format timestamp:step (e.g., 1775990916270:1) for Redis sorted set members. Because Redis sorted sets require unique members, when hundreds of requests arrive on the exact same millisecond, they overwrite each other. The server effectively "loses" the count of the vast majority of the burst requests.

To Reproduce
Steps to reproduce the behavior:

1. In resources/default-settings.yaml (or .nostr/settings.yaml), set a strict rate limit for testing (e.g., message.rateLimits to 5 per minute) and ensure ipWhitelist is empty.
2. Start the nostream server using Docker (./scripts/start).
3. Create a Node.js script using the ws package that opens 1000 connections and sends a dummy REQ payload simultaneously using Promise.all().
4. Run the script and count the ["NOTICE", "rate limited"] responses versus accepted responses.
5. See error: The server accepts significantly more requests than the configured limit (e.g., accepting ~60 requests instead of 5).

Expected behavior
The server should strictly enforce the rate limit and prevent race conditions. If the limit is 5 per minute, exactly 5 requests should be accepted, and the remaining 995 should accurately receive a "rate-limited" rejection, even if they arrive in the exact same millisecond. No data loss should occur in Redis.

System (please complete the following information):

• OS: Linux
• Platform: Docker
• Version: Latest (master)

Logs
Proof of Concept Script Output:

Connecting to ws://localhost:8008...
1000 connections established. Preparing concurrent payload...
Firing parallel requests in the exact same millisecond...

--- Test Results ---
Total Requests Sent: 1000
Accepted: 59
Rate Limited (Rejected): 941
--------------------
⚠️ BYPASS SUCCESSFUL: More requests were accepted than the configured rate limit (5) allowed.

Additional Context
Proposed Fix:
Two changes are required in sliding-window-rate-limiter.ts :

1. Use a UUID to guarantee uniqueness for every hit in Redis to prevent data loss:

import { randomUUID } from 'crypto'
const member = `${now}:${step}:${randomUUID()}`

2. Break apart the Promise.all in the hit method and await the Redis operations sequentially so the read accurately reflects the prior write :

await this.cache.addToSortedSet(key, member, now.toString())
// ...
const count = await this.cache.getRangeFromSortedSet(key, windowStart)

[YashIIT0909]

@cameri @phoenix-server should i submit a PR for this ?

[saniddhyaDubey]

I believe your approach will solve the data loss, but still the race condition will exists under high concurrency. Because both the redis command, read and write, are different operations. The actual solution to this is wrapping the entire per-request hit function into a lua script and call lua script with the data. Lua scripts guarantees atomicity and we won't be having the overhead to maintain UUIDs and anything at all!

[YashIIT0909]

I agree with your point. The UUID + sequencing fix addresses the collision/data-loss issue and significantly reduces the bypass window, but it still relies on separate Redis operations and is not fully atomic under extreme concurrency.

A Lua-script-based check-and-insert is the more correct long-term solution because it makes the decision and state update happen atomically in Redis. That would remove TOCTOU risk entirely and avoid extra member-uniqueness handling in application code.