Address review thread 4071470992: path check, changelog, timeout help, error message, cross-platform test, output path sandboxing

Agent-Logs-Url: https://github.com/advanced-security/codeql-development-mcp-server/sessions/0666a5dd-b460-4e6e-9749-f729c46f0b62

Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com>

Author: Copilot

CHANGELOG.md

@@ -79,7 +79,7 @@ _Changes on `main` since the latest tagged release that have not yet been includ
 - `McpProvider.requestRestart()` now invalidates the environment cache and bumps a `+rN` revision suffix so VS Code reliably restarts the MCP server after configuration changes. ([#196](https://github.com/advanced-security/codeql-development-mcp-server/pull/196))
 - Cached the extension version in the provider constructor to avoid repeated synchronous reads of `package.json`. ([#196](https://github.com/advanced-security/codeql-development-mcp-server/pull/196))
 - New `codeql-mcp.enableAnnotationTools` setting (default: `true`) auto-sets `ENABLE_ANNOTATION_TOOLS` and `MONITORING_STORAGE_LOCATION` environment variables; `additionalEnv` overrides for advanced users. ([#199](https://github.com/advanced-security/codeql-development-mcp-server/pull/199))
-- Simplified annotation tool environment: the extension no longer explicitly sets `ENABLE_ANNOTATION_TOOLS` since the server now defaults to `true`. ([#223](https://github.com/advanced-security/codeql-development-mcp-server/pull/223))
+- The earlier `codeql-mcp.enableAnnotationTools` setting is no longer applicable and has been removed from the extension as annotation tools are now enabled by default. ([#223](https://github.com/advanced-security/codeql-development-mcp-server/pull/223))
 
 #### Infrastructure & CI/CD
 

client/cmd/integration_tests.go

@@ -38,7 +38,7 @@ func init() {
 	f.StringVar(&integrationTestsFlags.tools, "tools", "", "Comma-separated list of tool names to test")
 	f.StringVar(&integrationTestsFlags.tests, "tests", "", "Comma-separated list of test case names to run")
 	f.BoolVar(&integrationTestsFlags.noInstall, "no-install-packs", false, "Skip CodeQL pack installation")
-	f.IntVar(&integrationTestsFlags.timeout, "timeout", 0, "Per-tool-call timeout in seconds (0 = use server defaults)")
+	f.IntVar(&integrationTestsFlags.timeout, "timeout", 0, "Per-tool-call timeout in seconds (0 = use client defaults)")
 }
 
 // mcpToolCaller adapts the MCP client to the ToolCaller interface.

client/internal/mcp/client.go

@@ -40,7 +40,7 @@ const (
 	ConnectTimeout = 30 * time.Second
 
 	// CloseTimeoutErr is the error message returned when Close times out.
-	CloseTimeoutErr = "MCP client close timed out after 3s; server subprocess may still be running"
+	CloseTimeoutErr = "MCP client close timed out after 3s; attempted to kill server subprocess"
 )
 
 // Config holds the configuration for connecting to an MCP server.

client/internal/mcp/client_test.go

@@ -2,6 +2,7 @@ package mcp
 
 import (
 	"context"
+	"os"
 	"os/exec"
 	"strings"
 	"testing"
@@ -41,6 +42,18 @@ func (h *hangCloser) ListResources(_ context.Context, _ mcp.ListResourcesRequest
 	return nil, nil
 }
 
+// TestMain handles the subprocess helper mode used by
+// TestClose_KillsSubprocessOnTimeout. When GO_TEST_HANG_SUBPROCESS is set,
+// the process simply blocks forever, simulating a stuck MCP server on all
+// platforms (no dependency on an external `sleep` binary).
+func TestMain(m *testing.M) {
+	if os.Getenv("GO_TEST_HANG_SUBPROCESS") == "1" {
+		// Block forever so the parent test can verify force-kill behaviour.
+		select {}
+	}
+	os.Exit(m.Run())
+}
+
 func TestTimeoutForTool_CodeQLTools(t *testing.T) {
 	codeqlTools := []string{
 		"codeql_query_run",
@@ -113,18 +126,25 @@ func TestClose_TimeoutReturnsError(t *testing.T) {
 	if CloseTimeoutErr == "" {
 		t.Fatal("CloseTimeoutErr should not be empty")
 	}
-	if CloseTimeoutErr != "MCP client close timed out after 3s; server subprocess may still be running" {
+	if CloseTimeoutErr != "MCP client close timed out after 3s; attempted to kill server subprocess" {
 		t.Errorf("CloseTimeoutErr = %q, want specific timeout message", CloseTimeoutErr)
 	}
 }
 
 func TestClose_KillsSubprocessOnTimeout(t *testing.T) {
 	t.Parallel()
 
-	// Start a long-running subprocess to simulate a stuck server.
-	proc := exec.Command("sleep", "60")
+	// Re-exec the current test binary as a subprocess that blocks forever.
+	// This avoids a dependency on an external `sleep` binary (not available
+	// on Windows) while still exercising the real process-kill path.
+	exe, err := os.Executable()
+	if err != nil {
+		t.Fatalf("cannot determine test executable: %v", err)
+	}
+	proc := exec.Command(exe, "-test.run=^$") // run no tests; just reach TestMain
+	proc.Env = append(os.Environ(), "GO_TEST_HANG_SUBPROCESS=1")
 	if err := proc.Start(); err != nil {
-		t.Skipf("cannot start subprocess for test: %v", err)
+		t.Fatalf("cannot start subprocess for test: %v", err)
 	}
 
 	hang := &hangCloser{released: make(chan struct{})}
@@ -135,17 +155,17 @@ func TestClose_KillsSubprocessOnTimeout(t *testing.T) {
 	}
 
 	start := time.Now()
-	err := c.Close()
+	closeErr := c.Close()
 	elapsed := time.Since(start)
 
 	// Release the hanging closer goroutine (cleanup).
 	close(hang.released)
 
-	if err == nil {
+	if closeErr == nil {
 		t.Fatal("Close() should return a timeout error, got nil")
 	}
-	if !strings.Contains(err.Error(), "timed out") {
-		t.Errorf("Close() error = %q; want a message containing 'timed out'", err.Error())
+	if !strings.Contains(closeErr.Error(), "timed out") {
+		t.Errorf("Close() error = %q; want a message containing 'timed out'", closeErr.Error())
 	}
 
 	// Elapsed time should be roughly the 3-second timeout window.
@@ -162,10 +182,9 @@ func TestClose_KillsSubprocessOnTimeout(t *testing.T) {
 
 	select {
 	case waitErr := <-waitCh:
-		// Process exited (possibly as a zombie until Wait() is called).
 		// A killed process returns an error from Wait().
 		if waitErr == nil {
-			t.Error("subprocess exited with success; expected it to be killed (non-zero exit)")
+			t.Error("subprocess exited cleanly; expected it to be killed (non-zero exit)")
 		}
 	case <-time.After(2 * time.Second):
 		_ = proc.Process.Kill()
@@ -183,4 +202,4 @@ func TestConstants(t *testing.T) {
 	if ConnectTimeout != 30*time.Second {
 		t.Errorf("ConnectTimeout = %v, want 30s", ConnectTimeout)
 	}
-}
+}

client/internal/testing/runner.go

@@ -225,9 +225,14 @@ func (r *Runner) runSingleTest(toolName, testCase, toolDir string) {
 	// Resolve {{tmpdir}} placeholders in all params
 	params = resolvePathPlaceholders(params, r.tmpBase)
 
-	// Clean up stale interpretedOutput from prior test runs so that
-	// directory comparisons only see output from this invocation.
-	cleanStaleOutput(toolName, params, ".")
+	// Redirect bare relative output paths (output, interpretedOutput, outputDir)
+	// to a per-test directory under tmpBase so that tool invocations do not
+	// create artifacts in the process working directory (repo root).
+	params = rewriteRelativeOutputPaths(params, r.tmpBase, toolName, testCase)
+
+	// Clean up stale output files from prior runs of THIS test so that
+	// comparisons only see output produced by this invocation.
+	cleanStaleOutput(toolName, params, r.tmpBase)
 
 	// Call the tool (using server tool name which may differ from fixture dir name)
 	content, isError, callErr := r.caller.CallToolRaw(serverToolName, params)
@@ -444,10 +449,39 @@ func fileExists(path string) bool {
 	return err == nil
 }
 
+// rewriteRelativeOutputPaths rewrites bare relative file paths for the
+// "output", "interpretedOutput", and "outputDir" parameters to point inside a
+// per-test subdirectory of tmpBase. This prevents tool invocations from
+// creating artifacts in the process working directory (the repo root).
+//
+// Paths that are already absolute — whether constructed by buildToolParams or
+// resolved from a {{tmpdir}} placeholder — are left unchanged.
+func rewriteRelativeOutputPaths(params map[string]any, tmpBase, toolName, testCase string) map[string]any {
+	outDir := filepath.Join(tmpBase, "test-output", toolName, testCase)
+	result := make(map[string]any, len(params))
+	for k, v := range params {
+		switch k {
+		case "output", "interpretedOutput", "outputDir":
+			if s, ok := v.(string); ok && s != "" && !filepath.IsAbs(s) {
+				if err := os.MkdirAll(outDir, 0o750); err != nil {
+					fmt.Fprintf(os.Stderr, "  warning: cannot create output dir %s: %v\n", outDir, err)
+				}
+				result[k] = filepath.Join(outDir, filepath.Base(s))
+				continue
+			}
+		}
+		result[k] = v
+	}
+	return result
+}
+
 // cleanStaleOutput removes stale interpretedOutput files or directories from
 // prior codeql_query_run test invocations. This prevents stale results from
-// affecting directory comparisons. Only relative paths without directory
-// traversals are cleaned (CWE-22 prevention).
+// affecting directory comparisons.
+//
+// baseDir is used as the root for resolving relative paths. The function only
+// removes paths that resolve to within baseDir (CWE-22 prevention). Absolute
+// paths that are already within baseDir are cleaned directly.
 func cleanStaleOutput(toolName string, params map[string]any, baseDir string) {
 	if toolName != "codeql_query_run" {
 		return
@@ -461,17 +495,31 @@ func cleanStaleOutput(toolName string, params map[string]any, baseDir string) {
 		return
 	}
 
-	normalized := filepath.Clean(outputPath)
+	// Resolve to an absolute path.
+	var fullPath string
+	if filepath.IsAbs(outputPath) {
+		fullPath = outputPath
+	} else {
+		normalized := filepath.Clean(outputPath)
+		// Reject bare directory traversals in relative paths.
+		if normalized == ".." || strings.HasPrefix(normalized, ".."+string(filepath.Separator)) {
+			fmt.Fprintf(os.Stderr, "  Skipping interpretedOutput cleanup: unsafe path %q\n", outputPath)
+			return
+		}
+		fullPath = filepath.Join(baseDir, normalized)
+	}
 
-	// Reject absolute paths and directory traversals (CWE-22).
-	if filepath.IsAbs(normalized) ||
-		strings.HasPrefix(normalized, ".."+string(filepath.Separator)) ||
-		normalized == ".." {
-		fmt.Fprintf(os.Stderr, "  Skipping interpretedOutput cleanup: unsafe path %q\n", outputPath)
+	// Only remove paths within baseDir (CWE-22 prevention).
+	absBase, err := filepath.Abs(baseDir)
+	if err != nil {
+		return
+	}
+	rel, err := filepath.Rel(absBase, fullPath)
+	if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
+		fmt.Fprintf(os.Stderr, "  Skipping interpretedOutput cleanup: path %q is outside base dir\n", outputPath)
 		return
 	}
 
-	fullPath := filepath.Join(baseDir, normalized)
 	os.RemoveAll(fullPath)
 }
 

client/internal/testing/runner_test.go

@@ -3,6 +3,7 @@ package testing
 import (
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 )
 
@@ -66,6 +67,75 @@ func TestResolvePathPlaceholders(t *testing.T) {
 	}
 }
 
+func TestRewriteRelativeOutputPaths(t *testing.T) {
+	dir := t.TempDir()
+
+	tests := []struct {
+		name       string
+		params     map[string]any
+		wantAbsKey string
+		wantBase   string
+	}{
+		{
+			name:       "relative output redirected to tmpBase",
+			params:     map[string]any{"output": "query-results.bqrs"},
+			wantAbsKey: "output",
+			wantBase:   filepath.Join(dir, "test-output", "codeql_query_run", "my_test"),
+		},
+		{
+			name:       "relative interpretedOutput redirected to tmpBase",
+			params:     map[string]any{"interpretedOutput": "query-results.sarif"},
+			wantAbsKey: "interpretedOutput",
+			wantBase:   filepath.Join(dir, "test-output", "codeql_query_run", "my_test"),
+		},
+		{
+			name:       "absolute output left unchanged",
+			params:     map[string]any{"output": filepath.Join(dir, "after", "output.txt")},
+			wantAbsKey: "",
+		},
+		{
+			name: "non-output keys left unchanged",
+			params: map[string]any{
+				"query":    "relative/query.ql",
+				"database": "relative/db",
+			},
+			wantAbsKey: "",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := rewriteRelativeOutputPaths(tt.params, dir, "codeql_query_run", "my_test")
+
+			if tt.wantAbsKey == "" {
+				// Verify output-related keys are unchanged (or non-existent)
+				for k, orig := range tt.params {
+					if got, ok := result[k]; !ok || got != orig {
+						t.Errorf("key %q: got %v, want %v (should be unchanged)", k, got, orig)
+					}
+				}
+				return
+			}
+
+			// Verify the key was rewritten to an absolute path under the expected base
+			got, ok := result[tt.wantAbsKey]
+			if !ok {
+				t.Fatalf("key %q missing from result", tt.wantAbsKey)
+			}
+			gotStr, ok := got.(string)
+			if !ok {
+				t.Fatalf("key %q is not a string: %T", tt.wantAbsKey, got)
+			}
+			if !filepath.IsAbs(gotStr) {
+				t.Errorf("rewritten path %q is not absolute", gotStr)
+			}
+			if !strings.HasPrefix(gotStr, tt.wantBase) {
+				t.Errorf("rewritten path %q does not start with expected base %q", gotStr, tt.wantBase)
+			}
+		})
+	}
+}
+
 func TestToolPriority(t *testing.T) {
 	tests := []struct {
 		name     string
@@ -408,19 +478,42 @@ func TestCleanStaleOutputRelativeDir(t *testing.T) {
 	}
 }
 
-func TestCleanStaleOutputRejectsAbsolutePath(t *testing.T) {
+func TestCleanStaleOutputAbsolutePathWithinBase(t *testing.T) {
+	// After rewriteRelativeOutputPaths, interpretedOutput is an absolute path
+	// within tmpBase. cleanStaleOutput should clean those paths.
 	dir := t.TempDir()
-	absFile := filepath.Join(dir, "safe-file")
-	os.WriteFile(absFile, []byte("keep"), 0o600)
+	staleFile := filepath.Join(dir, "test-output", "query-results.sarif")
+	os.MkdirAll(filepath.Dir(staleFile), 0o755)
+	os.WriteFile(staleFile, []byte("stale"), 0o600)
 
 	params := map[string]any{
-		"interpretedOutput": absFile, // absolute path
+		"interpretedOutput": staleFile, // absolute path inside dir
 	}
 
 	cleanStaleOutput("codeql_query_run", params, dir)
 
-	if !fileExists(absFile) {
-		t.Error("absolute path should NOT be removed")
+	if fileExists(staleFile) {
+		t.Error("absolute path within base dir should be removed")
+	}
+}
+
+func TestCleanStaleOutputRejectsAbsolutePathOutsideBase(t *testing.T) {
+	// Absolute paths that are OUTSIDE baseDir must not be removed.
+	dir := t.TempDir()
+	outsideFile := filepath.Join(dir, "outside-file")
+	os.WriteFile(outsideFile, []byte("keep"), 0o600)
+
+	innerBase := filepath.Join(dir, "inner")
+	os.MkdirAll(innerBase, 0o755)
+
+	params := map[string]any{
+		"interpretedOutput": outsideFile, // absolute path OUTSIDE innerBase
+	}
+
+	cleanStaleOutput("codeql_query_run", params, innerBase)
+
+	if !fileExists(outsideFile) {
+		t.Error("absolute path outside base dir should NOT be removed")
 	}
 }