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)); + } }