Simple Rate Limiting - #4
Conversation
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
youtubekit-server | 6a7ed36 | Feb 09 2026, 07:31 PM |
📝 WalkthroughWalkthroughIntroduces a Cloudflare Durable Objects-based rate limiter supporting daily and weekly quota windows with automatic window rolling and retry-after calculations. Adds comprehensive documentation for Durable Objects best practices, Wrangler CLI usage, and testing patterns. Updates type definitions to include workflow retention semantics and expanded telemetry attributes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Worker
participant AppRateLimiter as App Rate Limiter<br/>(Durable Object)
participant Storage
Client->>Worker: WebSocket upgrade request<br/>(with App-ID header)
Worker->>Worker: Normalize & extract App ID
Worker->>AppRateLimiter: admit(cost)
AppRateLimiter->>Storage: Fetch limiter_state<br/>(for this App ID)
AppRateLimiter->>AppRateLimiter: Roll expired windows<br/>(daily & weekly)
AppRateLimiter->>AppRateLimiter: Check daily limit<br/>Check weekly limit
alt Quota Exceeded
AppRateLimiter->>AppRateLimiter: Calculate retryAfterSeconds<br/>(from window resets)
AppRateLimiter-->>Worker: RateLimitDecision<br/>(allowed=false)
Worker-->>Client: 429 Too Many Requests<br/>(with rate-limit headers)
else Within Limits
AppRateLimiter->>Storage: Upsert limiter_state<br/>(increment counts)
AppRateLimiter-->>Worker: RateLimitDecision<br/>(allowed=true)
Worker->>Worker: Proceed with<br/>WebSocket upgrade
Worker-->>Client: 101 Switching Protocols
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 7
🤖 Fix all issues with AI agents
In @.agents/skills/durable-objects/references/testing.md:
- Around line 9-11: Update the vitest version recommendation in the testing doc
so it matches the project's package.json (change `vitest@~3.2.0` to
`vitest@~3.0.7`) or replace the pinned version with a generic reference like
`vitest` to avoid mismatch; ensure the command line in the doc still includes
`@cloudflare/vitest-pool-workers` and that the example uses the same versioning
approach as the project's package.json.
In @.agents/skills/durable-objects/references/workers.md:
- Around line 344-345: Remove the incorrect example command `wrangler d1 execute
DB --command "SELECT * FROM _cf_DO"` and replace it with accurate guidance:
Durable Objects are not listed via a D1 SQL command — tell readers to view
Durable Objects in the Cloudflare dashboard or inspect bindings in wrangler.toml
(or point to the Cloudflare API for programmatic queries) and provide a short
example/reference to the dashboard or API instead of the bogus `_cf_DO` command.
In @.agents/skills/wrangler/SKILL.md:
- Around line 196-201: The fenced code block showing the `.dev.vars` example is
missing a language specifier; update that snippet (the triple-backtick block
containing "API_KEY=local-dev-key" and
"DATABASE_URL=postgres://localhost:5432/dev") to include a language such as ini
(e.g., change ``` to ```ini) so markdownlint MD040 is satisfied and the block is
properly highlighted.
In `@README.md`:
- Line 42: Update the README sentence to make it explicit that rate limiting
applies only to WebSocket upgrade requests on /v1, not all HTTP requests:
mention the header/identifier `X-AppID-v1`, the `/v1` path, and that a Durable
Object with anchored windows enforces limits specifically for WebSocket
connection upgrades; revise the line "Requests are limited per app ID
(`X-AppID-v1`) using a Durable Object with anchored windows:" to something like
"WebSocket upgrade requests to /v1 are rate-limited per app ID (`X-AppID-v1`)
via a Durable Object with anchored windows."
In `@src/durable-objects/app-rate-limiter.ts`:
- Around line 72-87: The deny response computes remainingDaily/remainingWeekly
from nextState.dayCount/weekCount which can show positive remaining even when
the attempted cost caused the denial; update the deny branch to compute
remaining values from policy.dailyLimit - nextDaily and policy.weeklyLimit -
nextWeekly (clamped to >=0) so the response reflects the attempted cost (use
nextDaily, nextWeekly, policy.dailyLimit, policy.weeklyLimit and keep
retryAfterSeconds via calculateRetryAfterSeconds).
In `@src/index.ts`:
- Around line 29-48: The code logs raw App IDs (PII) when reading the header via
APP_ID_HEADER and normalizing with normalizeAppID; change logging in the
websocket upgrade handling and the rate-limit warning in the block around
checkRateLimit(appID, env) to log a redacted or hashed form instead of the raw
appID: compute a deterministic redaction/hash (e.g., SHA256 or stable masking)
of the value returned by normalizeAppID and use that redactedId in console.log
and in the JSON passed to console.warn (including appID -> redactedAppID) so the
rest of the rate-limit fields (limitDaily, remainingDaily, etc.) remain
unchanged. Ensure the redaction function is applied wherever appID is logged or
included in telemetry in this request path.
- Around line 35-37: The code currently uses the client-provided X-AppID-v1
header directly in checkRateLimit(appID, env), allowing clients to bypass
limits; fix by validating/authenticating the appID before rate
limiting—implement an authentication step (e.g., verify JWT/API key/mTLS) in the
request handler (the code that reads X-AppID-v1) and reject requests with
missing/invalid appIDs, or map the authenticated client to a server-side
principal/tenant ID and pass that canonical tenant identifier to checkRateLimit
instead of the raw header; update any Durable Object lookup logic to use the
server-side principal/tenant ID so new client-supplied appIDs cannot spawn
unbounded Durable Objects.
🧹 Nitpick comments (1)
wrangler.toml (1)
17-18: Consider whether default rate limits should differ between dev and production.Both the root (dev) and production environments use the same limits (
5000daily /20000weekly). During local development, a lower limit might be more useful for testing rate-limit behavior, while production might warrant different tuning. This is optional — just calling it out for consideration.Also applies to: 35-36
| Create `.dev.vars` for local development secrets: | ||
|
|
||
| ``` | ||
| API_KEY=local-dev-key | ||
| DATABASE_URL=postgres://localhost:5432/dev | ||
| ``` |
There was a problem hiding this comment.
Add a language specifier to the fenced code block.
The .dev.vars example block is missing a language identifier, flagged by markdownlint (MD040). Use ```ini or ```bash or ```text to specify the format.
Proposed fix
-```
+```ini
API_KEY=local-dev-key
DATABASE_URL=postgres://localhost:5432/dev</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.20.0)</summary>
[warning] 198-198: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @.agents/skills/wrangler/SKILL.md around lines 196 - 201, The fenced code
block showing the .dev.vars example is missing a language specifier; update
that snippet (the triple-backtick block containing "API_KEY=local-dev-key" and
"DATABASE_URL=postgres://localhost:5432/dev") to include a language such as ini
(e.g., change toini) so markdownlint MD040 is satisfied and the block is
properly highlighted.
</details>
<!-- fingerprinting:phantom:poseidon:phoenix -->
<!-- This is an auto-generated reply by CodeRabbit -->
There was a problem hiding this comment.
Just agent skills. No code or user facing docs
There was a problem hiding this comment.
@alexeichhorn, understood! Since this is internal agent skills documentation, I'll leave it as-is. Thanks for clarifying!
|
|
||
| ## Rate Limiting | ||
|
|
||
| Requests are limited per app ID (`X-AppID-v1`) using a Durable Object with anchored windows: |
There was a problem hiding this comment.
Clarify that rate limiting applies to WebSocket upgrade requests, not all requests.
The current wording "Requests are limited per app ID" could be interpreted as all HTTP requests being rate-limited. Per the implementation, only WebSocket connection upgrades on /v1 are subject to rate limiting. Consider making this more specific to avoid confusion.
🤖 Prompt for AI Agents
In `@README.md` at line 42, Update the README sentence to make it explicit that
rate limiting applies only to WebSocket upgrade requests on /v1, not all HTTP
requests: mention the header/identifier `X-AppID-v1`, the `/v1` path, and that a
Durable Object with anchored windows enforces limits specifically for WebSocket
connection upgrades; revise the line "Requests are limited per app ID
(`X-AppID-v1`) using a Durable Object with anchored windows:" to something like
"WebSocket upgrade requests to /v1 are rate-limited per app ID (`X-AppID-v1`)
via a Durable Object with anchored windows."
There was a problem hiding this comment.
There are no other requests....
There was a problem hiding this comment.
@alexeichhorn You're absolutely right! Since this server exclusively handles WebSocket connections for YouTube stream extraction, the current wording is perfectly clear. My suggestion to add extra specificity was unnecessary given that context. Thanks for clarifying! 👍
| // Log the App ID header for debugging purposes | ||
| const appID = request.headers.get('X-AppID-v1') ?? 'unknown'; | ||
| const appID = normalizeAppID(request.headers.get(APP_ID_HEADER)); | ||
| console.log(`App ID: ${appID}`); | ||
|
|
||
| // Only handle GET /v1?videoID=... as WebSocket upgrades | ||
| if (url.pathname === '/v1' && request.headers.get('Upgrade') === 'websocket') { | ||
| try { | ||
| const decision = await checkRateLimit(appID, env); | ||
| if (!decision.allowed) { | ||
| console.warn( | ||
| 'Rate limit rejected request', | ||
| JSON.stringify({ | ||
| appID, | ||
| path: url.pathname, | ||
| limitDay: decision.limitDaily, | ||
| remainingDay: decision.remainingDaily, | ||
| limitWeek: decision.limitWeekly, | ||
| remainingWeek: decision.remainingWeekly, | ||
| retryAfterSeconds: decision.retryAfterSeconds, | ||
| }) |
There was a problem hiding this comment.
Avoid logging raw App IDs (privacy/PII exposure).
These logs include the full App ID, which can be a tenant/user identifier. Please redact or hash before logging and use the redacted value in the warning payload.
🔒 Suggested redaction
- console.log(`App ID: ${appID}`);
+ const appIDForLog = appID === 'unknown' ? appID : `${appID.slice(0, 8)}…`;
+ console.log(`App ID: ${appIDForLog}`);
...
- appID,
+ appID: appIDForLog,🤖 Prompt for AI Agents
In `@src/index.ts` around lines 29 - 48, The code logs raw App IDs (PII) when
reading the header via APP_ID_HEADER and normalizing with normalizeAppID; change
logging in the websocket upgrade handling and the rate-limit warning in the
block around checkRateLimit(appID, env) to log a redacted or hashed form instead
of the raw appID: compute a deterministic redaction/hash (e.g., SHA256 or stable
masking) of the value returned by normalizeAppID and use that redactedId in
console.log and in the JSON passed to console.warn (including appID ->
redactedAppID) so the rest of the rate-limit fields (limitDaily, remainingDaily,
etc.) remain unchanged. Ensure the redaction function is applied wherever appID
is logged or included in telemetry in this request path.
There was a problem hiding this comment.
No issue. This is not very private
There was a problem hiding this comment.
@alexeichhorn, understood! I'll note that App IDs in this project aren't considered sensitive.
| try { | ||
| const decision = await checkRateLimit(appID, env); | ||
| if (!decision.allowed) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 "X-AppID-v1|APP_ID_HEADER|normalizeAppID|appID" -SRepository: alexeichhorn/YouTubeKit-Server
Length of output: 2545
🏁 Script executed:
#!/bin/bash
rg -n -C3 "authorization|auth|jwt|api key|apikey|client id|tenant" -SRepository: alexeichhorn/YouTubeKit-Server
Length of output: 7061
🏁 Script executed:
#!/bin/bash
# Get context around the specific lines mentioned (35-37 in src/index.ts)
head -60 src/index.ts | tail -35Repository: alexeichhorn/YouTubeKit-Server
Length of output: 1553
🏁 Script executed:
#!/bin/bash
# Find where appID is extracted/set
rg -n "appID\s*=" -A 2 -B 2Repository: alexeichhorn/YouTubeKit-Server
Length of output: 320
App ID is client-controlled with no authentication—clients can bypass rate limits and create unbounded Durable Objects.
The X-AppID-v1 header is extracted and used directly for rate limiting without validation or authentication. Clients can freely rotate appIDs to obtain fresh rate limit quotas, and each unique appID creates a separate Durable Object instance, risking resource exhaustion. Implement one of:
- Require authenticated client credentials (JWT, API key, mTLS)
- Map client requests to server-side principals/tenants before rate limiting
- Reject requests with missing or invalid appIDs
🤖 Prompt for AI Agents
In `@src/index.ts` around lines 35 - 37, The code currently uses the
client-provided X-AppID-v1 header directly in checkRateLimit(appID, env),
allowing clients to bypass limits; fix by validating/authenticating the appID
before rate limiting—implement an authentication step (e.g., verify JWT/API
key/mTLS) in the request handler (the code that reads X-AppID-v1) and reject
requests with missing/invalid appIDs, or map the authenticated client to a
server-side principal/tenant ID and pass that canonical tenant identifier to
checkRateLimit instead of the raw header; update any Durable Object lookup logic
to use the server-side principal/tenant ID so new client-supplied appIDs cannot
spawn unbounded Durable Objects.
There was a problem hiding this comment.
Will be fixed later...
There was a problem hiding this comment.
@alexeichhorn, understood. Since this is a critical security concern, would you like me to create a GitHub issue to track this authentication enhancement for a future PR? That way it won't be forgotten.
1 participant
Note
Medium Risk
Introduces a new Durable Object backed by SQLite and gates WebSocket upgrades on it; incorrect limits, binding/migration config, or DO failures could block legitimate traffic or return 429/503 unexpectedly.
Overview
Adds per-appID rate limiting for WebSocket connections (
/v1upgrades) using a new Durable Object (AppRateLimiter) with SQLite-backed anchored day/week windows and configurable limits.Worker requests now normalize
X-AppID-v1, call the DO to admit/deny, and return429withRetry-After+ rate limit headers when exceeded (or503if the limiter errors).wrangler.toml/worker-configuration.d.tsare updated with the DO binding, migration, andRATE_LIMIT_DAILY_REQUESTS/RATE_LIMIT_WEEKLY_REQUESTSvars; README documents the new behavior, andpackage.jsonadds atypecheckscript.Written by Cursor Bugbot for commit 6a7ed36. This will update automatically on new commits. Configure here.