fix(secrets): close the pre-host hole in the store-path guard (0.6.1)

Found by the HistorianGateway adoption of 0.6.0, which probed the rejected paths instead of observing a clean boot — a clean boot being equally consistent with the validator not being wired at all. An application that expands ${secret:} references before building its host composes this subsystem in a throwaway ServiceCollection with no IHostEnvironment in it, then migrates the store from that container. SecretsOptionsValidator resolves the content root from IHostEnvironment and skips the under-content-root rule when it is absent, so pre-host an absolute in-content-root path passed the only rule still applying, the migrator CREATED the database, and the boot failed a second later once the real host registered IHostEnvironment and the rule finally fired. The observable result was a rejected boot that still left an empty store, with -wal and -shm siblings, at the rejected path — the same artifact that made the MxGateway outage read as "the database is there, it's just empty", which is precisely what this guard exists to prevent. A relative path was unaffected because the absolute-path rule fires with or without a content root; that asymmetry is what exposed it. The mistake was conflating "no IHostEnvironment registered" with "no content root exists". New AddZbSecrets overload takes the content root explicitly; the 3-argument overload is unchanged and still falls back to IHostEnvironment, so this is additive. SecretsOptionsValidator gains a string? constructor alongside the IHostEnvironment? one. Reproduced first in an isolated harness before changing anything: a bare host rejects both bad paths leaving no artifacts, and a bare ServiceCollection with no IHostEnvironment migrates an in-content-root path to disk. Both are now regression tests, the second paired with a positive control asserting the old behaviour still holds when no content root is supplied — so the test is about the explicit content root rather than about paths in general. Also documents an upgrade note that applies to 0.6.0: a store seeded under the old relative default is orphaned rather than migrated, and nothing signals it.
2026-08-11 08:37:53 -04:00
parent 105c230cb1
commit cbfe296279
5 changed files with 170 additions and 7 deletions
@@ -5,6 +5,44 @@ follows [Keep a Changelog](https://keepachangelog.com/); versions are shared acr
 package in the family (see `Directory.Build.props`) even when a given release only touches
 one of them.

+## 0.6.1 — 2026-08-11
+
+### Fixed
+
+- **The 0.6.0 guard had a hole at exactly the moment the store file is created** (found by the
+  HistorianGateway adoption of 0.6.0, which probed the rejected paths rather than observing a
+  clean boot). An application that expands `${secret:}` references *before* building its host
+  composes this subsystem in a throwaway `ServiceCollection` with **no `IHostEnvironment` in it**,
+  then migrates the store from that container. `SecretsOptionsValidator` resolves the content root
+  from `IHostEnvironment` and skips the under-content-root rule when it is absent — so pre-host an
+  absolute in-content-root path passed the only rule still applying, the migrator **created the
+  database**, and the boot then failed a second later once the real host registered
+  `IHostEnvironment` and the rule finally fired.
+
+  The observable result was a rejected boot that *still left an empty store, with `-wal`/`-shm`
+  siblings, sitting at the rejected path* — the same artifact that made the outage this guard
+  exists to prevent read as "the database is there, it's just empty". A relative path was
+  unaffected, because the absolute-path rule fires with or without a content root; that asymmetry
+  is what exposed it.
+
+  Conflating "no `IHostEnvironment` registered" with "no content root exists" was the mistake. New
+  overload **`AddZbSecrets(services, config, sectionPath, contentRootPath)`** takes the content
+  root explicitly; the 3-argument overload is unchanged and still falls back to `IHostEnvironment`.
+  **Pre-host composition should pass it** — e.g. `builder.Environment.ContentRootPath`. Passing
+  `null` remains correct only where there genuinely is no content root, such as the CLI.
+  `SecretsOptionsValidator` gains a `string?` constructor alongside the `IHostEnvironment?` one.
+
+### Upgrade note (applies to 0.6.0 as well)
+
+- **A store seeded under the pre-0.6.0 default is orphaned, not migrated, and nothing says so.**
+  The old default was the relative `secrets.db`, which resolved against the process working
+  directory; the new default is absolute. An application that relied on the old default silently
+  starts using a different file, and anything previously seeded into the old one is stranded. This
+  is a dev-loop concern for the apps in this family (every deployment sets an explicit absolute
+  path), but check for a stray `*-secrets.db` beside your project before assuming an empty store
+  after upgrade means the store was lost. Deleting the key from configuration, rather than leaving
+  a relative value, is what makes the new default apply.
+
 ## 0.6.0 — 2026-08-11

 ### Added
@@ -5,7 +5,7 @@
    <Nullable>enable</Nullable>
    <ImplicitUsings>enable</ImplicitUsings>
    <LangVersion>latest</LangVersion>
-    <Version>0.6.0</Version>
+    <Version>0.6.1</Version>
    <ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
    <PackageReadmeFile>README.md</PackageReadmeFile>
  </PropertyGroup>
@@ -25,11 +25,35 @@ public sealed class SecretsOptionsValidator : OptionsValidatorBase<SecretsOption
    /// </summary>
    /// <param name="hostEnvironment">
    /// The host environment supplying the content root, or <see langword="null"/> when the secrets
-    /// subsystem is composed outside a host (the CLI does this). With no content root the
-    /// under-content-root rule is skipped; the absolute-path rule still applies.
+    /// subsystem is composed outside a host. With no content root the under-content-root rule is
+    /// skipped; the absolute-path rule still applies.
    /// </param>
-    public SecretsOptionsValidator(IHostEnvironment? hostEnvironment) =>
-        _contentRootPath = hostEnvironment?.ContentRootPath;
+    public SecretsOptionsValidator(IHostEnvironment? hostEnvironment)
+        : this(hostEnvironment?.ContentRootPath)
+    {
+    }
+
+    /// <summary>
+    /// Initializes a new instance of the <see cref="SecretsOptionsValidator"/> class with an
+    /// explicit content root.
+    /// </summary>
+    /// <param name="contentRootPath">
+    /// The application content root, or <see langword="null"/> when there genuinely is none.
+    /// </param>
+    /// <remarks>
+    /// This overload exists because "no <see cref="IHostEnvironment"/> registered" and "no content
+    /// root exists" are different things, and conflating them left a hole in the guard. An
+    /// application that expands <c>${secret:}</c> references BEFORE building its host composes the
+    /// subsystem in a throwaway <c>ServiceCollection</c> with no <see cref="IHostEnvironment"/> in
+    /// it, then migrates the store from that container — so the content-root rule was skipped at
+    /// exactly the moment the store file gets created. The boot then failed a second later, once
+    /// the real host registered <see cref="IHostEnvironment"/> and the rule finally fired, leaving
+    /// an empty database sitting at the rejected path: the same artifact that made the outage this
+    /// guard exists to prevent read as "the database is there, it's just empty".
+    /// Pre-host composition should pass the content root here explicitly.
+    /// </remarks>
+    public SecretsOptionsValidator(string? contentRootPath) =>
+        _contentRootPath = contentRootPath;

    /// <inheritdoc />
    protected override void Validate(ValidationBuilder builder, SecretsOptions options)
@@ -33,7 +33,29 @@ public static class SecretsServiceCollectionExtensions
    public static IServiceCollection AddZbSecrets(
        this IServiceCollection services,
        IConfiguration config,
-        string sectionPath)
+        string sectionPath) => services.AddZbSecrets(config, sectionPath, contentRootPath: null);
+
+    /// <summary>
+    /// As <see cref="AddZbSecrets(IServiceCollection, IConfiguration, string)"/>, with the content
+    /// root supplied explicitly rather than resolved from <see cref="IHostEnvironment"/>.
+    /// </summary>
+    /// <param name="services">The service collection to add to.</param>
+    /// <param name="config">The application configuration.</param>
+    /// <param name="sectionPath">Path of the configuration section holding the secrets options.</param>
+    /// <param name="contentRootPath">
+    /// The application content root. Pass this whenever the subsystem is composed OUTSIDE a host —
+    /// most importantly from a pre-host <c>${secret:}</c> expander, which builds a throwaway
+    /// container with no <see cref="IHostEnvironment"/> in it and then migrates the store. Without
+    /// it the under-content-root rule is skipped at exactly the moment the store file is created,
+    /// so a rejected path still gets an empty database written to it before the real host aborts.
+    /// Pass <see langword="null"/> only where there genuinely is no content root, such as the CLI.
+    /// </param>
+    /// <returns>The same <paramref name="services"/> instance, for chaining.</returns>
+    public static IServiceCollection AddZbSecrets(
+        this IServiceCollection services,
+        IConfiguration config,
+        string sectionPath,
+        string? contentRootPath)
    {
        ArgumentNullException.ThrowIfNull(services);
        ArgumentNullException.ThrowIfNull(config);
@@ -53,7 +75,10 @@ public static class SecretsServiceCollectionExtensions
        // rather than skipping it. Guarding on the concrete registration gives both properties.
        if (services.All(d => d.ServiceType != typeof(SecretsOptionsValidator)))
        {
-            services.AddSingleton(sp => new SecretsOptionsValidator(sp.GetService<IHostEnvironment>()));
+            // An explicitly supplied content root wins; otherwise fall back to IHostEnvironment,
+            // which is present in a real host and absent in a pre-host container.
+            services.AddSingleton(sp => new SecretsOptionsValidator(
+                contentRootPath ?? sp.GetService<IHostEnvironment>()?.ContentRootPath));
            services.AddSingleton<IValidateOptions<SecretsOptions>>(
                sp => sp.GetRequiredService<SecretsOptionsValidator>());
        }
@@ -6,6 +6,7 @@ using Microsoft.Extensions.Options;
 using ZB.MOM.WW.Configuration;
 using ZB.MOM.WW.Secrets.Configuration;
 using ZB.MOM.WW.Secrets.DependencyInjection;
+using ZB.MOM.WW.Secrets.Sqlite;

 namespace ZB.MOM.WW.Secrets.Tests.Configuration;

@@ -236,6 +237,81 @@ public sealed class SecretsStorePathRulesTests
            provider.GetRequiredService<IOptions<SecretsOptions>>().Value.SqlitePath);
    }

+    [Fact]
+    public void A_pre_host_container_given_the_content_root_refuses_before_the_store_is_created()
+    {
+        // REGRESSION (found by the HistorianGateway adoption of 0.6.0). An application that expands
+        // ${secret:} references before building its host composes this subsystem in a throwaway
+        // ServiceCollection with NO IHostEnvironment, then migrates the store from that container.
+        // The content-root rule was skipped there — so an absolute in-content-root path passed the
+        // only rule still applying, the migrator CREATED the database, and the boot then failed a
+        // second later once the real host registered IHostEnvironment. That left an empty store at
+        // the rejected path: the exact artifact that made the outage this guard exists to prevent
+        // read as "the database is there, it's just empty".
+        string contentRoot = Path.Combine(Path.GetTempPath(), $"zb-prehost-{Guid.NewGuid():N}");
+        Directory.CreateDirectory(contentRoot);
+        string dbPath = Path.Combine(contentRoot, "store.db");
+
+        try
+        {
+            IConfiguration config = new ConfigurationBuilder()
+                .AddInMemoryCollection(new Dictionary<string, string?>
+                {
+                    ["Secrets:SqlitePath"] = dbPath,
+                })
+                .Build();
+
+            var services = new ServiceCollection();
+            services.AddZbSecrets(config, "Secrets", contentRoot);   // the content root, explicitly
+            using ServiceProvider provider = services.BuildServiceProvider();
+
+            Assert.Throws<OptionsValidationException>(
+                () => _ = provider.GetRequiredService<SecretsSqliteConnectionFactory>());
+
+            // The point of the test: refused BEFORE anything touched the filesystem.
+            Assert.False(File.Exists(dbPath), "a rejected path must not be left holding a store file");
+            Assert.False(File.Exists(dbPath + "-wal"));
+            Assert.False(File.Exists(dbPath + "-shm"));
+        }
+        finally
+        {
+            Directory.Delete(contentRoot, recursive: true);
+        }
+    }
+
+    [Fact]
+    public void Without_a_content_root_the_pre_host_container_still_cannot_be_told_it_is_fine()
+    {
+        // Positive control for the test above: the SAME path, with the content root NOT supplied,
+        // is the pre-0.6.1 behaviour — the rule is skipped and nothing objects. This is what makes
+        // the assertion above about the explicit content root rather than about paths in general.
+        string contentRoot = Path.Combine(Path.GetTempPath(), $"zb-prehost-{Guid.NewGuid():N}");
+        Directory.CreateDirectory(contentRoot);
+        string dbPath = Path.Combine(contentRoot, "store.db");
+
+        try
+        {
+            IConfiguration config = new ConfigurationBuilder()
+                .AddInMemoryCollection(new Dictionary<string, string?>
+                {
+                    ["Secrets:SqlitePath"] = dbPath,
+                })
+                .Build();
+
+            var services = new ServiceCollection();
+            services.AddZbSecrets(config, "Secrets");   // no content root — the CLI's situation
+            using ServiceProvider provider = services.BuildServiceProvider();
+
+            // Absolute, so the rooted rule passes; the content-root rule cannot fire. Documented,
+            // and the reason pre-host composition must pass the content root explicitly.
+            Assert.NotNull(provider.GetRequiredService<SecretsSqliteConnectionFactory>());
+        }
+        finally
+        {
+            Directory.Delete(contentRoot, recursive: true);
+        }
+    }
+
    private sealed class StubHostEnvironment : IHostEnvironment
    {
        public string EnvironmentName { get; set; } = "Production";