From e06d30c16d19800d8b9f347a5bb0971f3aa1d82c Mon Sep 17 00:00:00 2001 From: Chris Knight Date: Fri, 7 Aug 2026 16:23:49 -0400 Subject: [PATCH] Harden MCP tools for minimal-rights operation: directory allow-listing, dry-run, order-file audit merge_conflicts now validates relativePaths and orderOverrides keys against Paths.ModsDirectory (proper Path.GetFullPath-based prefix check, not naive StartsWith) before any scan or merge runs, rejecting absolute/UNC/`..`-escaping entries with a clear error. Neither value was actually joined into a filesystem path anywhere in this codebase - this closes no live traversal, it's defense-in-depth against that changing later, and it fixes the "silently matches nothing" gap for a malicious-looking relativePaths entry. Audited orderOverrides for other misuse: values were already whitelisted via ModFile.ContainsMod before reaching Path.Combine, but the validation had two real gaps - a partial/duplicate mod list would silently merge an incomplete or self-paired chain and still report success, and a single-remaining-real-source override (reachable once a file's already-merged output re-enters conflict.Mods as a pseudo-source) would report "merged" having done nothing. FileMerger.ResolveMergeOrder now requires at least two entries, no duplicates, and every real source mod covered (excluding the configured merged-mod name itself, matching scan_conflicts's own documented guidance for re-merging). This validation is shared with the `merge` CLI verb's --order-file, so a pre-existing partial order-file that used to be silently accepted now gets rejected per-file instead. Added merge_conflicts's dryRun mode: previews which conflicts would auto-solve without writing merged output, repacking a bundle, or modifying MergeInventory.xml. Distinct from scan_conflicts's alreadyResolved (which only re-checks existing merge records) since dryRun actually exercises the merge engine for currently-unresolved conflicts. Output is redirected under TempBundleContent instead of the real destination; MergeInventory.Load gained an allowSave flag since it can otherwise write to disk on its own (backfilling an old record's missing hash) before dryRun is ever consulted; a dry run now also predicts the same "output already exists, declined" outcome a real run would hit, so the two don't disagree on a conflict whose output is already on disk. Documented minimal required permissions in a new Mcp/CLAUDE.md and updated the root CLAUDE.md's MCP mode section. AI-assisted: implemented by Claude Code per repo convention (CONTRIBUTING.md). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01GXAuGMLB44T5Zv5o5ZzKah --- CLAUDE.md | 7 +- .../Cli/MergeOperations.cs | 5 +- .../Inventory/FileMerger.cs | 135 +++++++++++++--- .../Inventory/MergeInventory.cs | 14 +- WitcherScriptMerger.Core/Mcp/CLAUDE.md | 32 ++++ WitcherScriptMerger.Core/Mcp/WsmMcpTools.cs | 147 ++++++++++++++++-- 6 files changed, 300 insertions(+), 40 deletions(-) create mode 100644 WitcherScriptMerger.Core/Mcp/CLAUDE.md diff --git a/CLAUDE.md b/CLAUDE.md index 8b59c35..7f5c2f4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -76,7 +76,7 @@ The CLI path (`FileMerger.MergeConflictsHeadless`) is a separate, parallel orche `WitcherScriptMerger.exe merge [--order-file ]` merges every auto-solvable conflict without opening any window, then exits. No-args still launches the GUI unchanged; passing `merge` is what selects the CLI path in `Program.cs`. -- **Orchestration**: `Cli/MergeOperations.ScanConflicts()` runs `ModFileIndex.BuildAsync` synchronously (via a `ManualResetEventSlim`) and returns the built index; `MergeOperations.RunMerge(inventory, conflicts, mergedModName, orderOverrides)` calls `FileMerger.MergeConflictsHeadless`. Both the `merge` CLI verb and the MCP tools (see "MCP mode" below) call these instead of duplicating the scan/wait/merge sequence. `MergeConflictsHeadless` iterates `ModFileIndex.Conflicts` directly — those are plain `ModFile`/`FileHash` objects (relative path, category, per-mod name and hash), so no `TreeNode`/`ConflictTree` is ever constructed for this path. `MergeFlatConflictHeadless` and `MergeBundleConflictHeadless` mirror `MergeFlatFileNode`/`MergeBundleFileNode`'s logic against that plain data instead of `TreeNode.GetMetadata()`; bundle merges reuse `GetUnpackedFiles`/`PackNewBundle` unchanged. Per-file mod order defaults to `LoadOrderComparer` (matching `ConflictTree`'s own default sort, `Controls/SMTreeSorter.cs`); the `--order-file` JSON (`{"relative\\path.ws": ["modA", "modB"]}`) overrides specific files without requiring every conflict to be listed. +- **Orchestration**: `Cli/MergeOperations.ScanConflicts()` runs `ModFileIndex.BuildAsync` synchronously (via a `ManualResetEventSlim`) and returns the built index; `MergeOperations.RunMerge(inventory, conflicts, mergedModName, orderOverrides)` calls `FileMerger.MergeConflictsHeadless`. Both the `merge` CLI verb and the MCP tools (see "MCP mode" below) call these instead of duplicating the scan/wait/merge sequence. `MergeConflictsHeadless` iterates `ModFileIndex.Conflicts` directly — those are plain `ModFile`/`FileHash` objects (relative path, category, per-mod name and hash), so no `TreeNode`/`ConflictTree` is ever constructed for this path. `MergeFlatConflictHeadless` and `MergeBundleConflictHeadless` mirror `MergeFlatFileNode`/`MergeBundleFileNode`'s logic against that plain data instead of `TreeNode.GetMetadata()`; bundle merges reuse `GetUnpackedFiles`/`PackNewBundle` unchanged. Per-file mod order defaults to `LoadOrderComparer` (matching `ConflictTree`'s own default sort, `Controls/SMTreeSorter.cs`); the `--order-file` JSON (`{"relative\\path.ws": ["modA", "modB"]}`) overrides specific files without requiring every conflict to be listed. Within a listed file's mod list, `FileMerger.ResolveMergeOrder` requires: no unknown mod names, no duplicate names, at least two entries, and every one of that file's *real* source mods present at least once — any violation rejects that one file with a clear error (via `AppState.Notifier.ShowError`) rather than silently merging an incomplete, self-paired, or single-entry (no-op) chain. This applies to both the CLI's `--order-file` and the MCP `merge_conflicts` tool's `orderOverrides`, since both funnel through this shared method. "Real source mods" deliberately excludes the configured merged-mod name itself: once a file has already been merged once, its own merged-mod folder re-enters `conflict.Mods` as if it were a source (see `scan_conflicts`'s tool description below), so re-merging after a source mod's file changes only needs to list the actual mods again, not the previous merge output too — the at-least-two-entries rule still applies, though, so listing only that one remaining real mod (with the merged-mod folder excluded from the requirement) is rejected rather than silently merged with nothing to merge against. - **`IMergeNotifier`** (Core: `IMergeNotifier.cs`, `NotifyTypes.cs`, `HeadlessMergeNotifier.cs`; host: `MainForm.cs`'s implementation): replaces every direct `Program.MainForm.ShowMessage/ShowError/ShowModal` call in domain code with `AppState.Notifier.*` (host code still spells this `Program.Notifier.*`, via the pass-through property). The interface is defined against neutral `NotifyResult`/`NotifyButtons`/`DialogIcon` types, not `DialogResult`/`MessageBoxButtons`/`MessageBoxIcon` — Core can't reference `System.Windows.Forms` at all. `MainForm` translates those neutral types to/from real `MessageBox.Show(...)`/`DialogResult` calls; this is **not** a behavior-identical passthrough — one real prompt (`LoadOrderValidator`'s "Custom Load Order Problem" dialog) lost a `MessageBoxManager`-based custom button caption that used to mark its Cancel option as destructive/permanent (that caption mechanism was likely already silently broken pre-split — `MessageBoxManager.Register()`'s `AppDomain.GetCurrentThreadId()`-based hook doesn't reliably work on modern .NET — but the loss is real either way; the warning is now spelled out in the message body instead). `ShowModal(Form)` isn't part of `IMergeNotifier` at all — every call site is GUI-only, interactive code in the host project, which calls `MainForm.ShowModal` directly instead of going through the notifier abstraction. `HeadlessMergeNotifier` writes to the console and returns a fixed, non-destructive default for every decision: don't overwrite an existing merge output, don't use a merge name that's still conflicting, don't continue past a canceled/failed merge — except where a caller explicitly overrides that generic default via `ShowMessage`'s `defaultResult` parameter (added for `LoadOrderValidator`, whose YesNoCancel prompt has an inverted-from-usual safety shape: Cancel, not Yes/No, is the one destructive/permanent choice there). This is also what fixed a real null-ref hazard in `CustomLoadOrder.Refresh()`, which used to reach `Program.MainForm` at construction time. - **`KDiff3.RunHeadless`**: see the "Verify KDiff3 process behavior..." and "KDiff3's pop-up window can't be suppressed" compatibility constraints above for the window-persistence detection this relies on and why the window itself is left alone rather than hidden. `-o` always targets a scratch path under `Paths.TempBundleContent\HeadlessOutput\`, copied to the real output only after a confirmed clean exit — a killed process can never leave a partial file at the real path. - **Verification status**: the flat-file path (`Categories.Script`/`Categories.Xml`) is verified end-to-end against real conflicting files (including the encoding-mismatch scenario) and a synthetic guaranteed-conflict, in a scratch game/mods tree — never against a live install. The bundle path (`Categories.BundleText`) is code-reviewed and mirrors the proven flat-file orchestration, and its two building blocks (`GetUnpackedFiles`, `PackNewBundle`) are unchanged, already-exercised code — but it hasn't been round-tripped through a real bundle-vs-bundle conflict. If `tempbundlecontent` accumulates across many CLI runs during development, clear it between runs — one debugging session saw a single very slow run after a long buildup that didn't reproduce once it was cleared. @@ -86,9 +86,10 @@ The CLI path (`FileMerger.MergeConflictsHeadless`) is a separate, parallel orche `WitcherScriptMerger.exe mcp` runs an MCP (Model Context Protocol) server over stdio, using the official `ModelContextProtocol` NuGet package (`Host.CreateApplicationBuilder().Services.AddMcpServer().WithStdioServerTransport().WithToolsFromAssembly()`, wired up in `Program.cs`'s `RunMcp`). It's a third entry point alongside the GUI and the `merge` CLI verb — same `args.Length > 0` routing in `Main`, dispatched on `args[0] == "mcp"`. Lets an MCP client (e.g. a Claude Code session) inspect conflicts and drive merges directly instead of only through one-shot CLI invocations. - **Why stdio, and why logging goes to stderr**: an MCP client spawns WSM as a child process and communicates over its redirected stdin/stdout pipes, so stdout must stay reserved for protocol frames — `RunMcp` configures `builder.Logging.AddConsole(o => o.LogToStandardErrorThreshold = LogLevel.Trace)` to keep the SDK's own request-handler logging off stdout. `Program.MaybeAttachConsole()` also skips `AttachConsole` for the `mcp` verb specifically (checks `args[1]` before calling it) — there's no parent console to attach to in this scenario, and it's pointless at best. Verified: a hand-rolled stdio client (`initialize` → `tools/list` → `tools/call` for each tool) round-tripped clean JSON-RPC on stdout with all SDK logging correctly landing on stderr. -- **Tools** (`Mcp/WsmMcpTools.cs`, `[McpServerToolType]` static class): `scan_conflicts` (no args — scans and returns every conflict's relative path, category, per-mod hashes, default merge order, and whether it's already resolved), `merge_conflicts` (optional `relativePaths` to restrict to specific files, optional `orderOverrides` — same shape as the CLI's `--order-file`, minus the file; returns `{merged: [...], skipped: [...]}`), `get_status` (dependency validation, resolved game/mods directories, configured merged-mod name, current conflict count), `list_merges` (enumerates `MergeInventory.xml`'s existing `Merge` records). All four reuse `Cli/MergeOperations` and the same `IMergeNotifier`/`HeadlessMergeNotifier` machinery as the `merge` CLI verb — merge decisions get the same safe non-destructive defaults either way. +- **Tools** (`Mcp/WsmMcpTools.cs`, `[McpServerToolType]` static class): `scan_conflicts` (no args — scans and returns every conflict's relative path, category, per-mod hashes, default merge order, and whether it's already resolved), `merge_conflicts` (optional `relativePaths` to restrict to specific files, optional `orderOverrides` — same shape as the CLI's `--order-file`, minus the file, and validated per-file the same way (see "CLI mode" above — no duplicates, every real source mod covered), optional `dryRun` to preview which conflicts would auto-solve without writing merged output, repacking a bundle, or touching `MergeInventory.xml`; returns `{merged: [...], skipped: [...], unmatched: [...], dryRun}` — `unmatched` lists any requested `relativePaths` entry that's in-scope but no longer a detected conflict), `get_status` (dependency validation, resolved game/mods directories, configured merged-mod name, current conflict count), `list_merges` (enumerates `MergeInventory.xml`'s existing `Merge` records). All four reuse `Cli/MergeOperations` and the same `IMergeNotifier`/`HeadlessMergeNotifier` machinery as the `merge` CLI verb — merge decisions get the same safe non-destructive defaults either way. +- **Directory allow-listing**: `merge_conflicts`'s `relativePaths` and `orderOverrides` keys are validated (`WsmMcpTools.EnsureInScope`/`IsWithinModsDirectory`) to resolve inside `Paths.ModsDirectory` before any scan or merge runs — an absolute path, UNC path, or `..\`-escaping entry is rejected with a clear error instead of silently matching nothing. Neither value is actually joined into a filesystem path anywhere in this codebase today (`relativePaths` is only ever compared for equality against already-scanned `ModFile.RelativePath` values; `orderOverrides` values reach `Path.Combine` in `FileMerger.GetModFile` but only after being validated against `ModFile.ContainsMod`, a whitelist of real scanned mod folder names) — this check is defense-in-depth, not a fix for a live traversal. This check applies mods-directory-relative semantics uniformly to every category; for `Categories.BundleText` specifically, `conflict.RelativePath` is actually a path *internal to a bundle archive* (from `QuickBms.GetBundleContentPaths`), not one rooted at `Paths.ModsDirectory` — an ordinary internal path (e.g. `engine\foo.ws`) still validates fine, but a bundle whose internal listing itself contained a rooted or `..`-bearing entry would make that specific conflict unreachable via `relativePaths` (rejected as out-of-scope even though it's a legitimate conflict). Unexercised: every scratch config used to verify this unit has `CheckBundleContents=false`, consistent with the bundle path's existing "code-reviewed but not round-tripped" verification status below. See `Mcp/CLAUDE.md` for the minimal-permissions summary this unit added. - **State per call, not cached across calls**: every tool call re-scans (`ModFileIndex.BuildAsync`) and re-loads `MergeInventory.Load(Paths.Inventory)` fresh rather than keeping a long-lived server-side cache, since the mods folder or `MergeInventory.xml` can change between calls (including from a concurrently-running GUI instance) and there's no test suite to catch a staleness bug. -- **Verification status**: smoke-tested end-to-end against the same scratch game/mods tree used for CLI mode verification — `initialize`, `tools/list`, and all four tools called via a hand-rolled stdio client, including a `merge_conflicts` call that exercised the real `KDiff3.RunHeadless` path (detected a genuine conflict, killed the stuck process, returned it in `skipped`). Never run against a live install. +- **Verification status**: smoke-tested end-to-end against the same scratch game/mods tree used for CLI mode verification — `initialize`, `tools/list`, and all four tools called via a hand-rolled stdio client, including a `merge_conflicts` call that exercised the real `KDiff3.RunHeadless` path (detected a genuine conflict, killed the stuck process, returned it in `skipped`). Never run against a live install. The directory-allow-listing and `dryRun` additions were verified against a real KDiff3 stand-in (dependency paths present but not a real KDiff3.exe, so merges reliably fail/skip) via the same stdio-client approach, plus an in-process harness against `WitcherScriptMerger.Core` directly with a fake `IMergeEngine` (real KDiff3/QuickBMS/wcc_lite weren't available in that verification environment) — see the PR that introduced them for exactly what each covered. ### Compatibility constraints diff --git a/WitcherScriptMerger.Core/Cli/MergeOperations.cs b/WitcherScriptMerger.Core/Cli/MergeOperations.cs index da76c96..88d52bd 100644 --- a/WitcherScriptMerger.Core/Cli/MergeOperations.cs +++ b/WitcherScriptMerger.Core/Cli/MergeOperations.cs @@ -29,13 +29,14 @@ public static FileMerger.HeadlessMergeSummary RunMerge( MergeInventory inventory, IEnumerable conflicts, string mergedModName, - IReadOnlyDictionary orderOverrides) + IReadOnlyDictionary orderOverrides, + bool dryRun = false) { // AppState.MergeEngine is supplied once by the host project at startup // (Program.cs) - see Tools/IMergeEngine.cs for why Core can't construct // its one real implementation (KDiff3MergeEngine) itself. var merger = new FileMerger(inventory, AppState.MergeEngine); - return merger.MergeConflictsHeadless(conflicts, mergedModName, orderOverrides); + return merger.MergeConflictsHeadless(conflicts, mergedModName, orderOverrides, dryRun); } } } diff --git a/WitcherScriptMerger.Core/Inventory/FileMerger.cs b/WitcherScriptMerger.Core/Inventory/FileMerger.cs index 5fa3633..d5e8adb 100644 --- a/WitcherScriptMerger.Core/Inventory/FileMerger.cs +++ b/WitcherScriptMerger.Core/Inventory/FileMerger.cs @@ -354,22 +354,32 @@ bool ConfirmContinueAfterCanceledMerge(int remainingMergesForFile, Merge merge) public HeadlessMergeSummary MergeConflictsHeadless( IEnumerable conflicts, string mergedModName, - IReadOnlyDictionary orderOverrides) + IReadOnlyDictionary orderOverrides, + bool dryRun = false) { var summary = new HeadlessMergeSummary(); foreach (var conflict in conflicts.Where(c => c.Category == Categories.Script || c.Category == Categories.Xml || c.Category == Categories.BundleText)) { - var orderedNames = ResolveMergeOrder(conflict, orderOverrides); + var orderedNames = ResolveMergeOrder(conflict, mergedModName, orderOverrides); if (orderedNames == null) { summary.Skipped.Add(conflict.RelativePath); continue; } + // A dry run always merges into a throwaway record instead of one pulled + // from _inventory.Merges - that way nothing this pass does to `merge` + // (BundleName, recorded source hashes via RecordMergedSources) can ever + // mutate a live object still referenced by the loaded inventory, even if + // some other code path calls Save() later. isNew is irrelevant for a dry + // run since the block below that would add it to the inventory is itself + // skipped for dry runs. var isNew = false; - var merge = _inventory.Merges.FirstOrDefault(m => m.RelativePath.EqualsIgnoreCase(conflict.RelativePath)); + Merge merge = null; + if (!dryRun) + merge = _inventory.Merges.FirstOrDefault(m => m.RelativePath.EqualsIgnoreCase(conflict.RelativePath)); if (merge == null) { isNew = true; @@ -382,8 +392,8 @@ public HeadlessMergeSummary MergeConflictsHeadless( var isBundle = conflict.Category == Categories.BundleText; var fullyMerged = isBundle - ? MergeBundleConflictHeadless(conflict, merge, orderedNames) - : MergeFlatConflictHeadless(conflict, merge, mergedModName, orderedNames); + ? MergeBundleConflictHeadless(conflict, merge, orderedNames, dryRun) + : MergeFlatConflictHeadless(conflict, merge, mergedModName, orderedNames, dryRun); if (!fullyMerged) { @@ -391,7 +401,10 @@ public HeadlessMergeSummary MergeConflictsHeadless( continue; } - if (isNew && merge.Mods.Count > 1) + // Dry run never adds to the inventory or flags the bundle as needing a + // repack - it only reports what *would* happen, so PackNewBundle (which + // overwrites the real blob0.bundle) never runs for one either. + if (!dryRun && isNew && merge.Mods.Count > 1) { if (isBundle) { @@ -430,7 +443,7 @@ public HeadlessMergeSummary MergeConflictsHeadless( return summary; } - bool MergeFlatConflictHeadless(ModFile conflict, Merge merge, string mergedModName, string[] orderedNames) + bool MergeFlatConflictHeadless(ModFile conflict, Merge merge, string mergedModName, string[] orderedNames, bool dryRun) { var firstHash = conflict.Mods.First(h => h.Name.EqualsIgnoreCase(orderedNames[0])); var source1 = MergeSource.FromFlatFile(new FileInfo(conflict.GetModFile(orderedNames[0])), firstHash); @@ -438,12 +451,27 @@ bool MergeFlatConflictHeadless(ModFile conflict, Merge merge, string mergedModNa var relPath = Paths.GetRelativePath( source1.TextFile.FullName, Path.Combine(Paths.ModsDirectory, source1.Name)); - - _outputPath = Path.Combine(Paths.ModsDirectory, mergedModName, relPath); - - if (File.Exists(_outputPath) && !ConfirmOutputOverwrite(_outputPath)) + var realOutputPath = Path.Combine(Paths.ModsDirectory, mergedModName, relPath); + + // Checked against the real would-be output path regardless of dryRun: a real + // run always declines to overwrite an existing output (HeadlessMergeNotifier's + // fixed default), so a dry run needs to predict that same "already exists, + // would be skipped" outcome rather than only ever reporting whether the text + // itself would auto-solve - otherwise a preview and the real run it's meant to + // predict could disagree on a conflict whose output already exists. + if (File.Exists(realOutputPath) && !ConfirmOutputOverwrite(realOutputPath)) return false; + // KDiff3 always physically writes its -o target on a successful solve - there's + // no "check without writing" mode - so a dry run still needs somewhere real to + // land. Routing it under TempBundleContent instead of the real mod output path + // means CleanUpTempFiles() at the end of MergeConflictsHeadless deletes it + // afterward, so nothing from a dry run is meant to survive past this call (best + // -effort, like the rest of this method's cleanup - see CleanUpTempFiles). + _outputPath = dryRun + ? Path.Combine(Paths.TempBundleContent, "DryRun", conflict.RelativePath) + : realOutputPath; + _vanillaFile = new FileInfo(conflict.GetVanillaFile()); for (int i = 1; i < orderedNames.Length; ++i) @@ -451,7 +479,7 @@ bool MergeFlatConflictHeadless(ModFile conflict, Merge merge, string mergedModNa var hash = conflict.Mods.First(h => h.Name.EqualsIgnoreCase(orderedNames[i])); var source2 = MergeSource.FromFlatFile(new FileInfo(conflict.GetModFile(orderedNames[i])), hash); - var mergedFile = MergeTextHeadless(merge, source1, source2); + var mergedFile = MergeTextHeadless(merge, source1, source2, dryRun); if (mergedFile == null) return false; source1 = MergeSource.FromFlatFile(mergedFile, null); @@ -459,15 +487,25 @@ bool MergeFlatConflictHeadless(ModFile conflict, Merge merge, string mergedModNa return true; } - bool MergeBundleConflictHeadless(ModFile conflict, Merge merge, string[] orderedNames) + bool MergeBundleConflictHeadless(ModFile conflict, Merge merge, string[] orderedNames, bool dryRun) { merge.BundleName = Path.GetFileName(Paths.RetrieveMergedBundlePath()); - _outputPath = Path.Combine(Paths.MergedBundleContent, conflict.RelativePath); + var realOutputPath = Path.Combine(Paths.MergedBundleContent, conflict.RelativePath); - if (File.Exists(_outputPath) && !ConfirmOutputOverwrite(_outputPath)) + // See the matching comment in MergeFlatConflictHeadless - same reasoning: check + // against the real would-be output regardless of dryRun, so a preview predicts + // the same "already exists, declined" outcome a real run would hit. + if (File.Exists(realOutputPath) && !ConfirmOutputOverwrite(realOutputPath)) return false; + // Rooted under TempBundleContent instead of MergedBundleContent for a dry run so + // its intermediate merge text can never linger there either - see the matching + // comment in MergeFlatConflictHeadless. + _outputPath = dryRun + ? Path.Combine(Paths.TempBundleContent, "DryRun", conflict.RelativePath) + : realOutputPath; + _vanillaFile = null; var firstHash = conflict.Mods.First(h => h.Name.EqualsIgnoreCase(orderedNames[0])); @@ -481,7 +519,7 @@ bool MergeBundleConflictHeadless(ModFile conflict, Merge merge, string[] ordered if (!GetUnpackedFiles(conflict.RelativePath, ref source1, ref source2)) return false; - var mergedFile = MergeTextHeadless(merge, source1, source2); + var mergedFile = MergeTextHeadless(merge, source1, source2, dryRun); if (mergedFile == null) return false; source1 = MergeSource.FromFlatFile(mergedFile, null); @@ -493,17 +531,69 @@ bool MergeBundleConflictHeadless(ModFile conflict, Merge merge, string[] ordered // LoadOrderComparer ordering ConflictTree's default sort already uses // (Controls/SMTreeSorter.cs), so headless behavior matches the GUI's default // without needing every conflict spelled out. - string[] ResolveMergeOrder(ModFile conflict, IReadOnlyDictionary orderOverrides) + string[] ResolveMergeOrder(ModFile conflict, string mergedModName, IReadOnlyDictionary orderOverrides) { if (orderOverrides != null && orderOverrides.TryGetValue(conflict.RelativePath, out var explicitOrder)) { + if (explicitOrder == null) + { + AppState.Notifier.ShowError($"Order file's mod list for {conflict.RelativePath} is null."); + return null; + } + var unknown = explicitOrder.Where(name => !conflict.ContainsMod(name)).ToArray(); if (unknown.Any()) { AppState.Notifier.ShowError( - $"Order file lists unknown mod(s) for {conflict.RelativePath}: {string.Join(", ", unknown)}"); + $"Order file lists unknown mod(s) for {conflict.RelativePath}: " + + string.Join(", ", unknown.Select(n => n ?? "(null)"))); return null; } + + var distinctCount = explicitOrder.Distinct(StringComparer.OrdinalIgnoreCase).Count(); + if (distinctCount != explicitOrder.Length) + { + AppState.Notifier.ShowError( + $"Order file's mod list for {conflict.RelativePath} names the same mod more than once."); + return null; + } + + // A merge needs at least a pair to actually do anything - without this, + // a conflict left with only one *real* source after excluding + // mergedModName (e.g. every other source mod's file was since removed, + // leaving only the previous merge output and one real mod) could pass an + // override naming just that one real mod: nothing "missing", no + // duplicate, but MergeFlatConflictHeadless's chain loop (starting at + // index 1) would never run, silently reporting the file as fully merged + // having merged nothing and written nothing. + if (explicitOrder.Length < 2) + { + AppState.Notifier.ShowError( + $"Order file's mod list for {conflict.RelativePath} must name at least two mods to merge."); + return null; + } + + // Every *real* source mod must be covered - omitting one would otherwise + // merge an incomplete chain and still report the file as fully merged. + // mergedModName itself doesn't count as a required source: once a file has + // already been merged once, its own merged-mod folder re-enters + // conflict.Mods as if it were a source (scan_conflicts's own description + // warns clients about this), and the documented way to re-merge after a + // source mod's file changes is to list just the real mods again - not to + // also re-list the previous merge output. + var requiredNames = conflict.Mods + .Select(m => m.Name) + .Where(name => !name.EqualsIgnoreCase(mergedModName)) + .ToArray(); + var missing = requiredNames.Where(name => !explicitOrder.Any(n => n.EqualsIgnoreCase(name))).ToArray(); + if (missing.Any()) + { + AppState.Notifier.ShowError( + $"Order file's mod list for {conflict.RelativePath} is missing conflicting mod(s): " + + string.Join(", ", missing)); + return null; + } + return explicitOrder; } @@ -513,7 +603,7 @@ string[] ResolveMergeOrder(ModFile conflict, IReadOnlyDictionary orderOverrides = null) + [Description("Relative paths to merge; omit to merge every detected conflict. Each must " + + "resolve inside the configured mods directory - absolute paths, UNC paths, and " + + "\"..\\\" segments that would escape it are rejected.")] string[] relativePaths = null, + [Description("Map of relative path to an explicit, ordered list of mod names for that " + + "file - at least two, no duplicates, and every one of that file's real source " + + "mods must appear (the configured merged-mod name itself doesn't need to be " + + "listed, even if it shows up as a source because the file was already merged " + + "once).")] Dictionary orderOverrides = null, + [Description("If true, evaluates which conflicts would auto-solve without writing any " + + "merged output, repacking any bundle, or modifying MergeInventory.xml.")] bool dryRun = false) { + // Validated before touching the mods folder or dependency state: this is pure + // input validation the caller controls, so it should fail fast and + // independently of whatever else is or isn't configured. + EnsureInScope(relativePaths, orderOverrides); + RequireDependenciesAndModsDirectory(); var mergedModName = Paths.RetrieveMergedModName(); if (string.IsNullOrWhiteSpace(mergedModName)) throw new InvalidOperationException("MergedModName isn't configured in App.config."); + // ModFile.RelativePath always uses '\' (built via Path.Combine/GetRelativePath + // on Windows). A client-supplied relativePaths entry using '/' already passes + // IsWithinModsDirectory's scope check (Path.GetFullPath normalizes separators), + // but a raw EqualsIgnoreCase against RelativePath below would not - normalize + // here so an in-scope path in a different, still-valid separator style doesn't + // silently fail to match its own conflict and land in `unmatched` looking like + // it was never a conflict at all. + var normalizedRelativePaths = relativePaths?.Select(p => p.Replace('/', '\\')).ToArray(); + lock (_inventoryLock) { - AppState.Inventory = MergeInventory.Load(Paths.Inventory); + // allowSave: !dryRun - MergeInventory.Load() can itself write to + // MergeInventory.xml (AddMissingHashes backfilling an old record's null + // hash) before dryRun is ever otherwise consulted; without this, a dry run + // against a legacy inventory file could still touch disk despite the + // tool's own "without... modifying MergeInventory.xml" description. + AppState.Inventory = MergeInventory.Load(Paths.Inventory, allowSave: !dryRun); var modIndex = MergeOperations.ScanConflicts(); - var conflicts = relativePaths == null + var conflicts = (normalizedRelativePaths == null ? modIndex.Conflicts - : modIndex.Conflicts.Where(c => relativePaths.Any(p => p.EqualsIgnoreCase(c.RelativePath))); - - var summary = MergeOperations.RunMerge(AppState.Inventory, conflicts, mergedModName, orderOverrides); - AppState.Inventory.Save(); - - return new { merged = summary.Merged, skipped = summary.Skipped }; + : modIndex.Conflicts.Where(c => normalizedRelativePaths.Any(p => p.EqualsIgnoreCase(c.RelativePath)))) + .ToArray(); + + // In-scope but no longer a detected conflict (e.g. resolved or removed + // between scan and this call) is reported back, not silently dropped - + // only an out-of-scope path (rejected above) is a hard error. Derived from + // `conflicts` (already the relativePaths-matching subset) rather than + // re-scanning the full modIndex.Conflicts a second time. + var unmatched = normalizedRelativePaths == null + ? Array.Empty() + : normalizedRelativePaths.Where(p => !conflicts.Any(c => c.RelativePath.EqualsIgnoreCase(p))).ToArray(); + + // orderOverrides keys are matched against conflict.RelativePath elsewhere + // (FileMerger.ResolveMergeOrder) via a plain Dictionary lookup, which - built + // from JSON with no comparer specified - is ordinal case-sensitive by + // default and wouldn't tolerate a '/'-separated key either. Rebuilding it + // here (case-insensitive comparer, '\' separators) keeps that lookup + // consistent with every other path/name comparison in this codebase, so a + // differently-cased or differently-separated but otherwise-correct key + // isn't silently ignored. + var normalizedOrderOverrides = orderOverrides == null + ? null + : orderOverrides.ToDictionary( + kv => kv.Key.Replace('/', '\\'), + kv => kv.Value, + StringComparer.OrdinalIgnoreCase); + + var summary = MergeOperations.RunMerge(AppState.Inventory, conflicts, mergedModName, normalizedOrderOverrides, dryRun); + + // FileMerger guarantees a dry run never adds or updates records in the + // loaded inventory (see MergeConflictsHeadless), but skipping the disk + // write too is a second, independent guarantee that a preview call can + // never leave MergeInventory.xml touched, even if that were ever weakened. + if (!dryRun) + AppState.Inventory.Save(); + + return new { merged = summary.Merged, skipped = summary.Skipped, unmatched, dryRun }; } } @@ -126,5 +186,70 @@ static void RequireDependenciesAndModsDirectory() if (!Directory.Exists(Paths.ModsDirectory)) throw new InvalidOperationException("Mods directory not found - check GameDirectory/ModsDirectory in App.config."); } + + // Directory allow-listing for merge_conflicts's two path-shaped inputs. Neither is + // currently joined into a filesystem path by itself - relativePaths is only ever + // compared for equality against already-scanned ModFile.RelativePath values + // (WsmMcpTools.MergeConflicts, above), and orderOverrides keys are only ever + // looked up the same way (FileMerger.ResolveMergeOrder) - so there's no live + // traversal vector through either today. This exists as defense-in-depth against + // that changing later, and so a malicious-looking value is rejected with a clear + // error up front instead of just silently matching nothing. + static void EnsureInScope(string[] relativePaths, Dictionary orderOverrides) + { + var offenders = new List(); + + if (relativePaths != null) + offenders.AddRange(relativePaths.Where(p => !IsWithinModsDirectory(p)).Select(p => p ?? "(null)")); + + if (orderOverrides != null) + offenders.AddRange(orderOverrides.Keys.Where(k => !IsWithinModsDirectory(k))); + + if (offenders.Any()) + throw new ArgumentException( + "The following path(s) are outside the configured mods directory and were rejected: " + + string.Join(", ", offenders.Distinct())); + } + + // A relative path is in scope only if resolving it against the configured mods + // directory still lands inside that directory. Path.IsPathRooted rejects both + // absolute paths and UNC paths outright (Path.Combine silently discards its first + // argument when the second is rooted, which is exactly the bypass that check + // closes). Path.GetFullPath then normalizes any "..\" segments before the + // comparison, which is a proper prefix check against the fully-qualified root + // (with a trailing separator, so "ModsDirectory" can't be spoofed by a sibling + // directory like "ModsDirectoryEvil") rather than a naive string StartsWith. + // This does NOT resolve symlinks - Path.GetFullPath doesn't either - but that's + // fine here: this value is only ever compared against already-scanned + // ModFile.RelativePath strings or used as a dictionary key, never opened + // directly, so a symlink planted inside the mods directory can't be exploited + // through this check. + static bool IsWithinModsDirectory(string relativePath) + { + if (string.IsNullOrWhiteSpace(relativePath) || Path.IsPathRooted(relativePath)) + return false; + + var root = Paths.ModsDirectory; + if (string.IsNullOrWhiteSpace(root)) + return false; + + string fullRoot, candidate; + try + { + fullRoot = Path.GetFullPath(root); + candidate = Path.GetFullPath(Path.Combine(fullRoot, relativePath)); + } + catch (Exception ex) when (ex is ArgumentException || ex is NotSupportedException || ex is PathTooLongException) + { + return false; + } + + var rootWithSeparator = fullRoot.EndsWith(Path.DirectorySeparatorChar) + ? fullRoot + : fullRoot + Path.DirectorySeparatorChar; + + return candidate.Equals(fullRoot, StringComparison.OrdinalIgnoreCase) + || candidate.StartsWith(rootWithSeparator, StringComparison.OrdinalIgnoreCase); + } } }