fix: resolve Codacy security findings by davidnmbond · Pull Request #205 · panoramicdata/PanoramicData.NugetManagement

davidnmbond · 2026-11-13T14:30:05Z

CI-14 remediation: the clone containment check lacked a trailing
separator, so an advisory path into a sibling folder sharing the
clone's name prefix (repo-evil next to repo) was rewritten. Covered
by a new test that failed before the fix.
ReviewPacketTests: split the fake AWS key and JWT literals so the
secret scanner no longer reports the test data.
nosemgrep on the 18 remaining Opengrep matches. CommandInjection
fires on every Process.Start, including literal git and cmd.exe in
tests; unsafe-path-combine fires on trusted clone-root parameters.
Earlier input validation did not clear them.

- CI-14 remediation: the clone containment check lacked a trailing separator, so an advisory path into a sibling folder sharing the clone's name prefix (repo-evil next to repo) was rewritten. Covered by a new test that failed before the fix. - ReviewPacketTests: split the fake AWS key and JWT literals so the secret scanner no longer reports the test data. - nosemgrep on the 18 remaining Opengrep matches. CommandInjection fires on every Process.Start, including literal git and cmd.exe in tests; unsafe-path-combine fires on trusted clone-root parameters. Earlier input validation did not clear them.

codacy-production · 2026-11-13T14:32:24Z

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity · 0 duplication

Metric Results

Complexity 0

Duplication 0

View in Codacy

🟢 Coverage 66.67% diff coverage · +0.00% coverage variation

Metric Results

Coverage variation ✅ +0.00% coverage variation (-1.00%)

Diff coverage ✅ 66.67% diff coverage

View coverage diff in Codacy

Coverage variation details

Coverable lines Covered lines Coverage

Common ancestor commit (c97435f) 22793 14520 63.70%

Head commit (ec23bb9) 22793 (+0) 14520 (+0) 63.70% (+0.00%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

Coverable lines Covered lines Diff coverage

Pull request (#205) 12 8 66.67%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.

_{TIP This summary will be updated as you push new changes.}

codacy-production

Pull Request Overview

⚠️ Resolve issues before merging

The path validation still has blocking weaknesses: it does not prevent symlink/reparse-point traversal, and its case-insensitive comparison can accept differently cased paths on case-sensitive systems. These gaps mean remediation may write outside the clone, including to sibling directories or external targets.

The detached-process and folder-opening paths remain untested, increasing regression risk in security-sensitive process-launch behavior. The required shell-launch scenarios are missing; the other required scenarios are present.

Test suggestions

Reject a remediation path targeting a sibling directory with a clone-name prefix.
Verify the scrubber still removes AWS-key-shaped and JWT-shaped fixture values.
Exercise shell-based detached process launching and verify the process is started safely.
Exercise opening a validated directory through the shell.
Test successful and failed detached process launches, including argument forwarding and invalid executable handling.
Test folder opening for valid, non-rooted, missing, and process-launch failure paths.

Prompt proposal for missing tests

Consider implementing these tests if applicable:
1. Exercise shell-based detached process launching and verify the process is started safely.
2. Exercise opening a validated directory through the shell.
3. Test successful and failed detached process launches, including argument forwarding and invalid executable handling.
4. Test folder opening for valid, non-rooted, missing, and process-launch failure paths.

_{TIP How was this review? Give us feedback}

codacy-production · 2026-11-13T14:32:46Z

-			// The path comes from advisory data; never let it leave the clone.
-			if (!fullPath.StartsWith(root, StringComparison.OrdinalIgnoreCase) || !File.Exists(fullPath))
+			// The path comes from advisory data; never let it leave the clone (or reach a sibling sharing its prefix).
+			if (!fullPath.StartsWith(root + Path.DirectorySeparatorChar, StringComparison.OrdinalIgnoreCase) || !File.Exists(fullPath))


_{🔴 HIGH RISK}

This containment check is insufficient on some filesystems: a path within the clone may traverse a symlink/reparse point to an external target, and case-insensitive comparison can accept a differently cased sibling on case-sensitive systems. Resolve and safely validate the target, reject symlink/reparse-point traversal, and use a platform-appropriate path comparison before reading or writing.

_{TIP React 👍 if this review is right, or reply with what's wrong and how it could be better to help improve the reviewer.}

codacy-production · 2026-11-13T14:32:46Z

+			using var process = Process.Start(new ProcessStartInfo // nosemgrep
 			{
-				FileName = path,
+				FileName = path, // nosemgrep
 				UseShellExecute = true
 			});


_{🟡 MEDIUM RISK}

Suggestion: Add tests for OpenFolder covering valid rooted directories, non-rooted and missing paths, and process-start failures. Inject or abstract the launcher so tests do not open a real file manager.

_{TIP React 👍 if this review is right, or reply with what's wrong and how it could be better to help improve the reviewer.}

codacy-production · 2026-11-13T14:32:46Z

@@ -143,7 +143,7 @@ private static void AppendQuoted(StringBuilder builder, string value)
 			startInfo.ArgumentList.Add(argument);
 		}

-		var started = Process.Start(startInfo);
+		var started = Process.Start(startInfo); // nosemgrep


_{🟡 MEDIUM RISK}

Suggestion: Add focused tests for StartViaShell covering successful and failed launches, argument forwarding, invalid executable handling, and the returned process behavior without launching arbitrary applications.

_{TIP React 👍 if this review is right, or reply with what's wrong and how it could be better to help improve the reviewer.}

codacy-production Bot reviewed Nov 13, 2026

View reviewed changes

davidnmbond merged commit cd3fc8f into main Nov 13, 2026
8 checks passed

davidnmbond deleted the fix/codacy-security-items branch November 13, 2026 15:00

Navigation Menu

Repository navigation

fix: resolve Codacy security findings - #205

fix: resolve Codacy security findings#205
davidnmbond merged 1 commit into
mainfrom
fix/codacy-security-items

davidnmbond commented Nov 13, 2026

Uh oh!

codacy-production Bot commented Nov 13, 2026 •

edited

Loading

Uh oh!

codacy-production Bot left a comment

Uh oh!

codacy-production Bot Nov 13, 2026

Uh oh!

codacy-production Bot Nov 13, 2026

Uh oh!

codacy-production Bot Nov 13, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

davidnmbond commented Nov 13, 2026

Uh oh!

codacy-production Bot commented Nov 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Up to standards ✅

Uh oh!

codacy-production Bot left a comment

Choose a reason for hiding this comment

Pull Request Overview

Test suggestions

Uh oh!

codacy-production Bot Nov 13, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot Nov 13, 2026

Choose a reason for hiding this comment

Uh oh!

codacy-production Bot Nov 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

codacy-production Bot commented Nov 13, 2026 •

edited

Loading