Pin every STORE connection's session time zone to UTC (#4277)

Darling/Darling.Tests/DarlingStoreLoginsTests.cs

@@ -847,7 +847,13 @@ public void EachHost_BuildsItsUnmanagedPoolFromTheResolver_AndItsManagedPoolAsBe
         var start = Body(code, "private async Task<bool> TryStartServerAsync(");
 
         Assert.DoesNotContain("config.Postgres.ConnectionString", start, StringComparison.Ordinal);
-        Assert.Contains("NpgsqlDataSource.Create(storeConnectionString)", start, StringComparison.Ordinal);
+
+        /* #4277: every STORE data source pins its session timezone to UTC, so the resolved
+           storeConnectionString is wrapped before it reaches Create rather than passed bare. */
+        Assert.Contains(
+            "NpgsqlDataSource.Create(DarlingStoreConnection.PinSessionTimeZoneUtc(storeConnectionString))",
+            start,
+            StringComparison.Ordinal);
 
         var assignments = Regex.Matches(start, @"storeConnectionString\s*=(?!=)").Count;
         Assert.Equal(2, assignments);

Darling/Darling.Tests/StoreConnectionStringAppendPinTests.cs

@@ -0,0 +1,140 @@
+/*
+ * 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 System;
+using System.Linq;
+using Npgsql;
+using PerformanceMonitor.Darling.Service;
+using PerformanceMonitor.Darling.Storage;
+using PerformanceMonitor.Darling.Viewer;
+using Xunit;
+
+namespace Darling.Tests;
+
+/// <summary>
+/// Round-1 security review on #4285 (the #4277 PR): <see cref="DarlingStoreConnection.PinSessionTimeZoneUtc"/>,
+/// <see cref="DarlingWorker.EnsureStoreSearchPath"/> and <see cref="ViewerDataService.ApplyConnectionTimeout"/>
+/// used to set their one keyword by rewriting the caller's connection string through a builder. A builder's
+/// writer emits an explicit empty value (<c>Password=''</c>) as a bare <c>Key=</c>, and its own reader treats a
+/// bare <c>Key=</c> as "not set" — silently dropping the keyword and letting Npgsql fall back to a <c>PG*</c>
+/// environment variable or a default file where the caller's empty value meant "never fall back". All three now
+/// APPEND their one keyword to the caller's own string instead. This is the harness the review's fix
+/// recommendation described running against 24 inputs, committed: every OTHER keyword in the caller's string,
+/// including an explicit empty one, must survive byte for byte, and the output must literally start with the
+/// caller's own string.
+/// </summary>
+public sealed class StoreConnectionStringAppendPinTests
+{
+    /// <summary>Each row is independently valid Npgsql connection-string content and exercises one of the
+    /// round-1 review's risk shapes. None of the five sets Timezone, Search Path or Timeout, so all three
+    /// helpers see their own keyword as ABSENT on every row (the "already present" branch of each helper is
+    /// covered separately, below and in the pre-existing helper tests).</summary>
+    public static TheoryData<string, string> RiskyConnectionStrings() => new()
+    {
+        { "quoted-empty Password", "Host=store;Database=darling;Password=''" },
+        { "double-quoted-empty Root Certificate", "Host=store;Database=darling;Root Certificate=\"\"" },
+        { "password holding ; ' \" and =", "Host=store;Database=darling;Password='ab;cd''ef\"gh=ij'" },
+        { "trailing semicolon", "Host=store;Database=darling;" },
+        { "trailing spaces", "Host=store;Database=darling   " },
+    };
+
+    [Theory]
+    [MemberData(nameof(RiskyConnectionStrings))]
+    public void PinSessionTimeZoneUtc_AppendsWithoutErasingAnyOtherKeyword(string label, string input)
+    {
+        var output = DarlingStoreConnection.PinSessionTimeZoneUtc(input);
+
+        AssertAppendedAndEveryOtherKeywordSurvives(input, output, label);
+        Assert.Equal("UTC", new NpgsqlConnectionStringBuilder(output).Timezone);
+    }
+
+    [Theory]
+    [MemberData(nameof(RiskyConnectionStrings))]
+    public void EnsureStoreSearchPath_AppendsWithoutErasingAnyOtherKeyword(string label, string input)
+    {
+        var output = DarlingWorker.EnsureStoreSearchPath(input);
+
+        AssertAppendedAndEveryOtherKeywordSurvives(input, output, label);
+        Assert.Equal(DarlingManagedPostgres.SearchPath, new NpgsqlConnectionStringBuilder(output).SearchPath);
+    }
+
+    [Theory]
+    [MemberData(nameof(RiskyConnectionStrings))]
+    public void ApplyConnectionTimeout_AppendsWithoutErasingAnyOtherKeyword(string label, string input)
+    {
+        var output = ViewerDataService.ApplyConnectionTimeout(input, 42);
+
+        AssertAppendedAndEveryOtherKeywordSurvives(input, output, label);
+        Assert.Equal(42, new NpgsqlConnectionStringBuilder(output).Timeout);
+    }
+
+    /// <summary>#4285 round-1 Low 1, generalized to all three helpers: the parsed value of every keyword the
+    /// CALLER's string sets — including an explicit empty one, which a builder round trip erases — must be
+    /// identical before and after, and the output must literally start with the caller's own string (an
+    /// append, never a rewrite).</summary>
+    private static void AssertAppendedAndEveryOtherKeywordSurvives(string input, string output, string label)
+    {
+        Assert.True(
+            output.StartsWith(input, StringComparison.Ordinal),
+            $"{label}: output does not start with the caller's own string — input=[{input}] output=[{output}]");
+
+        var before = new NpgsqlConnectionStringBuilder(input);
+        var after = new NpgsqlConnectionStringBuilder(output);
+
+        foreach (var key in before.Keys.Cast<string>())
+        {
+            Assert.True(after.ContainsKey(key), $"{label}: keyword '{key}' is missing from the output");
+            Assert.Equal(before[key], after[key]);
+        }
+    }
+
+    /// <summary>The pin always wins, even over an existing Timezone spelled in odd case — Npgsql keeps only the
+    /// LAST value for a repeated keyword, so the appended one, being last, wins regardless of the existing
+    /// key's case.</summary>
+    [Fact]
+    public void PinSessionTimeZoneUtc_OverwritesAnExistingTimezoneInOddCase()
+    {
+        const string input = "Host=store;Database=darling;TIMEZONE=America/New_York";
+
+        var output = DarlingStoreConnection.PinSessionTimeZoneUtc(input);
+
+        var parsed = new NpgsqlConnectionStringBuilder(output);
+        Assert.Equal("UTC", parsed.Timezone);
+        Assert.Equal("darling", parsed.Database);
+    }
+
+    /// <summary>Detection (<see cref="NpgsqlConnectionStringBuilder.SearchPath"/>) is alias-aware and
+    /// case-insensitive, so an odd-case existing Search Path still counts as present, and the string is
+    /// returned unchanged rather than double-appended.</summary>
+    [Fact]
+    public void EnsureStoreSearchPath_DetectsAnExistingSearchPathInOddCase_AndLeavesItUnchanged()
+    {
+        const string input = "Host=store;Database=darling;SEARCHPATH=reporting,public";
+
+        var output = DarlingWorker.EnsureStoreSearchPath(input);
+
+        Assert.Equal(input, output);
+        Assert.Equal("reporting,public", new NpgsqlConnectionStringBuilder(output).SearchPath);
+    }
+
+    /// <summary>
+    /// <see cref="DarlingStoreConnection.PinSessionTimeZoneUtc"/> and <see cref="DarlingWorker.EnsureStoreSearchPath"/>
+    /// both still parse the caller's string once, through <see cref="NpgsqlConnectionStringBuilder"/>, before
+    /// deciding what to append — <c>PinSessionTimeZoneUtc</c> to keep today's malformed-string exception at
+    /// this exact statement (round-1 review's Q2), <c>EnsureStoreSearchPath</c> as its unchanged presence
+    /// check. Pinned so neither parse is later mistaken for dead code and deleted.
+    /// </summary>
+    [Fact]
+    public void PinSessionTimeZoneUtcAndEnsureStoreSearchPath_StillThrowOnAMalformedString()
+    {
+        const string malformed = "Host=store;sslmode=NotARealSslMode";
+
+        Assert.Throws<ArgumentException>(() => DarlingStoreConnection.PinSessionTimeZoneUtc(malformed));
+        Assert.Throws<ArgumentException>(() => DarlingWorker.EnsureStoreSearchPath(malformed));
+    }
+}

Darling/Darling.Tests/StoreSessionTimeZoneLiveTests.cs

@@ -0,0 +1,129 @@
+/*
+ * 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 System;
+using System.Threading;
+using System.Threading.Tasks;
+using Npgsql;
+using PerformanceMonitor.Darling.Storage;
+using Xunit;
+
+namespace Darling.Tests;
+
+/// <summary>
+/// #4277 live proof: a STORE connection opened the way the product opens one
+/// (<c>DarlingStoreConnection.PinSessionTimeZoneUtc</c>) reports session <c>timezone</c> UTC even when the
+/// DATABASE's own default is <c>America/New_York</c> — proven on a #1776 own-store scratch database
+/// (<c>ALTER DATABASE ... SET timezone</c>, not the shared rig conf, so it means something on a UTC CI
+/// runner too, not only a rig deliberately set to New York).
+///
+/// <para>The second assertion exercises the one bare-<c>now()</c> store SQL predicate
+/// <see cref="StoreSqlClockDisciplineTests"/> waives on ARITHMETIC margin, not on zone-correctness —
+/// <see cref="DarlingModuleMap.RefreshSql"/>'s <c>collection_time >= now() - interval '2 days'</c>. Pinned to
+/// that exact text by an <c>Assert.Contains</c> so a future change to the interval cannot silently stale this
+/// proof. Two rows straddle the 48h boundary with a 2h margin either side (inside: 46h old, outside: 50h
+/// old) rather than one row safely in the middle, because a naive column read under a WEST-of-UTC session
+/// (America/New_York is UTC-4 in September) is interpreted as YOUNGER than its true UTC meaning — the window
+/// only ever WIDENS, so it only ever ADMITS more rows. A single "inside" row stays admitted under either
+/// session and cannot discriminate; only the "outside" row's wrong admission does — which is also why this
+/// predicate is safely waived rather than a bug: RefreshSql's own remarks show its worst case (34-60h) still
+/// covers what it needs regardless of offset, but that tolerance is a fact about ITS margin, not a licence
+/// for a naive-to-now() comparison to ignore the session zone in general.</para>
+/// </summary>
+[Collection("live-postgres")]
+public sealed class StoreSessionTimeZoneLiveTests
+{
+    private static string? BaseConnectionString => Environment.GetEnvironmentVariable("DARLING_TEST_PG");
+
+    [Fact]
+    public async Task AStoreConnection_PinsToUtc_EvenWhenTheDatabaseDefaultIsAmericaNewYork()
+    {
+        var baseCs = BaseConnectionString;
+        Assert.SkipWhen(
+            string.IsNullOrEmpty(baseCs),
+            "Set DARLING_TEST_PG to a Postgres connection string to run the live #4277 store-timezone-pin test.");
+
+        var ct = TestContext.Current.CancellationToken;
+
+        /* #1776 own-store: a database-level default this test sets must not leak into any other test that
+           shares DARLING_TEST_PG's own database. */
+        await using var scratch = await ScratchPostgres.CreateAsync(baseCs!, ct);
+
+        await using (var admin = new NpgsqlConnection(baseCs))
+        {
+            await admin.OpenAsync(ct);
+            await using var alter = new NpgsqlCommand(
+                $"ALTER DATABASE \"{scratch.DatabaseName}\" SET timezone = 'America/New_York'", admin);
+            await alter.ExecuteNonQueryAsync(ct);
+        }
+
+        /* ALTER DATABASE ... SET only takes effect for sessions opened AFTER it commits, so migrate through a
+           fresh connection rather than the admin one above. */
+        await using (var migrate = new NpgsqlConnection(scratch.ConnectionString))
+        {
+            await migrate.OpenAsync(ct);
+            await PgMigrations.MigrateAsync(migrate, ct);
+        }
+
+        /* The database's own default, UNPINNED — confirms the fixture set up a genuinely non-UTC store
+           before asserting that the product's pin overrides it. */
+        await using (var unpinned = new NpgsqlConnection(scratch.ConnectionString))
+        {
+            await unpinned.OpenAsync(ct);
+            Assert.Equal("America/New_York", await ShowTimeZoneAsync(unpinned, ct));
+        }
+
+        /* The way the product opens a STORE connection: PinSessionTimeZoneUtc first, then the connection. */
+        var pinnedConnectionString = DarlingStoreConnection.PinSessionTimeZoneUtc(scratch.ConnectionString);
+        await using var connection = new NpgsqlConnection(pinnedConnectionString);
+        await connection.OpenAsync(ct);
+
+        Assert.Equal("UTC", await ShowTimeZoneAsync(connection, ct));
+
+        Assert.Contains(
+            "collection_time >= now() - interval '2 days'", DarlingModuleMap.RefreshSql, StringComparison.Ordinal);
+
+        await using (var create = new NpgsqlCommand(
+            "CREATE TEMP TABLE probe_4277 (collection_time timestamp NOT NULL)", connection))
+        {
+            await create.ExecuteNonQueryAsync(ct);
+        }
+
+        /* Kind=Unspecified so Npgsql binds `timestamp` (naive), never `timestamptz` — the same naive-UTC
+           digits every collector row carries, not a value that shifts on its own before the comparison even
+           runs. */
+        var insideWindow = DateTime.SpecifyKind(DateTime.UtcNow.AddHours(-46), DateTimeKind.Unspecified);
+        var outsideWindow = DateTime.SpecifyKind(DateTime.UtcNow.AddHours(-50), DateTimeKind.Unspecified);
+
+        await using (var insert = new NpgsqlCommand(
+            "INSERT INTO probe_4277 (collection_time) VALUES ($1), ($2)", connection))
+        {
+            insert.Parameters.AddWithValue(insideWindow);
+            insert.Parameters.AddWithValue(outsideWindow);
+            await insert.ExecuteNonQueryAsync(ct);
+        }
+
+        await using var read = new NpgsqlCommand(
+            "SELECT count(*) FILTER (WHERE collection_time = $1), count(*) FILTER (WHERE collection_time = $2) "
+            + "FROM probe_4277 WHERE collection_time >= now() - interval '2 days'", connection);
+        read.Parameters.AddWithValue(insideWindow);
+        read.Parameters.AddWithValue(outsideWindow);
+        await using var reader = await read.ExecuteReaderAsync(ct);
+        await reader.ReadAsync(ct);
+
+        Assert.Equal(1L, reader.GetInt64(0)); // 46h old, inside the 48h window: returned, as #4277 asks.
+        Assert.Equal(0L, reader.GetInt64(1)); // 50h old, past the 48h window: NOT returned — only true under UTC.
+    }
+
+    private static async Task<string> ShowTimeZoneAsync(NpgsqlConnection connection, CancellationToken ct)
+    {
+        await using var command = new NpgsqlCommand("SHOW timezone", connection);
+
+        return (string)(await command.ExecuteScalarAsync(ct))!;
+    }
+}