From 5fdd8a570ab9e165987dfa5763b954a5f75cecfd Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 11 Aug 2026 08:49:44 -0400 Subject: [PATCH] fix(secrets): run the store-path guard in the pre-host container (0.6.1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 0.6.0 put the store-path rules in the shared library, but the guard was not running at the moment that matters here. CreateBuilder resolves ${secret:} references before the host exists, using a throwaway ServiceCollection that contains no IHostEnvironment — and it runs the store migrator, which creates the database. The library resolved the content root from IHostEnvironment alone, so it could not distinguish "no content root" from "no host registered" and skipped the under-content-root rule entirely. The store was created at the rejected path; the boot then failed a moment later when the real host validated. The leftover empty database with its -wal/-shm siblings is exactly the artifact that made the 2026-08-09 credential loss read as "the database is there, it's just empty". The pin alone does not close this. An app with a correctly configured path shows no symptom and is still unprotected, because the guard simply is not running when the store is created. 0.6.1 adds a 4-argument AddZbSecrets overload taking the content root explicitly, and the call site has to use it. The in-host registration below needs nothing. Verified by removing the fix rather than by observing a clean boot — which is how this survived its first release. With the 3-argument overload the new test fails by finding a created database at src/ZB.MOM.WW.MxGateway.Server/probe-secrets-*.db: inside the source tree, since that is what the content root resolves to under test. Two things about the test itself, both of which it would have been easy to get subtly wrong: It asserts no-file-created before asserting that startup threw. "It threw" is the weaker claim, and asserting it first masks the stronger one — the run that proved this defect would have reported "no exception was thrown" and said nothing about the database sitting in the source tree. The accepting case asserts the database *is* created, not merely that nothing threw. A not-null builder is close to a tautology once no exception escaped, and it would still pass if the pre-host container stopped opening the store at all — which would also quietly void the rejecting case, since that one can only observe a file the migration would otherwise have written. The two assertions hold each other up. Found by HistorianGateway's adoption, which probed the rejected paths instead of observing a successful boot. --- .../GatewayApplication.cs | 12 +- .../ZB.MOM.WW.MxGateway.Server.csproj | 6 +- .../SecretsStorePathGuardTests.cs | 111 ++++++++++++++++++ 3 files changed, 125 insertions(+), 4 deletions(-) create mode 100644 src/ZB.MOM.WW.MxGateway.Tests/Configuration/SecretsStorePathGuardTests.cs diff --git a/src/ZB.MOM.WW.MxGateway.Server/GatewayApplication.cs b/src/ZB.MOM.WW.MxGateway.Server/GatewayApplication.cs index c9d2cdd..d5415c8 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/GatewayApplication.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/GatewayApplication.cs @@ -78,9 +78,19 @@ public static class GatewayApplication // here (SecretNotFoundException); config with no tokens is untouched (no-op), so this is safe // to always run. CreateBuilder is synchronous and single-shot at bootstrap, so the two awaits // are driven via GetAwaiter().GetResult() (no sync-context deadlock risk during host startup). + // The content root is passed explicitly because this container is a throwaway + // ServiceCollection with no IHostEnvironment in it. Without it the library cannot tell + // "no content root exists" from "no host is registered", so it skips the + // under-content-root rule — and the migrator below CREATES the store before the real host + // ever validates. The boot then fails a moment later, having already left an empty + // database with its -wal/-shm siblings at the very path the rule rejects. That artifact is + // what made the 2026-08-09 outage read as "the database is there, it's just empty". + // DO NOT simplify this to the 3-argument overload: it still compiles, the app still boots + // when the path is correct, and the guard silently stops running at the one moment that + // matters. #pragma warning disable ASP0000 // deliberate throwaway container, disposed here, shares no singletons using (var secretsProvider = new ServiceCollection() - .AddZbSecrets(builder.Configuration, "Secrets") + .AddZbSecrets(builder.Configuration, "Secrets", builder.Environment.ContentRootPath) .BuildServiceProvider()) #pragma warning restore ASP0000 { diff --git a/src/ZB.MOM.WW.MxGateway.Server/ZB.MOM.WW.MxGateway.Server.csproj b/src/ZB.MOM.WW.MxGateway.Server/ZB.MOM.WW.MxGateway.Server.csproj index e9258b2..93162cf 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/ZB.MOM.WW.MxGateway.Server.csproj +++ b/src/ZB.MOM.WW.MxGateway.Server/ZB.MOM.WW.MxGateway.Server.csproj @@ -21,9 +21,9 @@ - - - + + + diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Configuration/SecretsStorePathGuardTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Configuration/SecretsStorePathGuardTests.cs new file mode 100644 index 0000000..d5aabcd --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Tests/Configuration/SecretsStorePathGuardTests.cs @@ -0,0 +1,111 @@ +using Microsoft.AspNetCore.Builder; +using ZB.MOM.WW.MxGateway.Server; + +namespace ZB.MOM.WW.MxGateway.Tests.Configuration; + +// Mutates the process-global Secrets__SqlitePath that GatewayApplication.CreateBuilder reads; +// serialized against every other collection so a parallel host-building test cannot inherit the +// deliberately-rejected path. See GlobalEnvironmentCollection. +[Collection(TestSupport.GlobalEnvironmentCollection.Name)] +public sealed class SecretsStorePathGuardTests +{ + private const string SqlitePathVariable = "Secrets__SqlitePath"; + + /// + /// Verifies the store-path guard runs in the pre-host secrets container, which is the + /// only place it matters. + /// + /// + /// + /// CreateBuilder resolves ${secret:} references before the host exists, using a + /// throwaway that + /// contains no IHostEnvironment — and it runs the store migrator, which creates the + /// database. A library that infers the content root from IHostEnvironment alone + /// cannot distinguish "no content root" from "no host registered" and skips the rule here, so + /// the store is created at the rejected path and only then does the real host refuse to start. + /// The leftover empty database with its -wal/-shm siblings is precisely the + /// artifact that made the 2026-08-09 credential loss read as "the database is there, it's just + /// empty". The gateway therefore passes the content root explicitly. + /// + /// + /// The assertion that no file was created is the load-bearing one. A test that merely observed + /// a failed boot would pass even while the store was being written, because the failure arrives + /// afterwards either way — which is exactly how this defect survived its first release. + /// + /// + [Fact] + public void CreateBuilder_RejectsSecretsStoreUnderContentRoot_WithoutCreatingIt() + { + string? original = Environment.GetEnvironmentVariable(SqlitePathVariable); + string contentRoot = ResolveContentRoot(); + string rejected = Path.Combine(contentRoot, $"probe-secrets-{Guid.NewGuid():N}.db"); + + try + { + Environment.SetEnvironmentVariable(SqlitePathVariable, rejected); + + // Capture rather than Assert.ThrowsAny, so the store-creation assertions below are + // reported first. Ordering matters here: "it threw" is the weaker claim, and asserting + // it first would mask the stronger one — that nothing was written before it threw. + Exception? thrown = Record.Exception(() => GatewayApplication.CreateBuilder([])); + + Assert.False(File.Exists(rejected), $"the rejected store was created at {rejected}"); + Assert.False(File.Exists(rejected + "-wal"), "a write-ahead log was created for the rejected store"); + Assert.False(File.Exists(rejected + "-shm"), "a shared-memory file was created for the rejected store"); + Assert.NotNull(thrown); + } + finally + { + Environment.SetEnvironmentVariable(SqlitePathVariable, original); + + // Delete defensively: if the guard ever regresses this test writes a database into the + // content root, which on a dev machine is the source tree. + foreach (string leftover in new[] { rejected, rejected + "-wal", rejected + "-shm" }) + { + if (File.Exists(leftover)) + { + File.Delete(leftover); + } + } + } + } + + /// + /// Verifies a store path outside the content root is accepted and actually used. + /// + /// + /// The assertion is that the database exists afterwards, not merely that nothing threw. A + /// not-null builder is very close to a tautology once no exception escaped, so it would pass + /// even if the pre-host container had stopped opening the store altogether — which would also + /// silently void the negative test above, since that one can only observe a file the migration + /// would otherwise have written. Proving the accepted path gets a real database is what keeps + /// the rejected-path assertion meaningful. + /// + [Fact] + public void CreateBuilder_AcceptsSecretsStoreOutsideContentRoot_AndCreatesIt() + { + string? original = Environment.GetEnvironmentVariable(SqlitePathVariable); + string directory = Directory.CreateTempSubdirectory("mxgw-secrets-ok").FullName; + string accepted = Path.Combine(directory, "secrets.db"); + + try + { + Environment.SetEnvironmentVariable(SqlitePathVariable, accepted); + + WebApplicationBuilder builder = GatewayApplication.CreateBuilder([]); + + Assert.NotNull(builder); + Assert.True(File.Exists(accepted), $"the accepted store was not created at {accepted}"); + } + finally + { + Environment.SetEnvironmentVariable(SqlitePathVariable, original); + Directory.Delete(directory, recursive: true); + } + } + + // The content root CreateBuilder will use, taken from a builder created with the suite's normal + // (valid) store path rather than assumed from the test's working directory. + private static string ResolveContentRoot() => + GatewayApplication.CreateBuilder([]).Environment.ContentRootPath; +}