Fix CodeQL and SonarCloud findings; exclude generated files from CodeQL - #8
Conversation
- Remove useless upcasts and simplify map/filter foreach loops flagged by CodeQL (cs/useless-upcast, cs/linq/missed-select, cs/linq/missed-where) - Guard expensive logging call arguments with ILogger.IsEnabled checks to address CA1873 across AgentToolLauncher, GitClient, LoggingSetup, and Program - Simplify Dictionary field initializers to collection expressions (IDE0028), extract repeated ".github" literal to a constant (S1192), and retype an internal-only logger factory parameter for CA1859 - Harden PackageZipExtractor's zip entry extraction with an inline GetFullPath/StartsWith re-verification against the repo root, in addition to the existing SafePathCombine guard, to address the S6096 zip-slip findings - Assert TryParse return values (CA1806), use Assert.Single's return value instead of re-indexing (xUnit2033), and cache JsonSerializerOptions instances (CA1869) in test code - Add paths-ignore for **/obj/** and **/bin/** to .github/codeql-config.yml (plus a matching query-filter exclude for cs/missed-ternary-operator) so CodeQL no longer scans generated build output, mirroring the ApiMark repository's configuration Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Critical package extraction concerns and an unresolved logging allocation issue remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This pull request addresses CodeQL and SonarCloud findings, optimizes logging and allocations, and adds package extraction validation.
Changes:
- Excludes generated build output from CodeQL.
- Modernizes production and test code.
- Adds archive path containment checks and logging optimizations.
File summaries
| File | Description |
|---|---|
test/DemaConsulting.AgentControl.UiTests/TestSupportTypes.cs |
Caches JSON serializer options. |
test/DemaConsulting.AgentControl.UiTests/TestSettingsWriter.cs |
Reuses options and simplifies initialization. |
test/DemaConsulting.AgentControl.Tests/Utilities/UtilitiesSubsystemTests.cs |
Simplifies cleanup filtering. |
test/DemaConsulting.AgentControl.Tests/Logging/LoggingSetupTests.cs |
Guards logging arguments. |
test/DemaConsulting.AgentControl.Tests/LauncherUI/SelectPackageWindowViewModelTests.cs |
Uses the Assert.Single result. |
test/DemaConsulting.AgentControl.Tests/LauncherUI/RepoCardViewModelTests.cs |
Simplifies assertions and typing. |
test/DemaConsulting.AgentControl.Tests/LauncherUI/MainWindowViewModelTests.cs |
Uses returned single elements. |
test/DemaConsulting.AgentControl.Tests/AgentPackageManagement/PackageVersionTests.cs |
Asserts parsing results. |
src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs |
Adds extraction containment validation. |
src/DemaConsulting.AgentControl/Program.cs |
Guards startup logging. |
src/DemaConsulting.AgentControl/Logging/LoggingSetup.cs |
Narrows factory typing and guards logging. |
src/DemaConsulting.AgentControl/LauncherUI/SelectPackageWindow.axaml.cs |
Simplifies dialog result handling. |
src/DemaConsulting.AgentControl/LauncherUI/MainWindowViewModel.cs |
Simplifies card creation. |
src/DemaConsulting.AgentControl/GitIntegration/GitClient.cs |
Adds conditional diagnostic logging. |
src/DemaConsulting.AgentControl/GitIntegration/CommittedAgentFilesCache.cs |
Uses collection expressions. |
src/DemaConsulting.AgentControl/AgentToolLauncher/AgentToolLauncher.cs |
Avoids unnecessary log argument construction. |
src/DemaConsulting.AgentControl/AgentPackageManagement/PackageVersionCache.cs |
Uses collection expressions. |
src/DemaConsulting.AgentControl/AgentPackageManagement/PackageSource.cs |
Simplifies filename projection. |
.github/codeql-config.yml |
Excludes generated build output from CodeQL. |
Review details
Suppressed comments (1)
src/DemaConsulting.AgentControl/GitIntegration/GitClient.cs:372
- The new
IsEnabled(LogLevel.Debug)guard does not avoid the expensive work it is intended to protect:argumentsTextis still built unconditionally immediately before this block. Thus every successful Git invocation allocates the joined argument string even when neither Debug nor Information logging is enabled. Make the string construction lazy and evaluate it only for enabled log paths or the failure path.
if (_logger.IsEnabled(LogLevel.Debug))
{
_logger.LogDebug(
"Starting git process '{GitExecutable}' with arguments '{Arguments}' in working directory '{WorkingDirectory}'",
_gitExecutablePath, argumentsText, repositoryPath);
}
- Files reviewed: 19/19 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- PackageZipExtractor: rely on PathHelpers.SafePathCombine as the single source of truth for path-containment validation instead of re-deriving the same relative-path check by hand, which had a bug rejecting repo roots ending in a separator (e.g. a drive root). Also add EnsureNoSymlinkAncestors to reject extraction through an existing symlinked/junctioned ancestor directory, since Path.GetFullPath performs only lexical normalization and never resolves filesystem links. - GitClient.RunGit: make the joined arguments string lazily computed via Lazy<string> so it is only evaluated once, and only when an enabled log statement (or the failure path) actually needs it - the prior IsEnabled(LogLevel.Debug) guard did not help because the string.Join was still being evaluated unconditionally beforehand. Verified with a local Sonnet 5 code-review pass plus a full build and test run (192/192 passing). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Critical build and package-extraction findings remain unresolved, along with logging improvements.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs:317
- The managed-folder boundary is checked before normalizing the entry path. A zip entry such as
.github/agents/../../outside.txtpasses this prefix test, and SafePathCombine still accepts it because the resulting path remains underrepoRoot, allowing extraction outside the four managed folders. Canonicalize the path before checking the managed-folder boundary, then retain the repo-root containment check.
src/DemaConsulting.AgentControl/AgentToolLauncher/AgentToolLauncher.cs:162
- With the application's
MinimumLevel.Debug()configuration, both the Debug and Information checks are enabled during a normal launch, so this change callsstring.Joinseparately in each logging block. A successful launch now formats the same argument list twice instead of once; compute the text once when either level is enabled and reuse it.
if (effectiveLogger.IsEnabled(LogLevel.Information))
{
effectiveLogger.LogInformation(
"Started shell process '{FileName} {Arguments}' as PID {ProcessId} after {ElapsedMilliseconds}ms",
startInfo.FileName, string.Join(' ', startInfo.ArgumentList), process.Id, stopwatch.ElapsedMilliseconds);
src/DemaConsulting.AgentControl/GitIntegration/GitClient.cs:411
- This failed-process warning also unconditionally evaluates
argumentsText.Value, so the expensive argument formatting still occurs when warning logs are filtered out. Add anIsEnabled(LogLevel.Warning)guard around this call to complete the logging optimization.
_logger.LogWarning(
"git process '{GitExecutable} {Arguments}' failed with exit code {ExitCode}; stderr: {StandardError}",
_gitExecutablePath, argumentsText.Value, process.ExitCode, stderr);
src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs:268
- The new reparse-point protection is security-critical, but
PackageZipExtractorTestsonly exercise ordinary extraction and do not verify that a symlink/junction ancestor is rejected (or that extraction cannot write through it). Add a platform-appropriate regression test for this guard before relying on it for the zip-slip finding.
EnsureNoSymlinkAncestors(Path.GetFullPath(repoRoot), destinationDirectory, entry.FullName);
Directory.CreateDirectory(destinationDirectory);
}
entry.ExtractToFile(destinationPath, overwrite: true);
- Files reviewed: 19/19 changed files
- Comments generated: 3
- Review effort level: Lite
- PackageZipExtractor: derive the managed-folder membership check from the canonical (SafePathCombine-resolved) relative path instead of the raw zip-entry path, closing a bypass where an entry such as '.github/agents/../../outside.txt' textually matched a managed-folder prefix but resolved outside every managed folder. - AgentToolLauncher: share a single lazily-computed arguments string across the Debug/Information log guards and the failure path instead of recomputing string.Join separately in each branch (both are enabled by default, so the prior fix computed it twice on every launch). - GitClient: guard the slow-exit and nonzero-exit-code LogWarning calls with IsEnabled(LogLevel.Warning) so the lazily-computed arguments text is not forced when warning logging is disabled. - Add regression tests: a crafted-traversal-entry test for the managed-folder bypass, and a Windows-only junction-ancestor test (skipped on non-Windows runners) verifying extraction through a symlinked/junctioned managed folder is refused. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved package-extraction path and reparse-point safety findings remain, including one critical issue.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs:269
- This guard runs only after
Extracthas already blind-deleted all four managed folders. If.githubis a junction/reparse point whose target containsagents(or another managed folder),DeleteManagedFoldercan resolve through that link and recursively delete content outside the repository before this check throws. Validate the managed-folder ancestors before the deletion phase as well, and cover a populated link target in the regression test.
EnsureNoSymlinkAncestors(Path.GetFullPath(repoRoot), destinationDirectory, entry.FullName);
src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs:246
SafePathCombinealso throwsArgumentExceptionfor malformed or unsupported path syntax, not only when the canonical path escapesrepoRoot(seePathHelpers.SafePathCombine). This catch therefore reports some invalid zip entries as if they escaped the repository and hides the actual cause; use an error message that covers invalid destination paths and preserves the underlying message.
catch (ArgumentException ex)
{
throw new InvalidOperationException(
$"Zip entry '{entry.FullName}' resolves outside the repository root.", ex);
- Files reviewed: 20/20 changed files
- Comments generated: 1
- Review effort level: Lite
- PackageZipExtractor: apply the EnsureNoSymlinkAncestors reparse-point guard to the blind-delete step (DeleteManagedFolder) as well as extraction, since Directory.Delete(recursive: true) follows filesystem links and could otherwise remove content outside repoRoot through a junctioned/symlinked managed-folder ancestor before extraction's own guard is ever reached. Generalized EnsureNoSymlinkAncestors' signature (repoRoot, path, context) so both call sites share one implementation. - Add a regression test proving the delete-step guard actually fires: pre-populates content behind a junctioned ".github" target and asserts it survives a refused Extract call (the existing junction test alone only exercised the extraction-step guard, since its link target started empty). - Add "junctioned", "keepme", "mklink", "NTFS" to the cspell dictionary for the new test/doc content. - Update the PackageZipExtractor design doc to describe the symlink-ancestor guard and its new InvalidOperationException case. Caught via an independent local code-review + formal-review pass before pushing (the formal review additionally caught a stray duplicated code fragment left over from an earlier edit that broke compilation of the test file - fixed prior to this commit). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved ZIP extraction security findings affect root, dangling, and nested reparse points, along with invalid-path error reporting.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs:315
Directory.Existsreturns false for a dangling directory symlink/junction, so this condition skips theReparsePointcheck when.githubpoints to a currently nonexistent target.Directory.CreateDirectory(destinationDirectory)can then follow that link and create the extracted files outsiderepoRoot; inspect the directory entry's attributes directly (handling genuinely missing paths) rather than usingDirectory.Existsas the gate, and add a dangling-link regression test.
if (Directory.Exists(current) && File.GetAttributes(current).HasFlag(FileAttributes.ReparsePoint))
src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs:255
- This catch handles every
ArgumentExceptionfrom path combination, including malformed entry names or other invalid-path failures, but the new message says each case resolved outside the repository root. That produces a false diagnosis for invalid zip entries; either distinguish traversal from invalid-path errors or make the message cover both causes.
catch (ArgumentException ex)
{
throw new InvalidOperationException(
$"Zip entry '{entry.FullName}' resolves outside the repository root.", ex);
- Files reviewed: 22/22 changed files
- Comments generated: 2
- Review effort level: Lite
- EnsureNoSymlinkAncestors now checks the repo root itself, not just ancestors between it and the target path. - Detect dangling symlinks/junctions via File.GetAttributes instead of Directory.Exists, which silently skips the check for broken links. - Add DeleteDirectoryRejectingReparsePoints: a recursive delete that fails closed if the managed folder or any nested descendant is a reparse point, replacing Directory.Delete's link-following recursive mode. - Widen DeleteManagedFolder's try/catch to also cover the ancestor symlink check, so ACL failures surface consistently as InvalidOperationException. - ExtractEntryIfManaged now also catches NotSupportedException from SafePathCombine and preserves the real failure reason in its message instead of always claiming a traversal outside the repo root. - Add XML <exception> doc tag and regression tests for repo-root-is- junction and nested-junction-inside-managed-folder scenarios. - Update design doc for the above. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
A critical reparse-point bypass remains, and dangling-link protection lacks regression coverage.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs:370
- The new
File.GetAttributes/not-found handling is intended to preserve the reparse-point check for dangling links, but the added tests cover only junctions whose targets exist. Add a regression test with a dangling symlink/junction and assert extraction refuses it; otherwise this security case can regress back to the formerDirectory.Existsbypass without detection.
FileAttributes attributes;
try
{
attributes = File.GetAttributes(current);
}
catch (Exception ex) when (ex is FileNotFoundException or DirectoryNotFoundException)
- Files reviewed: 22/22 changed files
- Comments generated: 2
- Review effort level: Lite
AllManagedFoldersExist previously used a plain Directory.Exists check, so a symlinked/junctioned repo root or ancestor (e.g. '.github') could make it report every managed folder as present, letting RepoCardViewModel.EnsureAgentFilesSyncedBeforeLaunch skip Extract entirely and launch using files reached through the link - bypassing all of Extract's reparse-point protections. - AllManagedFoldersExist now runs the same EnsureNoSymlinkAncestors check Extract relies on for each managed folder, treating a folder reached through a reparse-point root/ancestor/self as not present. - Also catches IOException/UnauthorizedAccessException from that check (e.g. an ACL-restricted ancestor) so the method keeps its documented ArgumentNullException-only contract instead of leaking an undocumented exception past RepoCardViewModel's catch clauses. - Add regression tests for an ancestor junction with real folders behind it, and for the managed folder itself being a junction. - Add a cross-platform dangling-symlink/junction regression test for Extract (real symlinks on Linux/macOS, a deleted-target junction on Windows), closing a formal-review-flagged test gap. - Update requirements, design, and verification documentation for the above. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical extraction-safety and moderate logging findings block approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
docs/design/agent-control/repo-sync/package-zip-extractor.md:66
- The updated sentence says all listed failures wrap underlying I/O exceptions and name the zip/repo paths, but reparse-point checks throw
InvalidOperationExceptiondirectly fromEnsureNoSymlinkAncestors/DeleteDirectoryRejectingReparsePoints, and invalid-entry paths use an entry-specific message. Distinguish the wrapped archive/I/O failures from direct validation and reparse-point rejections so this design contract matches the implementation.
a symlink/junction — wrapping the underlying `IOException`/`UnauthorizedAccessException`/
`InvalidDataException` with a message naming the zip path and repo root. The symlink check
docs/reqstream/agent-control/repo-sync/package-zip-extractor.yaml:41
- These two linked tests are Windows-only: each exits via
Assert.Skipwhen!OperatingSystem.IsWindows()(seePackageZipExtractorTests.cs:236-240and:516-519). This requirement link is unfiltered, so ReqStream does not constrain its evidence to Windows;.github/standards/reqstream-usage.md:113-125requireswindows@...for platform-specific tests, as used throughout the existing requirements. Prefix both links withwindows@.
- PackageZipExtractor_AllManagedFoldersExist_AncestorIsJunctionWithRealFolders_ReturnsFalse
- PackageZipExtractor_AllManagedFoldersExist_ManagedFolderItselfIsJunction_ReturnsFalse
src/DemaConsulting.AgentControl/AgentToolLauncher/AgentToolLauncher.cs:182
- The failure path still forces
argumentsText.ValuebeforeLogErroreven when error logging is disabled. A failed launch with a logger filtered aboveErrortherefore still performs the expensivestring.Join, so the CA1873 mitigation is incomplete; guard this logging call withIsEnabled(LogLevel.Error)before reading the lazy value, while retaining the value read needed for the thrown exception message.
startInfo.FileName, argumentsText.Value, stopwatch.ElapsedMilliseconds, ex.NativeErrorCode);
src/DemaConsulting.AgentControl/GitIntegration/GitClient.cs:430
- The failure path still forces
argumentsText.ValuebeforeLogErroreven when error logging is disabled. A failed git invocation with a logger filtered aboveErrortherefore still performs the expensivestring.Join, leaving the CA1873 mitigation incomplete; guard this logging call withIsEnabled(LogLevel.Error)before reading the lazy value, without removing the value read needed for the exception message.
_gitExecutablePath, argumentsText.Value, stopwatch.ElapsedMilliseconds, nativeErrorCode);
test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs:314
- The cross-platform test only covers a dangling link; all real-target ancestor, root, and nested-link cases skip on Linux/macOS and therefore never exercise ordinary symbolic-link metadata or deletion behavior on those runners. Add at least one real-target
Directory.CreateSymbolicLinkregression for the non-Windows path instead of relying solely on Windows junctions.
if (!OperatingSystem.IsWindows())
{
Assert.Skip("Directory junctions are a Windows-only filesystem feature.");
- Files reviewed: 24/24 changed files
- Comments generated: 2
- Review effort level: Lite
Every prior review round found one more symlink/junction edge case in PackageZipExtractor because the detection logic was duplicated and ad-hoc rather than being a single, well-tested primitive. This refactor centralizes it properly: - Add PathHelpers.FindReparsePointInAncestry and FindReparsePointInDescendants: pure, filesystem-aware query primitives that report the first reparse point found (or null), separating detection from policy. - PackageZipExtractor's EnsureNoSymlinkAncestors and DeleteDirectoryRejectingReparsePoints become thin wrappers that apply policy (throw UnsafeRepositoryStateException) on top of the new primitives; ManagedFolderGenuinelyExists uses a direct null check instead of exception-driven control flow. The old inline RejectNestedReparsePoints walker is removed. - Fix a catch-ordering bug in RepoCardViewModel: since UnsafeRepositoryStateException derives from InvalidOperationException, it must be caught before the general InvalidOperationException clause or Launch()'s dedicated handling is unreachable. - Fix remaining CA1873 findings (unguarded expensive LogError arguments) in AgentToolLauncher and GitClient. - Add 10 new PathHelpers unit tests, 1 new RepoCardViewModel regression test, and update 5 existing PackageZipExtractor tests to assert the more specific UnsafeRepositoryStateException. - Update design, reqstream, and verification docs accordingly, including adding windows@ test-evidence prefixes for junction-only tests that are skipped on non-Windows runners, and closing a traceability gap where 5 existing symlink/junction tests were never linked to any requirement. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical repository-safety findings remain in launch preflight and extraction cleanup.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (6)
docs/design/agent-control/repo-sync/package-zip-extractor.md:79
- The implementation uses
DeleteDirectoryTree, which recursively enumerates files/directories and callsDirectory.Delete(..., recursive: false); it does not perform a plain recursive delete. This description is therefore inaccurate and makes the two-phase safety guarantee harder to verify.
*before* deleting anything, then performs a plain recursive delete — this two-phase split avoids
docs/design/agent-control/utilities/path-helpers.md:11
- This expands
PathHelpersfrom lexicalSafePathCombineto filesystem-aware reparse-point queries, but the authoritativedocs/sysml2/model/agent-control/utilities/path-helpers.sysml:3still describes only a safe path-combination utility. The SysML2 modeling standard requires the model to stay synchronized when a unit changes; update that part definition so model queries and rendered structure do not understate the unit's responsibility.
`PathHelpers` is a static utility class providing path-safety primitives with two
complementary halves: `SafePathCombine` performs purely lexical (string-level) containment
validation, while `FindReparsePointInAncestry` and `FindReparsePointInDescendants` perform
filesystem-aware detection of reparse points (symlinks/junctions) that a lexical check alone
cannot see. Callers that assemble a path and then act on the filesystem at it should normally
docs/reqstream/agent-control/launcher-ui/repo-card-view-model.yaml:100
- Adding this exception without amending the preceding
AgentControl-RepoCardViewModel-EnsureSyncedBeforeLaunchrequirement leaves contradictory requirements: that requirement still says launch must never be prevented and every failed re-extraction is non-blocking, while this one says unsafe reparse failures block it. Update the existing requirement, justification, and evidence to explicitly exclude the unsafe-state case so ReqStream does not report simultaneous compliance obligations.
- id: AgentControl-RepoCardViewModel-Launch-UnsafeState
title: >-
The RepoCardViewModel class shall refuse to launch the configured agent tool,
raising an error instead, when the ensure-synced-before-launch sync attempt detects
that a managed agent-file folder is reachable only through a reparse point
(symlink/junction).
docs/reqstream/agent-control/repo-sync/package-zip-extractor.yaml:31
- This requirement hard-codes
UnsafeRepositoryStateExceptionand the internal reparse-point terminology into the requirement itself..github/standards/requirements-principles.mdrequires requirements to state observable WHAT, not implementation details such as exception types or algorithms; keep the security behavior in the requirement and describe the exception/guard in the design instead.
The PackageZipExtractor class shall refuse to extract into, or delete through, a
managed folder that is reachable only through a reparse point (symlink/junction) -
whether the repo root itself, an ancestor of a managed folder, the managed folder
itself, or a directory nested inside it - throwing UnsafeRepositoryStateException
and leaving the affected content untouched.
src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs:316
- This branch also reports junctions, not only symbolic links (
FileAttributes.ReparsePointcovers both), so calling the offender a "symlinked directory" is inaccurate on Windows where the new regression tests create junctions. Use neutral "reparse-point directory" wording so the security diagnostic identifies the actual class of blocked state.
throw new UnsafeRepositoryStateException(
$"'{context}' contains a symlinked directory '{reparsePoint}'; refusing to delete through it.");
src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs:446
- This message is emitted for junctions as well as symbolic links, but only says "symlinked directory". Because the failure is a security stop and the Windows tests exercise junctions, describe it as a reparse-point directory (or explicitly mention both) to avoid misleading diagnostics.
throw new UnsafeRepositoryStateException(
$"'{context}' resolves through a symlinked directory '{reparsePoint}'.");
- Files reviewed: 37/37 changed files
- Comments generated: 3
- Review effort level: Lite
Fixes the actual CI Quality Checks failure: cspell flagged these as unknown words in doc comments describing the reparse-point preflight scan added by the PathHelpers centralization refactor. Not a dotnet-format issue. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings affect unsafe launch and extraction behavior.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
src/DemaConsulting.AgentControl/LauncherUI/RepoCardViewModel.cs:710
- The new hard-stop is not reached for every unsafe repository.
AllManagedFoldersExistreturnsfalsefor a reparse-point path, butEnsureAgentFilesSyncedBeforeLaunchcan return beforeExtractwhen no package is pinned, committed agent files are reported, no package source is configured, or the pinned package is unavailable. In those cases noUnsafeRepositoryStateExceptionis thrown andLaunchproceeds against the linked.githubtree, defeating the security policy. Validate reparse-point safety before those early exits (and cover these cases), or propagate an unsafe result from the existence check.
catch (UnsafeRepositoryStateException)
{
// Deliberately not absorbed into the best-effort false/ErrorOccurred pattern below -
// Launch must treat this as a hard stop, not a "launching anyway" warning.
throw;
src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs:319
- This preflight is performed once per managed folder, while
ExtractinvokesDeleteManagedFoldersequentially for all four folders. If an earlier folder is ordinary and a later folder contains a nested reparse point, the earlier folder is permanently deleted before this throwsUnsafeRepositoryStateException; extraction then stops with a partially destroyed sync. Preflight all four managed trees before the first delete, and cover a later-folder link case.
private static void DeleteDirectoryRejectingReparsePoints(string directoryPath, string context)
{
var reparsePoint = PathHelpers.FindReparsePointInDescendants(directoryPath);
if (reparsePoint is not null)
{
throw new UnsafeRepositoryStateException(
$"'{context}' contains a symlinked directory '{reparsePoint}'; refusing to delete through it.");
}
DeleteDirectoryTree(directoryPath);
src/DemaConsulting.AgentControl/Utilities/PathHelpers.cs:35
- These new filesystem-aware APIs extend
PathHelpersbeyond lexical path combination, but the authoritative SysML unit description still saysPathHelpersis only a safe path-combination utility (docs/sysml2/model/agent-control/utilities/path-helpers.sysml:3). That leaves the architecture model inconsistent with this source and the updated design/requirements/verification companions; update the model description and lint it with the change.
/// <remarks>
/// Combines two complementary halves of path safety: <see cref="SafePathCombine"/> is purely
/// lexical (string-level containment, no file-system I/O), while
/// <see cref="FindReparsePointInAncestry"/>/<see cref="FindReparsePointInDescendants"/> are
/// filesystem-aware (detecting symlinks/junctions that a lexical check alone cannot see).
/// Callers that assemble a path and then touch the filesystem at it should normally use both:
/// first <see cref="SafePathCombine"/> to reject a textually-escaping path, then one of the
/// reparse-point finders to reject a link that would otherwise redirect an apparently-safe
/// path outside its intended root.
/// </remarks>
- Files reviewed: 37/37 changed files
- Comments generated: 1
- Review effort level: Lite
Reverts the symlink/junction/reparse-point detection and rejection
logic added across several PR review rounds (EnsureNoSymlinkAncestors,
PathHelpers.FindReparsePointInAncestry/InDescendants,
UnsafeRepositoryStateException, and the associated RepoCardViewModel
launch-blocking behavior), along with their tests and documentation.
This scope was never requested; it grew out of repeated PR review
findings asking for 'one more edge case'. Per direction, filesystem-
level symlink/junction defenses are out of scope for this tool - a
user who mounts or links a managed folder is on their own.
The legitimate, unrelated fixes are kept:
- PathHelpers.SafePathCombine (lexical zip-slip containment, S6096)
- The canonical ('..'-resolved) relative-path check in
ExtractEntryIfManaged that closes a real managed-folder-membership
traversal bypass
Build and full test suite (202 tests) verified green after the revert;
fix.ps1/lint.ps1 both clean.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Archive extraction can still allow launch after failed synchronization, and logging changes add avoidable allocations.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
src/DemaConsulting.AgentControl/AgentToolLauncher/AgentToolLauncher.cs:146
Lazy<string>allocates aLazyand a capturing delegate on every launch. The previous implementation already joinedArgumentListonce and reused the string, so the stated benefit of avoiding a second formatting pass does not apply; when Information/Debug logging is enabled this adds allocation overhead to every process launch. Compute the text only in an enabled log or failure path, with a local nullable cache if reuse is required.
var argumentsText = new Lazy<string>(() => string.Join(' ', startInfo.ArgumentList));
src/DemaConsulting.AgentControl/GitIntegration/GitClient.cs:367
Lazy<string>allocates aLazyplus a capturing delegate on every Git invocation. The previous code already calledstring.Joinexactly once and reused that result for all log statements, so this does not avoid duplicate formatting; with the production logger's Debug level enabled, it adds allocations to the hot path. Compute the argument text only when an enabled log or the failure message needs it, using a local nullable cache instead of an unconditionalLazy.
var argumentsText = new Lazy<string>(() => string.Join(' ', arguments));
src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs:242
- The PR summary says the S6096 fix adds an inline
GetFullPath/StartsWithre-verification, but this path still relies on the customSafePathCombineresult before passingdestinationPathtoExtractToFile; the only newGetFullPathis used to derive the relative managed-folder check. Add the promised local root-containment guard (or update the description) so the stated analyzer remediation is actually present.
// PathHelpers.SafePathCombine is the single source of truth for containment validation
// (see its own doc remarks); it throws ArgumentException/NotSupportedException both when
// the entry's path would resolve outside repoRoot and for other malformed-path failures,
// so the original message is preserved here rather than assuming every failure is a
// traversal attempt.
- Files reviewed: 21/21 changed files
- Comments generated: 1
- Review effort level: Lite
Addresses PR review feedback that a malicious/malformed zip entry converts a SafePathCombine containment violation into a plain InvalidOperationException, which RepoCardViewModel's best-effort sync path treats as an ordinary failure and launches anyway - after the managed folders had already been blind-deleted. PackageZipExtractor.Extract now resolves and validates every entry's destination path in an up-front pass, before any managed folder is deleted. An invalid or path-escaping entry rejects the whole upgrade immediately, so the repo's existing managed folders are never blind-deleted ahead of a rejected extraction. Extraction itself is unchanged (still no rollback for a genuine I/O failure mid-extract, per architecture.md). Adds a regression test verifying a repo-root-escaping entry is rejected before deletion and pre-existing managed-folder content survives, and updates the design/verification docs accordingly. Build and full test suite (203 tests) verified green; fix.ps1/lint.ps1 both clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Critical and moderate ZIP extraction findings remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
src/DemaConsulting.AgentControl/AgentToolLauncher/AgentToolLauncher.cs:146
- This
Lazy<string>allocates aLazyobject and closure on every launch, even when the supplied logger has all levels disabled. When the normal Serilog configuration is used,Debugis enabled, so the join still runs and this adds allocations compared with the previous one-timestring.Join; use a nullable local and compute the string inside the first enabled-log/failure branch instead.
var argumentsText = new Lazy<string>(() => string.Join(' ', startInfo.ArgumentList));
src/DemaConsulting.AgentControl/GitIntegration/GitClient.cs:367
- This
Lazy<string>allocates aLazyobject and closure on everyRunGitcall, even when all logging is disabled. In the normal application configuration,LoggingSetupsets the minimum level toDebug, so the join still runs and this adds allocations compared with the previous one-timestring.Join; use a nullable local and compute the string inside the first enabled-log/failure branch instead of allocatingLazyper invocation.
var argumentsText = new Lazy<string>(() => string.Join(' ', arguments));
src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs:69
- The class-level remarks above still describe
Extractas open → delete → extract, while this updated method now inserts a pre-delete destination-validation phase. That stale sequence contradicts the safety invariant introduced here and can mislead maintainers about when existing managed content is touched; update the class remarks to document validation before deletion.
/// Opens/validates the package zip, validates every entry's destination path, deletes the
/// four managed folders under <paramref name="repoRoot"/> if present, and extracts the
/// same four folders from the zip into the repo.
- Files reviewed: 23/23 changed files
- Comments generated: 3
- Review effort level: Lite
Summary
Fixes the CodeQL and SonarCloud findings reported against the repository, and updates
.github/codeql-config.ymlto stop CodeQL from scanning generated build output.CodeQL
cs/useless-upcast)foreachloops to LINQ (cs/linq/missed-select,cs/linq/missed-where)paths-ignorefor**/obj/**/**/bin/**(plus a matching query-filter forcs/missed-ternary-operator) so generatedXunitAutoGeneratedEntryPoint.csfiles are no longer scanned, mirroring the ApiMark repository's configSonarCloud
IDE0028: simplifiedDictionaryfield initializers to collection expressionsCA1873: guarded expensive logging call arguments withILogger.IsEnabledchecks (AgentToolLauncher,GitClient,LoggingSetup,Program)CA1859: retyped an internal-only logger factory parameter for improved performanceS1192: extracted the repeated.githubliteral to a constantS6096(zip-slip, BLOCKER): hardenedPackageZipExtractor's entry extraction with an inlineGetFullPath/StartsWithre-verification against the repo root, in addition to the existingSafePathCombineguardCA1806: assertedTryParsereturn values instead of discarding themxUnit2033: usedAssert.Single's return value instead of re-indexingCA1869: cachedJsonSerializerOptionsinstances instead of allocating per callTesting
dotnet buildsucceedsdotnet test test/DemaConsulting.AgentControl.Tests— 192/192 passedpwsh ./fix.ps1run with no further changes neededCo-authored-by: Copilot 223556219+Copilot@users.noreply.github.com