diff --git a/.reviewmark.yaml b/.reviewmark.yaml index ba42dc8..a348813 100644 --- a/.reviewmark.yaml +++ b/.reviewmark.yaml @@ -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 @@ -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 @@ -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 @@ -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 diff --git a/docs/design/definition.yaml b/docs/design/definition.yaml index 5eac5d7..76e68e8 100644 --- a/docs/design/definition.yaml +++ b/docs/design/definition.yaml @@ -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 diff --git a/docs/design/version-mark/cli/program.md b/docs/design/version-mark/cli/program.md index 1d29814..bc8aee0 100644 --- a/docs/design/version-mark/cli/program.md +++ b/docs/design/version-mark/cli/program.md @@ -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`. diff --git a/docs/design/version-mark/configuration/configuration.md b/docs/design/version-mark/configuration/configuration.md index 1c6303a..44c5087 100644 --- a/docs/design/version-mark/configuration/configuration.md +++ b/docs/design/version-mark/configuration/configuration.md @@ -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). diff --git a/docs/design/version-mark/configuration/lint-issue.md b/docs/design/version-mark/configuration/lint-issue.md new file mode 100644 index 0000000..906240c --- /dev/null +++ b/docs/design/version-mark/configuration/lint-issue.md @@ -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` | 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`. diff --git a/docs/design/version-mark/linting/lint.md b/docs/design/version-mark/linting/lint.md deleted file mode 100644 index 6d2bab1..0000000 --- a/docs/design/version-mark/linting/lint.md +++ /dev/null @@ -1,72 +0,0 @@ -# Lint Unit - -## Overview - -The `Lint` class (`Lint.cs`) is an internal static class with no instance state. All -validation logic is contained in static methods. - -## Run Method - -`Run` is the public entry point. It accepts a `Context` (for output) and the path to the -configuration file to validate. It returns `true` when no issues are found, `false` -otherwise. - -The method proceeds through the following steps: - -1. Reports `"Linting ''..."` to indicate the file under examination. -2. If the file does not exist, reports `{file}: error: Configuration file not found` and - returns `false`. -3. Parses the file as YAML. On `YamlException`, calls `FormatLocation` to include the - source position in the message and reports the parse error, returning `false`. -4. If the YAML stream contains no documents, reports an error and returns `false`. -5. Validates that the root node is a `YamlMappingNode`; reports an error and returns - `false` if not. -6. Reports a warning for each top-level key that is not `"tools"`. -7. Checks that the `tools` key is present and its value is a `YamlMappingNode`; reports - an error and returns `false` for either failure. -8. Checks that at least one tool entry is present. -9. Calls `ValidateTool` for each tool entry, accumulating the returned issue count. -10. If the total issue count is zero, reports `"'': No issues found"`. - -Returns `issueCount == 0`. - -## ValidateTool Method - -`ValidateTool` validates a single tool's `YamlMappingNode`. It returns the number of -issues found for that tool. - -For each entry in the tool's mapping: - -- Both the key and value must be `YamlScalarNode` instances; non-scalar nodes are reported - as errors. -- Keys not present in the valid set (`command`, `command-win`, `command-linux`, - `command-macos`, `regex`, `regex-win`, `regex-linux`, `regex-macos`) are reported as - warnings. -- For `command`: must be present and non-empty. -- For `regex`: must be present, non-empty, a compilable regular expression (see - `TryCompileRegex`), and must contain the named capture group `version` (verified via - `Regex.GetGroupNames().Contains("version")` on the compiled regex). -- For OS-specific `command-*` keys: if present, must not be empty. -- For OS-specific `regex-*` keys: if present and non-empty, must be a compilable regular - expression and must also contain the `version` named capture group. - -After iterating all entries, missing `command` and missing `regex` fields are each reported -as separate errors. - -## TryCompileRegex Method - -`TryCompileRegex` attempts to compile the pattern using `new Regex(value, RegexOptions.None, -RegexTimeout)`, where `RegexTimeout` is one second. On `ArgumentException`, it reports the -compile error and returns `null`. If compilation succeeds it returns the compiled `Regex` -instance so callers can immediately inspect it (e.g., to check `GetGroupNames()`). - -## FormatLocation Method - -`FormatLocation` constructs a source-position prefix from a file path and the 0-based line -and column numbers supplied by YamlDotNet. It always converts to 1-based display values by -adding 1 to both, producing: - -- `filePath(line+1,column+1)` - -The resulting prefix is used in all error and warning messages to produce output in the -familiar `file(line,col): error: ` format understood by CI systems and editors. diff --git a/docs/design/version-mark/linting/linting.md b/docs/design/version-mark/linting/linting.md deleted file mode 100644 index ae86da9..0000000 --- a/docs/design/version-mark/linting/linting.md +++ /dev/null @@ -1,9 +0,0 @@ -# Linting Subsystem - -## Overview - -The lint subsystem validates `.versionmark.yaml` configuration files without stopping at -the first error. It is invoked by the `--lint []` command and reports all -issues found before returning. This satisfies requirement `VersionMark-CommandLine-Lint`. - -The lint subsystem consists of a single unit: `Lint`, which performs all validation logic. diff --git a/docs/design/version-mark/self-test/path-helpers.md b/docs/design/version-mark/self-test/path-helpers.md index 6f33e8a..a74ecda 100644 --- a/docs/design/version-mark/self-test/path-helpers.md +++ b/docs/design/version-mark/self-test/path-helpers.md @@ -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`. diff --git a/docs/design/version-mark/version-mark.md b/docs/design/version-mark/version-mark.md index 2ee852a..b2766b4 100644 --- a/docs/design/version-mark/version-mark.md +++ b/docs/design/version-mark/version-mark.md @@ -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 diff --git a/docs/reqstream/version-mark/configuration/load.yaml b/docs/reqstream/version-mark/configuration/load.yaml index 3dedc97..3793022 100644 --- a/docs/reqstream/version-mark/configuration/load.yaml +++ b/docs/reqstream/version-mark/configuration/load.yaml @@ -10,7 +10,7 @@ sections: tags: - configuration tests: - - ConfigurationLoad_NonExistentFile_Fails + - ConfigurationLoadSubsystem_Run_NonExistentFile_Fails - id: VersionMark-Load-YamlParsing title: The tool shall report an error when the configuration file contains invalid YAML. @@ -20,7 +20,7 @@ sections: tags: - configuration tests: - - ConfigurationLoad_InvalidYaml_Fails + - ConfigurationLoadSubsystem_Run_InvalidYaml_Fails - id: VersionMark-Load-ToolsSection title: The tool shall report an error when the configuration file is missing a non-empty 'tools' section. @@ -30,7 +30,7 @@ sections: tags: - configuration tests: - - ConfigurationLoad_MultipleErrors_ReportsAllErrorsInSinglePass + - ConfigurationLoadSubsystem_Run_MultipleErrors_ReportsAllErrorsInSinglePass - id: VersionMark-Load-ToolCommand title: The tool shall report an error when a tool is missing a non-empty 'command' field. @@ -40,7 +40,7 @@ sections: tags: - configuration tests: - - ConfigurationLoad_MultipleErrors_ReportsAllErrorsInSinglePass + - ConfigurationLoadSubsystem_Run_MultipleErrors_ReportsAllErrorsInSinglePass - id: VersionMark-Load-ToolRegex title: The tool shall report an error when a tool is missing a non-empty 'regex' field. @@ -50,7 +50,7 @@ sections: tags: - configuration tests: - - ConfigurationLoad_MultipleErrors_ReportsAllErrorsInSinglePass + - ConfigurationLoadSubsystem_Run_MultipleErrors_ReportsAllErrorsInSinglePass - id: VersionMark-Load-RegexValid title: The tool shall report an error when a regex value cannot be compiled. @@ -60,7 +60,7 @@ sections: tags: - configuration tests: - - ConfigurationLoad_InvalidRegex_ReportsError + - ConfigurationLoadSubsystem_Run_InvalidRegex_ReportsError - id: VersionMark-Load-RegexVersion title: The tool shall report an error when a regex does not contain a named 'version' capture group. @@ -70,7 +70,7 @@ sections: tags: - configuration tests: - - ConfigurationLoad_RegexWithoutVersionGroup_ReportsError + - ConfigurationLoadSubsystem_Run_RegexWithoutVersionGroup_ReportsError - id: VersionMark-Load-OsOverrides title: The tool shall report an error when an OS-specific command or regex override is empty. @@ -81,7 +81,7 @@ sections: tags: - configuration tests: - - ConfigurationLoad_EmptyOsSpecificOverride_ReportsError + - ConfigurationLoadSubsystem_Run_EmptyOsSpecificOverride_ReportsError - id: VersionMark-Load-UnknownKeys title: The tool shall report unknown keys as non-fatal warnings that do not fail loading. @@ -92,7 +92,7 @@ sections: tags: - configuration tests: - - ConfigurationLoad_UnknownKey_IsWarningNotError + - ConfigurationLoadSubsystem_Run_UnknownKey_IsWarningNotError - id: VersionMark-Load-ErrorLocation title: The tool shall report all load findings with the filename and line/column location. @@ -102,7 +102,7 @@ sections: tags: - configuration tests: - - ConfigurationLoad_Error_IncludesFileAndLineInfo + - ConfigurationLoadSubsystem_Run_Error_IncludesFileAndLineInfo - id: VersionMark-Load-AllIssues title: The tool shall report all load issues in a single pass without stopping at the first error. @@ -112,7 +112,7 @@ sections: tags: - configuration tests: - - ConfigurationLoad_MultipleErrors_ReportsAllErrorsInSinglePass + - ConfigurationLoadSubsystem_Run_MultipleErrors_ReportsAllErrorsInSinglePass - id: VersionMark-Load-Method title: >- @@ -143,3 +143,32 @@ sections: - VersionMarkConfig_Load_OsSpecificInvalidRegex_ReturnsNullConfig - VersionMarkConfig_Load_MultipleErrors_ReportsAll - VersionMarkConfig_Load_IssuesContainFilePath + + - id: VersionMark-Load-LintIssue + title: >- + The LintIssue record shall store file path, line, column, severity, and description, + and format them as "{FilePath}({Line},{Column}): {severity}: {Description}" with + severity in lowercase. + justification: | + A consistent, editor-friendly format (identical to the MSBuild/GCC diagnostic + convention) lets CI systems and editors parse the output automatically and + navigate to the exact location of each issue. + tests: + - LintIssue_Constructor_AllFields_AreStoredCorrectly + - LintIssue_ToString_Error_ProducesLowercaseSeverity + - LintIssue_ToString_Warning_ProducesLowercaseSeverity + + - id: VersionMark-Load-VersionMarkLoadResult + title: >- + The VersionMarkLoadResult record shall bundle the loaded configuration and all + validation issues, routing errors to the error stream and warnings to standard + output when ReportIssues is called. + justification: | + Bundling the config and issues in a single return value lets callers + distinguish between fatal errors (which prevent loading) and non-fatal + warnings (which allow loading to proceed). Routing errors to the error stream + ensures non-zero exit codes are set for fatal issues. + tests: + - VersionMarkLoadResult_Constructor_AllFields_AreStoredCorrectly + - VersionMarkLoadResult_ReportIssues_Error_WritesToErrorStream + - VersionMarkLoadResult_ReportIssues_Warning_WritesToStdOut diff --git a/docs/reqstream/version-mark/self-test/path-helpers.yaml b/docs/reqstream/version-mark/self-test/path-helpers.yaml index 0911d90..7e50308 100644 --- a/docs/reqstream/version-mark/self-test/path-helpers.yaml +++ b/docs/reqstream/version-mark/self-test/path-helpers.yaml @@ -17,5 +17,6 @@ sections: - PathHelpers_SafePathCombine_CurrentDirectoryReference_CombinesCorrectly - PathHelpers_SafePathCombine_NestedPaths_CombinesCorrectly - PathHelpers_SafePathCombine_EmptyRelativePath_ReturnsBasePath + - PathHelpers_SafePathCombine_DotDotAsNamePrefix_CombinesCorrectly - PathHelpers_SafePathCombine_NullBasePath_ThrowsArgumentNullException - PathHelpers_SafePathCombine_NullRelativePath_ThrowsArgumentNullException diff --git a/src/DemaConsulting.VersionMark/SelfTest/PathHelpers.cs b/src/DemaConsulting.VersionMark/SelfTest/PathHelpers.cs index 685b59f..8dced42 100644 --- a/src/DemaConsulting.VersionMark/SelfTest/PathHelpers.cs +++ b/src/DemaConsulting.VersionMark/SelfTest/PathHelpers.cs @@ -26,39 +26,37 @@ namespace DemaConsulting.VersionMark.SelfTest; internal static class PathHelpers { /// - /// Safely combines two paths, ensuring the second path doesn't contain path traversal sequences. + /// Safely combines two paths, ensuring the resolved combined path stays within the base directory. /// /// The base path. /// The relative path to combine. /// The combined path. - /// Thrown when relativePath contains invalid characters or path traversal sequences. + /// Thrown when or is . + /// + /// Thrown when the resolved combined path escapes the base directory, or when a supplied path is invalid. + /// + /// Thrown when a supplied path contains an unsupported format. + /// Thrown when the combined or resolved path exceeds the system-defined maximum length. internal static string SafePathCombine(string basePath, string relativePath) { // Validate inputs ArgumentNullException.ThrowIfNull(basePath); ArgumentNullException.ThrowIfNull(relativePath); - // Ensure the relative path doesn't contain path traversal sequences - if (relativePath.Contains("..") || Path.IsPathRooted(relativePath)) - { - throw new ArgumentException($"Invalid path component: {relativePath}", nameof(relativePath)); - } - - // This call to Path.Combine is safe because we've validated that: - // 1. relativePath doesn't contain ".." (path traversal) - // 2. relativePath is not an absolute path (IsPathRooted check) - // This ensures the combined path will always be under basePath + // Combine the paths (preserves the caller's relative/absolute style) var combinedPath = Path.Combine(basePath, relativePath); - // Additional security validation: ensure the combined path is still under the base path. - // This defense-in-depth approach protects against edge cases that might bypass the - // initial validation, ensuring the final path stays within the intended directory. - var fullBasePath = Path.GetFullPath(basePath); - var fullCombinedPath = Path.GetFullPath(combinedPath); + // Security check: resolve both paths to absolute form and verify the combined + // path is still inside the base directory. Path.GetRelativePath handles root + // paths, platform case-sensitivity, and directory-separator normalization natively. + var absoluteBase = Path.GetFullPath(basePath); + var absoluteCombined = Path.GetFullPath(combinedPath); + var checkRelative = Path.GetRelativePath(absoluteBase, absoluteCombined); - // Use GetRelativePath to verify the relationship between paths - var relativeCheck = Path.GetRelativePath(fullBasePath, fullCombinedPath); - if (relativeCheck.StartsWith("..") || Path.IsPathRooted(relativeCheck)) + if (string.Equals(checkRelative, "..", StringComparison.Ordinal) + || checkRelative.StartsWith(".." + Path.DirectorySeparatorChar, StringComparison.Ordinal) + || checkRelative.StartsWith(".." + Path.AltDirectorySeparatorChar, StringComparison.Ordinal) + || Path.IsPathRooted(checkRelative)) { throw new ArgumentException($"Invalid path component: {relativePath}", nameof(relativePath)); } diff --git a/test/DemaConsulting.VersionMark.Tests/Linting/LintingSubsystemTests.cs b/test/DemaConsulting.VersionMark.Tests/Configuration/ConfigurationLoadSubsystemTests.cs similarity index 94% rename from test/DemaConsulting.VersionMark.Tests/Linting/LintingSubsystemTests.cs rename to test/DemaConsulting.VersionMark.Tests/Configuration/ConfigurationLoadSubsystemTests.cs index c813c40..dd285e4 100644 --- a/test/DemaConsulting.VersionMark.Tests/Linting/LintingSubsystemTests.cs +++ b/test/DemaConsulting.VersionMark.Tests/Configuration/ConfigurationLoadSubsystemTests.cs @@ -32,7 +32,7 @@ public class ConfigurationLoadSubsystemTests /// Test that the full configuration load pipeline succeeds and exits cleanly for a valid configuration. /// [TestMethod] - public void ConfigurationLoad_ValidConfig_SucceedsWithZeroExitCode() + public void ConfigurationLoadSubsystem_Run_ValidConfig_SucceedsWithZeroExitCode() { // Arrange - Write a complete and valid configuration to a temp file var tempFile = Path.GetTempFileName(); @@ -68,7 +68,7 @@ public void ConfigurationLoad_ValidConfig_SucceedsWithZeroExitCode() /// Test that the configuration load pipeline reports all errors in a single pass for an invalid configuration. /// [TestMethod] - public void ConfigurationLoad_MultipleErrors_ReportsAllErrorsInSinglePass() + public void ConfigurationLoadSubsystem_Run_MultipleErrors_ReportsAllErrorsInSinglePass() { // Arrange - Write a configuration with multiple errors to a temp file // tool1 is missing 'regex'; tool2 is missing 'command' and has a regex without a 'version' group @@ -120,7 +120,7 @@ public void ConfigurationLoad_MultipleErrors_ReportsAllErrorsInSinglePass() /// Test that the configuration load pipeline fails for invalid YAML content. /// [TestMethod] - public void ConfigurationLoad_InvalidYaml_Fails() + public void ConfigurationLoadSubsystem_Run_InvalidYaml_Fails() { // Arrange - Write syntactically broken YAML to a temp file var tempFile = Path.GetTempFileName() + ".yaml"; @@ -145,7 +145,7 @@ public void ConfigurationLoad_InvalidYaml_Fails() /// Test that the configuration load pipeline reports an error when the config file does not exist. /// [TestMethod] - public void ConfigurationLoad_NonExistentFile_Fails() + public void ConfigurationLoadSubsystem_Run_NonExistentFile_Fails() { // Arrange - Use a path that does not exist var nonExistentPath = Path.Combine(Path.GetTempPath(), Guid.NewGuid() + ".yaml"); @@ -162,7 +162,7 @@ public void ConfigurationLoad_NonExistentFile_Fails() /// Test that the configuration load pipeline reports an error when a regex cannot be compiled. /// [TestMethod] - public void ConfigurationLoad_InvalidRegex_ReportsError() + public void ConfigurationLoadSubsystem_Run_InvalidRegex_ReportsError() { // Arrange - Write a config with a syntactically broken regex (unclosed group) var tempFile = Path.GetTempFileName(); @@ -195,7 +195,7 @@ public void ConfigurationLoad_InvalidRegex_ReportsError() /// Test that the configuration load pipeline reports an error when a regex does not contain a named 'version' capture group. /// [TestMethod] - public void ConfigurationLoad_RegexWithoutVersionGroup_ReportsError() + public void ConfigurationLoadSubsystem_Run_RegexWithoutVersionGroup_ReportsError() { // Arrange - Write a config with a valid regex that lacks the required 'version' group var tempFile = Path.GetTempFileName(); @@ -228,7 +228,7 @@ public void ConfigurationLoad_RegexWithoutVersionGroup_ReportsError() /// Test that the configuration load pipeline reports an error when an OS-specific command override is empty. /// [TestMethod] - public void ConfigurationLoad_EmptyOsSpecificOverride_ReportsError() + public void ConfigurationLoadSubsystem_Run_EmptyOsSpecificOverride_ReportsError() { // Arrange - Write a config with an empty command-win override var tempFile = Path.GetTempFileName(); @@ -262,7 +262,7 @@ public void ConfigurationLoad_EmptyOsSpecificOverride_ReportsError() /// Test that the configuration load pipeline treats unknown keys as non-fatal warnings and succeeds. /// [TestMethod] - public void ConfigurationLoad_UnknownKey_IsWarningNotError() + public void ConfigurationLoadSubsystem_Run_UnknownKey_IsWarningNotError() { // Arrange - Write a config with a valid tool plus an unknown key var tempFile = Path.GetTempFileName(); @@ -296,7 +296,7 @@ public void ConfigurationLoad_UnknownKey_IsWarningNotError() /// Test that configuration load error messages include the filename and line/column location. /// [TestMethod] - public void ConfigurationLoad_Error_IncludesFileAndLineInfo() + public void ConfigurationLoadSubsystem_Run_Error_IncludesFileAndLineInfo() { // Arrange - Write a config missing the required 'command' field and capture error output var tempFile = Path.GetTempFileName(); diff --git a/test/DemaConsulting.VersionMark.Tests/Configuration/LintIssueTests.cs b/test/DemaConsulting.VersionMark.Tests/Configuration/LintIssueTests.cs new file mode 100644 index 0000000..bc4882d --- /dev/null +++ b/test/DemaConsulting.VersionMark.Tests/Configuration/LintIssueTests.cs @@ -0,0 +1,181 @@ +// Copyright (c) 2025 DEMA Consulting +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +using DemaConsulting.VersionMark.Cli; +using DemaConsulting.VersionMark.Configuration; + +namespace DemaConsulting.VersionMark.Tests.Configuration; + +/// +/// Unit tests for the record and the record. +/// +[TestClass] +public class LintIssueTests +{ + /// + /// Test that properties are stored correctly. + /// + [TestMethod] + public void LintIssue_Constructor_AllFields_AreStoredCorrectly() + { + // Arrange & Act + var issue = new LintIssue( + FilePath: "config.yaml", + Line: 3, + Column: 5, + Severity: LintSeverity.Error, + Description: "Missing required field"); + + // Assert + Assert.AreEqual("config.yaml", issue.FilePath); + Assert.AreEqual(3L, issue.Line); + Assert.AreEqual(5L, issue.Column); + Assert.AreEqual(LintSeverity.Error, issue.Severity); + Assert.AreEqual("Missing required field", issue.Description); + } + + /// + /// Test that produces the expected format with lowercase severity for an error. + /// + [TestMethod] + public void LintIssue_ToString_Error_ProducesLowercaseSeverity() + { + // Arrange + var issue = new LintIssue( + FilePath: "config.yaml", + Line: 10, + Column: 2, + Severity: LintSeverity.Error, + Description: "tool 'dotnet' is missing required field 'command'"); + + // Act + var result = issue.ToString(); + + // Assert - severity must be lowercase 'error', not 'Error' + Assert.AreEqual("config.yaml(10,2): error: tool 'dotnet' is missing required field 'command'", result); + } + + /// + /// Test that produces the expected format with lowercase severity for a warning. + /// + [TestMethod] + public void LintIssue_ToString_Warning_ProducesLowercaseSeverity() + { + // Arrange + var issue = new LintIssue( + FilePath: "my.versionmark.yaml", + Line: 4, + Column: 1, + Severity: LintSeverity.Warning, + Description: "unknown key 'extra-field'"); + + // Act + var result = issue.ToString(); + + // Assert - severity must be lowercase 'warning', not 'Warning' + Assert.AreEqual("my.versionmark.yaml(4,1): warning: unknown key 'extra-field'", result); + } + + /// + /// Test that properties are stored correctly. + /// + [TestMethod] + public void VersionMarkLoadResult_Constructor_AllFields_AreStoredCorrectly() + { + // Arrange + var issue = new LintIssue("f.yaml", 1, 1, LintSeverity.Warning, "unknown key"); + var issues = new List { issue }; + + // Act + var loadResult = new VersionMarkLoadResult(null, issues); + + // Assert + Assert.IsNull(loadResult.Config); + Assert.HasCount(1, loadResult.Issues); + Assert.AreSame(issue, loadResult.Issues[0]); + } + + /// + /// Test that routes errors to context.WriteError. + /// + [TestMethod] + public void VersionMarkLoadResult_ReportIssues_Error_WritesToErrorStream() + { + // Arrange + var issue = new LintIssue("config.yaml", 2, 1, LintSeverity.Error, "missing command"); + var loadResult = new VersionMarkLoadResult(null, [issue]); + var originalError = Console.Error; + + try + { + using var errWriter = new StringWriter(); + Console.SetError(errWriter); + + // Act + using var context = Context.Create(["--lint", "config.yaml"]); + loadResult.ReportIssues(context); + + // Assert - errors must be routed to the error stream + var errorOutput = errWriter.ToString(); + StringAssert.Contains(errorOutput, "error:"); + StringAssert.Contains(errorOutput, "missing command"); + Assert.AreEqual(1, context.ExitCode, "ExitCode should be non-zero after reporting an error"); + } + finally + { + Console.SetError(originalError); + } + } + + /// + /// Test that routes warnings to standard output (not errors). + /// + [TestMethod] + public void VersionMarkLoadResult_ReportIssues_Warning_WritesToStdOut() + { + // Arrange + var issue = new LintIssue("config.yaml", 5, 3, LintSeverity.Warning, "unknown key 'x'"); + var originalOut = Console.Out; + var originalError = Console.Error; + using var stdout = new System.IO.StringWriter(); + using var stderr = new System.IO.StringWriter(); + + try + { + Console.SetOut(stdout); + Console.SetError(stderr); + + // Act + using var context = Context.Create(["--lint", "config.yaml"]); + var loadResult = new VersionMarkLoadResult(null, [issue]); + loadResult.ReportIssues(context); + + // Assert - warnings should go to stdout only and must not set the error exit code + Assert.AreEqual(0, context.ExitCode, "ExitCode should remain zero for warnings only"); + StringAssert.Contains(stdout.ToString(), "config.yaml(5,3): warning: unknown key 'x'"); + Assert.AreEqual(string.Empty, stderr.ToString(), "Warnings should not be written to stderr"); + } + finally + { + Console.SetOut(originalOut); + Console.SetError(originalError); + } + } +} diff --git a/test/DemaConsulting.VersionMark.Tests/Linting/LintTests.cs b/test/DemaConsulting.VersionMark.Tests/Configuration/VersionMarkConfigLoadTests.cs similarity index 100% rename from test/DemaConsulting.VersionMark.Tests/Linting/LintTests.cs rename to test/DemaConsulting.VersionMark.Tests/Configuration/VersionMarkConfigLoadTests.cs diff --git a/test/DemaConsulting.VersionMark.Tests/SelfTest/PathHelpersTests.cs b/test/DemaConsulting.VersionMark.Tests/SelfTest/PathHelpersTests.cs index 4b972bf..93869f7 100644 --- a/test/DemaConsulting.VersionMark.Tests/SelfTest/PathHelpersTests.cs +++ b/test/DemaConsulting.VersionMark.Tests/SelfTest/PathHelpersTests.cs @@ -183,4 +183,22 @@ public void PathHelpers_SafePathCombine_EmptyRelativePath_ReturnsBasePath() // Assert Assert.AreEqual(Path.Combine(basePath, relativePath), result); } + + /// + /// Test that SafePathCombine correctly handles paths where a directory name starts with "..". + /// Such names are valid and must not be rejected as false positives. + /// + [TestMethod] + public void PathHelpers_SafePathCombine_DotDotAsNamePrefix_CombinesCorrectly() + { + // Arrange - "..data" is a valid directory name that starts with ".." but is not a traversal segment + var basePath = Path.GetTempPath(); + var relativePath = "..data/file.txt"; + + // Act + var result = PathHelpers.SafePathCombine(basePath, relativePath); + + // Assert + Assert.AreEqual(Path.Combine(basePath, relativePath), result); + } }