Skip to content

MCP-530 Relax Origin check for authenticated and bootstrap requests - #465

Merged
nquinquenel merged 4 commits into
masterfrom
bug/nq/MCP-530-origin-check
Jun 25, 2026
Merged

MCP-530 Relax Origin check for authenticated and bootstrap requests#465
nquinquenel merged 4 commits into
masterfrom
bug/nq/MCP-530-origin-check

Conversation

@nquinquenel

@nquinquenel nquinquenel commented Jun 24, 2026

Copy link
Copy Markdown
Member

Summary by Gitar

  • Security updates:
    • Relaxed Origin header enforcement in McpSecurityFilter for authenticated requests, OAuth bootstrap paths (/mcp), and /.well-known/* endpoints.
    • Remote host bindings no longer enforce strict Origin checks.
  • Documentation:
    • Updated http-authentication-architecture.md to reflect new origin validation logic and clarify browser-layer backstop role.
  • Testing:
    • Added integration tests to verify authenticated request bypass and enforcement on non-MCP paths.
    • Updated unit tests to cover new bypass conditions for bootstrap paths and tokens.

This will update automatically on new commits.

@hashicorp-vault-sonar-prod

hashicorp-vault-sonar-prod Bot commented Jun 24, 2026

Copy link
Copy Markdown

MCP-530

Comment thread src/main/java/org/sonarsource/sonarqube/mcp/transport/McpSecurityFilter.java Outdated
Comment thread src/main/java/org/sonarsource/sonarqube/mcp/transport/McpSecurityFilter.java Outdated
@nquinquenel
nquinquenel force-pushed the bug/nq/MCP-530-origin-check branch from c0c3a06 to a88f66a Compare June 24, 2026 17:46
Comment thread src/main/java/org/sonarsource/sonarqube/mcp/transport/McpSecurityFilter.java Outdated
@nquinquenel
nquinquenel marked this pull request as ready for review June 25, 2026 08:38
Comment thread src/main/java/org/sonarsource/sonarqube/mcp/transport/McpSecurityFilter.java Outdated

@damien-urruty-sonarsource damien-urruty-sonarsource 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.

LGTM, but maybe a review from IST or AppSec would help

@nquinquenel
nquinquenel force-pushed the bug/nq/MCP-530-origin-check branch from f72e84e to a29b530 Compare June 25, 2026 14:18
@gitar-bot

gitar-bot Bot commented Jun 25, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 4 resolved / 4 findings

Relaxed Origin enforcement for authenticated requests and bootstrap endpoints while addressing DNS-rebinding vulnerabilities. No issues found.

✅ 4 resolved
Security: DNS-resolving Origin host reopens DNS-rebinding bypass

📄 src/main/java/org/sonarsource/sonarqube/mcp/transport/McpSecurityFilter.java:201-215
isLoopbackOrigin now resolves the host taken from the attacker-controlled Origin header via InetAddress.getByName(host).isLoopbackAddress(). This defeats the very DNS-rebinding protection the filter is named for: an attacker who controls a domain that resolves to a loopback address (e.g. http://rebind.attacker.com → 127.0.0.1, a classic DNS-rebinding setup) will pass the loopback check, because getByName follows DNS. The previous implementation matched the Origin host literally against {localhost, 127.0.0.1, [::1]}, which could not be bypassed this way.

While Bearer authentication is the primary control and this path only applies to unauthenticated requests on local bindings to non-/mcp paths, the change is a real regression of the documented backstop. Recommend matching the Origin host against literal loopback forms (or only resolving when the host is already an IP literal) instead of performing name resolution on untrusted input.

Performance: Blocking DNS lookups per request on Origin/host binding

📄 src/main/java/org/sonarsource/sonarqube/mcp/transport/McpSecurityFilter.java:174-185 📄 src/main/java/org/sonarsource/sonarqube/mcp/transport/McpSecurityFilter.java:201-215
InetAddress.getByName(...) performs a synchronous, potentially blocking DNS lookup. isLocalBinding() is invoked multiple times per request (from both isOriginAllowed and isOriginInAllowlist) and resolves hostBinding each time, and isLoopbackOrigin resolves the attacker-controlled Origin host. For unauthenticated requests, an attacker can supply arbitrary Origin hostnames that force repeated DNS resolution on the request thread, adding latency and a potential DoS amplification vector. Consider computing isLocalBinding once (e.g. cache the result at construction time) and avoiding name resolution of untrusted Origin hosts (see related finding).

Edge Case: Blank Origin header now bypasses origin enforcement

📄 src/main/java/org/sonarsource/sonarqube/mcp/transport/McpSecurityFilter.java:101 📄 src/test/java/org/sonarsource/sonarqube/mcp/transport/McpSecurityFilterTest.java:340
The guard changed from origin != null && !isOriginAllowed(origin) to origin != null && !origin.isBlank() && ..., and the edge-case test flipped from 'empty origin header should be rejected' to 'should not be rejected'. A request with Origin: "" (blank) now skips enforcement entirely and is passed through (subject to downstream auth). This is low risk because browsers cannot forge a blank Origin for cross-site requests and non-browser clients aren't subject to DNS rebinding, but it does widen what the filter lets through. If the intent was only to tolerate a missing Origin header, the blank-string case is arguably better treated the same as a disallowed origin. Confirm this broadening is intentional; if so, no action needed.

Security: Origin bypass for /mcp & /.well-known relies entirely on downstream auth

📄 src/main/java/org/sonarsource/sonarqube/mcp/transport/McpSecurityFilter.java:149-163
shouldEnforceOrigin now skips DNS-rebinding origin enforcement for any tokenless request to /mcp or /.well-known/* (isOAuthBootstrapRequest). This is safe today only because AuthenticationFilter runs after McpSecurityFilter and returns 401 for tokenless requests in TOKEN mode, and because /.well-known metadata is non-sensitive by design. The browser-layer backstop is therefore entirely gone for these paths — if a future change ever adds an unauthenticated handler under /mcp, reorders the filters, or serves sensitive data from a /.well-known/* path, a DNS-rebinding attacker would regain access with no origin guard. Note also that isOAuthBootstrapRequest matches /mcp for ALL methods (GET, DELETE, etc.), broader than the documented 'POST /mcp (401 discovery)'. Consider narrowing the bootstrap exemption to the methods/paths actually used for OAuth discovery, and/or adding a regression test asserting that a tokenless /mcp request with a disallowed origin is still rejected by the auth layer, to lock in the assumption this relaxation depends on.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqubecloud

Copy link
Copy Markdown

@nquinquenel
nquinquenel merged commit 2e29484 into master Jun 25, 2026
11 checks passed
@nquinquenel
nquinquenel deleted the bug/nq/MCP-530-origin-check branch June 25, 2026 14:29
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.

2 participants