Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 11 additions & 17 deletions .reviewmark.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,9 @@ reviews:
- "docs/design/version-mark/configuration/configuration.md" # subsystem design
- "docs/design/version-mark/configuration/version-mark-config.md" # VersionMarkConfig unit design
- "docs/design/version-mark/configuration/tool-config.md" # ToolConfig unit design
- "docs/design/version-mark/configuration/lint-issue.md" # LintIssue unit design
- "test/**/Configuration/ConfigurationSubsystemTests.cs" # subsystem tests
- "test/**/Configuration/ConfigurationLoadSubsystemTests.cs" # load subsystem tests

- id: VersionMark-VersionMarkConfig-Review
title: Review of VersionMarkConfig Unit
Expand All @@ -88,6 +90,7 @@ reviews:
- "docs/design/version-mark/configuration/version-mark-config.md" # design
- "src/**/Configuration/VersionMarkConfig.cs" # implementation
- "test/**/Configuration/VersionMarkConfigTests.cs" # unit tests
- "test/**/Configuration/VersionMarkConfigLoadTests.cs" # Load method unit tests

- id: VersionMark-ToolConfig-Review
title: Review of ToolConfig Unit
Expand All @@ -97,6 +100,14 @@ reviews:
- "src/**/Configuration/VersionMarkConfig.cs" # implementation
- "test/**/Configuration/VersionMarkConfigTests.cs" # unit tests

- id: VersionMark-LintIssue-Review
title: Review of LintIssue Unit
paths:
- "docs/reqstream/version-mark/configuration/load.yaml" # requirements
- "docs/design/version-mark/configuration/lint-issue.md" # design
- "src/**/Configuration/LintIssue.cs" # implementation
- "test/**/Configuration/LintIssueTests.cs" # unit tests

# Capture Subsystem
- id: VersionMark-Capture-Subsystem
title: Review of Capture Subsystem
Expand Down Expand Up @@ -131,23 +142,6 @@ reviews:
- "src/**/Publishing/MarkdownFormatter.cs" # implementation
- "test/**/Publishing/MarkdownFormatterTests.cs" # unit tests

# Linting Subsystem
- id: VersionMark-Linting-Subsystem
title: Review of Linting Subsystem
paths:
- "docs/reqstream/version-mark/linting/linting.yaml" # subsystem requirements
- "docs/design/version-mark/linting/linting.md" # subsystem design
- "docs/design/version-mark/linting/lint.md" # Lint unit design
- "test/**/Linting/LintingSubsystemTests.cs" # subsystem tests

- id: VersionMark-Lint-Review
title: Review of Lint Unit
paths:
- "docs/reqstream/version-mark/linting/lint.yaml" # requirements
- "docs/design/version-mark/linting/lint.md" # design
- "src/**/Linting/Lint.cs" # implementation
- "test/**/Linting/LintTests.cs" # unit tests

# SelfTest Subsystem
- id: VersionMark-SelfTest-Subsystem
title: Review of SelfTest Subsystem
Expand Down
3 changes: 1 addition & 2 deletions docs/design/definition.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,11 @@ input-files:
- docs/design/version-mark/configuration/configuration.md
- docs/design/version-mark/configuration/tool-config.md
- docs/design/version-mark/configuration/version-mark-config.md
- docs/design/version-mark/configuration/lint-issue.md
- docs/design/version-mark/capture/capture.md
- docs/design/version-mark/capture/version-info.md
- docs/design/version-mark/publishing/publishing.md
- docs/design/version-mark/publishing/markdown-formatter.md
- docs/design/version-mark/linting/linting.md
- docs/design/version-mark/linting/lint.md
- docs/design/version-mark/self-test/self-test.md
- docs/design/version-mark/self-test/validation.md
- docs/design/version-mark/self-test/path-helpers.md
Expand Down
6 changes: 4 additions & 2 deletions docs/design/version-mark/cli/program.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,5 +50,7 @@ error handling to `context.WriteError`. These methods satisfy requirements
## RunLint

`RunLint` is a private helper called from `Run`. It resolves the configuration file path,
defaulting to `.versionmark.yaml` when `context.LintFile` is `null`, then delegates to
`Lint.Run`. This satisfies requirement `VersionMark-CommandLine-Lint`.
defaulting to `.versionmark.yaml` when `context.LintFile` is `null`, then calls
`VersionMarkConfig.Load` to validate the configuration. It reports all discovered issues via
`result.ReportIssues` and confirms success when no issues were found. This satisfies
requirement `VersionMark-CommandLine-Lint`.
7 changes: 4 additions & 3 deletions docs/design/version-mark/configuration/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
## Overview

The configuration subsystem reads and interprets the `.versionmark.yaml` file that defines
which tools to capture and how to extract their versions. It consists of two units:
`ToolConfig` (per-tool settings) and `VersionMarkConfig` (the top-level configuration
container).
which tools to capture and how to extract their versions, and reports any validation issues
found during loading. It consists of three units: `ToolConfig` (per-tool settings),
`VersionMarkConfig` (the top-level configuration container and validation entry point), and
`LintIssue` (the types used to surface validation issues to callers).
45 changes: 45 additions & 0 deletions docs/design/version-mark/configuration/lint-issue.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
# LintIssue Unit

## Overview

`LintIssue.cs` contains the types used to surface validation issues found while loading a
`.versionmark.yaml` configuration file. It defines three public types: the `LintSeverity`
enumeration, the `LintIssue` record, and the `VersionMarkLoadResult` record.

## LintSeverity Enumeration

`LintSeverity` classifies the severity of a validation issue:

| Value | Meaning |
|-----------|----------------------------------------------------------------------------|
| `Warning` | Non-fatal advisory message; loading continues. |
| `Error` | Fatal validation failure that prevents the configuration from being built. |

## LintIssue Record

`LintIssue` represents a single issue found during configuration loading. It carries:

| Property | Type | Description |
|---------------|----------------|------------------------------------------------|
| `FilePath` | `string` | Path to the file containing the issue. |
| `Line` | `long` | One-based line number of the issue. |
| `Column` | `long` | One-based column number of the issue. |
| `Severity` | `LintSeverity` | Severity classification. |
| `Description` | `string` | Human-readable description of the issue. |

`ToString` formats the record as `"{FilePath}({Line},{Column}): {severity}: {Description}"`,
where `{severity}` is emitted in lowercase (`warning` or `error`), producing output in
the familiar `file(line,col): error: message` format understood by CI systems and editors.

## VersionMarkLoadResult Record

`VersionMarkLoadResult` is the return type of `VersionMarkConfig.Load`. It bundles two
properties:

| Property | Type | Description |
|----------|----------------------------|-----------------------------------------------------------------------------|
| `Config` | `VersionMarkConfig?` | The loaded configuration, or `null` when errors prevented building it. |
| `Issues` | `IReadOnlyList<LintIssue>` | All validation issues; may contain warnings even when `Config` is non-null. |

The `ReportIssues` method writes all issues to a `Context`, routing `LintSeverity.Error`
issues to `context.WriteError` and `LintSeverity.Warning` issues to `context.WriteLine`.
72 changes: 0 additions & 72 deletions docs/design/version-mark/linting/lint.md

This file was deleted.

9 changes: 0 additions & 9 deletions docs/design/version-mark/linting/linting.md

This file was deleted.

47 changes: 35 additions & 12 deletions docs/design/version-mark/self-test/path-helpers.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,46 @@

## Overview

The `PathHelpers` class (`PathHelpers.cs`) provides a single static method,
`SafePathCombine`, designed to defend against path-traversal attacks when constructing
file paths from partially-trusted input.
`PathHelpers` is a static utility class that provides a safe path-combination method. It
protects callers against path-traversal attacks by verifying the resolved combined path stays
within the base directory. Note that `Path.GetFullPath` normalizes `.`/`..` segments but does
not resolve symlinks or reparse points, so this check guards against string-level traversal
only.

## SafePathCombine Method

`SafePathCombine` takes a `basePath` and a `relativePath` and returns a combined path,
subject to two layers of validation:
```csharp
internal static string SafePathCombine(string basePath, string relativePath)
```

1. **Pre-combination check**: rejects `relativePath` if it contains `".."` or is a rooted
(absolute) path.
2. **Post-combination check**: resolves both paths with `Path.GetFullPath` and calls
`Path.GetRelativePath` to verify the combined path still sits under `basePath`.
Combines `basePath` and `relativePath` safely, ensuring the resulting path remains within
the base directory.

If either check fails, `ArgumentException` is thrown. This defense-in-depth approach
guards against edge-cases that might bypass the initial string check while remaining
straightforward to audit.
**Validation steps:**

1. Reject null inputs via `ArgumentNullException.ThrowIfNull`.
2. Combine the paths with `Path.Combine` to produce the candidate path (preserving the
caller's relative/absolute style).
3. Resolve both `basePath` and the candidate to absolute form with `Path.GetFullPath`.
4. Compute `Path.GetRelativePath(absoluteBase, absoluteCombined)` and reject the input if
the result is exactly `".."`, starts with `".."` followed by `Path.DirectorySeparatorChar`
or `Path.AltDirectorySeparatorChar`, or is itself rooted (absolute), which would indicate
the combined path escapes the base directory.

## Design Decisions

- **`Path.GetRelativePath` for containment check**: Using `GetRelativePath` to verify
containment handles root paths (e.g. `/`, `C:\`), platform case-sensitivity, and
directory-separator normalization natively. The containment test should treat `..` as an
escaping segment only when it is the entire relative result or is followed by a directory
separator, avoiding false positives for valid in-base names such as `..data`.
- **Post-combine canonical-path check**: Resolving paths after combining handles all traversal
patterns — `../`, embedded `/../`, absolute-path overrides, and platform edge cases —
without fragile pre-combine string inspection of `relativePath`.
- **ArgumentException on invalid input**: Callers receive a specific `ArgumentException`
identifying `relativePath` as the problematic parameter, making debugging straightforward.
- **No logging or error accumulation**: `SafePathCombine` is a pure utility method that throws
on invalid input; it does not interact with the `Context` or any output mechanism.

`PathHelpers` is used by `Validation` when constructing paths inside temporary directories
for self-validation tests. This satisfies requirement `VersionMark-PathHelpers-SafeCombine`.
10 changes: 7 additions & 3 deletions docs/design/version-mark/version-mark.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,15 @@ Cli Subsystem → Capture Subsystem (VersionInfo.LoadFromFile) → Publishing Su
### Lint Mode

```text
Cli Subsystem → Linting Subsystem
Cli Subsystem → Configuration Subsystem (VersionMarkConfig.Load)
```

1. The Cli Subsystem (Program) calls `RunLint`, which resolves the config file path.
2. The Linting Subsystem validates the YAML structure and reports all issues.
1. The Cli Subsystem (Program) calls `RunLint`, which resolves the config file path,
defaulting to `.versionmark.yaml`.
2. `RunLint` delegates to `VersionMarkConfig.Load`, which validates the YAML structure and
returns a `VersionMarkLoadResult` containing all `LintIssue` records found.
3. `RunLint` calls `result.ReportIssues` to write all issues to the context, then confirms
success when no issues were found.

### Validate Mode

Expand Down
Loading
Loading