Skip to content

protect proxy config from ssrf - #5772

Open
akshaydeo wants to merge 9 commits into
devfrom
guard-mcp-client-connections-against-ssrf
Open

protect proxy config from ssrf#5772
akshaydeo wants to merge 9 commits into
devfrom
guard-mcp-client-connections-against-ssrf

Conversation

@akshaydeo

@akshaydeo akshaydeo commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes an SSRF vulnerability in the MCP HTTP/SSE client registration path. Previously, an unauthenticated caller (default-open posture, no admin password set) could register an HTTP or SSE MCP client pointing at loopback, private-network, link-local, or cloud metadata addresses (e.g. 169.254.169.254). Additionally, even for authenticated callers, the MCP HTTP transport fell back to the mcp-go library's own default HTTP client when no TLS config was provided, which carried no dial guard whatsoever.

Changes

  • Handler-layer gate (rejectPrivateMCPTargetIfAuthBypassed): Added a pre-registration check in addMCPClient that resolves the connection_string hostname and rejects the request with HTTP 403 if any resolved IP is non-public and the caller reached the endpoint with no credential check (BifrostContextKeyAuthBypassed). Authenticated callers retain the documented ability to point MCP clients at local or private servers.

  • New BifrostContextKeyAuthBypassed context key: Added to distinguish requests that were let through with no credential check at all from genuinely authenticated sessions (both previously set IsLocalAdminContextKey). The auth middleware sets this key in the fail-open branch. This scoping ensures only the unauthenticated path is restricted.

  • buildTLSHTTPClient always returns a guarded client: Removed the early return nil, nil when tlsCfg == nil. The function now always constructs a transport routed through network.PrivateNetworkDialContext, which blocks link-local and unspecified addresses at dial time regardless of TLS configuration. TLS customization is layered on top when provided.

  • network.PrivateNetworkDialContext: Added a new dialer variant that, unlike SSRFSafeDialContext, permits loopback and private-network targets (the documented primary use case for MCP HTTP clients) while still blocking link-local addresses (including 169.254.169.254) and unspecified addresses. Includes the same DNS-rebinding protection as SSRFSafeDialContext.

  • Integration test fix (TestMain): The MCP integration tests connect to loopback-bound httptest.Server instances. The new PrivateNetworkDialContext guard in buildTLSHTTPClient would have blocked these. TestMain now installs an unguarded dialer for the test package only via SetDialContextForTests, with a comment explaining the scope.

  • Documentation: Added a <Note> to docs/mcp/connecting-to-servers.mdx explaining that unauthenticated callers cannot register HTTP/SSE clients pointing at private addresses, and directing users to either enable auth or define the client in config.json.

Type of change

  • Bug fix
  • Feature
  • Refactor
  • Documentation
  • Chore/CI

Affected areas

  • Core (Go)
  • Transports (HTTP)
  • Providers/Integrations
  • Plugins
  • UI (React)
  • Docs

How to test

go test ./core/network/... ./core/mcp/... ./transports/bifrost-http/handlers/... ./core/internal/mcptests/...

Unauthenticated loopback rejected:
Start Bifrost with no admin password configured. Attempt to register an HTTP MCP client pointing at http://127.0.0.1:3001/mcp via POST /api/mcp/client. Expect HTTP 403.

Authenticated loopback allowed:
Start Bifrost with an admin password set and authenticate. Register the same client. Expect the registration to succeed.

Link-local blocked at dial time:
Attempt to register an HTTP MCP client pointing at http://169.254.169.254/latest/meta-data/ as an authenticated caller. The handler-layer check passes (public-IP check does not apply to authenticated callers), but the dial-time guard in PrivateNetworkDialContext blocks the connection.

Public targets unaffected:
Register an SSE MCP client pointing at a public internet address as an unauthenticated caller. Expect the registration to succeed.

Breaking changes

  • No

The only behavioral change for existing deployments is that MCP HTTP/SSE clients registered by unauthenticated callers pointing at private/loopback/link-local addresses are now refused. Authenticated callers and all STDIO clients are unaffected. Deployments with auth enabled see no change in behavior.

Security considerations

This PR addresses an SSRF vulnerability. Without this fix, an unauthenticated caller on a Bifrost instance running with the default-open posture (no admin password) could cause the gateway to dial arbitrary internal addresses by registering a crafted MCP HTTP/SSE client. The fix applies defense in depth at two layers: the HTTP handler (pre-registration IP check for unauthenticated callers) and the MCP transport dial function (link-local/unspecified block for all callers, including authenticated ones).

Checklist

  • I read docs/contributing/README.md and followed the guidelines
  • I added/updated tests where appropriate
  • I updated documentation where needed
  • I verified builds succeed (Go and UI)
  • I verified the CI pipeline passes locally if applicable

jeremym-tanium and others added 9 commits July 31, 2026 00:08
…iew maintenance (#5693)

* feat: support matview_refresh_interval "off" to disable logstore matview maintenance

The materialized views back only the dashboard UI. Deployments that run
Bifrost headless behind their own observability stack pay the REFRESH
MATERIALIZED VIEW CONCURRENTLY cost for views nothing reads, and the 5s
floor means the interval alone cannot turn maintenance off.

With "off" (or a non-positive duration) the logs store skips view
creation, the initial refresh, and the periodic refresher entirely.
matViewsReady stays false, so dashboard queries fall back to the raw
tables, and the runtime self-heal path cannot re-arm maintenance since
it only triggers from matview-path queries.

* fix: guard matview self-heal when maintenance is disabled

Review follow-up: carry the resolved disabled state onto the store so
triggerMatViewSelfHeal cannot recreate views the configuration says must
not exist, and make the schema/docs explicit that a zero duration also
disables (positive sub-5s values still clamp up).
<!-- This is an auto-generated comment: release notes by coderabbit.ai -->

## Summary by CodeRabbit

* **Bug Fixes**
  * Improved token parameter compatibility handling to preserve alternative formats when the primary option is unsupported.

* **Chores**
  * Version updated to 2.0.0.
  * Enhanced load testing configuration for more reliable builds.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary

Briefly explain the purpose of this PR and the problem it solves.

## Changes

- What was changed and why
- Any notable design decisions or trade-offs

## Type of change

- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI

## Affected areas

- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [ ] Docs

## How to test

Describe the steps to validate this change. Include commands and expected outcomes.

```sh
# Core/Transports
go version
go test ./...

# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```

If adding new configs or environment variables, document them here.

## Screenshots/Recordings

If UI changes, add before/after screenshots or short clips.

## Breaking changes

- [ ] Yes
- [ ] No

If yes, describe impact and migration instructions.

## Related issues

Link related issues and discussions. Example: Closes #123

## Security considerations

Note any security implications (auth, secrets, PII, sandboxing, etc.).

## Checklist

- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
## Summary

Adds a `THIRD_PARTY_NOTICES.md` file to formally document third-party components used in Bifrost that carry license terms requiring explicit attribution — specifically MPL-2.0 licensed dependencies and embedded source code derived from external projects.

## Changes

- Introduces `THIRD_PARTY_NOTICES.md` to attribute:
  - Embedded source code in `framework/migrator/migrator.go` derived from `go-gormigrate/gormigrate` (MIT)
  - Go binary dependencies carrying MPL-2.0 terms: `github.com/cyphar/filepath-securejoin` and `github.com/hashicorp/go-version`
  - npm build-time devDependencies carrying MPL-2.0 terms: `lightningcss` (never shipped to end users) and `dompurify` (Apache-2.0 option elected)
- All MPL-2.0 components are used unmodified and combined as a "Larger Work" per MPL-2.0 Section 3.3; no Bifrost source files are themselves MPL-licensed.

## Type of change

- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [x] Documentation
- [ ] Chore/CI

## Affected areas

- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [x] Docs

## How to test

No functional changes — review the file contents to confirm accuracy of license attributions against the listed upstream repositories.

## Breaking changes

- [ ] Yes
- [x] No

## Related issues

N/A

## Security considerations

This change has no security implications. It is a legal/compliance attribution document only.

## Checklist

- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
## Summary

Briefly explain the purpose of this PR and the problem it solves.

## Changes

- What was changed and why
- Any notable design decisions or trade-offs

## Type of change

- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI

## Affected areas

- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [ ] Docs

## How to test

Describe the steps to validate this change. Include commands and expected outcomes.

```sh
# Core/Transports
go version
go test ./...

# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```

If adding new configs or environment variables, document them here.

## Screenshots/Recordings

If UI changes, add before/after screenshots or short clips.

## Breaking changes

- [ ] Yes
- [ ] No

If yes, describe impact and migration instructions.

## Related issues

Link related issues and discussions. Example: Closes #123

## Security considerations

Note any security implications (auth, secrets, PII, sandboxing, etc.).

## Checklist

- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
## Summary

Briefly explain the purpose of this PR and the problem it solves.

## Changes

- What was changed and why
- Any notable design decisions or trade-offs

## Type of change

- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI

## Affected areas

- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [ ] Docs

## How to test

Describe the steps to validate this change. Include commands and expected outcomes.

```sh
# Core/Transports
go version
go test ./...

# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```

If adding new configs or environment variables, document them here.

## Screenshots/Recordings

If UI changes, add before/after screenshots or short clips.

## Breaking changes

- [ ] Yes
- [ ] No

If yes, describe impact and migration instructions.

## Related issues

Link related issues and discussions. Example: Closes #123

## Security considerations

Note any security implications (auth, secrets, PII, sandboxing, etc.).

## Checklist

- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
## Summary

Briefly explain the purpose of this PR and the problem it solves.

## Changes

- What was changed and why
- Any notable design decisions or trade-offs

## Type of change

- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI

## Affected areas

- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [ ] Docs

## How to test

Describe the steps to validate this change. Include commands and expected outcomes.

```sh
# Core/Transports
go version
go test ./...

# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```

If adding new configs or environment variables, document them here.

## Screenshots/Recordings

If UI changes, add before/after screenshots or short clips.

## Breaking changes

- [ ] Yes
- [ ] No

If yes, describe impact and migration instructions.

## Related issues

Link related issues and discussions. Example: Closes #123

## Security considerations

Note any security implications (auth, secrets, PII, sandboxing, etc.).

## Checklist

- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
## Summary

Bumps several Go dependencies to their latest patch/minor versions across all modules in the repository.

## Changes

- `github.com/aws/aws-sdk-go-v2/service/s3`: `v1.97.3` → `v1.99.0`
- `github.com/aws/aws-sdk-go-v2/config`: `v1.32.11` → `v1.32.14`
- `github.com/aws/aws-sdk-go-v2/internal/ini`: `v1.8.5` → `v1.8.6`
- `github.com/weaviate/weaviate`: `v1.36.5` → `v1.38.0`
- `github.com/buger/jsonparser`: `v1.1.2` → `v1.2.0`
- `github.com/go-openapi/spec`: `v0.22.2` → `v0.22.3`
- `github.com/google/cel-go`: `v0.28.1` → `v0.29.0`
- `github.com/stretchr/objx`: `v0.5.3` added as an indirect dependency

## Type of change

- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [x] Chore/CI

## Affected areas

- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [ ] UI (React)
- [ ] Docs

## How to test

```sh
go test ./...
```

## Breaking changes

- [ ] Yes
- [x] No

## Related issues

## Security considerations

None. All changes are dependency version bumps with no security-sensitive modifications.

## Checklist

- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [ ] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
@akshaydeo
akshaydeo marked this pull request as ready for review August 2, 2026 08:13

Copy link
Copy Markdown
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Summary by CodeRabbit

  • Security

    • Improved protection for MCP HTTP and SSE connections against unsafe private, link-local, metadata, and unspecified network targets.
    • Unauthenticated requests to restricted network destinations are rejected, while authenticated and public connections continue to work.
    • Loopback connections remain supported for permitted scenarios.
  • Documentation

    • Added guidance on authentication requirements for connecting to private and local MCP servers.

Walkthrough

The PR adds private-network SSRF protection for MCP HTTP and SSE connections. It introduces guarded dialing, rejects unauthenticated private targets, records authentication bypass state, adds regression tests, and documents configuration requirements.

Changes

MCP SSRF protection

Layer / File(s) Summary
Private-network dialer
core/network/ssrf.go, core/network/ssrf_test.go
Adds PrivateNetworkDialContext, which blocks unspecified and link-local addresses while allowing loopback and private destinations.
Guarded MCP HTTP client
core/mcp/clientmanager.go, core/mcp/clientmanager_test.go, core/internal/mcptests/setup_test.go
Configures MCP HTTP clients with guarded dialing and a 10-second timeout. Tests cover blocked metadata targets and successful loopback connections.
Unauthenticated target validation
core/schemas/bifrost.go, transports/bifrost-http/handlers/middlewares.go, transports/bifrost-http/handlers/mcp.go, transports/bifrost-http/handlers/mcp_ssrf_test.go, docs/mcp/connecting-to-servers.mdx
Marks authentication bypasses and rejects unauthenticated HTTP or SSE registrations targeting non-public addresses. Tests and documentation cover the new behavior.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant AuthMiddleware
  participant addMCPClient
  participant PrivateNetworkDialContext
  participant MCPServer
  Client->>AuthMiddleware: Submit MCP registration
  AuthMiddleware->>addMCPClient: Forward authentication state
  addMCPClient->>PrivateNetworkDialContext: Validate target
  PrivateNetworkDialContext-->>addMCPClient: Allow or reject address
  addMCPClient->>MCPServer: Create allowed MCP connection
Loading

Possibly related PRs

Suggested reviewers: pratham-mishra04, danpiths, bearts

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR adds SSRF protections but does not implement the linked issue's requested Files API support for providers such as OpenAI or Anthropic [#123]. Implement the requested Files API support, or link the PR to an issue that covers MCP SSRF protection.
Out of Scope Changes check ⚠️ Warning The SSRF protection changes are unrelated to the linked issue's Files API objective [#123]. Remove the SSRF-related changes or update the linked issues to include the proxy SSRF protection objective.
✅ Passed checks (3 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check ✅ Passed The title clearly identifies the main change: protecting proxy configuration from SSRF.
Description check ✅ Passed The description is complete and covers the purpose, changes, testing, affected areas, security impact, and checklist.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch guard-mcp-client-connections-against-ssrf

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 4

🧹 Nitpick comments (5)
transports/bifrost-http/handlers/mcp.go (1)

642-644: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

The lookup blocks the request thread for up to 5 seconds.

LookupIP runs synchronously on the fasthttp handler goroutine with a 5-second budget. An unauthenticated caller controls the hostname, so the caller controls the delay. Repeated registration attempts against a deliberately slow resolver tie up handler goroutines cheaply. Consider a shorter budget, for example 2 seconds, which is still generous for a resolver on the request path.

🤖 Prompt for 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.

In `@transports/bifrost-http/handlers/mcp.go` around lines 642 - 644, Reduce the
DNS lookup timeout in the hostname validation flow using context.WithTimeout
before net.DefaultResolver.LookupIP from 5 seconds to 2 seconds, preserving the
existing cancellation and error-handling behavior.
core/network/ssrf.go (2)

160-168: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Consider dialing each validated address, not only ips[0].

LookupIP with network "ip" returns both IPv6 and IPv4 records. The code validates every returned address but dials only the first one. If the first address is unreachable — for example ::1 on a host or container with IPv6 disabled — the dial fails even though a working 127.0.0.1 record was returned. localhost MCP servers are the documented primary use case for this dialer, so this path is likely to be hit.

All addresses are already validated before the dial, so iterating does not weaken the guard.

♻️ Proposed fix: try each validated address
-		return dialer.DialContext(ctx, netw, net.JoinHostPort(ips[0].String(), port))
+		var dialErr error
+		for _, ip := range ips {
+			conn, err := dialer.DialContext(ctx, netw, net.JoinHostPort(ip.String(), port))
+			if err == nil {
+				return conn, nil
+			}
+			dialErr = err
+		}
+		return nil, dialErr
🤖 Prompt for 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.

In `@core/network/ssrf.go` around lines 160 - 168, Update the dialing logic after
validation in the relevant SSRF dialer to attempt each validated address in ips,
rather than only ips[0]. Return immediately on the first successful dial; if
every attempt fails, return the final or appropriately aggregated dial error
while preserving the existing address validation and error behavior.

153-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add an injectable resolver seam to PrivateNetworkDialContext. Its direct net.DefaultResolver call bypasses ipLookuper, so tests cannot cover its DNS error, empty-result, or multi-address paths. Share the seam without applying SSRFSafeDialContext’s public-IP restriction.

🤖 Prompt for 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.

In `@core/network/ssrf.go` at line 153, Update PrivateNetworkDialContext to
resolve hosts through the existing injectable ipLookuper seam instead of
directly calling net.DefaultResolver.LookupIP, ensuring DNS error, empty-result,
and multi-address paths are testable. Reuse the seam’s established configuration
while preserving PrivateNetworkDialContext’s current behavior without adding
SSRFSafeDialContext’s public-IP restriction.
core/mcp/clientmanager_test.go (1)

33-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

These tests depend on testDialContextOverride being nil.

buildTLSHTTPClient replaces the guarded dialer whenever the package-level testDialContextOverride is set. TestBuildTLSHTTPClientBlocksLinkLocal asserts the guard error string, so it fails if any other test in package mcp sets the override and does not restore it. Reset the override at the start of these tests, or use t.Cleanup to restore it, so the result does not depend on test order.

🤖 Prompt for 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.

In `@core/mcp/clientmanager_test.go` around lines 33 - 41, The test depends on the
package-level testDialContextOverride being nil but does not isolate that state.
Update TestBuildTLSHTTPClientBlocksLinkLocal to save the current override, clear
it before calling buildTLSHTTPClient, and restore it with t.Cleanup so the guard
assertion remains independent of test order.
transports/bifrost-http/handlers/mcp_ssrf_test.go (1)

58-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a case for a hostname that does not resolve.

The current cases all use IP literals, which is correct for determinism. No case covers the LookupIP error branch in rejectPrivateMCPTargetIfAuthBypassed. That branch currently returns false and allows the registration, which is the bypass flagged on transports/bifrost-http/handlers/mcp.go lines 639-654. Add a case with a hostname guaranteed not to resolve, for example a name under the reserved .invalid TLD, and assert the expected outcome after the branch is fixed.

🤖 Prompt for 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.

In `@transports/bifrost-http/handlers/mcp_ssrf_test.go` around lines 58 - 72,
Update rejectPrivateMCPTargetIfAuthBypassed to reject registration when LookupIP
fails instead of returning false and allowing the target. Add a test case
alongside
TestRejectPrivateMCPTargetIfAuthBypassed_UnauthenticatedPublicTargetAllowed
using a hostname under the reserved .invalid TLD, and assert that the request is
rejected.
🤖 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 `@core/mcp/clientmanager.go`:
- Around line 32-45: Restrict the test dial override so production importers
cannot access the SSRF bypass, using a build-tagged or internal test-only seam,
and store reads and writes through an atomic pointer for race-free access in
buildTLSHTTPClient and SetDialContextForTests. In core/mcp/clientmanager_test.go
lines 33-41, reset testDialContextOverride before the guard tests and register
t.Cleanup to restore it afterward, ensuring
TestBuildTLSHTTPClientBlocksLinkLocal is isolated from test order.

In `@docs/mcp/connecting-to-servers.mdx`:
- Around line 158-164: The HTTP client example currently uses localhost without
warning, contradicting the restriction described in the Note, and the Web UI tab
omits the same restriction. Update the HTTP example with a public host or a
clear dashboard-auth requirement for localhost, and add equivalent
loopback/private-network/link-local/CGNAT guidance to the Web UI tab so both
client-creation paths remain consistent.

In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 639-654: Update the unauthenticated target validation around
net.DefaultResolver.LookupIP to fail closed: when resolution returns an error,
send the same forbidden response and return true instead of allowing
registration. Also prevent DNS rebinding by carrying the validated public IP
into the subsequent MCP connection path, or otherwise pinning that address so
the connect-time dial uses the address checked by this validation.

In `@transports/bifrost-http/handlers/middlewares.go`:
- Around line 1011-1016: Ensure every credential-free whitelisted-route bypass,
including `/api/mcp/client`, sets `schemas.BifrostContextKeyAuthBypassed` before
calling `next(ctx)`. Update the middleware’s whitelist handling to preserve the
MCP gate’s rejection of private HTTP/SSE targets, and add a regression test
covering a whitelisted loopback `/api/mcp/client` request.

---

Nitpick comments:
In `@core/mcp/clientmanager_test.go`:
- Around line 33-41: The test depends on the package-level
testDialContextOverride being nil but does not isolate that state. Update
TestBuildTLSHTTPClientBlocksLinkLocal to save the current override, clear it
before calling buildTLSHTTPClient, and restore it with t.Cleanup so the guard
assertion remains independent of test order.

In `@core/network/ssrf.go`:
- Around line 160-168: Update the dialing logic after validation in the relevant
SSRF dialer to attempt each validated address in ips, rather than only ips[0].
Return immediately on the first successful dial; if every attempt fails, return
the final or appropriately aggregated dial error while preserving the existing
address validation and error behavior.
- Line 153: Update PrivateNetworkDialContext to resolve hosts through the
existing injectable ipLookuper seam instead of directly calling
net.DefaultResolver.LookupIP, ensuring DNS error, empty-result, and
multi-address paths are testable. Reuse the seam’s established configuration
while preserving PrivateNetworkDialContext’s current behavior without adding
SSRFSafeDialContext’s public-IP restriction.

In `@transports/bifrost-http/handlers/mcp_ssrf_test.go`:
- Around line 58-72: Update rejectPrivateMCPTargetIfAuthBypassed to reject
registration when LookupIP fails instead of returning false and allowing the
target. Add a test case alongside
TestRejectPrivateMCPTargetIfAuthBypassed_UnauthenticatedPublicTargetAllowed
using a hostname under the reserved .invalid TLD, and assert that the request is
rejected.

In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 642-644: Reduce the DNS lookup timeout in the hostname validation
flow using context.WithTimeout before net.DefaultResolver.LookupIP from 5
seconds to 2 seconds, preserving the existing cancellation and error-handling
behavior.
🪄 Autofix (Beta)

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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 80cb413e-2357-4c91-8254-c239b3d89f35

📥 Commits

Reviewing files that changed from the base of the PR and between e493a6a and 6d23107.

📒 Files selected for processing (10)
  • core/internal/mcptests/setup_test.go
  • core/mcp/clientmanager.go
  • core/mcp/clientmanager_test.go
  • core/network/ssrf.go
  • core/network/ssrf_test.go
  • core/schemas/bifrost.go
  • docs/mcp/connecting-to-servers.mdx
  • transports/bifrost-http/handlers/mcp.go
  • transports/bifrost-http/handlers/mcp_ssrf_test.go
  • transports/bifrost-http/handlers/middlewares.go

Comment thread core/mcp/clientmanager.go
Comment on lines +32 to +45
// testDialContextOverride, when non-nil, replaces the SSRF-safe dialer used by
// buildTLSHTTPClient. It exists solely so that integration tests (which stand
// up MCP servers on loopback-bound httptest.Server instances) can reach them;
// production code must never call SetDialContextForTests.
var testDialContextOverride func(ctx context.Context, network, addr string) (net.Conn, error)

// SetDialContextForTests overrides the dial function used to build outbound MCP
// HTTP/SSE connections. Test-only: this disables the SSRF guard that keeps
// unauthenticated callers from making the gateway dial loopback/private/internal
// addresses through MCP client registration. Pass nil to restore the default
// SSRF-safe dialer.
func SetDialContextForTests(dial func(ctx context.Context, network, addr string) (net.Conn, error)) {
testDialContextOverride = dial
}

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.

🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

One unsynchronized, exported dial override drives both findings. testDialContextOverride is a package-level variable in the production core/mcp package, written through an exported setter and read without synchronization. That single design choice both exposes an SSRF kill switch to any importer and makes the guard tests depend on test order.

  • core/mcp/clientmanager.go#L32-L45: restrict the seam so production builds cannot reach it, for example through a build-tagged file or an internal/ testing package, and store the value in an atomic.Pointer so concurrent set and read stay race-free.
  • core/mcp/clientmanager_test.go#L33-L41: reset testDialContextOverride at the start of the guard tests and restore it with t.Cleanup, so TestBuildTLSHTTPClientBlocksLinkLocal does not depend on which tests ran before it.
📍 Affects 2 files
  • core/mcp/clientmanager.go#L32-L45 (this comment)
  • core/mcp/clientmanager_test.go#L33-L41
🤖 Prompt for 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.

In `@core/mcp/clientmanager.go` around lines 32 - 45, Restrict the test dial
override so production importers cannot access the SSRF bypass, using a
build-tagged or internal test-only seam, and store reads and writes through an
atomic pointer for race-free access in buildTLSHTTPClient and
SetDialContextForTests. In core/mcp/clientmanager_test.go lines 33-41, reset
testDialContextOverride before the guard tests and register t.Cleanup to restore
it afterward, ensuring TestBuildTLSHTTPClientBlocksLinkLocal is isolated from
test order.

Comment on lines +158 to +164
<Note>
If [dashboard authentication](/quickstart/gateway/setting-up-auth) is disabled or not yet
configured, an HTTP or SSE client whose `connection_string` resolves to a loopback,
private-network, link-local, or carrier-grade-NAT address is refused - you must either enable
dashboard auth and authenticate first, or define the client in `config.json` instead. Clients
pointed at a public address (like the SSE example below) are unaffected either way.
</Note>

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.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

The Note contradicts the example directly below it.

The Note states that an HTTP client whose connection_string resolves to a loopback address is refused when dashboard authentication is disabled. The Add HTTP Client example on the next lines uses "connection_string": "http://localhost:3001/mcp". A reader who follows the quickstart with authentication disabled copies that command and receives HTTP 403.

Make the example consistent with the Note. Either use a public host in the HTTP example and keep localhost in a separate, clearly labelled "requires dashboard auth" snippet, or add a one-line callout on the example itself.

The same restriction applies to clients created through the Web UI tab, because that path reaches the same handler. The Web UI tab does not mention it. Add the restriction there as well so the two tabs stay in parity with the code.

As per path instructions, "Check docs for parity with code, config.schema.json, and provider behavior."

🤖 Prompt for 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.

In `@docs/mcp/connecting-to-servers.mdx` around lines 158 - 164, The HTTP client
example currently uses localhost without warning, contradicting the restriction
described in the Note, and the Web UI tab omits the same restriction. Update the
HTTP example with a public host or a clear dashboard-auth requirement for
localhost, and add equivalent loopback/private-network/link-local/CGNAT guidance
to the Web UI tab so both client-creation paths remain consistent.

Source: Path instructions

Comment on lines +639 to +654
// A bounded, request-independent context: this lookup is a pre-check, not
// a dial, and must not be tied to the request's own (often much longer)
// deadline.
lookupCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
ips, err := net.DefaultResolver.LookupIP(lookupCtx, "ip", parsed.Hostname())
if err != nil {
return false
}
for _, ip := range ips {
if !network.IsPublicIP(ip) {
SendError(ctx, fasthttp.StatusForbidden, "unauthenticated callers cannot register MCP clients that connect to loopback, private-network, or link-local addresses; set an admin password to allow this")
return true
}
}
return false

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.

🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift

A DNS failure lets an unauthenticated caller register a private target.

Line 645 returns false when LookupIP fails, so the registration proceeds. An attacker who controls the target hostname can make resolution fail at this moment and succeed later, when core/mcp performs its own lookup at connect time. PrivateNetworkDialContext permits loopback and private ranges by design, so nothing downstream stops the connection. The control is bypassed completely.

The same gap exists without a failure, through DNS rebinding: this function resolves the hostname now, and PrivateNetworkDialContext resolves it again at dial time. A hostname that returns a public address here and 127.0.0.1 at connect time passes both checks.

Two changes close both paths:

  1. Reject the registration when resolution fails, instead of allowing it. Resolution failure is not evidence that the target is public.
  2. Do not rely on registration-time resolution alone. Propagate the auth-bypassed decision to the connect path, or pin the validated IP for the connection, so the address that was checked is the address that is dialed.

Item 1 is a small change and removes the immediate bypass.

As per path instructions, apply "fail-closed behavior" for HTTP/API security review.

🔒️ Proposed fix for the fail-open DNS branch
 	ips, err := net.DefaultResolver.LookupIP(lookupCtx, "ip", parsed.Hostname())
-	if err != nil {
-		return false
-	}
+	if err != nil {
+		SendError(ctx, fasthttp.StatusForbidden, "unauthenticated callers cannot register MCP clients whose target hostname cannot be resolved and verified as public; set an admin password to allow this")
+		return true
+	}
+	if len(ips) == 0 {
+		SendError(ctx, fasthttp.StatusForbidden, "unauthenticated callers cannot register MCP clients whose target hostname resolves to no addresses; set an admin password to allow this")
+		return true
+	}
 	for _, ip := range ips {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// A bounded, request-independent context: this lookup is a pre-check, not
// a dial, and must not be tied to the request's own (often much longer)
// deadline.
lookupCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
ips, err := net.DefaultResolver.LookupIP(lookupCtx, "ip", parsed.Hostname())
if err != nil {
return false
}
for _, ip := range ips {
if !network.IsPublicIP(ip) {
SendError(ctx, fasthttp.StatusForbidden, "unauthenticated callers cannot register MCP clients that connect to loopback, private-network, or link-local addresses; set an admin password to allow this")
return true
}
}
return false
// A bounded, request-independent context: this lookup is a pre-check, not
// a dial, and must not be tied to the request's own (often much longer)
// deadline.
lookupCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
ips, err := net.DefaultResolver.LookupIP(lookupCtx, "ip", parsed.Hostname())
if err != nil {
SendError(ctx, fasthttp.StatusForbidden, "unauthenticated callers cannot register MCP clients whose target hostname cannot be resolved and verified as public; set an admin password to allow this")
return true
}
if len(ips) == 0 {
SendError(ctx, fasthttp.StatusForbidden, "unauthenticated callers cannot register MCP clients whose target hostname resolves to no addresses; set an admin password to allow this")
return true
}
for _, ip := range ips {
if !network.IsPublicIP(ip) {
SendError(ctx, fasthttp.StatusForbidden, "unauthenticated callers cannot register MCP clients that connect to loopback, private-network, or link-local addresses; set an admin password to allow this")
return true
}
}
return false
🤖 Prompt for 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.

In `@transports/bifrost-http/handlers/mcp.go` around lines 639 - 654, Update the
unauthenticated target validation around net.DefaultResolver.LookupIP to fail
closed: when resolution returns an error, send the same forbidden response and
return true instead of allowing registration. Also prevent DNS rebinding by
carrying the validated public IP into the subsequent MCP connection path, or
otherwise pinning that address so the connect-time dial uses the address checked
by this validation.

Source: Path instructions

Comment on lines +1011 to +1016
// Distinct from IsLocalAdminContextKey (also true for genuinely
// authenticated sessions): this specifically marks requests that
// were let through with no credential check at all, so handlers
// gating a dangerous capability can refuse it even while the
// rest of the API stays open under the default-open posture.
ctx.SetUserValue(schemas.BifrostContextKeyAuthBypassed, true)

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.

🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Determine whether the MCP client route can be reached through a bypass branch.
rg -n -C5 'whitelistedRoutes|shouldSkip|isRealtimeTransportEndpoint' transports/bifrost-http
rg -n -C3 'api/mcp/client' transports/bifrost-http

Repository: maximhq/bifrost

Length of output: 20438


🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- middleware branches and auth marker ---'
sed -n '930,1095p' transports/bifrost-http/handlers/middlewares.go
printf '%s\n' '--- bypass marker consumers and gate ---'
rg -n -C8 'BifrostContextKeyAuthBypassed|rejectPrivateMCPTargetIfAuthBypassed' transports/bifrost-http
printf '%s\n' '--- relevant tests ---'
rg -n -C10 'AuthBypassed|private.?network|MCP.*target|whitelist|realtime' transports/bifrost-http/handlers --glob '*_test.go'
printf '%s\n' '--- repository stack/status hints ---'
git status --short
git branch --show-current
git log --oneline -8

Repository: maximhq/bifrost

Length of output: 50372


🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- route registration and middleware composition ---'
rg -n -C6 'MCPHandler|RegisterRoutes|APIMiddleware\(\)|ChainMiddlewares' transports/bifrost-http --glob '*.go' | head -180
printf '%s\n' '--- whitelist configuration definitions and updates ---'
rg -n -C6 'WhitelistedRoutes|whitelisted_routes|whitelistedRoutes|UpdateWhitelistedRoutes' . --glob '*.go' --glob '*.json' --glob '*.md' | head -220
printf '%s\n' '--- realtime endpoint set and exact MCP path overlap ---'
sed -n '730,790p' transports/bifrost-http/handlers/middlewares.go
python3 - <<'PY'
from pathlib import Path
p = Path("transports/bifrost-http/handlers/middlewares.go").read_text()
mcp = "/api/mcp/client"
print("MCP path appears in realtime endpoint declaration:", mcp in p[p.find("realtimeTransportPaths"):p.find("func hasVirtualKeyCredential")])
print("MCP path appears in system whitelist:", mcp in p[p.find("systemWhitelistedRoutes"):p.find("whitelistedPrefixes")])
print("MCP path is accepted by configurable exact/prefix whitelist:", True)
PY
printf '%s\n' '--- concise repository stack/status ---'
git status --short
git branch --show-current
git log --oneline -8

Repository: maximhq/bifrost

Length of output: 41985


Mark whitelist bypasses as unauthenticated.

When whitelistedRoutes matches /api/mcp/client, the middleware calls next(ctx) without setting BifrostContextKeyAuthBypassed. The MCP gate then treats the missing marker as authenticated and allows private HTTP/SSE targets. Set the marker on every credential-free bypass, or record explicit authentication and reject when it is absent. Add a regression test for a whitelisted /api/mcp/client loopback target.

🤖 Prompt for 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.

In `@transports/bifrost-http/handlers/middlewares.go` around lines 1011 - 1016,
Ensure every credential-free whitelisted-route bypass, including
`/api/mcp/client`, sets `schemas.BifrostContextKeyAuthBypassed` before calling
`next(ctx)`. Update the middleware’s whitelist handling to preserve the MCP
gate’s rejection of private HTTP/SSE targets, and add a regression test covering
a whitelisted loopback `/api/mcp/client` request.

Source: Path instructions

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.

3 participants