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 <noreply@anthropic.com>
This commit is contained in:
@@ -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=
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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,20 +170,16 @@ 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:
|
||||
# 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",
|
||||
"agent auth optional invalid token path=%s",
|
||||
request.url.path,
|
||||
resolved[:6],
|
||||
)
|
||||
return None
|
||||
await _touch_agent_presence(request, session, agent)
|
||||
|
||||
155
backend/tests/test_agent_auth_security.py
Normal file
155
backend/tests/test_agent_auth_security.py
Normal file
@@ -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",))]
|
||||
@@ -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(
|
||||
|
||||
@@ -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
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@@ -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=<hex-digest>` (GitHub-style)
|
||||
- `X-Webhook-Signature: sha256=<hex-digest>`
|
||||
|
||||
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.
|
||||
|
||||
@@ -45,7 +45,7 @@ Some endpoints are designed for autonomous agents and use an agent token header:
|
||||
X-Agent-Token: <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 <agent-token>` 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`.
|
||||
|
||||
@@ -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 <agent-token>` 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.
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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=<hex-digest>` (GitHub-style)
|
||||
- `X-Webhook-Signature: sha256=<hex-digest>`
|
||||
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user