From 467a97f5404b7cb14026783154ea6a23f6f740ff Mon Sep 17 00:00:00 2001 From: Aqua Date: Sat, 7 Nov 2026 11:04:00 +0000 Subject: [PATCH] Trusted root: lexical check before any filesystem access, cached env setting, stricter log escaping Review (Heimdall): a path outside the pinned root (as resolved or as given) is refused by string comparison before File.Exists or any link is followed; the trusted-root setting is its own small type that takes the environment variable once, caches it and refuses blank values; Printable also escapes backslash, format characters (bidi, zero-width) and the line and paragraph separators; dev-watch matches the host ack by the unique slot leaf with doubled backslashes; dev-reload prunes live-slots and rejects stale reopen state up front. Co-Authored-By: Claude Sonnet 5.5 --- scripts/dev-reload.ps1 | 21 +++- scripts/dev-watch.ps1 | 7 +- src/Aquarium.Engine/AquariumHost.cs | 25 +---- .../Runtime/ClientRuntimeLoader.cs | 40 +++++-- .../Runtime/TrustedRootSetting.cs | 48 +++++++++ .../ClientRuntimeLoaderTrustTests.cs | 101 +++++++++++++++--- 6 files changed, 196 insertions(+), 46 deletions(-) create mode 100644 src/Aquarium.Engine/Runtime/TrustedRootSetting.cs diff --git a/scripts/dev-reload.ps1 b/scripts/dev-reload.ps1 index aa86abb..92e7bc0 100644 --- a/scripts/dev-reload.ps1 +++ b/scripts/dev-reload.ps1 @@ -22,6 +22,7 @@ $devRoot = Join-Path $repoRoot "artifacts\dev-reload" # it is the host's trusted root, so state, cache and logs in $devRoot are not part of it. $assemblyRoot = Join-Path $devRoot "assemblies" $slotRoot = Join-Path $assemblyRoot "slots" +$liveSlotRoot = Join-Path $assemblyRoot "live-slots" $visibleStatePath = Join-Path $devRoot "state.clixml" $headlessStatePath = Join-Path $devRoot "headless-state.clixml" $statePath = if ($Headless) { $headlessStatePath } else { $visibleStatePath } @@ -82,9 +83,12 @@ function Remove-OldSlots { return } - $slots = Get-ChildItem -Path $slotRoot -Directory -ErrorAction SilentlyContinue | - Sort-Object LastWriteTimeUtc -Descending | - Select-Object -Skip $RetainSlots + # Both roots hold client code the host may load; dev-watch adds a live slot per change and never prunes. + $slots = foreach ($root in @($slotRoot, $liveSlotRoot)) { + Get-ChildItem -Path $root -Directory -ErrorAction SilentlyContinue | + Sort-Object LastWriteTimeUtc -Descending | + Select-Object -Skip $RetainSlots + } foreach ($slot in $slots) { try { @@ -245,6 +249,17 @@ function Reopen-VisibleProcess { $liveAssemblyPath = [string]$state.liveAssembly } + # Stale state fails here, in words, instead of as a refused load inside the host. + $assemblyRootPrefix = [System.IO.Path]::GetFullPath($assemblyRoot).TrimEnd('\') + '\' + foreach ($recorded in @($slotPath, $liveAssemblyPath)) { + if (-not [System.IO.Path]::GetFullPath($recorded).StartsWith($assemblyRootPrefix, [System.StringComparison]::OrdinalIgnoreCase)) { + throw "Visible dev-reload state names '$recorded', outside the assemblies root '$assemblyRoot' (state from before the trusted-root layout, or the root moved). Run dev-reload without -Reopen to rebuild." + } + } + if (-not (Test-Path -LiteralPath $liveAssemblyPath)) { + throw "Visible dev-reload state names '$liveAssemblyPath', which no longer exists (pruned slot). Run dev-reload without -Reopen to rebuild." + } + $process = Start-AquariumProcess -SlotPath $slotPath -LiveAssemblyPath $liveAssemblyPath -RunHeadless:$false Wait-ForStartedAquarium -Process $process -ExpectWindow:$true diff --git a/scripts/dev-watch.ps1 b/scripts/dev-watch.ps1 index 10bd9ca..0b9c9cb 100644 --- a/scripts/dev-watch.ps1 +++ b/scripts/dev-watch.ps1 @@ -455,12 +455,15 @@ function Wait-LiveReloadAcknowledged { $stderrLog = if ($state.stderrLog) { [string]$state.stderrLog } else { $null } $timeoutSeconds = [Math]::Max(1, $LiveReloadAckTimeoutSeconds) $deadline = (Get-Date).AddSeconds($timeoutSeconds) - $appliedLine = "Client runtime reload applied: $liveAssemblyPath" + # The host logs the resolved path with backslashes doubled, which can differ from the path built here when the + # repo sits under a link. The slot directory name is unique, so match its leaf and the assembly name. + $slotLeaf = Split-Path -Leaf (Split-Path -Parent $liveAssemblyPath) + $appliedSuffix = ("$slotLeaf\" + (Split-Path -Leaf $liveAssemblyPath)).Replace("\", "\\") while ((Get-Date) -lt $deadline) { if (Test-Path $stdoutLog) { $stdoutTail = (Get-Content -Path $stdoutLog -Tail 120 -ErrorAction SilentlyContinue) -join "`n" - if ($stdoutTail.Contains($appliedLine)) { + if ($stdoutTail -match ("(?m)^Client runtime reload applied: .*" + [regex]::Escape($appliedSuffix) + "\s*$")) { Write-WatchLog "Live reload acknowledged by host: $liveAssemblyPath" return } diff --git a/src/Aquarium.Engine/AquariumHost.cs b/src/Aquarium.Engine/AquariumHost.cs index edf5d42..1b0d430 100644 --- a/src/Aquarium.Engine/AquariumHost.cs +++ b/src/Aquarium.Engine/AquariumHost.cs @@ -258,30 +258,9 @@ public static class AquariumHost ?? Environment.GetEnvironmentVariable("AQUARIUM_LIVE_ASSEMBLY"); } - internal static string? ParseClientTrustedRoot(IReadOnlyCollection args) - { - // Read once and removed on every path, so processes this one starts do not inherit a root meant for it. - var fromEnvironment = Environment.GetEnvironmentVariable("AQUARIUM_CLIENT_TRUSTED_ROOT"); - Environment.SetEnvironmentVariable("AQUARIUM_CLIENT_TRUSTED_ROOT", null); + private static readonly TrustedRootSetting TrustedRoot = TrustedRootSetting.FromProcessEnvironment(); - var values = args.ToArray(); - for (var index = 0; index < values.Length; index++) - { - if (!string.Equals(values[index], "--client-trusted-root", StringComparison.OrdinalIgnoreCase)) - { - continue; - } - - if (index + 1 >= values.Length || string.IsNullOrWhiteSpace(values[index + 1])) - { - throw new ArgumentException("--client-trusted-root needs a directory; an empty value would fall back to the default root."); - } - - return values[index + 1]; - } - - return fromEnvironment; - } + private static string? ParseClientTrustedRoot(IReadOnlyCollection args) => TrustedRoot.Resolve(args); private static string? ParseClientReloadPointerPath(IReadOnlyCollection args) { diff --git a/src/Aquarium.Engine/Runtime/ClientRuntimeLoader.cs b/src/Aquarium.Engine/Runtime/ClientRuntimeLoader.cs index a032d96..a9c227a 100644 --- a/src/Aquarium.Engine/Runtime/ClientRuntimeLoader.cs +++ b/src/Aquarium.Engine/Runtime/ClientRuntimeLoader.cs @@ -9,6 +9,7 @@ public sealed class ClientRuntimeLoader : IDisposable private readonly string? clientAssemblyPath; private readonly string? clientReloadPointerPath; private readonly string? trustedRoot; + private readonly string? trustedRootAsGiven; private readonly Stopwatch reloadClock = Stopwatch.StartNew(); private readonly AquariumRuntimeOptions options; private ReloadedRuntime? current; @@ -34,6 +35,7 @@ public sealed class ClientRuntimeLoader : IDisposable var root = string.IsNullOrWhiteSpace(trustedRoot) ? this.clientAssemblyPath is null ? null : Path.GetDirectoryName(this.clientAssemblyPath) : Path.GetFullPath(trustedRoot); + this.trustedRootAsGiven = root; if (root is not null) { root = ResolveLinks(root); @@ -144,6 +146,7 @@ public sealed class ClientRuntimeLoader : IDisposable try { RequireLocalFullyQualified(candidatePath); // string rules first: no filesystem or network probe of a UNC path + RequireLexicallyUnderRoot(candidatePath); // and no probe of anything outside the root, whatever a link there points at if (!File.Exists(candidatePath)) { return; @@ -171,15 +174,34 @@ public sealed class ClientRuntimeLoader : IDisposable private string ResolveTrusted(string assemblyPath) => trustedRoot is null ? throw new InvalidOperationException("No trusted root for client runtime assemblies is set.") - : ResolveTrusted(trustedRoot, assemblyPath); + : ResolveTrusted(trustedRoot, assemblyPath, trustedRootAsGiven); + + private void RequireLexicallyUnderRoot(string path) => + RequireLexicallyUnder(Path.GetFullPath(path), trustedRoot, trustedRootAsGiven); + + // Pure string comparison against the root as resolved and as given: a path outside both is refused before any + // filesystem access, so a link there (to a UNC share, a mapped drive) is never followed or probed. + private static void RequireLexicallyUnder(string fullPath, params string?[] roots) + { + if (!roots.Any(root => root is not null && IsUnder(root, fullPath))) + { + throw new InvalidOperationException($"Client runtime path is outside the trusted root '{Printable(roots[0] ?? "")}': {Printable(fullPath)}"); + } + } + + private static bool IsUnder(string root, string path) + { + var relative = Path.GetRelativePath(root, path); + return !(relative == ".." || relative.StartsWith(".." + Path.DirectorySeparatorChar, StringComparison.Ordinal) || Path.IsPathRooted(relative)); + } // The final path of a local, fully qualified file under the already-resolved root; throws otherwise. - internal static string ResolveTrusted(string resolvedRoot, string path) + internal static string ResolveTrusted(string resolvedRoot, string path, string? rootAsGiven = null) { RequireLocalFullyQualified(path); + RequireLexicallyUnder(Path.GetFullPath(path), resolvedRoot, rootAsGiven); var resolved = ResolveLinks(Path.GetFullPath(path)); - var relative = Path.GetRelativePath(resolvedRoot, resolved); - if (relative == ".." || relative.StartsWith(".." + Path.DirectorySeparatorChar, StringComparison.Ordinal) || Path.IsPathRooted(relative)) + if (!IsUnder(resolvedRoot, resolved)) { throw new InvalidOperationException($"Client runtime path is outside the trusted root '{Printable(resolvedRoot)}': {Printable(path)}"); } @@ -195,13 +217,19 @@ public sealed class ClientRuntimeLoader : IDisposable } } - // Paths come from files other people can write: show control characters escaped, never as raw log lines. + // Paths come from files other people can write: backslashes, control, format (bidi, zero-width) and line/paragraph + // separator characters are escaped, so a name cannot forge or hide a log line. internal static string Printable(string text) { var builder = new System.Text.StringBuilder(text.Length); foreach (var c in text) { - if (char.IsControl(c)) + if (c == '\\') + { + builder.Append("\\\\"); + } + else if (char.IsControl(c) || char.GetUnicodeCategory(c) is System.Globalization.UnicodeCategory.Format + or System.Globalization.UnicodeCategory.LineSeparator or System.Globalization.UnicodeCategory.ParagraphSeparator) { builder.Append(c switch { '\n' => "\\n", '\r' => "\\r", '\t' => "\\t", _ => $"\\u{(int)c:x4}" }); } diff --git a/src/Aquarium.Engine/Runtime/TrustedRootSetting.cs b/src/Aquarium.Engine/Runtime/TrustedRootSetting.cs new file mode 100644 index 0000000..f99bd2e --- /dev/null +++ b/src/Aquarium.Engine/Runtime/TrustedRootSetting.cs @@ -0,0 +1,48 @@ +namespace Aquarium.Engine.Runtime; + +/// +/// The client runtime trusted root as given on the command line or in AQUARIUM_CLIENT_TRUSTED_ROOT. The environment +/// value is taken (read and cleared) the first time anything is resolved, on every path, and cached: children do not +/// inherit it and a second resolution does not silently fall back to the default root. A blank flag value, a flag with +/// no value or a blank environment value is refused. +/// +internal sealed class TrustedRootSetting(Func takeEnvironment) +{ + private const string Flag = "--client-trusted-root"; + private const string Variable = "AQUARIUM_CLIENT_TRUSTED_ROOT"; + private readonly Lazy fromEnvironment = new(takeEnvironment); + + public static TrustedRootSetting FromProcessEnvironment() => new(() => + { + var value = Environment.GetEnvironmentVariable(Variable); + Environment.SetEnvironmentVariable(Variable, null); + return value; + }); + + public string? Resolve(IReadOnlyCollection args) + { + var environment = fromEnvironment.Value; + if (environment is not null && string.IsNullOrWhiteSpace(environment)) + { + throw new ArgumentException($"{Variable} is set but empty; an empty value would fall back to the default root."); + } + + var values = args.ToArray(); + for (var index = 0; index < values.Length; index++) + { + if (!string.Equals(values[index], Flag, StringComparison.OrdinalIgnoreCase)) + { + continue; + } + + if (index + 1 >= values.Length || string.IsNullOrWhiteSpace(values[index + 1])) + { + throw new ArgumentException($"{Flag} needs a directory; an empty value would fall back to the default root."); + } + + return values[index + 1]; + } + + return environment; + } +} diff --git a/tests/Aquarium.Engine.Tests/ClientRuntimeLoaderTrustTests.cs b/tests/Aquarium.Engine.Tests/ClientRuntimeLoaderTrustTests.cs index bebacc8..2dde1e6 100644 --- a/tests/Aquarium.Engine.Tests/ClientRuntimeLoaderTrustTests.cs +++ b/tests/Aquarium.Engine.Tests/ClientRuntimeLoaderTrustTests.cs @@ -254,24 +254,87 @@ public sealed class ClientRuntimeLoaderTrustTests : IDisposable } [Fact] - public void TheTrustedRootEnvironmentVariableIsClearedOnEveryPathAndAnEmptyFlagIsRefused() + public void TheTrustedRootEnvironmentVariableIsTakenOnceOnEveryPathAndCached() { const string Name = "AQUARIUM_CLIENT_TRUSTED_ROOT"; Environment.SetEnvironmentVariable(Name, "/from/env"); - Assert.Equal("/from/flag", AquariumHost.ParseClientTrustedRoot(["--client-trusted-root", "/from/flag"])); - Assert.Null(Environment.GetEnvironmentVariable(Name)); + var setting = TrustedRootSetting.FromProcessEnvironment(); + Assert.Equal("/from/flag", setting.Resolve(["--client-trusted-root", "/from/flag"])); + Assert.Null(Environment.GetEnvironmentVariable(Name)); // cleared although the flag won + Assert.Equal("/from/env", setting.Resolve([])); // cached: a second resolution does not fall to the default root + } - Environment.SetEnvironmentVariable(Name, "/from/env"); - Assert.Equal("/from/env", AquariumHost.ParseClientTrustedRoot([])); - Assert.Null(Environment.GetEnvironmentVariable(Name)); + [Theory] + [InlineData("")] + [InlineData(" ")] + public void ABlankEnvironmentValueIsRefusedLikeABlankFlag(string value) + { + Assert.Throws(() => new TrustedRootSetting(() => value).Resolve([])); + Assert.Throws(() => new TrustedRootSetting(() => null).Resolve(["--client-trusted-root", value])); + Assert.Throws(() => new TrustedRootSetting(() => null).Resolve(["--client-trusted-root"])); + } - Environment.SetEnvironmentVariable(Name, "/from/env"); - Assert.Throws(() => AquariumHost.ParseClientTrustedRoot(["--client-trusted-root", ""])); - Assert.Null(Environment.GetEnvironmentVariable(Name)); + [Fact] + public void APathOutsideTheRootIsRefusedBeforeAnyLinkOnItIsFollowed() + { + // A link loop outside the root: following it would throw "Too many links"; the string check must refuse first. + var a = Path.Combine(outside, "a"); + var b = Path.Combine(outside, "b"); + try + { + Directory.CreateSymbolicLink(a, b); + Directory.CreateSymbolicLink(b, a); + } + catch (Exception error) when (error is IOException or UnauthorizedAccessException) + { + // No links here: the check is still exercised by the nonexistent outside path below. + } - Environment.SetEnvironmentVariable(Name, "/from/env"); - Assert.Throws(() => AquariumHost.ParseClientTrustedRoot(["--client-trusted-root"])); - Assert.Null(Environment.GetEnvironmentVariable(Name)); + var error2 = Assert.Throws( + () => ClientRuntimeLoader.ResolveTrusted(ClientRuntimeLoader.ResolveLinks(trusted), Path.Combine(a, "client.dll"))); + Assert.Contains("outside the trusted root", error2.Message); + } + + [Fact] + public void ANonexistentPointerTargetOutsideTheRootIsRefusedAndReportedNotSilentlyIgnored() + { + var pointer = Path.Combine(work, "pointer.txt"); + File.WriteAllText(pointer, Path.Combine(outside, "not-there.dll")); + using var loader = new ClientRuntimeLoader(Options, Copy(trusted, "a.dll"), pointer); + loader.Load(); + + var errors = CaptureError(() => + { + Thread.Sleep(600); + loader.Update(0.01f, new InputState()); + }); + + Assert.Contains("outside the trusted root", errors); + } + + [SymlinkFact] + public void ARootReachedThroughALinkAsGivenStillAcceptsPathsUnderIt() + { + var real = Directory.CreateDirectory(Path.Combine(work, "real")).FullName; + var rootLink = Path.Combine(work, "rootlink"); + Directory.CreateSymbolicLink(rootLink, real); + Copy(real, "a.dll"); + var viaLink = Path.Combine(rootLink, "b.dll"); + File.Copy(typeof(Aquarium.Sample.Minimal.MinimalRuntime).Assembly.Location, viaLink); + using var loader = new ClientRuntimeLoader(Options, Path.Combine(rootLink, "a.dll"), trustedRoot: rootLink); + loader.Load(); + loader.Reload(viaLink, Options); + } + + [Fact] + public void LoggedPathsEscapeBackslashesLineSeparatorsAndBidiAndZeroWidthControls() + { + Assert.Equal("a\\\\b", ClientRuntimeLoader.Printable("a\\b")); + Assert.Equal("a\\u2028b", ClientRuntimeLoader.Printable("a\u2028b")); + Assert.Equal("a\\u2029b", ClientRuntimeLoader.Printable("a\u2029b")); + Assert.Equal("a\\u202eb", ClientRuntimeLoader.Printable("a\u202eb")); + Assert.Equal("a\\u200bb", ClientRuntimeLoader.Printable("a\u200bb")); + Assert.Equal("plain-name.dll", ClientRuntimeLoader.Printable("plain-name.dll")); } [Fact] @@ -287,6 +350,20 @@ public sealed class ClientRuntimeLoaderTrustTests : IDisposable Assert.Contains("artifacts\\dev-reload\\assemblies\\live-slots", watch); } + [Fact] + public void DevScriptsMatchTheHostAckByItsUniqueSlotLeafPruneLiveSlotsAndRejectStaleReopenState() + { + var scripts = Path.Combine(RepositoryRoot(), "scripts"); + var reload = File.ReadAllText(Path.Combine(scripts, "dev-reload.ps1")); + var watch = File.ReadAllText(Path.Combine(scripts, "dev-watch.ps1")); + + Assert.Contains(".Replace(\"\\\", \"\\\\\")", watch); // the host logs backslashes doubled + Assert.DoesNotContain("Contains($appliedLine)", watch); + Assert.Contains("@($slotRoot, $liveSlotRoot)", reload); + Assert.Contains("outside the assemblies root", reload); + Assert.Contains("no longer exists (pruned slot)", reload); + } + private static string CaptureError(Action action) { var original = Console.Error;