Skip to content

Security audit: MCP server secret management #216

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.

Sign up for GitHub

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

Merged
RafaelPo merged 3 commits into from
Feb 25, 2026
Merged

Security audit: MCP server secret management #216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
956103e
Add secret management security audit report
RafaelPo Feb 25, 2026
a5f2f04
Fix H-2 and M-4: encrypt PendingAuth in Redis, enforce UPLOAD_SECRET …
RafaelPo Feb 25, 2026
67d12bb
Remove audit report from branch (fixes only)
RafaelPo Feb 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions everyrow-mcp/src/everyrow_mcp/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,11 +276,12 @@ async def _validate_auth_request(
raise HTTPException(status_code=400, detail="Missing state")

key = build_key("pending", state)
pending_data = (
pending_data_encrypted = (
await self._redis.getdel(key) if consume else await self._redis.get(key)
)
if pending_data is None:
if pending_data_encrypted is None:
raise HTTPException(status_code=400, detail="Invalid or expired state")
pending_data = decrypt_value(pending_data_encrypted)

@sentry sentry Bot Feb 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The code attempts to decrypt PendingAuth data from Redis without handling cases where the data might be unencrypted from a previous version, causing an unhandled exception.
Severity: HIGH

Suggested Fix

Wrap the decryption call in a try...except InvalidToken block. In the except block, attempt to use the data as plaintext to maintain backward compatibility for entries created before the deployment. This ensures a smooth transition for in-flight authentication flows.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: everyrow-mcp/src/everyrow_mcp/auth.py#L284

Potential issue: The introduction of encryption for `PendingAuth` data lacks a backward
compatibility or migration strategy. During a deployment, if an authentication flow was
initiated before the update, an unencrypted `PendingAuth` entry will exist in Redis.
When the new code in `_validate_auth_request()` tries to process this entry, the call to
`decrypt_value(pending_data_encrypted)` will fail. The underlying `Fernet.decrypt()`
function will raise an `InvalidToken` exception on the plaintext data. Since this
exception is not caught, it will result in a 500 error, breaking the authentication flow
for any user caught in this transition period. The issue is temporary, as the affected
Redis keys have a 10-minute TTL.

Did we get this right? 👍 / 👎 to inform future reviews.

return PendingAuth.model_validate_json(pending_data)

async def _validate_client(self, pending: PendingAuth) -> None:
Expand Down Expand Up @@ -357,7 +358,7 @@ async def authorize(
await self._redis.setex(
name=build_key("pending", state),
time=settings.pending_auth_ttl,
value=pending.model_dump_json(),
value=encrypt_value(pending.model_dump_json()),
)
return f"{settings.mcp_server_url}/auth/start/{state}"

Expand All @@ -370,7 +371,7 @@ async def handle_start(self, request: Request) -> RedirectResponse:
await self._redis.setex(
name=build_key("pending", state),
time=settings.pending_auth_ttl,
value=pending.model_dump_json(),
value=encrypt_value(pending.model_dump_json()),
)
response = RedirectResponse(url=pending.supabase_redirect_url, status_code=302)
response.set_cookie(
Expand Down
4 changes: 2 additions & 2 deletions everyrow-mcp/src/everyrow_mcp/http_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,9 @@ def configure_http_mode(
mcp.settings.host = host
mcp.settings.port = port

if not settings.upload_secret:
if not settings.upload_secret or len(settings.upload_secret) < 32:
raise RuntimeError(
"UPLOAD_SECRET must be set in HTTP mode for HMAC signing. "
"UPLOAD_SECRET must be at least 32 characters in HTTP mode for HMAC signing. "
'Generate one with: python -c "import secrets; print(secrets.token_urlsafe(32))"'
)
if not no_auth and not settings.redis_password:
Expand Down