security: sign Telegram/ManyChat/Twilio webhooks, throttle by IP, hard-stop the budget guard - #2
Closed
Consultoriaintegralrf wants to merge 1 commit into
Conversation
…d-stop the budget guard A code audit found that Telegram, Twilio, and ManyChat webhooks accepted any POST with no authenticity check — unlike Meta/WhatsApp, which already verify X-Hub-Signature-256. Anyone who found the Worker URL could forge messages on those channels, including impersonating the owner via OWNER_TELEGRAM_CHAT_ID (not a secret). There was also no rate limiting anywhere, and the monthly AI budget guard only downgraded the model tier — it never stopped spend or alerted the owner. - Telegram: verify X-Telegram-Bot-Api-Secret-Token against a new TELEGRAM_WEBHOOK_SECRET (set via setWebhook's secret_token). - ManyChat: verify a new X-Manychat-Secret header (ManyChat doesn't sign natively) against MANYCHAT_WEBHOOK_SECRET. - Twilio: verify X-Twilio-Signature using the existing TWILIO_AUTH_TOKEN — no new secret needed. - All three fail closed, mirroring verifyMetaSignature's existing pattern. - New IP throttle (src/rate-limit.ts, D1-backed, no new binding) on the three webhooks and /kb/reindex, ahead of any signature check. - Budget guard (src/budget.ts) now pauses the bot (same bot_paused switch as the dashboard) and notifies the owner once spend hits 1.5x the monthly budget, instead of only ever downgrading to the cheap tier. Breaking for existing deployments: Telegram and ManyChat webhooks will 403 until TELEGRAM_WEBHOOK_SECRET / MANYCHAT_WEBHOOK_SECRET are configured (see wrangler.toml comments), and `pnpm db:apply:remote` needs a re-run for the new rate_limits table. Twilio needs no config change. pnpm typecheck clean; pnpm test: 469/469 passing (32 new). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe change adds monthly AI budget hard stops, fail-closed webhook authentication for Telegram, ManyChat, and Twilio, and D1-backed IP rate limiting for webhooks and knowledge-base reindex requests. ChangesMonthly budget enforcement
Webhook authentication and rate limiting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WebhookClient
participant Worker
participant D1
participant SecretVerifier
participant AgentRoute
WebhookClient->>Worker: POST webhook request
Worker->>D1: Check bucket and client IP
D1-->>Worker: Allow or HTTP 429
Worker->>SecretVerifier: Validate provider credential
SecretVerifier-->>Worker: Valid or HTTP 403
Worker->>AgentRoute: Forward accepted webhook
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/agent.ts`:
- Around line 284-289: Update the guard.paused branch in the agent request flow
to return immediately after logging the monthly budget hard-stop, preventing
execution from reaching streamText() or sendReply(). Preserve the existing tier
assignment and behavior when the guard is not paused.
In `@src/budget.ts`:
- Around line 94-106: Update the botPaused handling in the budget guard to
atomically claim the pause state using a conditional insert/update operation
that indicates whether this invocation changed SETTING_KEYS.botPaused. Remove
the separate settings.get()/settings.set() sequence, and call notifyOwner() only
when the current invocation wins the atomic claim; preserve the existing guard
return for already-paused cases.
In `@src/rate-limit.ts`:
- Around line 37-49: Update the rate-limit persistence flow around the
INSERT/SELECT operations to remove expired rows using a bounded, preferably
scheduled or batched cleanup rather than retaining reset keys indefinitely. Add
an index on rate_limits.window_start and ensure cleanup targets only expired
windows while preserving active rate-limit rows and existing counting behavior.
In `@wrangler.toml`:
- Around line 73-75: Update the rate-limiting documentation in wrangler.toml to
explicitly require running pnpm db:apply:remote for existing deployments before
using guarded routes, ensuring the rate_limits D1 migration is applied before
requests reach authentication.
- Around line 76-79: Update the rate-limit guidance comments in wrangler.toml to
reference the Workers rate-limit API binding as [[ratelimits]] instead of
[[rules]]. Clarify that account-level WAF rate-limiting rules require an
Enterprise plan, or direct non-Enterprise users to zone-level rules.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 02b708d7-182e-4662-9db4-97c16115a768
📒 Files selected for processing (16)
src/agent.tssrc/budget.tssrc/channels/manychat.tssrc/channels/telegram.tssrc/channels/twilio.tssrc/db/schema.sqlsrc/env.tssrc/index.tssrc/rate-limit.tstest/budget.test.tstest/channels/manychat.test.tstest/channels/telegram.test.tstest/channels/twilio.test.tstest/index.test.tstest/rate-limit.test.tswrangler.toml
Comment on lines
+284
to
289
| if (guard.paused) { | ||
| console.error( | ||
| `[SupportAgent] monthly budget hard-stop ($${spent.toFixed(2)} >= 1.5x $${cfg.monthlyBudgetUsd}) — bot paused, owner notified`, | ||
| ); | ||
| } | ||
| tier = guard.tier; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stop processing after a hard stop.
When guard.paused is true, this only logs and then proceeds to streamText() and sendReply(). Return immediately after logging so the triggering request does not generate another paid response after the budget pause.
Proposed fix
if (guard.paused) {
console.error(
`[SupportAgent] monthly budget hard-stop ($${spent.toFixed(2)} >= 1.5x $${cfg.monthlyBudgetUsd}) — bot paused, owner notified`,
);
+ return;
}
tier = guard.tier;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (guard.paused) { | |
| console.error( | |
| `[SupportAgent] monthly budget hard-stop ($${spent.toFixed(2)} >= 1.5x $${cfg.monthlyBudgetUsd}) — bot paused, owner notified`, | |
| ); | |
| } | |
| tier = guard.tier; | |
| if (guard.paused) { | |
| console.error( | |
| `[SupportAgent] monthly budget hard-stop ($${spent.toFixed(2)} >= 1.5x $${cfg.monthlyBudgetUsd}) — bot paused, owner notified`, | |
| ); | |
| return; | |
| } | |
| tier = guard.tier; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/agent.ts` around lines 284 - 289, Update the guard.paused branch in the
agent request flow to return immediately after logging the monthly budget
hard-stop, preventing execution from reaching streamText() or sendReply().
Preserve the existing tier assignment and behavior when the guard is not paused.
Comment on lines
+94
to
+106
| const settings = new SettingsRepo(db); | ||
| if ((await settings.get(SETTING_KEYS.botPaused)) === "1") { | ||
| return { ...guard, paused: false }; // ya pausado — no volver a notificar | ||
| } | ||
|
|
||
| await settings.set(SETTING_KEYS.botPaused, "1"); | ||
| await notifyOwner(env, { | ||
| reason: "presupuesto de IA excedido", | ||
| summary: | ||
| `🚨 El bot se pausó solo: el gasto de IA de este mes ($${monthCostUsd.toFixed(2)}) ` + | ||
| `superó 1.5x tu presupuesto ($${budgetUsd}). Reactívalo desde el panel (Agente) cuando quieras.`, | ||
| ticketId: "budget-guard", | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the global pause claim atomic.
Two concurrent agents can both read an unpaused setting before either upsert completes, so both call notifyOwner(). Replace the separate get()/set() with a conditional insert/update that reports whether this invocation actually changed bot_paused, and notify only the winner.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/budget.ts` around lines 94 - 106, Update the botPaused handling in the
budget guard to atomically claim the pause state using a conditional
insert/update operation that indicates whether this invocation changed
SETTING_KEYS.botPaused. Remove the separate settings.get()/settings.set()
sequence, and call notifyOwner() only when the current invocation wins the
atomic claim; preserve the existing guard return for already-paused cases.
Comment on lines
+37
to
+49
| await db.run( | ||
| `INSERT INTO rate_limits (key, window_start, count) | ||
| VALUES (?, ?, 1) | ||
| ON CONFLICT(key) DO UPDATE SET | ||
| count = CASE WHEN rate_limits.window_start = excluded.window_start THEN rate_limits.count + 1 ELSE 1 END, | ||
| window_start = excluded.window_start`, | ||
| [key, windowStart], | ||
| ); | ||
|
|
||
| const row = await db.first<{ count: number }>( | ||
| `SELECT count FROM rate_limits WHERE key = ?`, | ||
| [key], | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Add bounded retention for expired rate-limit keys.
Rows are reset but never deleted. Requests from rotating IPs create permanent bucket:ip records, allowing unbounded D1 growth and eventual failures for all guarded routes. Add expiry cleanup (preferably scheduled/batched) and index window_start for that cleanup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/rate-limit.ts` around lines 37 - 49, Update the rate-limit persistence
flow around the INSERT/SELECT operations to remove expired rows using a bounded,
preferably scheduled or batched cleanup rather than retaining reset keys
indefinitely. Add an index on rate_limits.window_start and ensure cleanup
targets only expired windows while preserving active rate-limit rows and
existing counting behavior.
Comment on lines
+73
to
+75
| # Rate limiting: el Worker ya trae un throttle por IP de baja fricción | ||
| # (src/rate-limit.ts, D1-backed) en los webhooks y /kb/reindex. Para producción | ||
| # real (sobre todo si vas a revender/manejar varios clientes), agrega además una |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Document the required D1 migration.
Existing deployments will query rate_limits immediately; without pnpm db:apply:remote, guarded routes fail before authentication.
Proposed fix
# Rate limiting: el Worker ya trae un throttle por IP de baja fricción
# (src/rate-limit.ts, D1-backed) en los webhooks y /kb/reindex. Para producción
+# Antes de desplegar esta versión sobre una D1 existente, ejecuta:
+# pnpm db:apply:remote
+# Esto crea la tabla `rate_limits` requerida por esos guards.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Rate limiting: el Worker ya trae un throttle por IP de baja fricción | |
| # (src/rate-limit.ts, D1-backed) en los webhooks y /kb/reindex. Para producción | |
| # real (sobre todo si vas a revender/manejar varios clientes), agrega además una | |
| # Rate limiting: el Worker ya trae un throttle por IP de baja fricción | |
| # (src/rate-limit.ts, D1-backed) en los webhooks y /kb/reindex. Para producción | |
| # Antes de desplegar esta versión sobre una D1 existente, ejecuta: | |
| # pnpm db:apply:remote | |
| # Esto crea la tabla `rate_limits` requerida por esos guards. | |
| # real (sobre todo si vas a revender/manejar varios clientes), agrega además una |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wrangler.toml` around lines 73 - 75, Update the rate-limiting documentation
in wrangler.toml to explicitly require running pnpm db:apply:remote for existing
deployments before using guarded routes, ensuring the rate_limits D1 migration
is applied before requests reach authentication.
Comment on lines
+76
to
+79
| # Cloudflare Rate Limiting Rule a nivel de cuenta — es la capa principal, corre | ||
| # antes de que el request le cueste nada al Worker: | ||
| # dashboard → tu dominio → Security → WAF → Rate limiting rules | ||
| # (o `wrangler` con un [[rules]] tipo ratelimit una vez el binding sea GA) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the rate-limit guidance in wrangler.toml.
[[rules]] is for module bundler rules, not bindings. For the Workers rate-limit API, the required Wrangler binding is [[ratelimits]]; also qualify account-level WAF rate-limiting rules as enterprise-only, or direct users to zone-level rules.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wrangler.toml` around lines 76 - 79, Update the rate-limit guidance comments
in wrangler.toml to reference the Workers rate-limit API binding as
[[ratelimits]] instead of [[rules]]. Clarify that account-level WAF
rate-limiting rules require an Enterprise plan, or direct non-Enterprise users
to zone-level rules.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Qué cambia
Cierra tres huecos de seguridad reales encontrados en una auditoría del código: los webhooks de Telegram, Twilio y ManyChat no verificaban el origen del request (a diferencia de Meta/WhatsApp, que sí firman); no había rate limiting en ningún lado; y el guard de presupuesto mensual de IA nunca frenaba el gasto, solo degradaba el modelo.
X-Telegram-Bot-Api-Secret-Tokencontra un nuevo secretTELEGRAM_WEBHOOK_SECRET(se configura al registrar el webhook consecret_token).X-Manychat-SecretcontraMANYCHAT_WEBHOOK_SECRET(ManyChat no firma nativamente, así que esto es un shared secret que se configura como header custom en el flow).X-Twilio-Signature(HMAC-SHA1 oficial de Twilio) usando elTWILIO_AUTH_TOKENque ya existía — sin secret nuevo.verifyMetaSignatureensrc/channels/meta.ts.src/rate-limit.ts, sobre el D1 existente, sin binding nuevo) en los tres webhooks y en/kb/reindex, antes de verificar firma/token.src/budget.ts): además de degradar a modelo barato, al llegar a 1.5x el presupuesto mensual pausa el bot entero (mismo switchbot_pauseddel panel) y notifica al dueño por su canal de handoff.Por qué
Sin verificación de origen, cualquiera que encontrara la URL del Worker podía mandar un Update/mensaje falso a Telegram/Twilio/ManyChat — incluso suplantar al dueño en Telegram, ya que
OWNER_TELEGRAM_CHAT_IDno es secreto. Sin rate limiting, esto además permitía abuso de costo/DoS. El budget guard solo degradando el modelo no evita que una ráfaga rebase el presupuesto de todos modos.Cómo lo probaste
pnpm testpasa (469/469, 32 tests nuevos)pnpm typechecklimpioChecklist
member/Nota para quien revise — breaking, intencional
Los bots ya desplegados con Telegram o ManyChat dejarán de responder por esos canales hasta que se configure
TELEGRAM_WEBHOOK_SECRET/MANYCHAT_WEBHOOK_SECRET(documentado en los comentarios dewrangler.toml). Twilio no requiere cambios de config. También hace faltapnpm db:apply:remotepara la tabla nuevarate_limits.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests