feat(mcp) add MCP control for Cap.#1942
Conversation
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| no_args_schema(), | ||
| ToolAnnotations::mutating(), | ||
| ), | ||
| tool( |
There was a problem hiding this comment.
When a caller invokes a tool name that doesn't exist, execute_tool returns ToolFailure::InvalidParams, which handle_request maps to JSON-RPC error code -32602 ("Invalid params"). MCP clients that inspect the error code cannot distinguish "you passed bad arguments" from "that tool doesn't exist," and the MCP spec recommends returning an isError: true tool result (not a protocol-level error) for tool-level failures such as an unrecognized name. Under the current mapping, strict clients may treat the response as a protocol violation rather than a tool error and abort the session.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/mcp.rs
Line: 1267
Comment:
**Unknown tool falls through as "Invalid params"**
When a caller invokes a tool name that doesn't exist, `execute_tool` returns `ToolFailure::InvalidParams`, which `handle_request` maps to JSON-RPC error code `-32602` ("Invalid params"). MCP clients that inspect the error code cannot distinguish "you passed bad arguments" from "that tool doesn't exist," and the MCP spec recommends returning an `isError: true` tool result (not a protocol-level error) for tool-level failures such as an unrecognized name. Under the current mapping, strict clients may treat the response as a protocol violation rather than a tool error and abort the session.
How can I resolve this? If you propose a fix, please make it concise.| const httpMcpClientConfig = (config: McpServerConfig) => | ||
| JSON.stringify( | ||
| { | ||
| cap: { | ||
| url: config.endpoint, | ||
| headers: { | ||
| Authorization: `Bearer ${config.token}`, | ||
| }, | ||
| }, | ||
| }, | ||
| null, | ||
| 2, | ||
| ); |
There was a problem hiding this comment.
null values when server has never been enabled
When config.token is null (server was never enabled), the template literal produces "Authorization": "Bearer null" and "url": null in the preview. While the Copy button is disabled in this state, the displayed snippet is misleading. Guard the config display so it only renders the HTTP tab content when both endpoint and token are non-null.
| const httpMcpClientConfig = (config: McpServerConfig) => | |
| JSON.stringify( | |
| { | |
| cap: { | |
| url: config.endpoint, | |
| headers: { | |
| Authorization: `Bearer ${config.token}`, | |
| }, | |
| }, | |
| }, | |
| null, | |
| 2, | |
| ); | |
| const httpMcpClientConfig = (config: McpServerConfig) => { | |
| if (!config.endpoint || !config.token) { | |
| return "Enable MCP and start the server to view the HTTP configuration."; | |
| } | |
| return JSON.stringify( | |
| { | |
| cap: { | |
| url: config.endpoint, | |
| headers: { | |
| Authorization: `Bearer ${config.token}`, | |
| }, | |
| }, | |
| }, | |
| null, | |
| 2, | |
| ); | |
| }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/routes/(window-chrome)/settings/cli.tsx
Line: 78-90
Comment:
**HTTP config preview renders literal `null` values when server has never been enabled**
When `config.token` is `null` (server was never enabled), the template literal produces `"Authorization": "Bearer null"` and `"url": null` in the preview. While the Copy button is disabled in this state, the displayed snippet is misleading. Guard the config display so it only renders the HTTP tab content when both `endpoint` and `token` are non-null.
```suggestion
const httpMcpClientConfig = (config: McpServerConfig) => {
if (!config.endpoint || !config.token) {
return "Enable MCP and start the server to view the HTTP configuration.";
}
return JSON.stringify(
{
cap: {
url: config.endpoint,
headers: {
Authorization: `Bearer ${config.token}`,
},
},
},
null,
2,
);
};
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| "copy_video_to_clipboard", | ||
| "Copy Video To Clipboard", | ||
| "Copy a local video file path to the clipboard.", | ||
| path_schema("path"), |
There was a problem hiding this comment.
auth.strip_prefix("Bearer ") != Some(token) is a standard string equality check, which can leak timing information. Even on localhost, a sufficiently capable adversary making many rapid requests could theoretically exploit this. The subtle crate provides ConstantTimeEq for this pattern. Given the token is 73 characters and the server is localhost-only, the practical risk is very low, but aligning with best practice here is straightforward.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/mcp.rs
Line: 1432
Comment:
**Non-constant-time bearer token comparison**
`auth.strip_prefix("Bearer ") != Some(token)` is a standard string equality check, which can leak timing information. Even on localhost, a sufficiently capable adversary making many rapid requests could theoretically exploit this. The `subtle` crate provides `ConstantTimeEq` for this pattern. Given the token is 73 characters and the server is localhost-only, the practical risk is very low, but aligning with best practice here is straightforward.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| let mut request = client | ||
| .post(&endpoint.endpoint) | ||
| .bearer_auth(&endpoint.token) | ||
| .header("Accept", "application/json, text/event-stream") |
There was a problem hiding this comment.
You’re advertising text/event-stream here but the client always tries to parse the response body as JSON. If the desktop endpoint ever switches to SSE for streaming responses, this will fail; consider restricting the Accept header to JSON unless you’re going to implement SSE parsing.
| .header("Accept", "application/json, text/event-stream") | |
| .header("Accept", "application/json") |
| let mut server = runtime.server.lock().await; | ||
| if let Some(handle) = server.as_ref() { | ||
| settings.port = Some(handle.port); | ||
| settings.endpoint = Some(endpoint_for_port(handle.port)); | ||
| save_settings(app, &settings)?; | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let listener = bind_listener(settings.port).await?; |
There was a problem hiding this comment.
runtime.server.lock().await is held across the bind_listener(...).await call here. It’s probably fine in practice, but it’s an easy way to introduce lock contention / deadlocks later; I’d drop the lock before awaiting and then re-acquire when writing the handle.
| let mut server = runtime.server.lock().await; | |
| if let Some(handle) = server.as_ref() { | |
| settings.port = Some(handle.port); | |
| settings.endpoint = Some(endpoint_for_port(handle.port)); | |
| save_settings(app, &settings)?; | |
| return Ok(()); | |
| } | |
| let listener = bind_listener(settings.port).await?; | |
| { | |
| let mut server = runtime.server.lock().await; | |
| if let Some(handle) = server.as_ref() { | |
| settings.port = Some(handle.port); | |
| settings.endpoint = Some(endpoint_for_port(handle.port)); | |
| save_settings(app, &settings)?; | |
| return Ok(()); | |
| } | |
| } | |
| let listener = bind_listener(settings.port).await?; |
| @@ -2265,15 +2267,18 @@ async fn get_current_recording( | |||
| ) -> Result<JsonValue<Option<CurrentRecording>>, ()> { | |||
| let state = state.read().await; | |||
There was a problem hiding this comment.
This state.read().await guard is held through the whole match below, including an .await (inner.is_paused().await). Holding an async lock across an await can cause subtle contention/deadlocks; consider extracting what you need and dropping the guard before awaiting.
1 participant
A quick demo:
Fixes: #1943
https://github.com/user-attachments/assets/cfb44b64-e736-4379-85e5-f1debe664347
Greptile Summary
This PR adds full MCP (Model Context Protocol) support to Cap, enabling AI agents and external tools to control Cap Desktop via both a local stdio shim (
cap mcp) and a direct HTTP transport.apps/desktop/src-tauri/src/mcp.rs): A new ~2000-line Axum-based HTTP server runs inside the desktop app, exposing 40+ tools behind bearer-token authentication and session management with a 30-minute TTL.apps/cli/src/mcp.rs):cap mcpreads the MCP port and token from the desktop store on disk and proxies stdin/stdout JSON-RPC messages to the local HTTP server.apps/desktop/src/routes/(window-chrome)/settings/cli.tsx): A new MCP section in CLI settings lets users enable/disable the server, rotate the token, and copy ready-to-paste client configs for both local and HTTP transports.Confidence Score: 3/5
The change is broadly well-structured and the auth/session plumbing is sound, but the unknown-tool error response is semantically wrong and could cause MCP client sessions to abort instead of continuing after a bad tool call.
The desktop MCP server is a large new subsystem. The most notable issue is that an unrecognized tool name in tools/call returns a JSON-RPC protocol error (-32602) rather than an isError: true tool result — strict MCP clients may abort the session instead of handling it gracefully. The other findings are cosmetic or low-risk. The auth model is solid and the stdio shim logic is clean and well-tested.
apps/desktop/src-tauri/src/mcp.rs — specifically the execute_tool unknown-tool branch and the token comparison in validate_authorization.
Security Review
validate_authorization): Bearer tokens are compared with standard string equality rather than a constant-time function. Practical risk is very low (localhost only, 73-char token).validate_originfunction correctly blocks non-local origins as a secondary CSRF defense.open_external_linkandstart_video_importaccept arbitrary URLs and paths but all require a valid bearer token.Important Files Changed
pausedfield to CurrentRecording.nullvalues when the server has never been started.mcpmodule and adds theMcpCLI command variant.Comments Outside Diff (1)
apps/desktop/src-tauri/src/mcp.rs, line 2359-2385 (link)rejects_invalid_tool_schematest does not exercise schema validationDespite its name, this test never reaches the schema-validation path.
handle_requestchecks forapp.is_none()and returns-32603("Cap Desktop app handle is unavailable") before callingexecute_tool. The actual invalid-argument path —ToolFailure::InvalidParams→-32602— is never asserted.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat(mcp) add MCP control for Cap." | Re-trigger Greptile