diff --git a/Darling/Darling.Tests/DarlingMcpAlertToolsTests.cs b/Darling/Darling.Tests/DarlingMcpAlertToolsTests.cs index 88a7b1f9d..ee5fe57df 100644 --- a/Darling/Darling.Tests/DarlingMcpAlertToolsTests.cs +++ b/Darling/Darling.Tests/DarlingMcpAlertToolsTests.cs @@ -245,11 +245,17 @@ public void AlertSettingsSelect_ColumnCount_MatchesTheOrdinalsRead() /// UPDATE — and it would need an exemption entry, which is the same hand-maintained fact that let six /// columns go missing in the first place. If a genuinely read-only column ever arrives, exempt it BY NAME /// here with its reason; do not loosen the equality. + /// + /// #3314 made the tool span TWO config tables, so the equality is held PER TABLE and each read's + /// own SELECT list is the expectation for its own plane. A single equality over the union would be + /// satisfiable by compensating drift — a column dropped from one table's write set and a stray added to + /// the other's would net to the same set — and it is the weaker check precisely where the new plane is + /// thinnest. /// [Fact] public void EveryColumnRead_IsEmittedByThePayload_AndAcceptedByTheWriter() { - var (columns, error) = ParseAsPartialUpdate(SerializedSettingsPayload(SampleSettingsRow())); + var (targets, error) = ParseAsPartialUpdate(SerializedSettingsPayload(SampleSettingsRow())); /* Every key the payload emits is accepted. The parse stops at the FIRST rejection, so a non-null error here names the exact key update_alert_settings would refuse from its own read. */ @@ -257,11 +263,158 @@ error here names the exact key update_alert_settings would refuse from its own r /* Each accepted key claimed its own column — two keys sharing one would make whichever lost the parse order a silent no-op, with the caller told both were updated. */ - Assert.Equal(columns.Count, columns.Distinct(StringComparer.Ordinal).Count()); + Assert.Equal(targets.Count, targets.Distinct().Count()); + + /* Every table the writer can reach is compared, driven off the tool's own list — so a third plane + added without a read to match it fails here instead of going uncompared. */ + foreach (var table in DarlingMcpAlertTools.WritableTables) + { + Assert.Equal( + SelectedColumnsOf(table).OrderBy(c => c, StringComparer.Ordinal).ToArray(), + ColumnsFor(targets, table).OrderBy(c => c, StringComparer.Ordinal).ToArray()); + } + + /* And the writer reaches no table outside that list — the direction the loop above cannot see. */ + Assert.Empty(targets.Select(t => t.Table).Except(DarlingMcpAlertTools.WritableTables, StringComparer.Ordinal)); + } + + /// + /// #3314: updated_fields reports BARE column names across two tables, which is only unambiguous + /// while no writable column name appears on both. Qualifying them instead would have redefined every + /// existing entry of a consumer-visible array, so the uniqueness is the thing being relied on — asserted + /// here rather than assumed, and it is the assertion that fails on the day a second table grows a + /// same-named column. + /// + [Fact] + public void WritableColumnNames_DoNotCollideAcrossTheTwoTables() + { + var byTable = DarlingMcpAlertTools.WritableTables + .Select(t => SelectedColumnsOf(t).ToArray()) + .ToArray(); + + /* A count comparison is satisfied by two empty sets, and an emptied SELECT list is exactly the + accident that would produce them -- so each plane is asserted non-empty first. */ + Assert.Equal(DarlingMcpAlertTools.WritableTables.Length, byTable.Length); + Assert.All(byTable, columns => Assert.NotEmpty(columns)); + + Assert.Equal( + byTable.Sum(columns => columns.Length), + byTable.SelectMany(columns => columns).Distinct(StringComparer.Ordinal).Count()); + } + /// + /// #3314 round two: the two config tables are read under ONE snapshot, and the ISOLATION LEVEL is the + /// whole mechanism. PostgreSQL takes a fresh snapshot per statement under READ COMMITTED, so wrapping + /// the two SELECTs in a default transaction reads exactly like a fix and changes nothing — this is the + /// one line whose being wrong is invisible to every behavioural test that does not race a writer. + /// + /// The split itself cannot come back by accident: the two single-table reads are private and take + /// the combined method's connection and transaction, so calling one alone does not compile. That is why + /// this test pins the LEVEL and the entry point rather than counting call sites — the compiler already + /// holds the part a test would be redundant for, and the level is the part it cannot. + /// + [Fact] + public void TheTwoConfigTables_AreReadUnderOneRepeatableReadSnapshot() + { + var reader = ReadRepoFile(System.IO.Path.Combine( + "Darling", "PerformanceMonitor.Darling.Service", "Mcp", "DarlingAlertReader.cs")); + var tools = ReadRepoFile(System.IO.Path.Combine( + "Darling", "PerformanceMonitor.Darling.Service", "Mcp", "DarlingMcpAlertTools.cs")); + + Assert.Contains("System.Data.IsolationLevel.RepeatableRead", reader, StringComparison.Ordinal); + Assert.DoesNotContain("IsolationLevel.ReadCommitted", reader, StringComparison.Ordinal); + + /* The single-table reads are private, so the split cannot be reintroduced -- asserted so that + widening either back to public is a decision someone makes here rather than a quiet edit. */ + Assert.Contains("private static async Task ReadAlertSettingsAsync", reader, StringComparison.Ordinal); + Assert.Contains("private static async Task ReadDeliveryCooldownAsync", reader, StringComparison.Ordinal); + + /* And both tool paths go through the combined entry point -- get_alert_settings and the post-write + re-read, which is the one described to the caller as the authoritative merged state. */ + Assert.Equal(2, System.Text.RegularExpressions.Regex.Matches( + tools, @"GetAlertConfigurationAsync\(postgres\)").Count); + } + + /// + /// #3314: the delivery cooldown is reachable through the control plane under a CHANNEL-NEUTRAL name, and + /// the stored name still works. The whole defect was that the only throttle on a Slack / Teams / + /// PagerDuty / generic-webhook post was named for email, lived in the SMTP config block, and could not be + /// read or written by any MCP tool — so on a headless box with no SMTP at all the sole path to it was a + /// desktop app. + /// + /// Both spellings are asserted to reach the SAME column, and sending BOTH in one body is asserted + /// to be REFUSED. Two SET clauses for one column is a Postgres error, so without the guard the failure + /// would surface as a dialect message naming neither key the caller sent; and were the duplicate ever + /// tolerated instead, one of the two values would win silently while the caller was told both applied. + /// + /// The canonical name is also asserted to be the ONLY one the read emits. An alias that round-trips + /// is an alias that becomes a second name for the same setting on the wire, which is its own defect for a + /// client diffing a read against a write. + /// + [Fact] + public void DeliveryCooldown_IsWritableUnderBothNames_ButEmittedUnderOnlyOne() + { + var canonical = ParseAsPartialUpdate((JsonObject)JsonNode.Parse( + "{\"delivery\":{\"cooldown_minutes\":45}}")!); + Assert.Null(canonical.Error); Assert.Equal( - SelectedAlertSettingsColumns().OrderBy(c => c, StringComparer.Ordinal).ToArray(), - columns.OrderBy(c => c, StringComparer.Ordinal).ToArray()); + new[] { (DarlingMcpAlertTools.NotificationTable, DarlingMcpAlertTools.DeliveryCooldownColumn) }, + canonical.Targets.ToArray()); + + var alias = ParseAsPartialUpdate((JsonObject)JsonNode.Parse( + "{\"email_cooldown_minutes\":45}")!); + Assert.Null(alias.Error); + Assert.Equal(canonical.Targets.ToArray(), alias.Targets.ToArray()); + + /* Both spellings at once: refused, and the message names both so the caller knows which to drop. */ + var both = ParseAsPartialUpdate((JsonObject)JsonNode.Parse( + "{\"email_cooldown_minutes\":45,\"delivery\":{\"cooldown_minutes\":45}}")!); + Assert.NotNull(both.Error); + Assert.Contains("delivery.cooldown_minutes", both.Error!, StringComparison.Ordinal); + Assert.Contains(DarlingMcpAlertTools.DeliveryCooldownColumn, both.Error!, StringComparison.Ordinal); + + /* The read emits the channel-neutral name and NOT the stored alias. */ + var payload = SerializedSettingsPayload(SampleSettingsRow(), deliveryCooldownMinutes: 45); + Assert.Equal(45, payload["delivery"]!["cooldown_minutes"]!.GetValue()); + Assert.DoesNotContain(DarlingMcpAlertTools.DeliveryCooldownColumn, payload.Select(kv => kv.Key)); + } + + /// + /// #3314: the delivery cooldown's write bound is DarlingAlertSettings' clamp EXACTLY — the same + /// parity and + /// hold, and for the same + /// reason: a wider bound lets the tool ACCEPT a value the engine silently rewrites on read, which + /// presents to the operator as the setting not sticking, with nothing saying no. + /// + /// This is what makes the 120-minute ceiling an ENGINE decision rather than a bound edit. Raising + /// it here alone would reintroduce exactly that class of bug; raising it properly means moving the clamp + /// in both SKUs. The ceiling stayed: the cooldown is one global number applied to every fingerprint on + /// every server, so stretching it to silence ONE recurring signature silences everything else at the same + /// cadence — and a mute rule does that job scoped, expiring, and disclosed by get_mute_rules, where a + /// multi-hour cooldown suppresses posts that no tool reports. + /// + [Theory] + [InlineData(0, false)] + [InlineData(1, true)] + [InlineData(120, true)] + [InlineData(121, false)] + public void DeliveryCooldownWriteBounds_MatchTheEngineClamp(int minutes, bool accepted) + { + foreach (var body in new[] + { + $"{{\"delivery\":{{\"cooldown_minutes\":{minutes}}}}}", + $"{{\"email_cooldown_minutes\":{minutes}}}", + }) + { + var parsed = ParseAsPartialUpdate((JsonObject)JsonNode.Parse(body)!); + Assert.Equal(accepted, parsed.Error is null); + Assert.Equal(accepted ? 1 : 0, parsed.Targets.Count); + } + + /* The engine's own clamp, so the numbers above are not a second opinion about the range. */ + var settings = ReadRepoFile(System.IO.Path.Combine( + "Darling", "PerformanceMonitor.Darling.Service", "DarlingAlertSettings.cs")); + Assert.Contains("Math.Clamp(_config.Smtp.EmailCooldownMinutes, 1, 120)", settings, StringComparison.Ordinal); } /// @@ -309,11 +462,24 @@ public void AgConnectionAndBlockingWaitWriteBounds_MatchTheEngineClamps() /// The reader's SELECT list, split from the SHIPPED constant the same way /// counts it — so it cannot drift /// from what the reader actually asks the store for. - private static IReadOnlyList SelectedAlertSettingsColumns() + private static IReadOnlyList SelectedAlertSettingsColumns() => + SelectedColumnsOf(DarlingMcpAlertTools.AlertSettingsTable); + + /// The SELECT list of whichever SHIPPED read constant serves , split the + /// same way. Mapped from the table name rather than taking the SQL as a parameter so a caller iterating + /// WritableTables cannot silently compare a plane against the wrong read — an unmapped table + /// throws here instead of being skipped. + private static IReadOnlyList SelectedColumnsOf(string table) { - var sql = Reader.AlertSettingsSelectSql; + var sql = table switch + { + DarlingMcpAlertTools.AlertSettingsTable => Reader.AlertSettingsSelectSql, + DarlingMcpAlertTools.NotificationTable => Reader.DeliveryCooldownSelectSql, + _ => throw new ArgumentOutOfRangeException(nameof(table), table, "No MCP read constant is mapped to this table."), + }; + var select = sql[(sql.IndexOf("SELECT", StringComparison.Ordinal) + 6).. - sql.IndexOf("FROM config_alert_settings", StringComparison.Ordinal)]; + sql.IndexOf("FROM " + table, StringComparison.Ordinal)]; return select.Split(',', StringSplitOptions.RemoveEmptyEntries) .Select(c => c.Trim()) .Where(c => c.Length > 0) @@ -324,27 +490,39 @@ private static IReadOnlyList SelectedAlertSettingsColumns() /// serializes with and re-parsed. Runtime rather than source-parsing for the reason Lite's /// McpAlertSettingsKeyTests gives: the C# identifier is not automatically the wire key, so only /// serializing proves what a client receives — and therefore what it would hand back. - private static JsonObject SerializedSettingsPayload(Reader.AlertSettingsReadRow row) + private static JsonObject SerializedSettingsPayload(Reader.AlertSettingsReadRow row, int deliveryCooldownMinutes = 15) { var build = typeof(DarlingMcpAlertTools).GetMethod( "BuildAlertSettingsPayload", BindingFlags.NonPublic | BindingFlags.Static)!; - var payload = build.Invoke(null, new object[] { row })!; + var payload = build.Invoke(null, new object[] { row, deliveryCooldownMinutes })!; var json = JsonSerializer.Serialize(payload, payload.GetType(), McpHelpers.JsonOptions); return (JsonObject)JsonNode.Parse(json)!; } - /// Runs a body through the tool's REAL partial-update parser and reports the columns it would - /// write plus the first validation error, if any. - private static (IReadOnlyList Columns, string? Error) ParseAsPartialUpdate(JsonObject body) + /// Runs a body through the tool's REAL partial-update parser and reports the (table, column) + /// pairs it would write plus the first validation error, if any. Reflected over the UpdateTarget record's + /// properties rather than cast to a tuple shape, so adding a field to it does not silently change what + /// this reads. + private static (IReadOnlyList<(string Table, string Column)> Targets, string? Error) ParseAsPartialUpdate(JsonObject body) { var build = typeof(DarlingMcpAlertTools).GetMethod( "BuildAlertSettingsUpdate", BindingFlags.NonPublic | BindingFlags.Static)!; var result = build.Invoke(null, new object[] { body })!; var type = result.GetType(); - var updates = (IEnumerable<(string Column, NpgsqlParameter Param)>)type.GetField("Item1")!.GetValue(result)!; - return (updates.Select(u => u.Column).ToList(), (string?)type.GetField("Item2")!.GetValue(result)); + var updates = ((System.Collections.IEnumerable)type.GetField("Item1")!.GetValue(result)!).Cast().ToList(); + var targets = updates.Select(u => + { + var t = u.GetType(); + return ((string)t.GetProperty("Table")!.GetValue(u)!, (string)t.GetProperty("Column")!.GetValue(u)!); + }).ToList(); + return (targets, (string?)type.GetField("Item2")!.GetValue(result)); } + /// The columns the parser would write to one table. + private static IReadOnlyList ColumnsFor( + IReadOnlyList<(string Table, string Column)> targets, string table) => + targets.Where(t => t.Table == table).Select(t => t.Column).ToList(); + /// A plausible settings row whose every value sits INSIDE the writer's bounds, so the invariant /// above fails on a missing or unaccepted KEY rather than on a value. Named arguments deliberately: a new /// column makes this stop compiling until someone supplies it, which is the moment to decide whether the @@ -545,9 +723,14 @@ await DarlingMcpTestData.ExecAsync(connection, ct, VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11)", when, ServerId, ServerName, "High CPU", 92.5, 80.0, true, "email", null, false, "CPU sustained above threshold"); - /* Seed the single global settings row — every column has a default, so id alone suffices. */ + /* Seed the single global settings row — every column has a default, so id alone suffices. + BOTH singletons, because #3314 made get_alert_settings read the delivery cooldown off + config_notification: the service seeds the two in one pass, and the tool reports `unavailable` + rather than a fabricated default when either is missing. */ await DarlingMcpTestData.ExecAsync(connection, ct, "INSERT INTO config_alert_settings (id) VALUES (1) ON CONFLICT (id) DO NOTHING"); + await DarlingMcpTestData.ExecAsync(connection, ct, + "INSERT INTO config_notification (id) VALUES (1) ON CONFLICT (id) DO NOTHING"); await DarlingMcpTestData.ExecAsync(connection, ct, @"INSERT INTO config_mute_rules (id, enabled, created_at_utc, expires_at_utc, reason, server_name, metric_name, database_pattern, query_text_pattern, wait_type_pattern, job_name_pattern) @@ -599,8 +782,14 @@ public async Task AlertWriteTools_TuneSettings_AndMuteRuleRoundTrip_AgainstDevPo /* Seed the two singleton rows the writes touch (a no-op if they already exist on a shared store). */ await DarlingMcpTestData.ExecAsync(connection, ct, "INSERT INTO config_service (id) VALUES (1) ON CONFLICT (id) DO NOTHING"); await DarlingMcpTestData.ExecAsync(connection, ct, "INSERT INTO config_alert_settings (id) VALUES (1) ON CONFLICT (id) DO NOTHING"); + await DarlingMcpTestData.ExecAsync(connection, ct, "INSERT INTO config_notification (id) VALUES (1) ON CONFLICT (id) DO NOTHING"); var originalThreshold = Convert.ToInt32(await ScalarAsync(connection, ct, "SELECT cpu_threshold_percent FROM config_alert_settings WHERE id = 1")); + /* #3314: captured for the same reason as the threshold — these two are SINGLETONS the whole store + shares, and the delivery cooldown now governs channel volume for every later test and every later + run on a reused database. */ + var originalFireCooldown = Convert.ToInt32(await ScalarAsync(connection, ct, "SELECT cooldown_minutes FROM config_alert_settings WHERE id = 1")); + var originalDeliveryCooldown = Convert.ToInt32(await ScalarAsync(connection, ct, "SELECT email_cooldown_minutes FROM config_notification WHERE id = 1")); var versionBefore = Convert.ToInt64(await ScalarAsync(connection, ct, "SELECT config_version FROM config_service WHERE id = 1")); var newThreshold = originalThreshold == 91 ? 71 : 91; // a distinct, in-range value var muteTag = "mcp_alert_write_e2e_" + Guid.NewGuid().ToString("N"); // own-scoped cleanup tag @@ -623,6 +812,46 @@ public async Task AlertWriteTools_TuneSettings_AndMuteRuleRoundTrip_AgainstDevPo var versionAfter = Convert.ToInt64(await ScalarAsync(connection, ct, "SELECT config_version FROM config_service WHERE id = 1")); Assert.True(versionAfter > versionBefore, "config_version should self-bump on a config_alert_settings write"); + /* #3314: the delivery cooldown round-trips THROUGH THE STORE, and through the OTHER table. The + shape pins prove the parser routes it; only this proves the two-table write executes, that the + value lands in config_notification, and that a read straight afterwards reports what was + written. Both spellings are exercised, since the alias exists so an existing config keeps + working and an alias nobody writes with is an alias nobody has tested. */ + foreach (var (body, expected) in new[] + { + ("{\"delivery\":{\"cooldown_minutes\":37}}", 37), + ("{\"email_cooldown_minutes\":41}", 41), + }) + { + var cooldown = await DarlingMcpAlertTools.UpdateAlertSettings(postgres, body); + Assert.Equal("updated", DarlingMcpTestData.StatusOf(cooldown)); + using var doc = JsonDocument.Parse(cooldown); + Assert.Equal(expected, doc.RootElement.GetProperty("settings").GetProperty("delivery").GetProperty("cooldown_minutes").GetInt32()); + Assert.Contains("email_cooldown_minutes", doc.RootElement.GetProperty("updated_fields").EnumerateArray().Select(e => e.GetString())); + Assert.Equal(expected, Convert.ToInt32(await ScalarAsync(connection, ct, "SELECT email_cooldown_minutes FROM config_notification WHERE id = 1"))); + } + + /* One body spanning BOTH tables: two UPDATE statements in one transaction, and both land. A + partial application is the failure this transaction exists to prevent, and a same-table-only + body could never expose it. */ + var spanning = await DarlingMcpAlertTools.UpdateAlertSettings( + postgres, $"{{\"cooldown_minutes\":7,\"delivery\":{{\"cooldown_minutes\":53}}}}"); + Assert.Equal("updated", DarlingMcpTestData.StatusOf(spanning)); + Assert.Equal(7, Convert.ToInt32(await ScalarAsync(connection, ct, "SELECT cooldown_minutes FROM config_alert_settings WHERE id = 1"))); + Assert.Equal(53, Convert.ToInt32(await ScalarAsync(connection, ct, "SELECT email_cooldown_minutes FROM config_notification WHERE id = 1"))); + + /* Out of range on either spelling, and both spellings at once, write NOTHING. */ + foreach (var rejected in new[] + { + "{\"delivery\":{\"cooldown_minutes\":121}}", + "{\"email_cooldown_minutes\":0}", + "{\"email_cooldown_minutes\":60,\"delivery\":{\"cooldown_minutes\":60}}", + }) + { + Assert.Equal("invalid", DarlingMcpTestData.StatusOf(await DarlingMcpAlertTools.UpdateAlertSettings(postgres, rejected))); + Assert.Equal(53, Convert.ToInt32(await ScalarAsync(connection, ct, "SELECT email_cooldown_minutes FROM config_notification WHERE id = 1"))); + } + /* An unknown field writes NOTHING (validated before the UPDATE). */ Assert.Equal("invalid", DarlingMcpTestData.StatusOf(await DarlingMcpAlertTools.UpdateAlertSettings(postgres, "{\"cpu\":{\"bogus\":1}}"))); @@ -655,7 +884,8 @@ The RESTORE is why this teardown matters more than most (#1902): config_alert_se run on a reused database — reading a CPU threshold this test invented. */ await LiveStoreCleanup.RunAsync(cs!, bodySucceeded, async (cleanup, cleanupCt) => { - await DarlingMcpTestData.ExecAsync(cleanup, cleanupCt, "UPDATE config_alert_settings SET cpu_threshold_percent = $1 WHERE id = 1", originalThreshold); + await DarlingMcpTestData.ExecAsync(cleanup, cleanupCt, "UPDATE config_alert_settings SET cpu_threshold_percent = $1, cooldown_minutes = $2 WHERE id = 1", originalThreshold, originalFireCooldown); + await DarlingMcpTestData.ExecAsync(cleanup, cleanupCt, "UPDATE config_notification SET email_cooldown_minutes = $1 WHERE id = 1", originalDeliveryCooldown); await DarlingMcpTestData.ExecAsync(cleanup, cleanupCt, "DELETE FROM config_mute_rules WHERE reason = $1", muteTag); }); } diff --git a/Darling/Darling.Tests/DarlingSecuritySplitLiveTests.cs b/Darling/Darling.Tests/DarlingSecuritySplitLiveTests.cs index 36a1f5949..910db9c30 100644 --- a/Darling/Darling.Tests/DarlingSecuritySplitLiveTests.cs +++ b/Darling/Darling.Tests/DarlingSecuritySplitLiveTests.cs @@ -361,6 +361,24 @@ await ExecAsync(mcp, await ExecAsync(owner, "INSERT INTO config.config_alert_settings (id) VALUES (1) ON CONFLICT (id) DO NOTHING", ct); await ExecAsync(mcp, "UPDATE config.config_alert_settings SET enabled = enabled WHERE id = 1", ct); + /* #3314: the delivery cooldown, on config_notification rather than config_alert_settings, so + update_alert_settings spans two tables. This is the one live check that separates the two ways + the write can 42501 — a missing column SELECT and a missing column UPDATE raise the IDENTICAL + "permission denied for table config_notification", so a single passing write attempt cannot + attribute itself. The read half is asserted by the secret-column loop above (which SELECTs the + whole non-secret set, email_cooldown_minutes included); this is the write half, and the + sibling-column denials below are what prove the grant is really one column wide rather than + the table-wide write it would be easiest to reach for. */ + await ExecAsync(owner, "INSERT INTO config.config_notification (id) VALUES (1) ON CONFLICT (id) DO NOTHING", ct); + await ExecAsync(mcp, "UPDATE config.config_notification SET email_cooldown_minutes = email_cooldown_minutes WHERE id = 1", ct); + + foreach (var sibling in new[] { "smtp_encrypted_password = 'x'", "slack_url = 'x'", "smtp_host = 'x'" }) + { + var siblingDenied = await Assert.ThrowsAsync(async () => + await ExecAsync(mcp, $"UPDATE config.config_notification SET {sibling} WHERE id = 1", ct)); + Assert.Equal("42501", siblingDenied.SqlState); + } + /* But mcp is STILL denied a write to a config table it was NOT granted (42501) — config_command is the service-credential pivot; the alert-tuning grants did not widen into a schema-wide config write. */ var stillDenied = await Assert.ThrowsAsync(async () => @@ -546,6 +564,10 @@ IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = '{McpRole}') THEN GRANT INSERT, UPDATE, DELETE ON config.config_mute_rules TO {McpRole}; GRANT UPDATE ON config.config_alert_settings TO {McpRole}; GRANT UPDATE (config_version, updated_at) ON config.config_service TO {McpRole}; +-- #3314: the DELIVERY cooldown, the one alert knob stored on config_notification. COLUMN-level, because that +-- table holds the SMTP password blob and the Teams/Slack/generic webhook URLs and the PagerDuty routing key. +-- Mirrors DarlingManagedRoles section 8. +GRANT UPDATE (email_cooldown_minutes) ON config.config_notification TO {McpRole}; -- The mcp server-onboarding writes (add_servers / remove_server): CRUD on the single config_monitored_servers -- table. Mirrors DarlingManagedRoles section 9. The beacon is already covered by the config_service column grant -- above (a config_monitored_servers write fires the same SECURITY-INVOKER bump trigger). diff --git a/Darling/Darling.Tests/McpConfigReadAvoidsSecretColumnsTests.cs b/Darling/Darling.Tests/McpConfigReadAvoidsSecretColumnsTests.cs index 58564c399..ae0584d4f 100644 --- a/Darling/Darling.Tests/McpConfigReadAvoidsSecretColumnsTests.cs +++ b/Darling/Darling.Tests/McpConfigReadAvoidsSecretColumnsTests.cs @@ -112,6 +112,49 @@ public void TheNotificationReadStillNamesCarvedSecretColumns() Assert.NotEmpty(namedSecrets); } + /// + /// #3314 put a config_notification read back on the MCP surface — the delivery cooldown, the one + /// alert-engine knob stored on that table — so the carve now has to hold for a read that DOES ship, + /// rather than by the whole read having been removed. + /// + /// The mirror image of , and it + /// fails in the costly direction: that one asserts the PRIVILEGED read still names secrets (so the skip + /// stays justified), this one asserts the MCP read names NONE. Column-level denial answers for the whole + /// TABLE, so a single carved column added to this SELECT does not degrade the read — it 42501s the entire + /// call, and that is the #2293 failure, where skipping one denied row simply moved the error to the next. + /// Every non-secret column is derived from the ACL rather than listed, so a column reclassified as secret + /// makes THIS fail on the day of the reclassification instead of on the next deployment. + /// + /// The read is also asserted to stay narrow: the value the tool needs is one column, and + /// SELECT * — or a convenience widening to "the non-secret columns" — is denied outright by the + /// carve for the star and is a pointless secret-adjacent read for the rest. + /// + [Fact] + public void TheMcpDeliveryCooldownReadNamesNoCarvedSecretColumn() + { + var sql = PerformanceMonitor.Darling.Service.Mcp.DarlingAlertReader.DeliveryCooldownSelectSql; + var acl = DarlingManagedRoles.ViewerRestrictedConfigTables + .Single(t => string.Equals(t.Table, "config_notification", StringComparison.Ordinal)); + + Assert.Contains("FROM config_notification", sql, StringComparison.Ordinal); + Assert.DoesNotContain("*", sql, StringComparison.Ordinal); + + var named = acl.SecretColumns.Where(c => sql.Contains(c, StringComparison.Ordinal)).ToArray(); + Assert.Empty(named); + + /* And it really does name the one non-secret column it needs -- without this the assertions above + are satisfied by a SELECT that reads nothing from the table at all. */ + var selected = sql[(sql.IndexOf("SELECT", StringComparison.Ordinal) + 6).. + sql.IndexOf("FROM config_notification", StringComparison.Ordinal)] + .Split(',', StringSplitOptions.RemoveEmptyEntries) + .Select(c => c.Trim()) + .Where(c => c.Length > 0) + .ToArray(); + + Assert.Equal(new[] { "email_cooldown_minutes" }, selected); + Assert.Contains("email_cooldown_minutes", acl.NonSecretColumns); + } + private static string RepoRoot([CallerFilePath] string thisFile = "") { var dir = Path.GetDirectoryName(thisFile)!; diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingManagedRoles.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingManagedRoles.cs index 3306c10e9..48dfbe97e 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingManagedRoles.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingManagedRoles.cs @@ -41,7 +41,8 @@ namespace PerformanceMonitor.Darling.Service; /// INSERT on collect.analysis_findings and config.analysis_muted (what analyze_server /// persists + the mute tool need), INSERT/UPDATE/DELETE on config.custom_views (the /// custom-view tools, #1599), the alert-tuning writes (INSERT/UPDATE/DELETE on -/// config.config_mute_rules + UPDATE on the singleton config.config_alert_settings, plus the +/// config.config_mute_rules + UPDATE on the singleton config.config_alert_settings + UPDATE on +/// the single non-secret email_cooldown_minutes column of config.config_notification, plus the /// two beacon columns of config.config_service so the settings write's self-bump trigger can fire), /// and the server-onboarding writes (INSERT/UPDATE/DELETE on config.config_monitored_servers for the /// add_servers/remove_server tools — a single non-secret-KEY table; the credential column stays @@ -243,7 +244,7 @@ public static async Task EnsureProvisionedAsync( await command.ExecuteNonQueryAsync(cancellationToken); logger.LogInformation( - "Least-privilege roles ready (admin: read both schemas + write config; viewer: read-only + write config.custom_views; mcp: viewer's reads + INSERT on analysis_findings/analysis_muted + write config.custom_views + tune alerting (config_mute_rules, config_alert_settings, config_service reload beacon) + onboard servers (config_monitored_servers)) — the Viewer and MCP host no longer connect as the superuser"); + "Least-privilege roles ready (admin: read both schemas + write config; viewer: read-only + write config.custom_views; mcp: viewer's reads + INSERT on analysis_findings/analysis_muted + write config.custom_views + tune alerting (config_mute_rules, config_alert_settings, config_notification.email_cooldown_minutes, config_service reload beacon) + onboard servers (config_monitored_servers)) — the Viewer and MCP host no longer connect as the superuser"); /* CLAMPED, not raw: the batch above wrote the clamped form, so returning the raw read would hand the caller a baseline that differs from what the roles actually carry (a stored 0 provisions '15s'). @@ -704,8 +705,9 @@ ALTER DEFAULT PRIVILEGES FOR ROLE {owner} IN SCHEMA {config} -- / create_mute_rule / delete_mute_rule let a token-holder tune the SAME alert engine the Viewer's Settings -- window drives: INSERT/UPDATE/DELETE on config_mute_rules (the mute rules the delivery paths honor) and UPDATE -- on the SINGLETON config_alert_settings row (id=1 -- UPDATE only, never INSERT/DELETE: the row is a fixed --- singleton the service seeds). Still NARROW -- never the config_command service-credential pivot, the --- monitored-servers/notification secret tables, or a schema-wide config write. +-- singleton the service seeds). Still NARROW -- never the config_command service-credential pivot, a +-- schema-wide config write, or any SECRET column: the one config_notification write is a single column +-- (see #3314 below), and the monitored-servers credential column stays SELECT-carved. -- The beacon caveat: a config_alert_settings write fires the existing statement-level bump trigger -- (trg_bump_alert_settings -> config_bump_version), which UPDATEs config_service.config_version AS THE CURRENT -- ROLE (the trigger function is SECURITY INVOKER). So mcp ALSO needs UPDATE on JUST the two beacon columns of @@ -722,6 +724,20 @@ ALTER DEFAULT PRIVILEGES FOR ROLE {owner} IN SCHEMA {config} GRANT INSERT, UPDATE, DELETE ON {config}.config_mute_rules TO {mcp}; GRANT UPDATE ON {config}.config_alert_settings TO {mcp}; GRANT UPDATE (config_version, updated_at) ON {config}.config_service TO {mcp}; +-- #3314: the DELIVERY cooldown -- the sole throttle on a Slack/Teams/PagerDuty/webhook post -- is the one +-- alert-engine knob stored on config_notification rather than config_alert_settings, so update_alert_settings +-- spans two tables and needs a write here. This DOES widen mcp into a table holding bearer secrets (the SMTP +-- password blob, the Teams/Slack/generic webhook URLs, the PagerDuty routing key), so the grant is +-- COLUMN-level on exactly that one column -- the same shape as the config_service beacon grant above and for +-- the same reason. MEASURED, not assumed: as mcp, the baseline UPDATE raises 42501; with this grant it +-- succeeds; with the column SELECT revoked and this grant kept it STILL succeeds (so UPDATE is the privilege +-- doing the work, not an ambient SELECT); and a write to smtp_encrypted_password, slack_url or even the +-- non-secret sibling smtp_host stays 42501. A missing SELECT and a missing UPDATE both raise the identical +-- 42501 permission-denied-for-table-config_notification message, so only isolating the grants separates them. +-- The READ side needs nothing: email_cooldown_minutes is already in the section-6 non-secret column carve. +-- The BEACON is already covered: config_notification carries trg_bump_notification -> config_bump_version +-- (SECURITY INVOKER), which UPDATEs config_service.config_version AS mcp, and the column grant above serves it. +GRANT UPDATE (email_cooldown_minutes) ON {config}.config_notification TO {mcp}; -- 9. Server onboarding (the MCP server-admin write tools): the mcp role's monitored-server writes, mirroring -- sections 7/8's model (an EXPLICIT single-table statement, NO ALTER DEFAULT PRIVILEGES). add_servers / diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingAlertReader.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingAlertReader.cs index abe495223..efe8a0e31 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingAlertReader.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingAlertReader.cs @@ -16,8 +16,9 @@ namespace PerformanceMonitor.Darling.Service.Mcp; /// /// Service-side reads for the alerts MCP tools () — the alert-history log -/// (config_alert_log) and the single global alert-settings row (config_alert_settings), both -/// STORED reads (no live monitored-server hit). Each SQL is reproduced from the viewer's proven read +/// (config_alert_log), the single global alert-settings row (config_alert_settings), and the +/// delivery cooldown that lives on config_notification instead, all STORED reads (no live +/// monitored-server hit). Each SQL is reproduced from the viewer's proven read /// (ViewerDataService.AlertHistory.cs / .AlertSettings.cs) rather than referenced — the MCP /// host is in the Service assembly and cannot reference the WPF Viewer, the same reason /// reproduces the viewer's config SQL. The reads live in public @@ -29,6 +30,10 @@ namespace PerformanceMonitor.Darling.Service.Mcp; /// running DarlingAlertSettings, so it reports the alert engine + analysis config the service is /// actually using (matching the viewer's Settings-window prefill), or null when the store has not seeded it /// yet. +/// +/// The delivery cooldown is the one alert-control-plane value on a DIFFERENT table, which is why it +/// gets its own read rather than another column on the settings SELECT — see +/// for why the SELECT list is deliberately one column wide. /// internal static class DarlingAlertReader { @@ -187,11 +192,13 @@ FROM config_alert_settings WHERE id = 1"; /// Reads the single global alert-settings row, or null when the store has not seeded it yet - /// (a pre-control-plane store, or the service has not started). - public static async Task GetAlertSettingsAsync( - NpgsqlDataSource postgres, CancellationToken cancellationToken = default) + /// (a pre-control-plane store, or the service has not started). Takes the caller's connection and + /// transaction rather than the data source, so it cannot be invoked outside the shared snapshot + /// establishes — see there for why that matters. + private static async Task ReadAlertSettingsAsync( + NpgsqlConnection connection, NpgsqlTransaction transaction, CancellationToken cancellationToken = default) { - await using var command = postgres.CreateCommand(AlertSettingsSelectSql); + await using var command = new NpgsqlCommand(AlertSettingsSelectSql, connection) { Transaction = transaction }; command.CommandTimeout = McpCommandDeadlines.ReadSeconds; await using var reader = await command.ExecuteReaderAsync(cancellationToken); if (!await reader.ReadAsync(cancellationToken)) @@ -225,4 +232,82 @@ FROM config_alert_settings /* #2391: V79 file-growth knobs at 54–57. */ reader.GetBoolean(54), reader.GetInt32(55), reader.GetInt32(56), reader.GetInt32(57)); } + + /* ─────────────────────── delivery cooldown (a SECOND config table) ─────────────────────── */ + + /// The per-fingerprint DELIVERY cooldown the shared notification paths throttle on + /// (WebhookAlertService and EmailSendCore both pass it to IncidentCooldown), stored + /// as config_notification.email_cooldown_minutes. Reported and accepted under the channel-neutral + /// name delivery.cooldown_minutes: the column predates the webhook channels and one number now + /// governs Slack, Teams, PagerDuty, the generic webhook AND email, so a headless deployment with no SMTP + /// at all is still throttled by it. + /// + /// A SEPARATE read because this is the ONE column of the alert control plane that does not live on + /// config_alert_settings — which is also why it reached 3.5.0 reachable only from the WPF Settings + /// window. The SELECT list is exactly one non-secret column, deliberately: config_notification + /// holds the SMTP password and the Teams/Slack/generic/PagerDuty bearer URLs, and the section-6 ACL + /// (DarlingManagedRoles.ViewerRestrictedConfigTables) SELECT-carves every one of them from + /// mcp — while column-level denial answers for the whole TABLE, so naming a single carved column + /// here would 42501 the entire read. That is the #2293/#2298 failure exactly, and it is why the host's + /// own whole-row LoadViewAsync was removed rather than made to skip rows; a tool-time read of + /// columns the carve GRANTS is the shape that survives. McpConfigReadAvoidsSecretColumnsTests + /// pins that this SELECT names no carved column. + public const string DeliveryCooldownSelectSql = @" +SELECT email_cooldown_minutes +FROM config_notification +WHERE id = 1"; + + /// Reads the delivery cooldown, or null when the store has no notification row. Null is a + /// distinct answer rather than the shipped 15: the service seeds this row and + /// config_alert_settings in ONE pass, so "settings present, notification absent" is not a state + /// the product produces, and reporting a number nobody wrote would claim a reading never taken. + private static async Task ReadDeliveryCooldownAsync( + NpgsqlConnection connection, NpgsqlTransaction transaction, CancellationToken cancellationToken = default) + { + await using var command = new NpgsqlCommand(DeliveryCooldownSelectSql, connection) { Transaction = transaction }; + command.CommandTimeout = McpCommandDeadlines.ReadSeconds; + await using var reader = await command.ExecuteReaderAsync(cancellationToken); + if (!await reader.ReadAsync(cancellationToken) || reader.IsDBNull(0)) + { + return null; + } + + return reader.GetInt32(0); + } + + /// Both halves of the alert configuration, read in ONE snapshot: the settings row and the + /// delivery cooldown that lives on the other table. Either may be null when the store has not seeded + /// that singleton yet. + public sealed record AlertConfigurationRead(AlertSettingsReadRow? Settings, int? DeliveryCooldownMinutes); + + /// + /// The alert configuration across BOTH config tables, under one snapshot. + /// + /// Two independent reads would let an update_alert_settings commit land between them and + /// hand the caller a payload mixing pre- and post-update state across the two tables — a stale + /// cooldown_minutes beside a fresh delivery.cooldown_minutes, or the reverse. The write + /// path already refuses to leave a half-landed state; a read that can REPORT one puts the asymmetry + /// back, and the post-write re-read is described to the caller as the authoritative merged state, which + /// it would not be. + /// + /// REPEATABLE READ, and the level is the whole mechanism. PostgreSQL takes a FRESH snapshot + /// per statement under READ COMMITTED, so wrapping these two SELECTs in a default transaction reads + /// exactly like a fix and changes nothing at all. The two single-table reads are private and take this + /// method's connection and transaction, so the split cannot be reintroduced by calling one of them + /// alone — a stronger guarantee than a test, since it does not compile. + /// + /// Read-only, so the transaction is disposed rather than committed; nothing here writes. + /// + public static async Task GetAlertConfigurationAsync( + NpgsqlDataSource postgres, CancellationToken cancellationToken = default) + { + await using var connection = await postgres.OpenConnectionAsync(cancellationToken); + await using var transaction = await connection.BeginTransactionAsync( + System.Data.IsolationLevel.RepeatableRead, cancellationToken); + + var settings = await ReadAlertSettingsAsync(connection, transaction, cancellationToken); + var deliveryCooldownMinutes = await ReadDeliveryCooldownAsync(connection, transaction, cancellationToken); + + return new AlertConfigurationRead(settings, deliveryCooldownMinutes); + } } diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpAlertTools.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpAlertTools.cs index 7896cdf0e..c4cc2522c 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpAlertTools.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpAlertTools.cs @@ -127,19 +127,32 @@ public static async Task GetAlertHistory( } } - [McpServerTool(Name = "get_alert_settings"), Description("Gets the current alert configuration the service is using: which alerts are enabled and their thresholds (CPU, blocking, deadlocks, poison waits, long-running queries/jobs, tempdb, low disk, failed jobs, database state, Availability Group health, connection loss), the cooldown, excluded databases, the deadlock/blocking delivery mode, and the scheduled-analysis cadence. This is the single global settings row the service hot-swaps in. SMTP/webhook delivery credentials are managed separately and are not reported here — configure them in the standalone Darling Viewer app's Settings window (Notifications section), which connects to this store (including remotely, not just localhost) rather than requiring desktop access to this specific box.")] + [McpServerTool(Name = "get_alert_settings"), Description("Gets the current alert configuration the service is using: which alerts are enabled and their thresholds (CPU, blocking, deadlocks, poison waits, long-running queries/jobs, tempdb, low disk, failed jobs, database state, Availability Group health, connection loss), the cooldown, excluded databases, the deadlock/blocking delivery mode and cooldown, and the scheduled-analysis cadence. TWO different cooldowns are reported and they govern different stages: top-level cooldown_minutes gates whether the alert engine FIRES at all, while delivery.cooldown_minutes is the per-alert-fingerprint throttle on the resulting Slack/Teams/PagerDuty/webhook/email post. A channel going quiet with alerts still in get_alert_history is delivery.cooldown_minutes, not cooldown_minutes. SMTP/webhook delivery credentials are managed separately and are not reported here — configure them in the standalone Darling Viewer app's Settings window (Notifications section), which connects to this store (including remotely, not just localhost) rather than requiring desktop access to this specific box.")] public static async Task GetAlertSettings( NpgsqlDataSource postgres) { try { - var s = await DarlingAlertReader.GetAlertSettingsAsync(postgres); + /* ONE snapshot across both config tables. The delivery cooldown is the single value on + config_notification rather than config_alert_settings, so reporting the configuration takes two + SELECTs -- and two independent reads could straddle a concurrent update_alert_settings commit + and report a mix of pre- and post-update state, which is the very thing the write path takes a + transaction to avoid producing. See DarlingAlertReader.GetAlertConfigurationAsync. */ + var (s, deliveryCooldown) = await DarlingAlertReader.GetAlertConfigurationAsync(postgres); if (s is null) return McpHelpers.Status( "unavailable", "No alert-settings row is present in the store yet. The service seeds it on startup (or the Viewer's Settings window writes it); until then the service runs on its darling.json defaults."); - return JsonSerializer.Serialize(BuildAlertSettingsPayload(s), McpHelpers.JsonOptions); + /* Absent means the notification row is unseeded, which the service seeds in the SAME pass as the + settings row -- so it is the same unseeded control plane the arm above reports, and reporting + the shipped 15 instead would state a number nobody wrote. */ + if (deliveryCooldown is null) + return McpHelpers.Status( + "unavailable", + "No notification row is present in the store yet, so the delivery cooldown cannot be reported. The service seeds it alongside the alert-settings row on startup (or the Viewer's Settings window writes it); until then the service runs on its darling.json defaults."); + + return JsonSerializer.Serialize(BuildAlertSettingsPayload(s, deliveryCooldown.Value), McpHelpers.JsonOptions); } catch (Exception ex) { @@ -155,8 +168,15 @@ public static async Task GetAlertSettings( /// set equality between this payload's keys and the columns AlertSettingsSelectSql reads — /// EveryColumnRead_IsEmittedByThePayload_AndAcceptedByTheWriter. Adding a key here without the /// matching arm in (or the reverse) fails that test rather than - /// shipping. - private static object BuildAlertSettingsPayload(DarlingAlertReader.AlertSettingsReadRow s) => new + /// shipping. + /// + /// That set equality is held PER TABLE (#3314). Every key here but one maps to a + /// config_alert_settings column; delivery.cooldown_minutes maps to + /// config_notification.email_cooldown_minutes, so it is passed in separately rather than read off + /// — a single set equality across both planes would compare a union against one + /// table's SELECT list and be satisfiable by drift on either side. + private static object BuildAlertSettingsPayload( + DarlingAlertReader.AlertSettingsReadRow s, int deliveryCooldownMinutes) => new { alerts_enabled = s.Enabled, notify_connection_changes = s.NotifyConnectionChanges, @@ -240,7 +260,20 @@ than a second opt-in behind one. The thresholds take the house _threshold_ }, cooldown_minutes = s.CooldownMinutes, excluded_databases = s.ExcludedDatabases, - delivery = new { mode = s.DeliveryMode, per_event_max = s.PerEventMax }, + delivery = new + { + mode = s.DeliveryMode, + per_event_max = s.PerEventMax, + /* #3314: the per-fingerprint DELIVERY cooldown -- stored as + config_notification.email_cooldown_minutes and, until now, the one alert-engine number no MCP + tool reported and none could write, so a headless Slack-only deployment could reach the sole + throttle on its channel volume only through the WPF Settings window. Reported HERE, beside + mode and per_event_max, because volume is what it governs and that is where someone tuning + volume looks -- not under a channel name for a channel they may not have configured. + DISTINCT from the top-level cooldown_minutes, which gates AlertEngine's FIRE decision: two + stages, two numbers, and only one of them used to be visible. */ + cooldown_minutes = deliveryCooldownMinutes + }, analysis = new { enabled = s.AnalysisEnabled, @@ -322,7 +355,14 @@ and the second one is a mute somebody INTENDED that is no longer in force. "the Viewer's Settings window enforces — thresholds in range, cpu.mode 'sql'|'total', delivery.mode " + "'Summary'|'PerEvent', counts within their bounds; an out-of-range value or an unknown field returns " + "{status:\"invalid\", ...} and writes NOTHING. On success the running service hot-reloads the change within " + - "one collection sweep. SMTP/webhook delivery credentials are managed separately and cannot be set here " + + "one collection sweep. TWO cooldowns are writable and they are different stages: cooldown_minutes gates " + + "the engine's FIRE decision, delivery.cooldown_minutes throttles the per-fingerprint post to " + + "Slack/Teams/PagerDuty/webhook/email (its stored name, email_cooldown_minutes, is also accepted as a " + + "top-level alias, but send only one of the two spellings). For silencing ONE recurring signature for a " + + "long stretch, use create_mute_rule instead of a long delivery cooldown: a mute is scoped, expires, is " + + "listed by get_mute_rules, and still logs the alert, where the cooldown is global to every fingerprint " + + "on every server and no tool reports what it suppressed. SMTP/webhook delivery credentials are managed " + + "separately and cannot be set here " + "— configure them in the standalone Darling Viewer app's Settings window (Notifications section), which " + "connects to this store (including remotely, not just localhost) rather than requiring desktop access to " + "this specific box. " + @@ -362,30 +402,75 @@ public static async Task UpdateAlertSettings( /* Only the provided columns are written. Column names are this method's compile-time constants (never the caller's input), so interpolating them into the SET list is injection-safe; every VALUE is a - bound parameter. The single-row config_version self-bump is left to the config-table trigger. */ - var setClause = string.Join(", ", updates.Select((u, i) => $"{u.Column} = ${i + 1}")); - await using var command = postgres.CreateCommand($"UPDATE config_alert_settings SET {setClause} WHERE id = 1"); - command.CommandTimeout = McpCommandDeadlines.ReadSeconds; - foreach (var (_, param) in updates) - { - command.Parameters.Add(param); - } + bound parameter. The single-row config_version self-bump is left to the config-table trigger. - var affected = await command.ExecuteNonQueryAsync(); - if (affected == 0) + ONE STATEMENT PER TABLE, in ONE transaction (#3314): the delivery cooldown lives on + config_notification while every other knob is on config_alert_settings, and a partial update is + the worst outcome available here -- the caller is told "updated" and the re-read below reports a + merged state that half-landed, with no indication which half. The statement order is FIXED + rather than the grouping's hash order, so two concurrent tools can never take the two singleton + rows in opposite orders. Both tables carry a bump trigger, so either statement alone is enough + to make the service reload. + + NEITHER statement touches modified_at, matching what this tool has always done to + config_alert_settings. Both Viewer upserts do bump it, and the asymmetry is deliberate: nothing + reads modified_at -- it is in no viewer projection, no payload and no decision -- and bumping it + on config_notification would mean granting mcp UPDATE on a SECOND column of a table holding + bearer secrets, to maintain a value with no reader. */ + await using var connection = await postgres.OpenConnectionAsync(); + await using var transaction = await connection.BeginTransactionAsync(); + + foreach (var table in WritableTables) { - return Outcome("unavailable", - "No alert-settings row is present in the store yet (id = 1). The service seeds it on startup (or the Viewer's Settings window writes it); until then there is nothing to update."); + var forTable = updates.Where(u => string.Equals(u.Table, table, StringComparison.Ordinal)).ToList(); + if (forTable.Count == 0) + { + continue; + } + + var setClause = string.Join(", ", forTable.Select((u, i) => $"{u.Column} = ${i + 1}")); + /* Constructed two-arg on the store connection with the transaction ASSIGNED, not passed as a + third ctor argument. McpReadCommandTimeoutTests classifies every command on this surface as + addressing the store or a monitored TARGET, and the three-arg + new NpgsqlCommand(sql, connection, transaction) form is the monitored-target shape (the + HypoPG experiment's, bounded by a server-side SET LOCAL rather than by McpCommandDeadlines). + Widening that allowlist to admit a transaction would have let a real target command take a + store bound, so the store command takes the shape the guard already recognises. The SQL is + hoisted to a local for the same reason: the recognised form's first argument is an + identifier. */ + var sql = $"UPDATE {table} SET {setClause} WHERE id = 1"; + await using var command = new NpgsqlCommand(sql, connection) { Transaction = transaction }; + command.CommandTimeout = McpCommandDeadlines.ReadSeconds; + foreach (var target in forTable) + { + command.Parameters.Add(target.Param); + } + + if (await command.ExecuteNonQueryAsync() == 0) + { + /* Rolled back by the transaction's disposal on the early return -- nothing this call + provided is left applied, so the reported failure and the store agree. */ + return Outcome("unavailable", + $"No {table} row is present in the store yet (id = 1). The service seeds it on startup (or the Viewer's Settings window writes it); until then there is nothing to update."); + } } + await transaction.CommitAsync(); + /* Re-read so the caller sees the authoritative merged state — the write fired the config-table trigger - that self-bumps config_version, so the running service reloads this within one sweep. */ - var reread = await DarlingAlertReader.GetAlertSettingsAsync(postgres); + that self-bumps config_version, so the running service reloads this within one sweep. ONE snapshot + across both tables, for the reason get_alert_settings uses one: "authoritative merged state" is + a claim a pair of independent reads cannot keep. */ + var (reread, rereadCooldown) = await DarlingAlertReader.GetAlertConfigurationAsync(postgres); return JsonSerializer.Serialize(new { status = "updated", + /* Bare column names, unqualified, even though two tables are now in play: this array is a + consumer API and qualifying the existing entries would redefine every one of them. No + writable column name appears on both tables, so a bare name is still unambiguous -- + asserted, not assumed, by WritableColumnNames_DoNotCollideAcrossTheTwoTables. */ updated_fields = updates.Select(u => u.Column).ToArray(), - settings = reread is null ? null : BuildAlertSettingsPayload(reread) + settings = reread is null || rereadCooldown is null ? null : BuildAlertSettingsPayload(reread, rereadCooldown.Value) }, McpHelpers.JsonOptions); } catch (Exception ex) @@ -489,6 +574,20 @@ public static async Task DeleteMuteRule( } } + /// The singleton config rows update_alert_settings writes, in the order it writes them — see + /// the statement-order note at the write itself. Also the read side's table set: the settings row is + /// 's source and the notification row is + /// 's. + internal static readonly string[] WritableTables = { AlertSettingsTable, NotificationTable }; + + internal const string AlertSettingsTable = "config_alert_settings"; + internal const string NotificationTable = "config_notification"; + + /// The delivery cooldown's stored column name — the wire alias as well as the column, which is + /// the whole point of keeping it: delivery.cooldown_minutes is what the tool reports, and this is + /// what every existing config file, Settings window and hand-written UPDATE already calls it. + internal const string DeliveryCooldownColumn = "email_cooldown_minutes"; + private static string? Trimmed(string? value) => string.IsNullOrWhiteSpace(value) ? null : value.Trim(); /// A small {status, message} envelope for a non-data write outcome (invalid / not_found / unavailable) @@ -505,21 +604,24 @@ private static string Outcome(string status, string message) => /// nothing — on the FIRST bad value or unknown field (top-level or nested). Column names are this method's /// compile-time constants (never the input), so interpolating them into the UPDATE's SET list is injection-safe. /// - private static (List<(string Column, NpgsqlParameter Param)> Updates, string? Error) BuildAlertSettingsUpdate(JsonObject body) + private static (List Updates, string? Error) BuildAlertSettingsUpdate(JsonObject body) { - var updates = new List<(string Column, NpgsqlParameter Param)>(); + var updates = new List(); string? error = null; void AddBool(string column, JsonNode? node, string field) { if (error != null) return; if (node is JsonValue v && v.TryGetValue(out var b)) - updates.Add((column, new NpgsqlParameter { TypedValue = b })); + updates.Add(new UpdateTarget(AlertSettingsTable, column, field, new NpgsqlParameter { TypedValue = b })); else error = $"'{field}' must be true or false."; } - void AddInt(string column, JsonNode? node, string field, int min, int max) + /* `table` defaults to the settings row because all but one column lives there; the delivery + cooldown passes NotificationTable. Routing rather than a second parser so every field still + validates through one set of adders and one first-error rule. */ + void AddInt(string column, JsonNode? node, string field, int min, int max, string table = AlertSettingsTable) { if (error != null) return; if (node is JsonValue v && v.TryGetValue(out var i)) @@ -527,7 +629,7 @@ void AddInt(string column, JsonNode? node, string field, int min, int max) if (i < min || i > max) error = $"'{field}' must be an integer between {min} and {max}."; else - updates.Add((column, new NpgsqlParameter { TypedValue = i })); + updates.Add(new UpdateTarget(table, column, field, new NpgsqlParameter { TypedValue = i })); } else { @@ -547,7 +649,7 @@ void AddLong(string column, JsonNode? node, string field, long min, long max) if (l < min || l > max) error = $"'{field}' must be an integer between {min} and {max}."; else - updates.Add((column, new NpgsqlParameter { TypedValue = l })); + updates.Add(new UpdateTarget(AlertSettingsTable, column, field, new NpgsqlParameter { TypedValue = l })); } else { @@ -563,7 +665,7 @@ void AddDouble(string column, JsonNode? node, string field, double min, double m if (d < min || d > max) error = $"'{field}' must be a number between {min.ToString("0.0", CultureInfo.InvariantCulture)} and {max.ToString("0.0", CultureInfo.InvariantCulture)}."; else - updates.Add((column, new NpgsqlParameter { TypedValue = d })); + updates.Add(new UpdateTarget(AlertSettingsTable, column, field, new NpgsqlParameter { TypedValue = d })); } else { @@ -580,7 +682,7 @@ void AddEnum(string column, JsonNode? node, string field, params string[] allowe if (match == null) error = $"'{field}' must be one of: {string.Join(", ", allowed)}."; else - updates.Add((column, new NpgsqlParameter { TypedValue = match })); + updates.Add(new UpdateTarget(AlertSettingsTable, column, field, new NpgsqlParameter { TypedValue = match })); } else { @@ -607,7 +709,8 @@ void AddStringArray(string column, JsonNode? node, string field) } } - updates.Add((column, new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Array | NpgsqlDbType.Text, Value = array.ToArray() })); + updates.Add(new UpdateTarget(AlertSettingsTable, column, field, + new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Array | NpgsqlDbType.Text, Value = array.ToArray() })); } else { @@ -646,6 +749,13 @@ shipped configuration (one alert per outage, no re-fire), not an invalid one. */ case "notify_connection_down_at_startup": AddBool("notify_connection_down_at_startup", prop.Value, "notify_connection_down_at_startup"); break; case "connection_refire_minutes": AddInt("connection_refire_minutes", prop.Value, "connection_refire_minutes", 0, 1440); break; case "cooldown_minutes": AddInt("cooldown_minutes", prop.Value, "cooldown_minutes", 1, 120); break; + /* #3314: the STORED column name, accepted as an alias for delivery.cooldown_minutes so an + existing darling.json key, a hand-written UPDATE, or Lite's settings.json spelling keeps + working against the tool. Not emitted by get_alert_settings -- two wire keys for one + column is its own bug for a client, so the alias is write-only and the canonical name is + the only one that round-trips. Sending both in one body is refused below rather than + letting one silently win. */ + case DeliveryCooldownColumn: AddInt(DeliveryCooldownColumn, prop.Value, DeliveryCooldownColumn, 1, 120, NotificationTable); break; case "excluded_databases": AddStringArray("excluded_databases", prop.Value, "excluded_databases"); break; case "cpu": @@ -859,6 +969,13 @@ which is why the rise floor is 0 and not 1. */ { case "mode": AddEnum("delivery_mode", n, "delivery.mode", "Summary", "PerEvent"); break; case "per_event_max": AddInt("per_event_max", n, "delivery.per_event_max", 1, 100); break; + /* #3314. The bound is DarlingAlertSettings' Clamp(..., 1, 120) EXACTLY -- the + same parity the file-growth and AG bounds hold, and for the same reason: a + wider bound here would ACCEPT a value the engine then silently rewrites on + read, which presents to the operator as the setting not sticking. Raising the + ceiling is therefore an engine change across both SKUs, not a bound edit; see + the PR for why long suppression belongs to create_mute_rule instead. */ + case "cooldown_minutes": AddInt(DeliveryCooldownColumn, n, "delivery.cooldown_minutes", 1, 120, NotificationTable); break; default: error = $"Unknown field 'delivery.{k}'."; break; } }); @@ -886,6 +1003,33 @@ which is why the rise floor is 0 and not 1. */ } } + /* Two accepted keys claiming ONE column. The only pair that can do this today is + delivery.cooldown_minutes and its email_cooldown_minutes alias, and both landing in one SET list + is a Postgres error (multiple assignments to the same column) -- so refusing it here names the two + spellings instead of surfacing a dialect message, and the caller learns which key to drop. Checked + over (table, column) because the two planes are written by separate statements. */ + if (error == null) + { + var clash = updates + .GroupBy(u => (u.Table, u.Column)) + .FirstOrDefault(g => g.Count() > 1); + if (clash != null) + { + error = $"'{string.Join("' and '", clash.Select(u => u.Field))}' are two names for the same setting " + + $"({clash.Key.Table}.{clash.Key.Column}); send only one of them."; + } + } + return (updates, error); } + + /// One validated field of a partial update: the TABLE it writes, the column, the wire field + /// name it arrived under (for the two-names-one-column message), and the bound parameter. + /// + /// The table is carried per field because update_alert_settings spans two config tables + /// (#3314): all but one column is on the singleton config_alert_settings row, and the delivery + /// cooldown is on the singleton config_notification row. Carrying it beats inferring it from the + /// column name — an inference that would be correct today and silently wrong the first time a second + /// notification knob arrives. + private sealed record UpdateTarget(string Table, string Column, string Field, NpgsqlParameter Param); } diff --git a/Darling/PerformanceMonitor.Darling.Service/darling.sample.json b/Darling/PerformanceMonitor.Darling.Service/darling.sample.json index 958242f09..e7481f1c1 100644 --- a/Darling/PerformanceMonitor.Darling.Service/darling.sample.json +++ b/Darling/PerformanceMonitor.Darling.Service/darling.sample.json @@ -262,6 +262,14 @@ // SMTP alert delivery (optional). Enabled when host + from + to are all set. For an // authenticated relay, set username and run --encrypt-password for the password blob. + // + // emailCooldownMinutes is NOT email-only despite living here and despite its name: it is the + // per-alert-fingerprint throttle the shared delivery paths apply to EVERY channel - Teams, Slack, + // PagerDuty, the generic webhook and email alike - so it governs channel volume on a deployment with + // no SMTP configured at all. Clamped 1-120. Read and written through the control plane as + // delivery.cooldown_minutes (get_alert_settings / update_alert_settings); the key keeps its name here + // so existing files keep working. To silence ONE recurring signature for a long stretch, add a mute + // rule rather than lengthening this - a mute is scoped, expires, and is listed back to you. "smtp": { "host": "", "port": 587, diff --git a/Darling/README.md b/Darling/README.md index a78c44ef4..861b6fc86 100644 --- a/Darling/README.md +++ b/Darling/README.md @@ -556,7 +556,7 @@ The embedded MCP server, over Streamable HTTP bound to `localhost` by default (s The create/update/delete tools are the one view-authoring **write** surface; create/update run the SAME `ValidateDefinition` authority as `validate_custom_view`, so an invalid definition is rejected before it stores; every tool routes through the SAME store + validator + compile-and-run + catalog the web viewer's editor uses (no divergent second implementation). This write surface is part of what the MCP token gates — see [What a token can reach](#opt-in-network-endpoints-lan) below. -- **Alert-tuning write tools (Darling-only)** — `update_alert_settings`, `create_mute_rule`, and `delete_mute_rule` let an MCP client TUNE the alert engine the fleet shares — the SAME config `get_alert_settings` / `get_mute_rules` read and the Viewer's Settings window writes. `update_alert_settings` is a PARTIAL update of the single global settings row: read via `get_alert_settings`, change fields, and send only those back in the same nested shape; every field is validated against the SAME ranges/enums the Settings window enforces BEFORE any write, an out-of-range or unknown field returns `{status:"invalid"}` and writes nothing, and the write self-bumps `config_version` so the running service hot-reloads within one collection sweep. `create_mute_rule` / `delete_mute_rule` reuse the SAME `PgMuteRuleStore` `get_mute_rules` reads through (and the same GUID id-generation the Viewer's mute-create path uses). None touches a monitored SQL Server or the collected data — only the shared alert configuration; SMTP/webhook delivery credentials are out of scope (the `mcp` role cannot read or write the secret columns). It is part of what the MCP token gates — see [What a token can reach](#opt-in-network-endpoints-lan) below. +- **Alert-tuning write tools (Darling-only)** — `update_alert_settings`, `create_mute_rule`, and `delete_mute_rule` let an MCP client TUNE the alert engine the fleet shares — the SAME config `get_alert_settings` / `get_mute_rules` read and the Viewer's Settings window writes. `update_alert_settings` is a PARTIAL update of the alert configuration: read via `get_alert_settings`, change fields, and send only those back in the same nested shape; every field is validated against the SAME ranges/enums the Settings window enforces BEFORE any write, an out-of-range or unknown field returns `{status:"invalid"}` and writes nothing, and the write self-bumps `config_version` so the running service hot-reloads within one collection sweep. `create_mute_rule` / `delete_mute_rule` reuse the SAME `PgMuteRuleStore` `get_mute_rules` reads through (and the same GUID id-generation the Viewer's mute-create path uses). It spans the two singleton config rows the alert engine reads: every knob is on `config_alert_settings` except the per-fingerprint DELIVERY cooldown, which is stored as `config_notification.email_cooldown_minutes` and reported and accepted as `delivery.cooldown_minutes` — a channel-neutral name, because that one number is the only throttle on a Slack / Teams / PagerDuty / generic-webhook post as well as on email, so a headless deployment with no SMTP configured at all is still governed by it (the stored spelling is accepted as a write-only alias). Note the two cooldowns are different stages: top-level `cooldown_minutes` gates whether the engine FIRES, `delivery.cooldown_minutes` throttles the resulting post. None touches a monitored SQL Server or the collected data — only the shared alert configuration; SMTP/webhook delivery credentials are out of scope (the `mcp` role can read and write exactly the one non-secret cooldown column of the notification table, and cannot read or write any secret column). It is part of what the MCP token gates — see [What a token can reach](#opt-in-network-endpoints-lan) below. - **Server-onboarding write tools (Darling-only)** — `add_servers` (BULK) and `remove_server` let an MCP client stand up or tear down FLEET monitoring conversationally ("monitor these twenty servers with this login"), the service-side twin of the Viewer's Add / Manage Servers dialogs. `add_servers` takes a JSON **array** of server objects (`host` required; optional `display_name` / `database` / `read_only_intent` / `multi_subnet_failover`; `auth` `Windows`/`SQL` with `username`+`password` for SQL; and the exposed TLS options `encrypt_mode` `Optional`/`Mandatory`/`Strict` + `trust_server_certificate`) and processes them **in order**: it validates each entry, PROBES the connection in-process (reusing the same `DarlingServerConnector.ProbeAsync` the `--test-connection` verb runs — the service holds the network path + credentials, so no `test_connect` command plane is needed), skips a case-folded duplicate (`duplicate`) of an already-monitored server or an earlier entry, DPAPI-encrypts the SQL password (the service identity, so it round-trips at collection time), and INSERTs the row mirroring the service's own seed shape. A server that fails to connect is `connection_failed` and the batch continues; Entra/MFA/Service-Principal/Managed-Identity auth is `invalid` (the service connects with Windows or SQL only). `remove_server` DELETEs a monitored server by name (resolved the same way every `server_name` is) — already-collected history is kept. Both write only the monitoring store's `config.config_monitored_servers` registry; neither runs anything on a monitored server beyond the one-time probe. **The SQL password travels to the endpoint inside `add_servers`' request** and is DPAPI-encrypted at rest (never returned) — it is part of what the MCP token gates, and it puts a credential on the wire; see [What a token can reach](#opt-in-network-endpoints-lan) below. @@ -1171,9 +1171,9 @@ Table names are unchanged — only their schema moved — and the shared SQL kee | `darling` | superuser / owner | the service (collection, migration, provisioning) | | `admin` | SELECT on both schemas — **including** the secret columns, which the Settings window reads — plus INSERT/UPDATE/DELETE on `config` only. No statement timeout | the Viewer, by default (`connectAs: "admin"`) | | `viewer` | SELECT on all of `collect`, and on `config` **minus the secret columns** of `config_monitored_servers` / `config_command` / `config_notification` (carved fail-closed, below) + INSERT/UPDATE/DELETE on `config.custom_views` only (the web composer's saved views). Runs under `statement_timeout = 15s` | a locked-down Viewer (`connectAs: "viewer"`), and the web dashboard | -| `mcp` | `viewer`'s exact read surface + INSERT on `collect.analysis_findings` / `config.analysis_muted` + INSERT/UPDATE/DELETE on `config.custom_views` (the custom-view tools) + the alert-tuning writes (INSERT/UPDATE/DELETE on `config.config_mute_rules`, UPDATE on `config.config_alert_settings`, and the `config_service` reload-beacon columns) + the server-onboarding writes (INSERT/UPDATE/DELETE on `config.config_monitored_servers` — the credential column stays SELECT-carved, so it can WRITE a password blob but never READ one back) | the store identity the opt-in MCP **network** endpoint connects as (managed only); dormant until MCP is exposed on the LAN | +| `mcp` | `viewer`'s exact read surface + INSERT on `collect.analysis_findings` / `config.analysis_muted` + INSERT/UPDATE/DELETE on `config.custom_views` (the custom-view tools) + the alert-tuning writes (INSERT/UPDATE/DELETE on `config.config_mute_rules`, UPDATE on `config.config_alert_settings`, UPDATE on the single non-secret `email_cooldown_minutes` column of `config.config_notification`, and the `config_service` reload-beacon columns) + the server-onboarding writes (INSERT/UPDATE/DELETE on `config.config_monitored_servers` — the credential column stays SELECT-carved, so it can WRITE a password blob but never READ one back) | the store identity the opt-in MCP **network** endpoint connects as (managed only); dormant until MCP is exposed on the LAN | -`admin` cannot `DROP`, alter schema, touch `collect` data, or create objects — it can only do what the Viewer's mute-rule / alert-dismiss surfaces need. The `mcp` role is narrower still: it reads exactly what `viewer` reads (the secret config columns are carved out identically) and its writes are a small, enumerated set — the two analysis-table INSERTs (`analyze_server` + `mute_analysis_finding`), the single-table `config.custom_views` CRUD (the custom-view tools), the alert-tuning writes (`config.config_mute_rules` CRUD + a single-row `config.config_alert_settings` UPDATE, plus the two `config_service` beacon columns so a settings write's self-bump trigger can fire), and the server-onboarding writes (`config.config_monitored_servers` CRUD for `add_servers` / `remove_server` — its `config_monitored_servers` write fires the SAME `config_service` beacon trigger, already covered by that column grant) — so a token-holder on the network MCP endpoint can never reach the `config`-table service-credential pivot, the secret columns, or a service flag like `paused`. Even on `config_monitored_servers`, which it may write, the `encrypted_password` column stays in the fail-closed secret carve, so `mcp` can WRITE a credential blob (onboarding) but can never READ one back. `ALTER DEFAULT PRIVILEGES` means new collector tables auto-inherit SELECT for `admin`/`viewer`, so the model never drifts as collectors are added (every `mcp` write is an explicit single-table/single-column grant, deliberately not schema-wide). +`admin` cannot `DROP`, alter schema, touch `collect` data, or create objects — it can only do what the Viewer's mute-rule / alert-dismiss surfaces need. The `mcp` role is narrower still: it reads exactly what `viewer` reads (the secret config columns are carved out identically) and its writes are a small, enumerated set — the two analysis-table INSERTs (`analyze_server` + `mute_analysis_finding`), the single-table `config.custom_views` CRUD (the custom-view tools), the alert-tuning writes (`config.config_mute_rules` CRUD + a single-row `config.config_alert_settings` UPDATE + a single-COLUMN `config.config_notification` UPDATE for the delivery cooldown, plus the two `config_service` beacon columns so a settings write's self-bump trigger can fire), and the server-onboarding writes (`config.config_monitored_servers` CRUD for `add_servers` / `remove_server` — its `config_monitored_servers` write fires the SAME `config_service` beacon trigger, already covered by that column grant) — so a token-holder on the network MCP endpoint can never reach the `config`-table service-credential pivot, the secret columns, or a service flag like `paused`. Even on `config_monitored_servers`, which it may write, the `encrypted_password` column stays in the fail-closed secret carve, so `mcp` can WRITE a credential blob (onboarding) but can never READ one back. `config_notification` is the sharpest case of the same shape: it holds the SMTP password blob, the Teams/Slack/generic webhook URLs and the PagerDuty routing key, so the write there is COLUMN-level on the one non-secret cooldown column — `mcp` cannot write even a non-secret sibling like `smtp_host`, let alone a credential. `ALTER DEFAULT PRIVILEGES` means new collector tables auto-inherit SELECT for `admin`/`viewer`, so the model never drifts as collectors are added (every `mcp` write is an explicit single-table/single-column grant, deliberately not schema-wide). **Managed mode** provisions all of this automatically on every start (idempotent and self-healing), generating a per-role DPAPI-LocalMachine credential — `pg-admin-credential.dpapi`, `pg-viewer-credential.dpapi`, and `pg-mcp-credential.dpapi` beside the data directory, same posture as the owner's `pg-credential.dpapi`. Nothing to configure beyond `connectAs`. @@ -1310,7 +1310,7 @@ New-NetFirewallRule -DisplayName "Darling MCP" -Direction Inbound -Action Allow **What a token-holder can — and cannot — do.** Start with the boundary: **no MCP tool runs SQL an AI client wrote against your monitored servers.** No such tool exists, and a stored custom view cannot become one either — a composed query names only `collect.*` collector tables in the monitoring store. The only live contact with a monitored SQL Server is `analyze_server`'s plan fetch and `add_servers`' one-time connection probe, and both run the product's own fixed, read-only queries under the same least-privilege monitoring login the collectors use — the ceiling on what they can see is the ceiling you granted that login, and it has no write grants to hit. Everything else answers from the monitoring store. -What the token does gate is the monitor's own configuration and collected data: the entire read surface, `analyze_server`, the Custom Views tools (create / modify / delete the saved dashboards and notebooks in `config.custom_views`), the alert-tuning tools (`update_alert_settings` / `create_mute_rule` / `delete_mute_rule`), and the server-onboarding tools (`add_servers` / `remove_server`), which edit the monitored-server registry in `config.config_monitored_servers` — including storing a SQL-auth credential for a server they add. The store-side identity is still the least-privilege `mcp` role: read, the two analysis-table INSERTs, INSERT/UPDATE/DELETE on the single `config.custom_views` table (the same narrow write the web composer's `viewer` role has), the narrow alert-config writes (`config.config_mute_rules` CRUD + a single-row `config.config_alert_settings` UPDATE, plus the `config_service` reload-beacon columns), and the single-table `config.config_monitored_servers` CRUD. So a token-holder can read everything collected, trigger analysis, author custom views, tune alerting, and onboard/offboard servers — and can never reach the `config_command` service-credential pivot, the carved secret columns (SMTP/webhook credentials, and the monitored-server `encrypted_password` blob it can WRITE during onboarding but never READ back, all included), or a service flag like `paused`. Custom-view JSON and alert config carry no secrets. Guard the token like the keys to your monitoring configuration — that is what it opens; your SQL Servers are not behind it. +What the token does gate is the monitor's own configuration and collected data: the entire read surface, `analyze_server`, the Custom Views tools (create / modify / delete the saved dashboards and notebooks in `config.custom_views`), the alert-tuning tools (`update_alert_settings` / `create_mute_rule` / `delete_mute_rule`), and the server-onboarding tools (`add_servers` / `remove_server`), which edit the monitored-server registry in `config.config_monitored_servers` — including storing a SQL-auth credential for a server they add. The store-side identity is still the least-privilege `mcp` role: read, the two analysis-table INSERTs, INSERT/UPDATE/DELETE on the single `config.custom_views` table (the same narrow write the web composer's `viewer` role has), the narrow alert-config writes (`config.config_mute_rules` CRUD + a single-row `config.config_alert_settings` UPDATE + a single-column `config.config_notification` UPDATE for the delivery cooldown, plus the `config_service` reload-beacon columns), and the single-table `config.config_monitored_servers` CRUD. So a token-holder can read everything collected, trigger analysis, author custom views, tune alerting, and onboard/offboard servers — and can never reach the `config_command` service-credential pivot, the carved secret columns (SMTP/webhook credentials, and the monitored-server `encrypted_password` blob it can WRITE during onboarding but never READ back, all included), or a service flag like `paused`. Custom-view JSON and alert config carry no secrets. Guard the token like the keys to your monitoring configuration — that is what it opens; your SQL Servers are not behind it. **`add_servers` carries a credential in its request.** A SQL-auth `password` rides the request JSON; the service DPAPI-encrypts it at rest and never returns it, but on the wire it is only as protected as the endpoint — the same plaintext HTTP the token rides. On a segment you do not fully trust, front the MCP port with the TLS reverse proxy below, and prefer Windows/integrated auth for onboarded servers where you can — then no per-server secret crosses the wire at all. diff --git a/Lite/Mcp/McpAlertTools.cs b/Lite/Mcp/McpAlertTools.cs index 71f498e0a..6fcd1a64b 100644 --- a/Lite/Mcp/McpAlertTools.cs +++ b/Lite/Mcp/McpAlertTools.cs @@ -77,7 +77,7 @@ public static async Task GetAlertHistory( } } - [McpServerTool(Name = "get_alert_settings"), Description("Gets the current alert configuration this instance is running on: which alerts are enabled and their thresholds (CPU, blocking, deadlocks, poison waits, long-running queries and jobs, tempdb space, low disk, PVS, file growth, failed jobs, database state, Availability Group health, connection loss), the cooldown, the excluded databases, the deadlock/blocking delivery mode, the scheduled-analysis cadence, and the SMTP email configuration. The same nested shape Darling's get_alert_settings returns, minus its self_alerts group (the headless service's own store-volume and collection-health thresholds, which a single-instance Lite install has no equivalent for) and plus smtp, which Lite delivers itself. Read-only: Lite has no update_alert_settings, so these change in the Settings window.")] + [McpServerTool(Name = "get_alert_settings"), Description("Gets the current alert configuration this instance is running on: which alerts are enabled and their thresholds (CPU, blocking, deadlocks, poison waits, long-running queries and jobs, tempdb space, low disk, PVS, file growth, failed jobs, database state, Availability Group health, connection loss), the cooldown, the excluded databases, the deadlock/blocking delivery mode and cooldown, the scheduled-analysis cadence, and the SMTP email configuration. The two cooldowns govern different stages: top-level cooldown_minutes gates whether an alert FIRES, delivery.cooldown_minutes throttles the per-fingerprint email/Teams/Slack/PagerDuty/webhook send. The same nested shape Darling's get_alert_settings returns, minus its self_alerts group (the headless service's own store-volume and collection-health thresholds, which a single-instance Lite install has no equivalent for) and plus smtp, which Lite delivers itself. Read-only: Lite has no update_alert_settings, so these change in the Settings window.")] public static Task GetAlertSettings() { try @@ -227,7 +227,15 @@ cannot drift the way Lite's app-local CpuAlertMode could. Darling's update_alert_settings validates delivery.mode against "Summary"/"PerEvent", which are those same member names. */ mode = App.AlertDeliveryMode.ToString(), - per_event_max = App.AlertPerEventMaxPerCycle + per_event_max = App.AlertPerEventMaxPerCycle, + /* #3314. Lite runs the SAME shared throttle -- WebhookAlertService and EmailSendCore both + hand IncidentCooldown App.EmailCooldownMinutes through AppAlertSettings -- so the + channel-neutral name Darling adopted applies here verbatim. The stored spelling stays + email_cooldown_minutes in settings.json and the Settings window; only the wire key is + channel-neutral, because one number governs Teams, Slack, PagerDuty, the generic + webhook AND email. DISTINCT from cooldown_minutes above, which gates the engine's FIRE + decision rather than the post. */ + cooldown_minutes = App.EmailCooldownMinutes }, analysis = new {