Close the two firewall residues #2432 disclosed (#2436)

[erikdarlingdata]

Closes #2436.

Both residues were filed as "arguably deliberate, decide it explicitly." Reading the code decided them: each is a case where --configure-firewall contradicts a posture the rest of the product already holds, and one of them turns out to be destructive rather than merely untidy.

1. A rule for a surface that is switched off — closed

mcp.network says HOW an endpoint would be exposed; whether it runs at all is config_service.mcp_enabled, and the supervisor stops the server outright when that is false. PlanFirewallRules read only the network block, so the elevated verb opened an inbound allow rule on a port with nothing behind it — the same shape #2414 was filed about, one field over.

The defence for keeping it — the rule is ready for the day the surface is enabled, and enabling it from the Viewer is a store write with no elevated step in it — is real, and loses to three things:

• The product's posture everywhere else is that no listener means no rule. --disable-mcp sweeps the surface, and DarlingMcpHostService's stop path already documents an admin removing the rule with this very verb.
• The two verbs disagreed. --disable-mcp closed the port and the next --configure-firewall re-opened it — and install-darling.ps1 runs that verb on every upgrade, so a deliberately disabled endpoint had its port re-opened by an upgrade and the disable did not stay done.
• The convenience is deferred, not lost: the service's own start-up check reports the missing rule and prints the command.

The rejected alternative is written down beside the decision (DescribeDisabledSurface), because it is the argument the next reader will have again.

The install-time half is the same rule and not just symmetry: before the store exists, mcp.enabled is the value config_service.mcp_enabled is seeded with, so a network block beside enabled = false describes an endpoint that will not start on the first run either. Opening its port was never "ready for later", it was ready for nothing.

2. The upgrade that lands on the pre-change port — closed, and it was worse than the issue said

The issue described the rule landing on the old port "for one cycle". --configure-firewall sweeps a surface's rules for every port before opening the current one, and install-darling.ps1 runs it with the service stopped — which on a managed install means the store is down too, being a child of the service. So on an upgrade of a box whose port was moved in the Viewer, the rule matching that wildcard is the working one. The elevated verb the operator was told to run is what closes the live LAN surface. That is what tipped this away from "keep warning".

Two changes, and they need each other:

The sweep is now conditional on the run being able to vouch for what the surface is doing. Its whole justification is knowing that; a run that fell back to darling.json's seed does not. The port may have moved, so another port's rule may be the live one — and mcp_enabled may have been turned on with --enable-mcp or in the Viewer, which never write back to darling.json, so the file's enabled = false may be years stale and the surface may be serving right now. The rule for the file's port is still created (the fresh-install case, where the file cannot be wrong), and the enable command removes its own exact DisplayName first, so declining the sweep costs nothing in idempotence — it defers the cleanup to a run that can see the control plane. When the store did answer the sweep is authoritative again and nothing changes, or this would trade one residue for the one #2432 was filed to remove.

The withholding is scoped precisely, and the scoping is the interesting part (it was the review finding on the first cut — #2442 (comment)). It is not "never sweep on a fallback", which would stop the installer collecting rules left behind by an exposure that was removed from darling.json. Whether a surface is exposed at all is mcp.network / web.network, which are file-only with no config_service equivalent by the #2389 design — the control plane can switch an exposed surface off, never switch an unexposed one on. So a bind that resolves loopback-only is loopback-only whatever the store would have said, and its sweep needs nothing the run is missing. The sweep is withheld for exactly the surfaces the file exposes on a run that could not read the control plane.

The installer makes that later run happen, re-reconciling after Start-Service on an upgrade. Only on an upgrade, and that is the point rather than an optimisation: a fresh install's first start spends ~2 minutes on initdb and the first migration, so a second call there could not read the store either and would re-print the fallback disclosure about a port that is correct by definition. Skipping it by construction beats skipping it by hoping the timing works out.

It is not a guarantee and the comment says so — the store answers when it answers, the verb bounds its read to ten seconds, and if it still cannot the operator has exactly the remedy they had before. The window narrows; it does not close.

Verification

The suite targets net10.0-windows and cannot run on macOS, so the real test file was compiled into a net10.0 xUnit-shim harness and run against both branches.

• 70 passed / 0 failed on this branch.
• Five of the new assertions are expressible against dev and all five are red there: the two control-plane-disabled cases, the file-seed-disabled case, the installer's post-start guard, and the source pin that the sweep step is gated at all.
• The five that pin the sweep permission itself cannot be compiled against dev (FirewallRulePlan has no SweepOtherPorts there), which is the stronger statement. With those removed, dev runs 60 passed / 5 failed against the same file.

Five existing fixtures gained config.Mcp.Enabled = true. That is the fixtures becoming faithful rather than the assertions weakening — they meant "this endpoint is serving the LAN", which now has to say both halves. All five stay green against dev with that line added, which is what shows it changed nothing about what they were testing.

Deferred

--configure-firewall's not-elevated branch prints a runnable command only for Open plans, so a Remove that wants its sweep hands the operator nothing to run — and when every plan is a Remove it returns 0 saying nothing needs opening, even if a stale rule is still sitting open. That predates this PR; what this PR did was make it reachable more often, since a surface can now be Remove because it is disabled rather than only because it is loopback-only. The explanatory notes are now printed on that path so the operator at least learns why, but the remedy half is filed as #2445 rather than folded in: printing sweep commands and exiting non-zero would fire the installer's re-run banner on every non-elevated loopback install, and telling "a stale rule exists" from "nothing to do" needs a read-only Get-NetFirewallRule probe this branch currently does not make. The issue carries that trade and the measurement that should decide it.

No CHANGELOG entry, deliberately: [3.5.0] is a shipped, dated section and there is no [Unreleased] on dev — #2432 and #2428 both merged after the release and added none either, so an entry here would attribute the fix to a release that does not contain it.

🤖 Generated with Claude Code

[claude]

Correctness: the new SweepOtherPorts guard only covers the Open half of the ambiguity it's meant to close — the Remove half can still delete a live rule.

mcpOpen = mcpExposed && mcpToggle.Enabled (line 2813) means the plan now flips to Remove whenever mcpToggle.Enabled is false — including when that false came from darling.json's file value under Origin == File (store unreadable this run), not from a confirmed control-plane read. But SweepOtherPorts here is !mcpOpen || Origin == ControlPlane, i.e. every Remove plan sweeps unconditionally, regardless of why it's a Remove.

Concretely: an operator enables MCP via --enable-mcp or the Viewer's Settings — by design (EndpointToggleOrigin/#2389) this is a store-only write; darling.json's mcp.enabled stays false forever after. On the next upgrade, install-darling.ps1 step 4c runs --configure-firewall with the service (and therefore the managed store) stopped, so TryReadEndpointTogglesAsync returns mcpStore = null → Origin = File. mcpToggle.Enabled falls back to the file's stale false, mcpOpen becomes false, the plan becomes Remove, and — because SweepOtherPorts is forced true for every Remove — the wildcard sweep deletes the rule for the port the endpoint is actually (per the control plane) serving on.

This is the same class of bug the PR is fixing (an elevated verb the operator runs closing a live LAN surface), just reached through the enable/disable flag instead of the port. It's currently masked in the common case by the new step 5a re-reconcile after Start-Service, but 5a is itself best-effort and bounded to a 10s store read (per TryReadEndpointTogglesAsync's own doc comment) — if the store is still coming up, the rule stays removed on an endpoint that IS running, until a manual re-run. Pre-PR, Action never depended on Enabled at all, so this specific failure mode didn't exist before this change.

The FirewallRulePlan.SweepOtherPorts doc comment (line ~277) explicitly scopes the guard to "exactly one combination: an Open plan whose port came from darling.json's seed" — but the same uncertainty (file vs. control-plane truth) applies just as much when it's the enabled flag, not just the port, that came from the unread file. Consider withholding the sweep whenever Origin == File regardless of mcpOpen/webOpen, not just on the Open side. (Same issue at line 2846 for the web dashboard twin.)

[erikdarlingdata]

Correct, and thank you — that is the same outage arriving through the other field, and I had talked myself into "a Remove needs no opinion about which port is live", which is true of the port and false of the flag.

Fixed in 055ddbe. The gate is now !mcpExposed || Origin == ControlPlane rather than !mcpOpen || .... Not "never sweep on a fallback", because that would stop the installer collecting rules left behind by an exposure that was removed from darling.json: whether a surface is exposed at all is mcp.network / web.network, which are file-only with no config_service equivalent by the #2389 design, so the control plane can switch an exposed surface off but can never switch an unexposed one on. A bind that resolves loopback-only is loopback-only whatever the store would have said, and its sweep needs nothing this run could not read. The sweep is withheld for exactly the surfaces the FILE exposes on a run that could not reach the control plane — where the port and the enable flag are both guesses, and either way of guessing wrong deletes a live rule.

Two tests added, one per direction: PlanFirewallRules_StoreUnreadable_DoesNotSweepAwayARuleTheControlPlaneMayBeServingOn (your exact scenario, MCP and web) and PlanFirewallRules_ALoopbackSurfaceAlwaysSweeps_BecauseNetworkExposureIsFileOnly (the half that must keep sweeping).

[claude]

Reviewed. This is Darling-only service/installer infrastructure (firewall rule planning + install script), no T-SQL and no Lite counterpart — Lite has nothing analogous to the Postgres-store-backed firewall reconcile, so parity doesn't apply here.

One correctness issue posted inline on DarlingCliCommands.cs (line 2826, twin at 2846): the new SweepOtherPorts gate only withholds the wildcard sweep for Open plans whose port fell back to the file's seed. It does not withhold the sweep for Remove plans that resulted from the same fallback (Origin == File) — and since mcpOpen/webOpen now also factor in the Enabled toggle, a Remove can be produced by a stale, unreadable-store fallback just as easily as by a genuine "surface is off" state. Concretely: MCP/web enabled purely via --enable-mcp/the Viewer (store-only, by design) + an upgrade where install-darling.ps1 runs --configure-firewall with the service (and store) stopped → the plan falls back to the file's stale enabled = false, resolves to Remove, and unconditionally sweeps away the rule for the port the endpoint is actually serving on. The new step 5a re-reconcile after Start-Service usually self-heals this, but it's a best-effort, 10s-bounded store read, so it isn't guaranteed to. This looks like the same class of bug the PR is closing, just reached via the enable flag rather than the port — happy to be told I'm missing a guard elsewhere.

Everything else — the EndpointToggleOrigin split, the install script's $isUpgrade capture/placement, the new test coverage — looks sound and well-reasoned. No security issues (no new PowerShell string interpolation of untrusted input; Cidr still flows through the existing CanonicalCidrOrNull/ClassifyAllowFrom gate before reaching a command builder) and no missing-index-DMV suggestions to worry about here since this touches no T-SQL.

[claude]

DescribeDisabledSurface's File-origin branch assumes the file's enabled = false is a pre-first-start seed ("which is the value the store is SEEDED from ... or set {section}.enabled before the first start"). But Origin == File doesn't only mean "fresh install" — TryReadEndpointTogglesAsync (line ~3283) collapses every failure mode into the same null/File result: BYO postgres, a missing stored credential, a 10s timeout, or any other connection exception, all on a box that has already had its first start and whose store may already hold mcp_enabled = true.

Concretely: an established, previously-configured box where the store is just transiently unreachable (e.g. mid-restart, DPAPI credential hiccup) hits this exact branch. The plan becomes Remove with SweepOtherPorts = false, and the CLI loop (~line 3186) prints both messages back to back:

1. plan.Note (this function): "...so the service will not start it and no rule belongs on that port — turn it on with --enable-mcp (or set mcp.enabled before the first start)" — flatly asserts the endpoint is off and frames it as pre-first-start.
2. The sweep-declined message a few lines later (~3229): "...so that off may be stale and the rule may be the one the endpoint is being served on" — the opposite: uncertain, possibly currently serving.

These two lines contradict each other in the same run's output, and the first one actively misleads an operator on a long-lived box into thinking the surface is definitely off when the whole point of the SweepOtherPorts = false branch is that it might not be. DescribeFirewallPortAuthority right above already solves this correctly for the port case by taking storeUnavailableReason and phrasing it as "could NOT read the control plane" without asserting a pre-first-start seed; DescribeDisabledSurface doesn't take that parameter and can't make the same distinction.

[erikdarlingdata]

Right on both counts — Origin == File is every store-read failure, not a fresh install, and the two lines really did contradict each other in the same run with the wrong one first.

Fixed in 0b3fb8e. DescribeDisabledSurface now takes storeUnavailableReason and says the same three-state thing DescribeFirewallPortAuthority says about the port: which plane answered, why it could do no better, and both readings of the value it fell back on — correct on a box whose store has never been written, possibly stale on one that has run before, because --enable-mcp and the Viewer write only the control plane and never back to the file. The control-plane branch keeps its flat wording, because there it really is a fact, and PlanFirewallRules_ControlPlaneAnsweredOff_SaysSoWithoutAStalenessCaveat pins that the two do not collapse into one hedged message that under-states a certain answer.

[claude]

Reviewed. This PR is scoped entirely to Darling's firewall reconcile logic (DarlingCliCommands.PlanFirewallRules, its tests, and install-darling.ps1) — no T-SQL, no Lite-side files touched, and firewall management has no Lite counterpart, so there's no parity drift here.

The core logic — gating Open on the effective enable toggle, and withholding SweepOtherPorts only when a surface is both file-exposed and Origin == File — is internally consistent and well covered by the new tests, including the subtle "control plane already answered, sweep is authoritative again" and "loopback surfaces always sweep because exposure is file-only" cases.

Left one inline finding: DescribeDisabledSurface's File-origin message text assumes the untouched value is a pre-first-start seed, but Origin == File also covers a previously-configured box where the store is merely unreachable right now — and in that case the CLI prints that message immediately followed by the sweep-declined message, which says the opposite (the surface may still be serving). Worth aligning the wording the way DescribeFirewallPortAuthority already does for the port case.

No other correctness, security, or performance issues found.

[claude]

Reviewed the diff (DarlingCliCommands.cs, install-darling.ps1, DarlingFirewallCheckTests.cs). Notes below; no blocking findings.

Correctness

• Traced mcpOpen = mcpExposed && mcpToggle.Enabled end to end: mcpExposed comes purely from ResolveMcpBind (listen/allowFrom/token/managed/container — confirmed it never reads mcp.Enabled), so gating on the toggle's Enabled is genuinely new information, not a double-count. That's the core Firewall residues: an upgrade opens the pre-change port for one cycle, and a disabled surface still gets a rule #2436 fix and it's correctly wired for both MCP and web.
• SweepOtherPorts: !mcpExposed || Origin == ControlPlane (and the web twin) checked against all four quadrants (exposed×origin, open×origin) — matches the accompanying tests in each case, including the two review-driven tightenings in commits 2 and 3 (withholding the sweep on the enable flag being a File-origin guess, not just the port; and softening DescribeDisabledSurface's File-branch wording from a flat fact to a "may be stale" reading). Good catch on both, and the fix is correct.
• Verified the Remove+SweepOtherPorts=false combination (disabled surface, unreadable store) is a deliberate true no-op — no sweep, no open, existing rule (if any) untouched — which matches the printed "leaving alone" message and the design rationale in the comments.
• install-darling.ps1: $isUpgrade is captured before sc.exe create/config runs (correctly, since it stops being observable after), and the 5a re-reconcile is gated to run only post-Start-Service on upgrades, guarded from the fresh-install ~2min initdb window. TryReadEndpointTogglesAsync's 10s bound plus the fallback disclosure means a slow-starting store on the upgrade path degrades to the pre-existing warn-and-remedy behavior rather than hanging or silently guessing — reasonable.
• Confirmed CheckMcpFirewallAsync/CheckWebFirewallAsync (the running service's own read-only verification) are only invoked from inside TryStartServerAsync, i.e. only when the supervisor already decided to start the surface — so a control-plane-disabled surface doesn't spuriously warn about a "missing" rule that was deliberately removed. No inconsistency introduced there.

Lite/Darling parity: No counterpart needed. Lite has no Windows Firewall management surface (it's a standalone desktop app with no installed network-exposed service); grepped for NetFirewallRule/netsh advfirewall across Lite and found only unrelated Azure SQL firewall code. Not a drift case.

Security: This is a net-positive change — it closes an inbound-allow rule that previously stayed open for a control-plane-disabled endpoint (dead port exposure), and closes the upgrade-time race where --configure-firewall could sweep away the live rule when the store was unreadable. No injection surface changes (ReadEndpointTogglesSql is a static parameterless query, untouched by this diff).

Performance: Negligible — one extra best-effort store read (already bounded to 10s, same as the existing #2414 path) on upgrade installs only.

One pre-existing (not introduced by this PR) observation, not a blocker: in the --configure-firewall not-elevated branch, only Open-action plans get a printed remedy command; a Remove-action plan needing its sweep (including the new control-plane-confirmed-off case) prints nothing actionable when not elevated. That gap predates this diff and is outside its scope, but now applies to a more common case (an admin disabling MCP/web) than before.

[claude]

This loop covers the toOpen == 0 case, but the sibling branch just below (the toOpen > 0 mixed case, error.WriteLine("This shell is not elevated...") + the foreach (var plan in plans.Where(p => p.Action == FirewallRuleAction.Open)) loop) only ever surfaces Open plans. A Remove plan's note — including the new DescribeDisabledSurface message this PR adds — is silently dropped there. An operator with one surface to open and another to close (e.g. MCP newly exposed, web just disabled in the Viewer) only ever learns about the first.

More generally: neither non-elevated branch tells the operator that removal also requires elevation. The Open case gets a concrete command to paste into an elevated shell; the Remove case gets nothing actionable in either branch — at best a note here saying "no rule was opened," which reads as "nothing to do" when actually a LAN-exposed rule for a now-disabled surface is still sitting open.

This is more than a pre-existing gap now: this PR's own rationale is that surfaces get disabled via the Viewer with no elevated step, so "toggle off in the Viewer, then run --configure-firewall from a non-elevated shell to check" is now a realistic sequence — and it silently leaves the stale open rule in place, which is a variant of the exact "disable didn't stay done" bug this PR closes elsewhere.

Suggest at minimum printing Remove plans' notes in the mixed-case branch too (matching the toOpen == 0 branch), and making explicit that a Remove also needs an elevated re-run.

[erikdarlingdata]

Fair — the previous commit fixed the wrong half. The mixed case is exactly where the two surfaces disagree and the operator most needs to be told which is which, and it was still dropping the disabled surface entirely.

The notes now print for every plan before either branch, so this path shows the same per-surface lines the elevated path prints as it works.

The remedy half is filed as #2445 rather than folded in: printing a sweep command for Remove plans and exiting non-zero would fire the installer's re-run banner on every non-elevated loopback install, and telling "a stale rule exists" from "nothing to do" needs a read-only Get-NetFirewallRule probe this branch currently does not make. The issue lays out that trade and the measurement that should decide it.

On the CHANGELOG note: checked, and it does not apply here. [3.5.0] is a shipped, dated section, and there is no [Unreleased] on dev — #2432 and #2428 both merged after the release and added no entry either. Adding to [3.5.0] would attribute this fix to a release that does not contain it.

[claude]

Reviewed the change (Darling firewall reconciliation for #2436: the ENABLE flag now gates rule creation, and the sweep is withheld when the run can't tell whether a fallback port/toggle is stale). This is C#/PowerShell, not T-SQL, so the T-SQL style rules don't apply; Lite has no equivalent installer-managed Windows Firewall logic, so there's no Lite/Darling parity concern here.

Overall: the core logic (mcpOpen/webOpen gating, SweepOtherPorts derivation, DescribeDisabledSurface, the install script's upgrade-only re-reconcile at 5a) checks out against the EndpointToggle/ResolveEndpointToggle/DescribeFirewallPortAuthority machinery it builds on, and the new tests in DarlingFirewallCheckTests.cs exercise every branch I traced by hand (control-plane-off, file-seed-off, store-unreadable+exposed, store-unreadable+loopback, sweep-withheld vs sweep-authoritative). $isUpgrade is captured once from $existing before the service is (re)created and used once at 5a — no reassignment hazard. BuildFirewallEnableCommand's exact-DisplayName removal confirms the "declining the sweep costs nothing in idempotence" claim in the comments.

One real gap, left as an inline comment on DarlingCliCommands.cs: the non-elevated branch of ConfigureFirewallAsync never surfaces Remove actions actionably. In the mixed case (some surfaces need Open, others Remove), only Open plans get a note/command printed — a disabled surface's stale LAN-exposed rule (exactly what DescribeDisabledSurface describes) is silently dropped. Since this PR's own premise is that surfaces now get disabled via the Viewer with no elevated step, "toggle off in the Viewer → run --configure-firewall unelevated to check" is a realistic sequence that leaves a stale open rule with no indication a follow-up elevated run is needed.

Minor/non-blocking: CHANGELOG.md doesn't have an entry for this fix under [3.5.0] — every other fix in that file (including the #2432/#2414 predecessors this PR references) gets one with the issue number and a long-form rationale, so this may just be pending before merge.

[claude]

This still leaves a gap that's been raised in earlier review rounds and only half-fixed: the not-elevated branch now explains why via plan.Note, but it never surfaces a remediation command for a Remove plan — only Open plans get a command printed a few lines below (plans.Where(p => p.Action == FirewallRuleAction.Open)); BuildFirewallSweepCommand is never called anywhere in this not-elevated branch.

Concretely: a surface gets disabled from the Viewer (control-plane write, no elevated step), the plan becomes Remove with SweepOtherPorts = true (Origin is ControlPlane, so it's certain — not the withheld case). An operator then runs --configure-firewall from a non-elevated prompt to check status:

• If every other surface is also non-Open (toOpen == 0), this branch prints the disabled-surface note, then "No endpoint wants an open port, so no rule was opened" and returns 0 — implying success/nothing-to-do, even though the stale LAN-facing allow rule from before the surface was disabled is still sitting open and needs an elevated sweep to remove.
• If some other surface does need Open (toOpen > 0, falls into the return 1 branch a few lines further down), the printed elevated-command list only contains that surface's enable command — the disabled surface's sweep command is silently omitted from the remediation list, even though the exit code correctly signals "action needed."

Either way the operator is never given the actual command (DarlingManagedPostgres.BuildFirewallSweepCommand(wildcard), as used in the elevated branch further down) to remove the stranded rule — which is exactly the class of residue this PR exists to close. Since sweeping is idempotent/harmless whether or not a rule currently exists, it seems safe to also print a sweep command for every plan with Action == Remove && SweepOtherPorts, and to only report "nothing to do" when literally nothing — open or sweep — is pending.

[claude]

Reviewed the current state of the diff (post b002dc8, after four prior automated review rounds already tightened SweepOtherPorts, the DescribeDisabledSurface wording, and the not-elevated summary text). This is Darling-only service/installer code (DarlingCliCommands.cs, install-darling.ps1, tests) — no T-SQL, and no Lite counterpart exists (Lite has no installed Windows service or firewall management surface), so parity doesn't apply here.

Traced the core logic and it holds up:

• mcpOpen/webOpen = exposed (file-only, per mcp.enabled is store-authoritative but mcp.network is file-authoritative, and the disagreement logs as success #2389 design) AND enabled (control-plane-authoritative when readable) — correctly gates rule creation on both axes now.
• SweepOtherPorts: !exposed || Origin == ControlPlane is applied uniformly regardless of Action, so a Remove reached via a stale File-origin fallback is correctly withheld from sweeping too (this was the issue flagged and fixed in the 055ddbeb commit) — confirmed this isn't reintroduced.
• install-darling.ps1's $isUpgrade is captured from $existing before sc.exe create/config runs (the one place it's observable), and 5a's re-reconcile is correctly gated to run only post-Start-Service on upgrades, guarded from the fresh-install initdb window. The new InstallScript_ReReconcilesAfterTheFirstStart_ButOnlyOnAnUpgrade test pins both the presence and the ordering correctly.
• No injection surface changes — Cidr still flows through CanonicalCidrOrNull/ClassifyAllowFrom before reaching a command builder, ReadEndpointTogglesSql is untouched and parameterless.

One remaining gap, posted inline on DarlingCliCommands.cs (~line 3192): the not-elevated branch of ConfigureFirewallAsync now prints why a Remove is needed (the plan.Note) but still never prints the actual sweep command for the operator to run elevated — only Open plans get a command. When toOpen == 0, it returns 0 with a "nothing to open" message even if a Remove+SweepOtherPorts plan means a stale rule is still sitting open. This is the same gap the 3rd and 4th review rounds flagged; the most recent commit added the explanatory note but didn't add the missing remediation command, so the underlying gap (no way to learn/run the sweep command without elevation) is still open.

Minor, non-blocking: CHANGELOG.md has no entry for this fix under a version heading, unlike its #2432/#2414 predecessors it references — may just be pending before merge.