Security: firewall command injection, darling.json ACL, and MCP Host-header guard (#1646, #1647, #1648)

CHANGELOG.md

@@ -31,6 +31,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **Darling + Lite: security hardening - firewall command injection via `allowFrom`, an unprotected `darling.json`, and MCP hosts with no DNS-rebinding guard** ([#1646], [#1647], [#1648]) - four findings from the 2026-07 maintenance security review, all in the same blast radius: what a local user, or a browser running on the monitoring host, can reach. **(1) Command injection in `--enable-web` / `--enable-mcp` ([#1646]).** `ReconcileEndpointFirewallAsync` read `web.network.allowFrom` (or the MCP equivalent) out of darling.json and interpolated it UNQUOTED into the PowerShell `-Command` string that creates the scoped firewall rule, gated by nothing but a blank check. It was the one `BuildFirewallEnableCommand` caller that never parsed the value first - every other call site passes a canonicalized `IPNetwork.ToString()`, and the `--configure-network` wizard validates before writing, but the toggle verbs load the config with `DarlingConfig.Load`, which deserializes and never calls `Validate()`, so nothing upstream caught it either. An `allowFrom` of `10.0.0.0/8; <command>` therefore executed that command - directly, when the verb runs elevated (the documented way to run it), or via the operator, when it is not, because the non-elevated path PRINTS the fully-injected command and tells them to paste it into an elevated PowerShell. Fixed in two layers, because either alone leaves a sharp edge: the verb now parses `allowFrom` as a CIDR and passes the PARSER'S canonical output (so nothing unvalidated is carried through even on the valid path), refusing outright on a parse failure - no firewall change, and deliberately no printed command, since a handoff executes the injection just as surely; and `BuildFirewallEnableCommand`/`BuildFirewallDisableCommand` now single-quote and escape every value they interpolate (PowerShell escapes `'` by doubling), so the builders are safe regardless of what any present or future caller hands them. Real rule names contain no quote, so the emitted commands are byte-for-byte unchanged. **(2) `darling.json` was never ACL-hardened ([#1647]).** It holds every monitored server's `encryptedPassword`, the MCP bearer token, the web dashboard access token, and in BYO mode the store connection string - all DPAPI **LocalMachine** scope with an entropy constant published in this open-source repo, which means anything that can READ the file can unprotect the lot. That is by design; `DarlingFileSecurity` says so itself ("the file ACL is therefore the real access boundary"). The config just never got that ACL: every harden call site targeted the credential files under the data directory, while the config sat beside the binary, and the README's recommended install extracts the zip to `C:\PerformanceMonitorDarling` - a folder created directly under `C:\` inherits `BUILTIN\Users: Read & Execute` from the root DACL. Any local unprivileged user could read it, decrypt every SQL Server password, and lift the tokens that unlock the MCP write surface and the web dashboard - and, where the install directory was user-writable, plant the payload for (1). The service now hardens the resolved config path at startup with the same posture the admin/viewer credentials get (`allowInteractiveRead: true` - the Viewer and the CLI verbs run as the interactive operator and must still read it), `install-darling.ps1` applies the identical ACL right after service creation (which is when the `NT SERVICE` virtual account first has a SID to grant), and a file that is STILL readable by Users/Authenticated Users/Everyone after the attempt logs Critical rather than crashing a monitoring service over a permissions problem. The check keys on `ReadData` specifically, not the composite `Read` mask, so a metadata-only ACE does not cry wolf. **(3) Neither MCP host checked the `Host` header ([#1648]).** The web dashboard got this guard in [#1576], with the reasoning written into the code: a loopback surface is tokenless, so a browser ON the host that loads attacker content can be DNS-rebound to `127.0.0.1` and read and write the whole surface same-origin. That applies identically to the MCP host at :5152 in its default loopback configuration, which installs NO auth middleware at all - and that surface is no longer read-only, having gained custom-view CRUD, `add_servers`/`remove_server`, and alert-config writes. The `application/json` content type does not save it: under a rebind the browser treats the request as same-origin, so no CORS preflight applies, and `ModelContextProtocol.AspNetCore` does not add the check itself (per the MCP spec it is the application's job). The decision moves into a shared `HostHeaderGuard` in `PerformanceMonitor.Common` - the library both apps already reference - and is installed as the FIRST middleware, in BOTH bind modes, on Darling's MCP host and on Lite's, mirroring the web host. Lite is loopback-only, single-user, and holds no service-account privilege, so its exposure is informational; it is fixed in the same change anyway, because a guard that exists in one app's copy and not the other's is the drift this codebase keeps paying for. The web host's behavior is byte-for-byte unchanged (its forwarder and existing tests are untouched, which is the proof). **(4) Lite: an unescaped export path ([#1648]).** `DataImportService.FlushOldDatabase` built `COPY (...) TO '{path}'` by interpolating the import folder raw while every sibling COPY routes the same value through `DuckDbInitializer.EscapeSqlPath`. An apostrophe is legal in a Windows path (`C:\Users\O'Brien\...`), so it closed the SQL literal, the COPY failed to parse, and the per-table catch swallowed it as a warning - the import reported success having silently flushed nothing. Tested throughout, and the tests pin the things that actually regress: a non-CIDR `allowFrom` can never reach a firewall command (proven as a property of the canonical output, not by enumerating payloads); the builders keep any hostile value inside one balanced single-quoted literal; the Host guard's full accept/reject matrix in both modes; that the guard middleware is really installed FIRST and ahead of the network-only gates in all three hosts, parsed from the shipped sources, because #1648 was a WIRING omission that a pure-function test would have passed on the vulnerable build; the ACL check against real Windows DACLs; and the parquet export end-to-end under a directory whose name contains a quote.
 - **Lite + Darling: four things that grew forever - two tables and two log directories** ([#1651], [#1652]) - found by the retention/archival audit in the 2026-07 maintenance pass, which confirmed everything else in both apps' retention is correctly wired. These were the exceptions, and each one had grown unbounded since the day it shipped. **Lite's `dismissed_archive_alerts`** - the sidecar that remembers you dismissed an alert that has since been archived to parquet - was INSERT-only with no purge path anywhere in the repo, and it is deliberately in `ArchiveService.PreservedConfigTables`, so it is RESTORED after the 512 MB emergency reset that incidentally bounds everything else. It was the one table designed to survive every reset with nothing to ever shrink it. It now purges on the same daily tick as the analysis-findings cleanup, at a 180-day horizon keyed on `alert_time`. The horizon is deliberately generous rather than tight: suppressing an archived alert is the row's entire job, so purging one while its parquet is still readable would make an alert you already dismissed reappear - and parquet retention is 3 months measured on the FILE's date prefix, which is the archive date and therefore always NEWER than the rows inside it. It stays preserved through the reset; it just finally has an age limit. **Darling's `config.config_command`** - the imperative command queue the viewer, MCP tools and CLI enqueue into - had a service-side purge that the viewer's own code already ASSUMED existed ("the service-side purge is the backstop", `ViewerDataService.RunTestConnectAsync`) but which nothing had ever implemented. The only DELETE was viewer-side, per-command, wired into exactly four self-cleaning flows, and best-effort with the exception swallowed; every other command type - pause/resume, `snapshot_now`, `analyze_now`, `purge_now`, the enable/firewall verbs, collector toggles, anything issued from MCP or the CLI - left a terminal row and its `result_json` behind permanently, as did those four whenever the delete failed or the viewer died mid-poll. It now purges alongside the `collection_log` (60d) and `config_alert_log` (90d) sweeps at a 30-day horizon, using the same compressed-chunk-safe time-sliced DELETE. It is keyed on `created_at` (NOT NULL, so no row can slip the horizon by never being stamped) and filtered to terminal rows only, in the DELETE **and** in both `min()` subqueries - a slice anchored on an ineligible row would delete nothing and the drain loop's "an empty slice means we are done" termination would stop the purge early with older rows still there. A `pending` or `in_progress` row is never purged however old: deleting a live command strands whoever is polling it. The statement is schema-qualified deliberately - unlike `collection_log` and `config_alert_log` (created bare, so they live in `collect`), this table really is in `config`, and a bare name would have resolved to a nonexistent `collect.config_command` and failed every night into a warning nobody reads. **Lite's application log** rotated daily but never pruned - "daily rotation" only ever meant a new file per day - so an install kept every `lite_yyyyMMdd.log` it had ever written. This was a parity gap inside Lite itself: both sibling loggers already swept at 7 days, and this one now sweeps the same way, on the same window, scoped to its own file name so it cannot touch theirs. **Darling's service log** had the sweep and the 14-day window already, but called it only from the logger provider's constructor - so a service left up for months, which is the entire point of a service, swept exactly once at startup and then never again while writing a file a day forever. The sweep now also rides the worker's daily maintenance tick beside the retention purge. Not addressed here and tracked separately: `pg.log` still has no rotation, because fixing it moves where Postgres writes its log and both `ReadServerLogTail` and the startup failure-diagnostics path depend on the current location - it needs its own change and a live bring-up test, not a drive-by.
 - **Lite: the Excluded Databases picker throws a raw firewall error on an Azure database-level-firewall server** ([#1642]) - the picker lists databases by connecting to `master` and reading `sys.databases` with no fallback, so the exact setup #1631/#1634 fixed for collection - an Azure server blocked at the server-level firewall but reachable through a database-level rule (error 40615) - still hit a bare "Cannot open server ... is not allowed to access the server" error box here. It now catches the fallback-eligible error via the shared `SqlErrorClassification.ShouldFallBackToSingleDatabase` (the same classifier the collectors use) and falls back to the connected database with an explanatory status note instead of failing outright. Lite-only: the Darling viewer reads its database inventory from the Postgres store and never opens master.
 - **Darling: a bring-your-own-Postgres `viewer` seat gets `permission denied` on the Manage Servers list and the Settings notification prefill** ([#1639]) - the fail-closed viewer column ACL (#1262) is authored ONCE in C# (`DarlingManagedRoles.ViewerRestrictedConfigTables`) and applied two ways: managed mode GENERATES the `REVOKE`-then-`GRANT SELECT (columns)` carve from that list on every service start, so it is correct by construction, while BYO mode has no service to generate anything - the operator runs `Darling/tools/provision-roles.sql` by hand, and that file's three `GRANT SELECT (...)` lists are a HAND COPY of the same columns. Nothing pinned the two together, and they drifted across three releases: `config_notification.generic_body_template` / `generic_proxy` arrived with the generic webhook channel ([#1506]) and `config_monitored_servers.alert_delivery_mode_override` was added to the C# list by [#1551] - where the gated-live ratchet caught the MANAGED half of exactly this bug and the BYO hand mirror was the half that got missed - and in neither case did the .sql follow. Because the carve is fail-CLOSED by design - `viewer`'s table-wide SELECT is revoked and only the enumerated columns re-granted - an unlisted column is INVISIBLE, so on a BYO store a read-only seat took a bare SQLSTATE `42501` on the two projections written specifically FOR it: `MonitoredServersSelectSql` (the Manage Servers list, which also drives the sidebar reconcile) and `NotificationSelectNoSecretSql` (the Settings prefill, where one such throw blanks every later section). Quiet in exactly the wrong direction - the columns exist, the script runs clean, `psql` as the owner sees everything, and only a read from the least-privilege role ever notices. Managed deployments were never affected. The three lists now match the C# source of truth exactly (nothing else was added - this closes existing drift, it does not widen the grant), and the real defect - a hand mirror with no guard - is closed by a new UNGATED `ProvisionRolesAclDriftTests`: it parses the SHIPPED .sql (copied beside the test binary, so there is no second copy to go stale and no repo path to hardcode), extracts every `GRANT SELECT (columns) ON schema.table TO role`, and asserts set-equality against the C# list in BOTH directions, plus the paired `REVOKE` and its ordering (without the REVOKE first the carve is a no-op and the secrets stay readable), that no secret column appears in any grant, and that the script still provisions admin+viewer only - never a half-provisioned `mcp` role, which BYO deliberately does not create. The guard is itself guarded: a meta-test mutates the real file three ways (drop a column, add one, delete a whole carve) and asserts the same comparison reports each, so a parser that silently matched nothing can never pass as "no drift". Verified by hand as well - removing either restored column reintroduces a failing test.
@@ -561,6 +562,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 [#1639]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1639
 [#1640]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1640
 [#1642]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1642
+[#1646]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1646
+[#1647]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1647
+[#1648]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1648
 [#1651]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1651
 [#1652]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1652
 [#1617]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1617

Darling/Darling.Tests/Darling.Tests.csproj

@@ -41,5 +41,12 @@
          AppContext.BaseDirectory, with no repo-root path walking. Same Link+copy delivery the Service
          project uses for tools\install-darling.ps1. -->
     <None Include="..\tools\provision-roles.sql" Link="Fixtures\provision-roles.sql" CopyToOutputDirectory="PreserveNewest" />
+    <!-- The two Darling Kestrel hosts' sources, copied beside the test binary (same Link+copy delivery as the
+         provisioning script above — the files live outside this project directory, so the default Compile glob
+         never sees them). HostHeaderGuardTests parses them to pin that the #1648 DNS-rebinding middleware is
+         installed FIRST and in BOTH bind modes: the defect was a WIRING omission, not a logic bug, so the pure
+         decision test alone would have passed on the broken build. -->
+    <None Include="..\PerformanceMonitor.Darling.Service\Mcp\DarlingMcpHostService.cs" Link="Fixtures\DarlingMcpHostService.cs" CopyToOutputDirectory="PreserveNewest" />
+    <None Include="..\PerformanceMonitor.Darling.Service\Mcp\DarlingWebHostService.cs" Link="Fixtures\DarlingWebHostService.cs" CopyToOutputDirectory="PreserveNewest" />
   </ItemGroup>
 </Project>