Skip to content

P1: compare_digest(str,str) unauthenticated 500 in MCP SSE server — query-param reachable (BLOCK-A class, sibling of #207) #208

Description

@servathadi

Gate-5 on PR #205 (sos-205-f1a3aee4-gate5.md) found the BLOCK-A shape — hmac.compare_digest(str, str) on a value that comes off the wire — alive in the MCP SSE server, on a path that is easier to reach than the one BLOCK-A described.

The defect

hmac.compare_digest on two str raises TypeError on any non-ASCII codepoint; == never raises. Where a wire-supplied string is compared this way, one crafted byte turns a safe compare into an unauthenticated HTTP 500.

  • sos/mcp/sos_mcp_sse.py:6177 — identical shape.
  • Reachable via a ?token= query parameter, not just a header. This matters: the header variant needed a raw socket (httpx/starlette TestClient refuse to encode non-ASCII headers client-side, which is why CI was structurally blind to it). A query param carries UTF-8 natively, so ordinary clients hit this — no latin-1 trick required.
  • sos/mcp/sos_mcp_sse.py:7439 is the signup funnel — same shape, externally reachable.

Fails closed (no auth bypass), but it is pre-auth, cheaper than presenting a real bad token (the compare runs before any bcrypt/DB work), un-throttleable, and log-floods on demand.

Fix

Compare bytes at every network boundary:

hmac.compare_digest(presented.encode("utf-8"), expected.encode("utf-8"))

guarded by a non-empty check on the expected value so an unset secret can't match an empty presentation (the P0-A fail-open pattern closed in #205).

Related, same class

Standing pattern

A fix that lands in one service and not its siblings leaves the class alive. Grep the whole repo for the exact shape of any fix before calling it donecompare_digest( with any header/query/body-derived str argument, and getenv(NAME, "") feeding an == on an auth path.

Testing note

A green suite is not evidence for the header variant — instrument with a raw socket against a real uvicorn, not TestClient. The query-param variant is testable normally, so it should get an ordinary regression test.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions