Feat/rate limiting - #1012
Conversation
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds database-backed rate-limit grants, shared API route matching, configurable route limits, rate-limit middleware, grant-based tiers, admin grant management, and automated coverage for quota and grant behavior. ChangesAPI rate limiting
Documentation formatting
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟠 High · up to This PR adds API rate limiting, but callers can currently bypass the limits by changing client-supplied identity headers, so protections for sign-in, registration, and password-reset endpoints may never activate. Expiry handling can also apply limits at the wrong time, and the reset response header is malformed; these concrete correctness and security issues should be fixed before merge. Sequence Diagram(s)sequenceDiagram
participant Client
participant apiRateLimitMiddleware
participant findApiRoute
participant getActiveRateLimitGrants
participant tosApiMiddleware
Client->>apiRateLimitMiddleware: Send API request
apiRateLimitMiddleware->>findApiRoute: Match method and pathname
findApiRoute-->>apiRateLimitMiddleware: Return route configuration
apiRateLimitMiddleware->>getActiveRateLimitGrants: Load active grants
getActiveRateLimitGrants-->>apiRateLimitMiddleware: Return matching grant tiers
alt Limit exceeded
apiRateLimitMiddleware-->>Client: Return JSON 429
else Limit available
apiRateLimitMiddleware->>tosApiMiddleware: Continue middleware chain
tosApiMiddleware-->>Client: Return response with rate-limit headers
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
app/db/drizzle/0048_tired_blue_shield.sql (1)
1-11: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider a unique constraint on
(kind, value).The table permits several enabled rows with the same
kindandvaluebut differenttier.resolveRateLimitTierinapp/middleware/rate-limit-api.server.tsusesgrants.find(...), so the applied tier then depends on row order. A partial unique index prevents duplicate active grants.♻️ Proposed migration addition
CREATE TABLE "rate_limit_grant" ( ... ); +--> statement-breakpoint +CREATE UNIQUE INDEX "rate_limit_grant_kind_value_active_uq" + ON "rate_limit_grant" ("kind", "value") WHERE "enabled";app/middleware/rate-limit-api.server.ts (2)
196-222: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the tier lookup against unknown values.
RATE_LIMIT_TIERS[matchedCredentialGrant.tier]andRATE_LIMIT_TIERS[matchedUserGrant.tier]assume the storedtieris one of the known keys. Thetiercolumn is plaintextwith no CHECK constraint, so a row written outside the typed helpers producesundefinedand aTypeErrorhere. The middleware runs on every API request, so one bad row breaks the whole API.Fall back to
DEFAULT_RATE_LIMIT_TIERwhen the tier is unknown.♻️ Proposed guard
+function toResolvedTier(tier: RateLimitTier): ResolvedRateLimitTier { + const config = RATE_LIMIT_TIERS[tier] + if (!config) return DEFAULT_RATE_LIMIT_TIER + return { name: tier, multiplier: config.multiplier } +}
72-76: 🩺 Stability & Availability | 🔵 TrivialPer-process buckets multiply the effective limit.
bucketsis a module-levelMap. Each server process or worker keeps its own counters. With N instances behind a load balancer the effective limit is N times the configured value, and a deploy resets all counters. ThecachedGrantscache has the same property, so a grant change takes up toGRANT_CACHE_TTL_MSper instance to apply.If the deployment runs more than one instance, move the counters to a shared store such as Redis, or document the per-instance behavior and set the configured limits accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 22a81cdf-9c08-4874-811f-1d0a5d1ef2a8
📒 Files selected for processing (15)
README.mdapp/db/drizzle/0048_tired_blue_shield.sqlapp/db/drizzle/meta/0048_snapshot.jsonapp/db/drizzle/meta/_journal.jsonapp/db/models/rate-limit-grant.server.tsapp/db/schema/index.tsapp/db/schema/rate-limit-grant.tsapp/lib/api-route-matching.tsapp/lib/api-routes.tsapp/middleware/rate-limit-api.server.tsapp/middleware/tos-api.server.tsapp/routes/admin._index.tsxapp/routes/admin.rate-limits.tsxapp/routes/api.tstests/lib/api-rate-limit.spec.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| function getRequesterCredential(request: Request) { | ||
| const credential = | ||
| request.headers.get('authorization') ?? | ||
| request.headers.get('x-osem-device-api-key') ?? | ||
| request.headers.get('x-service-key') | ||
|
|
||
| if (!credential) return null | ||
|
|
||
| return { | ||
| raw: credential, | ||
| hash: createHash('sha256').update(credential).digest('hex'), | ||
| } | ||
| } | ||
|
|
||
| function getRequesterKey(request: Request) { | ||
| const jwtPayload = getVerifiedJwtPayload(request) | ||
| if (jwtPayload?.sub) return `user:${normalizeEmail(String(jwtPayload.sub))}` | ||
|
|
||
| const credential = getRequesterCredential(request) | ||
| if (credential) return `credential:${credential.hash}` | ||
|
|
||
| return `ip:${getClientAddress(request)}` | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Build rate-limit bucket keys only from trusted identity data.
getRequesterKey currently accepts caller-controlled authorization and forwarding headers, so a client can rotate either value to obtain a new bucket on every request. This bypasses the limits for sign-in, registration, and password-reset endpoints.
Verify bearer tokens and database-backed device or service keys before using them. Otherwise use the server-derived client address, accepting forwarded headers only from a trusted proxy with configured hop handling. Update the tests so verified credentials share a bucket while unverified authorization and rotating forwarding headers fall back to the same client-address bucket.
📍 Affects 2 files
app/middleware/rate-limit-api.server.ts#L127-L149(this comment)tests/lib/api-rate-limit.spec.ts#L62-L89
| function rateLimitHeaders(result: RateLimitResult) { | ||
| return { | ||
| 'RateLimit-Limit': String(result.limit.maxRequests), | ||
| 'RateLimit-Remaining': String(result.remaining), | ||
| 'RateLimit-Reset': String(Math.ceil(result.resetAt / 1000)), | ||
| 'RateLimit-Policy': `${result.limit.maxRequests};w=${Math.ceil( | ||
| result.limit.windowMs / 1000, | ||
| )};tier="${result.tier}"`, | ||
| 'Retry-After': String(result.retryAfterSeconds), | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
IETF draft RateLimit header fields RateLimit-Reset delta seconds or timestamp
💡 Result:
The IETF standardization efforts for RateLimit header fields specify that the RateLimit-Reset header must use a relative delta-seconds format, not an absolute timestamp [1][2][3]. Key details regarding this specification include: Format: The RateLimit-Reset field is defined as a non-negative integer representing the number of seconds remaining until the quota resets [1][3]. It is explicitly designed to be compatible with the delay-seconds notation used in the Retry-After header [4][3][5]. Rationale: Standardizing on delta-seconds (a relative time) rather than a timestamp (an absolute time, such as a Unix epoch) is a deliberate choice intended to [1][3][6]: - Avoid dependency on clock synchronization between the client and the server [1][3][7]. - Ensure resilience against clock skew and manual clock adjustments [1][3][7]. - Mitigate the "thundering herd" problem, which can occur when many clients attempt to reconnect simultaneously at the exact moment specified by a shared, absolute timestamp [1][3][7]. Context: While legacy, non-standardized implementations (often using X-RateLimit-Reset) frequently used Unix timestamps, the standardized RateLimit-Reset header mandated by IETF drafts moves away from this practice to improve interoperability and reliability [6][8]. Implementers are advised that the server may dynamically adjust the RateLimit-Reset value between requests, such as during periods of resource saturation or when employing sliding window policies [1][3][9].
Citations:
- 1: https://datatracker.ietf.org/doc/html/draft-ietf-httpapi-ratelimit-headers-10
- 2: https://datatracker.ietf.org/doc/draft-ietf-httpapi-ratelimit-headers/04/
- 3: https://www.ietf.org/archive/id/draft-ietf-httpapi-ratelimit-headers-06.html
- 4: https://datatracker.ietf.org/doc/draft-ietf-httpapi-ratelimit-headers/00/
- 5: https://www.ietf.org/archive/id/draft-polli-ratelimit-headers-02.html
- 6: https://http.dev/ratelimit-reset
- 7: https://datatracker.ietf.org/doc/html/draft-polli-ratelimit-headers-00
- 8: https://dev.to/joseph_anady_214bacedf939/http-rate-control-headers-canonical-reference-419h
- 9: https://datatracker.ietf.org/doc/html/draft-ietf-httpapi-ratelimit-headers-02
🏁 Script executed:
#!/bin/bash
set -eu
file="$(fd -t f 'rate-limit-api\.server\.ts$' . | head -n 1)"
printf '%s\n' "FILE=$file"
sed -n '1,180p' "$file"
sed -n '270,325p' "$file"
printf '%s\n' '--- related symbols ---'
rg -n --glob '*.ts' 'type RateLimitResult|interface RateLimitResult|resetAt|retryAfterSeconds|RateLimit-Reset' .Repository: openSenseMap/frontend
Length of output: 7569
🏁 Script executed:
#!/bin/bash
set -eu
file="./app/middleware/rate-limit-api.server.ts"
sed -n '220,315p' "$file"
printf '%s\n' '--- rate-limit tests ---'
sed -n '1,110p' tests/lib/api-rate-limit.spec.ts
printf '%s\n' '--- semantic probe ---'
python3 - <<'PY'
import math
now_ms = 1_700_000_000_000
window_ms = 60_000
reset_at = now_ms + window_ms
retry_after_seconds = math.ceil((reset_at - now_ms) / 1000)
absolute_header = math.ceil(reset_at / 1000)
print({
"resetAt_ms": reset_at,
"Retry-After": retry_after_seconds,
"RateLimit-Reset_current": absolute_header,
"RateLimit-Reset_expected_delta": retry_after_seconds,
})
PYRepository: openSenseMap/frontend
Length of output: 5640
Emit remaining seconds in RateLimit-Reset. resetAt is an absolute Unix timestamp in milliseconds, but RateLimit-Reset requires delta-seconds. Use String(result.retryAfterSeconds) to match Retry-After.
| const date = new Date(value) | ||
| if (Number.isNaN(date.getTime())) { | ||
| return { ok: false, error: 'Enter a valid expiry date.' } | ||
| } | ||
|
|
||
| return { ok: true, value: date } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the repository-declared Node.js version, if present.
for file in .nvmrc .node-version .tool-versions package.json; do
if [ -f "$file" ]; then
echo "===== $file ====="
sed -n '1,120p' "$file"
fi
done
# A datetime-local value has no offset. Its resulting instant changes with server TZ.
for zone in UTC America/New_York; do
TZ="$zone" node -e '
const value = "2026-08-24T12:00";
console.log(process.env.TZ, new Date(value).toISOString());
'
doneRepository: openSenseMap/frontend
Length of output: 4606
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== outline ====="
ast-grep outline app/routes/admin.rate-limits.tsx
echo "===== parser and datetime-local references ====="
rg -n -C 4 'parseExpiresAt|datetime-local|expiresAt|expiry|expiration' app/routes/admin.rate-limits.tsx
echo "===== relevant source ranges ====="
sed -n '400,490p' app/routes/admin.rate-limits.tsx
sed -n '490,620p' app/routes/admin.rate-limits.tsxRepository: openSenseMap/frontend
Length of output: 6204
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== route action and loader ====="
sed -n '1,120p' app/routes/admin.rate-limits.tsx
echo "===== grant persistence references ====="
rg -n -C 5 'create|insert|update|expiresAt|parseGrantForm|rate.limit|grant' app/routes/admin.rate-limits.tsxRepository: openSenseMap/frontend
Length of output: 13145
Preserve the timezone when parsing an expiry value.
datetime-local submits no offset. new Date(value) therefore uses the server timezone, which can persist the wrong expiry instant when the administrator and server use different timezones.
Define the field as UTC in both the UI and parser, or submit an offset-aware ISO timestamp.
1 participant
Type of Change
Implementation
Checklist
devbranchAdditional Information