Partition pre-authentication rate limits by API-key public ID - #15
Partition pre-authentication rate limits by API-key public ID#15Lucenx9 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bc4a9f035
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const parsed = | ||
| match === null ? undefined : parseApiKeyToken(match[1] ?? ""); | ||
| if (parsed !== undefined) { | ||
| const decision = limiter.consume(parsed.publicId); |
There was a problem hiding this comment.
Preserve an aggregate cap on authentication attempts
An unauthenticated client can generate a fresh syntactically valid 16-character public ID for every request, so every call receives a full new bucket and proceeds through bearerAuthentication to the database-backed findActiveByPublicId query. Consequently, authenticationRequestsPerMinute no longer limits aggregate pre-authentication database work at all; retain an aggregate or non-attacker-controlled limit in addition to the per-ID bucket.
Useful? React with 👍 / 👎.
| if (this.bucket.tokens >= 1) { | ||
| this.bucket.tokens -= 1; | ||
| bucket.updatedAt = now; | ||
| this.buckets.set(publicId, bucket); |
There was a problem hiding this comment.
Bound or evict attacker-created rate-limit buckets
When clients rotate valid-format public IDs, each unauthenticated request permanently inserts another entry into this process-wide map. Entries are never removed after their tokens refill or IDs become inactive, allowing remote traffic to grow the heap without bound until the process restarts; add expiry/eviction or a hard capacity rather than retaining every observed ID indefinitely.
Useful? React with 👍 / 👎.
Motivation
Description
TokenBucketinAuthenticationRateLimiterwith aMap<string, TokenBucket>keyed by the parsed API-keypublicId, and changeconsume()toconsume(publicId: string).rateLimitAuthenticationto parse the bearer token first and calllimiter.consume(parsed.publicId)only when parsing succeeds (so buckets are partitioned per public id).Testing
npm run typecheckandnpm run lint, both succeeded.npm run format:checkfor the modified files, which succeeded.npm run test:unit -- --run tests/unit/authentication/api-key.test.ts, and the targeted unit tests passed.tests/security/authentication/bearer-authentication.test.ts) but the suite that requires a PostgreSQL Testcontainer could not start because no container runtime was available in the environment; as a result the security suite failed to execute in CI but the added regression test logic is present and exercised by unit coverage where applicable.Codex Task