fix(firewall): stop the rule audit timing out and echoing its own script - #259
Conversation
Resolving each rule's filters through the CIM association ($r | Get-NetFirewallPortFilter) costs ~80 ms, and the scan did three per rule. Past roughly 500 inbound rules that alone exceeded the 120 s execFile timeout, so Node killed PowerShell -- and because the script sets $ErrorActionPreference to SilentlyContinue there was no stderr, leaving the rejection message as nothing but the entire echoed script. Enumerate each filter class once with -All and join on InstanceID instead, keeping the per-rule association query as a fallback for anything missing from an index (-All can be denied on a policy store). Verified byte-identical RULE output against the old script; a 204-rule machine went from 55.2 s to 20.5 s unelevated, where the address-filter index is denied and every rule takes the slow path. Authenticode verdicts are memoized per path since revocation checks can block on the network. Also stream stdout instead of buffering it, via a new spawnTrackedLines helper. The PROG markers the script emits only move the progress bar if they arrive during the scan, and streaming lets a run that is cut short return its partial rules flagged as truncated rather than failing outright. When a run genuinely fails, report stderr or the exit code -- never the script. Fixes #246
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79578ea48b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pending += chunk | ||
| let nl = pending.indexOf('\n') | ||
| while (nl !== -1) { | ||
| onLine(pending.slice(0, nl)) |
There was a problem hiding this comment.
If onLine throws—for example, when the firewall progress callback races with destruction of the Electron webContents and send() rejects—the exception escapes this asynchronous EventEmitter handler instead of rejecting spawnTrackedLines. In the main process that becomes an uncaught exception and can terminate the application while leaving this promise unsettled; wrap callback invocation, terminate the child, and reject the operation.
Useful? React with 👍 / 👎.
1 participant
What does this PR do?
Makes the Firewall Audit scan complete instead of hitting its timeout, and makes it report a real reason when it genuinely fails.
-Alland join onInstanceID(which is the rule'sName), rather than resolving three CIM associations per rule.-Allcan be denied on a policy store, so a rule missing from an index still falls back to its own association query.spawnTrackedLineshelper inexec-utf8.ts, replacing the bufferedexecFile.applyFirewallChangestoo, which had the same trap.truncated: trueand an amber banner instead of being discarded.Why?
Fixes #246.
The reported symptom was
Error invoking remote method 'firewall:scan'echoing the entire PowerShell script with nothing below it. The issue suggested[Console]::OutputEncodingthrows when PowerShell runs with no attached console. I tested that directly against the exact spawn shape Kudu uses (execFile+windowsHide: true+ piped stdio) and it is not the cause — the preamble succeeds,[Console]::OutputEncoding.CodePageis65001, accented output round-trips, exit code 0.The actual cause is the 120 s timeout. Each
$r | Get-NetFirewall*Filteris a CIM association query costing ~80 ms, and the scan did three per rule — measured at 244 ms/rule. Past roughly 500 inbound rules that alone exceeds 120 s, so Node kills PowerShell; and because the script sets$ErrorActionPreference = 'SilentlyContinue', nothing reaches stderr. Node's rejection message isCommand failed: <the whole command line>, which for this script is several kilobytes of echoed PowerShell with the real reason nowhere in it. That is exactly the reporter's screenshot.Two things follow from streaming rather than buffering:
PROG|markers the script already emits only move the progress bar if they arrive while the scan is running. Buffering meant every progress event landed at once, after the scan was over.truncatedflag and banner rather than silently showing fewer rules.How to test
String.rawscript fromscanFirewallRuleson this branch and onmain, run both, and diff theRULE|lines — they should be identical.Measured on a real 204-rule machine, unelevated (the worst case: the address-filter index is denied, so every rule still takes the slow fallback path):
RULE|outputKudu ships a
requireAdministratormanifest, so in the app all three indexes populate and the join covers every rule.End-to-end through the real
scanFirewallRules:rules=204 total=204 truncated=false progressEvents=9.Checklist
npm testpasses — 2183 tests / 95 files, including 16 new (8 forspawnTrackedLines, 8 covering scan parsing, streamed progress, and the error/truncation paths)npm run buildsucceedsTypecheck is unchanged at 206 pre-existing errors before and after, none in the touched files.