Repository navigation
feat(registries): harden registry fetches against SSRF (CWE-918, MCP-1076) - #745
Merged
Merged
Conversation
…1076) Registry server-list fetches use a user-supplied URL (`registry add-source`), which CodeQL flags as go/request-forgery (#97/#98): a malicious or typo'd registry source could point the daemon at internal/metadata endpoints (169.254.169.254, RFC1918 ranges). Add an SSRF guard centered on a single `isBlockedIP` predicate (loopback, RFC1918, RFC6598 CGNAT, link-local incl. the cloud-metadata endpoint, IPv6 unique-local, unspecified, multicast; fails closed), applied at three layers: - dial time: a net.Dialer Control hook on the shared registry HTTP client rejects the actual resolved IP before connect — covers hostnames that resolve into blocked ranges and closes the DNS-rebinding TOCTOU window a parse-time check alone cannot. - fetch pre-flight: validateRegistryURL rejects a literal-IP host (defense in depth + an explicit barrier at the http.NewRequest sink). - add-source/edit-source: literal-IP fail-fast (no DNS, stays pure/offline) so a bad source is refused up front with the stable invalid_registry_url code. Opt out with a new top-level `allow_private_registry_fetch` config flag (default false) for operators who run a trusted registry mirror on an internal/private address. TDD: table tests for every blocked/allowed range (incl. CGNAT and RFC1918 boundaries), an end-to-end loopback dial block, add-source literal-IP rejection, and the two registry-add E2E daemon tests opt in via the new flag.
Deploying mcpproxy-docs with
|
| Latest commit: |
1ae8c20
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d4c1ef35.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://security-mcp-1076-registry-s.mcpproxy-docs.pages.dev |
The in-process server tests that fetch a registry served on a loopback httptest server (consistency_official, mcp_add_from_registry's startTestRegistry helper) now hit the new SSRF dial guard, because the registries-package test bypass does not apply to the server package's test binary. Set AllowPrivateRegistryFetch on their SetRegistriesFromConfig configs — exactly how an operator opts in for a trusted internal mirror — so the fetch is permitted while the production guard stays active for the add-source rejection test. Fixes the linux/arm64 Build Binaries leg (TestCrossSurfaceConsistency_*, TestHandleUpstreamServers_AddFromRegistry_*).
Member
Author
|
Changes requested — SSRF guard blocks loopback, breaking |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 27937618138 --repo smart-mcp-proxy/mcpproxy-go
|
CodexReviewer (round 2) found a real gap: the registry client clones http.DefaultTransport, which keeps Proxy: ProxyFromEnvironment. With HTTP_PROXY/HTTPS_PROXY set, the dialer connects to the PROXY as the first hop, so registryDialControl only ever validates the proxy's IP — never the real target. A hostname resolving to loopback/private/link-local (incl. 169.254.169.254) could still be fetched through a proxy. Add an application-layer guard (guardRegistryTargetHost) that resolves the target host and rejects the fetch if ANY resolved IP is in a blocked range. It runs before the request in registryGet, independent of the transport/proxy, so it holds in the proxy case; the dial-time Control stays as defense-in-depth for the direct path and DNS-rebind TOCTOU. Fail-open on resolver error (a flaky resolver must not break every fetch); literal IPs already covered pre-flight; relaxed by the allow_private_registry_fetch opt-in. Regression coverage: a public-looking hostname resolving to 169.254.169.254 is refused with HTTP(S)_PROXY set; plus direct predicate tests (mixed public+private resolution blocked, all-public allowed, resolver-error fail-open, public literal allowed). Resolver is injectable via a package var for deterministic, network-free tests.
![mcpproxy-gatekeeper[bot]](https://avatars.githubusercontent.com/u/216428285?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Gatekeeper approval — MCP-1076 SSRF hardening. KimiReviewer verdict ACCEPT on head 1ae8c20 (model-diverse): three-layer defense (literal-IP pre-flight + app-layer DNS resolution that holds under HTTP_PROXY + dial-time control for DNS-rebinding), with a proxy-bypass regression test covering exactly the path CodexReviewer flagged in round 1. CI fully green (arm64 + Unit + E2E + CodeQL). Deny-by-default; opt-in via allow_private_registry_fetch. Author (BackendEngineer) ≠ approver.
Dumbris
added a commit
that referenced
this pull request
Jun 24, 2026
The allow_private_registry_fetch flag is all-or-nothing: enabling it lifts the registry SSRF guard for every non-routable range at once, including the 169.254.169.254 cloud-metadata endpoint. Enabling it for a localhost dev registry therefore also re-opens the cloud-metadata SSRF vector. Document this sharp edge in the config doc comment, docs/configuration.md, and docs/registries.md, plus note the flag only takes effect on daemon (re)start / config reload. Related #745 Related MCP-3206
Dumbris
added a commit
that referenced
this pull request
Jul 3, 2026
…751) The allow_private_registry_fetch flag is all-or-nothing: enabling it lifts the registry SSRF guard for every non-routable range at once, including the 169.254.169.254 cloud-metadata endpoint. Enabling it for a localhost dev registry therefore also re-opens the cloud-metadata SSRF vector. Document this sharp edge in the config doc comment, docs/configuration.md, and docs/registries.md, plus note the flag only takes effect on daemon (re)start / config reload. Related #745 Related MCP-3206
nlaurance
pushed a commit
to nlaurance/mcpproxy-go
that referenced
this pull request
Aug 12, 2026
…ytes/ResponseBytes) (smart-mcp-proxy#553) * docs(069): Observability — usage statistics & graphs in Web UI spec Surface existing activity-log data (per-call server/tool/status/duration/bytes) as Web UI graphs so users see where agents spend tokens: per-tool call histogram, response-size token-sink ranking, error rate, latency, timeline, behind a dashboard switcher. Fast via actor-owned incremental stats aggregate + TTL cache (no per-request full scan). v1 uses bytes as token proxy; accurate per-call tokens deferred (FR-010). Roadmap PILLAR A. Paperclip goal d7164fdf. * docs(069): plan, research, data-model, contract & tasks for usage graphs Run /speckit.plan + /speckit.tasks. Codebase verification corrected three spec assumptions: ActivityRecord stores no byte sizes (capture two int fields at the write path), chart.js/vue-chartjs already installed (reuse, no library selection), and /activity/stats does not exist (add /activity/usage). Aggregation is actor-owned + incremental + snapshot + TTL cache + cold-start rebuild (CN-002/CN-003/FR-005). Cross-lane decomposition: backend streams A1->A2->A3 (my lane), frontend B1/B2 delegated, blocked by A3. Related smart-mcp-proxy#745 Co-Authored-By: Paperclip <noreply@paperclip.ing> * feat(069-A1): add RequestBytes/ResponseBytes to ActivityRecord with pre-truncation capture - Add `request_bytes` and `response_bytes` (omitempty int) fields to ActivityRecord; legacy records missing the fields decode to 0. - Populate both fields in ActivityService.handleToolCallCompleted from event payload. - Emit pre-truncation byte sizes in EmitActivityToolCallCompleted and its server wrappers; compute rawByteSize(result) and rawByteSize(activityArgs) before forwardContentResult is called so truncated responses still report full sizes. - Fix ListActivities to copy the new fields when constructing result records. - TDD: TestActivityRecord_ByteFieldsRoundTrip (T001) and TestHandleToolCallCompleted_ByteCapture (T003) written before implementation. Co-Authored-By: Paperclip <noreply@paperclip.ing> * test(069-A1): end-to-end pre-truncation byte-capture assertion (MCP-786) Dual-AI review of smart-mcp-proxy#553 flagged that TestHandleToolCallCompleted_ByteCapture only asserts the activity service stores values handed to it — it never exercises the A1 invariant that handleCallToolVariant measures rawByteSize on the RAW result/args BEFORE forwardContentResult truncates. Add TestRawByteSize_MeasuredBeforeTruncation (internal/server): drive an over-threshold response + args through the real measure-then-truncate sequence and assert (a) RequestBytes/ResponseBytes equal the full pre-truncation json.Marshal size, (b) the forwarded body is actually truncated and shorter than the raw response, (c) the captured ResponseBytes strictly exceeds the post-truncation payload — proving measurement precedes truncation. Also note the double-Marshal overhead on rawByteSize for future profiling (review's optional ask). Related: MCP-786, MCP-748, spec 069 --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
nlaurance
pushed a commit
to nlaurance/mcpproxy-go
that referenced
this pull request
Aug 12, 2026
…oxy#560) * feat(069-A2): actor-owned usage aggregate + persistence Spec 069 Stream A2 (T005–T010): in-memory usage rollup owned by the ActivityService goroutine, published to readers as a copy-on-write snapshot via atomic pointer, persisted to BBolt with cold-start load-or-rebuild. - UsageAggregate/ToolUsage/TimeBucket + incremental Apply (internal/runtime/usage_aggregate.go) - UsageStore: atomic-pointer COW snapshot; lock-free, non-blocking reads - ActivityService owns it: Apply on save in handleEvent, UsageSnapshot() reader, periodic 30s flush + flush-on-shutdown, cold-start load-or-rebuild (one full scan) - activity_stats BBolt bucket (versioned key) + byte-oriented persist/load + ScanAllActivities - observability.usage_cache_ttl (5s) + usage_persist_interval (30s) config: defaults + hot-reload - docs (configuration.md) + spec trace (tasks T005–T010, data-model A2 notes) TDD: aggregate math, snapshot/reads-never-block, persistence round-trip, cold-start rebuild vs load, and config defaults all covered. Full internal/runtime -race suite green (approval-hash canary safe). Related smart-mcp-proxy#745 Co-Authored-By: Paperclip <noreply@paperclip.ing> * perf(069-A2): make UsageStore.Apply O(1) on the activity hot path Apply previously cloned the entire usage aggregate (every tool + every time bucket) on every activity write via publishLocked, making the write hot path O(tools×buckets) instead of O(1) — violating spec 069 CN-002 ("aggregate update O(1) per activity write; must not block the request hot path"). Decouple publish from write: Apply now mutates the working aggregate under a short writer lock and only marks the snapshot stale (atomic dirty flag, O(1), no clone). The clone is deferred to Snapshot (publish-on-read): the first reader after a write burst materializes one fresh snapshot off the hot path; reads with no pending writes stay lock-free. The A3 endpoint and the 30s persist flush are the only readers, so clones are rare relative to writes. Test-first (ENG-1): TestUsageStore_ApplyDoesNotPublishPerWrite asserts 500 writes trigger zero publishes and exactly one clone on first read; BenchmarkUsageStore_Apply shows 1 alloc/op with 1000 primed tools (was O(tools) allocs/op). Existing snapshot/replace contract tests and the full -race runtime suite stay green. Related smart-mcp-proxy#560 Related MCP-835 * fix(069-A2): count blocked attempts + wire observability hot-reload Addresses CodexReviewer findings smart-mcp-proxy#2 and smart-mcp-proxy#3 on PR smart-mcp-proxy#560 (the O(1) fix cleared smart-mcp-proxy#1 once CI went green). smart-mcp-proxy#2 — blocked tool attempts were missing from the usage aggregate. They are persisted as blocked `policy_decision` records, but `Apply` dropped all non-tool_call records and `handlePolicyDecision` never fed the aggregate, so the contract's per-tool `blocked` field was permanently 0. `Apply` now also folds blocked policy_decisions: a blocked attempt never executed, so it increments only `Blocked` + `LastUsed` — not `Calls`, latency, bytes, or the executed-call timeline. `handlePolicyDecision` calls `usage.Apply` on save success so the live path matches a cold-start rebuild-from-scan. Extracted a `tool()` get-or-create helper. smart-mcp-proxy#3 — `observability.usage_persist_interval` claimed hot-reload but was only read at construction. `DetectConfigChanges` now flags an `observability` change and `ApplyConfig` pushes the new cadence into the running ActivityService via `SetUsagePersistInterval` (the flush loop already re-reads the interval each cycle). Test-first (ENG-1): aggregate counts blocked-only (not Calls/latency/ timeline); live `handlePolicyDecision` folds blocked into the snapshot; `DetectConfigChanges` detects observability as hot-reloadable; end-to-end `ApplyConfig` applies the new interval to a running runtime. Repointed the "ignores non-tool_calls" test to a non-blocked decision. Contract documents `blocked` semantics. Full internal/runtime+config+storage -race green; lint 0; personal+server builds. Related smart-mcp-proxy#560 Related MCP-835 Co-Authored-By: Paperclip <noreply@paperclip.ing> --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
nlaurance
pushed a commit
to nlaurance/mcpproxy-go
that referenced
this pull request
Aug 12, 2026
Serve the actor-owned usage aggregate (spec 069 A2) over REST so the Web UI usage graphs (B1/B2) have a contract to consume. Reads the in-memory snapshot only — never a per-request full-log scan (SC-005). - contracts: UsageAggregateResponse + UsageToolStat / UsageOtherBucket / UsageTimeBucket. - handler: handleActivityUsage with window (24h/7d/all), server/tool/status membership filters (FR-008), sort (calls/resp_bytes/error_rate/p95), top-N + 'other' fold, empty-state 200 (FR-009), 400 on bad enum/top. avg_* computed over sized (non-zero-byte) calls only; tokens_saved echoed from ServerTokenMetrics (FR-007). Short-TTL read cache keyed on the query honours observability.usage_cache_ttl (FR-005, hot-reloaded per request). - controller: UsageSnapshot() exposed through server -> runtime -> ActivityService. - oas: swag-generated path + schemas; verify-oas-coverage passes. Windowing note: per-tool metrics are lifetime-cumulative (the A2 aggregate keeps cumulative per-tool rollups + global hourly timeline buckets, no per-tool-per-window data); window scopes the timeline span and filters the tool list to tools active within it. Exact per-tool windowed counts are a deferred follow-on (would need per-tool time buckets in the aggregate). Tests: TestActivityUsage_* (ranking/error_rate/avg-excludes-0-byte/window/ top-N/filters/empty-state/400) replay records through the real Apply path; TestActivityUsage_NoFullScanPerRequest is the SC-005 perf assertion. Related smart-mcp-proxy#745
nlaurance
pushed a commit
to nlaurance/mcpproxy-go
that referenced
this pull request
Aug 12, 2026
…mart-mcp-proxy#570) Spec 069 Stream B1 (T016–T017). Adds the Overview↔Usage switcher to the dashboard and the typed client for the A3 GET /api/v1/activity/usage endpoint (smart-mcp-proxy#565). - api: getActivityUsage(params) forwards only supplied filters (window/server/tool/status/top/sort); UsageAggregateResponse + UsageToolStat/UsageOtherBucket/UsageTimeBucket types mirror the contract. - Dashboard: tabbed Overview↔Usage switcher rendered with v-show (never v-if) so Overview state survives a switch-back (SC-006). Usage panel has a 24h/7d/all window selector, tokens-saved headline (FR-007), loading/error/empty states (FR-009) and a baseline per-tool rollup table; the rich charts arrive in B2 (T018–T022). - Lazy load: the aggregate is fetched on first Usage activation and on window change only, so the Overview first paint is never blocked (SC-004). - Tests (TDD): activity-usage.spec (client param forwarding) + dashboard-usage-switcher.spec (default tab, lazy fetch, v-show preserve, window refetch). All 91 frontend unit tests green; vue-tsc clean. Related smart-mcp-proxy#745
nlaurance
pushed a commit
to nlaurance/mcpproxy-go
that referenced
this pull request
Aug 12, 2026
…p-proxy#571) Spec 069 Stream B2 (frontend). Surfaces the activity-log usage aggregate (GET /api/v1/activity/usage, A3) as Web UI graphs. - usage/CallHistogram.vue, ResponseSizeRanking.vue (size-based token-sink proxy, FR-006), ErrorRateChart.vue (+ per-tool p50/p95 latency table), Timeline.vue (calls/errors stacked, honors window/filters, FR-008). - views/Usage.vue: tokens-saved headline from ServerTokenMetrics (FR-007), window selector (24h/7d/all), status/sort filters, empty/low-data state (FR-009), 30s auto-refresh against the cached snapshot. - Dashboard.vue: Overview<->Usage switcher (T016); Overview kept via v-show so its state survives a round-trip (SC-006); Usage code-split via defineAsyncComponent so the chart bundle/fetch don't block first paint (SC-004). - api.ts getActivityUsage() (T017) + UsageAggregateResponse types. - utils/usageFormat.ts shared formatters + palette. Covers T016-T023. Verified with a Playwright sweep (7/7, 0 console errors) against populated + empty instances; report kept local per QA-report policy. Related smart-mcp-proxy#745
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Hardens registry URL fetches against SSRF / CodeQL
go/request-forgery(alerts #97/#98, MCP-1076). Registry server-list fetches use a user-supplied URL (registry add-source), so a malicious or typo'd source could point the daemon at internal/metadata endpoints (169.254.169.254, RFC1918 ranges).How
A single
isBlockedIPpredicate (newinternal/registries/ssrf.go) is the source of truth for blocked ranges: loopback, RFC1918 private, RFC6598 CGNAT (100.64/10), link-local incl. the cloud-metadata endpoint, IPv6 unique-local, unspecified, multicast — fails closed on an unparseable IP. It is enforced at three layers:net.Dialer.Controlhook on the shared registry HTTP client checks the actual resolved IP before connect. This is the authoritative guard: it catches hostnames that resolve into blocked ranges and closes the DNS-rebinding TOCTOU window a parse-time check alone cannot.validateRegistryURLrejects a literal-IP host (defense in depth + an explicit barrier at thehttp.NewRequestsink, alongside the existing scheme/host pin).invalid_registry_urlcode.Opt-out
New top-level
allow_private_registry_fetchconfig flag (default false = secure). Settrueonly to run a trusted registry mirror on an internal/private address. Wired throughSetRegistriesFromConfigso config hot-reload keeps the dial guard in sync. This is the "optional allow/deny policy" called for in the issue.Tests (TDD)
TestIsBlockedIP— every blocked range + public allow cases incl. RFC1918/CGNAT boundaries + IPv6 + fail-closed nil.TestHostLiteralBlocked,TestValidateRegistrySourceURL— literal-IP rejection, hostname pass-through, opt-in bypass.TestRegistryGet_BlocksLoopbackWhenGuardActive— end-to-end: guarded fetch refused at dial time, endpoint never reached.TestBuildRegistrySourceEntry_RejectsSSRFLiteralIP— add-source fail-fast.Verified locally:
go test ./internal/registries/... ./internal/config/... -race, registry-add E2E daemon tests, add/edit-source unit tests, golangci-lint v2 (.github/.golangci.yml) 0 issues, gofmt,make swaggerregenerated.Notes
CodeQL #97/#98 are pre-existing alerts on
main(created by #572, surfaced — not introduced — by #594). This PR adds the mitigating guard; the alerts can be dismissed as mitigated once merged.Closes MCP-1076.