security: fix open redirect and add small hardening#1227
Merged
Conversation
…g-slash middleware A request like `//evil.com/` was parsed by Go's request-URI parser as a path (leading `//` is not treated as authority when viaRequest=true), trimmed to `//evil.com`, and emitted as `Location: //evil.com` — which browsers follow off-host. Use `path.Clean` for the redirect target so any leading `//` is collapsed to `/` (and the trailing slash is still removed). Also covers the percent-encoded variant (`/%2Fevil.com/`), which the URL parser decodes to `//evil.com/` before the middleware sees it. Fixes GHSA-v8vw-gw5j-w7m6. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The /v0/auth/http endpoint fetched `https://<domain>/.well-known/mcp-registry-auth`
on a domain that only had to match a permissive regex. The regex accepted IP
literals (`127.0.0.1`, `169.254.169.254`, `10.0.0.5`) and single-label names
(`localhost`, `kubernetes`), and the underlying http.Client had no private-IP
guard. An unauthenticated attacker could probe internal HTTPS services from
inside the registry's network namespace; distinct error messages for
DNS-fail / non-200 / body-too-large gave a reachability oracle.
Two layers of defence:
- IsValidDomain now rejects IP literals and single-label names. This is
the cheap pre-flight filter and also covers the DNS verification path
(where TXT lookups for internal zones gave a similar oracle).
- DefaultHTTPKeyFetcher's transport now uses a custom DialContext that
resolves the host once and refuses to dial loopback, RFC1918, ULA,
link-local, link-local-multicast, or unspecified addresses. Dialling
the resolved IP directly also pins against DNS rebinding (a TOCTOU
where a domain validator sees a public IP and the actual dial sees an
internal one). TLS SNI / Host headers continue to use the requested
hostname since http.Transport sets them from the request URL.
The two `single part domain` test cases (using `localhost`) were exercising
the now-disallowed semantics and were removed; the equivalent pattern-derivation
logic is still covered by the `hyphenated domain` and `subdomain` cases.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ontext The SSRF guard introduced in the previous commit returned the first dial result for any non-blocked IP — success or failure. If DNS returned a stale public AAAA record (or any individually-unreachable address) before a working A record, auth failed where the default transport would have recovered by trying the next answer. Iterate through every non-blocked address; only report failure when either every IP was blocked (no public address resolved) or every public IP failed to dial. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Only ReadHeaderTimeout was set, leaving the registry vulnerable to slow-read DoS (clients trickling the request body) and connection hoarding via long-lived idle keep-alives. - ReadTimeout=30s bounds the full request read. - IdleTimeout=120s caps keep-alive connection holding. WriteTimeout is intentionally NOT set: the publish path runs outbound package validators sequentially (npm/pypi/nuget up to 10s each, OCI up to 30s), and a tight cap could cut off a legitimate multi-package publish mid-response — surfacing as a truncated read to the publisher even when the DB commit succeeded. Slow-response-read DoS is bounded upstream by NGINX ingress timeouts and the per-IP rate limit (180 rpm). Revisit once validators are parallelised or per-request package counts are bounded. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Defence-in-depth for the registry UI at `/`. The page renders
publisher-controlled content (server names, descriptions, repository URLs);
server-side validation plus the JS escape function in ui_index.html are the
primary XSS defences, and these headers limit the blast radius if either is
bypassed.
- X-Content-Type-Options: nosniff — prevent MIME sniffing
- X-Frame-Options: DENY — block clickjacking framing
- Content-Security-Policy — restrict script/style/connect/frame sources
to self plus the documented Tailwind CDN
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
huma.Error5xx(detail, err) serializes the wrapped err into the response body, which surfaces underlying pgx/SQL driver text (table/column hints, internal hostnames in connection failures) to unauthenticated callers on list/detail endpoints and to authenticated callers on edit/status flows. Log the wrapped error server-side (with the relevant key — server name, version) and return a sanitized message. 4xx responses that intentionally relay user-actionable validator output are unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The list endpoint wrapped filter.SubstringName as `%<input>%` and passed it to ILIKE without escaping. A `_` in the input matched any single character and `%` matched everything, letting a caller distort or enumerate the server-name space (`?search=_` returns all 1-char names, `?search=%` returns the whole table). Not SQL injection (still parameterized) and results are paginated, so impact is filter shaping rather than data exfiltration — but trivially fixable. Replace `\` `%` `_` with their escaped forms and add ESCAPE '\' to the ILIKE clause. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…mplifier ServerJSON.Version and Package.Version had only minLength on their JSON Schema annotations. The DB column is VARCHAR(255), so a 1MB version string would still fail at the DB layer — but only after the request had paid for JSON parse, validation, advisory-lock acquisition, and the registry-ownership fetch (npm/pypi). At pool capacity, that path is a cheap DoS amplifier. Add maxLength:"255" on both Go fields so huma rejects the request before any of that work happens. The OpenAPI spec already declared this on ServerJSON.Version; openapi.yaml is updated to match for Package.version, and server.schema.json is regenerated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
pypi.go and nuget.go interpolated pkg.Identifier and version into URL templates via fmt.Sprintf without url.PathEscape. An identifier such as \"foo/../bar\" normalised under url.URL parsing to a fetch against an unrelated package's metadata; an identifier with embedded \"?\" or \"#\" truncated the path. The validator could end up checking the wrong package for the publisher's mcpName claim. Validator inputs already pass the publish-time identifier regex, but defence-in-depth at the URL boundary costs nothing. Applied to PyPI's /pypi/<id>/<ver>/json fetch, NuGet's package-base index.json fetch, and the NuGet README URL template substitution. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The MCPB validator HEADs the publisher-supplied package URL to confirm public accessibility. Default http.Client follows up to 10 redirects; the URL allowlist (github.com / gitlab.com) only constrains the FIRST hop. A 30x bouncing through CDN/release-asset hosts could otherwise be steered toward attacker infrastructure or internal-only endpoints reached via DNS quirks. Set CheckRedirect to ErrUseLastResponse and treat the resulting 3xx as \"accessible\" when a Location header is present (legitimate path: GitHub release assets serve a 302 to a signed S3 URL; we don't need to follow to confirm reachability). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
log.Printf calls in the v0 handlers include url.PathUnescape'd server names and versions via %s. A path parameter containing %0A (newline) or terminal escape codes would split the log line, letting an attacker forge synthetic log entries seen by SIEM/log readers. Switch to %q so user input is always quoted unambiguously. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two gaps reported by the deep-review pass:
- General multicast (admin-scoped 239/8 and ff00::/8) was not blocked.
IsLinkLocalMulticast covered only 224.0.0/24 and ff02::/16. Replace
that narrower check with the broader IsMulticast.
- Carrier-Grade NAT (RFC 6598, 100.64.0.0/10) is not classified by any
Go stdlib Is* helper but is reachable on some cloud / mobile networks
where it shadows internal infrastructure. Add an explicit CIDR check.
Tests cover the new positive and negative boundaries (100.63.x and
100.128.x are still allowed).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
handle404 returned application/problem+json without nosniff. Sibling to the / handler set in b5dc495 — defence-in-depth in case the body is ever rendered in a context that does MIME sniffing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
GenerateTokenResponse rejected JWT issuance when the candidate token held publish permission for \"<blocked>/test\". DNS-issued tokens for a subdomain claimant carry both \"com.example/*\" (slash) and \"com.example.*\" (dot-wildcard) patterns; an attacker who proves DNS ownership of mailer.evil.com but not evil.com receives only the dot-wildcard form (\"com.evil.mailer.*\"), which strings.HasPrefix against \"com.evil/test\" fails to match — the block silently fails open exactly where it is intended to apply. Probe both \"<blocked>/test\" and \"<blocked>.test/x\" so subdomain issuances are covered. BlockedNamespaces is empty in prod today but this closes the gap before it gets populated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rdimitrov
changed the title
…working The CSP added in the previous commit set connect-src 'self', which silently blocked the page's prod / staging / custom base-URL selector — the whole point of which is to issue cross-origin XHRs from a single static page. Switch to connect-src *. The other directives (script-src, style-src, frame-ancestors, base-uri, form-action, default-src) still meaningfully limit the page's attack surface; connect-src isn't useful as a control when the feature it would constrain is by design cross-origin. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The fall-through loop tried each non-blocked address in DNS-return order under a single shared 10s http.Client budget. A hanging first address (stale AAAA, broken route, blackholed IP) could consume most of the budget before we ever tried the next answer, surfacing as a generic auth failure where Go's default Happy Eyeballs would have raced the addresses and recovered. Wrap each per-IP DialContext in a 3s context.WithTimeout. We give up fast on a hung address and try the next one, keeping the overall budget useful for the address that actually answers. Simpler than implementing true Happy Eyeballs and sufficient for the well-known fetcher's use. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…or schema The Go struct tag and openapi.yaml were updated to maxLength: 255 in an earlier commit, but the JSON schema embedded for use by /v0/validate was missed. This left the CLI / schema-driven validation paths without the bound that the API layer enforces, so a 1MB version string would slip past /validate even though it would be rejected at publish time. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
rdimitrov
added a commit
that referenced
this pull request
Apr 29, 2026
## Summary - Bumps prod imageTag from `1.7.4` to `1.7.5`. - Promotes the security release tagged at v1.7.5 (#1227 merged at 1201cbd). ## Test plan - [ ] Smoke staging at https://staging.registry.modelcontextprotocol.io — already done, all checks green - [ ] Roll out via Pulumi - [ ] Confirm prod health post-rollout: `curl https://registry.modelcontextprotocol.io/v0/health` 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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 this does
Fixes an open redirect plus a handful of small, mostly-invisible hardening changes around it.
What's in it
//evil.com/redirect off-host.ReadTimeoutandIdleTimeouton the HTTP server (noWriteTimeout— would risk cutting off legitimate multi-package publishes)./) and 404 responses.%and_as wildcards.Versionfield bounded to 255 chars; logs quote user input;BlockedNamespacesdenylist now catches subdomain claimants.Each change is its own commit so they can be reviewed (or reverted) independently.
Test plan
/after deploy to confirm UI still loads with the new CSPcurl https://<host>//evil.com/— confirm the open redirect is closed🤖 Generated with Claude Code