From fb8a9329237c7c6696feca0f3e32a06fc51ad1ca Mon Sep 17 00:00:00 2001 From: Abhimanyu Saharan Date: Sat, 7 Mar 2026 23:40:50 +0530 Subject: [PATCH] fix(security): Close review follow-up gaps Rate-limit the optional agent bearer path after user auth resolution so mixed user/agent routes no longer leave an unthrottled PBKDF2 path. Stop logging token prefixes on agent auth failures and require a locally supplied token for backend/.env.test instead of committing one. Update tests and docs to cover agent bearer fallback, configurable webhook signature headers, and the operator-facing security settings added by the hardening work. Co-Authored-By: Claude --- backend/.env.test | 5 +- backend/app/api/deps.py | 29 ++-- backend/app/core/agent_auth.py | 25 ++-- backend/tests/test_agent_auth_security.py | 155 ++++++++++++++++++++++ backend/tests/test_config_auth_mode.py | 3 +- backend/tests/test_security_fixes.py | 49 ++++++- docs/operations/README.md | 6 +- docs/reference/api.md | 4 +- docs/reference/authentication.md | 4 +- docs/reference/configuration.md | 24 ++++ docs/reference/security.md | 6 +- 11 files changed, 268 insertions(+), 42 deletions(-) create mode 100644 backend/tests/test_agent_auth_security.py diff --git a/backend/.env.test b/backend/.env.test index 63a8b650..8a55a253 100644 --- a/backend/.env.test +++ b/backend/.env.test @@ -1,5 +1,6 @@ # Commit-safe backend test environment. # Usage: +# export LOCAL_AUTH_TOKEN="$(python3 -c 'import secrets; print(secrets.token_urlsafe(48))')" # cd backend # uv run --env-file .env.test uvicorn app.main:app --reload --port 8000 @@ -17,9 +18,9 @@ BASE_URL=http://localhost:8000 # Auth mode: local for test/dev AUTH_MODE=local +# Set in your shell before starting the backend. # Must be non-placeholder and >= 50 chars. -# Generate with: python3 -c "import secrets; print(secrets.token_urlsafe(48))" -LOCAL_AUTH_TOKEN=local-auth-test-token-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +LOCAL_AUTH_TOKEN= # Clerk settings kept empty in local auth mode CLERK_SECRET_KEY= diff --git a/backend/app/api/deps.py b/backend/app/api/deps.py index 5b703301..d6ebea72 100644 --- a/backend/app/api/deps.py +++ b/backend/app/api/deps.py @@ -22,9 +22,9 @@ from dataclasses import dataclass from typing import TYPE_CHECKING, Literal from uuid import UUID -from fastapi import Depends, HTTPException, status +from fastapi import Depends, HTTPException, Request, status -from app.core.agent_auth import AgentAuthContext, get_agent_auth_context_optional +from app.core.agent_auth import get_agent_auth_context_optional from app.core.auth import AuthContext, get_auth_context, get_auth_context_optional from app.db.session import get_session from app.models.boards import Board @@ -46,8 +46,6 @@ if TYPE_CHECKING: from app.models.users import User AUTH_DEP = Depends(get_auth_context) -AUTH_OPTIONAL_DEP = Depends(get_auth_context_optional) -AGENT_AUTH_OPTIONAL_DEP = Depends(get_agent_auth_context_optional) SESSION_DEP = Depends(get_session) @@ -66,14 +64,29 @@ class ActorContext: agent: Agent | None = None -def require_user_or_agent( - auth: AuthContext | None = AUTH_OPTIONAL_DEP, - agent_auth: AgentAuthContext | None = AGENT_AUTH_OPTIONAL_DEP, +async def require_user_or_agent( + request: Request, + session: AsyncSession = SESSION_DEP, ) -> ActorContext: - """Authorize either a human user or an authenticated agent.""" + """Authorize either a human user or an authenticated agent. + + User auth is resolved first so normal bearer-token user traffic does not + also trigger agent-token verification on mixed user/agent routes. + """ + auth = await get_auth_context_optional( + request=request, + credentials=None, + session=session, + ) if auth is not None: require_user_actor(auth) return ActorContext(actor_type="user", user=auth.user) + agent_auth = await get_agent_auth_context_optional( + request=request, + agent_token=request.headers.get("X-Agent-Token"), + authorization=request.headers.get("Authorization"), + session=session, + ) if agent_auth is not None: return ActorContext(actor_type="agent", agent=agent_auth.agent) raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED) diff --git a/backend/app/core/agent_auth.py b/backend/app/core/agent_auth.py index 1d293f7e..ef88ac6b 100644 --- a/backend/app/core/agent_auth.py +++ b/backend/app/core/agent_auth.py @@ -133,9 +133,8 @@ async def get_agent_auth_context( agent = await _find_agent_for_token(session, resolved) if agent is None: logger.warning( - "agent auth invalid token path=%s token_prefix=%s", + "agent auth invalid token path=%s", request.url.path, - resolved[:6], ) raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED) await _touch_agent_presence(request, session, agent) @@ -171,21 +170,17 @@ async def get_agent_auth_context_optional( bool(authorization), ) return None - # Rate-limit when an agent token header is presented to prevent brute-force - # guessing via the optional auth path. Scoped to X-Agent-Token so that - # normal user Authorization headers are not throttled. - if agent_token: - client_ip = get_client_ip(request) - if not await agent_auth_limiter.is_allowed(client_ip): - raise HTTPException(status_code=status.HTTP_429_TOO_MANY_REQUESTS) + # Rate-limit any request that is actually attempting agent auth on the + # optional path. Shared user/agent dependencies resolve user auth first. + client_ip = get_client_ip(request) + if not await agent_auth_limiter.is_allowed(client_ip): + raise HTTPException(status_code=status.HTTP_429_TOO_MANY_REQUESTS) agent = await _find_agent_for_token(session, resolved) if agent is None: - if agent_token: - logger.warning( - "agent auth optional invalid token path=%s token_prefix=%s", - request.url.path, - resolved[:6], - ) + logger.warning( + "agent auth optional invalid token path=%s", + request.url.path, + ) return None await _touch_agent_presence(request, session, agent) return AgentAuthContext(actor_type="agent", agent=agent) diff --git a/backend/tests/test_agent_auth_security.py b/backend/tests/test_agent_auth_security.py new file mode 100644 index 00000000..e5499baa --- /dev/null +++ b/backend/tests/test_agent_auth_security.py @@ -0,0 +1,155 @@ +# ruff: noqa: INP001, SLF001 +"""Regression tests for agent-auth security hardening.""" + +from __future__ import annotations + +from types import SimpleNamespace + +import pytest +from fastapi import HTTPException + +from app.api import deps +from app.core import agent_auth +from app.core.auth import AuthContext + + +class _RecordingLimiter: + def __init__(self) -> None: + self.keys: list[str] = [] + + async def is_allowed(self, key: str) -> bool: + self.keys.append(key) + return True + + +async def _noop_touch(*_: object, **__: object) -> None: + return None + + +@pytest.mark.asyncio +async def test_optional_agent_auth_rate_limits_bearer_agent_token( + monkeypatch: pytest.MonkeyPatch, +) -> None: + limiter = _RecordingLimiter() + agent = SimpleNamespace(id="agent-1") + request = SimpleNamespace( + headers={"Authorization": "Bearer agent-secret"}, + client=SimpleNamespace(host="203.0.113.10"), + url=SimpleNamespace(path="/api/v1/tasks/task-1"), + method="POST", + ) + + async def _fake_find(_session: object, token: str) -> object: + assert token == "agent-secret" + return agent + + monkeypatch.setattr(agent_auth, "agent_auth_limiter", limiter) + monkeypatch.setattr(agent_auth, "_find_agent_for_token", _fake_find) + monkeypatch.setattr(agent_auth, "_touch_agent_presence", _noop_touch) + + ctx = await agent_auth.get_agent_auth_context_optional( + request=request, # type: ignore[arg-type] + agent_token=None, + authorization="Bearer agent-secret", + session=SimpleNamespace(), # type: ignore[arg-type] + ) + + assert ctx is not None + assert ctx.agent is agent + assert limiter.keys == ["203.0.113.10"] + + +@pytest.mark.asyncio +async def test_require_user_or_agent_skips_agent_auth_when_user_auth_succeeds( + monkeypatch: pytest.MonkeyPatch, +) -> None: + request = SimpleNamespace( + headers={"Authorization": "Bearer user-token"}, + client=SimpleNamespace(host="203.0.113.20"), + ) + + async def _fake_user_auth(**_: object) -> AuthContext: + return AuthContext(actor_type="user", user=SimpleNamespace(id="user-1")) + + async def _boom_agent_auth(**_: object) -> object: + raise AssertionError("agent auth should not run when user auth already succeeded") + + monkeypatch.setattr(deps, "get_auth_context_optional", _fake_user_auth) + monkeypatch.setattr(deps, "get_agent_auth_context_optional", _boom_agent_auth) + + actor = await deps.require_user_or_agent( + request=request, # type: ignore[arg-type] + session=SimpleNamespace(), # type: ignore[arg-type] + ) + + assert actor.actor_type == "user" + assert actor.user is not None + + +@pytest.mark.asyncio +async def test_required_agent_auth_invalid_token_logs_no_token_material( + monkeypatch: pytest.MonkeyPatch, +) -> None: + limiter = _RecordingLimiter() + logged: list[tuple[str, tuple[object, ...]]] = [] + request = SimpleNamespace( + headers={"X-Agent-Token": "invalid-agent-token"}, + client=SimpleNamespace(host="203.0.113.30"), + url=SimpleNamespace(path="/api/v1/agent/boards"), + method="POST", + ) + + async def _fake_find(_session: object, _token: str) -> None: + return None + + def _fake_warning(message: str, *args: object, **_: object) -> None: + logged.append((message, args)) + + monkeypatch.setattr(agent_auth, "agent_auth_limiter", limiter) + monkeypatch.setattr(agent_auth, "_find_agent_for_token", _fake_find) + monkeypatch.setattr(agent_auth.logger, "warning", _fake_warning) + + with pytest.raises(HTTPException) as exc_info: + await agent_auth.get_agent_auth_context( + request=request, # type: ignore[arg-type] + agent_token="invalid-agent-token", + authorization=None, + session=SimpleNamespace(), # type: ignore[arg-type] + ) + + assert exc_info.value.status_code == 401 + assert logged == [("agent auth invalid token path=%s", ("/api/v1/agent/boards",))] + + +@pytest.mark.asyncio +async def test_optional_agent_auth_invalid_token_logs_no_token_material( + monkeypatch: pytest.MonkeyPatch, +) -> None: + limiter = _RecordingLimiter() + logged: list[tuple[str, tuple[object, ...]]] = [] + request = SimpleNamespace( + headers={"Authorization": "Bearer invalid-agent-token"}, + client=SimpleNamespace(host="203.0.113.40"), + url=SimpleNamespace(path="/api/v1/tasks/task-2"), + method="POST", + ) + + async def _fake_find(_session: object, _token: str) -> None: + return None + + def _fake_warning(message: str, *args: object, **_: object) -> None: + logged.append((message, args)) + + monkeypatch.setattr(agent_auth, "agent_auth_limiter", limiter) + monkeypatch.setattr(agent_auth, "_find_agent_for_token", _fake_find) + monkeypatch.setattr(agent_auth.logger, "warning", _fake_warning) + + ctx = await agent_auth.get_agent_auth_context_optional( + request=request, # type: ignore[arg-type] + agent_token=None, + authorization="Bearer invalid-agent-token", + session=SimpleNamespace(), # type: ignore[arg-type] + ) + + assert ctx is None + assert logged == [("agent auth optional invalid token path=%s", ("/api/v1/tasks/task-2",))] diff --git a/backend/tests/test_config_auth_mode.py b/backend/tests/test_config_auth_mode.py index 6c6ac1fc..941da49a 100644 --- a/backend/tests/test_config_auth_mode.py +++ b/backend/tests/test_config_auth_mode.py @@ -101,8 +101,7 @@ def test_base_url_field_is_required(monkeypatch: pytest.MonkeyPatch) -> None: ) text = str(exc_info.value) - assert "base_url" in text - assert "Field required" in text + assert "BASE_URL must be set and non-empty" in text @pytest.mark.parametrize( diff --git a/backend/tests/test_security_fixes.py b/backend/tests/test_security_fixes.py index 01e3f4fd..a559591e 100644 --- a/backend/tests/test_security_fixes.py +++ b/backend/tests/test_security_fixes.py @@ -309,6 +309,50 @@ class TestWebhookHmacVerification: finally: await engine.dispose() + @pytest.mark.asyncio + async def test_webhook_accepts_configured_signature_header( + self, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """A configured signature_header should override the default header names.""" + engine = await _make_engine() + session_maker = async_sessionmaker(engine, class_=AsyncSession, expire_on_commit=False) + app = _build_webhook_test_app(session_maker) + + monkeypatch.setattr(board_webhooks, "enqueue_webhook_delivery", lambda p: True) + monkeypatch.setattr( + board_webhooks, + "webhook_ingest_limiter", + InMemoryRateLimiter(max_requests=1000, window_seconds=60.0), + ) + + secret = "my-secret-key" + async with session_maker() as session: + board, webhook = await _seed_webhook_with_secret(session, secret=secret) + webhook.signature_header = "X-Custom-Signature" + session.add(webhook) + await session.commit() + + body = b'{"event": "test"}' + sig = hmac_mod.new(secret.encode(), body, hashlib.sha256).hexdigest() + + try: + async with AsyncClient( + transport=ASGITransport(app=app), + base_url="http://testserver", + ) as client: + response = await client.post( + f"/api/v1/boards/{board.id}/webhooks/{webhook.id}", + content=body, + headers={ + "Content-Type": "application/json", + "X-Custom-Signature": f"sha256={sig}", + }, + ) + assert response.status_code == 202 + finally: + await engine.dispose() + # --------------------------------------------------------------------------- # Task 10: Prompt injection sanitization @@ -495,8 +539,3 @@ class TestWebhookPayloadSizeLimit: assert response.status_code == 413 finally: await engine.dispose() - - -# --------------------------------------------------------------------------- -# Task 12: Gateway token redaction -# --------------------------------------------------------------------------- diff --git a/docs/operations/README.md b/docs/operations/README.md index 123d771a..fe2b77f0 100644 --- a/docs/operations/README.md +++ b/docs/operations/README.md @@ -107,13 +107,13 @@ When using the in-memory backend in multi-process deployments, also apply rate l ### Webhook signature errors (403) -If a webhook has a `secret` configured, inbound payloads must include a valid HMAC-SHA256 signature in one of these headers: +If a webhook has a `secret` configured, inbound payloads must include a valid HMAC-SHA256 signature. If the webhook also sets `signature_header`, that exact header name must be used. Otherwise the backend checks these defaults: - `X-Hub-Signature-256: sha256=` (GitHub-style) - `X-Webhook-Signature: sha256=` -Missing or invalid signatures return `403 Forbidden`. If you see unexpected 403s on webhook ingest, verify that the sending service is computing the HMAC correctly using the webhook's secret. +Missing or invalid signatures return `403 Forbidden`. If you see unexpected 403s on webhook ingest, verify that the sending service is computing the HMAC correctly using the webhook's secret and sending it in the configured header. ### Webhook payload too large (413) -Webhook ingest enforces a **1 MB** payload size limit. Payloads exceeding this return `413 Content Too Large`. If you need to send larger payloads, consider sending a URL reference instead of inline content. +Webhook ingest enforces a **1 MB** payload size limit by default. Payloads exceeding this return `413 Content Too Large`. If you need to raise the limit, set `WEBHOOK_MAX_PAYLOAD_BYTES`; otherwise consider sending a URL reference instead of inline content. diff --git a/docs/reference/api.md b/docs/reference/api.md index a0291a55..4f2983c5 100644 --- a/docs/reference/api.md +++ b/docs/reference/api.md @@ -45,7 +45,7 @@ Some endpoints are designed for autonomous agents and use an agent token header: X-Agent-Token: ``` -In the backend, these are enforced via the “agent auth” context. When in doubt, consult the route’s dependencies (e.g., `require_user_or_agent`). +On shared user/agent routes, the backend also accepts `Authorization: Bearer ` after user auth does not resolve. When in doubt, consult the route’s dependencies (e.g., `require_user_or_agent`). Agent authentication is rate-limited to **20 requests per 60 seconds per IP**. Exceeding this limit returns `429 Too Many Requests`. @@ -83,7 +83,7 @@ The following per-IP rate limits are enforced on sensitive endpoints: | Endpoint | Limit | Window | | --- | --- | --- | -| Agent authentication (`X-Agent-Token`) | 20 requests | 60 seconds | +| Agent authentication (`X-Agent-Token` or agent bearer fallback on shared routes) | 20 requests | 60 seconds | | Webhook ingest (`POST .../webhooks/{id}`) | 60 requests | 60 seconds | When a rate limit is exceeded, the API returns `429 Too Many Requests`. diff --git a/docs/reference/authentication.md b/docs/reference/authentication.md index 68eac442..f1ca37b9 100644 --- a/docs/reference/authentication.md +++ b/docs/reference/authentication.md @@ -31,9 +31,9 @@ Frontend: ## Agent authentication -Autonomous agents authenticate via an `X-Agent-Token` header (not the bearer token used by human users). See [API reference](api.md) for details. +Autonomous agents primarily authenticate via an `X-Agent-Token` header. On shared user/agent routes, the backend also accepts `Authorization: Bearer ` after user auth does not resolve. See [API reference](api.md) for details. Security notes: - Agent auth is rate-limited to **20 requests per 60 seconds per IP**. Exceeding this returns `429 Too Many Requests`. -- On authentication failure, only a short prefix of the presented token is logged to aid debugging. Full tokens are never written to logs. +- Authentication failure logs never include token material. diff --git a/docs/reference/configuration.md b/docs/reference/configuration.md index 758b6eeb..eb2ffbae 100644 --- a/docs/reference/configuration.md +++ b/docs/reference/configuration.md @@ -18,6 +18,30 @@ See `.env.example` for defaults and required values. - **When required:** `AUTH_MODE=local` - **Policy:** Must be non-placeholder and at least 50 characters. +### `WEBHOOK_MAX_PAYLOAD_BYTES` + +- **Default:** `1048576` (1 MiB) +- **Purpose:** Maximum accepted inbound webhook payload size before the API returns `413 Content Too Large`. + +### `RATE_LIMIT_BACKEND` + +- **Default:** `memory` +- **Allowed values:** `memory`, `redis` +- **Purpose:** Selects whether rate limits are tracked per-process in memory or shared through Redis. + +### `RATE_LIMIT_REDIS_URL` + +- **Default:** _(blank)_ +- **When required:** `RATE_LIMIT_BACKEND=redis` and `RQ_REDIS_URL` is not set +- **Purpose:** Redis connection string used for shared rate limits. +- **Fallback:** If blank and Redis rate limiting is enabled, the backend falls back to `RQ_REDIS_URL`. + +### `TRUSTED_PROXIES` + +- **Default:** _(blank)_ +- **Purpose:** Comma-separated list of trusted reverse-proxy IPs or CIDRs used to honor `Forwarded` / `X-Forwarded-For` client IP headers. +- **Gotcha:** Leave this blank unless the direct peer is a proxy you control. + ## Security response headers These environment variables control security headers added to every API response. Set any variable to blank (`""`) to disable the corresponding header. diff --git a/docs/reference/security.md b/docs/reference/security.md index f1995626..831f596b 100644 --- a/docs/reference/security.md +++ b/docs/reference/security.md @@ -21,7 +21,7 @@ Per-IP rate limits are enforced on sensitive endpoints: | Endpoint | Limit | Window | Status on exceed | | --- | --- | --- | --- | -| Agent authentication (`X-Agent-Token`) | 20 requests | 60 seconds | `429` | +| Agent authentication (`X-Agent-Token` or agent bearer fallback on shared routes) | 20 requests | 60 seconds | `429` | | Webhook ingest (`POST .../webhooks/{id}`) | 60 requests | 60 seconds | `429` | Two backends are supported, selected via `RATE_LIMIT_BACKEND`: @@ -35,7 +35,7 @@ The Redis backend fails open — if Redis becomes unreachable during a request, ## Webhook HMAC verification -Webhooks may optionally have a `secret` configured. When a secret is set, inbound payloads must include a valid HMAC-SHA256 signature in one of these headers: +Webhooks may optionally have a `secret` configured. When a secret is set, inbound payloads must include a valid HMAC-SHA256 signature. If `signature_header` is configured on the webhook, that exact header is required. Otherwise the backend falls back to these default headers: - `X-Hub-Signature-256: sha256=` (GitHub-style) - `X-Webhook-Signature: sha256=` @@ -72,7 +72,7 @@ This boundary helps LLM-based agents distinguish trusted instructions from untru ## Agent token logging -On authentication failure, only a short prefix of the presented token is logged to aid debugging. Full tokens are never written to logs. +On authentication failure, logs include request context only. Token values and token prefixes are not written to logs. ## Cross-tenant isolation