Fix rate limiter counted every path against one bucket per IP

testbed/core/utils/errors.py

@@ -1,5 +1,6 @@
 import uuid
 from datetime import timezone, datetime
+from django.http import JsonResponse
 from rest_framework.response import Response
 
 """
@@ -21,7 +22,6 @@ class ErrorCodes:
     FORBIDDEN_ACCESS = "forbidden_access"
     UNAUTHORIZED = "unauthorized"
     ACTOR_MISMATCH = "actor_mismatch"
-
     # Rate Limiting Errors (429)
     RATE_LIMIT_EXCEEDED = "rate_limit_exceeded"
 
@@ -49,13 +49,62 @@ def generate_request_id():
     return str(uuid.uuid4())
 
 
+def build_error_payload(error_code, detail, request=None, hint=None, remediation=None):
+    """
+    Build the error body shared by every error response.
+
+    Two wrappers call this with the same body:
+        - build_error_response -> DRF Response for the view layer
+        - build_rate_limit_error -> JsonResponse for middleware
+
+    They can't share a wrapper because they run in different layers:
+        - Views need a DRF Response so activitypub_content can use
+            request.accepted_renderer to set application/activity+json.
+        - Middleware runs outside DRF, where a Response has no renderer
+            and raises on serialization, so it needs a plain JsonResponse.
+
+    Args:
+        error_code (str): Machine-readable error identifier from ErrorCodes
+        detail (str): Human-readable error description
+        request (HttpRequest, optional): Django request object for context
+        hint (str, optional): Additional context or explanation
+        remediation (str, optional): Actionable steps to fix the error
+
+    Returns:
+        dict: The error body, ready to be wrapped by either response class
+    """
+    error_data = {
+        "error_code": error_code,
+        "detail": detail,
+        "timestamp": datetime.now(timezone.utc).isoformat(),
+    }
+
+    # Add optional context fields if provided
+    if hint:
+        error_data["hint"] = hint
+
+    if remediation:
+        error_data["remediation"] = remediation
+
+    if request:
+        error_data["endpoint"] = request.path
+        error_data["method"] = request.method
+        # Generate request ID for this specific request
+        error_data["request_id"] = generate_request_id()
+
+    return error_data
+
+
 def build_error_response(error_code, detail, status_code, request=None, hint=None, remediation=None):
     """
-    Build standardized JSON error response.
+    Build standardized JSON error response for the DRF view layer.
 
     Creates consistent, developer-friendly error responses with comprehensive
     metadata for debugging, remediation, and support purposes.
 
+    The body is built by build_error_payload; this function only wraps it in the
+    response class the view layer needs.
+
     Args:
         error_code (str): Machine-readable error identifier from ErrorCodes
         detail (str): Human-readable error description
@@ -77,26 +126,16 @@ def build_error_response(error_code, detail, status_code, request=None, hint=Non
         ...     remediation="Request OAuth token with 'activitypub_account_portability' scope"
         ... )
     """
-    error_data = {
-        "error_code": error_code,
-        "detail": detail,
-        "timestamp": datetime.now(timezone.utc).isoformat(),
-    }
-
-    # Add optional context fields if provided
-    if hint:
-        error_data["hint"] = hint
-
-    if remediation:
-        error_data["remediation"] = remediation
-
-    if request:
-        error_data["endpoint"] = request.path
-        error_data["method"] = request.method
-        # Generate request ID for this specific request
-        error_data["request_id"] = generate_request_id()
-
-    return Response(error_data, status=status_code)
+    return Response(
+        build_error_payload(
+            error_code=error_code,
+            detail=detail,
+            request=request,
+            hint=hint,
+            remediation=remediation,
+        ),
+        status=status_code,
+    )
 
 
 def build_actor_not_found_error(actor_id, request=None):
@@ -163,27 +202,53 @@ def build_actor_mismatch_error(request=None):
     )
 
 
-def build_rate_limit_error(retry_after_seconds, request=None):
+def build_rate_limit_error(retry_after_seconds, request=None, limit=None, window=None):
     """
-    Build standardized 429 error for rate limiting with Retry-After header.
+    Build the complete 429 response for rate-limited requests (LOLA Section 6.7).
+
+    Returns a JsonResponse rather than a DRF Response, unlike every other builder in
+    this module, because rate limiting is enforced in middleware -- outside the DRF
+    view layer, where a Response has no renderer and raises on serialization.
+
+    - Retry-After: LOLA Section 6.7 describes rate limiting as a 429 "with a
+        Retry-After header", and destinations SHOULD honor it before resuming.
 
     Args:
-        retry_after_seconds (int): Seconds to wait before retrying
+        retry_after_seconds (int): Seconds until the current window resets
         request (HttpRequest, optional): Django request object for context
+        limit (int, optional): Configured request ceiling
+        window (int, optional): Window length in seconds
 
     Returns:
-        Response: 429 error response with rate limit context
+        JsonResponse: 429 response with Retry-After and federation-safe CORS headers
     """
-    response = build_error_response(
+    if limit is not None and window is not None:
+        hint = (
+            f"Too many requests: the limit is {limit} per {window} seconds. "
+            f"Please wait {retry_after_seconds} seconds before retrying"
+        )
+    else:
+        hint = f"Too many requests. Please wait {retry_after_seconds} seconds before retrying"
+
+    payload = build_error_payload(
         error_code=ErrorCodes.RATE_LIMIT_EXCEEDED,
         detail="Request rate limit exceeded",
-        status_code=429,
         request=request,
-        hint=f"Too many requests. Please wait {retry_after_seconds} seconds before retrying",
-        remediation="Implement exponential backoff or reduce request frequency"
+        hint=hint,
+        remediation="Honor the Retry-After header, then resume with exponential backoff",
     )
 
-    # Add standard Retry-After header for rate limiting
-    response['Retry-After'] = str(retry_after_seconds)
+    response = JsonResponse(payload, status=429)
+
+    # Standard rate-limiting header (RFC6585 / LOLA Section 6.7)
+    response["Retry-After"] = str(retry_after_seconds)
+
+    # Never let a 429 be replayed from a cache after the client has backed off
+    response["Cache-Control"] = "no-store"
+
+    # CORS for ActivityPub federation: Retry-After is unreadable to browser
+    # clients unless it is explicitly exposed
+    response["Access-Control-Allow-Origin"] = "*"
+    response["Access-Control-Expose-Headers"] = "Retry-After"
 
     return response

testbed/settings/base.py

@@ -62,7 +62,7 @@
     "django.contrib.sessions.middleware.SessionMiddleware",
     "django.middleware.common.CommonMiddleware",
     # LOLA Rate Limiting - positioned early to protect all endpoints
-    "testbed.core.middleware.rate_limiting.BasicRateLimitingMiddleware",
+    "testbed.core.middleware.rate_limiting.RateLimitingMiddleware",
     "django.middleware.csrf.CsrfViewMiddleware",
     "django.contrib.auth.middleware.AuthenticationMiddleware",
     "django.contrib.messages.middleware.MessageMiddleware",
@@ -259,3 +259,57 @@
         'rest_framework.authentication.SessionAuthentication',
     ],
 }
+
+# Rate limiting (LOLA Section 6.7)
+
+# Counters are per (rule, client IP) and live in the cache, so they are per-process.
+# See testbed/core/middleware/rate_limiting.py and docs/lola-rate-limiting.md for the design rationale.
+
+RATE_LIMIT_ENABLED = True
+
+# Longest matching prefix wins, so declaration order does not matter.
+# Limits are intentionally generous: one interactive OAuth authorization
+# spans several requests, and this testbed exists for people to exercise that flow repeatedly.
+RATE_LIMIT_RULES = [
+    {
+        "name": "oauth_authorize",
+        "prefix": "/oauth/authorize/",
+        "limit": 60,
+        "window": 300,
+    },
+    {
+        "name": "oauth_token",
+        "prefix": "/oauth/token/",
+        "limit": 120,
+        "window": 300,
+    },
+    {
+        "name": "lola_discovery",
+        "prefix": "/.well-known/oauth-authorization-server",
+        "limit": 60,
+        "window": 60,
+    },
+    {
+        "name": "lola_api",
+        "prefix": "/api/actors/",
+        "limit": 120,
+        "window": 60,
+    },
+]
+
+# Applied to any path not matched by a rule above.
+RATE_LIMIT_DEFAULT = {"name": "default", "limit": 300, "window": 60}
+
+# Paths that never consume a budget
+RATE_LIMIT_EXEMPT_PREFIXES = [
+    "/static/",
+    "/media/",
+    "/health",
+    "/favicon.ico",
+]
+
+# Number of proxies that append to X-Forwarded-For in front.
+# 0 means the header is not trusted at all and REMOTE_ADDR is used instead; each
+# deployment opts in by declaring how deep its own chain is. Set for Cloud Run in
+# production.py / staging.py.
+RATE_LIMIT_TRUSTED_PROXY_DEPTH = 0

testbed/settings/development.py

@@ -14,6 +14,8 @@
 ALLOWED_HOSTS = ["localhost", "127.0.0.1"]
 BASE_URL = "http://localhost:8000"
 
+RATE_LIMIT_ENABLED = env.bool("DJANGO_RATE_LIMIT_ENABLED", default=False)
+
 # Seeding settings
 SEED_ADMIN_USERNAME = "admin"
 SEED_ADMIN_EMAIL = "admin@seeding.com"

testbed/settings/production.py

@@ -13,6 +13,40 @@
 # Cloud Run uses X-Forwarded-Proto header for HTTPS detection
 SECURE_PROXY_SSL_HEADER = ('HTTP_X_FORWARDED_PROTO', 'https')
 
+"""
+Who counts as "the client" for rate limiting.
+
+Direct public Cloud Run traffic always passes through Google's managed frontend,
+so REMOTE_ADDR is that frontend rather than the caller and must not be assumed to be
+the original caller. The caller's real address is in X-Forwarded-For instead. That header is a
+list, and each machine appends what it saw -- so the entries on the RIGHT are
+trustworthy and the ones on the LEFT are whatever the caller typed.
+
+Cloud Run does not document a stable, trusted X-Forwarded-For layout for this
+run.app/domain-mapping path. (The "<client>, <load-balancer>" format is specified for
+external Application Load Balancers, which this testbed does not use yet.)
+
+This number says how many entries on the right belong to Google. The caller is the
+one just before them.
+
+     1  ->  "<caller>, <google>"     picks <caller>
+     0  ->  ignore the header, use REMOTE_ADDR (= Google, so everyone shares
+            a single rate-limit bucket)
+
+Why 1 when Google does not document the exact layout for this run.app /
+domain-mapping path: for honest callers 1 is never worse than 0. If the layout is
+what we expect, 1 identifies each caller correctly. If it is not, 1 falls back to
+REMOTE_ADDR and behaves exactly like 0.
+
+The risk of 1 is that a caller could pad the header to get a fresh bucket every
+request and dodge the limit -- and nothing logs when that happens.
+
+To check it after deploy: make one request with NO X-Forwarded-For, then look for
+"client resolution: xff_entries=N" in the logs. Set this to N - 1.
+See docs/lola-rate-limiting.md.
+"""
+RATE_LIMIT_TRUSTED_PROXY_DEPTH = env.int("DJANGO_RATE_LIMIT_TRUSTED_PROXY_DEPTH", default=1)
+
 # PostgreSQL for production
 DATABASES = {"default": env.db_url("DJ_DATABASE_CONN_STRING")}
 

testbed/settings/test.py

@@ -20,6 +20,9 @@
     }
 }
 
+# test_rate_limiting.py re-enables it explicitly with override_settings
+RATE_LIMIT_ENABLED = False
+
 # Faster password hashing for tests
 PASSWORD_HASHERS = [
     "django.contrib.auth.hashers.MD5PasswordHasher",