Skip to content

fix(mcp): restore real keyless identity and recovery correlation - #336

Merged
hmishra2250 merged 2 commits into
mainfrom
fix/hosted-keyless-identity-recovery-id
Jul 26, 2026
Merged

fix(mcp): restore real keyless identity and recovery correlation#336
hmishra2250 merged 2 commits into
mainfrom
fix/hosted-keyless-identity-recovery-id

Conversation

@hmishra2250

@hmishra2250 hmishra2250 commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • preserve Traefik's sanitized X-Forwarded-For value instead of replacing it with the Traefik pod IP
  • keep the MCP boundary fail-closed for missing, malformed, duplicate, or multi-hop identities
  • restore server-generated recovery request_id values and correlate execution-time recovery responses with [MCP_ACTION] error events

Why

Hosted anonymous traffic is currently bucketed by the Traefik pod address because nginx overwrites X-Forwarded-For with $remote_addr. Core uses the forwarded value for keyless quotas, credits, team identity, audit data, and Spur checks. This change restores the real edge-sanitized client identity without trusting client-provided forwarding chains.

The Stage-1 recovery contract also lost its server-generated request_id. This PR restores that correlation boundary without accepting JSON-RPC IDs or request headers as recovery IDs.

Scope / safety

  • exactly two commits: proxy identity, then recovery IDs
  • no schema, dependency, tool-surface, endpoint, auth-policy, or infra change
  • Traefik v3.5 default forwarded-header behavior was exercised locally with positive and negative controls
  • production topology checked read-only: externalTrafficPolicy: Local, MCP Services are ClusterIP, and no forwardedHeaders override is configured
  • expected rollout effect: old Traefik-IP keyless buckets age out under their existing TTL; real-client buckets begin independently
  • expected monitoring effect: Spur sees real client IPs, so VPN/proxy rejection volume may increase to its intended level

Validation

  • npm test — 46/46 passing
  • npm run lint
  • docker build -f Dockerfile.service -t firecrawl-mcp:keyless-identity-recovery-id .
  • nginx -t inside the built service image
  • Traefik v3.5.0 -> nginx -> MCP -> fake Core wire test:
    • baseline keyless Search succeeds with one stable identity
    • forged single XFF never reaches Core as the attacker-selected identity
    • forged multi-hop XFF never reaches Core as an attacker-selected identity
  • ZDR phase 1 and phase 2 recoveries:
    • include UUIDv4 request_id
    • reject client JSON-RPC/header IDs as the recovery ID
    • match the [MCP_ACTION] error request_id
    • make zero backend requests
  • managed OAuth Parse remains green, covering preservation of the non-enumerable managed credential when invocation context is correlated

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Restore real hosted keyless identity and server-generated recovery correlation. This forwards the edge-sanitized client IP and adds reliable request_id values that match [MCP_ACTION] logs, fixing per-IP quotas and recovery flows.

  • Bug Fixes
    • Proxy/identity: forward X-Forwarded-For from $http_x_forwarded_for and reject multi-hop or malformed values.
    • Recovery correlation: generate request_id on the server, include it in recovery payloads, and use the same ID in [MCP_ACTION] logs; ignore client JSON-RPC IDs and x-request-id.
    • Session flow: carry the requestId through tool execution and keyless recoveries (eligibility, quota, ZDR) and preserve the managed OAuth API key in the invocation context.

Written for commit b8dfb26. Summary will update on new commits.

Review in cubic

@hmishra2250
hmishra2250 marked this pull request as ready for review July 26, 2026 19:35
@hmishra2250
hmishra2250 merged commit c524c5a into main Jul 26, 2026
1 check passed
@hmishra2250
hmishra2250 deleted the fix/hosted-keyless-identity-recovery-id branch July 26, 2026 19:35
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.

1 participant