mirror of
https://github.com/Significant-Gravitas/AutoGPT.git
synced 2026-04-08 03:00:28 -04:00
fix: Address review round 2 — all 4 blockers
1. **Broken bot auth (CRITICAL)** — Replaced `Depends(lambda req: ...)` with proper `async def get_bot_api_key(request: Request)` dependency. The lambda pattern caused FastAPI to interpret `req` as a required query parameter, making all bot endpoints return 422. 2. **Timing-safe key comparison** — Switched from `!=` to `hmac.compare_digest()` to prevent timing side-channel attacks. 3. **Removed dead code** — Deleted the unused `verify_bot_api_key` function that had a stub body passing all requests. 4. **TOCTOU race in confirm** — Wrapped `PlatformLink.prisma().create()` in try/except for unique constraint violations. Concurrent requests with different tokens for the same identity now get a clean 409 instead of an unhandled 500. Also: regenerated openapi.json (removes spurious `req` query parameter that was leaking from the broken lambda pattern).
This commit is contained in:
@@ -14,6 +14,7 @@ Flow:
|
||||
check on next message via GET /api/platform-linking/resolve.
|
||||
"""
|
||||
|
||||
import hmac
|
||||
import logging
|
||||
import os
|
||||
import secrets
|
||||
@@ -22,7 +23,7 @@ from enum import Enum
|
||||
from typing import Annotated, Literal
|
||||
|
||||
from autogpt_libs import auth
|
||||
from fastapi import APIRouter, Depends, HTTPException, Security
|
||||
from fastapi import APIRouter, Depends, HTTPException, Request, Security
|
||||
from prisma.models import PlatformLink, PlatformLinkToken
|
||||
from pydantic import BaseModel, Field
|
||||
|
||||
@@ -52,31 +53,22 @@ class Platform(str, Enum):
|
||||
BOT_API_KEY = os.getenv("PLATFORM_BOT_API_KEY", "")
|
||||
|
||||
|
||||
async def verify_bot_api_key(
|
||||
authorization: str = Depends(lambda: ""),
|
||||
) -> None:
|
||||
"""Dependency that verifies the bot service API key."""
|
||||
# If no key is configured, reject all requests in production
|
||||
async def get_bot_api_key(request: Request) -> str | None:
|
||||
"""Extract the bot API key from the X-Bot-API-Key header."""
|
||||
return request.headers.get("x-bot-api-key")
|
||||
|
||||
|
||||
def _check_bot_api_key(api_key: str | None) -> None:
|
||||
"""Validate the bot API key. Uses constant-time comparison."""
|
||||
if not BOT_API_KEY:
|
||||
# No key configured — allow in development only
|
||||
if os.getenv("ENV", "development") != "development":
|
||||
raise HTTPException(
|
||||
status_code=503,
|
||||
detail="Bot API key not configured.",
|
||||
)
|
||||
# Allow in development without key
|
||||
return
|
||||
|
||||
# Check header
|
||||
|
||||
return None
|
||||
|
||||
|
||||
def _check_bot_api_key(request_api_key: str | None) -> None:
|
||||
"""Validate the bot API key from the X-Bot-API-Key header."""
|
||||
if not BOT_API_KEY:
|
||||
# Development mode: allow without key
|
||||
return
|
||||
if not request_api_key or request_api_key != BOT_API_KEY:
|
||||
if not api_key or not hmac.compare_digest(api_key, BOT_API_KEY):
|
||||
raise HTTPException(status_code=401, detail="Invalid bot API key.")
|
||||
|
||||
|
||||
@@ -156,9 +148,7 @@ class DeleteLinkResponse(BaseModel):
|
||||
)
|
||||
async def create_link_token(
|
||||
request: CreateLinkTokenRequest,
|
||||
x_bot_api_key: str | None = Depends(
|
||||
lambda req: req.headers.get("x-bot-api-key") # noqa: ARG005
|
||||
),
|
||||
x_bot_api_key: str | None = Depends(get_bot_api_key),
|
||||
) -> LinkTokenResponse:
|
||||
"""
|
||||
Called by the bot service when it encounters an unlinked user.
|
||||
@@ -230,9 +220,7 @@ async def create_link_token(
|
||||
)
|
||||
async def get_link_token_status(
|
||||
token: str,
|
||||
x_bot_api_key: str | None = Depends(
|
||||
lambda req: req.headers.get("x-bot-api-key") # noqa: ARG005
|
||||
),
|
||||
x_bot_api_key: str | None = Depends(get_bot_api_key),
|
||||
) -> LinkTokenStatusResponse:
|
||||
"""
|
||||
Called by the bot service to check if a user has completed linking.
|
||||
@@ -270,9 +258,7 @@ async def get_link_token_status(
|
||||
)
|
||||
async def resolve_platform_user(
|
||||
request: ResolveRequest,
|
||||
x_bot_api_key: str | None = Depends(
|
||||
lambda req: req.headers.get("x-bot-api-key") # noqa: ARG005
|
||||
),
|
||||
x_bot_api_key: str | None = Depends(get_bot_api_key),
|
||||
) -> ResolveResponse:
|
||||
"""
|
||||
Called by the bot service on every incoming message to check if
|
||||
@@ -354,15 +340,24 @@ async def confirm_link_token(
|
||||
)
|
||||
raise HTTPException(status_code=409, detail=detail)
|
||||
|
||||
# Create the link
|
||||
await PlatformLink.prisma().create(
|
||||
data={
|
||||
"userId": user_id,
|
||||
"platform": link_token.platform,
|
||||
"platformUserId": link_token.platformUserId,
|
||||
"platformUsername": link_token.platformUsername,
|
||||
}
|
||||
)
|
||||
# Create the link — catch unique constraint race condition
|
||||
try:
|
||||
await PlatformLink.prisma().create(
|
||||
data={
|
||||
"userId": user_id,
|
||||
"platform": link_token.platform,
|
||||
"platformUserId": link_token.platformUserId,
|
||||
"platformUsername": link_token.platformUsername,
|
||||
}
|
||||
)
|
||||
except Exception as exc:
|
||||
# Handle race condition: another request linked this identity
|
||||
if "unique" in str(exc).lower() or "Unique" in str(exc):
|
||||
raise HTTPException(
|
||||
status_code=409,
|
||||
detail="This platform account was just linked by another request.",
|
||||
) from exc
|
||||
raise
|
||||
|
||||
logger.info(
|
||||
"Linked %s:%s to user ...%s",
|
||||
@@ -473,9 +468,7 @@ class BotChatSessionResponse(BaseModel):
|
||||
)
|
||||
async def bot_create_session(
|
||||
request: BotChatRequest,
|
||||
x_bot_api_key: str | None = Depends(
|
||||
lambda req: req.headers.get("x-bot-api-key") # noqa: ARG005
|
||||
),
|
||||
x_bot_api_key: str | None = Depends(get_bot_api_key),
|
||||
) -> BotChatSessionResponse:
|
||||
"""
|
||||
Creates a new CoPilot chat session on behalf of a linked user.
|
||||
@@ -503,9 +496,7 @@ async def bot_create_session(
|
||||
)
|
||||
async def bot_chat_stream(
|
||||
request: BotChatRequest,
|
||||
x_bot_api_key: str | None = Depends(
|
||||
lambda req: req.headers.get("x-bot-api-key") # noqa: ARG005
|
||||
),
|
||||
x_bot_api_key: str | None = Depends(get_bot_api_key),
|
||||
):
|
||||
"""
|
||||
Send a message to CoPilot on behalf of a linked user and stream
|
||||
|
||||
@@ -5463,21 +5463,13 @@
|
||||
"summary": "Create a CoPilot session for a linked user (bot-facing)",
|
||||
"description": "Creates a new CoPilot chat session on behalf of a linked user.",
|
||||
"operationId": "postPlatform-linkingCreate a copilot session for a linked user (bot-facing)",
|
||||
"parameters": [
|
||||
{
|
||||
"name": "req",
|
||||
"in": "query",
|
||||
"required": true,
|
||||
"schema": { "title": "Req" }
|
||||
}
|
||||
],
|
||||
"requestBody": {
|
||||
"required": true,
|
||||
"content": {
|
||||
"application/json": {
|
||||
"schema": { "$ref": "#/components/schemas/BotChatRequest" }
|
||||
}
|
||||
}
|
||||
},
|
||||
"required": true
|
||||
},
|
||||
"responses": {
|
||||
"200": {
|
||||
@@ -5507,21 +5499,13 @@
|
||||
"summary": "Stream a CoPilot response for a linked user (bot-facing)",
|
||||
"description": "Send a message to CoPilot on behalf of a linked user and stream\nthe response back as Server-Sent Events.\n\nThe bot authenticates with its API key — no user JWT needed.",
|
||||
"operationId": "postPlatform-linkingStream a copilot response for a linked user (bot-facing)",
|
||||
"parameters": [
|
||||
{
|
||||
"name": "req",
|
||||
"in": "query",
|
||||
"required": true,
|
||||
"schema": { "title": "Req" }
|
||||
}
|
||||
],
|
||||
"requestBody": {
|
||||
"required": true,
|
||||
"content": {
|
||||
"application/json": {
|
||||
"schema": { "$ref": "#/components/schemas/BotChatRequest" }
|
||||
}
|
||||
}
|
||||
},
|
||||
"required": true
|
||||
},
|
||||
"responses": {
|
||||
"200": {
|
||||
@@ -5609,21 +5593,13 @@
|
||||
"summary": "Resolve a platform identity to an AutoGPT user",
|
||||
"description": "Called by the bot service on every incoming message to check if\nthe platform user has a linked AutoGPT account.",
|
||||
"operationId": "postPlatform-linkingResolve a platform identity to an autogpt user",
|
||||
"parameters": [
|
||||
{
|
||||
"name": "req",
|
||||
"in": "query",
|
||||
"required": true,
|
||||
"schema": { "title": "Req" }
|
||||
}
|
||||
],
|
||||
"requestBody": {
|
||||
"required": true,
|
||||
"content": {
|
||||
"application/json": {
|
||||
"schema": { "$ref": "#/components/schemas/ResolveRequest" }
|
||||
}
|
||||
}
|
||||
},
|
||||
"required": true
|
||||
},
|
||||
"responses": {
|
||||
"200": {
|
||||
@@ -5651,23 +5627,15 @@
|
||||
"summary": "Create a link token for an unlinked platform user",
|
||||
"description": "Called by the bot service when it encounters an unlinked user.\nGenerates a one-time token the user can use to link their account.",
|
||||
"operationId": "postPlatform-linkingCreate a link token for an unlinked platform user",
|
||||
"parameters": [
|
||||
{
|
||||
"name": "req",
|
||||
"in": "query",
|
||||
"required": true,
|
||||
"schema": { "title": "Req" }
|
||||
}
|
||||
],
|
||||
"requestBody": {
|
||||
"required": true,
|
||||
"content": {
|
||||
"application/json": {
|
||||
"schema": {
|
||||
"$ref": "#/components/schemas/CreateLinkTokenRequest"
|
||||
}
|
||||
}
|
||||
}
|
||||
},
|
||||
"required": true
|
||||
},
|
||||
"responses": {
|
||||
"200": {
|
||||
@@ -5739,12 +5707,6 @@
|
||||
"in": "path",
|
||||
"required": true,
|
||||
"schema": { "type": "string", "title": "Token" }
|
||||
},
|
||||
{
|
||||
"name": "req",
|
||||
"in": "query",
|
||||
"required": true,
|
||||
"schema": { "title": "Req" }
|
||||
}
|
||||
],
|
||||
"responses": {
|
||||
|
||||
Reference in New Issue
Block a user