fix(mcp): add timeouts, delete audit log, OAuth state nonce; improve PR review prompt
All checks were successful
Test / frontend-typecheck (pull_request) Successful in 1m54s
Test / rust-fmt-check (pull_request) Successful in 2m0s
Test / frontend-tests (pull_request) Successful in 1m53s
Test / rust-clippy (pull_request) Successful in 3m33s
PR Review Automation / review (pull_request) Successful in 4m54s
Test / rust-tests (pull_request) Successful in 4m57s
All checks were successful
Test / frontend-typecheck (pull_request) Successful in 1m54s
Test / rust-fmt-check (pull_request) Successful in 2m0s
Test / frontend-tests (pull_request) Successful in 1m53s
Test / rust-clippy (pull_request) Successful in 3m33s
PR Review Automation / review (pull_request) Successful in 4m54s
Test / rust-tests (pull_request) Successful in 4m57s
- call_tool: 30s hard timeout via tokio::time::timeout - discover_server: 60s hard timeout wrapping full connect+discover sequence - delete_mcp_server: write_audit_event before cascade delete, capturing server name, tool count, and resource count - initiate_mcp_oauth: append cryptographically random state nonce for CSRF - pr-review.yml: rewrite prompt to require line-quoted evidence for every finding and eliminate hallucinated false positives
This commit is contained in:
parent
8e1145ec7a
commit
28cfcf1ef4
@ -60,7 +60,7 @@ jobs:
|
|||||||
DIFF_CONTENT=$(head -n 500 /tmp/pr_diff.txt \
|
DIFF_CONTENT=$(head -n 500 /tmp/pr_diff.txt \
|
||||||
| grep -v -E '^[+-].*(password[[:space:]]*[=:"'"'"']|token[[:space:]]*[=:"'"'"']|secret[[:space:]]*[=:"'"'"']|api_key[[:space:]]*[=:"'"'"']|private_key[[:space:]]*[=:"'"'"']|Authorization:[[:space:]]|AKIA[A-Z0-9]{16}|xox[baprs]-[0-9]{10,13}-[0-9]{10,13}-[a-zA-Z0-9]{24}|gh[opsu]_[A-Za-z0-9_]{36,}|https?://[^@[:space:]]+:[^@[:space:]]+@)' \
|
| grep -v -E '^[+-].*(password[[:space:]]*[=:"'"'"']|token[[:space:]]*[=:"'"'"']|secret[[:space:]]*[=:"'"'"']|api_key[[:space:]]*[=:"'"'"']|private_key[[:space:]]*[=:"'"'"']|Authorization:[[:space:]]|AKIA[A-Z0-9]{16}|xox[baprs]-[0-9]{10,13}-[0-9]{10,13}-[a-zA-Z0-9]{24}|gh[opsu]_[A-Za-z0-9_]{36,}|https?://[^@[:space:]]+:[^@[:space:]]+@)' \
|
||||||
| grep -v -E '^[+-].*[A-Za-z0-9+/]{40,}={0,2}([^A-Za-z0-9+/=]|$)')
|
| grep -v -E '^[+-].*[A-Za-z0-9+/]{40,}={0,2}([^A-Za-z0-9+/=]|$)')
|
||||||
PROMPT="Analyze the following code changes for correctness, security issues, and best practices. PR Title: ${PR_TITLE}\n\nDiff:\n${DIFF_CONTENT}\n\nProvide a review with: 1) Summary, 2) Bugs/errors, 3) Security issues, 4) Best practices. Give specific comments with suggested fixes."
|
PROMPT="You are a senior engineer performing a focused code review. Your review must be grounded strictly in the diff provided — do not invent issues about code you cannot see.\n\nPR Title: ${PR_TITLE}\n\nDiff:\n${DIFF_CONTENT}\n\n## Instructions\n\n1. **Read the entire diff first.** Before raising any issue, verify it against the actual lines in the diff. If something appears to be missing, confirm it is absent from ALL relevant files in the diff before claiming it is missing.\n\n2. **Quote the evidence.** For every issue you raise, cite the specific file and line from the diff that supports your claim. If you cannot quote a line, do not raise the issue.\n\n3. **Distinguish severity clearly:**\n - BLOCKER: broken right now, will cause crashes or data loss\n - WARNING: works but has a real risk that should be addressed before merge\n - SUGGESTION: improvement worth considering in a follow-up PR\n\n4. **Do not raise issues about code outside the diff.** If a concern requires reading files not present in the diff, say 'outside the scope of this diff' and skip it.\n\n5. **Keep it concise.** Lead with a one-paragraph summary, then list only genuine findings with evidence. Avoid restating what the code already does correctly unless it is directly relevant to a finding.\n\n## Output format\n\n**Summary** (1 paragraph)\n\n**Findings** (only real issues with quoted evidence)\n- [BLOCKER/WARNING/SUGGESTION] filename:line — description and suggested fix\n\n**Verdict**: APPROVE / APPROVE WITH COMMENTS / REQUEST CHANGES"
|
||||||
BODY=$(jq -cn \
|
BODY=$(jq -cn \
|
||||||
--arg model "qwen3-coder-next" \
|
--arg model "qwen3-coder-next" \
|
||||||
--arg content "$PROMPT" \
|
--arg content "$PROMPT" \
|
||||||
|
|||||||
@ -79,6 +79,8 @@ pub async fn list_resources(
|
|||||||
}
|
}
|
||||||
|
|
||||||
/// Call an MCP tool and return its text result.
|
/// Call an MCP tool and return its text result.
|
||||||
|
/// Enforces a 30-second hard timeout to prevent a misbehaving server from
|
||||||
|
/// stalling the AI chat loop indefinitely.
|
||||||
pub async fn call_tool(
|
pub async fn call_tool(
|
||||||
conn: &McpConnection,
|
conn: &McpConnection,
|
||||||
tool_name: &str,
|
tool_name: &str,
|
||||||
@ -91,9 +93,9 @@ pub async fn call_tool(
|
|||||||
None => CallToolRequestParams::new(tool_name.to_string()),
|
None => CallToolRequestParams::new(tool_name.to_string()),
|
||||||
};
|
};
|
||||||
|
|
||||||
let result = conn
|
let result = tokio::time::timeout(std::time::Duration::from_secs(30), conn.call_tool(params))
|
||||||
.call_tool(params)
|
|
||||||
.await
|
.await
|
||||||
|
.map_err(|_| format!("MCP tool '{tool_name}' timed out after 30s"))?
|
||||||
.map_err(|e| format!("MCP tool call failed: {e}"))?;
|
.map_err(|e| format!("MCP tool call failed: {e}"))?;
|
||||||
|
|
||||||
if result.is_error == Some(true) {
|
if result.is_error == Some(true) {
|
||||||
|
|||||||
@ -58,6 +58,28 @@ pub async fn delete_mcp_server(
|
|||||||
) -> Result<(), String> {
|
) -> Result<(), String> {
|
||||||
{
|
{
|
||||||
let db = state.db.lock().map_err(|e| e.to_string())?;
|
let db = state.db.lock().map_err(|e| e.to_string())?;
|
||||||
|
|
||||||
|
// Capture server name and tool count for the audit entry before cascade delete
|
||||||
|
let server_name = get_server(&db, &id)?
|
||||||
|
.map(|s| s.name)
|
||||||
|
.unwrap_or_else(|| id.clone());
|
||||||
|
let tool_count = crate::mcp::store::get_tool_count(&db, &id).unwrap_or(0);
|
||||||
|
let resource_count = crate::mcp::store::get_resource_count(&db, &id).unwrap_or(0);
|
||||||
|
|
||||||
|
let details = serde_json::json!({
|
||||||
|
"server_name": server_name,
|
||||||
|
"tools_deleted": tool_count,
|
||||||
|
"resources_deleted": resource_count,
|
||||||
|
});
|
||||||
|
crate::audit::log::write_audit_event(
|
||||||
|
&db,
|
||||||
|
"mcp_server_deleted",
|
||||||
|
"mcp_server",
|
||||||
|
&id,
|
||||||
|
&details.to_string(),
|
||||||
|
)
|
||||||
|
.map_err(|e| format!("Audit log failed: {e}"))?;
|
||||||
|
|
||||||
delete_server(&db, &id)?;
|
delete_server(&db, &id)?;
|
||||||
}
|
}
|
||||||
// Remove live connection if present
|
// Remove live connection if present
|
||||||
@ -66,7 +88,7 @@ pub async fn delete_mcp_server(
|
|||||||
drop(connections);
|
drop(connections);
|
||||||
|
|
||||||
info!(server_id = %id, "MCP server deleted");
|
info!(server_id = %id, "MCP server deleted");
|
||||||
let _ = app_handle; // suppress unused warning
|
let _ = app_handle;
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -202,7 +224,7 @@ pub async fn initiate_mcp_oauth(
|
|||||||
.to_string();
|
.to_string();
|
||||||
|
|
||||||
let pkce = crate::integrations::auth::generate_pkce();
|
let pkce = crate::integrations::auth::generate_pkce();
|
||||||
let auth_url = crate::integrations::auth::build_auth_url(
|
let base_auth_url = crate::integrations::auth::build_auth_url(
|
||||||
&auth_endpoint,
|
&auth_endpoint,
|
||||||
&client_id,
|
&client_id,
|
||||||
&redirect_uri,
|
&redirect_uri,
|
||||||
@ -210,6 +232,19 @@ pub async fn initiate_mcp_oauth(
|
|||||||
&pkce,
|
&pkce,
|
||||||
);
|
);
|
||||||
|
|
||||||
|
// Append a cryptographically random state nonce for CSRF protection
|
||||||
|
let state_nonce = {
|
||||||
|
use rand::RngCore;
|
||||||
|
let mut bytes = [0u8; 16];
|
||||||
|
rand::rngs::OsRng.fill_bytes(&mut bytes);
|
||||||
|
hex::encode(bytes)
|
||||||
|
};
|
||||||
|
let auth_url = format!(
|
||||||
|
"{}&state={}",
|
||||||
|
base_auth_url,
|
||||||
|
urlencoding::encode(&state_nonce)
|
||||||
|
);
|
||||||
|
|
||||||
// Open WebView window for OAuth
|
// Open WebView window for OAuth
|
||||||
let window_label = format!("mcp-oauth-{id}");
|
let window_label = format!("mcp-oauth-{id}");
|
||||||
let _ = tauri::WebviewWindowBuilder::new(
|
let _ = tauri::WebviewWindowBuilder::new(
|
||||||
|
|||||||
@ -10,9 +10,22 @@ use crate::mcp::store::{
|
|||||||
|
|
||||||
/// Discover a single MCP server: connect, list tools/resources, persist.
|
/// Discover a single MCP server: connect, list tools/resources, persist.
|
||||||
/// Returns the updated connection on success or a descriptive error string.
|
/// Returns the updated connection on success or a descriptive error string.
|
||||||
|
/// Enforces a 60-second hard timeout over the entire connect + discover sequence.
|
||||||
pub async fn discover_server(
|
pub async fn discover_server(
|
||||||
server: &McpServer,
|
server: &McpServer,
|
||||||
app_handle: &tauri::AppHandle,
|
app_handle: &tauri::AppHandle,
|
||||||
|
) -> Result<McpConnection, String> {
|
||||||
|
tokio::time::timeout(
|
||||||
|
std::time::Duration::from_secs(60),
|
||||||
|
discover_server_inner(server, app_handle),
|
||||||
|
)
|
||||||
|
.await
|
||||||
|
.map_err(|_| format!("Discovery of '{}' timed out after 60s", server.name))?
|
||||||
|
}
|
||||||
|
|
||||||
|
async fn discover_server_inner(
|
||||||
|
server: &McpServer,
|
||||||
|
app_handle: &tauri::AppHandle,
|
||||||
) -> Result<McpConnection, String> {
|
) -> Result<McpConnection, String> {
|
||||||
use tauri::Manager;
|
use tauri::Manager;
|
||||||
|
|
||||||
|
|||||||
Loading…
Reference in New Issue
Block a user