fix: gateway security audit — SQL injection, auth bypass, service bindings - #34
Conversation
Adds standard ChittyOS branding header with tier badge, org badge, and accurate service description to README.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…dings P0: Fix SQL injection in chronicleTimeline — replaced sql.unsafe() string interpolation with parameterized tagged template queries for all user input (entityId, startDate, endDate, services). P0: Remove public mode auth fallback — requests are now rejected when MCP_API_KEY or OAUTH_CLIENT_ID/SECRET are not configured, instead of silently allowing unauthenticated access. P1: Add service bindings for same-zone Workers (SVC_CHITTYID, SVC_CHITTYAUTH, SVC_CHITTYCONNECT, SVC_CHITTYROUTER, SVC_CHITTYREGISTRY) to fix route-to-route fetch issue per CF docs. executeHttpTool now routes through bindings with direct fetch fallback for external services only. P1: Remove mcp.chitty.cc/* route conflict (owned by chittyconnect), assign own hostname mcp-chatgpt.chitty.cc/*. P2: Rename worker chatgpt-mcp-gateway → chittymcp-gateway, update compatibility_date to 2026-03-16, add [observability] block, standardize health endpoint to status:"ok" format. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@coderabbitai review |
|
Warning Review limit reached
More reviews will be available in 47 minutes and 31 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis pull request rebrands the service from "ChatGPT MCP Gateway" to "ChittyMCP Gateway", updates infrastructure bindings for inter-service communication via Cloudflare Workers, strengthens authentication validation logic, and refactors SQL query construction. Version bumped to 1.1.0 with updated API response fields and routing patterns. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mcp-gateway- chatgpt/src/index.js (1)
53-70:⚠️ Potential issue | 🟡 MinorMCP discovery document references old hostname
mcp.chitty.cc.The
issuer,authorization_endpoint, andtoken_endpointfields still point tomcp.chitty.cc, but the worker now servesmcp-chatgpt.chitty.cc. OAuth clients relying on this discovery document may fail if these endpoints are unreachable at the old hostname or if issuer validation is strict.🔧 Suggested fix
return c.json({ schema_version: '2024-11-05', - issuer: 'https://mcp.chitty.cc', - authorization_endpoint: 'https://mcp.chitty.cc/oauth/authorize', - token_endpoint: 'https://mcp.chitty.cc/oauth/token', + issuer: 'https://mcp-chatgpt.chitty.cc', + authorization_endpoint: 'https://mcp-chatgpt.chitty.cc/oauth/authorize', + token_endpoint: 'https://mcp-chatgpt.chitty.cc/oauth/token', scopes_supported: ['mcp:read', 'mcp:write'],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp-gateway-` chatgpt/src/index.js around lines 53 - 70, The discovery document returned by the route app.get('/.well-known/mcp.json', (c) => { ... }) still hardcodes the old hostname in the issuer, authorization_endpoint, and token_endpoint fields; update those fields to use the current service hostname (mcp-chatgpt.chitty.cc) or derive the host dynamically from the request/context so the discovery JSON matches the actual worker domain and avoids mismatched issuer/endpoints.mcp-gateway- chatgpt/src/services/tool-executor.js (1)
247-256:⚠️ Potential issue | 🟠 Major
chronicleSearchstill usessql.unsafe()— SQL injection risk remains.The
chronicleSearchfunction at line 253 still usessql.unsafe(whereClause)with string interpolation. ThewhereClausesarray is built with positional placeholders ($1,$2, etc.) andparamsarray, but this pattern bypasses the tagged template's automatic escaping. UnlikechronicleTimeline, this function was not refactored.🛡️ Suggested refactor using conditional tagged templates
Apply a similar pattern to what was done for
chronicleTimeline:async function chronicleSearch(sql, args) { const { query, filters = {}, limit = 100, offset = 0 } = args; - let whereClauses = []; - let params = []; - - // Add filters - if (filters.service) { - whereClauses.push(`service = $${params.length + 1}`); - params.push(filters.service); - } - // ... other filters ... - - const whereClause = whereClauses.length > 0 ? `WHERE ${whereClauses.join(' AND ')}` : ''; - - const results = await sql` - SELECT id, service, action, entity_id, user_id, status, metadata, created_at, - ts_rank(search_vector, to_tsquery('english', ${query || ''})) as relevance - FROM chronicle_events - ${whereClause ? sql.unsafe(whereClause) : sql``} - ORDER BY ${query ? sql`relevance DESC,` : sql``} created_at DESC - LIMIT ${limit} OFFSET ${offset} - `; + // Build explicit parameterized query branches similar to chronicleTimeline + // This is more verbose but eliminates sql.unsafe() usageAlternatively, consider using a query builder library that safely composes WHERE clauses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp-gateway-` chatgpt/src/services/tool-executor.js around lines 247 - 256, chronicleSearch currently injects a raw string via sql.unsafe(whereClause) which bypasses parameter escaping; replace that with composing the WHERE clause as an array of tagged-template fragments (the existing whereClauses array should hold sql`...` fragments or be rebuilt into them) and then inject it safely using the tagged template helpers (e.g. sql.join(whereClauses, sql` AND `)) instead of sql.unsafe; remove the string interpolation of whereClause, ensure the params/values are passed as template values to sql so limit, offset, query, and any positional parameters are properly escaped, and keep the conditional logic that omits the WHERE section when whereClauses is empty.
🧹 Nitpick comments (1)
mcp-gateway- chatgpt/src/lib/auth.js (1)
98-125: Timing-safe comparison has an early exit on length mismatch.The
constantTimeComparefunction returnsfalseimmediately when string lengths differ (line 108-110). This leaks timing information about whether the lengths match. While not exploitable for most API key formats, consider using a constant-time approach for length comparison as well.🔒 Optional: Pad or hash before length check
One approach is to always hash both inputs regardless of length, letting the hash comparison be the sole timing-safe check:
async function constantTimeCompare(a, b) { if (typeof a !== 'string' || typeof b !== 'string') { return false; } - // Use Web Crypto API for constant-time comparison const encoder = new TextEncoder(); const aBytes = encoder.encode(a); const bBytes = encoder.encode(b); - if (aBytes.length !== bBytes.length) { - return false; - } - // Hash both strings and compare hashes const aHash = await crypto.subtle.digest('SHA-256', aBytes); const bHash = await crypto.subtle.digest('SHA-256', bBytes); const aArray = new Uint8Array(aHash); const bArray = new Uint8Array(bHash); let result = 0; for (let i = 0; i < aArray.length; i++) { result |= aArray[i] ^ bArray[i]; } return result === 0; }Note: This changes semantics slightly (different-length strings with same prefix hash differently), but for credential comparison this is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp-gateway-` chatgpt/src/lib/auth.js around lines 98 - 125, The function constantTimeCompare currently returns early when lengths differ (aBytes.length !== bBytes.length); remove that early return and instead perform the same hashing-and-bytewise comparison for both inputs so timing does not reveal length differences: always encode and call crypto.subtle.digest on a and b (e.g., using TextEncoder -> aBytes/bBytes -> digest -> Uint8Array) and compare the resulting hash bytes with the existing XOR-accumulate loop (result |= aArray[i] ^ bArray[i]) to decide equality; if you must normalize lengths, do so deterministically (pad or hash full input) before hashing so the length check is not an early exit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@mcp-gateway-` chatgpt/src/index.js:
- Around line 53-70: The discovery document returned by the route
app.get('/.well-known/mcp.json', (c) => { ... }) still hardcodes the old
hostname in the issuer, authorization_endpoint, and token_endpoint fields;
update those fields to use the current service hostname (mcp-chatgpt.chitty.cc)
or derive the host dynamically from the request/context so the discovery JSON
matches the actual worker domain and avoids mismatched issuer/endpoints.
In `@mcp-gateway-` chatgpt/src/services/tool-executor.js:
- Around line 247-256: chronicleSearch currently injects a raw string via
sql.unsafe(whereClause) which bypasses parameter escaping; replace that with
composing the WHERE clause as an array of tagged-template fragments (the
existing whereClauses array should hold sql`...` fragments or be rebuilt into
them) and then inject it safely using the tagged template helpers (e.g.
sql.join(whereClauses, sql` AND `)) instead of sql.unsafe; remove the string
interpolation of whereClause, ensure the params/values are passed as template
values to sql so limit, offset, query, and any positional parameters are
properly escaped, and keep the conditional logic that omits the WHERE section
when whereClauses is empty.
---
Nitpick comments:
In `@mcp-gateway-` chatgpt/src/lib/auth.js:
- Around line 98-125: The function constantTimeCompare currently returns early
when lengths differ (aBytes.length !== bBytes.length); remove that early return
and instead perform the same hashing-and-bytewise comparison for both inputs so
timing does not reveal length differences: always encode and call
crypto.subtle.digest on a and b (e.g., using TextEncoder -> aBytes/bBytes ->
digest -> Uint8Array) and compare the resulting hash bytes with the existing
XOR-accumulate loop (result |= aArray[i] ^ bArray[i]) to decide equality; if you
must normalize lengths, do so deterministically (pad or hash full input) before
hashing so the length check is not an early exit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd2c3d81-1df5-490e-9a23-f96d4ddf2507
📒 Files selected for processing (5)
README.mdmcp-gateway- chatgpt/src/index.jsmcp-gateway- chatgpt/src/lib/auth.jsmcp-gateway- chatgpt/src/services/tool-executor.jsmcp-gateway- chatgpt/wrangler.toml
|
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== DIRECTORY STRUCTURE ==="
tree mcp-gateway-chatgpt/ 2>/dev/null || find . -type f | grep -v node_modules | grep -v .git | head -60
echo ""
echo "=== wrangler.toml ==="
cat mcp-gateway-chatgpt/wrangler.toml 2>/dev/null || fd -i "wrangler.toml" --exec cat {}
echo ""
echo "=== src/index.js ==="
cat mcp-gateway-chatgpt/src/index.js 2>/dev/null || fd -i "index.js" --exec cat {}
echo ""
echo "=== src/lib/auth.js ==="
cat mcp-gateway-chatgpt/src/lib/auth.js 2>/dev/null || fd -i "auth.js" --exec cat {}
echo ""
echo "=== src/services/tool-executor.js ==="
cat mcp-gateway-chatgpt/src/services/tool-executor.js 2>/dev/null || fd -i "tool-executor.js" --exec cat {}Length of output: 60291
Adversarial Security Review — PR
|
| Severity | Finding | File |
|---|---|---|
| 🔴 P0 | ReferenceError crash in chronicleTimeline return — regression |
tool-executor.js |
| 🔴 P0 | validateAccessToken prefix-only check — auth bypass survives |
oauth.js |
| 🔴 P0 | sql.unsafe() in chronicleSearch — SQL injection unfixed |
tool-executor.js |
| 🔴 P1 | [env.production] missing service binding redeclarations + corrupted wrangler.toml tail |
wrangler.toml |
| 🟡 P1 | !== timing oracle + split error messages in validateClientCredentials |
oauth.js |
| 🟡 P2 | SSE 5-min connection hold, no rate limit | index.js |
This PR should not be merged in its current state. The three P0 items are independently sufficient to block.
1 participant
Summary
chronicleTimelineusedsql.unsafe()with string interpolation — replaced with parameterized queriesmcp.chitty.cc/*(owned by chittyconnect), assignedmcp-chatgpt.chitty.cc/*chatgpt-mcp-gateway→chittymcp-gateway, updated compat date, added observability, standardized health endpointTest plan
chronicleTimelinetool works with parameterized queries (entityId, date range, service filter)/mcp/messagereturn 401 (not 200 public mode)wrangler deploy --dry-runpasses)connect.chitty.ccMCP endpoints{"status":"ok","service":"chittymcp-gateway"}🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores