[Gateway] Validate MCP headers and deterministic private catalogs - #385
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughModern Streamable HTTP requests validate routing headers and reject unsupported parameter headers. Modern catalogs redact parameter annotations, use private zero-TTL cache metadata, and return deterministic entries and template routes. Legacy and STDIO behavior remain unchanged. ChangesModern MCP gateway contracts
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ModernClient
participant miftahHttpServer
participant createMiftahRuntime
participant MiftahServer
participant RemoteUpstream
ModernClient->>miftahHttpServer: Send MCP request with routing headers
miftahHttpServer->>miftahHttpServer: Validate headers and reject Mcp-Param-* headers
miftahHttpServer->>createMiftahRuntime: Pass valid request metadata
createMiftahRuntime->>MiftahServer: Create modern request-scoped server
MiftahServer->>RemoteUpstream: Execute validated MCP operation
RemoteUpstream-->>MiftahServer: Return operation result
MiftahServer-->>ModernClient: Return result with private zero-TTL metadata
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/config.md`:
- Line 131: Update the cache behavior description in docs/config.md to state
that resource-template route identifiers are derived deterministically from the
upstream name, exposed name, and redacted catalog-visible URI template, rather
than only the upstream and original template. Preserve the existing
no-sensitive-input guarantee and surrounding cache semantics.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4e1ba110-ab03-4268-b10f-976f5cf0188f
📒 Files selected for processing (11)
CHANGELOG.mddocs/config.mddocs/library-api.mddocs/security.mdsrc/http/miftah-http-server.tssrc/mcp/server/miftah-server.tssrc/mcp/server/resource-prompt-registry.tssrc/runtime/create-miftah-runtime.tstests/helpers/fake-remote-upstream.tstests/mcp-v2-serving.test.tstests/resource-prompt-registry.test.ts
Closes #365
What changed
Mcp-Methodand operation-specificMcp-Namemirrors, with regression coverage for missing, duplicated, mismatched, malformed encoded, and valid encoded values before upstream execution.Mcp-Param-*forwarding safely: modern HTTP stripsx-mcp-headerschema declarations, rejects received parameter headers with a fixed non-reflective HTTP 400 JSON-RPC response, and never sends the header name or value to routing, policy, audit, diagnostics, or an upstream.ttlMs: 0, while leaving legacy result shapes unchanged.Why
Miftah uses the SDK's low-level
Server, so the SDK cannot validate schema-declared parameter headers for proxied tools. Rejecting that optional extension is safer than allowing an intermediary to authorize one argument view while Miftah executes another. A positive TTL is also not truthful until catalog caches can be keyed by authenticated principal, profile, policy, upstream capability, and configuration revision. Private zero-TTL results provide immediate invalidation and no cross-principal sharing.Compatibility
Validation
npx vitest run tests/mcp-v2-serving.test.ts tests/resource-prompt-registry.test.ts tests/tool-registry.test.ts— 33 passednpx vitest run tests/mcp-wrapper.test.ts -t 'resource template'— 3 passednpx vitest run tests/routing-policy.test.ts tests/audit-outcomes.test.ts tests/remote-transport.test.ts— 53 passednpm run typechecknpm run lint -- --quietnpm run buildnpm run check:packnpm run test:package— 31 passed on clean retrynpm test— 1,937 passed and 34 skipped; two failures are unchangeddevelopmenttest defects tracked separately in [Test] Make the local-stdio preset fixture independent of checkout path #383 and [Test] Make approval-continuation tamper mutation deterministic #384. [Test] Make the local-stdio preset fixture independent of checkout path #383 is checkout-path-sensitive because this worktree path contains the credential-shaped segmentheader-cache; [Test] Make approval-continuation tamper mutation deterministic #384 passed immediately in isolation after its input-dependent base64url mutation failed once.Summary by CodeRabbit
New Features
Mcp-Param-*headers with clear HTTP 400 errors.Documentation