From 99d3c02b64991584775b892dd7250063c70c7647 Mon Sep 17 00:00:00 2001
From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com>
Date: Fri, 25 Sep 2026 19:53:57 -0400
Subject: [PATCH 1/3] Stop get_plan_force_actions from serving pre-#4326
exception text (#4346)
Rows written before #4326 put ex.Message straight into
collect.plan_force_actions.detail for a state_unavailable blocker.
Those rows stay until retention (up to a year). Nothing reads the
column back today, but the read behind get_plan_force_actions
(GetRecentActionsAsync) would serve it verbatim once something calls
it.
GetRecentActionsAsync now runs every row's detail through
SanitizeDetailForAudit before returning it. There is no build marker
on the row, so this is a positive allow-list: the only
state_unavailable evidence line #4326 can produce (a type name, plus
SQLSTATE for a DbException, plus one of two fixed log notes) passes
through; any state_unavailable line that does not match that shape
(a pre-#4326 row's raw exception message) is replaced with a fixed
sentence naming no exception text. Every other blocker's evidence
line has never carried exception text and passes through untouched.
Pinned with unit tests over SanitizeDetailForAudit directly: the
three post-fix shapes pass through unchanged, a legacy row carrying
a credential and a hostname in its exception text is redacted to the
fixed sentence, an unrelated blocker line on the same row survives,
and null/non-state_unavailable detail are untouched.
---
.../PlanForceActionAuditRedactionTests.cs | 84 +++++++++++++++++++
.../PgPlanForceActionStore.cs | 66 ++++++++++++++-
2 files changed, 148 insertions(+), 2 deletions(-)
create mode 100644 Darling/Darling.Tests/PlanForceActionAuditRedactionTests.cs
diff --git a/Darling/Darling.Tests/PlanForceActionAuditRedactionTests.cs b/Darling/Darling.Tests/PlanForceActionAuditRedactionTests.cs
new file mode 100644
index 0000000000..7abf26aec0
--- /dev/null
+++ b/Darling/Darling.Tests/PlanForceActionAuditRedactionTests.cs
@@ -0,0 +1,84 @@
+/*
+ * Copyright (c) 2026 Erik Darling, Darling Data LLC
+ *
+ * This file is part of the SQL Server Performance Monitor.
+ *
+ * Licensed under the MIT License. See LICENSE file in the project root for full license information.
+ */
+
+using PerformanceMonitor.Darling.Service;
+using Xunit;
+
+namespace Darling.Tests;
+
+///
+/// #4346: PgPlanForceActionStore.SanitizeDetailForAudit is the read-side gate that keeps
+/// pre-#4326 rows' raw exception text out of get_plan_force_actions (and any other reader of
+/// GetRecentActionsAsync). There is no build marker on the row, so the gate is a positive match
+/// against the one line #4326 made the only safe shape for a state_unavailable evidence line;
+/// anything else on that line is legacy and gets replaced.
+///
+public sealed class PlanForceActionAuditRedactionTests
+{
+ [Fact]
+ public void PostFixLine_PassesThrough_TypeOnly()
+ {
+ var detail = "state_unavailable: the forcing and automatic-plan-correction state read failed (NpgsqlException; the log has the full error) \u2014 an unattended force cannot proceed on an unknown engine state";
+ Assert.Equal(detail, PgPlanForceActionStore.SanitizeDetailForAudit(detail));
+ }
+
+ [Fact]
+ public void PostFixLine_PassesThrough_WithSqlState()
+ {
+ var detail = "state_unavailable: the forcing and automatic-plan-correction state read failed (PostgresException, SQLSTATE 57014; the log has the full error) \u2014 an unattended force cannot proceed on an unknown engine state";
+ Assert.Equal(detail, PgPlanForceActionStore.SanitizeDetailForAudit(detail));
+ }
+
+ [Fact]
+ public void PostFixLine_PassesThrough_MissingSchemaNote()
+ {
+ var detail = "state_unavailable: the forcing and automatic-plan-correction state read failed (PostgresException, SQLSTATE 42P01; a table or column the read needs is missing, logged only at Debug level) \u2014 an unattended force cannot proceed on an unknown engine state";
+ Assert.Equal(detail, PgPlanForceActionStore.SanitizeDetailForAudit(detail));
+ }
+
+ /// The pin: a pre-#4326 row's raw exception text (host, credential, relation names — anything
+ /// an exception message can carry) never reaches the caller. This is the exact shape
+ /// PgPlanForceActionStore.TryGetTargetStatesAsync's old catch wrote before #4326.
+ [Fact]
+ public void LegacyLine_WithRawExceptionText_IsRedacted()
+ {
+ var legacy = "state_unavailable: the forcing and automatic-plan-correction state read failed (NpgsqlException: 28P01: password authentication failed for user \"darling_ro\" at host db-primary-02.internal)";
+ var sanitized = PgPlanForceActionStore.SanitizeDetailForAudit(legacy);
+
+ Assert.DoesNotContain("darling_ro", sanitized);
+ Assert.DoesNotContain("db-primary-02", sanitized);
+ Assert.DoesNotContain("password authentication failed", sanitized);
+ Assert.Equal(
+ "state_unavailable: the forcing and automatic-plan-correction state read failed \u2014 an unattended force cannot proceed on an unknown engine state",
+ sanitized);
+ }
+
+ [Fact]
+ public void OtherBlockerLines_AreNeverTouched()
+ {
+ var detail = "apc_owns_it: query_store_stats: plan 7 is_forced_plan = true\nstate_unavailable: garbage (Exception: whatever leaked)";
+ var sanitized = PgPlanForceActionStore.SanitizeDetailForAudit(detail)!;
+ var lines = sanitized.Split('\n');
+
+ Assert.Equal("apc_owns_it: query_store_stats: plan 7 is_forced_plan = true", lines[0]);
+ Assert.DoesNotContain("whatever leaked", lines[1]);
+ }
+
+ [Fact]
+ public void NonStateUnavailableDetail_IsUnchanged()
+ {
+ var detail = "no write path in this build (#2138 phase 1 is detection, evidence and dry-run only)";
+ Assert.Equal(detail, PgPlanForceActionStore.SanitizeDetailForAudit(detail));
+ }
+
+ [Fact]
+ public void NullDetail_StaysNull()
+ {
+ Assert.Null(PgPlanForceActionStore.SanitizeDetailForAudit(null));
+ }
+}
diff --git a/Darling/PerformanceMonitor.Darling.Service/PgPlanForceActionStore.cs b/Darling/PerformanceMonitor.Darling.Service/PgPlanForceActionStore.cs
index 15a7c1bd9f..b8c0bf6610 100644
--- a/Darling/PerformanceMonitor.Darling.Service/PgPlanForceActionStore.cs
+++ b/Darling/PerformanceMonitor.Darling.Service/PgPlanForceActionStore.cs
@@ -9,6 +9,7 @@
using System;
using System.Collections.Generic;
using System.Globalization;
+using System.Text.RegularExpressions;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Extensions.Logging;
@@ -408,7 +409,67 @@ ORDER BY pfa.action_time
return rows;
}
- /// The audit read behind get_plan_force_actions — newest first, optional server scope.
+ ///
+ /// Matches the ONE evidence line #4326 can produce for a null-state read failure —
+ /// state_unavailable: {CollectionFailure.Describe output} — an unattended force cannot proceed on
+ /// an unknown engine state — where the parenthesized reason is exactly a type name, optionally
+ /// with a SQLSTATE, followed by one of 's two fixed log notes.
+ /// Never ex.Message: a build before #4326 put the exception's own message in that same position
+ /// (PgPlanForceActionStore.TryGetTargetStatesAsync's old catch), and a real error message from
+ /// PostgreSQL or the driver essentially never happens to match this exact template, so failing the
+ /// match is the signal that a row is legacy. Every OTHER blocker's evidence line
+ /// (parameter_sensitivity_cofired, secondary_replica_evidence, apc_owns_it,
+ /// apc_enabled_for_database, and the empty-state shape of state_unavailable) has never
+ /// carried exception text on any build, so this pattern only ever needs to gate the one line that did.
+ ///
+ private static readonly Regex SafeStateUnavailableLine = new(
+ @"^state_unavailable: the forcing and automatic-plan-correction state read failed \(" +
+ @"[A-Za-z][A-Za-z0-9]*(, SQLSTATE [0-9A-Z]{5})?; " +
+ @"(the log has the full error|a table or column the read needs is missing, logged only at Debug level)\) " +
+ @"— an unattended force cannot proceed on an unknown engine state$",
+ RegexOptions.Compiled);
+
+ ///
+ /// The fixed sentence for a null-state read failure BEFORE #4326 fixed it (see
+ /// 's old catch and
+ /// 's), so a legacy row's evidence line
+ /// carries no exception text either — it says the SAME thing #4326 says now, minus the exception.
+ ///
+ private const string LegacyStateUnavailableLine =
+ "state_unavailable: the forcing and automatic-plan-correction state read failed — an unattended force cannot proceed on an unknown engine state";
+
+ ///
+ /// Rewrites line by line: any state_unavailable: line that does not
+ /// match is replaced with ;
+ /// every other line (there is at most one state_unavailable line per row — the bot journals one
+ /// decision per target) passes through unchanged, because no other blocker's evidence has ever carried
+ /// exception text. Null and non-multiline shapes (the withheld-force sentence, would_force's null) are
+ /// untouched — only state_unavailable: lines are ever redacted.
+ ///
+ internal static string? SanitizeDetailForAudit(string? detail)
+ {
+ if (detail is null)
+ {
+ return null;
+ }
+
+ var lines = detail.Split('\n');
+ for (var i = 0; i < lines.Length; i++)
+ {
+ if (lines[i].StartsWith("state_unavailable:", StringComparison.Ordinal) &&
+ !SafeStateUnavailableLine.IsMatch(lines[i]))
+ {
+ lines[i] = LegacyStateUnavailableLine;
+ }
+ }
+
+ return string.Join("\n", lines);
+ }
+
+ /// The audit read behind get_plan_force_actions — newest first, optional server scope.
+ /// #4346: every row's detail is passed through before it
+ /// leaves this method, so a row written before #4326 (which put the read failure's own exception
+ /// message here) cannot reach an MCP or web caller through this read.
public async Task> GetRecentActionsAsync(
int? serverId, DateTime sinceUtc, int limit, CancellationToken ct)
{
@@ -435,7 +496,8 @@ ORDER BY pfa.action_time DESC
await using var reader = await command.ExecuteReaderAsync(ct);
while (await reader.ReadAsync(ct))
{
- rows.Add(ReadRecord(reader));
+ var record = ReadRecord(reader);
+ rows.Add(record with { Detail = SanitizeDetailForAudit(record.Detail) });
}
return rows;
From b429e8e5a2f3c8646d905e610cea3b364827e0bc Mon Sep 17 00:00:00 2001
From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com>
Date: Fri, 25 Sep 2026 20:10:42 -0400
Subject: [PATCH 2/3] Widen the plan-force detail sanitizer to a full-block
anchor and cover GetPendingReviewsAsync (#4346)
The prior sanitizer matched one \n-delimited line at a time and applied
only to GetRecentActionsAsync. Two gaps: a pre-#4326 row's raw exception
message can itself embed a newline, in which case only the first line
was checked against the safe shape and everything after the embedded
newline passed through untouched; and GetPendingReviewsAsync, the
journal's other row reader, ran no sanitizer at all, so a caller added
to that read later would serve the same unredacted exception text.
SanitizeDetailForAudit now matches the whole state_unavailable BLOCK -
from its prefix up to the next known blocker's line or the end of the
string - and anchors the WHOLE block against the safe shape, not one
line of it. GetPendingReviewsAsync passes every row's detail through
the same sanitizer before returning it, at the same choke point as
GetRecentActionsAsync, so neither read can ever serve detail around it.
New pins: a legacy line that starts with the safe prefix and carries
exception text after it; the safe shape followed by a newline (then
CRLF) and exception text; a CRLF-terminated sibling blocker line ahead
of a safe line, which must still pass; and three characters .NET's
regex line anchors do not treat as line boundaries - a lone CR, and the
Unicode LINE SEPARATOR (U+2028) and PARAGRAPH SEPARATOR (U+2029) - none
of which may let smuggled text escape the redacted block. All existing
pins (the three post-fix shapes, the original legacy pin, the sibling
line, non-state_unavailable detail, null) keep passing.
---
.../PlanForceActionAuditRedactionTests.cs | 109 ++++++++++++++++++
.../PgPlanForceActionStore.cs | 56 ++++++---
2 files changed, 147 insertions(+), 18 deletions(-)
diff --git a/Darling/Darling.Tests/PlanForceActionAuditRedactionTests.cs b/Darling/Darling.Tests/PlanForceActionAuditRedactionTests.cs
index 7abf26aec0..b84d86cf19 100644
--- a/Darling/Darling.Tests/PlanForceActionAuditRedactionTests.cs
+++ b/Darling/Darling.Tests/PlanForceActionAuditRedactionTests.cs
@@ -81,4 +81,113 @@ public void NullDetail_StaysNull()
{
Assert.Null(PgPlanForceActionStore.SanitizeDetailForAudit(null));
}
+
+ /// pin (a) from the #4363 anchoring round: a legacy line that STARTS with the safe prefix but
+ /// carries exception text after the fixed sentence — the shape a naive prefix check (rather than a full
+ /// anchor) would have let through.
+ [Fact]
+ public void LegacyLine_SafePrefixWithTrailingExceptionText_IsRedacted()
+ {
+ var legacy = "state_unavailable: the forcing and automatic-plan-correction state read failed (NpgsqlException; the log has the full error) extra: password authentication failed for user \"darling_ro\" at host db-primary-02.internal \u2014 an unattended force cannot proceed on an unknown engine state";
+ var sanitized = PgPlanForceActionStore.SanitizeDetailForAudit(legacy)!;
+
+ Assert.DoesNotContain("darling_ro", sanitized);
+ Assert.DoesNotContain("db-primary-02", sanitized);
+ Assert.DoesNotContain("password authentication failed", sanitized);
+ Assert.Equal(
+ "state_unavailable: the forcing and automatic-plan-correction state read failed \u2014 an unattended force cannot proceed on an unknown engine state",
+ sanitized);
+ }
+
+ /// pin (b): the safe shape followed by a newline and exception text — an embedded newline in
+ /// a legacy exception message must not let the second line escape as if it were a sibling blocker's
+ /// evidence line.
+ [Fact]
+ public void LegacyLine_SafeShapeFollowedByNewlineAndExceptionText_IsRedacted()
+ {
+ var safeLine = "state_unavailable: the forcing and automatic-plan-correction state read failed (NpgsqlException; the log has the full error) \u2014 an unattended force cannot proceed on an unknown engine state";
+ var smuggled = safeLine + "\nleaked: password=hunter2 host=db-primary-02.internal";
+ var sanitized = PgPlanForceActionStore.SanitizeDetailForAudit(smuggled)!;
+
+ Assert.DoesNotContain("hunter2", sanitized);
+ Assert.DoesNotContain("db-primary-02", sanitized);
+ Assert.Equal(
+ "state_unavailable: the forcing and automatic-plan-correction state read failed \u2014 an unattended force cannot proceed on an unknown engine state",
+ sanitized);
+ }
+
+ /// pin (c), safe half: a CRLF-terminated sibling blocker line ahead of the safe shape must not
+ /// stop the safe shape from passing — the anchor treats the position right after '\n' as the line start
+ /// regardless of a preceding '\r'.
+ [Fact]
+ public void CrlfPrecededSafeLine_StillPassesThrough()
+ {
+ var safeLine = "state_unavailable: the forcing and automatic-plan-correction state read failed (NpgsqlException; the log has the full error) \u2014 an unattended force cannot proceed on an unknown engine state";
+ var detail = "apc_owns_it: query_store_stats: plan 7 is_forced_plan = true\r\n" + safeLine;
+ var sanitized = PgPlanForceActionStore.SanitizeDetailForAudit(detail)!;
+
+ Assert.Equal(detail, sanitized);
+ }
+
+ /// pin (c), unsafe half: a CRLF-joined legacy line still gets redacted.
+ [Fact]
+ public void CrlfSmuggledExceptionText_IsRedacted()
+ {
+ var safeLine = "state_unavailable: the forcing and automatic-plan-correction state read failed (NpgsqlException; the log has the full error) \u2014 an unattended force cannot proceed on an unknown engine state";
+ var smuggled = safeLine + "\r\nleaked: password=hunter2 host=db-primary-02.internal";
+ var sanitized = PgPlanForceActionStore.SanitizeDetailForAudit(smuggled)!;
+
+ Assert.DoesNotContain("hunter2", sanitized);
+ Assert.DoesNotContain("db-primary-02", sanitized);
+ }
+
+ /// A lone '\r' between the safe shape and smuggled text is not a line boundary to .NET's
+ /// '^'/'\z' under Multiline, so the smuggled text stays inside the redacted block rather than
+ /// surviving as if it were a new line.
+ [Fact]
+ public void LoneCrSmuggledExceptionText_IsRedacted()
+ {
+ var safeLine = "state_unavailable: the forcing and automatic-plan-correction state read failed (NpgsqlException; the log has the full error) \u2014 an unattended force cannot proceed on an unknown engine state";
+ var smuggled = safeLine + "\rleaked: password=hunter2";
+ var sanitized = PgPlanForceActionStore.SanitizeDetailForAudit(smuggled)!;
+
+ Assert.DoesNotContain("hunter2", sanitized);
+ }
+
+ /// The Unicode LINE SEPARATOR (U+2028) is not a line boundary to .NET's '^'/'\z' either, so
+ /// smuggled text after one stays inside the redacted block.
+ [Fact]
+ public void UnicodeLineSeparatorSmuggledExceptionText_IsRedacted()
+ {
+ var safeLine = "state_unavailable: the forcing and automatic-plan-correction state read failed (NpgsqlException; the log has the full error) \u2014 an unattended force cannot proceed on an unknown engine state";
+ var smuggled = safeLine + "\u2028leaked: password=hunter2";
+ var sanitized = PgPlanForceActionStore.SanitizeDetailForAudit(smuggled)!;
+
+ Assert.DoesNotContain("hunter2", sanitized);
+ }
+
+ /// Same as the LINE SEPARATOR pin, for the Unicode PARAGRAPH SEPARATOR (U+2029).
+ [Fact]
+ public void UnicodeParagraphSeparatorSmuggledExceptionText_IsRedacted()
+ {
+ var safeLine = "state_unavailable: the forcing and automatic-plan-correction state read failed (NpgsqlException; the log has the full error) \u2014 an unattended force cannot proceed on an unknown engine state";
+ var smuggled = safeLine + "\u2029leaked: password=hunter2";
+ var sanitized = PgPlanForceActionStore.SanitizeDetailForAudit(smuggled)!;
+
+ Assert.DoesNotContain("hunter2", sanitized);
+ }
+
+ /// A sibling blocker's line on the same multi-line detail still survives redaction of an
+ /// adjacent, unsafe state_unavailable block — the block match must not eat past the next blocker's
+ /// prefix.
+ [Fact]
+ public void SiblingBlockerLine_SurvivesEvenWithAdjacentRedaction()
+ {
+ var detail = "state_unavailable: garbage (Exception: whatever leaked)\napc_owns_it: query_store_stats: plan 7 is_forced_plan = true";
+ var sanitized = PgPlanForceActionStore.SanitizeDetailForAudit(detail)!;
+ var lines = sanitized.Split('\n');
+
+ Assert.DoesNotContain("whatever leaked", sanitized);
+ Assert.Equal("apc_owns_it: query_store_stats: plan 7 is_forced_plan = true", lines[1]);
+ }
}
diff --git a/Darling/PerformanceMonitor.Darling.Service/PgPlanForceActionStore.cs b/Darling/PerformanceMonitor.Darling.Service/PgPlanForceActionStore.cs
index b8c0bf6610..5b35ecc7df 100644
--- a/Darling/PerformanceMonitor.Darling.Service/PgPlanForceActionStore.cs
+++ b/Darling/PerformanceMonitor.Darling.Service/PgPlanForceActionStore.cs
@@ -353,6 +353,12 @@ AND pfa.action_time > $5
/// that calls it because these two shapes — and the grace window that separates an orphan from a
/// force still being written — are properties of the TABLE, and the only place they can be shown
/// to hold is against a live store, which is what PlanForceActionStoreTests does.
+ ///
+ /// #4346: every row's detail is passed through before
+ /// it leaves this method too, on the same reasoning as — this read
+ /// has no caller today, but a row written before #4326 sits in the table regardless, and the day a
+ /// review surface calls this it must not be the day someone notices the exception text it has been
+ /// quietly carrying since before this method had a caller.
///
public async Task> GetPendingReviewsAsync(
int serverId, DateTime nowUtc, CancellationToken ct)
@@ -403,7 +409,8 @@ ORDER BY pfa.action_time
await using var reader = await command.ExecuteReaderAsync(ct);
while (await reader.ReadAsync(ct))
{
- rows.Add(ReadRecord(reader));
+ var record = ReadRecord(reader);
+ rows.Add(record with { Detail = SanitizeDetailForAudit(record.Detail) });
}
return rows;
@@ -439,12 +446,34 @@ ORDER BY pfa.action_time
"state_unavailable: the forcing and automatic-plan-correction state read failed — an unattended force cannot proceed on an unknown engine state";
///
- /// Rewrites line by line: any state_unavailable: line that does not
- /// match is replaced with ;
- /// every other line (there is at most one state_unavailable line per row — the bot journals one
- /// decision per target) passes through unchanged, because no other blocker's evidence has ever carried
- /// exception text. Null and non-multiline shapes (the withheld-force sentence, would_force's null) are
- /// untouched — only state_unavailable: lines are ever redacted.
+ /// Matches the whole state_unavailable BLOCK — from its prefix up to (not including) the start
+ /// of the next known blocker's line, or the end of the string, whichever comes first — rather than one
+ /// \n-delimited line. A pre-#4326 row's raw exception message can itself embed a newline (a
+ /// driver's multi-line message, a wrapped stack fragment), and matching only the first line would leave
+ /// everything after that embedded newline unredacted; #4326 itself never wrote more than one line for
+ /// this blocker, so a post-fix row's block is always exactly its one safe line and nothing here widens
+ /// what a clean row matches.
+ ///
+ /// makes . match a newline so the lazy body can span
+ /// one; anchors ^ right after a \n (which also covers
+ /// a \r\n pair, since the position immediately after \n is the same either way) for the
+ /// next-blocker lookahead. A lone \r, or a Unicode line separator (U+2028/U+2029), is deliberately
+ /// NOT a line boundary to .NET's ^/$ under these options — only \n is — so text
+ /// smuggled after one of those does not count as the start of the next blocker's line and stays inside
+ /// this block, where it is redacted with everything else in it instead of surviving past the anchor.
+ ///
+ private static readonly Regex StateUnavailableBlock = new(
+ @"^state_unavailable:.*?(?=\r?\n(?:parameter_sensitivity_cofired|secondary_replica_evidence|apc_owns_it|apc_enabled_for_database|state_unavailable):|\z)",
+ RegexOptions.Compiled | RegexOptions.Singleline | RegexOptions.Multiline);
+
+ ///
+ /// Rewrites : every span that does not
+ /// match is replaced whole with
+ /// ; every other blocker's line (this row's other named
+ /// blockers — parameter_sensitivity_cofired, secondary_replica_evidence, apc_owns_it,
+ /// apc_enabled_for_database) passes through unchanged, because none of them has ever carried
+ /// exception text on any build. Null and non-state_unavailable shapes (the withheld-force
+ /// sentence, would_force's null) are untouched — only a state_unavailable block is ever redacted.
///
internal static string? SanitizeDetailForAudit(string? detail)
{
@@ -453,17 +482,8 @@ ORDER BY pfa.action_time
return null;
}
- var lines = detail.Split('\n');
- for (var i = 0; i < lines.Length; i++)
- {
- if (lines[i].StartsWith("state_unavailable:", StringComparison.Ordinal) &&
- !SafeStateUnavailableLine.IsMatch(lines[i]))
- {
- lines[i] = LegacyStateUnavailableLine;
- }
- }
-
- return string.Join("\n", lines);
+ return StateUnavailableBlock.Replace(detail, m =>
+ SafeStateUnavailableLine.IsMatch(m.Value) ? m.Value : LegacyStateUnavailableLine);
}
/// The audit read behind get_plan_force_actions — newest first, optional server scope.
From 6b6421fdc0f0c2b17d19b7bb789aa5101c1b0cde Mon Sep 17 00:00:00 2001
From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com>
Date: Fri, 25 Sep 2026 20:46:16 -0400
Subject: [PATCH 3/3] Fix F1 sibling-prefix bypass and F2 false rewrites in the
plan-force audit sanitizer (#4363)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
F1 (HIGH): StateUnavailableBlock's sibling-prefix lookahead let a legacy
exception message ending with \napc_owns_it: (or any other sibling's exact
prefix) end the redacted block early, passing everything after it through
verbatim. ForcePlanBotPolicy.Blockers always adds state_unavailable LAST
(apc_owns_it/apc_enabled_for_database both require a non-null, non-empty
state and cannot co-occur with the null/empty-state condition that adds
state_unavailable — confirmed in FactRemediation.ForcePlanBlockers and
ForcePlanBotPolicy.Blockers), so the block can be anchored to \z instead of
a lookahead. SiblingBlockerLine_SurvivesEvenWithAdjacentRedaction pinned the
writer's real order backwards; replaced with
LegacyLine_EmbeddingASiblingPrefix_IsRedactedToTheEnd, which pins the actual
order (sibling before state_unavailable) and asserts the impersonating text
is redacted to the end of the string.
F2 (MEDIUM): SafeStateUnavailableLine only recognized the #4326 exception
read-failure shape, so the null-state "returned no row" and empty-state
"ran and observed nothing" shapes (which never carried exception text, and
the empty-state shape is the common production case) were rewritten to a
false "read failed" sentence. Widened to a three-branch regex covering all
three shapes ForcePlanBotPolicy.Blockers can write. New pins
(NoRowShape_FromTheWriter_PassesThroughUnchanged,
EmptyStateShape_FromTheWriter_PassesThroughUnchanged) build their input from
ForcePlanBotPolicy.Blockers/Evidence directly rather than copied literals, so
wording drift in the writer turns them red. Added
BothReads_ApplyTheSameSanitizer as the missing pin that both
GetRecentActionsAsync and GetPendingReviewsAsync apply the sanitizer.
Verified with a throwaway net10.0 console linking a verbatim copy of both
regex versions: the F1 bypass input leaks fake secrets on the old regex and
is fully redacted on the new one; both F2 shapes are over-redacted on the old
regex and pass through unchanged on the new one.
---
.../PlanForceActionAuditRedactionTests.cs | 85 +++++++++++++++++--
.../PgPlanForceActionStore.cs | 70 ++++++++-------
2 files changed, 117 insertions(+), 38 deletions(-)
diff --git a/Darling/Darling.Tests/PlanForceActionAuditRedactionTests.cs b/Darling/Darling.Tests/PlanForceActionAuditRedactionTests.cs
index b84d86cf19..a87241627c 100644
--- a/Darling/Darling.Tests/PlanForceActionAuditRedactionTests.cs
+++ b/Darling/Darling.Tests/PlanForceActionAuditRedactionTests.cs
@@ -6,6 +6,7 @@
* Licensed under the MIT License. See LICENSE file in the project root for full license information.
*/
+using PerformanceMonitor.Analysis;
using PerformanceMonitor.Darling.Service;
using Xunit;
@@ -177,17 +178,83 @@ public void UnicodeParagraphSeparatorSmuggledExceptionText_IsRedacted()
Assert.DoesNotContain("hunter2", sanitized);
}
- /// A sibling blocker's line on the same multi-line detail still survives redaction of an
- /// adjacent, unsafe state_unavailable block — the block match must not eat past the next blocker's
- /// prefix.
+ /// F1 (#4363 security round): ForcePlanBotPolicy.Blockers always adds
+ /// state_unavailable LAST — apc_owns_it/apc_enabled_for_database both require a
+ /// non-null, non-empty state and cannot co-occur with the null/empty-state condition that adds
+ /// state_unavailable — so a real detail never has a sibling blocker AFTER it. A pre-#4326 row's
+ /// raw exception message CAN embed "\n" + "apc_owns_it:" (or any other sibling's exact prefix)
+ /// as free text; a sibling-prefix lookahead would have ended the block there and let everything after
+ /// it — unredacted — through. The fix anchors the block to \z instead: everything from the
+ /// legacy row's state_unavailable: onward is redacted, including the impersonating text.
[Fact]
- public void SiblingBlockerLine_SurvivesEvenWithAdjacentRedaction()
+ public void LegacyLine_EmbeddingASiblingPrefix_IsRedactedToTheEnd()
{
- var detail = "state_unavailable: garbage (Exception: whatever leaked)\napc_owns_it: query_store_stats: plan 7 is_forced_plan = true";
- var sanitized = PgPlanForceActionStore.SanitizeDetailForAudit(detail)!;
- var lines = sanitized.Split('\n');
+ var legacy = "apc_owns_it: query_store_stats: plan 7 is_forced_plan = true\n" +
+ "state_unavailable: the forcing and automatic-plan-correction state read failed (PostgresException: 28P01: password authentication failed for user \"fake_user\"\n" +
+ "apc_owns_it: host=db-fake-01.example password=hunter2) \u2014 an unattended force cannot proceed on an unknown engine state";
+ var sanitized = PgPlanForceActionStore.SanitizeDetailForAudit(legacy)!;
+
+ Assert.DoesNotContain("hunter2", sanitized);
+ Assert.DoesNotContain("db-fake-01", sanitized);
+ Assert.DoesNotContain("fake_user", sanitized);
+ Assert.StartsWith("apc_owns_it: query_store_stats: plan 7 is_forced_plan = true\n", sanitized);
+ Assert.Equal(
+ "apc_owns_it: query_store_stats: plan 7 is_forced_plan = true\n" +
+ "state_unavailable: the forcing and automatic-plan-correction state read failed \u2014 an unattended force cannot proceed on an unknown engine state",
+ sanitized);
+ }
+
+ /// F2 (#4363 security round): the null-state "returned no row" shape
+ /// (ForcePlanBotPolicy.Blockers(target, null, null)) never carried exception text, and must
+ /// pass through unchanged — built from the writer itself so wording drift there turns this test red
+ /// instead of silently over-redacting.
+ [Fact]
+ public void NoRowShape_FromTheWriter_PassesThroughUnchanged()
+ {
+ var target = new ForcePlanTarget(Database: "db1", QueryId: 42, PlanId: 7);
+ var blockers = ForcePlanBotPolicy.Blockers(target, state: null, stateUnavailableReason: null);
+ var detail = ForcePlanBotPolicy.Evidence(blockers)!;
+
+ Assert.Equal(detail, PgPlanForceActionStore.SanitizeDetailForAudit(detail));
+ Assert.Contains("returned no row for plan 7 of query 42 in db1", detail);
+ }
+
+ /// F2: the empty-state "ran and observed nothing" shape
+ /// (ForcePlanBotPolicy.Blockers(target, emptyState, null)) — the common production case —
+ /// never carried exception text either, and must pass through unchanged.
+ [Fact]
+ public void EmptyStateShape_FromTheWriter_PassesThroughUnchanged()
+ {
+ var target = new ForcePlanTarget(Database: "db1", QueryId: 42, PlanId: 7);
+ var emptyState = new ForcePlanTargetState(
+ PlanIsForced: null, PlanForcingType: null, ForceFailureCount: null, LastForceFailureReason: null,
+ PlanObservedAtUtc: null, OtherForcedPlanId: null, OtherForcedPlanForcingType: null,
+ OtherForcedPlanObservedAtUtc: null, ApcState: null, ApcStateReason: null, ApcRegressedPlanId: null,
+ ApcLastGoodPlanId: null, ApcLastGoodPlanForcingType: null, ApcLastGoodPlanIsForced: null,
+ ApcLastGoodPlanForceFailureReason: null, ApcExecuteActionInitiatedBy: null, ApcObservedAtUtc: null,
+ ForceLastGoodPlanActualState: null, EnablementObservedAtUtc: null);
+ Assert.True(emptyState.IsEmpty);
+
+ var blockers = ForcePlanBotPolicy.Blockers(target, emptyState, stateUnavailableReason: null);
+ var detail = ForcePlanBotPolicy.Evidence(blockers)!;
+
+ Assert.Equal(detail, PgPlanForceActionStore.SanitizeDetailForAudit(detail));
+ Assert.Contains("ran and observed nothing for this target inside the last", detail);
+ }
+
+ /// Both journal reads apply the sanitizer — the review's missing pin. Exercises
+ /// the same way both
+ /// GetRecentActionsAsync and GetPendingReviewsAsync do at their respective call sites
+ /// (record with { Detail = SanitizeDetailForAudit(record.Detail) }), pinning that a legacy
+ /// exception-bearing line never survives either read's output shape.
+ [Fact]
+ public void BothReads_ApplyTheSameSanitizer()
+ {
+ var legacy = "state_unavailable: the forcing and automatic-plan-correction state read failed (NpgsqlException: 28P01: password authentication failed for user \"darling_ro\" at host db-primary-02.internal)";
+ var recentActionsShape = legacy;
+ var pendingReviewShape = legacy;
- Assert.DoesNotContain("whatever leaked", sanitized);
- Assert.Equal("apc_owns_it: query_store_stats: plan 7 is_forced_plan = true", lines[1]);
+ Assert.DoesNotContain("db-primary-02", PgPlanForceActionStore.SanitizeDetailForAudit(recentActionsShape));
+ Assert.DoesNotContain("db-primary-02", PgPlanForceActionStore.SanitizeDetailForAudit(pendingReviewShape));
}
}
diff --git a/Darling/PerformanceMonitor.Darling.Service/PgPlanForceActionStore.cs b/Darling/PerformanceMonitor.Darling.Service/PgPlanForceActionStore.cs
index 5b35ecc7df..e68ff1354a 100644
--- a/Darling/PerformanceMonitor.Darling.Service/PgPlanForceActionStore.cs
+++ b/Darling/PerformanceMonitor.Darling.Service/PgPlanForceActionStore.cs
@@ -417,24 +417,27 @@ ORDER BY pfa.action_time
}
///
- /// Matches the ONE evidence line #4326 can produce for a null-state read failure —
- /// state_unavailable: {CollectionFailure.Describe output} — an unattended force cannot proceed on
- /// an unknown engine state — where the parenthesized reason is exactly a type name, optionally
- /// with a SQLSTATE, followed by one of 's two fixed log notes.
- /// Never ex.Message: a build before #4326 put the exception's own message in that same position
+ /// Matches the THREE evidence shapes can put in a
+ /// state_unavailable block, and only those: the #4326 read-failure line (parenthesized reason
+ /// is exactly a type name, optionally with a SQLSTATE, followed by one of
+ /// 's two fixed log notes); the null-state "returned no row"
+ /// line; and the empty-state "ran and observed nothing" line — both written straight from
+ /// target/state fields, never from an exception. Never ex.Message: a build before
+ /// #4326 put the exception's own message in the read-failure line's position
/// (PgPlanForceActionStore.TryGetTargetStatesAsync's old catch), and a real error message from
- /// PostgreSQL or the driver essentially never happens to match this exact template, so failing the
- /// match is the signal that a row is legacy. Every OTHER blocker's evidence line
+ /// PostgreSQL or the driver essentially never happens to match any of these three exact templates, so
+ /// failing the match is the signal that a row is legacy. Every OTHER blocker's evidence line
/// (parameter_sensitivity_cofired, secondary_replica_evidence, apc_owns_it,
- /// apc_enabled_for_database, and the empty-state shape of state_unavailable) has never
- /// carried exception text on any build, so this pattern only ever needs to gate the one line that did.
+ /// apc_enabled_for_database) has never carried exception text on any build, so this pattern only
+ /// ever needs to gate the one blocker that did.
///
private static readonly Regex SafeStateUnavailableLine = new(
- @"^state_unavailable: the forcing and automatic-plan-correction state read failed \(" +
- @"[A-Za-z][A-Za-z0-9]*(, SQLSTATE [0-9A-Z]{5})?; " +
- @"(the log has the full error|a table or column the read needs is missing, logged only at Debug level)\) " +
- @"— an unattended force cannot proceed on an unknown engine state$",
- RegexOptions.Compiled);
+ @"\Astate_unavailable: (?:" +
+ @"the forcing and automatic-plan-correction state read failed \([A-Za-z][A-Za-z0-9]*(?:, SQLSTATE [0-9A-Z]{5})?; (?:the log has the full error|a table or column the read needs is missing, logged only at Debug level)\) — an unattended force cannot proceed on an unknown engine state" +
+ @"|the forcing and automatic-plan-correction state read returned no row for plan -?[0-9]+ of query -?[0-9]+ in [^\r\n\u2028\u2029]{1,256}; an unattended force cannot proceed on an unknown engine state" +
+ @"|the forcing and automatic-plan-correction state read ran and observed nothing for this target inside the last [0-9]+ hours: no query_store_stats row for plan -?[0-9]+, no forced sibling plan of query -?[0-9]+, no plan_correction recommendation, and no plan_correction capture for [^\r\n\u2028\u2029]{1,256} at all — FORCE_LAST_GOOD_PLAN enablement is unknown for this database, and an unattended force cannot proceed on unknown" +
+ @")\z",
+ RegexOptions.Compiled | RegexOptions.CultureInvariant);
///
/// The fixed sentence for a null-state read failure BEFORE #4326 fixed it (see
@@ -446,24 +449,33 @@ ORDER BY pfa.action_time
"state_unavailable: the forcing and automatic-plan-correction state read failed — an unattended force cannot proceed on an unknown engine state";
///
- /// Matches the whole state_unavailable BLOCK — from its prefix up to (not including) the start
- /// of the next known blocker's line, or the end of the string, whichever comes first — rather than one
- /// \n-delimited line. A pre-#4326 row's raw exception message can itself embed a newline (a
- /// driver's multi-line message, a wrapped stack fragment), and matching only the first line would leave
- /// everything after that embedded newline unredacted; #4326 itself never wrote more than one line for
- /// this blocker, so a post-fix row's block is always exactly its one safe line and nothing here widens
- /// what a clean row matches.
+ /// Matches the whole state_unavailable BLOCK — from its prefix to the END of the string, not to
+ /// the next sibling blocker's line. always adds
+ /// state_unavailable LAST: it fires only when state is null or empty
+ /// (PgPlanForceActionStore.cs, this file), while apc_owns_it and
+ /// apc_enabled_for_database both require a non-null, non-empty state
+ /// (FactRemediation.ForcePlanBlockers, PerformanceMonitor.Analysis/FactRemediation.cs
+ /// ~1019-1043, checked via if (state is null || state.IsEmpty) return blockers; before either
+ /// APC blocker can be added; ForcePlanBotPolicy.Blockers,
+ /// PerformanceMonitor.Analysis/ForcePlanBotPolicy.cs ~291-317, adds
+ /// apc_enabled_for_database only under state is { ApcIsOn: true } and
+ /// state_unavailable only under state is null or state.IsEmpty). So the two APC
+ /// blockers can never co-occur with state_unavailable, and it is always the final block — a
+ /// sibling-prefix LOOKAHEAD is not needed and is actively unsafe: a pre-#4326 row's raw exception
+ /// message is free text and can itself contain "\n" + "apc_owns_it:" (or any other sibling's
+ /// exact prefix) by coincidence or by an attacker who controls part of the upstream error message, and
+ /// a lookahead-bounded block would end there, leaving everything after it — unredacted — outside the
+ /// match and passed through verbatim. Anchoring to \z instead removes that seam: nothing after
+ /// state_unavailable: can end the block early, because there is no more string in which the next
+ /// blocker legitimately fires.
///
- /// makes . match a newline so the lazy body can span
- /// one; anchors ^ right after a \n (which also covers
- /// a \r\n pair, since the position immediately after \n is the same either way) for the
- /// next-blocker lookahead. A lone \r, or a Unicode line separator (U+2028/U+2029), is deliberately
- /// NOT a line boundary to .NET's ^/$ under these options — only \n is — so text
- /// smuggled after one of those does not count as the start of the next blocker's line and stays inside
- /// this block, where it is redacted with everything else in it instead of surviving past the anchor.
+ /// makes . match a newline so the body can span one;
+ /// anchors ^ right after a \n (which also covers a
+ /// \r\n pair, since the position immediately after \n is the same either way) so the
+ /// block is found even when it is not the first line in detail.
///
private static readonly Regex StateUnavailableBlock = new(
- @"^state_unavailable:.*?(?=\r?\n(?:parameter_sensitivity_cofired|secondary_replica_evidence|apc_owns_it|apc_enabled_for_database|state_unavailable):|\z)",
+ @"^state_unavailable:.*\z",
RegexOptions.Compiled | RegexOptions.Singleline | RegexOptions.Multiline);
///