fix(secrets-cli): reference auditor — InvalidName containment, comment-key parity, name normalization

ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/ReferenceAuditor.cs

@@ -24,6 +24,12 @@ public enum ReferenceStatus
 
     /// <summary>The row exists but does not decrypt under the session's KEK (wrong or rotated KEK, or tampering).</summary>
     Undecryptable,
+
+    /// <summary>
+    /// The token names a value that is not a valid <see cref="SecretName"/> (illegal characters, an
+    /// unfilled placeholder like <c>REPLACE ME</c>, and so on), so it cannot be looked up at all.
+    /// </summary>
+    InvalidName,
 }
 
 /// <summary>
@@ -45,55 +51,99 @@ public sealed class ReferenceAuditor
 
     /// <summary>
     /// Walks every configuration value, collects each distinct <c>${secret:NAME}</c> reference with the
-    /// config key paths that use it, and classifies each name against the session's store. Findings are
+    /// config key paths that use it, and classifies each name against the session's store. References are
-    /// ordered by name (ordinal); each finding's config paths are likewise sorted and de-duplicated.
+    /// keyed by their <em>normalized</em> <see cref="SecretName.Value"/> so case/whitespace variants of one
+    /// secret collapse to a single finding whose name matches what a <c>get</c> would resolve; a token that
+    /// is not a valid name is reported once as <see cref="ReferenceStatus.InvalidName"/> under its raw text
+    /// rather than aborting the whole audit. Findings are ordered by name (ordinal); each finding's config
+    /// paths are likewise sorted and de-duplicated.
     /// </summary>
     /// <param name="session">The open store session whose target configuration is audited.</param>
     /// <param name="ct">A token to cancel the store lookups.</param>
-    /// <returns>One finding per distinct referenced secret name.</returns>
+    /// <returns>One finding per distinct referenced secret.</returns>
     public async Task<IReadOnlyList<ReferenceFinding>> AuditAsync(SecretsSession session, CancellationToken ct)
     {
         ArgumentNullException.ThrowIfNull(session);
 
-        // name -> ordered, de-duplicated set of config key paths. SortedDictionary keeps findings ordered
+        // group key -> collected reference. The key is the normalized SecretName.Value for a valid name
-        // by name; each SortedSet keeps that name's paths deterministic.
+        // (so variants collapse), or the raw trimmed token text for an invalid one. SortedDictionary keeps
-        var pathsByName = new SortedDictionary<string, SortedSet<string>>(StringComparer.Ordinal);
+        // findings ordered by name; each group's SortedSet keeps its paths deterministic.
+        var groups = new SortedDictionary<string, ReferenceGroup>(StringComparer.Ordinal);
         foreach (KeyValuePair<string, string?> entry in session.Target.Configuration.AsEnumerable())
         {
-            if (entry.Value is not { } value)
+            // Mirror SecretReferenceExpander.IsCommentKey: an entry whose leaf key segment starts with '_'
+            // (the "_comment" convention) may legitimately document the ${secret:...} syntax with a literal
+            // example token and is never resolved — so the auditor must not report it either.
+            if (entry.Value is not { } value || IsCommentKey(entry.Key))
             {
                 continue;
             }
 
             foreach (Match match in TokenPattern.Matches(value))
             {
-                string name = match.Groups[1].Value.Trim();
+                string raw = match.Groups[1].Value.Trim();
-                if (!pathsByName.TryGetValue(name, out SortedSet<string>? paths))
+                ReferenceGroup group = ResolveGroup(groups, raw);
-                {
+                group.Paths.Add(entry.Key);
-                    paths = new SortedSet<string>(StringComparer.Ordinal);
-                    pathsByName[name] = paths;
-                }
-
-                paths.Add(entry.Key);
             }
         }
 
-        var findings = new List<ReferenceFinding>(pathsByName.Count);
+        var findings = new List<ReferenceFinding>(groups.Count);
-        foreach ((string name, SortedSet<string> paths) in pathsByName)
+        foreach ((string key, ReferenceGroup group) in groups)
         {
-            ReferenceStatus status = await ClassifyAsync(session, name, ct).ConfigureAwait(false);
+            ReferenceStatus status = group.Name is { } name
-            findings.Add(new ReferenceFinding(name, status, [.. paths]));
+                ? await ClassifyAsync(session, name, ct).ConfigureAwait(false)
+                : ReferenceStatus.InvalidName;
+            findings.Add(new ReferenceFinding(key, status, [.. group.Paths]));
         }
 
         return findings;
     }
 
+    // Finds (or creates) the group a raw token belongs to. A valid name groups under its normalized value
+    // and carries the parsed SecretName; an invalid token (illegal chars, an unfilled placeholder, ...)
+    // groups under its raw text with no name, so one bad token becomes one InvalidName finding instead of
+    // throwing and aborting the entire audit.
+    private static ReferenceGroup ResolveGroup(
+        SortedDictionary<string, ReferenceGroup> groups, string raw)
+    {
+        string key;
+        SecretName? name;
+        try
+        {
+            var parsed = new SecretName(raw);
+            key = parsed.Value;
+            name = parsed;
+        }
+        catch (ArgumentException)
+        {
+            key = raw;
+            name = null;
+        }
+
+        if (!groups.TryGetValue(key, out ReferenceGroup? group))
+        {
+            group = new ReferenceGroup(name);
+            groups[key] = group;
+        }
+
+        return group;
+    }
+
+    // A configuration key is a comment/documentation key when its leaf segment (after the last ':') starts
+    // with '_'. Duplicated from SecretReferenceExpander.IsCommentKey — keep the two in lockstep.
+    private static bool IsCommentKey(string key)
+    {
+        int lastSeparator = key.LastIndexOf(':');
+        string leaf = lastSeparator >= 0 ? key[(lastSeparator + 1)..] : key;
+        return leaf.StartsWith('_');
+    }
+
     // Present ⇒ tombstone-check ⇒ (degraded ⇒ presence-only) ⇒ decrypt-probe. The decrypted plaintext is
     // discarded immediately (never stored in a finding); only the success/failure signal is kept.
     private static async Task<ReferenceStatus> ClassifyAsync(
-        SecretsSession session, string name, CancellationToken ct)
+        SecretsSession session, SecretName name, CancellationToken ct)
     {
-        StoredSecret? row = await session.Store.GetAsync(new SecretName(name), ct).ConfigureAwait(false);
+        StoredSecret? row = await session.Store.GetAsync(name, ct).ConfigureAwait(false);
         if (row is null)
         {
             return ReferenceStatus.Missing;
@@ -119,4 +169,13 @@ public sealed class ReferenceAuditor
             return ReferenceStatus.Undecryptable;
         }
     }
+
+    // A collated reference: the config key paths that use it, plus the parsed name (null when the token is
+    // not a valid SecretName — an InvalidName finding).
+    private sealed class ReferenceGroup(SecretName? name)
+    {
+        public SortedSet<string> Paths { get; } = new(StringComparer.Ordinal);
+
+        public SecretName? Name { get; } = name;
+    }
 }

ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Cli/Interactive/ReferenceAuditorTests.cs

@@ -157,6 +157,50 @@ public sealed class ReferenceAuditorTests : IDisposable
         Assert.Equal(ReferenceStatus.Undecryptable, Assert.Single(findings).Status);
     }
 
+    [Fact]
+    public async Task Malformed_reference_yields_InvalidName_finding_without_aborting_audit()
+    {
+        SecretsSession session = await Open(EnvKey(), Config(
+            ("Bad:Ref", "${secret:REPLACE ME!}"),
+            ("Good:Ref", "${secret:api/key}")));
+
+        IReadOnlyList<ReferenceFinding> findings =
+            await new ReferenceAuditor().AuditAsync(session, CancellationToken.None);
+
+        Assert.Equal(2, findings.Count);
+        Assert.Equal(ReferenceStatus.InvalidName, findings.Single(f => f.SecretName == "REPLACE ME!").Status);
+        Assert.Equal(ReferenceStatus.Missing, findings.Single(f => f.SecretName == "api/key").Status);
+    }
+
+    [Fact]
+    public async Task Comment_keys_are_skipped_like_the_expander()
+    {
+        SecretsSession session = await Open(EnvKey(), Config(
+            ("Docs:_comment", "Reference a secret like ${secret:example/thing}"),
+            ("Real:Key", "${secret:api/key}")));
+
+        IReadOnlyList<ReferenceFinding> findings =
+            await new ReferenceAuditor().AuditAsync(session, CancellationToken.None);
+
+        ReferenceFinding finding = Assert.Single(findings);
+        Assert.Equal("api/key", finding.SecretName);
+    }
+
+    [Fact]
+    public async Task Case_variant_references_collapse_to_one_normalized_finding()
+    {
+        SecretsSession session = await Open(EnvKey(), Config(
+            ("A:Conn", "${secret:Db/Pw}"),
+            ("B:Conn", "${secret:db/pw}")));
+
+        IReadOnlyList<ReferenceFinding> findings =
+            await new ReferenceAuditor().AuditAsync(session, CancellationToken.None);
+
+        ReferenceFinding finding = Assert.Single(findings);
+        Assert.Equal("db/pw", finding.SecretName);
+        Assert.Equal(["A:Conn", "B:Conn"], finding.ConfigPaths);
+    }
+
     [Fact]
     public async Task Degraded_session_reports_PresenceOnly_status()
     {