microsoft / microsoft/microsoft-ui-reactor

`mur docs`: CLI-supplied `--topic` reaches `Path.Combine` as the later segment, so a rooted value silently relocates the base

Open Beginner friendly
#1,034 0 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
C#
Stars
646
Forks
54
Avg merge
1d 3h
Merged PRs (30d)
84

Description

Follow-up from #1011, which fixed three instances of this class but deliberately left these two out of scope (they sit outside that PR's diff — pre-existing code whose line numbers merely shifted under its hunks).

## The class

`Path.Combine` silently discards everything before a rooted segment. When the later segment is content- or user-derived, that relocates the result **before** any containment check can observe it — and the check then compares an already-escaped path against an already-escaped root and reports success. A guard that runs, returns a correct answer, and answers the wrong question.

`Path.Join` concatenates unconditionally, which leaves the containment check as the sole decider and stops its correctness depending on a property of the input.

## The two remaining sites

```
src/Reactor.Cli/Docs/DiagramProcessor.cs:152 Path.Combine(diagramsRoot, topic)
src/Reactor.Cli/Docs/DiagramProcessor.cs:552 Path.Combine(diagramsRoot, topic)
```

`topic` is CLI-supplied:

```
src/Reactor.Cli/Docs/RenderDiagramsCommand.cs:12 var topic = GetOption(args, "--topic");
src/Reactor.Cli/Docs/CompileCommand.cs:13 var topic = GetOption(args, "--topic");
src/Reactor.Cli/Docs/CheckTierCommand.cs:13 var topic = CompileCommand.GetOption(args, "--topic");
```

Measured behaviour that makes this concrete:

```
Path.Combine("C:\repo\docs\_pipeline\diagrams", "D:/elsewhere") -> "D:/elsewhere"
Path.Join ("C:\repo\docs\_pipeline\diagrams", "D:/elsewhere") -> "C:\repo\docs\_pipeline\diagrams\D:/elsewhere"
```

So `mur docs render-diagrams --topic ` reads from wherever the rooted value points, rather than failing with a clear "no such topic".

## Severity

**Robustness, not a privilege boundary.** This is a dev-time CLI operating on a repository the developer already controls, and the user supplying `--topic` is the user being affected. The cost is a confusing failure instead of a clear one. Filed so the disposition survives rather than because it is urgent.

## Suggested fix

Switch both sites to `Path.Join` plus the existing containment helper, `DocPaths.ResolveContained(diagramsRoot, topic, "Topic")`, which already pairs `Join` with `IsUnder` and exists precisely so this rule lives in one place.

Audit note: the other 34 `Path.Combine` calls in `src/Reactor.Cli/Docs` take literal later segments (`Path.Combine(repoRoot, "docs")` and similar) and are safe — converting them would be churn.

## The general disposition, worth recording

**`Path.Join` for any path construction whose later segment is content-derived; `Path.Combine` only where every segment after the first is a literal.**

One caveat for whoever picks this up: the `github-code-quality` lint that flags this fires on `OutputPathContainmentTests.cs` too, where the `Combine` call is the **subject under measurement** — that test exists to demonstrate the `Combine`-vs-`Join` difference, and applying the suggested rewrite deletes the differential and makes the test measure nothing. Verified on #1011: the rewrite produced `1 failed`. That site should stay as it is.

Contributor guide

Open the contributing guide

Research direction

Start in src/Reactor.Cli/Docs/DiagramProcessor.cs at the two Path.Combine sites, then read DocPaths.ResolveContained and the --topic flow in RenderDiagramsCommand.cs, CompileCommand.cs, and CheckTierCommand.cs. Done means both CLI-supplied topic paths use the existing containment helper, while OutputPathContainmentTests.cs retains its intentional Combine-versus-Join measurement.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
cli
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
86/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.