From 287164bf151bda9c0beea8a4efca9abfef49f66f Mon Sep 17 00:00:00 2001 From: csa7mdm Date: Thu, 24 Sep 2026 05:21:00 +0300 Subject: [PATCH 1/5] Security model: SECURITY.md, README section, clean-env note; README timing from the shipped-defaults session Co-Authored-By: Claude Opus 5.5 --- README.md | 13 ++++++++-- SECURITY.md | 72 +++++++++++++++++++++++------------------------------ 2 files changed, 42 insertions(+), 43 deletions(-) diff --git a/README.md b/README.md index ed0d948..4e3b7d0 100644 --- a/README.md +++ b/README.md @@ -55,7 +55,7 @@ claude mcp add dotnetdevmcp -- dnx DotNetDevMCP --yes `dnx` downloads the package from NuGet.org on first run. Prefer a permanent install? `dotnet tool install -g DotNetDevMCP`, then use `dotnetdevmcp` as the command. -Pass `--load-solution ` to have Roslyn load your solution at startup, or let the agent call `SharpTool_LoadSolution` when it needs to. `--http --port 3001` serves Streamable HTTP instead of stdio. `dotnetdevmcp --help` lists everything. +Pass `--load-solution ` to have Roslyn load your solution at startup, or let the agent call `SharpTool_LoadSolution` when it needs to. `--http --port 3001` serves Streamable HTTP instead of stdio (localhost only, no authentication: see [Security](#security)). `--clean-env` starts `dotnet` and `git` with a minimal environment so tokens and cloud credentials in environment variables aren't passed on. `dotnetdevmcp --help` lists everything. Git and Monitoring tools (see the table below) are off by default - a shell an agent already has covers them, and every registered tool costs context tokens in every session. Pass `--enable git,monitoring` (comma-separated and/or repeated, e.g. `--enable git --enable monitoring`) to turn either or both on. @@ -136,10 +136,19 @@ On this repository, editing `ConcurrentExecutor.cs` selects 22 of 44 tests (the | `dotnet_test_run` | 44 | 8.3 s | | `dotnet_test_affected` (change to `ConcurrentExecutor.cs`) | 22 | 6.6 s | -The suite here is small, so the saving is small. On a real library the picture is clearer: [benchmarks/polly](https://github.com/csa7mdm/DotNetDevMCP/blob/main/benchmarks/polly/README.md) replays 40 Polly commits and injects faults into its code. A one-file change ran its 5 affected tests in 4.6 s against 33 s for the net10.0 suite, and the selections included 111 of the 112 tests the injected faults broke (the miss builds its object through reflection). Changes that reach hundreds of tests gain nothing: of the last 40 commits, 16 ran a filtered selection and 24 ran the full suite. The first selection of a session on busy code is slower (Roslyn binds the files it touches, then caches them). `dryRun: true` shows what it picked and why (`via`). +The suite here is small, so the saving is small. On a real library the picture is clearer: [benchmarks/polly](https://github.com/csa7mdm/DotNetDevMCP/blob/main/benchmarks/polly/README.md) replays 40 Polly commits and injects faults into its code. A one-file change ran its 5 affected tests in 5.1 s against 48.1 s for the net10.0 suite (same session), and the selections included 111 of the 112 tests the injected faults broke (the miss builds its object through reflection). Changes that reach hundreds of tests gain nothing: of the last 40 commits, 16 ran a filtered selection and 24 ran the full suite. The first selection of a session on busy code is slower (Roslyn binds the files it touches, then caches them). `dryRun: true` shows what it picked and why (`via`). ![Benchmark on Polly: 5 affected tests in 5.1 s against 48.1 s for the full suite; 6.5 KB of references against 202 KB of grep output](https://raw.githubusercontent.com/csa7mdm/DotNetDevMCP/main/docs/images/benchmark-polly.svg) +## Security + +DotNetDevMCP runs as you, for an agent you trust with your code. `dotnet build` and `dotnet test` execute whatever the solution +contains, so a malicious test or `.csproj` runs with your privileges, exactly as it would in your terminal; the server adds no +sandbox. What it does guarantee: tool arguments can't smuggle extra options into `dotnet` or `git`, Roslyn edits stay inside the +solution directory, and `--clean-env` keeps secrets in environment variables away from child processes. For code you don't +trust, run the agent and the server in a container with no credentials. Don't expose `--http` beyond localhost. Details: +[SECURITY.md](https://github.com/csa7mdm/DotNetDevMCP/blob/main/SECURITY.md#security-model). + ## Help, feedback and contributing - **Questions, ideas, "is this a bug?"**: [Discussions](https://github.com/csa7mdm/DotNetDevMCP/discussions). diff --git a/SECURITY.md b/SECURITY.md index f9c5e46..d3e58f2 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -44,57 +44,47 @@ We practice coordinated disclosure: - Once a fix is ready, we will coordinate a release timeline - We will publicly credit you for the discovery (unless you prefer to remain anonymous) -## Security Best Practices +## Security model -When using DotNetDevMCP: +DotNetDevMCP is a **local developer tool**. It runs as you, on your machine, for an AI agent you chose to trust with your +code. Read this before using it on code you don't trust or exposing it beyond your own machine. -1. **Keep Dependencies Updated** - - Regularly update to the latest version - - Monitor security advisories for .NET and dependencies +### What it can do on your machine -2. **Validate Input** - - Always validate and sanitize user input - - Be cautious with paths and file operations +- **Run code.** `dotnet build` and `dotnet test` execute whatever the solution contains: MSBuild targets (``), build + tasks, source generators, test code. If the agent (or text in the repository steering the agent, i.e. prompt injection) + writes a malicious test or `.csproj`, running the build runs it. This is true of `dotnet build` in any terminal; the + server adds no sandbox. +- **Read and write files.** Roslyn edit tools write only inside the loaded solution's directory (paths are normalized, so + `..` and look-alike sibling folders are rejected). Build, test and git tools accept any path you or the agent give them. +- **See your environment.** Child processes inherit the server's environment variables, including tokens and cloud + credentials, unless you start the server with `--clean-env`. -3. **Least Privilege** - - Run DotNetDevMCP with minimal required permissions - - Avoid running as administrator/root unless necessary +For a local agent that already has a shell (Claude Code, Cursor, Copilot agent mode), none of this is new power: the agent +could run the same commands itself. What the server guarantees is that its own tools don't widen that: arguments are passed +to `dotnet` and `git` as separate arguments and validated (framework, configuration, runtime, git refs), so a crafted value +can't add options such as `-p:CustomBeforeMicrosoftCommonTargets=...` or `--output=...`. -4. **Secure Configuration** - - Use secure defaults - - Review configuration for security implications - - Keep sensitive data (API keys, tokens) out of source control +### Options that reduce exposure -5. **Network Security** - - Use HTTPS for all network communications - - Validate SSL/TLS certificates - - Use secure authentication mechanisms +| Option | What it does | What it does not do | +|---|---|---| +| Default (git and monitoring tools off) | Fewer tools for the agent to misuse | - | +| `--clean-env` | Child processes get a minimal environment: tokens, API keys and cloud credentials in environment variables are not passed on | Not a sandbox: files such as `~/.aws/credentials` and the network are still reachable | +| Edits stay in the solution directory | Roslyn edit tools refuse paths outside it | Doesn't restrict what a build does | -## Known Security Considerations +### Untrusted code -### File System Access -DotNetDevMCP requires file system access to: -- Read source code files -- Execute build and test commands -- Write temporary files +For repositories you don't trust (a pull request from a stranger, a downloaded sample), run the agent and DotNetDevMCP inside a +container or VM with only that repository mounted, no credentials, and restricted network. The server cannot provide that +isolation itself. -**Mitigation**: Run in sandboxed environments when processing untrusted code. +### `--http` mode -### Code Execution -DotNetDevMCP executes: -- `dotnet build` commands -- `dotnet test` commands -- MSBuild scripts - -**Mitigation**: Validate all inputs and use isolated build environments. - -### Dependencies -DotNetDevMCP depends on: -- .NET Runtime -- Roslyn compiler -- Third-party NuGet packages - -**Mitigation**: Regularly update dependencies and monitor for vulnerabilities. +HTTP mode listens on `localhost` only and has **no authentication, TLS or origin checks**. Anyone who can reach the port can +build, test and edit with your privileges. Don't forward the port, put it behind a proxy, or run it on a shared machine. +A multi-user or hosted deployment would need authentication, a sandbox per session and audit logging; DotNetDevMCP doesn't +provide those today. ## Security Updates From 1bf0db52007fa216b8531e8b653c179fab701a87 Mon Sep 17 00:00:00 2001 From: csa7mdm Date: Thu, 24 Sep 2026 05:29:47 +0300 Subject: [PATCH 2/5] Project-level fallback for dotnet_test_affected; drop WorkflowEngine's redundant Task.Run When the Roslyn reference walk in dotnet_test_affected runs out of budget or its selection is too large, it used to run the whole solution. Try a cheaper middle ground first: walk the project reference graph (AffectedTestFinder. FindAffectedTestProjects) to find the test projects that can be affected by the change, and run just those (no name filter) when that's a real subset of all test projects. Falls back to the whole solution only when the reachable set is empty or covers every test project. Response now reports RanScope (selection/projects/solution) and TestProjectsRun alongside the existing SelectionComplete/RanWholeSolution fields. WorkflowEngine wrapped already-async parallel step execution in Task.Run for no reason - ExecuteStepAsync already catches its own exceptions, so starting its task directly is enough to run steps concurrently. Co-Authored-By: Claude Opus 5.5 --- src/DotNetDevMCP.Core/Models/TestingModels.cs | 10 ++ .../WorkflowEngine.cs | 6 +- .../AffectedTestFinder.cs | 55 ++++++++ .../Mcp/Tools/TestingTools.cs | 126 +++++++++++++----- .../AffectedTestFinderTests.cs | 113 ++++++++++++++++ 5 files changed, 278 insertions(+), 32 deletions(-) diff --git a/src/DotNetDevMCP.Core/Models/TestingModels.cs b/src/DotNetDevMCP.Core/Models/TestingModels.cs index 2b9b79f..ac988f6 100644 --- a/src/DotNetDevMCP.Core/Models/TestingModels.cs +++ b/src/DotNetDevMCP.Core/Models/TestingModels.cs @@ -61,3 +61,13 @@ public record AffectedTest(string FullyQualifiedName, string ProjectPath, string /// enough share of everything that running it filtered is likely slower than just running the whole solution. /// public record AffectedTestSelection(IReadOnlyList Tests, bool Complete, int SymbolsSearched, int TotalTestMethods); + +/// +/// Which slice of the solution a `dotnet_test_affected` run actually executed, once the selection/fallback decision is made. +/// Selection: the Roslyn reference-walk selection ran, filtered to just the affected tests (the common case). +/// Projects: the selection was incomplete or too large, so instead of everything, only the test projects reachable from the +/// change via the project reference graph ran (see AffectedTestFinder.FindAffectedTestProjects). +/// Solution: the selection was incomplete or too large, and even the project-reachability fallback covered every test +/// project (or none could be resolved), so the whole solution ran in one invocation, same as before this fallback existed. +/// +public enum AffectedRunScope { Selection, Projects, Solution } diff --git a/src/DotNetDevMCP.Orchestration/WorkflowEngine.cs b/src/DotNetDevMCP.Orchestration/WorkflowEngine.cs index 7293684..374eed9 100644 --- a/src/DotNetDevMCP.Orchestration/WorkflowEngine.cs +++ b/src/DotNetDevMCP.Orchestration/WorkflowEngine.cs @@ -95,12 +95,14 @@ public async Task ExecuteAsync( // Execute parallel steps if (parallelSteps.Any()) { - var parallelTasks = parallelSteps.Select(step => Task.Run(async () => + // ExecuteStepAsync is already async and catches its own exceptions (see below), so starting its task + // directly is enough to run steps concurrently - no thread-pool hop via Task.Run needed. + var parallelTasks = parallelSteps.Select(async step => { ReportProgress(progress, steps.Count, completedSteps, step.Name); var result = await ExecuteStepAsync(step, context, cancellationToken); return (step.Name, result); - }, cancellationToken)); + }); var parallelResults = await Task.WhenAll(parallelTasks); diff --git a/src/DotNetDevMCP.Testing/AffectedTestFinder.cs b/src/DotNetDevMCP.Testing/AffectedTestFinder.cs index 56dde7e..dd997cf 100644 --- a/src/DotNetDevMCP.Testing/AffectedTestFinder.cs +++ b/src/DotNetDevMCP.Testing/AffectedTestFinder.cs @@ -197,6 +197,61 @@ private static bool IsTestProject(Project p) => p.MetadataReferences.Any(r => Path.GetFileName(r.Display ?? "") is var f && (f.StartsWith("xunit", StringComparison.OrdinalIgnoreCase) || f.StartsWith("nunit", StringComparison.OrdinalIgnoreCase) || f.StartsWith("Microsoft.VisualStudio.TestPlatform.TestFramework", StringComparison.OrdinalIgnoreCase) || f.StartsWith("TUnit", StringComparison.OrdinalIgnoreCase))); + /// + /// Cheap fallback for when the reference walk in + /// can't finish, or finishes with too large a selection: instead of tracing symbols, walk the project reference graph. + /// Returns every test project that (transitively) references, via a Roslyn , a project + /// containing one of the changed files - or contains one itself. This is a superset of the affected tests (reflection + /// aside) and cheap, since it only looks at the project graph, not symbols or syntax. + /// + /// + /// Reachability is computed over every TFM variant of every project (unlike , which keeps only + /// the highest-TFM variant): a project's net8.0 variant can reference a different variant of a changed library than its + /// net10.0 variant does, so dropping variants here could miss a real edge. The result is deduped to project FILE paths + /// only at the very end, once every variant has had its say. + /// ponytail: a test project that depends on the changed code only through a PackageReference (no ProjectReference) has no + /// edge in this graph and will not be found. That's a real gap, not a bug - documenting it here and in the tool's + /// response note is the fix, since detecting package-mediated dependencies would need a much heavier analysis. + /// + public static IReadOnlyList FindAffectedTestProjects(Solution solution, IEnumerable changedFiles) + { + var changedProjectIds = new HashSet(); + foreach (var file in changedFiles) + { + var full = Path.GetFullPath(file); + foreach (var id in solution.GetDocumentIdsWithFilePath(full)) changedProjectIds.Add(id.ProjectId); + } + if (changedProjectIds.Count == 0) return []; + + // Reverse ProjectReference edges (referenced -> referencing projects), across all TFM variants. + var dependents = new Dictionary>(); + foreach (var project in solution.Projects) + { + foreach (var reference in project.ProjectReferences) + { + if (!dependents.TryGetValue(reference.ProjectId, out var list)) dependents[reference.ProjectId] = list = []; + list.Add(project.Id); + } + } + + var reachable = new HashSet(changedProjectIds); + var frontier = new Queue(changedProjectIds); + while (frontier.Count > 0) + { + if (!dependents.TryGetValue(frontier.Dequeue(), out var deps)) continue; + foreach (var dep in deps) if (reachable.Add(dep)) frontier.Enqueue(dep); + } + + return reachable.Select(solution.GetProject).OfType().Where(IsTestProject) + .Select(p => p.FilePath).OfType().Distinct(StringComparer.OrdinalIgnoreCase).ToList(); + } + + /// Every test project in the solution, deduped by file path across TFM variants. Denominator for deciding + /// whether reached "basically everything", where running the whole solution + /// in one invocation is simpler than filtering to a selection that isn't actually smaller. + public static IReadOnlyList AllTestProjectFilePaths(Solution solution) => + solution.Projects.Where(IsTestProject).Select(p => p.FilePath).OfType().Distinct(StringComparer.OrdinalIgnoreCase).ToList(); + /// "Polly.Core.Tests(net10.0)" -> 10.0; no TFM suffix -> 0. private static Version TfmVersion(string projectName) { diff --git a/src/DotNetDevMCP.Testing/Mcp/Tools/TestingTools.cs b/src/DotNetDevMCP.Testing/Mcp/Tools/TestingTools.cs index 617b83c..015a572 100644 --- a/src/DotNetDevMCP.Testing/Mcp/Tools/TestingTools.cs +++ b/src/DotNetDevMCP.Testing/Mcp/Tools/TestingTools.cs @@ -92,53 +92,119 @@ public static async Task RunAffected( var list = affected.Select(a => new { a.FullyQualifiedName, Project = Path.GetFileNameWithoutExtension(a.ProjectPath), a.Via }); var selectedFraction = selection.TotalTestMethods > 0 ? (double)affected.Count / selection.TotalTestMethods : 0; - // Two independent reasons to give up on filtering and just run everything: the walk didn't finish (unsafe to trust a - // partial set), or it did finish but the selection is big enough that running it filtered is likely slower anyway - // (measured on Polly: ~4% of tests ran 3.3x faster filtered, ~23% was slower than the whole suite). - var note = !selection.Complete - ? $"Selection stopped after {maxSelectionSeconds}s and {selection.SymbolsSearched} symbols: this change reaches too much code to trace cheaply. The tests listed are a partial set; a run executes the whole solution instead." - : selectedFraction > maxSelectedFraction - ? $"Selected {affected.Count} of {selection.TotalTestMethods} test methods ({selectedFraction:P0}), above the {maxSelectedFraction:P0} threshold: a selection this large is likely slower filtered than running the whole solution. Running the whole solution instead." - : null; - var ranWholeSolution = note is not null; + // Two independent reasons to give up on filtering: the walk didn't finish (unsafe to trust a partial set), or it did + // finish but the selection is big enough that running it filtered is likely slower anyway (measured on Polly: ~4% of + // tests ran 3.3x faster filtered, ~23% was slower than the whole suite). Either way, fall back to running whole test + // projects picked from the (cheap) project reference graph instead of jumping straight to the whole solution - a + // change to one library rarely reaches every test project (measured on Polly: 7 test projects, usually 1-2 affected). + string? note; + AffectedRunScope scope; + IReadOnlyList projectsToRun = []; + IReadOnlyList allTestProjects = []; + if (selection.Complete && selectedFraction <= maxSelectedFraction) + { + scope = AffectedRunScope.Selection; + note = null; + } + else + { + var reason = !selection.Complete + ? $"Selection stopped after {maxSelectionSeconds}s and {selection.SymbolsSearched} symbols: this change reaches too much code to trace cheaply." + : $"Selected {affected.Count} of {selection.TotalTestMethods} test methods ({selectedFraction:P0}), above the {maxSelectedFraction:P0} threshold: a selection this large is likely slower filtered than running the whole solution."; + + var reachableProjects = AffectedTestFinder.FindAffectedTestProjects(solutions.CurrentSolution, files); + allTestProjects = AffectedTestFinder.AllTestProjectFilePaths(solutions.CurrentSolution); + + if (reachableProjects.Count == 0 || reachableProjects.Count >= allTestProjects.Count) + { + scope = AffectedRunScope.Solution; + note = $"{reason} Every test project in the solution is reachable from the change (or none could be resolved to a project); running the whole solution instead."; + } + else + { + scope = AffectedRunScope.Projects; + projectsToRun = reachableProjects; + var names = string.Join(", ", reachableProjects.Select(Path.GetFileName)); + note = $"{reason} Running the test projects reachable from the change via project references instead: {names}. " + + "This is a superset of the affected tests (some of their other tests may also run); a test project that " + + "depends on the changed code only through a NuGet package reference, with no ProjectReference, is not " + + "detected by this fallback and may be missed."; + } + } + var ranWholeSolution = scope == AffectedRunScope.Solution; // kept for compatibility; true only when the whole solution ran. if (dryRun || (selection.Complete && affected.Count == 0)) { - return new { Success = true, ChangedFiles = files, SelectionComplete = selection.Complete, selection.SymbolsSearched, selection.TotalTestMethods, Note = note, RanWholeSolution = ranWholeSolution, AffectedTests = list, Ran = false }; + var plannedProjects = scope switch + { + AffectedRunScope.Projects => projectsToRun, + AffectedRunScope.Solution => allTestProjects, + _ => (IReadOnlyList)[], + }; + return new { Success = true, ChangedFiles = files, SelectionComplete = selection.Complete, selection.SymbolsSearched, selection.TotalTestMethods, Note = note, RanWholeSolution = ranWholeSolution, RanScope = ScopeName(scope), TestProjectsRun = plannedProjects.Select(Path.GetFileName), AffectedTests = list, Ran = false }; } TestRunSummary summary; - if (ranWholeSolution) + IReadOnlyList testProjectsRun; + switch (scope) { - // Incomplete selection, or a selection too large to be worth filtering: the safe/fast answer is everything. One solution-wide run, which builds it once. - summary = await runner.RunAsync(solutions.CurrentSolution.FilePath!, null, null, noBuild, framework, cancellationToken, timeoutSeconds); - } - else - { - var byProject = affected.GroupBy(a => a.ProjectPath).ToList(); + case AffectedRunScope.Solution: + // Incomplete/too-large selection and the project-reachability fallback still covers everything: the + // safe/fast answer is everything. One solution-wide run, which builds it once. + summary = await runner.RunAsync(solutions.CurrentSolution.FilePath!, null, null, noBuild, framework, cancellationToken, timeoutSeconds); + testProjectsRun = allTestProjects; + break; + + case AffectedRunScope.Projects: + // Project-level fallback: build the reachable test projects one at a time (they share references; parallel + // builds of the same outputs collide on file locks), then run all of them in parallel with no name filter - + // every test in each of these projects runs, not just the ones the symbol walk would have picked. + if (!noBuild) + { + foreach (var project in projectsToRun) + { + var tfm = string.IsNullOrWhiteSpace(framework) ? "" : $" --framework {framework}"; + var (exit, stdout, stderr, _) = await TestRunner.RunDotnetAsync($"build \"{project}\" -nologo{tfm}", cancellationToken, TestRunner.DirectoryOf(project)); + if (exit != 0) + { + return new { Success = false, ChangedFiles = files, AffectedTests = list, Ran = false, Error = $"Build failed for {Path.GetFileName(project)}:\n{BuildErrors(stdout + stderr)}" }; + } + } + } + var projectRuns = await Task.WhenAll(projectsToRun.Select(p => runner.RunAsync(p, null, null, noBuild: true, framework, cancellationToken, timeoutSeconds))); + summary = TestRunSummary.Merge(projectRuns); + testProjectsRun = projectsToRun; + break; - // Build one project at a time: test projects share references, and parallel builds of the same outputs collide on file locks. - if (!noBuild) - { - foreach (var project in byProject.Select(g => g.Key)) + default: // Selection + var byProject = affected.GroupBy(a => a.ProjectPath).ToList(); + + // Build one project at a time: test projects share references, and parallel builds of the same outputs collide on file locks. + if (!noBuild) { - var tfm = string.IsNullOrWhiteSpace(framework) ? "" : $" --framework {framework}"; - var (exit, stdout, stderr, _) = await TestRunner.RunDotnetAsync($"build \"{project}\" -nologo{tfm}", cancellationToken, TestRunner.DirectoryOf(project)); - if (exit != 0) + foreach (var project in byProject.Select(g => g.Key)) { - return new { Success = false, ChangedFiles = files, AffectedTests = list, Ran = false, Error = $"Build failed for {Path.GetFileName(project)}:\n{BuildErrors(stdout + stderr)}" }; + var tfm = string.IsNullOrWhiteSpace(framework) ? "" : $" --framework {framework}"; + var (exit, stdout, stderr, _) = await TestRunner.RunDotnetAsync($"build \"{project}\" -nologo{tfm}", cancellationToken, TestRunner.DirectoryOf(project)); + if (exit != 0) + { + return new { Success = false, ChangedFiles = files, AffectedTests = list, Ran = false, Error = $"Build failed for {Path.GetFileName(project)}:\n{BuildErrors(stdout + stderr)}" }; + } } } - } - // Then one dotnet test per affected project, in parallel. - var runs = await Task.WhenAll(byProject.Select(g => runner.RunAsync(g.Key, null, g.Select(a => a.FullyQualifiedName).ToList(), noBuild: true, framework, cancellationToken, timeoutSeconds))); - summary = TestRunSummary.Merge(runs); + // Then one dotnet test per affected project, in parallel. + var runs = await Task.WhenAll(byProject.Select(g => runner.RunAsync(g.Key, null, g.Select(a => a.FullyQualifiedName).ToList(), noBuild: true, framework, cancellationToken, timeoutSeconds))); + summary = TestRunSummary.Merge(runs); + testProjectsRun = byProject.Select(g => g.Key).ToList(); + break; } - return new { Success = summary.Success, ChangedFiles = files, SelectionComplete = selection.Complete, selection.SymbolsSearched, selection.TotalTestMethods, Note = note, RanWholeSolution = ranWholeSolution, AffectedTests = list, Ran = true, Run = Shape(summary) }; + return new { Success = summary.Success, ChangedFiles = files, SelectionComplete = selection.Complete, selection.SymbolsSearched, selection.TotalTestMethods, Note = note, RanWholeSolution = ranWholeSolution, RanScope = ScopeName(scope), TestProjectsRun = testProjectsRun.Select(Path.GetFileName), AffectedTests = list, Ran = true, Run = Shape(summary) }; } + private static string ScopeName(AffectedRunScope scope) => scope.ToString().ToLowerInvariant(); + private static string BuildErrors(string output) { var errors = output.Split('\n').Select(l => l.Trim()).Where(l => l.Contains(": error ")).Distinct().Take(20).ToList(); diff --git a/tests/DotNetDevMCP.Testing.Tests/AffectedTestFinderTests.cs b/tests/DotNetDevMCP.Testing.Tests/AffectedTestFinderTests.cs index f5e62d4..4bf1131 100644 --- a/tests/DotNetDevMCP.Testing.Tests/AffectedTestFinderTests.cs +++ b/tests/DotNetDevMCP.Testing.Tests/AffectedTestFinderTests.cs @@ -95,6 +95,119 @@ public async Task Reports_an_incomplete_selection_instead_of_dropping_tests_when Assert.False(affected.Complete); } + [Fact] + public void Project_fallback_finds_only_the_test_project_that_references_the_changed_library() + { + var solution = BuildTwoLibrarySolution(out var libATests, out _); + + var affected = AffectedTestFinder.FindAffectedTestProjects(solution, [Path.Combine(Root, "src", "A.cs")]); + + Assert.Equal([libATests], affected); + } + + [Fact] + public void Project_fallback_includes_a_test_project_whose_own_file_changed() + { + var solution = BuildTwoLibrarySolution(out var libATests, out _); + + // LibA.Tests.cs belongs to the test project itself, not to any library it references. + var affected = AffectedTestFinder.FindAffectedTestProjects(solution, [Path.Combine(Root, "test", "LibA.Tests.cs")]); + + Assert.Equal([libATests], affected); + } + + [Fact] + public void Project_fallback_walks_every_tfm_variant_of_the_reference_graph() + { + // LibA is multi-targeted, and its net8.0 variant carries a file the net10.0 variant doesn't (e.g. conditional + // compilation). LibA.Tests is multi-targeted too, and each TFM variant references only the matching LibA variant - + // so only the net8.0 edge leads from the changed file to LibA.Tests. Restricting the walk to one TFM (as + // AffectedTestFinder.SearchScope does for the symbol-level search) would miss this; the project-level fallback must not. + var workspace = new AdhocWorkspace(); + var corlib = MetadataReference.CreateFromFile(typeof(object).Assembly.Location); + var runtime = MetadataReference.CreateFromFile(Path.Combine(Path.GetDirectoryName(typeof(object).Assembly.Location)!, "System.Runtime.dll")); + var xunit = MetadataReference.CreateFromFile(typeof(FactAttribute).Assembly.Location); + var solution = workspace.CurrentSolution; + + var libA8 = ProjectId.CreateNewId(); + var libA10 = ProjectId.CreateNewId(); + var libAPath = Path.Combine(Root, "src", "LibA.csproj"); + var net8OnlyFile = Path.Combine(Root, "src", "A.net8.cs"); + solution = solution + .AddProject(ProjectInfo.Create(libA8, VersionStamp.Default, "LibA(net8.0)", "LibA", LanguageNames.CSharp, filePath: libAPath, metadataReferences: [corlib, runtime])) + .AddDocument(DocumentInfo.Create(DocumentId.CreateNewId(libA8), "A.net8.cs", + loader: TextLoader.From(TextAndVersion.Create(SourceText.From("namespace LibA; public class A8 { public int M() => 1; }"), VersionStamp.Default)), filePath: net8OnlyFile)) + .AddProject(ProjectInfo.Create(libA10, VersionStamp.Default, "LibA(net10.0)", "LibA", LanguageNames.CSharp, filePath: libAPath, metadataReferences: [corlib, runtime])); + + var testsPath = Path.Combine(Root, "test", "LibA.Tests.csproj"); + var tests8 = ProjectId.CreateNewId(); + var tests10 = ProjectId.CreateNewId(); + solution = solution + .AddProject(ProjectInfo.Create(tests8, VersionStamp.Default, "LibA.Tests(net8.0)", "LibA.Tests", LanguageNames.CSharp, filePath: testsPath, metadataReferences: [corlib, runtime, xunit])) + .AddProjectReference(tests8, new ProjectReference(libA8)) + .AddProject(ProjectInfo.Create(tests10, VersionStamp.Default, "LibA.Tests(net10.0)", "LibA.Tests", LanguageNames.CSharp, filePath: testsPath, metadataReferences: [corlib, runtime, xunit])) + .AddProjectReference(tests10, new ProjectReference(libA10)); + + var affected = AffectedTestFinder.FindAffectedTestProjects(solution, [net8OnlyFile]); + + Assert.Equal([testsPath], affected); + } + + [Fact] + public void AllTestProjectFilePaths_dedupes_multi_tfm_test_projects_by_file_path() + { + var solution = BuildTwoLibrarySolution(out var libATests, out var libBTests); + + var all = AffectedTestFinder.AllTestProjectFilePaths(solution); + + Assert.Equal([libATests, libBTests], all.Order()); + } + + /// Two independent libraries, each with its own dual-TFM test project referencing only that library. + private static Solution BuildTwoLibrarySolution(out string libATestsPath, out string libBTestsPath) + { + var workspace = new AdhocWorkspace(); + var corlib = MetadataReference.CreateFromFile(typeof(object).Assembly.Location); + var runtime = MetadataReference.CreateFromFile(Path.Combine(Path.GetDirectoryName(typeof(object).Assembly.Location)!, "System.Runtime.dll")); + var xunit = MetadataReference.CreateFromFile(typeof(FactAttribute).Assembly.Location); + var solution = workspace.CurrentSolution; + + ProjectId AddLib(string name, string file, string code) + { + var id = ProjectId.CreateNewId(); + solution = solution + .AddProject(ProjectInfo.Create(id, VersionStamp.Default, name, name, LanguageNames.CSharp, + filePath: Path.Combine(Root, "src", $"{name}.csproj"), metadataReferences: [corlib, runtime])) + .AddDocument(DocumentInfo.Create(DocumentId.CreateNewId(id), file, + loader: TextLoader.From(TextAndVersion.Create(SourceText.From(code), VersionStamp.Default)), filePath: Path.Combine(Root, "src", file))); + return id; + } + + string AddTests(string name, ProjectId libId, string code) + { + var csprojPath = Path.Combine(Root, "test", $"{name}.csproj"); + foreach (var tfm in new[] { "net8.0", "net10.0" }) + { + var id = ProjectId.CreateNewId(); + solution = solution + .AddProject(ProjectInfo.Create(id, VersionStamp.Default, $"{name}({tfm})", name, LanguageNames.CSharp, + filePath: csprojPath, metadataReferences: [corlib, runtime, xunit])) + .AddProjectReference(id, new ProjectReference(libId)) + .AddDocument(DocumentInfo.Create(DocumentId.CreateNewId(id), $"{name}.cs", + loader: TextLoader.From(TextAndVersion.Create(SourceText.From(code), VersionStamp.Default)), filePath: Path.Combine(Root, "test", $"{name}.cs"))); + } + return csprojPath; + } + + var libAId = AddLib("LibA", "A.cs", "namespace LibA; public class A { public int M() => 1; }"); + var libBId = AddLib("LibB", "B.cs", "namespace LibB; public class B { public int M() => 1; }"); + + libATestsPath = AddTests("LibA.Tests", libAId, "using Xunit; namespace LibA.Tests; public class T { [Fact] public void T1() { _ = new LibA.A().M(); } }"); + libBTestsPath = AddTests("LibB.Tests", libBId, "using Xunit; namespace LibB.Tests; public class T { [Fact] public void T1() { _ = new LibB.B().M(); } }"); + + return solution; + } + /// A library and its test project, the test project loaded twice as MSBuildWorkspace does for two TFMs. private static Solution BuildSolution(Dictionary lib, string tests) { From 7da7c24afb1931c3029eea7c98d24e0a66fe0d8b Mon Sep 17 00:00:00 2001 From: csa7mdm Date: Thu, 24 Sep 2026 05:33:29 +0300 Subject: [PATCH 3/5] Project fallback: only projects that declare tests (helper libraries referencing xUnit can't be run) Co-Authored-By: Claude Opus 5.5 --- src/DotNetDevMCP.Testing/AffectedTestFinder.cs | 14 ++++++++++++-- .../AffectedTestFinderTests.cs | 18 +++++++++++++++++- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/src/DotNetDevMCP.Testing/AffectedTestFinder.cs b/src/DotNetDevMCP.Testing/AffectedTestFinder.cs index dd997cf..3da18c6 100644 --- a/src/DotNetDevMCP.Testing/AffectedTestFinder.cs +++ b/src/DotNetDevMCP.Testing/AffectedTestFinder.cs @@ -242,15 +242,25 @@ public static IReadOnlyList FindAffectedTestProjects(Solution solution, foreach (var dep in deps) if (reachable.Add(dep)) frontier.Enqueue(dep); } - return reachable.Select(solution.GetProject).OfType().Where(IsTestProject) + return reachable.Select(solution.GetProject).OfType().Where(IsRunnableTestProject) .Select(p => p.FilePath).OfType().Distinct(StringComparer.OrdinalIgnoreCase).ToList(); } + /// + /// A project `dotnet test` can run: references a test framework AND declares a test method. Helper libraries such as + /// Polly.TestUtils reference xUnit without containing tests, and `dotnet test` on them fails with "No test projects were found". + /// Syntax only, so cheap; a syntax tree already parsed is cached by the workspace. + /// + private static bool IsRunnableTestProject(Project p) => + // ponytail: synchronous parse; the server has no synchronization context and parsing is cheap next to the build that follows. + IsTestProject(p) && p.Documents.Any(d => d.GetSyntaxRootAsync().GetAwaiter().GetResult() is { } root + && root.DescendantNodes().OfType().Any(HasTestAttributeSyntax)); + /// Every test project in the solution, deduped by file path across TFM variants. Denominator for deciding /// whether reached "basically everything", where running the whole solution /// in one invocation is simpler than filtering to a selection that isn't actually smaller. public static IReadOnlyList AllTestProjectFilePaths(Solution solution) => - solution.Projects.Where(IsTestProject).Select(p => p.FilePath).OfType().Distinct(StringComparer.OrdinalIgnoreCase).ToList(); + solution.Projects.Where(IsRunnableTestProject).Select(p => p.FilePath).OfType().Distinct(StringComparer.OrdinalIgnoreCase).ToList(); /// "Polly.Core.Tests(net10.0)" -> 10.0; no TFM suffix -> 0. private static Version TfmVersion(string projectName) diff --git a/tests/DotNetDevMCP.Testing.Tests/AffectedTestFinderTests.cs b/tests/DotNetDevMCP.Testing.Tests/AffectedTestFinderTests.cs index 4bf1131..d246e51 100644 --- a/tests/DotNetDevMCP.Testing.Tests/AffectedTestFinderTests.cs +++ b/tests/DotNetDevMCP.Testing.Tests/AffectedTestFinderTests.cs @@ -140,13 +140,29 @@ public void Project_fallback_walks_every_tfm_variant_of_the_reference_graph() .AddProject(ProjectInfo.Create(libA10, VersionStamp.Default, "LibA(net10.0)", "LibA", LanguageNames.CSharp, filePath: libAPath, metadataReferences: [corlib, runtime])); var testsPath = Path.Combine(Root, "test", "LibA.Tests.csproj"); + var testsFile = Path.Combine(Root, "test", "LibA.Tests.cs"); + const string testsCode = "using Xunit; namespace LibA.Tests; public class T { [Fact] public void T1() { } }"; var tests8 = ProjectId.CreateNewId(); var tests10 = ProjectId.CreateNewId(); solution = solution .AddProject(ProjectInfo.Create(tests8, VersionStamp.Default, "LibA.Tests(net8.0)", "LibA.Tests", LanguageNames.CSharp, filePath: testsPath, metadataReferences: [corlib, runtime, xunit])) .AddProjectReference(tests8, new ProjectReference(libA8)) + .AddDocument(DocumentInfo.Create(DocumentId.CreateNewId(tests8), "LibA.Tests.cs", + loader: TextLoader.From(TextAndVersion.Create(SourceText.From(testsCode), VersionStamp.Default)), filePath: testsFile)) .AddProject(ProjectInfo.Create(tests10, VersionStamp.Default, "LibA.Tests(net10.0)", "LibA.Tests", LanguageNames.CSharp, filePath: testsPath, metadataReferences: [corlib, runtime, xunit])) - .AddProjectReference(tests10, new ProjectReference(libA10)); + .AddProjectReference(tests10, new ProjectReference(libA10)) + .AddDocument(DocumentInfo.Create(DocumentId.CreateNewId(tests10), "LibA.Tests.cs", + loader: TextLoader.From(TextAndVersion.Create(SourceText.From(testsCode), VersionStamp.Default)), filePath: testsFile)); + + // A helper library that references xUnit but declares no tests (like Polly.TestUtils): `dotnet test` can't run it. + var utils = ProjectId.CreateNewId(); + solution = solution + .AddProject(ProjectInfo.Create(utils, VersionStamp.Default, "LibA.TestUtils(net8.0)", "LibA.TestUtils", LanguageNames.CSharp, + filePath: Path.Combine(Root, "test", "LibA.TestUtils.csproj"), metadataReferences: [corlib, runtime, xunit])) + .AddProjectReference(utils, new ProjectReference(libA8)) + .AddDocument(DocumentInfo.Create(DocumentId.CreateNewId(utils), "Fakes.cs", + loader: TextLoader.From(TextAndVersion.Create(SourceText.From("namespace LibA.TestUtils; public static class Fakes { public static int One() => 1; }"), VersionStamp.Default)), + filePath: Path.Combine(Root, "test", "Fakes.cs"))); var affected = AffectedTestFinder.FindAffectedTestProjects(solution, [net8OnlyFile]); From 6ad51355906f4b8504219d17b985e60ef2350265 Mon Sep 17 00:00:00 2001 From: csa7mdm Date: Thu, 24 Sep 2026 05:39:10 +0300 Subject: [PATCH 4/5] Security hardening: arguments as lists, validated names, normalized path boundary, --clean-env, VSTest filter widening - dotnet and git child processes get ProcessStartInfo.ArgumentList (one value = one argument) in BuildService, GitService and TestRunner; framework/runtime/configuration/MSBuild property names and git refs are validated; MSBuild property values escape ';' and ','; under Microsoft.Testing.Platform the filter may only carry --filter* / --treenode-filter options (no -p:/--property smuggling) - PathBoundary.IsWithin (GetFullPath + GetRelativePath) replaces the StartsWith solution-directory check - --clean-env (opt-in): child processes get an allow-listed environment; logs kept/dropped counts, names at Debug - VSTest name filter widens method -> class -> none past the command-line limit, like the MTP path - Tests for every injection string, path bypass, allow-list and widening case Started by a subagent that hit a usage limit before building or writing tests; completed, reviewed and tested here. Co-Authored-By: Claude Opus 5.5 --- src/DotNetDevMCP.Build/BuildService.cs | 134 +++++++++++++--- .../DotNetDevMCP.Build.csproj | 4 + .../Services/DocumentOperationsService.cs | 5 +- src/DotNetDevMCP.Core/ChildProcess.cs | 69 +++++++++ .../DotnetArgumentValidation.cs | 51 +++++++ src/DotNetDevMCP.Core/GitRefValidation.cs | 23 +++ src/DotNetDevMCP.Core/PathBoundary.cs | 44 ++++++ .../DotNetDevMCP.Server.csproj | 1 + src/DotNetDevMCP.Server/Program.cs | 16 +- .../DotNetDevMCP.SourceControl.csproj | 4 + .../Services/GitService.cs | 144 +++++++++++------- .../Mcp/Tools/TestingTools.cs | 25 ++- src/DotNetDevMCP.Testing/TestRunner.cs | 130 +++++++++++++--- .../BuildArgumentTests.cs | 32 ++++ .../SecurityHardeningTests.cs | 122 +++++++++++++++ .../RunnerArgumentTests.cs | 37 +++++ 16 files changed, 733 insertions(+), 108 deletions(-) create mode 100644 src/DotNetDevMCP.Core/ChildProcess.cs create mode 100644 src/DotNetDevMCP.Core/DotnetArgumentValidation.cs create mode 100644 src/DotNetDevMCP.Core/GitRefValidation.cs create mode 100644 src/DotNetDevMCP.Core/PathBoundary.cs create mode 100644 tests/DotNetDevMCP.Build.Tests/BuildArgumentTests.cs create mode 100644 tests/DotNetDevMCP.Core.Tests/SecurityHardeningTests.cs create mode 100644 tests/DotNetDevMCP.Testing.Tests/RunnerArgumentTests.cs diff --git a/src/DotNetDevMCP.Build/BuildService.cs b/src/DotNetDevMCP.Build/BuildService.cs index ada6f85..0d68931 100644 --- a/src/DotNetDevMCP.Build/BuildService.cs +++ b/src/DotNetDevMCP.Build/BuildService.cs @@ -2,6 +2,7 @@ using System.Diagnostics; using System.Text.RegularExpressions; +using DotNetDevMCP.Core; namespace DotNetDevMCP.Build; @@ -71,21 +72,38 @@ public async Task BuildAsync( options ??= new BuildOptions(); var stopwatch = Stopwatch.StartNew(); + var validationError = ValidateOptions(options); + if (validationError != null) + { + return ValidationFailure("BUILD002", validationError, stopwatch.Elapsed); + } + + string fullProjectPath; + try + { + fullProjectPath = NormalizeProjectPath(projectPath); + } + catch (ArgumentException ex) + { + return ValidationFailure("BUILD002", ex.Message, stopwatch.Elapsed); + } + try { - var arguments = BuildArguments("build", projectPath, options); + var arguments = BuildArgumentList("build", fullProjectPath, options); var startInfo = new ProcessStartInfo { FileName = "dotnet", - Arguments = arguments, UseShellExecute = false, RedirectStandardInput = true, RedirectStandardOutput = true, RedirectStandardError = true, CreateNoWindow = true, - WorkingDirectory = Path.GetDirectoryName(projectPath) ?? Environment.CurrentDirectory + WorkingDirectory = Path.GetDirectoryName(fullProjectPath) ?? Environment.CurrentDirectory }; + foreach (var arg in arguments) startInfo.ArgumentList.Add(arg); + ChildProcess.Prepare(startInfo); using var process = new Process { StartInfo = startInfo }; var output = new List(); @@ -154,21 +172,38 @@ public async Task CleanAsync( options ??= new BuildOptions(); var stopwatch = Stopwatch.StartNew(); + var validationError = ValidateOptions(options); + if (validationError != null) + { + return ValidationFailure("CLEAN002", validationError, stopwatch.Elapsed); + } + + string fullProjectPath; try { - var arguments = BuildArguments("clean", projectPath, options); + fullProjectPath = NormalizeProjectPath(projectPath); + } + catch (ArgumentException ex) + { + return ValidationFailure("CLEAN002", ex.Message, stopwatch.Elapsed); + } + + try + { + var arguments = BuildArgumentList("clean", fullProjectPath, options); var startInfo = new ProcessStartInfo { FileName = "dotnet", - Arguments = arguments, UseShellExecute = false, RedirectStandardInput = true, RedirectStandardOutput = true, RedirectStandardError = true, CreateNoWindow = true, - WorkingDirectory = Path.GetDirectoryName(Path.GetFullPath(projectPath)) ?? Environment.CurrentDirectory + WorkingDirectory = Path.GetDirectoryName(fullProjectPath) ?? Environment.CurrentDirectory }; + foreach (var arg in arguments) startInfo.ArgumentList.Add(arg); + ChildProcess.Prepare(startInfo); using var process = new Process { StartInfo = startInfo }; var output = new List(); @@ -221,19 +256,31 @@ public async Task RestoreAsync( { var stopwatch = Stopwatch.StartNew(); + string fullProjectPath; + try + { + fullProjectPath = NormalizeProjectPath(projectPath); + } + catch (ArgumentException ex) + { + return ValidationFailure("RESTORE002", ex.Message, stopwatch.Elapsed); + } + try { var startInfo = new ProcessStartInfo { FileName = "dotnet", - Arguments = $"restore \"{projectPath}\"", UseShellExecute = false, RedirectStandardInput = true, RedirectStandardOutput = true, RedirectStandardError = true, CreateNoWindow = true, - WorkingDirectory = Path.GetDirectoryName(Path.GetFullPath(projectPath)) ?? Environment.CurrentDirectory + WorkingDirectory = Path.GetDirectoryName(fullProjectPath) ?? Environment.CurrentDirectory }; + startInfo.ArgumentList.Add("restore"); + startInfo.ArgumentList.Add(fullProjectPath); + ChildProcess.Prepare(startInfo); using var process = new Process { StartInfo = startInfo }; var output = new List(); @@ -277,24 +324,60 @@ public async Task RestoreAsync( } } - private static string BuildArguments(string command, string projectPath, BuildOptions options) + /// + /// Checks every value in that names something (framework, runtime, + /// configuration, MSBuild property names) before it reaches a process argument. Called once up front + /// so build/clean fail fast with a clear message instead of handing dotnet a value that could be + /// misread as another option or, for a property value, another MSBuild property. + /// + private static string? ValidateOptions(BuildOptions options) => + DotnetArgumentValidation.ValidateFramework(options.Framework) + ?? DotnetArgumentValidation.ValidateRuntime(options.Runtime) + ?? DotnetArgumentValidation.ValidateConfiguration(options.Configuration) + ?? (options.Properties?.Keys.Select(DotnetArgumentValidation.ValidatePropertyName).FirstOrDefault(e => e != null)); + + /// Resolves and validates a caller-supplied project/solution path. Doesn't restrict it to any + /// directory (building another project on disk is a legitimate use), just normalizes it and rejects + /// what isn't a usable path. + private static string NormalizeProjectPath(string projectPath) { - var args = new List { command, $"\"{projectPath}\"" }; - - if (options.Configuration != null) - args.Add($"--configuration {options.Configuration}"); - - if (options.Framework != null) - args.Add($"--framework {options.Framework}"); + if (string.IsNullOrWhiteSpace(projectPath)) + throw new ArgumentException("projectPath is required."); + try + { + return Path.GetFullPath(projectPath); + } + catch (Exception ex) when (ex is ArgumentException or NotSupportedException or PathTooLongException) + { + throw new ArgumentException($"Invalid projectPath '{projectPath}': {ex.Message}"); + } + } - if (options.Runtime != null) - args.Add($"--runtime {options.Runtime}"); + private static BuildResult ValidationFailure(string code, string message, TimeSpan elapsed) => new( + Success: false, + ExitCode: -1, + Duration: elapsed, + Warnings: 0, + Errors: 1, + Output: message, + Diagnostics: new[] { new BuildDiagnostic(DiagnosticSeverity.Error, code, message) }); - if (options.NoBuild) - args.Add("--no-build"); + /// + /// Builds the dotnet CLI arguments for build/clean as a list: each value becomes exactly one + /// entry, so .NET does the quoting and a value like + /// "1.0 -p:CustomBeforeMicrosoftCommonTargets=C:\evil.targets" can never be split into extra + /// arguments the way it would be if concatenated into a single argument string. Assumes + /// already passed. + /// + public static List BuildArgumentList(string command, string projectPath, BuildOptions options) + { + var args = new List { command, projectPath }; - if (options.NoRestore) - args.Add("--no-restore"); + if (options.Configuration != null) { args.Add("--configuration"); args.Add(options.Configuration); } + if (options.Framework != null) { args.Add("--framework"); args.Add(options.Framework); } + if (options.Runtime != null) { args.Add("--runtime"); args.Add(options.Runtime); } + if (options.NoBuild) args.Add("--no-build"); + if (options.NoRestore) args.Add("--no-restore"); if (options.Verbosity != null) { @@ -306,18 +389,19 @@ private static string BuildArguments(string command, string projectPath, BuildOp 3 => "detailed", _ => "diagnostic" }; - args.Add($"--verbosity {verbosity}"); + args.Add("--verbosity"); + args.Add(verbosity); } if (options.Properties != null) { foreach (var prop in options.Properties) { - args.Add($"-p:{prop.Key}={prop.Value}"); + args.Add($"-p:{prop.Key}={DotnetArgumentValidation.EscapePropertyValue(prop.Value)}"); } } - return string.Join(" ", args); + return args; } private static List ParseDiagnostics(string output) diff --git a/src/DotNetDevMCP.Build/DotNetDevMCP.Build.csproj b/src/DotNetDevMCP.Build/DotNetDevMCP.Build.csproj index cc40514..4f5643e 100644 --- a/src/DotNetDevMCP.Build/DotNetDevMCP.Build.csproj +++ b/src/DotNetDevMCP.Build/DotNetDevMCP.Build.csproj @@ -6,6 +6,10 @@ enable + + + + diff --git a/src/DotNetDevMCP.CodeIntelligence/Services/DocumentOperationsService.cs b/src/DotNetDevMCP.CodeIntelligence/Services/DocumentOperationsService.cs index b53708d..34532cc 100644 --- a/src/DotNetDevMCP.CodeIntelligence/Services/DocumentOperationsService.cs +++ b/src/DotNetDevMCP.CodeIntelligence/Services/DocumentOperationsService.cs @@ -5,6 +5,7 @@ using System.Threading; using System.Threading.Tasks; using System.Xml; +using DotNetDevMCP.Core; using Microsoft.CodeAnalysis.Text; namespace DotNetDevMCP.CodeIntelligence.Services; @@ -332,7 +333,9 @@ private bool IsPathWithinSolutionDirectory(string filePath) { return false; } - return filePath.StartsWith(solutionDirectory, StringComparison.OrdinalIgnoreCase); + // PathBoundary.IsWithin resolves both sides with Path.GetFullPath before comparing, so ".." traversal + // and a sibling directory that merely shares a string prefix (App vs App-other) are both rejected. + return PathBoundary.IsWithin(filePath, solutionDirectory); } private bool IsReferencedBySolution(string filePath) { diff --git a/src/DotNetDevMCP.Core/ChildProcess.cs b/src/DotNetDevMCP.Core/ChildProcess.cs new file mode 100644 index 0000000..e41c6e6 --- /dev/null +++ b/src/DotNetDevMCP.Core/ChildProcess.cs @@ -0,0 +1,69 @@ +// Copyright (c) 2025 Ahmed Mustafa + +using System.Diagnostics; + +namespace DotNetDevMCP.Core; + +/// +/// Shared setup for the dotnet/git child processes spawned by BuildService, GitService and TestRunner. +/// Today this only carries the opt-in environment scrub (--clean-env): when enabled, a child gets +/// a minimal allow-listed environment instead of inheriting the server's full one, so cloud credentials, +/// API keys and tokens sitting in the server's environment don't leak into every dotnet/git invocation. +/// This is a scrub, not a sandbox: an allow-listed child still has the same file-system and network +/// access as the user running the server. +/// +public static class ChildProcess +{ + /// Set once at startup from --clean-env. Off by default: children inherit the full environment. + public static bool CleanEnvironment { get; set; } + + // Case-insensitive everywhere: env var names aren't case-sensitive to us, and Windows itself treats + // them case-insensitively, so applying the same rule on every OS is simpler than special-casing it. + private static readonly StringComparer NameComparer = StringComparer.OrdinalIgnoreCase; + + // Exact names dotnet/MSBuild/NuGet/git need to run at all, plus the handful of shell/user-profile + // variables .NET and git consult (temp dirs, home dir, locale, process info). + private static readonly string[] AllowedNames = + [ + "PATH", "PATHEXT", "SystemRoot", "SYSTEMDRIVE", "windir", "ComSpec", "OS", + "TEMP", "TMP", "TMPDIR", "HOME", "USERPROFILE", "HOMEDRIVE", "HOMEPATH", + "APPDATA", "LOCALAPPDATA", "ProgramData", "ProgramFiles", "ProgramFiles(x86)", "ProgramW6432", + "NUMBER_OF_PROCESSORS", "PROCESSOR_ARCHITECTURE", "USERNAME", "USER", "LOGNAME", "LANG", + // Restore behind a corporate proxy needs these (they may carry credentials, but without them NuGet can't reach a feed). + "HTTP_PROXY", "HTTPS_PROXY", "NO_PROXY", "ALL_PROXY", + ]; + + // Prefixes rather than exact names: CommonProgramFiles/(x86)/W6432, every DOTNET_*/NUGET_*/MSBUILD* + // tuning variable (including ones this process sets itself, like DOTNET_CLI_UI_LANGUAGE), and LC_*. + private static readonly string[] AllowedPrefixes = + [ + "CommonProgramFiles", "DOTNET_", "NUGET_", "MSBUILD", "LC_", + "SSL_CERT_", // custom CA bundles on Linux/macOS + "XDG_", // NuGet and git config locations on Linux + ]; + + /// Applies the scrub to if is on. No-op otherwise. + public static void Prepare(ProcessStartInfo startInfo) + { + if (!CleanEnvironment) return; + foreach (var name in startInfo.Environment.Keys.Where(k => !IsAllowed(k)).ToList()) + { + startInfo.Environment.Remove(name); + } + } + + /// + /// Reports what the scrub would do against this process's own environment, for the one-time startup + /// log line (never per spawn: the allow-list is fixed, so the count doesn't change between children). + /// + public static (int Kept, int Dropped, IReadOnlyList DroppedNames) DescribeEnvironment() + { + var names = Environment.GetEnvironmentVariables().Keys.Cast().ToList(); + var dropped = names.Where(n => !IsAllowed(n)).OrderBy(n => n, NameComparer).ToList(); + return (names.Count - dropped.Count, dropped.Count, dropped); + } + + private static bool IsAllowed(string name) => + AllowedNames.Contains(name, NameComparer) + || AllowedPrefixes.Any(p => name.StartsWith(p, StringComparison.OrdinalIgnoreCase)); +} diff --git a/src/DotNetDevMCP.Core/DotnetArgumentValidation.cs b/src/DotNetDevMCP.Core/DotnetArgumentValidation.cs new file mode 100644 index 0000000..fd728ea --- /dev/null +++ b/src/DotNetDevMCP.Core/DotnetArgumentValidation.cs @@ -0,0 +1,51 @@ +// Copyright (c) 2025 Ahmed Mustafa + +using System.Diagnostics; +using System.Text.RegularExpressions; + +namespace DotNetDevMCP.Core; + +/// +/// Validates the values that name things in dotnet/MSBuild command lines (framework, runtime, +/// configuration, MSBuild property names) before they become process arguments. Paired with +/// (each value exactly one argument, no shell involved), +/// this closes argument injection like a framework of "net10.0 -p:CustomBeforeMicrosoftCommonTargets= +/// C:\evil.targets" that would otherwise import an attacker's targets file. Shared by BuildService and +/// TestRunner, both of which accept framework/runtime/configuration from callers. +/// +public static class DotnetArgumentValidation +{ + // TFM (net10.0, net8.0-windows) or RID (win-x64, linux-x64): letters, digits, dot, dash. Must start + // with an alphanumeric so a value can never itself look like an option (e.g. "--logger:x"). + private static readonly Regex TfmOrRidShape = new(@"^[A-Za-z0-9][A-Za-z0-9.-]*$", RegexOptions.Compiled); + private static readonly Regex ConfigurationShape = new(@"^[A-Za-z0-9_-]+$", RegexOptions.Compiled); + private static readonly Regex MSBuildPropertyNameShape = new(@"^[A-Za-z_][A-Za-z0-9_.-]*$", RegexOptions.Compiled); + + public static string? ValidateFramework(string? framework) => + framework is null || TfmOrRidShape.IsMatch(framework) + ? null + : $"Invalid framework '{framework}': expected a target framework moniker like net10.0 or net8.0-windows."; + + public static string? ValidateRuntime(string? runtime) => + runtime is null || TfmOrRidShape.IsMatch(runtime) + ? null + : $"Invalid runtime '{runtime}': expected a runtime identifier like win-x64 or linux-x64."; + + public static string? ValidateConfiguration(string? configuration) => + configuration is null || ConfigurationShape.IsMatch(configuration) + ? null + : $"Invalid configuration '{configuration}': letters, digits, '_' and '-' only."; + + public static string? ValidatePropertyName(string name) => + MSBuildPropertyNameShape.IsMatch(name) + ? null + : $"Invalid MSBuild property name '{name}': must start with a letter or '_', followed by letters, digits, '_', '.' or '-'."; + + /// + /// MSBuild splits a "-p:Name=Value" switch on ';' and ',' into a property list, so either one unescaped in the + /// value lets a caller smuggle in a second property (e.g. "1,CustomBeforeMicrosoftCommonTargets=C:\evil.targets"). + /// ponytail: only the two separators are escaped, not the full MSBuild %XX grammar ($, @, etc.); that covers + /// the property-list-injection case this hardening pass targets. + /// + public static string EscapePropertyValue(string value) => value.Replace(";", "%3B").Replace(",", "%2C"); +} diff --git a/src/DotNetDevMCP.Core/GitRefValidation.cs b/src/DotNetDevMCP.Core/GitRefValidation.cs new file mode 100644 index 0000000..8d9c68c --- /dev/null +++ b/src/DotNetDevMCP.Core/GitRefValidation.cs @@ -0,0 +1,23 @@ +// Copyright (c) 2025 Ahmed Mustafa + +namespace DotNetDevMCP.Core; + +/// +/// Validates a git ref/branch/remote name supplied by a caller before it becomes a process argument. +/// Git treats any token starting with '-' as an option rather than a ref, so an unvalidated value like +/// "--upload-pack=x" (as a remote) or "--output=C:/x.txt" (as a diff base) gets parsed as a flag instead +/// of the ref/path it looks like. Rejecting a leading '-' and control characters keeps every well-formed +/// ref, branch and remote name working while closing that off. Shared by GitService (SourceControl) and +/// the git-diff path in TestingTools (Testing), both of which take a caller-supplied ref. +/// +public static class GitRefValidation +{ + public static string? Validate(string? value, string paramName) + { + if (value is null) return null; + if (value.Length == 0) return $"{paramName} must not be empty."; + if (value[0] == '-') return $"Invalid {paramName} '{value}': must not start with '-' (would be parsed as a git option)."; + if (value.Any(char.IsControl)) return $"Invalid {paramName} '{value}': control characters are not allowed."; + return null; + } +} diff --git a/src/DotNetDevMCP.Core/PathBoundary.cs b/src/DotNetDevMCP.Core/PathBoundary.cs new file mode 100644 index 0000000..8e803db --- /dev/null +++ b/src/DotNetDevMCP.Core/PathBoundary.cs @@ -0,0 +1,44 @@ +// Copyright (c) 2025 Ahmed Mustafa + +namespace DotNetDevMCP.Core; + +/// +/// Directory-boundary check shared by tools that must confirm a file lives under a given root +/// (e.g. the loaded solution directory). A plain StartsWith on the raw strings is fooled by +/// "..' traversal (C:\src\App\..\..\x) and by a sibling directory that merely shares a string +/// prefix (C:\src\App-other\x "starts with" C:\src\App); resolving both paths first and +/// comparing the relative path between them closes both holes. +/// +public static class PathBoundary +{ + /// + /// True if , once fully resolved, is itself or + /// nested under it. Both inputs may be relative; they are resolved against the current directory + /// the same way would. Comparison is ordinal (case-sensitive + /// on Linux/macOS, case-insensitive on Windows) via , which already + /// applies the right casing rule for the running OS. + /// + public static bool IsWithin(string path, string directory) + { + if (string.IsNullOrEmpty(path) || string.IsNullOrEmpty(directory)) return false; + + string fullPath, fullDirectory; + try + { + fullPath = Path.GetFullPath(path); + fullDirectory = Path.GetFullPath(directory); + } + catch (Exception ex) when (ex is ArgumentException or NotSupportedException or PathTooLongException) + { + return false; + } + + var relative = Path.GetRelativePath(fullDirectory, fullPath); + // "." = same directory. Anything escaping the root comes back rooted (different drive) or starting + // with "..": both cases mean the resolved path is outside, however the raw strings looked. + return relative == "." + || (relative != ".." + && !relative.StartsWith(".." + Path.DirectorySeparatorChar, StringComparison.Ordinal) + && !Path.IsPathRooted(relative)); + } +} diff --git a/src/DotNetDevMCP.Server/DotNetDevMCP.Server.csproj b/src/DotNetDevMCP.Server/DotNetDevMCP.Server.csproj index 428a595..1d151a3 100644 --- a/src/DotNetDevMCP.Server/DotNetDevMCP.Server.csproj +++ b/src/DotNetDevMCP.Server/DotNetDevMCP.Server.csproj @@ -29,6 +29,7 @@ + diff --git a/src/DotNetDevMCP.Server/Program.cs b/src/DotNetDevMCP.Server/Program.cs index 621e99e..0808c5e 100644 --- a/src/DotNetDevMCP.Server/Program.cs +++ b/src/DotNetDevMCP.Server/Program.cs @@ -9,6 +9,7 @@ using DotNetDevMCP.Build.Extensions; using DotNetDevMCP.CodeIntelligence.Extensions; using DotNetDevMCP.CodeIntelligence.Interfaces; +using DotNetDevMCP.Core; using DotNetDevMCP.Monitoring.Extensions; using DotNetDevMCP.Monitoring.Mcp.Tools; using DotNetDevMCP.Orchestration.Extensions; @@ -40,6 +41,7 @@ public static async Task Main(string[] args) var buildConfigurationOption = new Option("--build-configuration") { Description = "Build configuration used when loading the solution (Debug, Release)." }; var gitCommitEditsOption = new Option("--git-commit-edits") { Description = "Let edit tools (RenameSymbol, OverwriteMember, AddMember, MoveMember, FindAndReplace, CreateRoslynDocument, OverwriteRoslynDocument, ManageUsings, ManageAttributes) create a git branch and commit after each change, and enable SharpTool_Undo. Off by default: edits are still applied to disk and compile-checked, they just don't touch git or your current branch." }; var disableGitOption = new Option("--disable-git") { Description = "Deprecated, no-op. Git integration in code-intelligence tools is off by default; use --git-commit-edits to opt in." }; + var cleanEnvOption = new Option("--clean-env") { Description = "Give every dotnet/git child process a minimal, allow-listed environment instead of inheriting this server's full one. Scrubs environment variables only; it is not a sandbox: child processes still run with your user's file-system and network access. Off by default." }; var enableOption = new Option("--enable") { Description = "Enable optional tool groups, off by default: 'git' (repo status/branch/stage/commit/push/pull/log/diff) and 'monitoring' (process performance/GC/health/profiling). Comma-separated and/or repeated, e.g. \"--enable git,monitoring\" or \"--enable git --enable monitoring\".", @@ -50,7 +52,7 @@ public static async Task Main(string[] args) var root = new RootCommand("DotNetDevMCP - MCP server for .NET development: Roslyn code intelligence, build, affected-test selection, git, orchestration.") { - httpOption, portOption, logDirOption, logLevelOption, loadSolutionOption, buildConfigurationOption, gitCommitEditsOption, disableGitOption, enableOption + httpOption, portOption, logDirOption, logLevelOption, loadSolutionOption, buildConfigurationOption, gitCommitEditsOption, disableGitOption, enableOption, cleanEnvOption }; var parsed = root.Parse(args); @@ -72,6 +74,7 @@ public static async Task Main(string[] args) string[] enabledGroups = parsed.GetValue(enableOption) ?? []; bool enableGit = enabledGroups.Contains("git"); bool enableMonitoring = enabledGroups.Contains("monitoring"); + bool cleanEnv = parsed.GetValue(cleanEnvOption); Log.Logger = BuildLogger(logLevel, logDir); @@ -80,6 +83,17 @@ public static async Task Main(string[] args) Log.Warning("--disable-git is deprecated and has no effect: git integration is already off by default. Use --git-commit-edits to opt into it."); } + if (cleanEnv) + { + ChildProcess.CleanEnvironment = true; + var (kept, dropped, droppedNames) = ChildProcess.DescribeEnvironment(); + Log.Information( + "--clean-env enabled: dotnet/git child processes get a minimal environment ({Kept} variables kept, {Dropped} dropped). " + + "Scrubs environment variables only; it is not a sandbox: child processes still run with your user's file-system and network access.", + kept, dropped); + Log.Debug("Dropped environment variables: {Names}", string.Join(", ", droppedNames)); + } + try { Log.Information("Starting {App} v{Version} ({Transport})", ApplicationName, ApplicationVersion, http ? $"http://localhost:{port}" : "stdio"); diff --git a/src/DotNetDevMCP.SourceControl/DotNetDevMCP.SourceControl.csproj b/src/DotNetDevMCP.SourceControl/DotNetDevMCP.SourceControl.csproj index 5acafda..0b747b7 100644 --- a/src/DotNetDevMCP.SourceControl/DotNetDevMCP.SourceControl.csproj +++ b/src/DotNetDevMCP.SourceControl/DotNetDevMCP.SourceControl.csproj @@ -6,6 +6,10 @@ enable + + + + diff --git a/src/DotNetDevMCP.SourceControl/Services/GitService.cs b/src/DotNetDevMCP.SourceControl/Services/GitService.cs index cc924a3..f288025 100644 --- a/src/DotNetDevMCP.SourceControl/Services/GitService.cs +++ b/src/DotNetDevMCP.SourceControl/Services/GitService.cs @@ -3,6 +3,7 @@ using System.Diagnostics; using System.Text.Json; using System.Text.RegularExpressions; +using DotNetDevMCP.Core; namespace DotNetDevMCP.SourceControl.Services; @@ -64,18 +65,18 @@ public async Task GetRepoInfoAsync(string repoPath, CancellationTok { try { - var rootResult = await RunGitCommandAsync(repoPath, "rev-parse --show-toplevel", cancellationToken); + var rootResult = await RunGitCommandAsync(repoPath, ["rev-parse", "--show-toplevel"], cancellationToken); if (!rootResult.Success) throw new InvalidOperationException("Not a git repository"); var rootPath = rootResult.Output.Trim(); - var branchResult = await RunGitCommandAsync(rootPath, "branch --show-current", cancellationToken); + var branchResult = await RunGitCommandAsync(rootPath, ["branch", "--show-current"], cancellationToken); var currentBranch = branchResult.Output.Trim(); - var statusResult = await RunGitCommandAsync(rootPath, "status --porcelain", cancellationToken); + var statusResult = await RunGitCommandAsync(rootPath, ["status", "--porcelain"], cancellationToken); var changes = ParseStatus(statusResult.Output); - var remoteResult = await RunGitCommandAsync(rootPath, "remote", cancellationToken); + var remoteResult = await RunGitCommandAsync(rootPath, ["remote"], cancellationToken); var remotes = remoteResult.Output.Split('\n', StringSplitOptions.RemoveEmptyEntries); var (ahead, behind) = await GetAheadBehindCountAsync(rootPath, currentBranch, cancellationToken); @@ -101,7 +102,7 @@ public async Task GetRepoInfoAsync(string repoPath, CancellationTok /// public async Task GetCurrentBranchAsync(string repoPath, CancellationToken cancellationToken = default) { - var result = await RunGitCommandAsync(repoPath, "branch --show-current", cancellationToken); + var result = await RunGitCommandAsync(repoPath, ["branch", "--show-current"], cancellationToken); EnsureSuccess(result); return result.Output.Trim(); } @@ -111,8 +112,8 @@ public async Task GetCurrentBranchAsync(string repoPath, CancellationTok /// public async Task> GetBranchesAsync(string repoPath, bool includeRemote = false, CancellationToken cancellationToken = default) { - var command = includeRemote ? "branch -a" : "branch"; - var result = await RunGitCommandAsync(repoPath, command, cancellationToken); + var args = includeRemote ? new[] { "branch", "-a" } : new[] { "branch" }; + var result = await RunGitCommandAsync(repoPath, args, cancellationToken); EnsureSuccess(result); return result.Output.Split('\n', StringSplitOptions.RemoveEmptyEntries) @@ -125,11 +126,13 @@ public async Task> GetBranchesAsync(string repoPath, bool in /// public async Task CreateBranchAsync(string repoPath, string branchName, string? startPoint = null, CancellationToken cancellationToken = default) { - var command = startPoint != null - ? $"checkout -b {branchName} {startPoint}" - : $"checkout -b {branchName}"; - - return await RunGitCommandAsync(repoPath, command, cancellationToken); + var refError = GitRefValidation.Validate(branchName, nameof(branchName)) ?? GitRefValidation.Validate(startPoint, nameof(startPoint)); + if (refError != null) return Failed(refError); + + var args = new List { "checkout", "-b", branchName }; + if (startPoint != null) args.Add(startPoint); + + return await RunGitCommandAsync(repoPath, args, cancellationToken); } /// @@ -137,7 +140,10 @@ public async Task CreateBranchAsync(string repoPath, string branchNam /// public async Task CheckoutBranchAsync(string repoPath, string branchName, CancellationToken cancellationToken = default) { - return await RunGitCommandAsync(repoPath, $"checkout {branchName}", cancellationToken); + var refError = GitRefValidation.Validate(branchName, nameof(branchName)); + if (refError != null) return Failed(refError); + + return await RunGitCommandAsync(repoPath, ["checkout", branchName], cancellationToken); } /// @@ -145,8 +151,10 @@ public async Task CheckoutBranchAsync(string repoPath, string branchN /// public async Task StageAsync(string repoPath, IEnumerable files, CancellationToken cancellationToken = default) { - var fileList = string.Join(" ", files.Select(f => $"\"{f}\"")); - return await RunGitCommandAsync(repoPath, $"add {fileList}", cancellationToken); + // "--" ends option parsing: a file named e.g. "-x" is then unambiguously a pathspec, not a flag. + var args = new List { "add", "--" }; + args.AddRange(files); + return await RunGitCommandAsync(repoPath, args, cancellationToken); } /// @@ -154,7 +162,7 @@ public async Task StageAsync(string repoPath, IEnumerable fil /// public async Task StageAllAsync(string repoPath, CancellationToken cancellationToken = default) { - return await RunGitCommandAsync(repoPath, "add -A", cancellationToken); + return await RunGitCommandAsync(repoPath, ["add", "-A"], cancellationToken); } /// @@ -162,11 +170,12 @@ public async Task StageAllAsync(string repoPath, CancellationToken ca /// public async Task CommitAsync(string repoPath, string message, bool allowEmpty = false, CancellationToken cancellationToken = default) { - var command = allowEmpty - ? $"commit --allow-empty -m \"{message}\"" - : $"commit -m \"{message}\""; - - return await RunGitCommandAsync(repoPath, command, cancellationToken); + var args = new List { "commit" }; + if (allowEmpty) args.Add("--allow-empty"); + args.Add("-m"); + args.Add(message); + + return await RunGitCommandAsync(repoPath, args, cancellationToken); } /// @@ -174,12 +183,15 @@ public async Task CommitAsync(string repoPath, string message, bool a /// public async Task PushAsync(string repoPath, string? remote = null, string? branch = null, bool force = false, CancellationToken cancellationToken = default) { - var command = "push"; - if (force) command += " --force"; - if (remote != null) command += $" {remote}"; - if (branch != null) command += $" {branch}"; + var refError = GitRefValidation.Validate(remote, nameof(remote)) ?? GitRefValidation.Validate(branch, nameof(branch)); + if (refError != null) return Failed(refError); + + var args = new List { "push" }; + if (force) args.Add("--force"); + if (remote != null) args.Add(remote); + if (branch != null) args.Add(branch); - return await RunGitCommandAsync(repoPath, command, cancellationToken); + return await RunGitCommandAsync(repoPath, args, cancellationToken); } /// @@ -187,11 +199,14 @@ public async Task PushAsync(string repoPath, string? remote = null, s /// public async Task PullAsync(string repoPath, string? remote = null, string? branch = null, CancellationToken cancellationToken = default) { - var command = "pull"; - if (remote != null) command += $" {remote}"; - if (branch != null) command += $" {branch}"; + var refError = GitRefValidation.Validate(remote, nameof(remote)) ?? GitRefValidation.Validate(branch, nameof(branch)); + if (refError != null) return Failed(refError); - return await RunGitCommandAsync(repoPath, command, cancellationToken); + var args = new List { "pull" }; + if (remote != null) args.Add(remote); + if (branch != null) args.Add(branch); + + return await RunGitCommandAsync(repoPath, args, cancellationToken); } /// @@ -199,8 +214,11 @@ public async Task PullAsync(string repoPath, string? remote = null, s /// public async Task FetchAsync(string repoPath, string? remote = null, CancellationToken cancellationToken = default) { - var command = remote != null ? $"fetch {remote}" : "fetch --all"; - return await RunGitCommandAsync(repoPath, command, cancellationToken); + var refError = GitRefValidation.Validate(remote, nameof(remote)); + if (refError != null) return Failed(refError); + + var args = remote != null ? new[] { "fetch", remote } : new[] { "fetch", "--all" }; + return await RunGitCommandAsync(repoPath, args, cancellationToken); } /// @@ -208,11 +226,13 @@ public async Task FetchAsync(string repoPath, string? remote = null, /// public async Task> GetLogAsync(string repoPath, int count = 10, string? branch = null, CancellationToken cancellationToken = default) { - var format = "--pretty=format:%H|%an|%ae|%ad|%s"; - var command = $"log {format} -{count}"; - if (branch != null) command += $" {branch}"; + var refError = GitRefValidation.Validate(branch, nameof(branch)); + if (refError != null) throw new InvalidOperationException(refError); + + var args = new List { "log", "--pretty=format:%H|%an|%ae|%ad|%s", $"-{count}" }; + if (branch != null) args.Add(branch); - var result = await RunGitCommandAsync(repoPath, command, cancellationToken); + var result = await RunGitCommandAsync(repoPath, args, cancellationToken); EnsureSuccess(result); return result.Output.Split('\n', StringSplitOptions.RemoveEmptyEntries) @@ -233,37 +253,55 @@ public async Task> GetLogAsync(string repoPath, int count /// public async Task DiffAsync(string repoPath, string? file = null, string? commit1 = null, string? commit2 = null, CancellationToken cancellationToken = default) { - var command = "diff"; - if (commit1 != null && commit2 != null) - command = $"diff {commit1} {commit2}"; - else if (commit1 != null) - command = $"diff {commit1}"; - - if (file != null) - command += $" -- \"{file}\""; - - return await RunGitCommandAsync(repoPath, command, cancellationToken); + var refError = GitRefValidation.Validate(commit1, nameof(commit1)) ?? GitRefValidation.Validate(commit2, nameof(commit2)); + if (refError != null) return Failed(refError); + + var args = new List { "diff" }; + if (commit1 != null) args.Add(commit1); + if (commit2 != null) args.Add(commit2); + // "--" ends option/ref parsing: whatever follows is unambiguously the pathspec, never re-parsed as a ref or flag. + if (file != null) { args.Add("--"); args.Add(file); } + + return await RunGitCommandAsync(repoPath, args, cancellationToken); } + private static GitResult Failed(string error) => new(Success: false, Output: string.Empty, Error: error, Duration: TimeSpan.Zero); + /// - /// Runs a git command + /// Runs a git command. Arguments go through so each value is + /// exactly one argument (.NET does the quoting) and can never be split into extra arguments the way + /// concatenating into a single argument string would allow, e.g. a diff base of "--output=C:/x.txt". /// - private async Task RunGitCommandAsync(string repoPath, string arguments, CancellationToken cancellationToken = default) + private async Task RunGitCommandAsync(string repoPath, IReadOnlyList arguments, CancellationToken cancellationToken = default) { var stopwatch = Stopwatch.StartNew(); - + + if (string.IsNullOrWhiteSpace(repoPath)) + return Failed("repoPath is required."); + + string fullRepoPath; + try + { + fullRepoPath = Path.GetFullPath(repoPath); + } + catch (Exception ex) when (ex is ArgumentException or NotSupportedException or PathTooLongException) + { + return Failed($"Invalid repoPath '{repoPath}': {ex.Message}"); + } + try { var startInfo = new ProcessStartInfo { FileName = "git", - Arguments = arguments, UseShellExecute = false, RedirectStandardOutput = true, RedirectStandardError = true, CreateNoWindow = true, - WorkingDirectory = repoPath + WorkingDirectory = fullRepoPath }; + foreach (var arg in arguments) startInfo.ArgumentList.Add(arg); + ChildProcess.Prepare(startInfo); using var process = new Process { StartInfo = startInfo }; var output = new List(); @@ -348,7 +386,9 @@ private static IEnumerable GetUntrackedFiles(string output) private async Task<(int Ahead, int Behind)> GetAheadBehindCountAsync(string repoPath, string branch, CancellationToken cancellationToken = default) { - var result = await RunGitCommandAsync(repoPath, $"rev-list --left-right --count origin/{branch}...{branch}", cancellationToken); + // branch comes from our own "branch --show-current" output, not a caller-supplied value, so it + // doesn't need GitRefValidation; it's still one ArgumentList entry, so no quoting is needed either. + var result = await RunGitCommandAsync(repoPath, ["rev-list", "--left-right", "--count", $"origin/{branch}...{branch}"], cancellationToken); if (!result.Success) return (0, 0); diff --git a/src/DotNetDevMCP.Testing/Mcp/Tools/TestingTools.cs b/src/DotNetDevMCP.Testing/Mcp/Tools/TestingTools.cs index 617b83c..b75da8c 100644 --- a/src/DotNetDevMCP.Testing/Mcp/Tools/TestingTools.cs +++ b/src/DotNetDevMCP.Testing/Mcp/Tools/TestingTools.cs @@ -2,6 +2,7 @@ using System.ComponentModel; using DotNetDevMCP.CodeIntelligence.Interfaces; +using DotNetDevMCP.Core; using DotNetDevMCP.Core.Models; using Microsoft.Extensions.Logging; using ModelContextProtocol.Server; @@ -38,7 +39,7 @@ public static async Task Run( TestRunner runner, ILogger logger, [Description("Path to a test project (.csproj) or a solution (.sln)")] string path, - [Description("VSTest filter expression, e.g. FullyQualifiedName~OrderService|Category=Unit")] string? filter = null, + [Description("VSTest filter expression, e.g. FullyQualifiedName~OrderService|Category=Unit. For a project that runs under Microsoft.Testing.Platform, this is instead that test framework's own filter options, e.g. xUnit v3's `--filter-class My.Tests`; only --filter* and --treenode-filter options are accepted.")] string? filter = null, [Description("Exact fully qualified test names to run (Namespace.Class.Method). Combined with filter if both given.")] string[]? testNames = null, [Description("Skip the build. Only when nothing changed since the last build.")] bool noBuild = false, [Description("Run one target framework only, e.g. net10.0. Default: every framework the projects target.")] string? framework = null, @@ -73,6 +74,12 @@ public static async Task RunAffected( return new { Success = false, Error = "No solution loaded. Call SharpTool_LoadSolution first or start the server with --load-solution." }; } + var gitBaseError = GitRefValidation.Validate(gitBase, nameof(gitBase)); + if (gitBaseError != null) return new { Success = false, Error = gitBaseError }; + + var frameworkError = DotnetArgumentValidation.ValidateFramework(framework); + if (frameworkError != null) return new { Success = false, Error = frameworkError }; + var solutionDir = Path.GetDirectoryName(solutions.CurrentSolution.FilePath!)!; var sw = System.Diagnostics.Stopwatch.StartNew(); var files = changedFiles is { Length: > 0 } ? changedFiles : await GitChangedFilesAsync(solutionDir, gitBase, cancellationToken); @@ -122,8 +129,9 @@ public static async Task RunAffected( { foreach (var project in byProject.Select(g => g.Key)) { - var tfm = string.IsNullOrWhiteSpace(framework) ? "" : $" --framework {framework}"; - var (exit, stdout, stderr, _) = await TestRunner.RunDotnetAsync($"build \"{project}\" -nologo{tfm}", cancellationToken, TestRunner.DirectoryOf(project)); + var buildArgs = new List { "build", project, "-nologo" }; + if (!string.IsNullOrWhiteSpace(framework)) { buildArgs.Add("--framework"); buildArgs.Add(framework); } + var (exit, stdout, stderr, _) = await TestRunner.RunDotnetAsync(buildArgs, cancellationToken, TestRunner.DirectoryOf(project)); if (exit != 0) { return new { Success = false, ChangedFiles = files, AffectedTests = list, Ran = false, Error = $"Build failed for {Path.GetFileName(project)}:\n{BuildErrors(stdout + stderr)}" }; @@ -157,12 +165,17 @@ private static string BuildErrors(string output) Failures = s.Failures.Select(f => new { f.FullyQualifiedName, f.ErrorMessage, f.StackTrace, f.Output }), }; + // gitBase is validated by the caller (RunAffected) with GitRefValidation before this ever runs; validating + // again here would be redundant, but the ArgumentList below is what actually keeps it from being parsed as + // a git option (e.g. "--output=C:/x.txt") the way concatenating it into a single argument string would allow. private static async Task GitChangedFilesAsync(string repoDir, string? gitBase, CancellationToken ct) { - var args = gitBase is null ? "status --porcelain --untracked-files=all" : $"diff --name-only {gitBase}"; + List args = gitBase is null + ? ["status", "--porcelain", "--untracked-files=all"] + : ["diff", "--name-only", gitBase]; var (exit, output, err, _) = await TestRunner.RunProcessAsync("git", args, ct, repoDir); - if (exit != 0) throw new InvalidOperationException($"git {args} failed: {err.Trim()}"); - var (_, root, _, _) = await TestRunner.RunProcessAsync("git", "rev-parse --show-toplevel", ct, repoDir); + if (exit != 0) throw new InvalidOperationException($"git {string.Join(' ', args)} failed: {err.Trim()}"); + var (_, root, _, _) = await TestRunner.RunProcessAsync("git", ["rev-parse", "--show-toplevel"], ct, repoDir); root = root.Trim().Length == 0 ? repoDir : root.Trim(); return output.Split('\n', StringSplitOptions.RemoveEmptyEntries) .Select(l => gitBase is null ? l[3..].Trim() : l.Trim()) // porcelain lines are "XY path" diff --git a/src/DotNetDevMCP.Testing/TestRunner.cs b/src/DotNetDevMCP.Testing/TestRunner.cs index a1daea0..111b47a 100644 --- a/src/DotNetDevMCP.Testing/TestRunner.cs +++ b/src/DotNetDevMCP.Testing/TestRunner.cs @@ -5,6 +5,7 @@ using System.Text; using System.Text.Json; using System.Xml.Linq; +using DotNetDevMCP.Core; using DotNetDevMCP.Core.Models; namespace DotNetDevMCP.Testing; @@ -20,7 +21,9 @@ public sealed class TestRunner public async Task> DiscoverAsync(string projectPath, string? filter, CancellationToken ct) { projectPath = Path.GetFullPath(projectPath); - var (exit, stdout, stderr, _) = await RunDotnetAsync($"test \"{projectPath}\" --list-tests{FilterArg(filter)}", ct, DirectoryOf(projectPath)); + var args = new List { "test", projectPath, "--list-tests" }; + args.AddRange(FilterArgs(filter)); + var (exit, stdout, stderr, _) = await RunDotnetAsync(args, ct, DirectoryOf(projectPath)); if (exit != 0) { throw new InvalidOperationException($"dotnet test --list-tests failed:\n{Tail(stdout + stderr)}"); @@ -45,8 +48,13 @@ public async Task> DiscoverAsync(string projectPath, str /// would hang the MCP tool call with it. Past this, the whole process tree is killed and a failed summary is returned. public async Task RunAsync(string projectOrSolutionPath, string? filter, IReadOnlyCollection? testNames, bool noBuild, string? framework, CancellationToken ct, int timeoutSeconds = 600) { + var frameworkError = DotnetArgumentValidation.ValidateFramework(framework); + if (frameworkError != null) return TestRunSummary.Failed(frameworkError, TimeSpan.Zero); + projectOrSolutionPath = Path.GetFullPath(projectOrSolutionPath); - return UsesTestingPlatform(projectOrSolutionPath) + var mtp = UsesTestingPlatform(projectOrSolutionPath); + if (mtp && ValidateTestingPlatformFilter(filter) is { } filterError) return TestRunSummary.Failed(filterError, TimeSpan.Zero); + return mtp ? await RunTestingPlatformAsync(projectOrSolutionPath, filter, testNames, noBuild, framework, ct, timeoutSeconds) : await RunVsTestAsync(projectOrSolutionPath, filter, testNames, noBuild, framework, ct, timeoutSeconds); } @@ -88,13 +96,15 @@ private async Task RunTestingPlatformAsync(string path, string? (int ExitCode, string Stdout, string Stderr, bool TimedOut) run = default; foreach (var xunit in IsXunitByPath.TryGetValue(path, out var known) ? new[] { known } : new[] { true, false }) { - var args = new StringBuilder($"test {target} \"{path}\""); - if (noBuild) args.Append(" --no-build"); - if (!string.IsNullOrWhiteSpace(framework)) args.Append($" --framework {framework}"); - args.Append(xunit ? " --report-xunit-trx" : " --report-trx"); - args.Append(TestingPlatformNameFilter(testNames, xunit)); - if (!string.IsNullOrWhiteSpace(filter)) args.Append(' ').Append(filter); // the framework's own filter options, verbatim - run = await RunDotnetAsync(args.ToString(), ct, DirectoryOf(path), TimeSpan.FromSeconds(timeoutSeconds)); + var args = new List { "test", target, path }; + if (noBuild) args.Add("--no-build"); + if (!string.IsNullOrWhiteSpace(framework)) { args.Add("--framework"); args.Add(framework); } + args.Add(xunit ? "--report-xunit-trx" : "--report-trx"); + // TestingPlatformNameFilter still returns its quoted string form (tested as such); splitting it back + // into argv tokens here reproduces exactly what it describes, one ArgumentList entry per token. + args.AddRange(SplitArgs(TestingPlatformNameFilter(testNames, xunit))); + if (!string.IsNullOrWhiteSpace(filter)) args.AddRange(SplitArgs(filter)); // the framework's own filter options, verbatim + run = await RunDotnetAsync(args, ct, DirectoryOf(path), TimeSpan.FromSeconds(timeoutSeconds)); if (run.ExitCode != MtpInvalidCommandLine) { if (!run.TimedOut) IsXunitByPath[path] = xunit; break; } noBuild = true; // the first attempt already built } @@ -178,6 +188,21 @@ string Build(IEnumerable items, string xunitOption, Func private const int MaxFilterChars = 24_000; // Windows caps a command line at 32,767 chars + /// + /// Exact-name OR filter for VSTest, widened past the same way + /// widens for MTP: first to one contains-term per class, then + /// (still too long) to null, meaning "no name filter" — every widening step is a superset of the exact + /// names, so a run under the widened filter never executes fewer tests than the caller asked for. + /// + public static string? WidenVsTestFilter(IReadOnlyCollection names) + { + var byName = string.Join("|", names.Select(n => $"FullyQualifiedName={n}")); + if (byName.Length <= MaxFilterChars) return byName; + var classes = names.Select(n => n.LastIndexOf('.') is var i and > 0 ? n[..i] : n).Distinct().ToList(); + var byClass = string.Join("|", classes.Select(c => $"FullyQualifiedName~{c}.")); + return byClass.Length <= MaxFilterChars ? byClass : null; + } + private async Task RunVsTestAsync(string projectOrSolutionPath, string? filter, IReadOnlyCollection? testNames, bool noBuild, string? framework, CancellationToken ct, int timeoutSeconds) { var resultsDir = Path.Combine(Path.GetTempPath(), "dotnetdevmcp-trx", Guid.NewGuid().ToString("N")); @@ -186,23 +211,23 @@ private async Task RunVsTestAsync(string projectOrSolutionPath, var fullFilter = filter; if (testNames is { Count: > 0 }) { - // ponytail: OR of exact names. Command lines cap around 32k chars on Windows; ~200 long names is the practical ceiling, - // above that callers should run the whole project. - var byName = string.Join("|", testNames.Select(n => $"FullyQualifiedName={n}")); - fullFilter = string.IsNullOrEmpty(filter) ? byName : $"({filter})&({byName})"; + var widened = WidenVsTestFilter(testNames); + fullFilter = widened is null + ? filter // widening gave up entirely: fall back to just the base filter (or none), a superset of the exact names. + : string.IsNullOrEmpty(filter) ? widened : $"({filter})&({widened})"; } - var args = new StringBuilder($"test \"{projectOrSolutionPath}\" --logger trx --results-directory \"{resultsDir}\""); - if (noBuild) args.Append(" --no-build"); - if (!string.IsNullOrWhiteSpace(framework)) args.Append($" --framework {framework}"); + var args = new List { "test", projectOrSolutionPath, "--logger", "trx", "--results-directory", resultsDir }; + if (noBuild) args.Add("--no-build"); + if (!string.IsNullOrWhiteSpace(framework)) { args.Add("--framework"); args.Add(framework); } // Named per-test, not per-run: the process timeout below already bounds the whole run. Naming the hanging test needs a // shorter per-test window, capped at the run timeout so it can never itself become the reason nothing finishes in time. var hangTimeout = Math.Min(120, timeoutSeconds); - args.Append($" --blame-hang --blame-hang-timeout {hangTimeout}s --blame-hang-dump-type none"); // the name, not a multi-GB dump - args.Append(FilterArg(fullFilter)); + args.Add("--blame-hang"); args.Add("--blame-hang-timeout"); args.Add($"{hangTimeout}s"); args.Add("--blame-hang-dump-type"); args.Add("none"); // the name, not a multi-GB dump + args.AddRange(FilterArgs(fullFilter)); var sw = Stopwatch.StartNew(); - var run = await RunDotnetAsync(args.ToString(), ct, DirectoryOf(projectOrSolutionPath), TimeSpan.FromSeconds(timeoutSeconds)); + var run = await RunDotnetAsync(args, ct, DirectoryOf(projectOrSolutionPath), TimeSpan.FromSeconds(timeoutSeconds)); sw.Stop(); if (run.TimedOut) @@ -267,7 +292,62 @@ public static TestRunSummary ParseTrx(XDocument doc) results); } - private static string FilterArg(string? filter) => string.IsNullOrWhiteSpace(filter) ? "" : $" --filter \"{filter.Replace("\"", "\\\"")}\""; + private static IEnumerable FilterArgs(string? filter) => + string.IsNullOrWhiteSpace(filter) ? [] : ["--filter", filter]; + + /// + /// Splits a raw options string into argv tokens on whitespace, respecting double-quoted segments (so + /// e.g. --filter-method "My Test" becomes two tokens, not four). Used to turn a string built for + /// display/length-checking () or a caller-supplied "verbatim" + /// options string into individual entries. + /// + internal static IReadOnlyList SplitArgs(string raw) => SplitArgsCore(raw); + + /// + /// Under Microsoft.Testing.Platform the filter is passed as the test framework's own options, split into arguments. + /// `dotnet test` would also accept MSBuild options there (-p:CustomBeforeMicrosoftCommonTargets=... imports a + /// targets file), so every option in it must be a filter option: --filter* (xUnit v3, MSTest, NUnit) or + /// --treenode-filter (TUnit). Values between options are left alone. Returns an error, or null if allowed. + /// + private static readonly System.Text.RegularExpressions.Regex MsBuildSlashSwitch = new(@"^/[A-Za-z][A-Za-z0-9-]*([:=].*)?$"); + + public static string? ValidateTestingPlatformFilter(string? filter) + { + if (string.IsNullOrWhiteSpace(filter)) return null; + foreach (var token in SplitArgsCore(filter)) + { + // A value (method name, TUnit tree path like /*/*/MyClass/*) passes. MSBuild also takes Windows-style switches + // (/p:X=1), so a '/' token that is shaped like one (/word, or /word: or /word= followed by anything) counts as an option; tree paths continue with '/' or '*' instead. + if (!token.StartsWith('-') && !MsBuildSlashSwitch.IsMatch(token)) continue; + var option = token.Split('=', ':')[0]; + if (!option.StartsWith("--filter", StringComparison.Ordinal) && option != "--treenode-filter") + { + return $"Invalid filter option '{token}': under Microsoft.Testing.Platform only --filter* and --treenode-filter options are allowed."; + } + } + return null; + } + + private static IReadOnlyList SplitArgsCore(string raw) + { + var result = new List(); + var current = new StringBuilder(); + var inQuotes = false; + var hasToken = false; + foreach (var c in raw) + { + if (c == '"') { inQuotes = !inQuotes; hasToken = true; continue; } + if (!inQuotes && char.IsWhiteSpace(c)) + { + if (hasToken) { result.Add(current.ToString()); current.Clear(); hasToken = false; } + continue; + } + current.Append(c); + hasToken = true; + } + if (hasToken) result.Add(current.ToString()); + return result; + } private static string Tail(string s, int lines = 40) { @@ -281,18 +361,20 @@ private static string Tail(string s, int lines = 40) /// public static string DirectoryOf(string path) => Directory.Exists(path) ? path : Path.GetDirectoryName(Path.GetFullPath(path))!; - internal static Task<(int ExitCode, string Stdout, string Stderr, bool TimedOut)> RunDotnetAsync(string arguments, CancellationToken ct, string? workingDirectory = null, TimeSpan? timeout = null) + internal static Task<(int ExitCode, string Stdout, string Stderr, bool TimedOut)> RunDotnetAsync(IReadOnlyList arguments, CancellationToken ct, string? workingDirectory = null, TimeSpan? timeout = null) => RunProcessAsync("dotnet", arguments, ct, workingDirectory, timeout); /// /// Runs a child with all three std handles redirected: in stdio mode the parent's stdin/stdout ARE the MCP channel. /// A run must always return, so bounds it: past it, the whole process tree is killed /// (a child dotnet/testhost survives its parent otherwise) and TimedOut comes back true instead of throwing. + /// Arguments go through , one entry per value, so .NET does the + /// quoting and nothing can smuggle in extra arguments the way concatenating a single argument string would allow. /// internal static async Task<(int ExitCode, string Stdout, string Stderr, bool TimedOut)> RunProcessAsync( - string fileName, string arguments, CancellationToken ct, string? workingDirectory = null, TimeSpan? timeout = null) + string fileName, IReadOnlyList arguments, CancellationToken ct, string? workingDirectory = null, TimeSpan? timeout = null) { - var psi = new ProcessStartInfo(fileName, arguments) + var psi = new ProcessStartInfo(fileName) { RedirectStandardInput = true, RedirectStandardOutput = true, @@ -301,7 +383,9 @@ private static string Tail(string s, int lines = 40) CreateNoWindow = true, WorkingDirectory = workingDirectory ?? Environment.CurrentDirectory, }; + foreach (var arg in arguments) psi.ArgumentList.Add(arg); psi.Environment["DOTNET_CLI_UI_LANGUAGE"] = "en"; // we grep "The following Tests are available" + ChildProcess.Prepare(psi); using var p = Process.Start(psi) ?? throw new InvalidOperationException($"Could not start {fileName}"); p.StandardInput.Close(); // Read against `ct` only, never the timeout token: killing the process closes these pipes (EOF), so both tasks diff --git a/tests/DotNetDevMCP.Build.Tests/BuildArgumentTests.cs b/tests/DotNetDevMCP.Build.Tests/BuildArgumentTests.cs new file mode 100644 index 0000000..3f18a7d --- /dev/null +++ b/tests/DotNetDevMCP.Build.Tests/BuildArgumentTests.cs @@ -0,0 +1,32 @@ +using DotNetDevMCP.Build; + +namespace DotNetDevMCP.Build.Tests; + +public class BuildArgumentTests +{ + [Fact] + public void Each_value_is_one_argument_so_a_property_value_cannot_add_switches() + { + var options = new BuildOptions( + Configuration: "Release", + Framework: "net10.0", + Properties: new Dictionary { ["Version"] = "1.0 -p:CustomBeforeMicrosoftCommonTargets=C:/evil.targets" }); + + var args = BuildService.BuildArgumentList("build", Path.Combine(Path.GetTempPath(), "App.csproj"), options); + + Assert.Equal(1, args.Count(a => a.StartsWith("-p:", StringComparison.Ordinal))); + Assert.Contains("-p:Version=1.0 -p:CustomBeforeMicrosoftCommonTargets=C:/evil.targets", args); + Assert.Equal(["--configuration", "Release"], args.SkipWhile(a => a != "--configuration").Take(2)); + Assert.Equal(["--framework", "net10.0"], args.SkipWhile(a => a != "--framework").Take(2)); + } + + [Fact] + public void Property_list_separators_are_escaped() + { + var options = new BuildOptions(Properties: new Dictionary { ["Version"] = "1.0;Extra=1,More=2" }); + + var args = BuildService.BuildArgumentList("build", Path.Combine(Path.GetTempPath(), "App.csproj"), options); + + Assert.Contains("-p:Version=1.0%3BExtra=1%2CMore=2", args); + } +} diff --git a/tests/DotNetDevMCP.Core.Tests/SecurityHardeningTests.cs b/tests/DotNetDevMCP.Core.Tests/SecurityHardeningTests.cs new file mode 100644 index 0000000..829ee3b --- /dev/null +++ b/tests/DotNetDevMCP.Core.Tests/SecurityHardeningTests.cs @@ -0,0 +1,122 @@ +using System.Diagnostics; +using DotNetDevMCP.Core; + +namespace DotNetDevMCP.Core.Tests; + +/// The 0.3.2 hardening: values that name things can't smuggle options into dotnet/git, paths can't escape a root, +/// and --clean-env keeps secrets out of child processes. +public class SecurityHardeningTests +{ + [Theory] + [InlineData("net10.0")] + [InlineData("net8.0-windows")] + [InlineData("netstandard2.0")] + public void Real_frameworks_are_accepted(string framework) => Assert.Null(DotnetArgumentValidation.ValidateFramework(framework)); + + [Theory] + [InlineData("net10.0 --logger:x")] + [InlineData("--logger:x")] + [InlineData("net10.0;-p:X=1")] + public void Frameworks_that_would_add_options_are_rejected(string framework) => Assert.NotNull(DotnetArgumentValidation.ValidateFramework(framework)); + + [Fact] + public void Runtime_configuration_and_property_names_are_validated() + { + Assert.Null(DotnetArgumentValidation.ValidateRuntime("win-x64")); + Assert.NotNull(DotnetArgumentValidation.ValidateRuntime("win-x64 --self-contained")); + Assert.Null(DotnetArgumentValidation.ValidateConfiguration("Release")); + Assert.NotNull(DotnetArgumentValidation.ValidateConfiguration("Release -p:X=1")); + Assert.Null(DotnetArgumentValidation.ValidatePropertyName("Version")); + Assert.NotNull(DotnetArgumentValidation.ValidatePropertyName("X=1 -p:CustomBeforeMicrosoftCommonTargets")); + } + + [Theory] + [InlineData("1.0;CustomBeforeMicrosoftCommonTargets=C:/evil.targets")] + [InlineData("1.0,CustomBeforeMicrosoftCommonTargets=C:/evil.targets")] + public void Property_values_cannot_start_a_second_property(string value) + { + var escaped = DotnetArgumentValidation.EscapePropertyValue(value); + Assert.DoesNotContain(';', escaped); + Assert.DoesNotContain(',', escaped); + } + + [Theory] + [InlineData("main")] + [InlineData("feature/login")] + [InlineData("HEAD~3")] + [InlineData("origin")] + public void Real_git_refs_are_accepted(string value) => Assert.Null(GitRefValidation.Validate(value, "ref")); + + [Theory] + [InlineData("--output=C:/x.txt")] + [InlineData("--upload-pack=touch /tmp/x")] + [InlineData("-c")] + [InlineData("main\nrm")] + [InlineData("")] + public void Git_refs_that_would_be_options_are_rejected(string value) => Assert.NotNull(GitRefValidation.Validate(value, "ref")); + + private static readonly string Root = Path.Combine(Path.GetPathRoot(Path.GetTempPath())!, "repo", "src", "App"); + + [Fact] + public void Paths_inside_the_root_are_within_it() + { + Assert.True(PathBoundary.IsWithin(Path.Combine(Root, "Orders", "OrderService.cs"), Root)); + Assert.True(PathBoundary.IsWithin(Root, Root)); + Assert.True(PathBoundary.IsWithin(Root + Path.DirectorySeparatorChar, Root)); + Assert.True(PathBoundary.IsWithin(Path.Combine(Root, "..foo", "x.cs"), Root)); // a folder named "..foo" is inside + } + + [Fact] + public void Traversal_and_look_alike_siblings_are_outside() + { + Assert.False(PathBoundary.IsWithin(Path.Combine(Root, "..", "..", "secrets.txt"), Root)); + Assert.False(PathBoundary.IsWithin(Path.Combine(Root + "-other", "x.cs"), Root)); + Assert.False(PathBoundary.IsWithin(Path.GetDirectoryName(Root)!, Root)); + Assert.False(PathBoundary.IsWithin("", Root)); + } + + [Fact] + public void Relative_paths_are_resolved_before_comparing() + { + var relativeRoot = Path.Combine("rel", "App"); + Assert.True(PathBoundary.IsWithin(Path.Combine(relativeRoot, "x.cs"), relativeRoot)); + Assert.False(PathBoundary.IsWithin(Path.Combine(relativeRoot, "..", "Other", "x.cs"), relativeRoot)); + } + + [Fact] + public void Clean_env_drops_secrets_and_keeps_what_dotnet_needs() + { + var psi = StartInfoWith(("PATH", "/bin"), ("DOTNET_CLI_UI_LANGUAGE", "en"), ("HTTPS_PROXY", "http://proxy:8080"), + ("NUGET_PACKAGES", "/nuget"), ("AWS_SECRET_ACCESS_KEY", "secret"), ("GITHUB_TOKEN", "ghp_x"), ("OPENAI_API_KEY", "sk-x")); + try + { + ChildProcess.CleanEnvironment = true; + ChildProcess.Prepare(psi); + } + finally { ChildProcess.CleanEnvironment = false; } + + Assert.True(psi.Environment.ContainsKey("PATH")); + Assert.True(psi.Environment.ContainsKey("DOTNET_CLI_UI_LANGUAGE")); + Assert.True(psi.Environment.ContainsKey("HTTPS_PROXY")); + Assert.True(psi.Environment.ContainsKey("NUGET_PACKAGES")); + Assert.False(psi.Environment.ContainsKey("AWS_SECRET_ACCESS_KEY")); + Assert.False(psi.Environment.ContainsKey("GITHUB_TOKEN")); + Assert.False(psi.Environment.ContainsKey("OPENAI_API_KEY")); + } + + [Fact] + public void Clean_env_is_off_by_default() + { + var psi = StartInfoWith(("GITHUB_TOKEN", "ghp_x")); + ChildProcess.Prepare(psi); + Assert.True(psi.Environment.ContainsKey("GITHUB_TOKEN")); + } + + private static ProcessStartInfo StartInfoWith(params (string Name, string Value)[] variables) + { + var psi = new ProcessStartInfo("dotnet"); + psi.Environment.Clear(); + foreach (var (name, value) in variables) psi.Environment[name] = value; + return psi; + } +} diff --git a/tests/DotNetDevMCP.Testing.Tests/RunnerArgumentTests.cs b/tests/DotNetDevMCP.Testing.Tests/RunnerArgumentTests.cs new file mode 100644 index 0000000..b9ec44a --- /dev/null +++ b/tests/DotNetDevMCP.Testing.Tests/RunnerArgumentTests.cs @@ -0,0 +1,37 @@ +using DotNetDevMCP.Testing; + +namespace DotNetDevMCP.Testing.Tests; + +public class RunnerArgumentTests +{ + [Theory] + [InlineData("--filter-class My.Tests.OrderTests")] + [InlineData("--filter-method \"My.Tests.OrderTests.Submit works\"")] + [InlineData("--filter \"FullyQualifiedName~Orders\"")] + [InlineData("--treenode-filter /*/*/OrderTests/*")] + [InlineData("--treenode-filter /MyAssembly/My.Tests/*")] + public void Filter_options_are_allowed_under_the_testing_platform(string filter) => + Assert.Null(TestRunner.ValidateTestingPlatformFilter(filter)); + + [Theory] + [InlineData("--filter-class X -p:CustomBeforeMicrosoftCommonTargets=C:/evil.targets")] + [InlineData("/p:CustomBeforeMicrosoftCommonTargets=C:/evil.targets")] + [InlineData("/p:X")] + [InlineData("--results-directory C:/elsewhere")] + [InlineData("--property:X=1")] + public void Anything_else_is_rejected_under_the_testing_platform(string filter) => + Assert.NotNull(TestRunner.ValidateTestingPlatformFilter(filter)); + + [Fact] + public void VsTest_filter_widens_to_classes_then_to_no_filter() + { + var few = new[] { "Ns.C.A", "Ns.C.B" }; + Assert.Equal("FullyQualifiedName=Ns.C.A|FullyQualifiedName=Ns.C.B", TestRunner.WidenVsTestFilter(few)); + + var sameClass = Enumerable.Range(0, 2000).Select(i => $"Some.Long.Namespace.Tests.RetryTests.Method_{i}").ToList(); + Assert.Equal("FullyQualifiedName~Some.Long.Namespace.Tests.RetryTests.", TestRunner.WidenVsTestFilter(sameClass)); + + var manyClasses = Enumerable.Range(0, 2000).Select(i => $"Some.Long.Namespace.Tests.Class_{i}.Method").ToList(); + Assert.Null(TestRunner.WidenVsTestFilter(manyClasses)); + } +} From 71acb413e95825d82400f97e651d3c8af1048b02 Mon Sep 17 00:00:00 2001 From: csa7mdm Date: Thu, 24 Sep 2026 05:42:07 +0300 Subject: [PATCH 5/5] Release 0.3.2: version, changelog, supported versions Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 28 ++++++++++++++++++++++++ Directory.Build.props | 4 ++-- README.md | 2 +- SECURITY.md | 3 ++- src/DotNetDevMCP.Server/.mcp/server.json | 4 ++-- 5 files changed, 35 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d153f52..cc1c855 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,34 @@ All notable changes to DotNetDevMCP are documented here. The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); versions follow [SemVer](https://semver.org/). +## [0.3.2] - 2026-09-24 + +Security release, prompted by two external reviews; each claim was checked against the code first. Upgrade from 0.3.0/0.3.1. + +### Security +- **Argument injection.** Tool values were concatenated into `dotnet`/`git` command lines, so a crafted value could add options: + an MSBuild property value like `1.0 -p:CustomBeforeMicrosoftCommonTargets=evil.targets` imported a targets file (code + execution during the build), a `gitBase` like `--output=...` made git write a file, and `framework`, branch and remote values + could add flags. Every child process now receives its arguments as a list (one value, one argument); framework, runtime, + configuration, MSBuild property names and git refs are validated; property values escape MSBuild's `;` and `,` list + separators; under Microsoft.Testing.Platform the `filter` may only carry `--filter*` / `--treenode-filter` options. +- **Path check.** Roslyn edit tools checked "inside the solution" with a plain string prefix, so `..` segments and look-alike + sibling folders (`App-other` for `App`) passed. Paths are now resolved before comparing. + +### Added +- `--clean-env`: child processes get a minimal allow-listed environment (PATH, temp and profile folders, proxies, `DOTNET_*`, + `NUGET_*`, `MSBUILD*`...) instead of inheriting the server's, so tokens and cloud credentials in environment variables aren't + passed on. Not a sandbox. Startup logs how many variables were dropped; `--log-level Debug` lists their names. +- `dotnet_test_affected` project-level fallback: when the selection runs out of budget or is too large, it runs only the test + projects that reference the changed projects (`ranScope: "projects"`, `testProjectsRun`), not the whole solution. Helper + libraries that reference a test framework but declare no tests are skipped. +- Security model in SECURITY.md, the README and the [wiki](https://github.com/csa7mdm/DotNetDevMCP/wiki/Security). + +### Fixed +- The VSTest name filter had no length limit (Windows caps command lines at 32K); it now widens to classes, then to no + filter, like the Microsoft.Testing.Platform path. +- `WorkflowEngine` no longer wraps already-async steps in `Task.Run`. + ## [0.3.1] - 2026-09-24 Documentation and community release; no behavior changes. diff --git a/Directory.Build.props b/Directory.Build.props index 4bb451a..7645871 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -9,10 +9,10 @@ $(NoWarn);CS1591 - 0.3.1 + 0.3.2 false 0.2.0.0 - 0.3.1.0 + 0.3.2.0 Ahmed Mustafa diff --git a/README.md b/README.md index 4e3b7d0..1610b96 100644 --- a/README.md +++ b/README.md @@ -192,7 +192,7 @@ Built on the official [MCP C# SDK](https://github.com/modelcontextprotocol/cshar ## Status -0.3.1. The Roslyn tools are mature (they come from SharpTools). Testing, build, git and orchestration are newer and have been exercised on this repository and a few others; expect rough edges on unusual project layouts. Issues and PRs welcome, see [CONTRIBUTING](https://github.com/csa7mdm/DotNetDevMCP/blob/main/CONTRIBUTING.md). +0.3.2. The Roslyn tools are mature (they come from SharpTools). Testing, build, git and orchestration are newer and have been exercised on this repository and a few others; expect rough edges on unusual project layouts. Issues and PRs welcome, see [CONTRIBUTING](https://github.com/csa7mdm/DotNetDevMCP/blob/main/CONTRIBUTING.md). Known gaps: `dotnet_test_affected` follows C# references only (no reflection, no DI-by-convention, no string-keyed lookups), so a change reached only through those paths will not select the test; use `dryRun` to check what it picks. Tests that hang instead of failing are only caught by a run that finishes. Test attribute detection covers xUnit, NUnit and MSTest by attribute name. Past the command-line length limit the filter widens from methods to classes, then to the whole project (more tests, never fewer). diff --git a/SECURITY.md b/SECURITY.md index d3e58f2..2c69deb 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -6,7 +6,8 @@ Currently, only the latest version of DotNetDevMCP is supported with security up | Version | Supported | | ------- | ------------------ | -| 0.3.x | :white_check_mark: | +| 0.3.2+ | :white_check_mark: | +| 0.3.0-0.3.1 | :x: (argument injection, weak path check: upgrade) | | < 0.3 | :x: | ## Reporting a Vulnerability diff --git a/src/DotNetDevMCP.Server/.mcp/server.json b/src/DotNetDevMCP.Server/.mcp/server.json index 48f1d84..52207e7 100644 --- a/src/DotNetDevMCP.Server/.mcp/server.json +++ b/src/DotNetDevMCP.Server/.mcp/server.json @@ -2,7 +2,7 @@ "$schema": "https://static.modelcontextprotocol.io/schemas/2025-09-29/server.schema.json", "name": "io.github.csa7mdm/dotnetdevmcp", "description": "MCP server for .NET: Roslyn code navigation and refactoring, build, and affected-test selection.", - "version": "0.3.1", + "version": "0.3.2", "websiteUrl": "https://github.com/csa7mdm/DotNetDevMCP/wiki", "repository": { "url": "https://github.com/csa7mdm/DotNetDevMCP", @@ -13,7 +13,7 @@ "registryType": "nuget", "registryBaseUrl": "https://api.nuget.org/v3/index.json", "identifier": "DotNetDevMCP", - "version": "0.3.1", + "version": "0.3.2", "transport": { "type": "stdio" }, "packageArguments": [ {