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
- 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
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