fix(security): block SSRF via OAuth metadata discovery endpoints#3061
Open
0xcucumbersalad wants to merge 4 commits intodecocms:mainfrom
Open
fix(security): block SSRF via OAuth metadata discovery endpoints#30610xcucumbersalad wants to merge 4 commits intodecocms:mainfrom
0xcucumbersalad wants to merge 4 commits intodecocms:mainfrom
Conversation
The unauthenticated OAuth discovery endpoints proxied outbound requests to any URL stored in a connection record. An attacker could create a connection pointing to internal services (127.0.0.1, 169.254.169.254, etc.) and use the well-known endpoints to read internal responses. Adds isPrivateNetworkUrl() validation that blocks private/internal IP ranges (loopback, RFC 1918, link-local/IMDS, IPv6 equivalents) before any outbound fetch. Also includes org-scoped connection lookups and normalized error responses from the AUTHZ-VULN-06 fix. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reverts org-scoping and error normalization changes that are already covered by a separate PR. Keeps only the SSRF validation fix. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
🧪 BenchmarkShould we run the Virtual MCP strategy benchmark for this PR? React with 👍 to run the benchmark.
Benchmark will run on the next push after you react. |
Contributor
Release OptionsSuggested: Patch ( React with an emoji to override the release type:
Current version:
|
…heck Defaults to false (SSRF protection on). Set ALLOW_PRIVATE_NETWORK_CONNECTIONS=true for local dev where localhost MCP servers are expected. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/mesh/src/api/routes/oauth-proxy.test.ts">
<violation number="1" location="apps/mesh/src/api/routes/oauth-proxy.test.ts:447">
P2: These SSRF tests only assert the final 404, so they can still pass after a real outbound fetch fails instead of proving the request was blocked before any network call.</violation>
</file>
<file name="apps/mesh/src/shared/utils/url-validation.ts">
<violation number="1" location="apps/mesh/src/shared/utils/url-validation.ts:82">
P1: The `fe80::/10` check only matches `fe80::/16`, so other link-local IPv6 addresses still bypass the filter.</violation>
<violation number="2" location="apps/mesh/src/shared/utils/url-validation.ts:98">
P0: Resolve hostnames before allowing them, or internal DNS names still bypass the SSRF filter.</violation>
<violation number="3" location="apps/mesh/src/shared/utils/url-validation.ts:113">
P2: Only run the IPv6 private-range checks on IPv6 literals, or public hostnames like `fd.example.com` will be rejected.</violation>
</file>
<file name="apps/mesh/src/api/routes/oauth-proxy.ts">
<violation number="1" location="apps/mesh/src/api/routes/oauth-proxy.ts:60">
P1: The new SSRF guard only checks `connection_url`; it still trusts and fetches unvalidated `authorization_servers` URLs from origin metadata, so internal auth-server URLs remain reachable through the discovery proxy.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -0,0 +1,116 @@ | |||
| /** | |||
Contributor
There was a problem hiding this comment.
P0: Resolve hostnames before allowing them, or internal DNS names still bypass the SSRF filter.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/shared/utils/url-validation.ts, line 98:
<comment>Resolve hostnames before allowing them, or internal DNS names still bypass the SSRF filter.</comment>
<file context>
@@ -0,0 +1,116 @@
+ *
+ * Returns `true` if the URL should be blocked.
+ */
+export function isPrivateNetworkUrl(url: string): boolean {
+ let parsed: URL;
+ try {
</file context>
| // fc00::/7 — unique local | ||
| if (normalized.startsWith("fc") || normalized.startsWith("fd")) return true; | ||
| // fe80::/10 — link-local | ||
| if (normalized.startsWith("fe80")) return true; |
Contributor
There was a problem hiding this comment.
P1: The fe80::/10 check only matches fe80::/16, so other link-local IPv6 addresses still bypass the filter.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/shared/utils/url-validation.ts, line 82:
<comment>The `fe80::/10` check only matches `fe80::/16`, so other link-local IPv6 addresses still bypass the filter.</comment>
<file context>
@@ -0,0 +1,116 @@
+ // fc00::/7 — unique local
+ if (normalized.startsWith("fc") || normalized.startsWith("fd")) return true;
+ // fe80::/10 — link-local
+ if (normalized.startsWith("fe80")) return true;
+ // ::ffff:x.x.x.x — IPv4-mapped IPv6 (dotted or hex form)
+ const v4 = extractIPv4FromMappedIPv6(normalized);
</file context>
| ctx.organization?.id, | ||
| ); | ||
| const url = connection?.connection_url ?? null; | ||
| if (url && isPrivateNetworkUrl(url)) { |
Contributor
There was a problem hiding this comment.
P1: The new SSRF guard only checks connection_url; it still trusts and fetches unvalidated authorization_servers URLs from origin metadata, so internal auth-server URLs remain reachable through the discovery proxy.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/api/routes/oauth-proxy.ts, line 60:
<comment>The new SSRF guard only checks `connection_url`; it still trusts and fetches unvalidated `authorization_servers` URLs from origin metadata, so internal auth-server URLs remain reachable through the discovery proxy.</comment>
<file context>
@@ -43,15 +44,23 @@ const NO_METADATA_STATUSES = [404, 401, 406];
+ ctx.organization?.id,
+ );
+ const url = connection?.connection_url ?? null;
+ if (url && isPrivateNetworkUrl(url)) {
+ return null;
+ }
</file context>
| expect(body.authorization_endpoint).toBeUndefined(); | ||
| }); | ||
|
|
||
| test("blocks SSRF to loopback addresses", async () => { |
Contributor
There was a problem hiding this comment.
P2: These SSRF tests only assert the final 404, so they can still pass after a real outbound fetch fails instead of proving the request was blocked before any network call.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/api/routes/oauth-proxy.test.ts, line 447:
<comment>These SSRF tests only assert the final 404, so they can still pass after a real outbound fetch fails instead of proving the request was blocked before any network call.</comment>
<file context>
@@ -442,6 +443,56 @@ describe("OAuth Proxy Routes", () => {
expect(body.authorization_endpoint).toBeUndefined();
});
+
+ test("blocks SSRF to loopback addresses", async () => {
+ mockConnectionStorage({
+ connection_url: "http://127.0.0.1:51388",
</file context>
|
|
||
| if (BLOCKED_HOSTNAMES.has(bare.toLowerCase())) return true; | ||
| if (isPrivateIPv4(bare)) return true; | ||
| if (isPrivateIPv6(bare)) return true; |
Contributor
There was a problem hiding this comment.
P2: Only run the IPv6 private-range checks on IPv6 literals, or public hostnames like fd.example.com will be rejected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/shared/utils/url-validation.ts, line 113:
<comment>Only run the IPv6 private-range checks on IPv6 literals, or public hostnames like `fd.example.com` will be rejected.</comment>
<file context>
@@ -0,0 +1,116 @@
+
+ if (BLOCKED_HOSTNAMES.has(bare.toLowerCase())) return true;
+ if (isPrivateIPv4(bare)) return true;
+ if (isPrivateIPv6(bare)) return true;
+
+ return false;
</file context>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/mesh/src/api/routes/oauth-proxy.ts">
<violation number="1">
P1: Exposing `err.message` from failed outbound fetches on unauthenticated endpoints leaks internal network details (hostnames, DNS errors, connection timeouts). The old code intentionally returned a generic 404 "to avoid leaking connection existence." While 502 is a more correct status for a proxy failure, the raw error message should be omitted or replaced with a generic message.</violation>
<violation number="2" location="apps/mesh/src/api/routes/oauth-proxy.ts:55">
P1: OAuth proxy connection lookup is no longer organization-scoped, enabling potential cross-tenant connection access by ID.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| connectionId: string, | ||
| ctx: MeshContext, | ||
| ): Promise<string | null> { | ||
| const connection = await ctx.storage.connections.findById(connectionId); |
Contributor
There was a problem hiding this comment.
P1: OAuth proxy connection lookup is no longer organization-scoped, enabling potential cross-tenant connection access by ID.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/api/routes/oauth-proxy.ts, line 55:
<comment>OAuth proxy connection lookup is no longer organization-scoped, enabling potential cross-tenant connection access by ID.</comment>
<file context>
@@ -45,19 +46,19 @@ const NO_METADATA_STATUSES = [404, 401, 406];
- connectionId,
- ctx.organization?.id,
- );
+ const connection = await ctx.storage.connections.findById(connectionId);
const url = connection?.connection_url ?? null;
- if (url && isPrivateNetworkUrl(url)) {
</file context>
Suggested change
| const connection = await ctx.storage.connections.findById(connectionId); | |
| const connection = await ctx.storage.connections.findById( | |
| connectionId, | |
| ctx.organization?.id, | |
| ); |
| @@ -15,6 +15,8 @@ | |||
| import { Hono } from "hono"; | |||
Contributor
There was a problem hiding this comment.
P1: Exposing err.message from failed outbound fetches on unauthenticated endpoints leaks internal network details (hostnames, DNS errors, connection timeouts). The old code intentionally returned a generic 404 "to avoid leaking connection existence." While 502 is a more correct status for a proxy failure, the raw error message should be omitted or replaced with a generic message.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/api/routes/oauth-proxy.ts, line 481:
<comment>Exposing `err.message` from failed outbound fetches on unauthenticated endpoints leaks internal network details (hostnames, DNS errors, connection timeouts). The old code intentionally returned a generic 404 "to avoid leaking connection existence." While 502 is a more correct status for a proxy failure, the raw error message should be omitted or replaced with a generic message.</comment>
<file context>
@@ -476,8 +477,10 @@ const protectedResourceMetadataHandler = async (c: {
- // Return 404 (same as "not found") to avoid leaking connection existence
- return c.json({ error: "Connection not found" }, 404);
+ return c.json(
+ { error: "Failed to proxy OAuth metadata", message: err.message },
+ 502,
+ );
</file context>
Suggested change
| import { Hono } from "hono"; | |
| { error: "Failed to proxy OAuth metadata" }, |
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.
Summary
/.well-known/oauth-protected-resource,/.well-known/oauth-authorization-server) proxied outbound requests to any URL stored in a connection record, enabling internal service data retrieval and IMDS accessisPrivateNetworkUrl()utility that blocks private/internal network ranges before any outbound fetch:127.0.0.0/8,10.0.0.0/8,172.16.0.0/12,192.168.0.0/16,169.254.0.0/16,0.0.0.0/8::1,fc00::/7,fe80::/10, IPv4-mapped (::ffff:x.x.x.xin both dotted and hex form)localhostgetConnectionUrl()— the gateway for all outbound fetches in the OAuth proxyTest plan
bun test apps/mesh/src/shared/utils/url-validation.test.ts— 15/15 pass (loopback, RFC 1918, IMDS, IPv6, public URLs, edge cases)bun test apps/mesh/src/api/routes/oauth-proxy.test.ts— 30/30 pass (5 new SSRF tests + all existing tests)bun run lint— passes (0 errors)bun run fmt— passes🤖 Generated with Claude Code