From 5e2d3dd1bc3864b511248e9d4f2e7b615a3ba164 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 9 Nov 2026 15:13:49 -0500 Subject: [PATCH] fix(secrets): malformed ${secret:} references fail closed; README sample validates (scadaproj#45) Secrets-009: SecretReferenceExpander.ExpandAsync throws FormatException when a value holds a ${secret: marker that is not a complete reference with a non-blank name (${secret:}, an unterminated ${secret:name, a nested marker), instead of passing the literal text through as the credential. ExpandConfigurationAsync names the offending key (never the value) and writes nothing. The README wiring sample drops the relative "SqlitePath": "secrets.db" (which failed validation) for the per-user default and shows an absolute example; a test validates the sample. Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV --- ZB.MOM.WW.Secrets/README.md | 5 +- .../Configuration/SecretReferenceExpander.cs | 47 ++++++++++++++++++- .../Cli/ShippedCliSettingsTests.cs | 18 +++++++ .../SecretReferenceExpanderTests.cs | 43 +++++++++++++++++ 4 files changed, 110 insertions(+), 3 deletions(-) diff --git a/ZB.MOM.WW.Secrets/README.md b/ZB.MOM.WW.Secrets/README.md index 073ae38..4f99b13 100644 --- a/ZB.MOM.WW.Secrets/README.md +++ b/ZB.MOM.WW.Secrets/README.md @@ -84,7 +84,10 @@ await expander.ExpandConfigurationAsync((IConfigurationRoot)builder.Configuratio ```jsonc // appsettings.json "Secrets": { - "SqlitePath": "secrets.db", + // SqlitePath omitted: the per-user default (/ZB.MOM.WW/secrets.db) applies. + // A service sets an ABSOLUTE path OUTSIDE its install directory instead, e.g. + // "SqlitePath": "E:\\ZB\\data\\secrets.db" — a relative path, or one under the content root, + // fails startup validation. "MasterKey": { "Source": "Environment", "EnvVarName": "ZB_SECRETS_MASTER_KEY" }, "RunMigrationsOnStartup": true, "ResolveCacheTtl": "00:00:30" diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretReferenceExpander.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretReferenceExpander.cs index 4a2f9f0..ce1c70c 100644 --- a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretReferenceExpander.cs +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretReferenceExpander.cs @@ -10,7 +10,10 @@ namespace ZB.MOM.WW.Secrets.Configuration; /// referenced secret through an . Expansion is fail-closed: /// a token that names an absent secret throws rather than /// leaving the placeholder (or a blank) in place, so a misconfigured deployment fails loudly at -/// startup instead of silently running with an empty credential. +/// startup instead of silently running with an empty credential. A malformed reference — an +/// empty name (${secret:}) or a missing closing brace (${secret:name) — is equally +/// fail-closed: it throws instead of passing the literal text through as +/// the credential. /// public sealed class SecretReferenceExpander { @@ -37,6 +40,10 @@ public sealed class SecretReferenceExpander /// no tokens. /// /// A referenced secret does not exist (fail-closed). + /// + /// contains a ${secret: marker that is not a complete reference with + /// a non-blank name (fail-closed). The message never includes the value. + /// public async Task ExpandAsync(string raw, CancellationToken ct) { if (raw is null) @@ -45,6 +52,20 @@ public sealed class SecretReferenceExpander } MatchCollection matches = TokenPattern.Matches(raw); + + // Every marker must start a complete, named reference. "${secret:}" and an unterminated + // "${secret:name" do not match the pattern at all, and a marker swallowed inside another + // match ("${secret:a${secret:b}") is not one reference per marker — left alone, each would + // pass through verbatim and be used as the credential. + int markers = CountMarkers(raw); + if (markers != matches.Count + || matches.Any(m => string.IsNullOrWhiteSpace(m.Groups[1].Value))) + { + throw new FormatException( + "Malformed ${secret:...} reference: every ${secret: must be followed by a non-blank " + + "secret name and a closing '}'."); + } + if (matches.Count == 0) { return raw; @@ -122,6 +143,7 @@ public sealed class SecretReferenceExpander /// The loaded configuration root to expand. /// A token to cancel the operation. /// A referenced secret does not exist (fail-closed). + /// A value holds a malformed reference (fail-closed); names the key. public async Task ExpandConfigurationAsync(IConfigurationRoot config, CancellationToken ct) { ArgumentNullException.ThrowIfNull(config); @@ -143,7 +165,15 @@ public sealed class SecretReferenceExpander var expanded = new Dictionary(StringComparer.OrdinalIgnoreCase); foreach (KeyValuePair entry in pending) { - expanded[entry.Key] = await ExpandAsync(entry.Value, ct).ConfigureAwait(false); + try + { + expanded[entry.Key] = await ExpandAsync(entry.Value, ct).ConfigureAwait(false); + } + catch (FormatException ex) + { + // Name the key (never the value) so the operator can find the typo. + throw new FormatException($"Configuration key '{entry.Key}': {ex.Message}", ex); + } } if (expanded.Count == 0) @@ -161,6 +191,19 @@ public sealed class SecretReferenceExpander } } + private static int CountMarkers(string value) + { + int count = 0; + for (int at = value.IndexOf(TokenMarker, StringComparison.Ordinal); + at >= 0; + at = value.IndexOf(TokenMarker, at + TokenMarker.Length, StringComparison.Ordinal)) + { + count++; + } + + return count; + } + // ConfigurationManager: append (or reuse) the overlay source. It is never reloaded, so it keeps // winning over the file providers beneath it whatever they reload to. private static void PinInOverlay(IConfigurationBuilder builder, Dictionary expanded) diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Cli/ShippedCliSettingsTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Cli/ShippedCliSettingsTests.cs index 35fa6b6..ed55303 100644 --- a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Cli/ShippedCliSettingsTests.cs +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Cli/ShippedCliSettingsTests.cs @@ -9,6 +9,7 @@ namespace ZB.MOM.WW.Secrets.Tests.Cli; /// scadaproj#49: the appsettings.json the CLI ships must pass the store-path validator. It /// used to set "SqlitePath": "secrets.db", which the validator rejects as relative, so the /// headless CLI could not start from its own directory unless an environment variable overrode it. +/// Same check for the README's wiring sample (Secrets-009). /// public sealed class ShippedCliSettingsTests { @@ -44,4 +45,21 @@ public sealed class ShippedCliSettingsTests Assert.True(Path.IsPathFullyQualified(options.SqlitePath)); } + + [Fact] + public void The_readme_wiring_sample_passes_store_path_validation() + { + string readme = File.ReadAllText(RepoFile("README.md")); + int start = readme.IndexOf("\"Secrets\": {", StringComparison.Ordinal); + Assert.True(start >= 0, "README no longer has a Secrets sample block"); + int end = readme.IndexOf("\n},", start, StringComparison.Ordinal); + string block = "{" + readme[start..(end + 2)].TrimEnd(',') + "}"; + string json = string.Join('\n', block.Split('\n').Where(l => !l.TrimStart().StartsWith("//", StringComparison.Ordinal))); + + IConfiguration config = new ConfigurationBuilder() + .AddJsonStream(new MemoryStream(System.Text.Encoding.UTF8.GetBytes(json))) + .Build(); + + Validate(config); + } } diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Configuration/SecretReferenceExpanderTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Configuration/SecretReferenceExpanderTests.cs index bcc2eb8..05fa164 100644 --- a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Configuration/SecretReferenceExpanderTests.cs +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Configuration/SecretReferenceExpanderTests.cs @@ -129,4 +129,47 @@ public sealed class SecretReferenceExpanderTests // Real key still expanded. Assert.Equal("Server=.;Password=hunter2", config["ConnectionStrings:Db"]); } + + // ---------- Secrets-009: malformed references are fail-closed ---------- + + [Theory] + [InlineData("pw=${secret:}")] + [InlineData("pw=${secret: }")] + [InlineData("pw=${secret:sql/foo")] + [InlineData("pw=${secret:sql/foo};x=${secret:")] + [InlineData("pw=${secret:a${secret:sql/foo}")] + public async Task ExpandAsync_rejects_a_malformed_reference_instead_of_passing_it_through(string raw) + { + SecretReferenceExpander expander = Expander(("sql/foo", "hunter2")); + + var ex = await Assert.ThrowsAsync(() => expander.ExpandAsync(raw, CancellationToken.None)); + Assert.DoesNotContain("hunter2", ex.Message); + } + + [Fact] + public async Task ExpandConfigurationAsync_names_the_key_holding_a_malformed_reference_and_writes_nothing() + { + IConfigurationRoot config = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["Db:Good"] = "pw=${secret:sql/foo}", + ["Db:Typo"] = "Password=${secret:}", + }) + .Build(); + + var ex = await Assert.ThrowsAsync( + () => Expander(("sql/foo", "hunter2")).ExpandConfigurationAsync(config, CancellationToken.None)); + + Assert.Contains("Db:Typo", ex.Message); + Assert.DoesNotContain("Password", ex.Message); + Assert.Equal("pw=${secret:sql/foo}", config["Db:Good"]); // nothing half-expanded + } + + [Fact] + public async Task ExpandAsync_text_without_a_marker_is_still_returned_unchanged() + { + // Positive control: the guard is about the marker, not about braces or dollar signs. + string raw = "a=${env:X};b={secret};c=$secret:"; + Assert.Equal(raw, await Expander().ExpandAsync(raw, CancellationToken.None)); + } }