Skip to content

Commit 610b34a

Browse files
venkateshsakamuri-labvenkateshsakamuri-lab
authored
Merge branch 'main' into kaushik-IDE
2 parents cf05221 + 57568ed commit 610b34a

7 files changed

Lines changed: 291 additions & 43 deletions

File tree

‎CLAUDE.md‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -371,9 +371,30 @@ The Agent tab must not inherit the admin MCP token. `/api/agent/session` mints a
371371
on purpose (per-browser cookie), so a View as / new-thread Agent chat tagged
372372
`profile: u-marts-editor` still sends the admin MCP bearer. Chat-access policy
373373
then `resolveEffectivePolicy(..., actorIsAdmin=true)` → `none()`. The
374-
provisioner mirrors the target user's token onto every `deepsql.token` the
375-
live process might re-read (`DEEPSQL_TOKEN_FILE` mtime cache in
376-
`mcp/deepsql-phase1-lib.js`).
374+
provisioner writes the target user's token to `$HERMES_HOME/deepsql.token`,
375+
the one shared path the live process may have started from
376+
(`DEEPSQL_TOKEN_FILE` mtime cache in `mcp/deepsql-phase1-lib.js`).
377+
378+
**Never fan that write out across `profiles/*/deepsql.token`.** It did once,
379+
and made the agent credential globally last-writer-wins: any user opening the
380+
Agent tab overwrote every other user's token, so their agent authenticated as
381+
the newcomer. Verified end to end — `analyst`'s agent read an admin-only
382+
connection (403 on their own session, 200 with the agent token, 133 vault
383+
tables) and the `EDITOR_QUERY_EXECUTED` row named **admin**, not analyst. Two
384+
concurrent users was the whole trigger; no impersonation needed. The
385+
provisioner self-test asserted the fan-out as *correct* (it modelled only the
386+
View-as case, where overwriting is desired), so a green suite guarded the bug —
387+
it now asserts the opposite, that provisioning B leaves A's token intact.
388+
389+
Because that root file is still shared, the real guard is server-side:
390+
`McpTokenAuthenticationFilter` refuses an MCP token whose owner differs from
391+
the request's `DEEPSQL_MCP_USER_ID` claim (sent as `X-DeepSQL-Client-Agent`),
392+
answering `401 mcp_identity_mismatch`. The claim is only ever used to *refuse*,
393+
never to grant, so forging it cannot widen access. A claim that isn't a real
394+
DeepSQL username is ignored, which is what keeps editor/CLI MCP installs
395+
(`cursor`, `claude-desktop`, any `--caller-agent`) working. `probeMcpAuth`
396+
sends the same header so the boot health check exercises the binding instead
397+
of bypassing it.
377398
6. **The agent image build clones two third-party repos over the public internet,
378399
unauthenticated.** `agent/Dockerfile` fetches `NousResearch/hermes-agent` and
379400
`nesquena/hermes-webui` at build time. GitHub rate-limits unauthenticated

‎backend/src/main/java/com/dbaagent/controller/AgentBridgeController.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ public ResponseEntity<Map<String, Object>> session(
5959
Map<String, Object> response = new HashMap<>();
6060
response.put("profile", bootstrap.profile());
6161
response.put("username", username);
62-
boolean mcpAuthOk = agentBridgeService.probeMcpAuth(bootstrap.token());
62+
boolean mcpAuthOk = agentBridgeService.probeMcpAuth(bootstrap.token(), username);
6363
response.put("mcpAuthOk", mcpAuthOk);
6464
if (!mcpAuthOk) {
6565
response.put("mcpAuthError", "The DeepSQL Agent could not authenticate against this API with its "

‎backend/src/main/java/com/dbaagent/security/McpTokenAuthenticationFilter.java‎

Lines changed: 70 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
package com.dbaagent.security;
22

3+
import com.dbaagent.repository.UserRepository;
4+
import com.dbaagent.service.ClientContext;
35
import com.dbaagent.service.McpTokenService;
46
import jakarta.servlet.FilterChain;
57
import jakarta.servlet.ServletException;
@@ -8,6 +10,8 @@
810
import lombok.RequiredArgsConstructor;
911
import lombok.extern.slf4j.Slf4j;
1012
import org.springframework.beans.factory.annotation.Value;
13+
import org.springframework.http.HttpStatus;
14+
import org.springframework.http.MediaType;
1115
import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
1216
import org.springframework.security.core.context.SecurityContextHolder;
1317
import org.springframework.security.core.userdetails.UserDetails;
@@ -27,6 +31,7 @@ public class McpTokenAuthenticationFilter extends OncePerRequestFilter {
2731

2832
private final McpTokenService mcpTokenService;
2933
private final CustomUserDetailsService userDetailsService;
34+
private final UserRepository userRepository;
3035

3136
@Override
3237
protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain chain)
@@ -49,7 +54,26 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse
4954
return;
5055
}
5156

52-
mcpTokenService.authenticate(token, request.getRemoteAddr()).ifPresent(authenticatedToken -> {
57+
var authenticated = mcpTokenService.authenticate(token, request.getRemoteAddr());
58+
if (authenticated.isPresent() && !declaredUserMatches(request, authenticated.get().username())) {
59+
// The agent runtime keeps ONE MCP subprocess for every profile, and
60+
// the provisioner rotates its credential on disk. A token belonging
61+
// to a different user than the one this MCP process was started for
62+
// means the credential was overwritten by someone else's Agent-tab
63+
// open — authenticating it here would run this user's tools as that
64+
// other user (cross-user read + falsified audit attribution).
65+
// Fail closed: the caller must re-provision, not silently proceed.
66+
log.warn("Rejecting MCP token for {} — request declares user {}",
67+
authenticated.get().username(), declaredUser(request));
68+
response.setStatus(HttpStatus.UNAUTHORIZED.value());
69+
response.setContentType(MediaType.APPLICATION_JSON_VALUE);
70+
response.getWriter().write("{\"error\":\"mcp_identity_mismatch\",\"message\":"
71+
+ "\"This Agent session's credential belongs to a different user. "
72+
+ "Reopen the Agent tab to continue.\"}");
73+
return;
74+
}
75+
76+
authenticated.ifPresent(authenticatedToken -> {
5377
UserDetails userDetails = userDetailsService.loadUserByUsername(authenticatedToken.username());
5478
UsernamePasswordAuthenticationToken authentication = new UsernamePasswordAuthenticationToken(
5579
userDetails,
@@ -64,4 +88,49 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse
6488

6589
chain.doFilter(request, response);
6690
}
91+
92+
/**
93+
* The user this MCP process was provisioned for, as declared by the caller.
94+
*
95+
* <p>The agent provisioner writes {@code DEEPSQL_MCP_USER_ID=<username>} into
96+
* each profile's MCP server env, and the MCP shim forwards it as
97+
* {@link ClientContext#HEADER_AGENT}. It is a *claim*, not a credential — it
98+
* is only ever used to REFUSE a mismatched token, never to grant access, so
99+
* a forged value cannot widen access beyond what the token already allows.
100+
*/
101+
private static String declaredUser(HttpServletRequest request) {
102+
String declared = request.getHeader(ClientContext.HEADER_AGENT);
103+
return declared == null || declared.isBlank() ? null : declared.trim();
104+
}
105+
106+
/**
107+
* True when the request carries no user claim (editor/CLI installs, curl,
108+
* every pre-existing caller) or the claim matches the token's owner.
109+
*
110+
* <p>Absent claim stays permissive on purpose: {@code DEEPSQL_MCP_USER_ID}
111+
* defaults to non-username values for editor installs ("cursor",
112+
* "claude-code", "mcp-phase1"), and those tokens are not agent-provisioned,
113+
* so there is no shared-credential hazard to guard against. Only a claim
114+
* that looks like a DeepSQL agent profile identity is enforced.
115+
*/
116+
private boolean declaredUserMatches(HttpServletRequest request, String tokenOwner) {
117+
String declared = declaredUser(request);
118+
if (declared == null || tokenOwner == null) {
119+
return true;
120+
}
121+
if (declared.equalsIgnoreCase(tokenOwner)) {
122+
return true;
123+
}
124+
// The claim differs from the token owner. Enforce only when the claim
125+
// names a real DeepSQL user — that is the agent-provisioner case, where
126+
// DEEPSQL_MCP_USER_ID is a username and a mismatch means the shared
127+
// token file was overwritten by another user's Agent-tab open.
128+
//
129+
// Editor/CLI clients put a *tool* name here ("cursor", "claude-desktop",
130+
// "terminal", "mcp-phase1", or any --caller-agent value), which never
131+
// resolves to a user, so those callers are unaffected. Matching against
132+
// the user table rather than a denylist of sentinels keeps free-form
133+
// --caller-agent values working without a list to maintain.
134+
return userRepository.findByUsernameIgnoreCase(declared).isEmpty();
135+
}
67136
}

‎backend/src/main/java/com/dbaagent/service/AgentBridgeService.java‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -312,16 +312,30 @@ private void callProvisionerRevoke(String username) {
312312
* bearer credential against the local backend (loopback — this call never
313313
* leaves the box, so no external base-URL config is needed).
314314
*
315+
* <p>Sends the same {@code X-DeepSQL-Client-Agent: <username>} claim the
316+
* provisioned MCP process will send, so the probe passes only if the token
317+
* and the declared identity agree. A probe without that header would call a
318+
* token healthy that the live MCP process cannot actually use.
319+
*
320+
* @param token the credential to verify
321+
* @param username the identity the MCP process will declare for this profile
315322
* @return true if the API accepted the token (2xx), false on any
316323
* non-2xx/auth failure or network error.
317324
*/
318-
public boolean probeMcpAuth(String token) {
325+
public boolean probeMcpAuth(String token, String username) {
319326
if (token == null || token.isBlank()) {
320327
return false;
321328
}
322329
try {
323330
HttpRequest req = HttpRequest.newBuilder(URI.create(localApiBaseUrl + "/connections"))
324331
.header("Authorization", "Bearer " + token)
332+
// Declare the same identity the provisioned MCP process will send
333+
// (DEEPSQL_MCP_USER_ID -> X-DeepSQL-Client-Agent), so the probe
334+
// exercises the identity-binding check in
335+
// McpTokenAuthenticationFilter rather than bypassing it. Probing
336+
// without this header would report a token as healthy even when
337+
// the live MCP process's own calls will be refused.
338+
.header(ClientContext.HEADER_AGENT, username)
325339
.timeout(Duration.ofSeconds(5))
326340
.GET()
327341
.build();

‎backend/src/test/java/com/dbaagent/security/McpTokenAuthenticationFilterTest.java‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package com.dbaagent.security;
22

3+
import com.dbaagent.repository.UserRepository;
34
import com.dbaagent.service.McpTokenService;
45
import jakarta.servlet.ServletException;
56
import org.junit.jupiter.api.AfterEach;
@@ -22,6 +23,8 @@
2223

2324
import static org.junit.jupiter.api.Assertions.assertEquals;
2425
import static org.junit.jupiter.api.Assertions.assertNotNull;
26+
import static org.junit.jupiter.api.Assertions.assertNull;
27+
import static org.junit.jupiter.api.Assertions.assertTrue;
2528
import static org.mockito.Mockito.when;
2629

2730
@ExtendWith(MockitoExtension.class)
@@ -33,6 +36,9 @@ class McpTokenAuthenticationFilterTest {
3336
@Mock
3437
private CustomUserDetailsService userDetailsService;
3538

39+
@Mock
40+
private UserRepository userRepository;
41+
3642
@InjectMocks
3743
private McpTokenAuthenticationFilter filter;
3844

@@ -67,4 +73,100 @@ void authenticateUsesResolvedUsernameWithoutTouchingLazyEntity() throws ServletE
6773
assertNotNull(SecurityContextHolder.getContext().getAuthentication());
6874
assertEquals("alice", SecurityContextHolder.getContext().getAuthentication().getName());
6975
}
76+
77+
/**
78+
* The cross-user token leak, at the layer that stops it.
79+
*
80+
* <p>The agent runtime keeps ONE MCP subprocess and the provisioner rotates
81+
* its credential on disk, so another user's Agent-tab open could leave
82+
* analyst's MCP process holding admin's token. Authenticating that token
83+
* would run analyst's tools as admin — reading connections analyst has no
84+
* grant for, and writing admin into the audit row. The request's own
85+
* DEEPSQL_MCP_USER_ID claim ("analyst") contradicts the token owner
86+
* ("admin"), which is the signal to refuse.
87+
*/
88+
@Test
89+
void rejectsTokenWhoseOwnerDiffersFromTheDeclaredMcpUser() throws ServletException, IOException {
90+
ReflectionTestUtils.setField(filter, "authEnabled", true);
91+
92+
MockHttpServletRequest request = new MockHttpServletRequest("GET", "/api/connections");
93+
request.addHeader("Authorization", "Bearer dsql_mcp_public.secret");
94+
// This MCP process was provisioned for analyst...
95+
request.addHeader("X-DeepSQL-Client-Agent", "analyst");
96+
request.setRemoteAddr("127.0.0.1");
97+
98+
MockHttpServletResponse response = new MockHttpServletResponse();
99+
MockFilterChain chain = new MockFilterChain();
100+
101+
when(mcpTokenService.looksLikeMcpToken("dsql_mcp_public.secret")).thenReturn(true);
102+
// ...but the token file was overwritten with admin's credential.
103+
when(mcpTokenService.authenticate("dsql_mcp_public.secret", "127.0.0.1"))
104+
.thenReturn(Optional.of(new McpTokenService.AuthenticatedMcpToken(9L, "admin")));
105+
when(userRepository.findByUsernameIgnoreCase("analyst"))
106+
.thenReturn(Optional.of(new com.dbaagent.model.User()));
107+
108+
filter.doFilter(request, response, chain);
109+
110+
assertNull(SecurityContextHolder.getContext().getAuthentication(),
111+
"a mismatched MCP credential must not authenticate anyone");
112+
assertEquals(401, response.getStatus());
113+
assertTrue(response.getContentAsString().contains("mcp_identity_mismatch"));
114+
assertNull(chain.getRequest(), "the request must not reach downstream handlers");
115+
}
116+
117+
/** The normal agent case: the claim matches the token owner. */
118+
@Test
119+
void allowsTokenWhenDeclaredMcpUserMatchesOwner() throws ServletException, IOException {
120+
ReflectionTestUtils.setField(filter, "authEnabled", true);
121+
122+
MockHttpServletRequest request = new MockHttpServletRequest("GET", "/api/connections");
123+
request.addHeader("Authorization", "Bearer dsql_mcp_public.secret");
124+
request.addHeader("X-DeepSQL-Client-Agent", "analyst");
125+
request.setRemoteAddr("127.0.0.1");
126+
127+
MockHttpServletResponse response = new MockHttpServletResponse();
128+
MockFilterChain chain = new MockFilterChain();
129+
130+
when(mcpTokenService.looksLikeMcpToken("dsql_mcp_public.secret")).thenReturn(true);
131+
when(mcpTokenService.authenticate("dsql_mcp_public.secret", "127.0.0.1"))
132+
.thenReturn(Optional.of(new McpTokenService.AuthenticatedMcpToken(9L, "analyst")));
133+
when(userDetailsService.loadUserByUsername("analyst"))
134+
.thenReturn(new User("analyst", "ignored",
135+
List.of(new SimpleGrantedAuthority("ROLE_DEVELOPER"))));
136+
137+
filter.doFilter(request, response, chain);
138+
139+
assertEquals("analyst", SecurityContextHolder.getContext().getAuthentication().getName());
140+
}
141+
142+
/**
143+
* Editor/CLI installs put a *tool* name in this header ("cursor",
144+
* "claude-desktop", any --caller-agent value). Those tokens are not
145+
* agent-provisioned, so the claim must not be compared against a username —
146+
* otherwise every editor MCP install would 401.
147+
*/
148+
@Test
149+
void allowsEditorClientAgentThatIsNotADeepSqlUsername() throws ServletException, IOException {
150+
ReflectionTestUtils.setField(filter, "authEnabled", true);
151+
152+
MockHttpServletRequest request = new MockHttpServletRequest("GET", "/api/connections");
153+
request.addHeader("Authorization", "Bearer dsql_mcp_public.secret");
154+
request.addHeader("X-DeepSQL-Client-Agent", "cursor");
155+
request.setRemoteAddr("127.0.0.1");
156+
157+
MockHttpServletResponse response = new MockHttpServletResponse();
158+
MockFilterChain chain = new MockFilterChain();
159+
160+
when(mcpTokenService.looksLikeMcpToken("dsql_mcp_public.secret")).thenReturn(true);
161+
when(mcpTokenService.authenticate("dsql_mcp_public.secret", "127.0.0.1"))
162+
.thenReturn(Optional.of(new McpTokenService.AuthenticatedMcpToken(11L, "bob")));
163+
when(userRepository.findByUsernameIgnoreCase("cursor")).thenReturn(Optional.empty());
164+
when(userDetailsService.loadUserByUsername("bob"))
165+
.thenReturn(new User("bob", "ignored",
166+
List.of(new SimpleGrantedAuthority("ROLE_DEVELOPER"))));
167+
168+
filter.doFilter(request, response, chain);
169+
170+
assertEquals("bob", SecurityContextHolder.getContext().getAuthentication().getName());
171+
}
70172
}

0 commit comments

Comments
 (0)