fix(secrets): store-path rules compare locations, not spellings (scadaproj#45)

Secrets-005: AddIfNotRooted now uses Path.IsPathFullyQualified, so Windows drive-relative (C:secrets.db) and root-relative (\secrets.db) paths are rejected. AddIfUnderContentRoot refuses a store if EITHER its spelled location (device prefix stripped, loopback admin share mapped to its drive) OR its resolved location (symlinks/junctions followed, 8.3 names expanded on Windows) is under the likewise-resolved content root. A store spelled through the content root stays refused even if a link inside carries it elsewhere. UNC paths through other shares are compared as written. Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV
2026-11-09 15:09:56 -05:00
parent b64628de0b
commit eff8c6a00e
3 changed files with 315 additions and 13 deletions
@@ -65,7 +65,8 @@ public sealed class SecretsOptionsValidator : OptionsValidatorBase<SecretsOption

        SecretsStorePathRules.AddIfNotRooted(
            options.SqlitePath,
-            "Secrets:SqlitePath must be an absolute path. A relative value resolves against the "
+            "Secrets:SqlitePath must be an absolute (fully qualified) path. A relative value — or, on "
+                + "Windows, a drive-relative C:secrets.db or root-relative \\secrets.db — resolves against the "
                + "process working directory — for a Windows service that is C:\\Windows\\System32, "
                + "so the store lands outside the installation; in a dev loop it lands in the source "
                + "tree. Set an absolute path, or remove the key entirely to accept the "
@@ -1,3 +1,4 @@
+using System.Runtime.InteropServices;
 using ZB.MOM.WW.Configuration;

 namespace ZB.MOM.WW.Secrets.Configuration;
@@ -35,12 +36,27 @@ namespace ZB.MOM.WW.Secrets.Configuration;
 /// a boot error is seen immediately by the person who caused it, whereas a relocated store presents
 /// later as an authentication failure pointing nowhere near the configuration that caused it.
 /// </para>
+/// <para>
+/// Both rules compare <b>locations, not spellings</b> (Secrets-005). "Rooted" means fully qualified:
+/// Windows' drive-relative <c>C:secrets.db</c> and root-relative <c>\secrets.db</c> are rooted but
+/// still resolve against a current directory, so they are rejected. "Under the content root"
+/// compares both the spelled and the resolved location of each side, and fails if either is inside:
+/// symbolic links and junctions on the existing part of each path are followed to their final
+/// target (a store spelled through the content root is still refused even if a link inside it
+/// leads elsewhere — the next deploy's tree has no such link), and on Windows the <c>\\?\</c> / <c>\\.\</c> device
+/// prefixes are removed, a loopback administrative share (<c>\\localhost\C$\…</c>,
+/// <c>\\127.0.0.1\C$\…</c>, <c>\\&lt;this machine&gt;\C$\…</c>) is mapped back to its drive, and 8.3
+/// short names (<c>PROGRA~1</c>) are expanded. A UNC path through any other share is compared as
+/// written — what a share points at is server configuration this rule cannot see.
+/// </para>
 /// </remarks>
 public static class SecretsStorePathRules
 {
    /// <summary>
-    /// Records a failure when <paramref name="value"/> is non-blank and not an absolute (rooted)
-    /// path on the current OS. Blank is the caller's required-field concern and passes here.
+    /// Records a failure when <paramref name="value"/> is non-blank and not a fully qualified path on
+    /// the current OS (<see cref="Path.IsPathFullyQualified(string)"/>: on Windows a drive-relative
+    /// <c>C:secrets.db</c> or root-relative <c>\secrets.db</c> is rejected, because each still resolves
+    /// against a current directory). Blank is the caller's required-field concern and passes here.
    /// </summary>
    /// <param name="value">The configured path value.</param>
    /// <param name="message">The failure message to record.</param>
@@ -54,7 +70,7 @@ public static class SecretsStorePathRules
            return;
        }

-        if (!Path.IsPathRooted(value))
+        if (!Path.IsPathFullyQualified(value))
        {
            builder.Add(message);
        }
@@ -83,21 +99,39 @@ public static class SecretsStorePathRules
            return;
        }

-        string fullValue;
-        string fullRoot;
+        // Two readings, and the store is inside if EITHER says so:
+        //  - as SPELLED (full path, device prefix / loopback share normalised): a store configured
+        //    through the content root moves with it on a rename even when a link inside carries it
+        //    elsewhere today — the next deploy's tree has no such link, and the store is recreated
+        //    empty at the same spelling;
+        //  - as LOCATED (symlinks, junctions and 8.3 names resolved): a store spelled through an
+        //    alias of the content root, or a content root reached through a "current" link, sits in
+        //    the directory the deploy replaces.
+        string spelledValue, spelledRoot, locatedValue, locatedRoot;
        try
        {
-            fullValue = Path.GetFullPath(value);
-            fullRoot = Path.GetFullPath(contentRoot);
+            spelledValue = Spelled(value);
+            spelledRoot = Spelled(contentRoot);
+            locatedValue = Canonicalize(value);
+            locatedRoot = Canonicalize(contentRoot);
        }
        catch (Exception ex) when (ex is ArgumentException or NotSupportedException
-                                      or PathTooLongException or IOException)
+                                      or PathTooLongException or IOException
+                                      or UnauthorizedAccessException)
        {
            // A syntactically invalid path is a different failure with its own message; refusing
            // to guess here keeps one rule reporting one thing.
            return;
        }

+        if (IsUnder(spelledValue, spelledRoot) || IsUnder(locatedValue, locatedRoot))
+        {
+            builder.Add(message);
+        }
+    }
+
+    private static bool IsUnder(string fullValue, string fullRoot)
+    {
        // The separator is load-bearing, not cosmetic. Without it a bare prefix test treats
        // "/srv/app-data" as inside "/srv/app" and refuses a legitimate sibling directory — and
        // because this rule is fail-closed at startup, that false positive is an application that
@@ -112,9 +146,125 @@ public static class SecretsStorePathRules
            ? StringComparison.OrdinalIgnoreCase
            : StringComparison.Ordinal;

-        if (fullValue.StartsWith(rootWithSeparator, comparison))
-        {
-            builder.Add(message);
-        }
+        return fullValue.StartsWith(rootWithSeparator, comparison);
    }
+
+    private static string Spelled(string path) => OperatingSystem.IsWindows()
+        ? NormalizeWindowsSpelling(Path.GetFullPath(NormalizeWindowsSpelling(path)))
+        : Path.GetFullPath(path);
+
+    private const int MaxLinkDepth = 40;
+
+    /// <summary>
+    /// Returns the location <paramref name="path"/> names, independent of how it is spelled: full
+    /// path, Windows device prefix stripped and loopback admin share mapped to its drive, and every
+    /// existing component resolved through symbolic links / junctions (and, on Windows, 8.3 short
+    /// names expanded). Components that do not exist yet are appended as written.
+    /// </summary>
+    /// <param name="path">The path to canonicalise.</param>
+    /// <returns>The canonical full path.</returns>
+    internal static string Canonicalize(string path) => Canonicalize(path, depth: 0);
+
+    private static string Canonicalize(string path, int depth)
+    {
+        if (depth > MaxLinkDepth)
+        {
+            throw new IOException("Too many levels of symbolic links.");
+        }
+
+        string full = Spelled(path);
+
+        string root = Path.GetPathRoot(full) ?? string.Empty;
+        string[] parts = full[root.Length..].Split(
+            new[] { Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar },
+            StringSplitOptions.RemoveEmptyEntries);
+
+        string current = root;
+        for (int i = 0; i < parts.Length; i++)
+        {
+            string next = Path.Combine(current, parts[i]);
+            FileSystemInfo? info = Directory.Exists(next) ? new DirectoryInfo(next)
+                : File.Exists(next) ? new FileInfo(next)
+                : null;
+
+            if (info is null)
+            {
+                // Not created yet (the store usually is not, at validation time): nothing on disk
+                // can redirect the rest, so append it as written.
+                return Path.Combine(new[] { current }.Concat(parts[i..]).ToArray());
+            }
+
+            if (info.LinkTarget is not null)
+            {
+                FileSystemInfo? target = info.ResolveLinkTarget(returnFinalTarget: true);
+                if (target is not null)
+                {
+                    // The target's own parent directories may be links too: canonicalise it whole.
+                    next = Canonicalize(target.FullName, depth + 1);
+                }
+            }
+            else if (OperatingSystem.IsWindows())
+            {
+                next = ExpandShortName(next);
+            }
+
+            current = next;
+        }
+
+        return current;
+    }
+
+    /// <summary>
+    /// Strips the Win32 device prefixes (<c>\\?\C:\…</c>, <c>\\.\C:\…</c>, <c>\\?\UNC\…</c>) and maps
+    /// a loopback administrative share (<c>\\localhost\C$\…</c>) back to its drive.
+    /// </summary>
+    private static string NormalizeWindowsSpelling(string path)
+    {
+        string p = path.Replace('/', '\\');
+        if (p.StartsWith(@"\\?\UNC\", StringComparison.OrdinalIgnoreCase))
+        {
+            p = @"\\" + p[8..];
+        }
+        else if (p.StartsWith(@"\\?\", StringComparison.Ordinal) || p.StartsWith(@"\\.\", StringComparison.Ordinal))
+        {
+            p = p[4..];
+        }
+
+        if (p.StartsWith(@"\\", StringComparison.Ordinal))
+        {
+            string[] segments = p[2..].Split('\\');
+            if (segments.Length >= 2
+                && IsLoopbackHost(segments[0])
+                && segments[1].Length == 2
+                && char.IsAsciiLetter(segments[1][0])
+                && segments[1][1] == '$')
+            {
+                p = segments[1][0] + @":\" + string.Join('\\', segments[2..]);
+            }
+        }
+
+        return p;
+    }
+
+    private static bool IsLoopbackHost(string host) =>
+        host.Equals("localhost", StringComparison.OrdinalIgnoreCase)
+        || host.Equals("127.0.0.1", StringComparison.Ordinal)
+        || host.Equals("::1", StringComparison.Ordinal)
+        || host.Equals("[::1]", StringComparison.Ordinal)
+        || host.Equals(Environment.MachineName, StringComparison.OrdinalIgnoreCase);
+
+    private static string ExpandShortName(string existingPath)
+    {
+        if (!existingPath.Contains('~'))
+        {
+            return existingPath;
+        }
+
+        char[] buffer = new char[1024];
+        uint length = GetLongPathNameW(existingPath, buffer, (uint)buffer.Length);
+        return length > 0 && length < buffer.Length ? new string(buffer, 0, (int)length) : existingPath;
+    }
+
+    [DllImport("kernel32.dll", CharSet = CharSet.Unicode, SetLastError = true)]
+    private static extern uint GetLongPathNameW(string shortPath, [Out] char[] longPath, uint bufferLength);
 }
@@ -0,0 +1,151 @@
+using ZB.MOM.WW.Configuration;
+using ZB.MOM.WW.Secrets.Configuration;
+
+namespace ZB.MOM.WW.Secrets.Tests.Configuration;
+
+/// <summary>
+/// Secrets-005: the store-path rules must compare locations, not spellings. A path that reaches
+/// the content root through a symbolic link (or a content root that is itself reached through one)
+/// is still inside it; on Windows a drive-relative path is not "absolute", and device-prefixed,
+/// loopback-admin-share and 8.3 spellings of a path inside the content root are still inside it.
+/// </summary>
+public sealed class SecretsStorePathSpellingTests : IDisposable
+{
+    private readonly string _base;
+
+    public SecretsStorePathSpellingTests()
+    {
+        _base = Path.Combine(Path.GetTempPath(), $"zb-secrets-spelling-{Guid.NewGuid():N}");
+        Directory.CreateDirectory(Path.Combine(_base, "app", "data"));
+        Directory.CreateDirectory(Path.Combine(_base, "outside"));
+    }
+
+    public void Dispose()
+    {
+        try { Directory.Delete(_base, recursive: true); } catch (IOException) { }
+    }
+
+    private static bool Inside(string value, string root)
+    {
+        var builder = new ValidationBuilder();
+        SecretsStorePathRules.AddIfUnderContentRoot(value, root, "inside", builder);
+        return !builder.IsValid;
+    }
+
+    private static bool NotRooted(string value)
+    {
+        var builder = new ValidationBuilder();
+        SecretsStorePathRules.AddIfNotRooted(value, "relative", builder);
+        return !builder.IsValid;
+    }
+
+    private bool TryLink(string link, string target)
+    {
+        try
+        {
+            Directory.CreateSymbolicLink(link, target);
+            return true;
+        }
+        catch (Exception ex) when (ex is IOException or UnauthorizedAccessException)
+        {
+            return false; // Windows without the symlink privilege
+        }
+    }
+
+    [SkippableFact]
+    public void A_store_reached_through_a_symlink_into_the_content_root_is_inside_it()
+    {
+        string root = Path.Combine(_base, "app");
+        string link = Path.Combine(_base, "alias");
+        Skip.IfNot(TryLink(link, root), "cannot create a symbolic link here");
+
+        // Positive control: the same file spelled directly is caught today.
+        Assert.True(Inside(Path.Combine(root, "data", "secrets.db"), root));
+        Assert.True(Inside(Path.Combine(link, "data", "secrets.db"), root));
+    }
+
+    [SkippableFact]
+    public void A_content_root_reached_through_a_current_symlink_still_contains_its_store()
+    {
+        // The "current -> releases/42" deployment shape: the host reports the link, the store is
+        // configured with the real directory (or the other way round).
+        string real = Path.Combine(_base, "app");
+        string current = Path.Combine(_base, "current");
+        Skip.IfNot(TryLink(current, real), "cannot create a symbolic link here");
+
+        Assert.True(Inside(Path.Combine(real, "data", "secrets.db"), current));
+        Assert.True(Inside(Path.Combine(current, "data", "secrets.db"), real));
+    }
+
+    [SkippableFact]
+    public void A_store_spelled_through_the_content_root_stays_refused_even_if_a_link_leads_out()
+    {
+        // A "durable" link inside the app directory pointing at storage elsewhere: the bytes survive
+        // a rename, but the next deploy's tree has no such link, so the configured spelling then
+        // names a fresh empty store inside the new tree — the 2026-08-09 shape. Resolving links
+        // must ADD cases, never remove the spelled one.
+        string root = Path.Combine(_base, "app");
+        string link = Path.Combine(root, "durable");
+        Skip.IfNot(TryLink(link, Path.Combine(_base, "outside")), "cannot create a symbolic link here");
+
+        Assert.True(Inside(Path.Combine(link, "secrets.db"), root));
+        Assert.False(Inside(Path.Combine(_base, "outside", "secrets.db"), root)); // control
+    }
+
+    [Fact]
+    public void A_not_yet_created_store_is_compared_as_written()
+    {
+        string root = Path.Combine(_base, "app");
+        Assert.True(Inside(Path.Combine(root, "new", "deeper", "secrets.db"), root));
+        Assert.False(Inside(Path.Combine(_base, "outside", "new", "secrets.db"), root));
+    }
+
+    [SkippableFact]
+    public void Drive_and_root_relative_paths_are_not_absolute_on_windows()
+    {
+        Skip.IfNot(OperatingSystem.IsWindows(), "Windows path semantics");
+        Assert.True(NotRooted(@"C:secrets.db"));
+        Assert.True(NotRooted(@"\secrets.db"));
+        Assert.False(NotRooted(@"C:\data\secrets.db"));
+        Assert.False(NotRooted(@"\\server\share\secrets.db"));
+    }
+
+    [SkippableFact]
+    public void Device_prefixed_and_loopback_share_spellings_are_inside_on_windows()
+    {
+        Skip.IfNot(OperatingSystem.IsWindows(), "Windows path semantics");
+        string root = Path.Combine(_base, "app");
+        string full = Path.GetFullPath(Path.Combine(root, "data", "secrets.db"));
+        string drive = full[..1];
+        string rest = full[3..];
+
+        Assert.True(Inside(@"\\?\" + full, root));
+        Assert.True(Inside(@"\\.\" + full, root));
+        Assert.True(Inside($@"\\localhost\{drive}$\{rest}", root));
+        Assert.True(Inside($@"\\127.0.0.1\{drive}$\{rest}", root));
+        Assert.True(Inside($@"\\?\UNC\localhost\{drive}$\{rest}", root));
+    }
+
+    [SkippableFact]
+    public void An_8_3_short_name_spelling_is_inside_on_windows()
+    {
+        Skip.IfNot(OperatingSystem.IsWindows(), "Windows path semantics");
+        string longDir = Path.Combine(_base, "Application Directory");
+        Directory.CreateDirectory(longDir);
+        string? shortDir = ShortName(longDir);
+        Skip.If(shortDir is null || !shortDir.Contains('~'), "8.3 names are disabled on this volume");
+
+        Assert.True(Inside(Path.Combine(shortDir!, "secrets.db"), longDir));
+        Assert.True(Inside(Path.Combine(longDir, "secrets.db"), shortDir!));
+    }
+
+    private static string? ShortName(string path)
+    {
+        char[] buffer = new char[1024];
+        uint n = GetShortPathNameW(path, buffer, (uint)buffer.Length);
+        return n == 0 ? null : new string(buffer, 0, (int)n);
+    }
+
+    [System.Runtime.InteropServices.DllImport("kernel32.dll", CharSet = System.Runtime.InteropServices.CharSet.Unicode)]
+    private static extern uint GetShortPathNameW(string longPath, [System.Runtime.InteropServices.Out] char[] shortPath, uint size);
+}