Skip to content

security(email): enforce send throttling atomically across API workers #1379

Description

@seonghobae

Buyer/security defect

Protected develop@bc98789521d21271e84789888413c182aa111b4d enforces /api/emails/send with the module-level _email_send_attempts_by_scope dictionary plus a process-local Lock in backend/api/emails.py. The code itself labels this as a process-local throttle. A multi-worker or multi-replica deployment therefore gives each worker an independent 10-per-60-second bucket, allowing the configured send restriction to be bypassed by routing requests across workers.

This was independently surfaced by the exact-head Strix run for #1300: run 31885077422, job/check 95013095581, on #1300 head e774ae5fe3a3089de81373b2c32ee8c210090769. Strix classified it MEDIUM and correctly attributed the finding to an unchanged file, so it is a repository-level defect rather than a #1300 tools-mutation regression. The same Strix job later timed out/fell back, so its failed result must not be relabeled as passing evidence.

Required bounded fix

Replace process-local enforcement with one server-authoritative, shared, atomic rate-limit decision for each authorized (organization_id, user_id) send scope. The implementation must:

  • remain effective across multiple Python workers and horizontally scaled API replicas;
  • make the check-and-record decision atomic so concurrent requests cannot all observe the same pre-limit count;
  • preserve tenant/context isolation and never merge counters across different authorized scopes;
  • fail closed for the send operation when the shared enforcement state required to make a trustworthy decision is unavailable; do not silently fall back to process-local state;
  • keep the throttle independent of model/LLM judgment;
  • avoid storing message body, recipients, subject, credentials, or other unnecessary PII in limiter state;
  • expose an auditable, non-sensitive reason/event for allowed/blocked/unavailable decisions;
  • define retention/expiry so stale attempts do not accumulate indefinitely;
  • preserve the existing user-facing 429 Email send rate limit exceeded contract when the limit is actually exhausted, while distinguishing infrastructure/enforcement unavailability from quota exhaustion.

Prefer an existing durable/shared Naruon infrastructure primitive when it can provide an atomic contract without weakening database normalization or introducing a duplicate authority. Do not introduce an unpinned external dependency merely to clear Strix.

TDD acceptance evidence

Publish RED before production change with realistic tests that prove the current implementation can be bypassed under at least two independent worker-state instances or equivalent shared-store simulation. GREEN evidence must cover:

  1. 10 successful attempts in one 60-second scope and deterministic rejection of the 11th;
  2. concurrent attempts cannot oversubscribe the bucket;
  3. two simulated workers/relicas share the same effective bucket;
  4. distinct users and organizations remain isolated;
  5. expiry/window rollover is deterministic under an injected clock rather than sleep-based timing;
  6. shared-state failure blocks sending rather than reverting to local memory;
  7. no email content/recipient/subject is persisted in limiter state;
  8. focused plus full exact-head backend/security/coverage evidence is terminal-success.

Before implementation, refetch active email-writing PRs. At issue creation #1376 is an actively updated Cursor-owned email lane and #1355 also changes backend/api/emails.py; do not race either writer or bury this security change inside an unrelated diff. A dedicated current-base branch is appropriate only after active overlap is clear or the change can be isolated in a non-conflicting shared service with explicit integration ownership.

Merge boundary

A fix is not mergeable merely because the Strix finding disappears. The unchanged exact candidate head must satisfy every live repository + inherited organization rule, all required CI/security/coverage/dependency/provenance contexts, zero actionable review threads, and a qualifying independent non-author approval after the last push. No self-approval, gate weakening, status manufacturing, or ruleset bypass.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area: authAuthentication, authorization, identity, or tenant isolationarea: ci-cdCI, GitHub Actions, checks, release, or supply chainarea: dependenciesDependency or lockfile maintenancearea: securitySecurity boundary, hardening, or vulnerability preventionpriority: highHigh-priority or P1 workstatus: blockedBlocked by conflict, dependency, or required prerequisitetype: securitySecurity vulnerability or security-specific remediation

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions