Add bad-actor abuse controls: suspension, outbound-email pause, compute quotas, delivery insights - #911
Merged
Conversation
📝 WalkthroughWalkthroughThis change adds platform account suspension, automatic outbound-email abuse pauses, daily compute and egress quotas, admin moderation controls, email delivery analytics, supporting schema updates, tests, and documentation. ChangesAbuse controls
Compute quotas
Delivery insights
Documentation and migration ledger
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
kody-bot
marked this pull request as ready for review
July 24, 2026 16:22
Contributor
|
🔎 Preview deployed: https://kody-pr-911.kody-a99.workers.dev Worker: Mocks:
|
![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
🤖 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 `@docs/contributing/architecture/entitlements.md`:
- Around line 99-106: Update the outbound-fetch quota flow around getUserPlan
and executeGatewayFetch so a userId with missing or blank
FetchGatewayProps.email never resolves directly to the max plan. Resolve the
caller email from the stable userId before plan lookup, or reject the request
when it cannot be resolved, while preserving quota enforcement for contexts
without a userId.
In `@docs/use/email-primitives.md`:
- Around line 119-123: Update the delivery-events description in the email
primitives documentation to state that five or more bounced sends within a UTC
day trigger the automatic outbound-sending pause, while preserving the existing
spam-complaint trigger and pause behavior.
In `@packages/worker/src/app/admin-insights-data.ts`:
- Around line 147-157: The email delivery aggregation in the admin insights
query must not read cross-tenant events without an ownership boundary. Update
the query and surrounding insights flow to constrain results by the authorized
userId, including that value in any cache key, or explicitly route this metric
through an approved platform-wide aggregation boundary that does not expose
tenant-level outcomes directly.
In `@packages/worker/src/email/outbound-abuse.ts`:
- Around line 134-141: Remove the LIMIT 10 clause from the admin query used by
the outbound abuse notification flow so admins are not capped at ten recipients.
Preserve the existing admin filtering, ordering, and result mapping in the
admins lookup.
In `@packages/worker/src/mcp-auth.workers.test.ts`:
- Around line 165-170: Update the suspended-account query mock branch in
mcp-auth.workers.test.ts to validate the bound email/stableUserId against the
configured test user, matching the adjacent profile and verification branches,
and return null for non-matching values. Preserve returning options.suspendedAt
only for the expected bound identity so the mock detects missing or widened
query scoping in isAccountSuspended.
In `@packages/worker/src/mcp/fetch-gateway.node.test.ts`:
- Around line 24-40: Update the APP_DB D1 stub in the test environment so bind()
captures its arguments and run() validates the expected userId and relevant
entitlement upsert query contract before returning success. Ensure the stub
fails when userId is omitted or belongs to another account, while preserving
first() behavior.
In `@packages/worker/src/mcp/fetch-gateway.ts`:
- Around line 31-37: Update the gateway quota plan resolution around the email
field and its associated logic to work independently of optional email: derive
the account plan from userId, or ensure the verified caller email is propagated
by every gateway caller, including operation-request.ts authenticated calls.
Preserve the existing email-based behavior when available and ensure
authenticated users with email: null receive their account-specific quota
instead of the max fallback.
🪄 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: fc82be90-70dd-41fb-a723-c84c7ad7e415
📒 Files selected for processing (50)
docs/contributing/architecture/authentication.mddocs/contributing/architecture/entitlements.mddocs/contributing/security.mddocs/use/email-primitives.mdpackages/worker/client/routes/admin-insights.tsxpackages/worker/client/routes/admin-users.tsxpackages/worker/migrations/0090-user-abuse-controls.sqlpackages/worker/src/app/account-suspension.tspackages/worker/src/app/admin-insights-data.node.test.tspackages/worker/src/app/admin-insights-data.tspackages/worker/src/app/admin-user-usage-data.tspackages/worker/src/app/admin-users-data.tspackages/worker/src/app/authenticated-user.tspackages/worker/src/app/handlers/admin-users.node.test.tspackages/worker/src/app/handlers/admin-users.tspackages/worker/src/app/loader-data.tspackages/worker/src/app/request-auth-cache.tspackages/worker/src/app/session-info.tspackages/worker/src/community/community-flow-test-schema.tspackages/worker/src/db.tspackages/worker/src/email/delivery-queue.node.test.tspackages/worker/src/email/delivery-queue.tspackages/worker/src/email/inbound.tspackages/worker/src/email/inbound.workers.test.tspackages/worker/src/email/outbound-abuse.tspackages/worker/src/email/outbound-abuse.workers.test.tspackages/worker/src/email/outbound.tspackages/worker/src/email/outbound.workers.test.tspackages/worker/src/email/service.tspackages/worker/src/entitlements/plans.tspackages/worker/src/entitlements/service.tspackages/worker/src/entitlements/test-schema.tspackages/worker/src/execute-maintenance.node.test.tspackages/worker/src/execute-maintenance.tspackages/worker/src/mcp-auth.tspackages/worker/src/mcp-auth.workers.test.tspackages/worker/src/mcp/capabilities/admin/admin-capabilities.node.test.tspackages/worker/src/mcp/capabilities/admin/admin-shared.tspackages/worker/src/mcp/capabilities/admin/admin-user-usage.tspackages/worker/src/mcp/capabilities/openapi-provider/operation-request.tspackages/worker/src/mcp/fetch-gateway.node.test.tspackages/worker/src/mcp/fetch-gateway.tspackages/worker/src/mcp/fetch-gateway.workers.test.tspackages/worker/src/mcp/run-kody-registry.tspackages/worker/src/mcp/tools/execute.node.test.tspackages/worker/src/mcp/tools/execute.tspackages/worker/src/package-registry/test-schema.tspackages/worker/src/package-runtime/package-app.tstools/check-migrations.node.test.tstools/migration-ledger.json
Comment on lines
+147
to
+157
| db | ||
| .prepare( | ||
| // Provider ('cloudflare-email') events only: outbound | ||
| // delivery outcomes, excluding inbound routing rejections. | ||
| `SELECT substr(created_at, 1, 10) AS day, event_type, COUNT(*) AS n | ||
| FROM email_delivery_events | ||
| WHERE provider = 'cloudflare-email' AND created_at >= ? | ||
| GROUP BY day, event_type`, | ||
| ) | ||
| .bind(dayCutoff) | ||
| .all<EmailDeliveryDayRow>(), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a '0090-user-abuse-controls.sql' packages/worker/migrations
rg -n -i -C 4 'email_delivery_events|user_id|loadAdminInsightsData|admin/insights' \
packages/worker/migrations packages/worker/src packages/worker/client \
-g '*.sql' -g '*.ts' -g '*.tsx'Repository: kentcdodds/kody
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the admin-insights source and schema-related files without producing excessive output.
printf '--- relevant files ---\n'
fd -a 'admin-insights|0090-user-abuse|email_delivery|insights' packages/worker packages/worker/src packages/worker/client 2>/dev/null | sed -n '1,120p'
printf '\n--- admin-insights-data outline ---\n'
ast-grep outline packages/worker/src/app/admin-insights-data.ts --view compact 2>/dev/null | sed -n '1,220p' || rg -n "queryAdminInsights|loadAdminInsightsData|emailDeliveryByDay|email_delivery_events" packages/worker/src/app/admin-insights-data.ts -C 6
printf '\n--- targeted admin-insights-data snippets ---\n'
sed -n '1,220p' packages/worker/src/app/admin-insights-data.ts
printf '\n--- targeted schema migration snippets ---\n'
rg -n -C 5 "CREATE TABLE email_delivery_events|email_delivery_events" packages/worker/migrations -g '*.sql' | sed -n '1,220p'
printf '\n--- targeted TypeScript usage snippets ---\n'
rg -n -C 4 "emailDeliveryByDay|queryAdminInsights|loadAdminInsightsData|email_delivery_events|userId" packages/worker/src/app packages/worker/src/client packages/worker/client -g '*.ts' -g '*.tsx' | sed -n '1,260p'Repository: kentcdodds/kody
Length of output: 244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- relevant files ---'
fd -a 'admin-insights|0090-user-abuse|email_delivery|insights' packages/worker packages/worker/src packages/worker/client 2>/dev/null | sed -n '1,160p'
printf '%s\n' '--- admin-insights-data outline ---'
ast-grep outline packages/worker/src/app/admin-insights-data.ts --view compact 2>/dev/null | sed -n '1,220p' || true
printf '%s\n' '--- targeted admin-insights-data snippets ---'
sed -n '1,220p' packages/worker/src/app/admin-insights-data.ts
printf '%s\n' '--- targeted schema migration snippets ---'
rg -n -C 5 "CREATE TABLE email_delivery_events|email_delivery_events" packages/worker/migrations -g '*.sql' | sed -n '1,220p'
printf '%s\n' '--- targeted TypeScript usage snippets ---'
rg -n -C 4 "emailDeliveryByDay|queryAdminInsights|loadAdminInsightsData|email_delivery_events|userId" packages/worker/src/app packages/worker/src/client packages/worker/client -g '*.ts' -g '*.tsx' | sed -n '1,280p'Repository: kentcdodds/kody
Length of output: 40020
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- admin-insights-data rest ---'
sed -n '220,340p' packages/worker/src/app/admin-insights-data.ts
printf '%s\n' '--- admin-insights data test ---'
sed -n '1,240p' packages/worker/src/app/admin-insights-data.node.test.ts
printf '%s\n' '--- admin-insights handler ---'
sed -n '1,220p' packages/worker/src/app/handlers/admin-insights.ts
printf '%s\n' '--- loader-data admin types ---'
sed -n '1,220p' packages/worker/src/app/loader-data.ts
printf '%s\n' '--- routes admin-insights ---'
sed -n '1,260p' packages/worker/client/routes/admin-insights.tsx
printf '%s\n' '--- migrations full email_delivery_events table defs ---'
sed -n '176,195p' packages/worker/migrations/0030-email-primitives.sql
sed -n '40,96p' packages/worker/migrations/0061-email-delivery-lifecycle.sql
sed -n '215,311p' packages/worker/migrations/0031-unified-email-receipt.sqlRepository: kentcdodds/kody
Length of output: 28504
Keep insights aggregation scoped or explicitly platform-owned.
email_delivery_events.user_id is owned account data, so this aggregation should not read every tenant’s delivery outcomes without a userId predicate. Either scope this by an authorized userId/include it in the cache key for per-account insights, or define an explicit platform-wide aggregation boundary that does not expose cross-user tenant delivery events directly.
🤖 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 `@packages/worker/src/app/admin-insights-data.ts` around lines 147 - 157, The
email delivery aggregation in the admin insights query must not read
cross-tenant events without an ownership boundary. Update the query and
surrounding insights flow to constrain results by the authorized userId,
including that value in any cache key, or explicitly route this metric through
an approved platform-wide aggregation boundary that does not expose tenant-level
outcomes directly.
Source: Coding guidelines
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/worker/src/mcp-auth.workers.test.ts (1)
165-176: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle stable-user-only suspension lookups.
Production supports
WHERE stable_user_id = ?when no email is available, but this mock always treatsboundParams[0]as an email and reads the stable ID from index 1. Valid stable-user-only lookups therefore returnnull; customaccountByStableIdidentities are also compared against the defaults.Mirror the query shape and expected identity used by the adjacent verification mocks.
As per coding guidelines, every user-owned read path must remain scoped by
userId; this mock should validate the identity predicate actually used.Proposed fix
if (normalized.includes('select suspended_at from users')) { - const email = - typeof boundParams[0] === 'string' ? boundParams[0] : null - if (email !== defaultEmail) return null - if (normalized.includes('stable_user_id')) { - const stableUserId = - typeof boundParams[1] === 'string' ? boundParams[1] : null - if (stableUserId !== defaultStableUserId) return null - } + const expectedEmail = + options.accountByStableId?.email ?? defaultEmail + const expectedStableUserId = + options.accountByStableId?.stable_user_id ?? defaultStableUserId + const hasEmail = normalized.includes('email = ?') + const hasStableUserId = normalized.includes('stable_user_id = ?') + if (hasEmail) { + const email = + typeof boundParams[0] === 'string' ? boundParams[0] : null + if (email !== expectedEmail) return null + } + if (hasStableUserId) { + const index = hasEmail ? 1 : 0 + const stableUserId = + typeof boundParams[index] === 'string' ? boundParams[index] : null + if (stableUserId !== expectedStableUserId) return null + } return { suspended_at: options.suspendedAt ?? null } }🤖 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 `@packages/worker/src/mcp-auth.workers.test.ts` around lines 165 - 176, Update the mock branch handling isAccountSuspended queries in the query handler to distinguish email-based lookups from stable_user_id-only lookups: validate boundParams[0] against defaultEmail only when the query includes the email predicate, otherwise validate it against the expected stable user ID. Reuse the custom accountByStableId identity used by adjacent verification mocks so user-owned reads remain scoped to the predicate actually present.Source: Coding guidelines
🤖 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.
Duplicate comments:
In `@packages/worker/src/mcp-auth.workers.test.ts`:
- Around line 165-176: Update the mock branch handling isAccountSuspended
queries in the query handler to distinguish email-based lookups from
stable_user_id-only lookups: validate boundParams[0] against defaultEmail only
when the query includes the email predicate, otherwise validate it against the
expected stable user ID. Reuse the custom accountByStableId identity used by
adjacent verification mocks so user-owned reads remain scoped to the predicate
actually present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 17e9fad2-5ae9-4306-a78f-0bde2092b243
📒 Files selected for processing (8)
docs/contributing/architecture/entitlements.mddocs/use/email-primitives.mdpackages/worker/src/app/admin-insights-data.tspackages/worker/src/email/outbound-abuse.tspackages/worker/src/mcp-auth.workers.test.tspackages/worker/src/mcp/fetch-gateway.node.test.tspackages/worker/src/mcp/fetch-gateway.tspackages/worker/src/mcp/fetch-gateway.workers.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/worker/src/mcp/fetch-gateway.node.test.ts
- docs/use/email-primitives.md
- docs/contributing/architecture/entitlements.md
- packages/worker/src/mcp/fetch-gateway.workers.test.ts
- packages/worker/src/mcp/fetch-gateway.ts
- packages/worker/src/app/admin-insights-data.ts
- packages/worker/src/email/outbound-abuse.ts
… insights Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
…test scoping, doc threshold Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
…points Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
cursor
Bot
force-pushed
the
cursor/bad-actor-controls-b47e
branch
from
July 24, 2026 17:42
eb46df5 to
541bfff
Compare
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 541bfff. Configure here.
| email: callerContext.user.email, | ||
| resource: 'execute_calls_per_day', | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Meta execute bypasses daily quota
Medium Severity
This PR adds execute_calls_per_day enforcement only on the public MCP execute tool, but the meta execute capability still calls runModuleWithRegistry with no consumeDailyEntitlement. Package and in-sandbox runs that use that capability (including many nested runs from a single tool call) can exceed the documented daily execute limit.
Reviewed by Cursor Bugbot for commit 541bfff. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/check-migrations.node.test.ts (1)
375-384: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse an unused migration prefix for this fixture.
The test writes
0091-future.sql, butpackages/worker/migrations/andtools/migration-ledger.jsonalready have0091-user-abuse-controls.sql, so this lands a duplicate prefix before the assertion. Use an unused prefix such as0092-future.sqlconsistently, or another prefix greater than 91.🤖 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 `@tools/check-migrations.node.test.ts` around lines 375 - 384, Update the migration fixture in the test around the migrationPath, ledger.migrations entry, and related filename references to use an unused prefix greater than 91, such as 0092, consistently throughout the setup and assertions.
🤖 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 `@packages/worker/src/email/outbound-abuse.ts`:
- Around line 61-88: Update the complaint and bounce handling in the
delivery-status switch to persist and consult a resume watermark or per-event
application state, distinguishing events recorded before an account was resumed
from new events. Prevent historical replay events from pausing or notifying
again, while preserving crash recovery for a newly recorded complaint or bounce
that was persisted before pausing.
- Around line 158-164: Update the admins query in the outbound-abuse
notification flow to scope recipients through the paused user’s authorized
ownership/tenant relationship and bind the relevant userId parameter. Remove the
platform-wide admin lookup while preserving the existing email/username result
shape and notification behavior.
- Around line 193-196: Update the notification HTML construction in the outbound
abuse email flow to HTML-escape user-controlled content, including the username
embedded in text, before wrapping paragraphs in <p> elements. Apply the existing
escaping utility if available, and keep the surrounding document structure and
paragraph formatting unchanged.
---
Outside diff comments:
In `@tools/check-migrations.node.test.ts`:
- Around line 375-384: Update the migration fixture in the test around the
migrationPath, ledger.migrations entry, and related filename references to use
an unused prefix greater than 91, such as 0092, consistently throughout the
setup and assertions.
🪄 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: 32b48204-68bc-424b-af3e-df80a66f6d34
📒 Files selected for processing (50)
docs/contributing/architecture/authentication.mddocs/contributing/architecture/entitlements.mddocs/contributing/security.mddocs/use/email-primitives.mdpackages/worker/client/routes/admin-insights.tsxpackages/worker/client/routes/admin-users.tsxpackages/worker/migrations/0091-user-abuse-controls.sqlpackages/worker/src/app/account-suspension.tspackages/worker/src/app/admin-insights-data.node.test.tspackages/worker/src/app/admin-insights-data.tspackages/worker/src/app/admin-user-usage-data.tspackages/worker/src/app/admin-users-data.tspackages/worker/src/app/authenticated-user.tspackages/worker/src/app/handlers/admin-users.node.test.tspackages/worker/src/app/handlers/admin-users.tspackages/worker/src/app/loader-data.tspackages/worker/src/app/request-auth-cache.tspackages/worker/src/app/session-info.tspackages/worker/src/community/community-flow-test-schema.tspackages/worker/src/db.tspackages/worker/src/email/delivery-queue.node.test.tspackages/worker/src/email/delivery-queue.tspackages/worker/src/email/inbound.tspackages/worker/src/email/inbound.workers.test.tspackages/worker/src/email/outbound-abuse.tspackages/worker/src/email/outbound-abuse.workers.test.tspackages/worker/src/email/outbound.tspackages/worker/src/email/outbound.workers.test.tspackages/worker/src/email/service.tspackages/worker/src/entitlements/plans.tspackages/worker/src/entitlements/service.tspackages/worker/src/entitlements/test-schema.tspackages/worker/src/execute-maintenance.node.test.tspackages/worker/src/execute-maintenance.tspackages/worker/src/mcp-auth.tspackages/worker/src/mcp-auth.workers.test.tspackages/worker/src/mcp/capabilities/admin/admin-capabilities.node.test.tspackages/worker/src/mcp/capabilities/admin/admin-shared.tspackages/worker/src/mcp/capabilities/admin/admin-user-usage.tspackages/worker/src/mcp/capabilities/openapi-provider/operation-request.tspackages/worker/src/mcp/fetch-gateway.node.test.tspackages/worker/src/mcp/fetch-gateway.tspackages/worker/src/mcp/fetch-gateway.workers.test.tspackages/worker/src/mcp/run-kody-registry.tspackages/worker/src/mcp/tools/execute.node.test.tspackages/worker/src/mcp/tools/execute.tspackages/worker/src/package-registry/test-schema.tspackages/worker/src/package-runtime/package-app.tstools/check-migrations.node.test.tstools/migration-ledger.json
🚧 Files skipped from review as they are similar to previous changes (41)
- packages/worker/src/package-registry/test-schema.ts
- packages/worker/src/email/service.ts
- packages/worker/src/app/session-info.ts
- packages/worker/src/execute-maintenance.ts
- packages/worker/src/app/authenticated-user.ts
- packages/worker/src/execute-maintenance.node.test.ts
- packages/worker/src/entitlements/test-schema.ts
- packages/worker/src/mcp/capabilities/admin/admin-user-usage.ts
- packages/worker/src/db.ts
- packages/worker/src/mcp-auth.ts
- packages/worker/src/community/community-flow-test-schema.ts
- packages/worker/src/app/admin-user-usage-data.ts
- docs/contributing/architecture/authentication.md
- packages/worker/src/email/delivery-queue.node.test.ts
- packages/worker/src/email/outbound-abuse.workers.test.ts
- packages/worker/src/mcp/capabilities/openapi-provider/operation-request.ts
- packages/worker/src/email/outbound.ts
- packages/worker/src/email/outbound.workers.test.ts
- packages/worker/src/app/loader-data.ts
- packages/worker/client/routes/admin-insights.tsx
- docs/use/email-primitives.md
- packages/worker/src/email/inbound.workers.test.ts
- packages/worker/client/routes/admin-users.tsx
- packages/worker/src/mcp/fetch-gateway.node.test.ts
- docs/contributing/architecture/entitlements.md
- packages/worker/src/app/admin-insights-data.node.test.ts
- packages/worker/src/mcp/fetch-gateway.workers.test.ts
- packages/worker/src/mcp/tools/execute.ts
- packages/worker/src/email/inbound.ts
- packages/worker/src/app/request-auth-cache.ts
- packages/worker/src/mcp/tools/execute.node.test.ts
- packages/worker/src/package-runtime/package-app.ts
- packages/worker/src/app/handlers/admin-users.node.test.ts
- packages/worker/src/mcp/fetch-gateway.ts
- packages/worker/src/app/handlers/admin-users.ts
- packages/worker/src/mcp/capabilities/admin/admin-capabilities.node.test.ts
- packages/worker/src/email/delivery-queue.ts
- packages/worker/src/app/account-suspension.ts
- packages/worker/src/mcp-auth.workers.test.ts
- packages/worker/src/entitlements/plans.ts
- packages/worker/src/app/admin-users-data.ts
Comment on lines
+61
to
+88
| switch (input.deliveryStatus) { | ||
| case 'complained': { | ||
| // A freshly recorded complaint always pauses. A replayed or | ||
| // conflicting-duplicate complaint signal only pauses when a | ||
| // real persisted complaint event backs it (crash recovery | ||
| // between recording and pausing), never on its own. | ||
| if (!input.eventRecorded) { | ||
| const complaintsToday = await countProviderDeliveryEventsToday({ | ||
| db: input.env.APP_DB, | ||
| userId: input.userId, | ||
| eventType: 'complained', | ||
| now, | ||
| }) | ||
| if (complaintsToday < 1) return { paused: false } | ||
| } | ||
| break | ||
| } | ||
| case 'bounced': { | ||
| const bouncesToday = await countProviderDeliveryEventsToday({ | ||
| db: input.env.APP_DB, | ||
| userId: input.userId, | ||
| eventType: 'bounced', | ||
| now, | ||
| }) | ||
| if (bouncesToday < outboundEmailBouncePauseThresholdPerDay) { | ||
| return { paused: false } | ||
| } | ||
| break |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Prevent historical replay events from re-pausing a resumed account.
Clearing email_outbound_paused_at removes the only idempotency marker. A replayed complaint finds the existing complaint count, and a replayed bounce finds the existing threshold count, then both pause and notify again. Persist a resume watermark or per-event application state so pre-resume events cannot reapply the pause while retaining crash recovery.
🤖 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 `@packages/worker/src/email/outbound-abuse.ts` around lines 61 - 88, Update the
complaint and bounce handling in the delivery-status switch to persist and
consult a resume watermark or per-event application state, distinguishing events
recorded before an account was resumed from new events. Prevent historical
replay events from pausing or notifying again, while preserving crash recovery
for a newly recorded complaint or bounce that was persisted before pausing.
Comment on lines
+158
to
+164
| const admins = await input.env.APP_DB.prepare( | ||
| `SELECT u.email, u.username FROM users u | ||
| INNER JOIN user_roles ur ON ur.user_id = u.id | ||
| INNER JOIN roles r ON r.id = ur.role_id | ||
| WHERE r.name = 'admin' | ||
| ORDER BY u.id ASC`, | ||
| ).all<{ email: string; username: string }>() |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Scope the admin-recipient read to the owning user context.
This query reads every admin’s email and username without a userId-scoped ownership/tenant relationship, then shares the paused user’s event with all of them. Introduce and bind an authorized ownership relationship for the notifying scope rather than performing a platform-wide cross-user lookup. As per coding guidelines, every Kody multi-user read/write path must be scoped by userId; cross-user sharing is a bug.
🤖 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 `@packages/worker/src/email/outbound-abuse.ts` around lines 158 - 164, Update
the admins query in the outbound-abuse notification flow to scope recipients
through the paused user’s authorized ownership/tenant relationship and bind the
relevant userId parameter. Remove the platform-wide admin lookup while
preserving the existing email/username result shape and notification behavior.
Source: Coding guidelines
Comment on lines
+193
to
+196
| html: `<!doctype html><html lang="en"><body>${text | ||
| .split('\n\n') | ||
| .map((paragraph) => `<p>${paragraph}</p>`) | ||
| .join('')}</body></html>`, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Escape user-controlled values before generating notification HTML.
username is inserted into text and then rendered as HTML without escaping. A crafted username can inject markup into emails sent to administrators. HTML-escape dynamic values when constructing the HTML body.
Proposed fix
+const escapeHtml = (value: string) =>
+ value.replace(/[&<>"']/g, (character) =>
+ ({ '&': '&', '<': '<', '>': '>', '"': '"', "'": '&`#39`;' })[
+ character
+ ]!,
+ )
+
// ...
- html: `<!doctype html><html lang="en"><body>${text
+ html: `<!doctype html><html lang="en"><body>${escapeHtml(text)
.split('\n\n')
.map((paragraph) => `<p>${paragraph}</p>`)
.join('')}</body></html>`,📝 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
| html: `<!doctype html><html lang="en"><body>${text | |
| .split('\n\n') | |
| .map((paragraph) => `<p>${paragraph}</p>`) | |
| .join('')}</body></html>`, | |
| const escapeHtml = (value: string) => | |
| value.replace(/[&<>"']/g, (character) => | |
| ({ '&': '&', '<': '<', '>': '>', '"': '"', "'": '&`#39`;' })[ | |
| character | |
| ]!, | |
| ) | |
| // ... | |
| html: `<!doctype html><html lang="en"><body>${escapeHtml(text) | |
| .split('\n\n') | |
| .map((paragraph) => `<p>${paragraph}</p>`) | |
| .join('')}</body></html>`, |
🤖 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 `@packages/worker/src/email/outbound-abuse.ts` around lines 193 - 196, Update
the notification HTML construction in the outbound abuse email flow to
HTML-escape user-controlled content, including the username embedded in text,
before wrapping paragraphs in <p> elements. Apply the existing escaping utility
if available, and keep the surrounding document structure and paragraph
formatting unchanged.
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
3 participants
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.
Why
One bad actor can poison shared platform identity: every user sends mail from one platform domain through one Cloudflare Email Sending account, and every sandbox fetch leaves through the same Worker egress. Until now the platform had prevention (invite gate, verification, plan quotas) but no reactive controls — no account kill switch, no complaint-driven email pause, and no quota on execute/fetch volume despite metering both.
What
packages/worker/src/email/outbound-abuse.ts): the delivery queue evaluates provider delivery events; one spam complaint or 5+ bounces per UTC day setsusers.email_outbound_paused_at, blocks further sends, and notifies admin accounts via the transactional sender. Only persisted (deduped-by-provider_event_id) events drive the pause; idempotent (only transitions NULL); cleared by the auditedresume_email_outboundadmin action.users.suspended_at+packages/worker/src/app/account-suspension.ts): admin-set kill switch enforced fail-closed at browser session resolution, MCP bearer auth (403account_suspended), inbound email (boundedaccount-suspensionrejection), and outbound send. Auditedsuspend_user/unsuspend_useradmin actions with a Moderation panel on/admin/users(self-suspension blocked).execute_calls_per_dayandoutbound_fetches_per_dayentitlements, consumed atomically at the top of the MCPexecutetool andexecuteGatewayFetch(before bundling/secret expansion, so over-limit calls cost nothing). When a gateway caller carries no email (OpenAPI provider requests, package runtime), the gateway reverse-resolves the account from the stable userId so the caller's real plan binds. Both surfaces were already metered; this closes the metering → enforcement loop./admin/insightsgains a platform-wide per-day chart of provider delivery outcomes (delivered/deferred/bounced/failed/rejected/complained) so shared-domain reputation trouble is visible before providers act on it.Migration
0091-user-abuse-controls.sqladds both columns (renumbered from 0090 after rebasing on the webhook-endpoints work); ledger updated. Docs updated:security.md(new Abuse controls section),authentication.md,entitlements.md,email-primitives.md.Testing
npm run validatefully green on the rebased branch (format, lint, typecheck, 1274+ unit tests across 390 files, Playwright E2E, MCP E2E, primitives + migrations checks). New coverage: complaint/bounce pause thresholds, phantom-complaint and idempotency guards, paused/suspended send + inbound rejection, MCP 403 for suspended accounts, admin suspend/unsuspend/resume actions + audit events, fetch-gateway quota consumption/denial including the no-email reverse-resolution path, insights delivery-day builder.CodeRabbit and Bugbot review findings addressed: real-plan binding for email-less gateway calls, persisted-event-only abuse pause, userId-scoping assertions in test stubs, admin notification cap removed, exact bounce threshold documented.
System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@9c877620· Head:541bfff8Classification: extends — no new primitives; this PR adds columns, resources, and fail-closed gates to existing ones.
Primitives touched
d1-app-dbusers.suspended_at,users.email_outbound_paused_at(0091)emailemail-delivery-queueentitlementsexecute_calls_per_day,outbound_fetches_per_dayresourcesmcp-servermcp-oauthaccount_suspendedgate after email verificationcapabilities-executeemailinto fetch gateway propsapp-uirbacupdate:user:anyopenapi-bindingspackage-appsSystem map
Provider delivery events flow through the queue into the abuse monitor, which pauses senders in D1; suspension gates fan out from the users table to session, MCP, and email chokepoints.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Before / after
execute_calls_per_dayoutbound_fetches_per_dayInvariants
Per-user isolation unchanged: every new read/write is scoped by
userId/stable_user_id. Suspension gates fail closed at all chokepoints, mirroring the existingdeleting_at/ email-verification patterns. The admin insights delivery chart is a platform-wide admin-only aggregation of outcome counts with no user identifiers.Summary by CodeRabbit