Feature/mcp bearer token auth - #645
Conversation
…sues - Fix query_database table allowlist to use snake_case SQL names (was camelCase, causing broken queries) - Export ai-schema from schema barrel index - Replace manual JSON parsing with zValidator in mcp-auth-router - Remove misleading description keyword validation in query-builder (was checking free-text, not SQL) - Fix SELECT column quoting in query-builder - Remove eslint-disable and any type in query-builder - Extract shared fetchMatchPlayerRows helper to eliminate ~20 duplicate join chains - Refactor getPlayerStats, getHeadToHead, getScoringStats, getStreaks, getFormGuide, getEloProgression, getTeamChemistry to use shared helper - Use execFileSync with array args in install.ts to prevent command injection - Remove redundant expiry check in mcp-auth middleware (already in WHERE clause) - Combine duplicate import lines in mcp-schema - Extract CLIENT_ONLY_TOOLS constant to replace render_chart magic string - Remove drawProbability from ELO output (standard ELO has no draw model) - Remove all inline comments
process.cwd() resolved to repo root, writing wrong dist path into agent config
…bearer session auth
8143c24 to
df2146d
Compare
🚀 Preview Environment DeployedPreview URL: https://scorebrawl-pr-645.coding-cowboys.workers.dev Test Account:
This preview environment will be automatically cleaned up when the PR is closed. |
🎭 E2E Test Results✅ 20 passed | ❌ 0 failed | ⏱️ 45.0s
📊 View HTML Report Last updated: 2026-05-28T20:14:12.462Z |
![agent-coding-cowboy[bot]](https://avatars.githubusercontent.com/u/246662916?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR adds device authorization OAuth flow, MCP server with 40+ query tools, and device code schema. Three blocking issues: (1) N+1 queries throughout tool-executors.ts — multiple functions fetch all data then filter in-memory instead of using joins; (2) loose SQL value handling in query-builder.ts could allow injection; (3) no integration tests for device authorization OAuth flow. The MCP router and tool implementations are large (2656+ lines) and the N+1 patterns will cause CPU limit errors on Cloudflare Workers under load.
To re-request review after addressing feedback, push a new commit.
| const extractStats = (stats: unknown) => { | ||
| if (!stats || typeof stats !== "object" || "error" in stats) { | ||
| return { | ||
| name: "Unknown", |
There was a problem hiding this comment.
Uses in-memory filtering on full match player rows — N+1 anti-pattern. Cloudflare Workers have strict CPU limits. Should use database-level joins with WHERE clauses pushed down, not fetch-all-then-filter.
| args: { leagueId: string; seasonSlug?: string; limit?: number } | ||
| ) { | ||
| const { db } = ctx; | ||
| const limit = Math.min(args.limit ?? 5, 20); |
There was a problem hiding this comment.
Same N+1 pattern: fetches all match players, then filters by two player names in memory to find shared matches.
| .select({ | ||
| score: seasonPlayer.score, | ||
| userName: user.name, | ||
| guestName: guest.displayName, |
There was a problem hiding this comment.
Same anti-pattern: fetches all rows then filters in JavaScript instead of database-level filtering.
| const params: (string | number)[] = [args.leagueId]; | ||
|
|
||
| for (const w of args.where ?? []) { | ||
| const col = sanitizeIdentifier(w.column); |
There was a problem hiding this comment.
Potential SQL injection surface — values are string-concatenated rather than using typed parameter binding consistently.
| @@ -0,0 +1,206 @@ | |||
| import { Hono } from "hono"; | |||
There was a problem hiding this comment.
Security-critical OAuth device flow has no tRPC integration test coverage. Frontend page exists but backend endpoints are untested.
| const env = c.env; | ||
| const userAssets = c.get("userAssets"); | ||
|
|
||
| const activeOrganizationId = auth.session.activeOrganizationId; |
There was a problem hiding this comment.
How does this work for device auth?
🧹 Preview Environment Cleaned UpThe preview environment, database, R2 bucket, and queue have been deleted. |
2 participants
Implements two-step OAuth-style handoff for MCP CLI authentication:
Web UI mints a short-lived single-use auth code
CLI exchanges that code for a long-lived bearer token (scbr_…)
MCP requests authenticate with Authorization: Bearer header
Token is hashed at rest and stored with the user's active league
Includes:
mcp_auth_code and mcp_token database tables
/api/mcp-auth/authorize and /api/mcp-auth/exchange endpoints
Bearer-token auth middleware for /api/mcp
Updated mcp-login page to mint exchange codes
Updated CLI to exchange codes and send Bearer tokens
File-only token storage (no keytar dependency)
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.