From 8f954236db9995bf9d0eae0c06f6c4b4b819bb74 Mon Sep 17 00:00:00 2001 From: Stuart Meeks Date: Sun, 3 May 2026 04:36:38 +0000 Subject: [PATCH 1/2] security & correctness fixes from external review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - lock before staging mutation (prevents concurrent installer wiping another installer's in-flight staging dir on its way to losing the lock race) - rollback swap on copy failure (restore from .old/ instead of leaving a half-populated install dir) - validate asset name (reject path separators, parent refs, rooted paths — closes a path-traversal vector for malicious sources) - enforce HTTPS in HttpManifestSource by default (plain HTTP defeats SHA-256 verification because the SHA itself is MITM-able); opt in via SelfUpdaterOptions.AllowInsecureManifestSource for tests / dev - TOCTOU-safe install path: UpdateCommand fetches the release once and installs that exact instance. Adds ISelfUpdater.GetLatestRelease Async + InstallAsync(RemoteRelease, ...) overload; keeps parameterless InstallAsync as a documented convenience - realign csproj with sibling repos: GeneratePackageOnBuild only on Release; PackageOutputPath moved from C:\\nuget-local to artifacts/packages (platform-neutral, repo-local, gitignored) 150 tests, all passing (was 125). Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/ci.yml | 2 +- .gitignore | 5 +- CHANGELOG.md | 10 + .../Commands/UpdateCommand.cs | 30 ++- .../ISelfUpdater.cs | 53 +++-- ...Iteration.SpectreConsole.SelfUpdate.csproj | 4 +- .../Pipeline/SelfUpdater.cs | 9 + .../Pipeline/UpdateInstaller.cs | 149 ++++++++++++-- .../SelfUpdaterOptions.cs | 12 ++ .../ServiceCollectionExtensions.cs | 3 +- .../Sources/HttpManifestSource.cs | 32 ++- .../Commands/UpdateCommandTests.cs | 124 +++++++++--- .../Infrastructure/Stubs.cs | 8 + .../Pipeline/UpdateInstallerTests.cs | 188 ++++++++++++++++++ .../Sources/HttpManifestSourceTests.cs | 61 ++++++ 15 files changed, 611 insertions(+), 79 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0c10eb9..c937996 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,7 +31,7 @@ jobs: run: dotnet build --configuration Release --no-restore - name: Pack - run: dotnet pack --configuration Release --no-build --output ./artifacts -p:PackageOutputPath=${{ github.workspace }}/artifacts + run: dotnet pack --configuration Release --no-build --output ./artifacts - name: Upload package artifact uses: actions/upload-artifact@v6 diff --git a/.gitignore b/.gitignore index 2ec9ed1..da2d068 100644 --- a/.gitignore +++ b/.gitignore @@ -437,8 +437,5 @@ FodyWeavers.xsd *.msm *.msp -# PackageOutputPath in csproj uses Windows path C:\nuget-local\ — on Linux/macOS -# this resolves as a literal `C:/` directory under the project. CI overrides the -# path; ignore the leak. -**/C:/ +# Local NuGet pack output (csproj writes to artifacts/packages on Release builds). artifacts/ diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b8bd14..a2bf243 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Security & correctness fixes (planned 0.1.1) +- **Lock before staging mutation.** `UpdateInstaller.InstallAsync` now acquires `.update.lock` before any change to `.update//`. Previously a second installer could wipe a first installer's in-flight staging directory on its way to losing the lock race. +- **Rollback on swap failure.** A copy failure mid-swap now restores the install directory from `.old/` instead of leaving it half-populated. New `UpdateInstaller.RestoreFromOld` helper. +- **Asset-name validation.** New `UpdateInstaller.ValidateAssetName` rejects path separators, parent references, rooted paths, and any name whose `Path.GetFileName` doesn't round-trip — closing a path-traversal vector for malicious or misconfigured sources. +- **HTTPS enforcement in `HttpManifestSource`.** Plain-HTTP manifest URLs and asset URLs are now rejected by default. Opt in via `SelfUpdaterOptions.AllowInsecureManifestSource = true` (tests, internal mirrors, local dev). Plain HTTP defeats SHA-256 verification because the SHA itself is MITM-able. +- **TOCTOU-safe install path.** `UpdateCommand` now fetches the release once and installs that exact instance — no second source query between display and install. New `ISelfUpdater.GetLatestReleaseAsync()` and `ISelfUpdater.InstallAsync(RemoteRelease, ...)` overloads. The parameterless `InstallAsync` is kept as a convenience for non-interactive consumers (TOCTOU window documented). + +### Build hygiene +- `` is now Release-only; ordinary `dotnet build` and `dotnet test` no longer produce `.nupkg` files. Output path moved from `C:\nuget-local\` to `$(MSBuildThisFileDirectory)..\..\artifacts\packages` — platform-neutral, repo-local, and gitignored. + ### Added — initial public release (planned 0.1.0) - **Pluggable update sources** — `IUpdateSource` contract with three built-in implementations: - `HttpGitHubReleaseSource` (default) — public GitHub Releases via HttpClient. diff --git a/src/NextIteration.SpectreConsole.SelfUpdate/Commands/UpdateCommand.cs b/src/NextIteration.SpectreConsole.SelfUpdate/Commands/UpdateCommand.cs index ad5ee53..e4f0cfa 100644 --- a/src/NextIteration.SpectreConsole.SelfUpdate/Commands/UpdateCommand.cs +++ b/src/NextIteration.SpectreConsole.SelfUpdate/Commands/UpdateCommand.cs @@ -64,10 +64,13 @@ protected override async Task ExecuteAsync(CommandContext context, Settings var current = _checker.GetCurrentVersion() ?? "dev"; _console.MarkupLineInterpolated(CultureInfo.InvariantCulture, $"Current version: [bold]{current}[/]"); - UpdateInfo? info; + // Fetch the release once and use it for both display and install, + // so the user confirms exactly the release that gets installed + // (no TOCTOU window between "what's latest?" and "install latest"). + RemoteRelease? release; try { - info = await _checker.CheckAsync(cancellationToken).ConfigureAwait(false); + release = await _selfUpdater.GetLatestReleaseAsync(cancellationToken).ConfigureAwait(false); } catch (Exception ex) when (ex is not OperationCanceledException) { @@ -76,21 +79,21 @@ protected override async Task ExecuteAsync(CommandContext context, Settings return 1; } - if (info is null) + if (release is null) { _console.MarkupLine("[red]Could not determine the latest release. The update source returned no result.[/]"); return 1; } - _console.MarkupLineInterpolated(CultureInfo.InvariantCulture, $"Latest release: [bold]{info.LatestTag}[/]"); + _console.MarkupLineInterpolated(CultureInfo.InvariantCulture, $"Latest release: [bold]{release.Tag}[/]"); - if (!settings.Force && !info.IsUpdateAvailable) + if (!settings.Force && !IsUpdateAvailable(current, release.Tag)) { _console.MarkupLine("[green]Already up to date.[/]"); return 0; } - if (!settings.Yes && !PromptToContinue(info)) + if (!settings.Yes && !PromptToContinue(release.Tag)) { _console.MarkupLine("Aborted."); return 2; @@ -102,7 +105,7 @@ await _console.Status().StartAsync("Updating…", async statusContext => { var progress = new Progress(evt => statusContext.Status(StageLabel(evt.Stage))); - await _selfUpdater.InstallAsync(progress, cancellationToken).ConfigureAwait(false); + await _selfUpdater.InstallAsync(release, progress, cancellationToken).ConfigureAwait(false); }).ConfigureAwait(false); } catch (UpdateException ex) @@ -112,17 +115,24 @@ await _console.Status().StartAsync("Updating…", async statusContext => } _console.MarkupLineInterpolated(CultureInfo.InvariantCulture, - $"[green]Installed [bold]{info.LatestTag}[/]. Re-run the CLI to use the new version.[/]"); + $"[green]Installed [bold]{release.Tag}[/]. Re-run the CLI to use the new version.[/]"); return 0; } - private bool PromptToContinue(UpdateInfo info) + private bool PromptToContinue(string tag) { return _console.Confirm( - $"Install [bold]{info.LatestTag}[/] over the current install in {Markup.Escape(_installer.InstallDirectory)}?", + $"Install [bold]{tag}[/] over the current install in {Markup.Escape(_installer.InstallDirectory)}?", defaultValue: true); } + private static bool IsUpdateAvailable(string current, string latestTag) + { + // Defer to the same comparator the checker uses so behaviour is + // identical between the cached probe and this fresh one. + return Pipeline.UpdateChecker.IsNewer(current, latestTag); + } + private static string StageLabel(UpdateStage stage) => stage switch { UpdateStage.Downloading => "Downloading…", diff --git a/src/NextIteration.SpectreConsole.SelfUpdate/ISelfUpdater.cs b/src/NextIteration.SpectreConsole.SelfUpdate/ISelfUpdater.cs index 7787305..4a101cf 100644 --- a/src/NextIteration.SpectreConsole.SelfUpdate/ISelfUpdater.cs +++ b/src/NextIteration.SpectreConsole.SelfUpdate/ISelfUpdater.cs @@ -2,9 +2,10 @@ namespace NextIteration.SpectreConsole.SelfUpdate { /// /// The high-level façade most consumers interact with. Composes the - /// registered and - /// behind a single service. Resolved from - /// DI after calling services.AddSelfUpdater(...). + /// registered , + /// , and + /// behind a single service. Resolved from DI after calling + /// services.AddSelfUpdater(...). /// public interface ISelfUpdater { @@ -14,20 +15,42 @@ public interface ISelfUpdater Task CheckAsync(CancellationToken ct = default); /// - /// Resolve the latest release on the configured channel, download - /// it, run the verifier pipeline, extract the archive, and swap the - /// new files into the install directory. Throws + /// Hit the configured for the current + /// latest release on the configured channel. Returns + /// when nothing is available. Use this when + /// you want to display "what would be installed?" to the user + /// before calling + /// — the same release instance can be passed to install so the + /// displayed and installed versions are guaranteed to match (no + /// TOCTOU window between display and install). + /// + Task GetLatestReleaseAsync(CancellationToken ct = default); + + /// + /// Install the supplied release: download, run the verifier + /// pipeline, extract the archive, and swap the new files into the + /// install directory. Throws on any + /// failure. Prefer this overload when you've already shown the + /// user a specific release — passing it back avoids the second + /// source query the parameterless overload performs. + /// + Task InstallAsync( + RemoteRelease release, + IProgress? progress = null, + CancellationToken ct = default); + + /// + /// Convenience: query the source for the latest release on the + /// configured channel and install it. Throws /// when no release is available or - /// when any pipeline stage fails. + /// when any pipeline stage fails. Has a TOCTOU window: the + /// release returned by the source here may differ from the one a + /// prior reported. For interactive UIs, + /// prefer the + + /// + /// pair so the user confirms exactly the release that gets + /// installed. /// - /// Optional progress sink. - /// Cancellation token. - /// - /// v0.1 always installs the latest release on the configured - /// channel — there is no "install a specific older tag" overload. - /// Tag-pinned installs may arrive in a later version once every - /// source can resolve a release by tag. - /// Task InstallAsync( IProgress? progress = null, CancellationToken ct = default); diff --git a/src/NextIteration.SpectreConsole.SelfUpdate/NextIteration.SpectreConsole.SelfUpdate.csproj b/src/NextIteration.SpectreConsole.SelfUpdate/NextIteration.SpectreConsole.SelfUpdate.csproj index 2d17848..7a7a4cf 100644 --- a/src/NextIteration.SpectreConsole.SelfUpdate/NextIteration.SpectreConsole.SelfUpdate.csproj +++ b/src/NextIteration.SpectreConsole.SelfUpdate/NextIteration.SpectreConsole.SelfUpdate.csproj @@ -14,8 +14,8 @@ 0.1.0 Stuart Meeks Self-update for Spectre.Console CLIs: pluggable update sources (GitHub Releases over HTTP, GitHub Releases via gh CLI for private repos, generic HTTPS manifest, custom), SHA-256 verification, atomic file swap, and a drop-in `update` command. - true - C:\nuget-local\ + true + $(MSBuildThisFileDirectory)..\..\artifacts\packages true MIT README.md diff --git a/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/SelfUpdater.cs b/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/SelfUpdater.cs index c41f527..c7a2d4a 100644 --- a/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/SelfUpdater.cs +++ b/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/SelfUpdater.cs @@ -33,6 +33,15 @@ public SelfUpdater( public Task CheckAsync(CancellationToken ct = default) => _checker.CheckAsync(ct); + public Task GetLatestReleaseAsync(CancellationToken ct = default) => + _source.GetLatestAsync(_options.Channel, ct); + + public Task InstallAsync(RemoteRelease release, IProgress? progress = null, CancellationToken ct = default) + { + ArgumentNullException.ThrowIfNull(release); + return _installer.InstallAsync(release, progress, ct); + } + public async Task InstallAsync(IProgress? progress = null, CancellationToken ct = default) { var release = await _source.GetLatestAsync(_options.Channel, ct).ConfigureAwait(false); diff --git a/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/UpdateInstaller.cs b/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/UpdateInstaller.cs index 51cdef7..0bf2a63 100644 --- a/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/UpdateInstaller.cs +++ b/src/NextIteration.SpectreConsole.SelfUpdate/Pipeline/UpdateInstaller.cs @@ -66,15 +66,25 @@ public async Task InstallAsync(RemoteRelease release, IProgress(); + try + { + foreach (var entry in Directory.EnumerateFileSystemEntries(installDirectory)) + { + var name = Path.GetFileName(entry); + if (IsMaintenanceEntry(name)) continue; + + var dest = Path.Combine(oldDirectory, name); + if (File.Exists(entry)) + { + File.Move(entry, dest); + } + else + { + Directory.Move(entry, dest); + } + movedNames.Add(name); + } + } + catch { - var name = Path.GetFileName(entry); - if (IsMaintenanceEntry(name)) continue; + RestoreFromOld(oldDirectory, installDirectory, movedNames); + throw; + } - var dest = Path.Combine(oldDirectory, name); - if (File.Exists(entry)) + // Phase 2: copy new entries in. Track each placed entry so a + // failure mid-copy can rip out the partial replacement and + // restore the previous install from .old/. + var copiedNames = new List(); + try + { + foreach (var entry in Directory.EnumerateFileSystemEntries(sourceDirectory)) { - File.Move(entry, dest); + var name = Path.GetFileName(entry); + var dest = Path.Combine(installDirectory, name); + if (File.Exists(entry)) + { + File.Copy(entry, dest, overwrite: true); + } + else + { + CopyDirectory(entry, dest); + } + copiedNames.Add(name); } - else + } + catch + { + foreach (var name in copiedNames) { - Directory.Move(entry, dest); + TryDeleteEntry(Path.Combine(installDirectory, name)); } + RestoreFromOld(oldDirectory, installDirectory, movedNames); + throw; } + } - // Copy new files in. - foreach (var entry in Directory.EnumerateFileSystemEntries(sourceDirectory)) + // Move every named entry from .old/ back into install/, replacing + // anything currently at the destination. Best-effort — any single + // entry that can't be restored is logged silently; the original + // pipeline exception still surfaces to the caller, which is the + // useful signal. + internal static void RestoreFromOld(string oldDirectory, string installDirectory, IReadOnlyList names) + { + foreach (var name in names) { - var name = Path.GetFileName(entry); + var src = Path.Combine(oldDirectory, name); var dest = Path.Combine(installDirectory, name); - if (File.Exists(entry)) + try { - File.Copy(entry, dest, overwrite: true); + TryDeleteEntry(dest); + if (File.Exists(src)) + { + File.Move(src, dest); + } + else if (Directory.Exists(src)) + { + Directory.Move(src, dest); + } } - else + catch { - CopyDirectory(entry, dest); + // Best effort — partial-restore failures are surfaced + // implicitly through the pipeline exception. } } } + private static void TryDeleteEntry(string path) + { + try + { + if (File.Exists(path)) File.Delete(path); + else if (Directory.Exists(path)) Directory.Delete(path, recursive: true); + } + catch + { + // Best effort. + } + } + internal static bool IsMaintenanceEntry(string name) => string.Equals(name, StagingDirName, StringComparison.OrdinalIgnoreCase) || string.Equals(name, OldDirName, StringComparison.OrdinalIgnoreCase) @@ -217,6 +300,36 @@ internal static void CopyDirectory(string source, string destination) } } + internal static void ValidateAssetName(string name) + { + if (string.IsNullOrWhiteSpace(name)) + { + throw new UpdateException("Refusing to install: release asset has no name."); + } + if (name.Contains('\0', StringComparison.Ordinal)) + { + throw new UpdateException($"Refusing to install asset '{name}': contains a null character."); + } + if (name.Contains('/', StringComparison.Ordinal) + || name.Contains('\\', StringComparison.Ordinal) + || name.Equals("..", StringComparison.Ordinal) + || name.Equals(".", StringComparison.Ordinal)) + { + throw new UpdateException( + $"Refusing to install asset '{name}': name must be a single file segment with no path separators or parent references."); + } + if (Path.IsPathRooted(name)) + { + throw new UpdateException( + $"Refusing to install asset '{name}': name must not be a rooted path."); + } + if (!string.Equals(Path.GetFileName(name), name, StringComparison.Ordinal)) + { + throw new UpdateException( + $"Refusing to install asset '{name}': resolves to a different file segment."); + } + } + internal static string SanitizeTag(string tag) { // Stage directory names need to be filesystem-safe across all OSs. diff --git a/src/NextIteration.SpectreConsole.SelfUpdate/SelfUpdaterOptions.cs b/src/NextIteration.SpectreConsole.SelfUpdate/SelfUpdaterOptions.cs index 86743ca..efbe224 100644 --- a/src/NextIteration.SpectreConsole.SelfUpdate/SelfUpdaterOptions.cs +++ b/src/NextIteration.SpectreConsole.SelfUpdate/SelfUpdaterOptions.cs @@ -103,6 +103,18 @@ public sealed class SelfUpdaterOptions /// public bool UseDefaultSha256Verifier { get; set; } = true; + /// + /// When (default) the + /// rejects non-https + /// manifest URLs and non-https asset download URLs. Set to + /// to allow plain HTTP — useful for tests, + /// internal mirrors on a trusted network, and local development. + /// Production deployments should always leave this as + /// : the SHA in an HTTP-served manifest is + /// equally MITM-able, so plain HTTP defeats the verifier. + /// + public bool AllowInsecureManifestSource { get; set; } + // ---------- Source registration ---------- /// diff --git a/src/NextIteration.SpectreConsole.SelfUpdate/ServiceCollectionExtensions.cs b/src/NextIteration.SpectreConsole.SelfUpdate/ServiceCollectionExtensions.cs index 7085116..5c39a3f 100644 --- a/src/NextIteration.SpectreConsole.SelfUpdate/ServiceCollectionExtensions.cs +++ b/src/NextIteration.SpectreConsole.SelfUpdate/ServiceCollectionExtensions.cs @@ -103,7 +103,8 @@ private static void RegisterSource(IServiceCollection services, SelfUpdaterOptio case UpdateSourceKind.HttpManifest: services.AddSingleton(sp => new HttpManifestSource( sp.GetRequiredService(), - options.ManifestUrl!)); + options.ManifestUrl!, + allowInsecure: options.AllowInsecureManifestSource)); break; case UpdateSourceKind.CustomType: diff --git a/src/NextIteration.SpectreConsole.SelfUpdate/Sources/HttpManifestSource.cs b/src/NextIteration.SpectreConsole.SelfUpdate/Sources/HttpManifestSource.cs index 51fbc0c..dcbdfad 100644 --- a/src/NextIteration.SpectreConsole.SelfUpdate/Sources/HttpManifestSource.cs +++ b/src/NextIteration.SpectreConsole.SelfUpdate/Sources/HttpManifestSource.cs @@ -59,19 +59,42 @@ public sealed class HttpManifestSource : IUpdateSource private readonly IHttpClientFactory _httpClientFactory; private readonly Uri _manifestUrl; + private readonly bool _allowInsecure; - /// Initializes a new instance. + /// Initializes a new HTTPS-only instance. /// DI-resolved factory used per call. - /// Absolute URL of the JSON manifest. + /// Absolute URL of the JSON manifest. Must be https. public HttpManifestSource(IHttpClientFactory httpClientFactory, Uri manifestUrl) + : this(httpClientFactory, manifestUrl, allowInsecure: false) + { + } + + /// + /// Initializes a new instance, optionally allowing plain HTTP. Plain + /// HTTP defeats SHA-256 verification (the manifest's SHA can be + /// swapped in transit), so the insecure mode is only for tests, + /// internal mirrors on a trusted network, and local development. + /// + public HttpManifestSource(IHttpClientFactory httpClientFactory, Uri manifestUrl, bool allowInsecure) { ArgumentNullException.ThrowIfNull(httpClientFactory); ArgumentNullException.ThrowIfNull(manifestUrl); + if (!allowInsecure && !IsHttps(manifestUrl)) + { + throw new ArgumentException( + $"Manifest URL must use HTTPS. Got '{manifestUrl}'. To allow plain HTTP for tests or trusted networks, set SelfUpdaterOptions.AllowInsecureManifestSource = true.", + nameof(manifestUrl)); + } + _httpClientFactory = httpClientFactory; _manifestUrl = manifestUrl; + _allowInsecure = allowInsecure; } + private static bool IsHttps(Uri uri) => + string.Equals(uri.Scheme, Uri.UriSchemeHttps, StringComparison.OrdinalIgnoreCase); + /// public async Task GetLatestAsync(string? channel, CancellationToken ct) { @@ -117,6 +140,11 @@ public async Task DownloadAssetAsync(ReleaseAsset asset, Stream destination, IPr throw new UpdateException( $"Cannot download asset '{asset.Name}': the manifest did not publish a download URL."); } + if (!_allowInsecure && !IsHttps(asset.DownloadUrl)) + { + throw new UpdateException( + $"Refusing to download asset '{asset.Name}' over insecure URL '{asset.DownloadUrl}'. Set SelfUpdaterOptions.AllowInsecureManifestSource = true to permit plain HTTP for tests or trusted networks."); + } using var http = _httpClientFactory.CreateClient(); using var req = new HttpRequestMessage(HttpMethod.Get, asset.DownloadUrl); diff --git a/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Commands/UpdateCommandTests.cs b/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Commands/UpdateCommandTests.cs index 863c933..e29f109 100644 --- a/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Commands/UpdateCommandTests.cs +++ b/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Commands/UpdateCommandTests.cs @@ -15,16 +15,26 @@ public sealed class UpdateCommandTests { private delegate Task Runner(params string[] args); - private static readonly UpdateInfo UpdateAvailable = - new(CurrentVersion: "1.0.0", LatestTag: "v1.4.2", IsUpdateAvailable: true, ReleaseUrl: new Uri("https://example.com/r/v1.4.2")); - - private static readonly UpdateInfo UpToDate = - new(CurrentVersion: "1.4.2", LatestTag: "v1.4.2", IsUpdateAvailable: false, ReleaseUrl: null); + private static readonly RemoteRelease ReleaseV142 = new( + Tag: "v1.4.2", + Channel: null, + ReleaseNotesUrl: new Uri("https://example.com/r/v1.4.2"), + Assets: Array.Empty(), + PublishedAt: DateTimeOffset.UtcNow); + + private static readonly RemoteRelease ReleaseV100 = new( + Tag: "v1.0.0", + Channel: null, + ReleaseNotesUrl: null, + Assets: Array.Empty(), + PublishedAt: DateTimeOffset.UtcNow); [Fact] public async Task Execute_when_already_up_to_date_returns_zero() { - var (run, console, _) = BuildHarness(checker => checker.CheckImpl = _ => Task.FromResult(UpToDate)); + var (run, console, _) = BuildHarness( + configChecker: c => c.CurrentVersion = "1.4.2", + configUpdater: u => u.GetLatestImpl = _ => Task.FromResult(ReleaseV142)); var exit = await run("--yes"); @@ -33,9 +43,10 @@ public async Task Execute_when_already_up_to_date_returns_zero() } [Fact] - public async Task Execute_when_check_returns_null_returns_one() + public async Task Execute_when_get_latest_returns_null_returns_one() { - var (run, console, _) = BuildHarness(checker => checker.CheckImpl = _ => Task.FromResult(null)); + var (run, console, _) = BuildHarness( + configUpdater: u => u.GetLatestImpl = _ => Task.FromResult(null)); var exit = await run("--yes"); @@ -44,9 +55,10 @@ public async Task Execute_when_check_returns_null_returns_one() } [Fact] - public async Task Execute_when_check_throws_returns_one() + public async Task Execute_when_get_latest_throws_returns_one() { - var (run, console, _) = BuildHarness(checker => checker.CheckImpl = _ => throw new InvalidOperationException("network down")); + var (run, console, _) = BuildHarness( + configUpdater: u => u.GetLatestImpl = _ => throw new InvalidOperationException("network down")); var exit = await run("--yes"); @@ -59,7 +71,8 @@ public async Task Execute_when_check_throws_returns_one() public async Task Execute_when_user_declines_prompt_returns_two() { var (run, console, _) = BuildHarness( - configChecker: checker => checker.CheckImpl = _ => Task.FromResult(UpdateAvailable)); + configChecker: c => c.CurrentVersion = "1.0.0", + configUpdater: u => u.GetLatestImpl = _ => Task.FromResult(ReleaseV142)); console.Profile.Capabilities.Interactive = true; console.Input.PushTextWithEnter("n"); @@ -75,11 +88,15 @@ public async Task Execute_when_install_succeeds_returns_zero_and_calls_install() { var installCalls = 0; var (run, console, _) = BuildHarness( - configChecker: checker => checker.CheckImpl = _ => Task.FromResult(UpdateAvailable), - configUpdater: updater => updater.InstallImpl = (_, _) => + configChecker: c => c.CurrentVersion = "1.0.0", + configUpdater: u => { - installCalls++; - return Task.CompletedTask; + u.GetLatestImpl = _ => Task.FromResult(ReleaseV142); + u.InstallReleaseImpl = (_, _, _) => + { + installCalls++; + return Task.CompletedTask; + }; }); var exit = await run("--yes"); @@ -90,12 +107,65 @@ public async Task Execute_when_install_succeeds_returns_zero_and_calls_install() Assert.Contains("v1.4.2", console.Output, StringComparison.Ordinal); } + [Fact] + public async Task Execute_passes_resolved_release_to_install() + { + // Regression for review #5: the same RemoteRelease instance the + // command displayed must be the one handed to InstallAsync. + RemoteRelease? receivedRelease = null; + var (run, _, _) = BuildHarness( + configChecker: c => c.CurrentVersion = "1.0.0", + configUpdater: u => + { + u.GetLatestImpl = _ => Task.FromResult(ReleaseV142); + u.InstallReleaseImpl = (release, _, _) => + { + receivedRelease = release; + return Task.CompletedTask; + }; + }); + + await run("--yes"); + + Assert.NotNull(receivedRelease); + Assert.Same(ReleaseV142, receivedRelease); + } + + [Fact] + public async Task Execute_does_not_call_parameterless_install() + { + // Regression: the parameterless InstallAsync re-fetches latest + // from the source (TOCTOU). Make sure UpdateCommand never takes + // that path. + var parameterlessCalls = 0; + var (run, _, _) = BuildHarness( + configChecker: c => c.CurrentVersion = "1.0.0", + configUpdater: u => + { + u.GetLatestImpl = _ => Task.FromResult(ReleaseV142); + u.InstallImpl = (_, _) => + { + parameterlessCalls++; + return Task.CompletedTask; + }; + u.InstallReleaseImpl = (_, _, _) => Task.CompletedTask; + }); + + await run("--yes"); + + Assert.Equal(0, parameterlessCalls); + } + [Fact] public async Task Execute_when_install_throws_UpdateException_returns_three() { var (run, console, _) = BuildHarness( - configChecker: checker => checker.CheckImpl = _ => Task.FromResult(UpdateAvailable), - configUpdater: updater => updater.InstallImpl = (_, _) => throw new UpdateException("boom")); + configChecker: c => c.CurrentVersion = "1.0.0", + configUpdater: u => + { + u.GetLatestImpl = _ => Task.FromResult(ReleaseV142); + u.InstallReleaseImpl = (_, _, _) => throw new UpdateException("boom"); + }); var exit = await run("--yes"); @@ -109,11 +179,15 @@ public async Task Execute_when_force_and_up_to_date_proceeds_to_install() { var installCalls = 0; var (run, _, _) = BuildHarness( - configChecker: checker => checker.CheckImpl = _ => Task.FromResult(UpToDate), - configUpdater: updater => updater.InstallImpl = (_, _) => + configChecker: c => c.CurrentVersion = "1.4.2", + configUpdater: u => { - installCalls++; - return Task.CompletedTask; + u.GetLatestImpl = _ => Task.FromResult(ReleaseV142); + u.InstallReleaseImpl = (_, _, _) => + { + installCalls++; + return Task.CompletedTask; + }; }); var exit = await run("--yes", "--force"); @@ -125,11 +199,9 @@ public async Task Execute_when_force_and_up_to_date_proceeds_to_install() [Fact] public async Task Execute_prints_current_and_latest_versions() { - var (run, console, _) = BuildHarness(checker => - { - checker.CurrentVersion = "1.0.0"; - checker.CheckImpl = _ => Task.FromResult(UpdateAvailable); - }); + var (run, console, _) = BuildHarness( + configChecker: c => c.CurrentVersion = "1.0.0", + configUpdater: u => u.GetLatestImpl = _ => Task.FromResult(ReleaseV142)); await run("--yes"); diff --git a/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Infrastructure/Stubs.cs b/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Infrastructure/Stubs.cs index 5b2265a..fee142f 100644 --- a/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Infrastructure/Stubs.cs +++ b/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Infrastructure/Stubs.cs @@ -4,11 +4,19 @@ namespace NextIteration.SpectreConsole.SelfUpdate.Tests.Infrastructure internal sealed class StubSelfUpdater : ISelfUpdater { public Func>? CheckImpl { get; set; } + public Func>? GetLatestImpl { get; set; } public Func?, CancellationToken, Task>? InstallImpl { get; set; } + public Func?, CancellationToken, Task>? InstallReleaseImpl { get; set; } public Task CheckAsync(CancellationToken ct = default) => CheckImpl?.Invoke(ct) ?? Task.FromResult(null); + public Task GetLatestReleaseAsync(CancellationToken ct = default) => + GetLatestImpl?.Invoke(ct) ?? Task.FromResult(null); + + public Task InstallAsync(RemoteRelease release, IProgress? progress = null, CancellationToken ct = default) => + InstallReleaseImpl?.Invoke(release, progress, ct) ?? Task.CompletedTask; + public Task InstallAsync(IProgress? progress = null, CancellationToken ct = default) => InstallImpl?.Invoke(progress, ct) ?? Task.CompletedTask; } diff --git a/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Pipeline/UpdateInstallerTests.cs b/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Pipeline/UpdateInstallerTests.cs index b7ec795..f492c95 100644 --- a/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Pipeline/UpdateInstallerTests.cs +++ b/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Pipeline/UpdateInstallerTests.cs @@ -10,6 +10,9 @@ namespace NextIteration.SpectreConsole.SelfUpdate.Tests.Pipeline { public sealed class UpdateInstallerTests { + private static readonly string[] TwoEntryNames = { "a.txt", "subdir" }; + private static readonly string[] OneEntryName = { "a.txt" }; + [Fact] public async Task InstallAsync_swaps_files_into_install_directory() { @@ -146,6 +149,191 @@ await Assert.ThrowsAsync(() => installer.InstallAsync(release, progress: null, CancellationToken.None)); } + [Fact] + public async Task InstallAsync_when_lock_held_leaves_existing_staging_intact() + { + // Regression: the old order called ResetStaging *before* acquiring + // the lock, so a second installer would wipe a first installer's + // in-flight staging directory on its way to losing the lock race. + using var work = new TempDir(); + var installDir = Path.Combine(work.Path, "install"); + Directory.CreateDirectory(installDir); + + // Pre-populate a staging dir as if another installer were mid-download. + var stagingDir = Path.Combine(installDir, ".update", "v1.0.0"); + Directory.CreateDirectory(stagingDir); + var sentinel = Path.Combine(stagingDir, "in-flight-asset.zip"); + File.WriteAllBytes(sentinel, new byte[] { 1, 2, 3 }); + + // Hold the lock externally to simulate that other installer. + var lockPath = Path.Combine(installDir, ".update.lock"); + using var foreignLock = new FileStream(lockPath, FileMode.CreateNew, FileAccess.Write, FileShare.None); + + var release = BuildReleaseWithSingleAssetZip("v1.0.0", "myapp-v1.0.0-linux-x64.zip", ("a.txt", "x")); + var source = new FakeUpdateSource + { + AssetBytes = _ => CreateZipBytes(("myapp-v1.0.0-linux-x64", new[] { ("a.txt", "x") })), + }; + + var installer = NewInstaller(installDir, source, "linux-x64"); + await Assert.ThrowsAsync(() => + installer.InstallAsync(release, progress: null, CancellationToken.None)); + + Assert.True(File.Exists(sentinel), + "Lock acquisition must precede any staging mutation."); + } + + [Fact] + public async Task InstallAsync_when_resolver_returns_malicious_name_throws() + { + using var work = new TempDir(); + var installDir = Path.Combine(work.Path, "install"); + Directory.CreateDirectory(installDir); + + var malicious = new ReleaseAsset( + Name: "../escape.zip", + DownloadUrl: new Uri("https://example.com/x"), + SizeBytes: 100, + ContentType: null, + Metadata: new Dictionary()); + var release = new RemoteRelease( + Tag: "v1.0.0", + Channel: null, + ReleaseNotesUrl: null, + Assets: new[] { malicious }, + PublishedAt: DateTimeOffset.UtcNow); + + // Use a resolver that always returns the malicious asset, bypassing + // the default resolver's name-shape filter so we can prove the + // installer's own ValidateAssetName runs. + var installer = new UpdateInstaller( + new SelfUpdaterOptions { AppName = "myapp", UseDefaultSha256Verifier = false }, + new FakeUpdateSource(), + new AlwaysReturnAssetResolver(malicious), + Array.Empty(), + ridResolver: () => "linux-x64", + installDirResolver: () => installDir); + + var ex = await Assert.ThrowsAsync(() => + installer.InstallAsync(release, progress: null, CancellationToken.None)); + + Assert.Contains("Refusing to install asset", ex.Message, StringComparison.Ordinal); + } + + private sealed class AlwaysReturnAssetResolver : IAssetResolver + { + private readonly ReleaseAsset _asset; + public AlwaysReturnAssetResolver(ReleaseAsset asset) => _asset = asset; + public ReleaseAsset? Resolve(RemoteRelease release, string runtimeIdentifier) => _asset; + } + + [Theory] + [InlineData("../etc/passwd")] + [InlineData("..\\Windows\\System32\\cmd.exe")] + [InlineData("..")] + [InlineData(".")] + [InlineData("foo/bar.zip")] + [InlineData("foo\\bar.zip")] + [InlineData("")] + [InlineData(" ")] + public void ValidateAssetName_rejects_dangerous_names(string name) + { + Assert.Throws(() => UpdateInstaller.ValidateAssetName(name)); + } + + [Theory] + [InlineData("/etc/passwd")] + [InlineData("\\\\server\\share\\file.zip")] + public void ValidateAssetName_rejects_rooted_paths(string name) + { + // Branched out from the above because IsPathRooted is platform-dependent + // for Windows-style paths — both cases must throw on every platform. + Assert.Throws(() => UpdateInstaller.ValidateAssetName(name)); + } + + [Theory] + [InlineData("myapp-v1.0.0-linux-x64.zip")] + [InlineData("myapp-osx-arm64.tar.gz")] + [InlineData("file.with.many.dots.zip")] + [InlineData("a")] + public void ValidateAssetName_accepts_valid_filenames(string name) + { + UpdateInstaller.ValidateAssetName(name); // does not throw + } + + [Fact] + public void RestoreFromOld_moves_named_entries_back_into_install() + { + using var work = new TempDir(); + var installDir = work.Combine("install"); + var oldDir = work.Combine("old"); + Directory.CreateDirectory(installDir); + Directory.CreateDirectory(oldDir); + + File.WriteAllText(Path.Combine(oldDir, "a.txt"), "alpha"); + Directory.CreateDirectory(Path.Combine(oldDir, "subdir")); + File.WriteAllText(Path.Combine(oldDir, "subdir", "nested.txt"), "nested"); + + UpdateInstaller.RestoreFromOld(oldDir, installDir, TwoEntryNames); + + Assert.Equal("alpha", File.ReadAllText(Path.Combine(installDir, "a.txt"))); + Assert.True(Directory.Exists(Path.Combine(installDir, "subdir"))); + Assert.Equal("nested", File.ReadAllText(Path.Combine(installDir, "subdir", "nested.txt"))); + Assert.False(File.Exists(Path.Combine(oldDir, "a.txt"))); + } + + [Fact] + public void RestoreFromOld_overwrites_existing_destination_entries() + { + using var work = new TempDir(); + var installDir = work.Combine("install"); + var oldDir = work.Combine("old"); + Directory.CreateDirectory(installDir); + Directory.CreateDirectory(oldDir); + + File.WriteAllText(Path.Combine(installDir, "a.txt"), "garbage from a partial copy"); + File.WriteAllText(Path.Combine(oldDir, "a.txt"), "original"); + + UpdateInstaller.RestoreFromOld(oldDir, installDir, OneEntryName); + + Assert.Equal("original", File.ReadAllText(Path.Combine(installDir, "a.txt"))); + } + + [Fact] + public void Swap_when_phase2_copy_fails_restores_install_from_old() + { + // Force a deterministic Phase 2 failure by including a file + // named ".old" in the source. install/.old was just created + // as a directory by Swap's prelude, so the File.Copy of + // source/.old → install/.old fails. The rollback path should + // then remove anything Phase 2 placed and move the original + // contents back from .old/. + using var work = new TempDir(); + var installDir = work.Combine("install"); + Directory.CreateDirectory(installDir); + File.WriteAllText(Path.Combine(installDir, "current.txt"), "original"); + Directory.CreateDirectory(Path.Combine(installDir, "vendor")); + File.WriteAllText(Path.Combine(installDir, "vendor", "lib.txt"), "lib v1"); + + var sourceDir = work.Combine("src"); + Directory.CreateDirectory(sourceDir); + File.WriteAllText(Path.Combine(sourceDir, "newfile.txt"), "new content"); + File.WriteAllText(Path.Combine(sourceDir, ".old"), "should clash with the .old/ dir"); + + var oldDir = Path.Combine(installDir, ".old"); + + Assert.ThrowsAny(() => UpdateInstaller.Swap(sourceDir, installDir, oldDir)); + + // Original install state restored. + Assert.True(File.Exists(Path.Combine(installDir, "current.txt"))); + Assert.Equal("original", File.ReadAllText(Path.Combine(installDir, "current.txt"))); + Assert.True(Directory.Exists(Path.Combine(installDir, "vendor"))); + Assert.Equal("lib v1", File.ReadAllText(Path.Combine(installDir, "vendor", "lib.txt"))); + + // Anything Phase 2 managed to place before the failure is gone. + Assert.False(File.Exists(Path.Combine(installDir, "newfile.txt"))); + } + [Fact] public void Swap_preserves_maintenance_entries() { diff --git a/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Sources/HttpManifestSourceTests.cs b/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Sources/HttpManifestSourceTests.cs index db34edf..eedff5c 100644 --- a/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Sources/HttpManifestSourceTests.cs +++ b/tests/NextIteration.SpectreConsole.SelfUpdate.Tests/Sources/HttpManifestSourceTests.cs @@ -85,5 +85,66 @@ public async Task DownloadAssetAsync_streams_bytes_to_destination() Assert.Equal(payload, ms.ToArray()); } + + [Fact] + public void Constructor_when_manifest_url_is_http_throws_by_default() + { + var insecure = new Uri("http://example.com/latest.json"); + var ex = Assert.Throws(() => + new HttpManifestSource(new FakeHttpClientFactory(new FakeHttpHandler()), insecure)); + Assert.Contains("HTTPS", ex.Message, StringComparison.Ordinal); + Assert.Contains("AllowInsecureManifestSource", ex.Message, StringComparison.Ordinal); + } + + [Fact] + public void Constructor_when_allow_insecure_accepts_http_manifest_url() + { + var insecure = new Uri("http://example.com/latest.json"); + // Does not throw. + _ = new HttpManifestSource(new FakeHttpClientFactory(new FakeHttpHandler()), insecure, allowInsecure: true); + } + + [Fact] + public async Task DownloadAssetAsync_when_asset_url_is_http_throws_by_default() + { + var source = new HttpManifestSource(new FakeHttpClientFactory(new FakeHttpHandler()), ManifestUrl); + + var asset = new ReleaseAsset( + Name: "myapp.tar.gz", + DownloadUrl: new Uri("http://example.com/dl/myapp.tar.gz"), + SizeBytes: null, + ContentType: null, + Metadata: new Dictionary()); + + using var ms = new MemoryStream(); + var ex = await Assert.ThrowsAsync(() => + source.DownloadAssetAsync(asset, ms, progress: null, CancellationToken.None)); + Assert.Contains("Refusing to download", ex.Message, StringComparison.Ordinal); + Assert.Contains("http://", ex.Message, StringComparison.Ordinal); + } + + [Fact] + public async Task DownloadAssetAsync_when_allow_insecure_accepts_http_asset_url() + { + var payload = new byte[] { 0x01, 0x02 }; + var assetUrl = new Uri("http://example.com/dl/asset.tar.gz"); + var handler = new FakeHttpHandler + { + Responder = req => req.RequestUri == assetUrl + ? FakeHttpHandler.Bytes(payload) + : throw new InvalidOperationException("unexpected request URI"), + }; + var source = new HttpManifestSource( + new FakeHttpClientFactory(handler), + new Uri("http://example.com/latest.json"), + allowInsecure: true); + + using var ms = new MemoryStream(); + var asset = new ReleaseAsset("asset.tar.gz", assetUrl, payload.LongLength, null, new Dictionary()); + + await source.DownloadAssetAsync(asset, ms, progress: null, CancellationToken.None); + + Assert.Equal(payload, ms.ToArray()); + } } } From 1b38e4cd4c6b4d36a30721576746795e3dccb94d Mon Sep 17 00:00:00 2001 From: Stuart Meeks Date: Sun, 3 May 2026 04:41:00 +0000 Subject: [PATCH 2/2] bump version to 0.1.1 Matches the CHANGELOG entry for the security & correctness fixes in this branch. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../NextIteration.SpectreConsole.SelfUpdate.csproj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/NextIteration.SpectreConsole.SelfUpdate/NextIteration.SpectreConsole.SelfUpdate.csproj b/src/NextIteration.SpectreConsole.SelfUpdate/NextIteration.SpectreConsole.SelfUpdate.csproj index 7a7a4cf..5f7c229 100644 --- a/src/NextIteration.SpectreConsole.SelfUpdate/NextIteration.SpectreConsole.SelfUpdate.csproj +++ b/src/NextIteration.SpectreConsole.SelfUpdate/NextIteration.SpectreConsole.SelfUpdate.csproj @@ -11,7 +11,7 @@ NextIteration.SpectreConsole.SelfUpdate - 0.1.0 + 0.1.1 Stuart Meeks Self-update for Spectre.Console CLIs: pluggable update sources (GitHub Releases over HTTP, GitHub Releases via gh CLI for private repos, generic HTTPS manifest, custom), SHA-256 verification, atomic file swap, and a drop-in `update` command. true