Fix: Add missing subsystem-level security tests for Utilities#91
Merged
Malcolmnixon merged 1 commit intomainfrom Apr 5, 2026
Merged
Fix: Add missing subsystem-level security tests for Utilities#91Malcolmnixon merged 1 commit intomainfrom
Malcolmnixon merged 1 commit intomainfrom
Conversation
- Add Utilities_SafePathHandling_PathTraversal_ThrowsException test - Add Utilities_SafePathHandling_AbsolutePath_ThrowsException test - Add Utilities_SafePathHandling_NullInput_ThrowsException test - Update utilities.yaml to declare the new tests Agent-Logs-Url: https://github.com/demaconsulting/SarifMark/sessions/038c3c55-6bc1-4026-9215-6141f712609b Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
Copilot created this pull request from a session on behalf of
Malcolmnixon
April 5, 2026 17:23
View session
Contributor
There was a problem hiding this comment.
Pull request overview
This PR strengthens the Utilities subsystem security verification by adding subsystem-level tests that explicitly demonstrate PathHelpers.SafePathCombine rejects unsafe inputs (path traversal, absolute paths, null), and then links those tests to the relevant subsystem requirement so reviewers don’t need to rely on unit tests to confirm the security behavior.
Changes:
- Added three new subsystem-level negative tests in
UtilitiesTests.csfor traversal, absolute paths, and null input handling. - Updated the Utilities requirement doc to reference the new tests under
SarifMark-Utilities-SafePathHandling.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/DemaConsulting.SarifMark.Tests/Utilities/UtilitiesTests.cs | Adds subsystem-level security tests asserting unsafe path inputs are rejected. |
| docs/reqstream/sarifmark/utilities/utilities.yaml | Links the new subsystem tests to SarifMark-Utilities-SafePathHandling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Malcolmnixon
approved these changes
Apr 5, 2026
This was referenced Apr 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Performed formal reviews of all 14 review-sets defined in
.reviewmark.yaml. 13 review-sets passed without issues. One issue was identified in the SarifMark-Utilities review-set.Issue Found
The
UtilitiesTests.cssubsystem test file contained only one test (happy path), but the subsystem requirementSarifMark-Utilities-SafePathHandlingemphasizes preventing path-traversal vulnerabilities. The subsystem-level tests did not directly demonstrate the security properties — reviewers had to descend to unit-level tests to verify them.Changes
test/DemaConsulting.SarifMark.Tests/Utilities/UtilitiesTests.csAdded three new subsystem-level tests that demonstrate the security capabilities at the subsystem level:
Utilities_SafePathHandling_PathTraversal_ThrowsException— proves path-traversal attacks are rejectedUtilities_SafePathHandling_AbsolutePath_ThrowsException— proves absolute paths are rejectedUtilities_SafePathHandling_NullInput_ThrowsException— proves null inputs are rejecteddocs/reqstream/sarifmark/utilities/utilities.yamlUpdated requirement
SarifMark-Utilities-SafePathHandlingto declare the three new tests.Verification
All 128 tests pass across .NET 8, 9, and 10.
Review Reports
All 14 formal review reports are saved to
.agent-logs/(not committed):review-SarifMark-Purpose.md→ PASSEDreview-SarifMark-Architecture.md→ PASSEDreview-SarifMark-Design.md→ PASSEDreview-SarifMark-AllRequirements.md→ PASSEDreview-SarifMark-Cli.md→ PASSEDreview-SarifMark-Sarif.md→ PASSEDreview-SarifMark-SelfTest.md→ PASSEDreview-SarifMark-Utilities.md→ ISSUES FOUND (fixed by this PR)review-SarifMark-Program.md→ PASSEDreview-SarifMark-Cli-Context.md→ PASSEDreview-SarifMark-Sarif-SarifResult.md→ PASSEDreview-SarifMark-Sarif-SarifResults.md→ PASSEDreview-SarifMark-SelfTest-Validation.md→ PASSEDreview-SarifMark-Utilities-PathHelpers.md→ PASSED