Skip to content

test(mcp): lock test and save SSRF parity#1450

Open
floze-the-genius wants to merge 1 commit into
nextlevelbuilder:devfrom
floze-the-genius:fix/1070-mcp-ssrf-parity
Open

test(mcp): lock test and save SSRF parity#1450
floze-the-genius wants to merge 1 commit into
nextlevelbuilder:devfrom
floze-the-genius:fix/1070-mcp-ssrf-parity

Conversation

@floze-the-genius

Copy link
Copy Markdown

Summary

  • make MCP tool discovery injectable at the HTTP handler boundary while keeping mcp.DiscoverTools as the production default
  • prove /v1/mcp/servers/test and create/save return the same SSRF validation decision and error for local/private destinations
  • cover explicit private-host allowlisting, non-exempt cloud metadata, and normal public URL acceptance without real network calls

Closes #1070

Type

  • Feature
  • Bug fix
  • Hotfix (targeting main)
  • Refactor
  • Docs
  • CI/CD

Target Branch

dev

Checklist

  • go build ./... passes
  • go build -tags sqliteonly ./... passes
  • go vet ./... passes
  • Tests pass: go test -race -timeout=5m ./...
  • Web UI builds: cd ui/web && pnpm install --frozen-lockfile && pnpm build
  • No hardcoded secrets or credentials
  • SQL queries use parameterized $1, $2 (N/A: no SQL changes)
  • New user-facing strings added to all 3 locales (N/A: no user-facing strings)
  • Migration version bumped in internal/upgrade/version.go (N/A: no migration)

Test Plan

  • default policy rejects localhost, loopback, RFC1918, and cloud metadata before discovery or persistence
  • test and save expose the same validation error for each blocked URL
  • an explicitly allowlisted loopback host is accepted by both flows
  • cloud metadata remains blocked even when listed
  • a public IP URL is accepted by both flows

Surface parity: Web UI N/A because the API contract and UI behavior are unchanged.
Surface parity: CLI/runtime package N/A because this only locks existing HTTP validation semantics with regression coverage.

@clark-cant clark-cant left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review: Approve ✅nnClean test PR: injects MCP discovery for testability and adds comprehensive SSRF parity regression coverage for private/metadata rejection, explicit allowlisting, and public URL acceptance. Low risk; no production behavior change. Mandatory gates clear; no critical/important findings. CI is currently pending (UNSTABLE), so merge after required checks pass. Closes #1070

@clark-cant clark-cant left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maintainer Review — github-maintain

Verdict: APPROVED

Assessment

Gate Result
Duplicate / prior work No duplicate — #1248 added the allowlist mechanism, this PR adds test parity for test/save SSRF validation
Project standards Clean — follows existing patterns, no standards violations
Strategic necessity High — locks regression semantics for SSRF parity between test and save flows, directly closes #1070

Code review

internal/http/mcp.go (+7/-1)

  • Makes mcp.DiscoverTools injectable via discoverTools field on MCPHandler — clean dependency injection pattern
  • NewMCPHandler defaults to production mcp.DiscoverTools — zero behavioral change for callers
  • Signature matches the real function — no type drift

internal/http/mcp_tools.go (+1/-1)

  • One-line change: mcpbridge.DiscoverTools(...)h.discoverTools(...) — correct, minimal

internal/http/mcp_ssrf_parity_test.go (+195)

  • 4 test cases covering the full SSRF decision matrix:
    1. Private/loopback/metadata URLs rejected consistently by both test and save
    2. Explicit allowlist works for both flows
    3. Cloud metadata (169.254.169.254) blocked even when allowlisted — correct security invariant
    4. Public URLs accepted by both flows
  • Discovery never fires for blocked URLs — verified via counter
  • Store never creates servers for blocked URLs — verified via recorder
  • Test/save errors are compared for equality — this is the actual parity assertion

Quality

  • Correctness: Solid. The injectable discoverTools is the right approach — avoids network calls in tests while proving the validation path is identical.
  • Security: No regressions. Cloud metadata remains blocked even with allowlist — this is the correct hard invariant.
  • Maintainability: Clean. Test helpers are well-structured, no magic values.
  • Scope: Tight. 3 files, +203/-2, no scope creep.

Notes

  • CI shows UNSTABLE with no checks reported yet — this is likely a transient state for a new PR. The code itself is sound.
  • No labels on the PR — suggest adding area:mcp, area:security, maintain:triaged.

Closes #1070.

Reviewed by github-maintain

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.

GoClaw MCP test succeeds but save fails for local/private MCP URLs due to SSRF validation mismatch

2 participants