-
Notifications
You must be signed in to change notification settings - Fork 1
fix(email): enforce shared send throttling #1417
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
seonghobae
wants to merge
33
commits into
codex/stacked-pr-workflow-triggers
Choose a base branch
from
fix/email-shared-send-rate-limit
base: codex/stacked-pr-workflow-triggers
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
33 commits
Select commit
Hold shift + click to select a range
3d08f5f
fix(email): enforce shared send throttling
seonghobae 754cea8
Harden Cloud Agent env secrets and email_records.is_read migration (#…
cursor[bot] ae254c1
chore: sync send rate limit branch with develop
seonghobae 69fb72d
Merge remote-tracking branch 'origin/develop' into fix/pr1417-current
seonghobae 7125926
Merge branch 'develop' into fix/email-shared-send-rate-limit
seonghobae 46f4b92
test(postgres): seed email read state explicitly
seonghobae 6ae6425
Merge branch 'develop' into fix/email-shared-send-rate-limit
seonghobae 2d4ec7c
Merge branch 'develop' into fix/email-shared-send-rate-limit
seonghobae 48ab340
fix(email): enforce rolling send limit transaction
seonghobae 395a8a8
fix(email): bound rate-limit denial audits
seonghobae bb7085c
fix(email): pin limiter transaction isolation
seonghobae ad3ba4e
Merge remote-tracking branch 'origin/develop' into codex/pr1417-current
seonghobae 5a22a26
fix(email): isolate shared send throttle
seonghobae e22fa78
fix(email): isolate send limits by workspace
seonghobae 6039e1e
test(db): require security audit Alembic revision
seonghobae 5e4e3b9
test(email): pin limiter database-clock and smoke isolation
seonghobae a30ebfa
fix(db): formalize security audit events migration
seonghobae 8235c42
fix(email): use PostgreSQL clock for send quota
seonghobae c9b8c84
test(email): require bounded limiter reservation history
seonghobae 7f2091c
fix(email): prune expired limiter reservations
seonghobae c7b693c
test(email): reproduce bounded-pool send starvation
seonghobae d1d69d2
fix(email): release request read before limiter session
seonghobae a9f334a
test(email): preserve send-route session doubles
seonghobae 2e791fb
fix(email): integrate migrated PostgreSQL quota evidence
seonghobae 1666f76
fix(email): inherit nested migration isolation prerequisite
seonghobae 56025b1
fix(email): close pending SMTP sockets on cancellation
seonghobae dc8b53d
Merge canonical CI signal startup repair into shared send
seonghobae b7011d2
merge: inherit CI process registration repair
seonghobae 9e4ddde
merge: inherit multiline governance repair
seonghobae cc2c4cb
fix(email): retain complete CI governance parent repair
seonghobae 5dea509
merge: inherit complete CI owner publisher verification repair
seonghobae ec3e361
merge(email): inherit validated clean-summary gate repair
seonghobae 489bcbe
merge(email): inherit validated dependency prerequisite
seonghobae File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| """Formalize durable security audit events in the Alembic upgrade path. | ||
|
|
||
| Revision ID: 0018_security_audit_events | ||
| Revises: 0017_merge_newsdom_carddav_heads | ||
| Create Date: 2026-09-05 00:00:00.000000 | ||
|
|
||
| Older installations can already contain ``security_audit_events`` because the | ||
| legacy bootstrap path created it outside Alembic. This revision is therefore | ||
| idempotent: it creates the table when absent and reconciles the model-owned | ||
| indexes when the table already exists. Downgrade intentionally preserves the | ||
| durable audit table and its evidence rather than deleting security history that | ||
| may predate this revision. | ||
| """ | ||
|
|
||
| from alembic import op | ||
| import sqlalchemy as sa | ||
|
|
||
| revision = "0018_security_audit_events" | ||
| down_revision = "0017_merge_newsdom_carddav_heads" | ||
| branch_labels = None | ||
| depends_on = None | ||
|
|
||
| _TABLE = "security_audit_events" | ||
| _INDEXES: tuple[tuple[str, list[str]], ...] = ( | ||
| ("ix_security_audit_events_actor_user_id", ["actor_user_id"]), | ||
| ("ix_security_audit_events_actor_role", ["actor_role"]), | ||
| ("ix_security_audit_events_organization_id", ["organization_id"]), | ||
| ("ix_security_audit_events_workspace_id", ["workspace_id"]), | ||
| ("ix_security_audit_events_event_action", ["event_action"]), | ||
| ("ix_security_audit_events_resource_type", ["resource_type"]), | ||
| ("ix_security_audit_events_resource_uid", ["resource_uid"]), | ||
| ("ix_security_audit_events_observed_at", ["observed_at"]), | ||
| ( | ||
| "ix_security_audit_events_scope_time", | ||
| ["organization_id", "workspace_id", "observed_at"], | ||
| ), | ||
| ( | ||
| "ix_security_audit_events_actor_scope", | ||
| ["actor_user_id", "organization_id", "workspace_id"], | ||
| ), | ||
| ) | ||
|
|
||
|
|
||
| def upgrade() -> None: | ||
| """Create the audit schema missing from Alembic-managed upgrades.""" | ||
| connection = op.get_bind() | ||
| inspector = sa.inspect(connection) | ||
|
|
||
| if not inspector.has_table(_TABLE): | ||
| op.create_table( | ||
| _TABLE, | ||
| sa.Column("event_uid", sa.String(), nullable=False), | ||
| sa.Column("actor_user_id", sa.String(), nullable=False), | ||
| sa.Column("actor_role", sa.String(), nullable=False), | ||
| sa.Column("organization_id", sa.String(), nullable=True), | ||
| sa.Column("workspace_id", sa.String(), nullable=False), | ||
| sa.Column("event_action", sa.String(), nullable=False), | ||
| sa.Column("resource_type", sa.String(), nullable=False), | ||
| sa.Column("resource_uid", sa.String(), nullable=True), | ||
| sa.Column("evidence_source", sa.String(), nullable=False), | ||
| sa.Column("detail_text", sa.Text(), nullable=True), | ||
| sa.Column( | ||
| "observed_at", | ||
| sa.DateTime(timezone=True), | ||
| nullable=False, | ||
| server_default=sa.text("CURRENT_TIMESTAMP"), | ||
| ), | ||
| sa.PrimaryKeyConstraint("event_uid"), | ||
| ) | ||
|
|
||
| for index_name, column_names in _INDEXES: | ||
| op.create_index( | ||
| index_name, | ||
| _TABLE, | ||
| column_names, | ||
| if_not_exists=True, | ||
| ) | ||
|
|
||
|
|
||
| def downgrade() -> None: | ||
| """Preserve durable security evidence created before or after this revision.""" | ||
| # This revision reconciles a table that may predate Alembic ownership. | ||
| # Dropping it on downgrade could destroy security evidence owned by the | ||
| # earlier bootstrap path, so schema rollback deliberately leaves it intact. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| """Join shared-send audit and workspace registry migration histories. | ||
|
|
||
| Both parents own durable data. This revision only reconciles the graph so the | ||
| normal ``upgrade head`` path applies both prerequisites without deleting either. | ||
| """ | ||
|
|
||
| revision = "0020_merge_send_registry" | ||
| down_revision = ("0018_security_audit_events", "0019_email_read_state_repair") | ||
| branch_labels = None | ||
| depends_on = None | ||
|
|
||
|
|
||
| def upgrade() -> None: | ||
| """Join already-applied parent revisions without changing their data.""" | ||
|
|
||
|
|
||
| def downgrade() -> None: | ||
| """Split revision bookkeeping without deleting parent-owned data.""" |
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,186 @@ | ||
| """Shared, fail-closed email send throttling.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import datetime | ||
| import hashlib | ||
| import logging | ||
| from dataclasses import dataclass | ||
| from typing import TYPE_CHECKING, Literal | ||
|
|
||
| from sqlalchemy import bindparam, delete, func, select | ||
| from sqlalchemy.ext.asyncio import AsyncSession | ||
|
|
||
| from db.models import SecurityAuditEvent | ||
| from db.session import AsyncSessionLocal | ||
|
|
||
| if TYPE_CHECKING: | ||
| from api.auth import AuthContext | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| SEND_RATE_LIMIT_MAX_ATTEMPTS = 10 | ||
| SEND_RATE_LIMIT_WINDOW_SECONDS = 60 | ||
| SEND_RATE_LIMIT_NAMESPACE = "naruon-email-send-rate-limit" | ||
|
|
||
|
|
||
| class EmailSendRateLimitUnavailable(RuntimeError): | ||
| """The shared rate-limit state cannot provide a trustworthy decision.""" | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class EmailSendRateLimitDecision: | ||
| """A non-sensitive rate-limit decision returned to the send endpoint.""" | ||
|
|
||
| allowed: bool | ||
| reason: Literal["allowed", "quota_exhausted"] | ||
|
|
||
|
|
||
| def rate_limit_scope_hash( | ||
| user_id: str, organization_id: str | None, workspace_id: str | ||
| ) -> str: | ||
| """Return a non-reversible identifier for one authorized send scope.""" | ||
| organization_scope = organization_id or "<personal>" | ||
| value = ( | ||
| f"{SEND_RATE_LIMIT_NAMESPACE}\0{organization_scope}\0{workspace_id}\0{user_id}" | ||
| ) | ||
| return hashlib.sha256(value.encode("utf-8")).hexdigest() | ||
|
|
||
|
|
||
| def _lock_key(scope_hash: str) -> int: | ||
| return int.from_bytes(bytes.fromhex(scope_hash[:16]), byteorder="big", signed=True) | ||
|
|
||
|
|
||
| def _session_uses_postgresql(session: AsyncSession) -> bool: | ||
| try: | ||
| bind = session.get_bind() | ||
| except Exception: | ||
| return False | ||
| return getattr(getattr(bind, "dialect", None), "name", None) == "postgresql" | ||
|
|
||
|
|
||
| def _audit_event( | ||
| auth_context: AuthContext, | ||
| *, | ||
| scope_hash: str, | ||
| decision: EmailSendRateLimitDecision, | ||
| observed_at: datetime.datetime, | ||
| ) -> SecurityAuditEvent: | ||
| return SecurityAuditEvent( | ||
| actor_user_id=auth_context.user_id, | ||
| actor_role=auth_context.role, | ||
| organization_id=auth_context.organization_id, | ||
| workspace_id=auth_context.workspace_id, | ||
| event_action=f"email_send_rate_limit.{decision.reason}", | ||
| resource_type="email_send_rate_limit", | ||
| resource_uid=f"email_send_scope:{scope_hash}", | ||
| evidence_source="services.email_send_rate_limiter", | ||
| observed_at=observed_at, | ||
| detail_text=( | ||
| f"decision={decision.reason};" | ||
| f"window_seconds={SEND_RATE_LIMIT_WINDOW_SECONDS};" | ||
| f"max_attempts={SEND_RATE_LIMIT_MAX_ATTEMPTS}" | ||
| ), | ||
| ) | ||
|
|
||
|
|
||
| async def enforce_send_email_rate_limit( | ||
| auth_context: AuthContext, | ||
| *, | ||
| now: datetime.datetime | None = None, | ||
| ) -> EmailSendRateLimitDecision: | ||
| """Atomically reserve one send attempt in a rolling PostgreSQL window. | ||
|
|
||
| A limiter-owned transaction prevents committing unrelated request work. | ||
| PostgreSQL advisory locking serializes the count-and-record decision across | ||
| workers. Production timestamps come from PostgreSQL after the scope lock so | ||
| worker clock skew and lock wait do not weaken the real-time quota. Expired | ||
| allowed rows are transient reservation state and are pruned under that same | ||
| lock; durable quota-denial audit evidence is retained. ``now`` remains an | ||
| explicit deterministic test seam. | ||
| """ | ||
| observed_at = now | ||
| scope_hash = rate_limit_scope_hash( | ||
| auth_context.user_id, | ||
| auth_context.organization_id, | ||
| auth_context.workspace_id, | ||
| ) | ||
| scope_uid = f"email_send_scope:{scope_hash}" | ||
| async with AsyncSessionLocal() as session: | ||
| if not _session_uses_postgresql(session): | ||
| raise EmailSendRateLimitUnavailable | ||
| try: | ||
| await session.connection( | ||
| execution_options={"isolation_level": "READ COMMITTED"} | ||
| ) | ||
| await session.execute( | ||
| select(func.pg_advisory_xact_lock(bindparam("lock_key"))), | ||
| {"lock_key": _lock_key(scope_hash)}, | ||
| ) | ||
| if observed_at is None: | ||
| database_clock = await session.execute(select(func.clock_timestamp())) | ||
| observed_at = database_clock.scalar_one() | ||
| window_started_at = observed_at - datetime.timedelta( | ||
| seconds=SEND_RATE_LIMIT_WINDOW_SECONDS | ||
| ) | ||
| await session.execute( | ||
| delete(SecurityAuditEvent).where( | ||
| SecurityAuditEvent.resource_uid == scope_uid, | ||
| SecurityAuditEvent.event_action | ||
| == "email_send_rate_limit.allowed", | ||
| SecurityAuditEvent.observed_at <= window_started_at, | ||
| ) | ||
| ) | ||
| result = await session.execute( | ||
| select(func.count()) | ||
| .select_from(SecurityAuditEvent) | ||
|
seonghobae marked this conversation as resolved.
|
||
| .where( | ||
| SecurityAuditEvent.resource_uid == scope_uid, | ||
| SecurityAuditEvent.event_action | ||
| == "email_send_rate_limit.allowed", | ||
| SecurityAuditEvent.observed_at > window_started_at, | ||
| ) | ||
| ) | ||
| allowed = result.scalar_one() < SEND_RATE_LIMIT_MAX_ATTEMPTS | ||
| decision = EmailSendRateLimitDecision( | ||
| allowed=allowed, | ||
| reason="allowed" if allowed else "quota_exhausted", | ||
| ) | ||
| record_decision = allowed | ||
|
seonghobae marked this conversation as resolved.
|
||
| if not allowed: | ||
| denied_result = await session.execute( | ||
| select(func.count()) | ||
| .select_from(SecurityAuditEvent) | ||
| .where( | ||
| SecurityAuditEvent.resource_uid == scope_uid, | ||
| SecurityAuditEvent.event_action | ||
| == "email_send_rate_limit.quota_exhausted", | ||
| SecurityAuditEvent.observed_at > window_started_at, | ||
| ) | ||
| ) | ||
| record_decision = denied_result.scalar_one() == 0 | ||
| if record_decision: | ||
| session.add( | ||
| _audit_event( | ||
| auth_context, | ||
| scope_hash=scope_hash, | ||
| decision=decision, | ||
| observed_at=observed_at, | ||
| ) | ||
| ) | ||
| await session.commit() | ||
| return decision | ||
| except Exception as exc: | ||
| try: | ||
| await session.rollback() | ||
| except Exception as rollback_exc: | ||
| logger.warning( | ||
| "Email send rate limiter rollback failed; error_type=%s", | ||
| type(rollback_exc).__name__, | ||
| ) | ||
| logger.warning( | ||
| "Email send rate limiter decision unavailable; " | ||
| "event_action=email_send_rate_limit.unavailable error_type=%s", | ||
| type(exc).__name__, | ||
| ) | ||
| raise EmailSendRateLimitUnavailable from exc | ||
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.