Skip to content

Add early auth checks for MCP POSTs#148

Merged
mabashian merged 1 commit intomainfrom
security/AAP-70224-auth-before-session
Apr 22, 2026
Merged

Add early auth checks for MCP POSTs#148
mabashian merged 1 commit intomainfrom
security/AAP-70224-auth-before-session

Conversation

@mabashian
Copy link
Copy Markdown
Member

Addresses https://redhat.atlassian.net/browse/AAP-70224

Prevent unauthenticated DoS by rejecting POSTs to /mcp that lack both an MCP session ID and Authorization header before parsing the JSON body. Validate Bearer tokens prior to creating StreamableHTTPServerTransport during initialization and store session data only after successful token validation (addresses AAP-70224). Update existing E2E tests to expect 401 for unauthenticated requests and add a new security E2E test that mocks the AAP service to verify fast rejection of unauthenticated/invalid-token requests and successful session creation for valid tokens.

Prevent unauthenticated DoS by rejecting POSTs to /mcp that lack both an MCP session ID and Authorization header before parsing the JSON body. Validate Bearer tokens prior to creating StreamableHTTPServerTransport during initialization and store session data only after successful token validation (addresses AAP-70224). Update existing E2E tests to expect 401 for unauthenticated requests and add a new security E2E test that mocks the AAP service to verify fast rejection of unauthenticated/invalid-token requests and successful session creation for valid tokens.
@github-actions
Copy link
Copy Markdown

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 86.36% 773 / 895
🔵 Statements 85.89% 798 / 929
🟢 Functions 90.86% (🎯 70%) 169 / 186
🟢 Branches 76.92% (🎯 70%) 420 / 546
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
src/index.ts 81.36% 62.9% 92% 81.23% 39, 103-106, 146, 190, 206, 215, 244-247, 282-285, 311, 334, 344, 375, 383, 387, 401-402, 413, 425-428, 458-461, 537-545, 586-590, 597-609, 624, 629-633, 652-662, 685-687, 708-721, 810-811, 845, 905-931, 935-936
Generated in workflow #408 for commit 352e1d6 by the Vitest Coverage Report Action

@mabashian mabashian requested review from de1987, goneri and jameswnl April 20, 2026 17:37
Copy link
Copy Markdown
Contributor

@jameswnl jameswnl left a comment

Choose a reason for hiding this comment

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

Looks good — the two-layer defense is well architected and directly addresses AAP-70224. A few observations:

[medium] Path matching is overly broad (src/index.ts)

if (req.method === "POST" && req.path.includes("/mcp"))

.includes("/mcp") would match any path containing that substring (e.g. /something/mcp-unrelated). Consider tightening to something like req.path === "/mcp" || req.path.endsWith("/mcp") to match only the actual MCP routes.

[low] Duplicated JSON-RPC error response structure (src/index.ts)

The 401 error response { jsonrpc: "2.0", error: { code: -32000, message: "..." }, id: ... } is constructed inline in three places (middleware, missing token check, invalid token check). Consider extracting a small helper to reduce duplication.

[low] Timing assertion may be flaky (tests/security/unauthenticated-dos-prevention-e2e.test.ts)

expect(responseTime).toBeLessThan(100) — while 100ms is generous, timing-based assertions are sensitive to CI load. Worth keeping an eye on if it starts flaking.

@mabashian mabashian merged commit bf02b06 into main Apr 22, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants