fix: preserve multiline PowerShell quote state and BITS URL matching - #15
Conversation
This comment has been minimized.
This comment has been minimized.
WalkthroughPowerShell scanner now tracks single/double-quote state to avoid treating 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 QodoFix PowerShell comment parsing and Start-BitsTransfer URL detection
WalkthroughsDescription• Improve PowerShell block comment parsing to handle quoted markers correctly • Add quote tracking to distinguish literal comment syntax from actual comments • Fix Start-BitsTransfer pattern to match positional URL arguments • Add test coverage for quoted comment markers and positional arguments Diagramflowchart LR
A["PowerShell Parser"] -->|"Add quote tracking"| B["stripPowerShellBlockComments"]
B -->|"Handle escaped quotes"| C["isEscapedPowerShellDoubleQuote"]
D["Start-BitsTransfer Pattern"] -->|"Support positional URLs"| E["Improved Regex"]
F["Test Coverage"] -->|"Quoted markers"| G["New Tests"]
F -->|"Positional arguments"| G
File Changes1. internal/inspect/powershell.go
|
Code Review by Qodo
1. Start-BitsTransfer yields DecisionApproval
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| Pattern: regexp.MustCompile(`(?i)\bStart-BitsTransfer\b(?:.*\s-Source\b.*https?://|\s+https?://)`), | ||
| Action: core.DecisionApproval, | ||
| Reason: "Downloads content via BITS", |
There was a problem hiding this comment.
1. start-bitstransfer yields decisionapproval 📘 Rule violation ⛨ Security
The Windows download builtin rule explicitly returns core.DecisionApproval (and new tests/fixtures assert APPROVAL) for Start-BitsTransfer with a positional URL. This conflicts with the requirement that Windows builds must block APPROVAL paths instead of allowing approval-related behavior to proceed.
Agent Prompt
## Issue description
Windows-related builtins/tests were extended to produce `APPROVAL` for `Start-BitsTransfer` positional-URL downloads, but the compliance requirement says APPROVAL flows must be blocked on Windows builds.
## Issue Context
The PR updates the Windows download builtin pattern and adds new coverage asserting `APPROVAL`.
## Fix Focus Areas
- internal/policy/builtins_windows_download.go[44-46]
- internal/policy/policy_test.go[402-415]
- testdata/fixtures/commands.yaml[240-242]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![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 improves PowerShell block comment stripping by accounting for string literals and updates the Start-BitsTransfer detection policy to support positional URL arguments. Feedback includes handling backtick-escaped comment markers, supporting double-double quote escaping in strings, refactoring the escape-checking logic for broader applicability, and simplifying the Start-BitsTransfer regex to be less restrictive.
| i += 2 | ||
| continue | ||
| } | ||
| if i+1 < len(line) && !inSingleQuote && !inDoubleQuote && line[i] == '<' && line[i+1] == '#' { |
There was a problem hiding this comment.
The PowerShell block comment marker <# can be escaped with a backtick (e.g., `<#), in which case it does not start a comment. The current implementation should check for backtick escaping to avoid incorrectly stripping code that contains an escaped marker.
| if i+1 < len(line) && !inSingleQuote && !inDoubleQuote && line[i] == '<' && line[i+1] == '#' { | |
| if i+1 < len(line) && !inSingleQuote && !inDoubleQuote && line[i] == '<' && line[i+1] == '#' && !isBacktickEscaped(line, i) { |
| case '"': | ||
| if !inSingleQuote && !isEscapedPowerShellDoubleQuote(line, i) { | ||
| inDoubleQuote = !inDoubleQuote | ||
| } |
There was a problem hiding this comment.
PowerShell supports escaping double quotes within double-quoted strings using a double-double quote (""), similar to how single quotes are handled. The current logic only accounts for backtick escaping (`"), which can lead to incorrect quote state tracking if "" is encountered.
case '"':
if !inSingleQuote {
if inDoubleQuote && i+1 < len(line) && line[i+1] == '"' {
b.WriteString("\"\"")
i += 2
continue
}
if !isBacktickEscaped(line, i) {
inDoubleQuote = !inDoubleQuote
}
}| return b.String() | ||
| } | ||
|
|
||
| func isEscapedPowerShellDoubleQuote(line string, idx int) bool { |
There was a problem hiding this comment.
Consider renaming this function to something more generic like isBacktickEscaped. This function checks for backtick escapes which are applicable to various characters in PowerShell (like the <# comment marker), not just double quotes.
| func isEscapedPowerShellDoubleQuote(line string, idx int) bool { | |
| func isBacktickEscaped(line string, idx int) bool { |
| { | ||
| ID: "builtin:windows:start-bitstransfer-url", | ||
| Pattern: regexp.MustCompile(`(?i)\bStart-BitsTransfer\b.*\s-Source\b.*https?://`), | ||
| Pattern: regexp.MustCompile(`(?i)\bStart-BitsTransfer\b(?:.*\s-Source\b.*https?://|\s+https?://)`), |
There was a problem hiding this comment.
The regex for Start-BitsTransfer is too restrictive for positional arguments. It currently only matches if the URL immediately follows the command name (with whitespace) or if the -Source parameter is explicitly used. This will fail to match if other flags (e.g., -Priority, -Description) are placed before a positional URL. A simpler and more robust approach is to match the command followed by any URL.
| Pattern: regexp.MustCompile(`(?i)\bStart-BitsTransfer\b(?:.*\s-Source\b.*https?://|\s+https?://)`), | |
| Pattern: regexp.MustCompile("(?i)\\bStart-BitsTransfer\\b.*https?://"), |
![kody-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
| { | ||
| ID: "builtin:windows:start-bitstransfer-url", | ||
| Pattern: regexp.MustCompile(`(?i)\bStart-BitsTransfer\b.*\s-Source\b.*https?://`), | ||
| Pattern: regexp.MustCompile(`(?i)\bStart-BitsTransfer\b(?:.*\s-Source\b.*https?://|\s+https?://)`), |
There was a problem hiding this comment.
The regular expression for Start-BitsTransfer incorrectly matches commands where a URL is used as a destination, not a source. The pattern .*\s-Source\b.*https?:// is too broad, causing it to match a URL anywhere after the -Source flag appears, including in a -Destination parameter. This results in incorrectly flagging uploads as downloads.
| Pattern: regexp.MustCompile(`(?i)\bStart-BitsTransfer\b(?:.*\s-Source\b.*https?://|\s+https?://)`), | |
| Pattern: regexp.MustCompile(`(?i)\bStart-BitsTransfer\b(?:(?:.*\s)?-Source\s+['"]?https?://|\s+['"]?https?://)`), |
Warning
This is an experimental feature that generates committable changes. Review the diff before applying. Results may be incorrect.
Prompt for LLM
File internal/policy/builtins_windows_download.go:
Line 44:
The following Go regular expression is intended to detect when the PowerShell command `Start-BitsTransfer` is used to download a file from an HTTP URL. It needs to handle both named parameters (e.g., `-Source http://...`) and positional parameters (e.g., `Start-BitsTransfer http://...`). However, the current regex has a flaw where it incorrectly flags uploads (e.g., `Start-BitsTransfer -Source C:\file.txt -Destination http://...`) as downloads. Please analyze the provided regex, identify the part that causes this false positive, and suggest a more precise version that correctly identifies only downloads.
Suggested Code:
Pattern: regexp.MustCompile(`(?i)\bStart-BitsTransfer\b(?:(?:.*\s)?-Source\s+['"]?https?://|\s+['"]?https?://)`),
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15 +/- ##
==========================================
+ Coverage 74.50% 74.52% +0.01%
==========================================
Files 84 84
Lines 10154 10185 +31
==========================================
+ Hits 7565 7590 +25
- Misses 2030 2034 +4
- Partials 559 561 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 101-145: The bug is that inSingleQuote and inDoubleQuote are
reinitialized per line, causing multi-line PowerShell strings to be lost and
block-comment detection to misfire; move the declarations for inSingleQuote and
inDoubleQuote out of the per-line scope so their state persists across lines
(e.g., make them fields on the parser struct or function parameters that are
returned/updated), update the calling code to pass/receive these booleans (or
use pointer/struct state) and keep using isEscapedPowerShellDoubleQuote,
blockCommentDepth, and the existing loop logic unchanged so multi-line quoted
strings correctly prevent "<#" from being interpreted as a block comment start.
🪄 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: cb129895-148a-4a98-bca9-36b0bb3f492d
📒 Files selected for processing (5)
internal/inspect/powershell.gointernal/inspect/powershell_test.gointernal/policy/builtins_windows_download.gointernal/policy/policy_test.gotestdata/fixtures/commands.yaml
php-workx
changed the title
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 68-77: Before mutating the quote state with
stripPowerShellBlockComments, save the current line-start quote context (e.g.,
capture inSingleQuote and inDoubleQuote into local vars like
startInSingleQuote/startInDoubleQuote) inside the loop that iterates lines; then
use those saved start-of-line flags when performing the full-line/comment
fast-path check (the logic that treats a line beginning with '#' as a comment)
so a '#' that is inside a string continued from the prior line is not
misclassified as a comment. Ensure you still call stripPowerShellBlockComments
to update the ongoing quote state after saving the start-of-line flags.
🪄 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: eb837947-a4c5-4847-be8f-cee5cd85a1da
📒 Files selected for processing (2)
internal/inspect/powershell.gointernal/inspect/powershell_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/inspect/powershell_test.go
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:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/inspect/powershell.go (1)
141-154: Consider handling doubled double-quote escapes ("") in addition to backtick escapes.PowerShell supports two escape mechanisms inside double-quoted strings: backtick (
`") and doubled quote (""). The current code handles backtick viaisEscapedPowerShellDoubleQuotebut treats""as close-then-open, which happens to work for most cases due to the double toggle. However, edge cases like"<#""..."could theoretically confuse the state.Practical bypass impact is limited since the double-toggle often preserves signal detection, so this is optional.
Potential enhancement for doubled double-quote handling
case '"': - if !*inSingleQuote && !isEscapedPowerShellDoubleQuote(line, i) { + if !*inSingleQuote && !isEscapedPowerShellDoubleQuote(line, i) { + // Handle "" escape inside double-quoted string + if *inDoubleQuote && i+1 < len(line) && line[i+1] == '"' { + b.WriteString(`""`) + i += 2 + continue + } *inDoubleQuote = !*inDoubleQuote }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/inspect/powershell.go` around lines 141 - 154, The double-quote case currently only checks isEscapedPowerShellDoubleQuote but should also treat a doubled double-quote ("") as an escaped quote when inside a double-quoted PowerShell string; update the '"' branch so that if not *inSingleQuote and not isEscapedPowerShellDoubleQuote(line, i) and i+1 < len(line) && line[i+1] == '"' then append the pair (like b.WriteString(`""`)), advance i by 2 and continue instead of toggling *inDoubleQuote; otherwise keep the existing toggle behavior for normal quotes. This uses the existing symbols inDoubleQuote, inSingleQuote, isEscapedPowerShellDoubleQuote, line and i to locate and fix the logic.
🤖 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/inspect/powershell.go`:
- Around line 141-154: The double-quote case currently only checks
isEscapedPowerShellDoubleQuote but should also treat a doubled double-quote ("")
as an escaped quote when inside a double-quoted PowerShell string; update the
'"' branch so that if not *inSingleQuote and not
isEscapedPowerShellDoubleQuote(line, i) and i+1 < len(line) && line[i+1] == '"'
then append the pair (like b.WriteString(`""`)), advance i by 2 and continue
instead of toggling *inDoubleQuote; otherwise keep the existing toggle behavior
for normal quotes. This uses the existing symbols inDoubleQuote, inSingleQuote,
isEscapedPowerShellDoubleQuote, line and i to locate and fix the logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3eec9335-9a25-43fd-bcca-7dc267448062
📒 Files selected for processing (2)
internal/inspect/powershell.gointernal/inspect/powershell_test.go
1 participant
Summary
Improve the follow-up Windows review fixes by preserving PowerShell quote state across lines when stripping block comments, and by recognizing positional
Start-BitsTransfersource URLs.Test Plan
go test ./internal/inspect -run 'TestScanPowerShell_(MultilineQuotedCommentMarkersAreLiteral|QuotedCommentMarkersAreLiteral|CommentSkipping|NestedBlockComments_ResumesAfterClose|DetectsWindowsSignals)' -count=1go test ./internal/policy ./internal/core -run 'TestEvaluateBuiltins_WindowsStartBitsTransferPositionalURL|TestClassify_GoldenFixtures' -count=1just check-localChecklist
just devpassesSummary by CodeRabbit
Bug Fixes
Tests