diff --git a/src/DodoSSH.Client.App/App.axaml.cs b/src/DodoSSH.Client.App/App.axaml.cs index 015f7ab..b49c046 100644 --- a/src/DodoSSH.Client.App/App.axaml.cs +++ b/src/DodoSSH.Client.App/App.axaml.cs @@ -352,39 +352,9 @@ internal sealed partial class DodoSshApp : Application copyToClipboard: ClipboardWriter(desktop), readClipboard: ClipboardReader(desktop), updates: updates, - openInEditor: OpenInDefaultEditorAsync, + openInEditor: LocalEditor.OpenAsync, watchFile: WatchFile); - /// - /// Hands a local file to whatever this machine opens that kind of file with. - /// - /// - /// - /// One of the two halves of editing a remote file in place, and both are this platform's rather than - /// the shell's: what opens a .conf is a registry association, a desktop entry or a Launch - /// Services binding depending on which desktop this is, and watching a file is a platform object with - /// its own thread. The shell decides when; see TransfersViewModel.EditRemoteAsync. - /// - /// - /// UseShellExecute is the whole of it, and it is what makes the same three lines work on all - /// three desktops: Windows resolves the association, macOS goes through open and Linux through - /// xdg-open, each of which .NET reaches for when the flag is set. Without it the path is treated - /// as an executable to run, which for a .conf fails and for something else would be worse. - /// - /// - /// The process is not waited on and is deliberately not kept: an editor may hand off to an already-open - /// instance and exit at once, so its lifetime says nothing about whether the file is still being - /// edited. What ends an edit session is the user saying so — see the file browser's STOP EDITING. - /// - /// - private static Task OpenInDefaultEditorAsync(string path) - { - using var opened = System.Diagnostics.Process.Start( - new System.Diagnostics.ProcessStartInfo(path) { UseShellExecute = true }); - - return Task.CompletedTask; - } - /// /// Watches one file for saves, until the handle is disposed. /// diff --git a/src/DodoSSH.Client.App/Platform/LocalEditor.cs b/src/DodoSSH.Client.App/Platform/LocalEditor.cs new file mode 100644 index 0000000..b6c7f39 --- /dev/null +++ b/src/DodoSSH.Client.App/Platform/LocalEditor.cs @@ -0,0 +1,126 @@ +using System.Diagnostics; + +namespace DodoSSH.Client.App.Platform; + +/// +/// Opens a local copy of a remote file for editing, without running it. +/// +/// +/// +/// One of the two halves of editing a remote file in place, and both are this platform's rather than the +/// shell's: what opens a .conf is a registry association, a desktop entry or a Launch Services binding +/// depending on which desktop this is. The shell decides when; see +/// TransfersViewModel.EditRemoteAsync. +/// +/// +/// ◆ "Open" is not "edit", and for a file somebody else named the difference is code execution. The +/// file comes from a host, and whoever can write to that host chooses its name. Handing deploy.bat, +/// notes.hta or run.command to the desktop's default action runs it, on this machine, as this +/// user — so a file whose default action is to run is opened with an editor instead. On Windows that is the +/// type's own edit verb where it has one (Notepad, for .bat, .cmd, .js, +/// .vbs, .ps1 and .reg) and Notepad where it has not; on macOS it is open -t, +/// which is the default text editor whatever the type. Everything else still goes to its association, which +/// is the point of the feature — an nginx.conf should open in whatever this person edits configs with. +/// +/// +/// Linux is the exception: there is no portable "the text editor", so a .desktop entry — the one type +/// xdg-open may launch rather than open — is refused with a sentence instead. A shell script is opened +/// by every mainstream desktop's file associations in an editor, not run. +/// +/// +/// The process is not waited on and is deliberately not kept: an editor may hand off to an already-open +/// instance and exit at once, so its lifetime says nothing about whether the file is still being edited. +/// What ends an edit session is the user saying so — see the file browser's STOP EDITING. +/// +/// +internal static class LocalEditor +{ + /// + /// Extensions Windows runs rather than opens, beyond whatever PATHEXT lists on this machine. + /// + /// + /// PATHEXT covers the console's notion of executable (.exe, .bat, .js, + /// .vbs…) and is read as well, because an interpreter's installer may add its own. These are the + /// ones Explorer acts on that are not in it: shortcuts, installers, HTML applications, control panel + /// items, and the script hosts that installers do not register there. + /// + private static readonly string[] WindowsRunsThese = + [ + ".application", ".appref-ms", ".bat", ".chm", ".cmd", ".com", ".cpl", ".diagcab", ".exe", ".hta", + ".inf", ".jar", ".js", ".jse", ".library-ms", ".lnk", ".msc", ".msi", ".msp", ".pif", ".ps1", + ".psm1", ".py", ".pyw", ".reg", ".scf", ".scr", ".search-ms", ".settingcontent-ms", ".url", + ".vbe", ".vbs", ".website", ".ws", ".wsc", ".wsf", ".wsh", ".xll", + ]; + + /// Extensions Launch Services runs, or installs, rather than opens. + private static readonly string[] MacRunsThese = + [ + ".app", ".command", ".dmg", ".fileloc", ".inetloc", ".jar", ".mpkg", ".pkg", ".scpt", ".terminal", + ".tool", ".webloc", ".workflow", + ]; + + /// Opens for editing. + /// + /// This desktop would run the file and has no editor to offer in its place. + /// + internal static Task OpenAsync(string path) + { + ArgumentNullException.ThrowIfNull(path); + + using var opened = Process.Start(StartInfoFor(path)); + + return Task.CompletedTask; + } + + /// What to start so that opens in an editor. + internal static ProcessStartInfo StartInfoFor(string path) + { + if (OperatingSystem.IsWindows() && WouldRunOnWindows(path)) + { + var edit = new ProcessStartInfo(path) { UseShellExecute = true, Verb = "edit" }; + + if (edit.Verbs.Contains("edit", StringComparer.OrdinalIgnoreCase)) + { + return edit; + } + + return new ProcessStartInfo("notepad.exe") { UseShellExecute = false, ArgumentList = { path } }; + } + + if (OperatingSystem.IsMacOS() && HasExtension(path, MacRunsThese)) + { + return new ProcessStartInfo("open") { UseShellExecute = false, ArgumentList = { "-t", path } }; + } + + if (OperatingSystem.IsLinux() && HasExtension(path, [".desktop"])) + { + throw new InvalidOperationException( + $"{Path.GetFileName(path)} is a launcher, which this desktop would run rather than open. " + + "Download it and open it in an editor instead."); + } + + return new ProcessStartInfo(path) { UseShellExecute = true }; + } + + /// Whether Windows' default action for this file is to run it. + internal static bool WouldRunOnWindows(string path, string? pathExt = null) + { + var listed = (pathExt ?? Environment.GetEnvironmentVariable("PATHEXT") ?? string.Empty) + .Split(';', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries); + + return HasExtension(path, WindowsRunsThese) || HasExtension(path, listed); + } + + /// Whether the file's last extension is one of these, ignoring case. + /// + /// Trailing dots and spaces are dropped first, because Windows drops them too: run.bat. is + /// run.bat by the time anything acts on it. + /// + private static bool HasExtension(string path, IEnumerable extensions) + { + var extension = Path.GetExtension(Path.GetFileName(path).TrimEnd('.', ' ')); + + return extension.Length > 0 + && extensions.Contains(extension, StringComparer.OrdinalIgnoreCase); + } +} diff --git a/src/DodoSSH.Client.ObjectStore/S3FileStore.cs b/src/DodoSSH.Client.ObjectStore/S3FileStore.cs index 7f33d97..72faa92 100644 --- a/src/DodoSSH.Client.ObjectStore/S3FileStore.cs +++ b/src/DodoSSH.Client.ObjectStore/S3FileStore.cs @@ -283,6 +283,9 @@ internal sealed class S3FileStore : IRemoteFileStore /// have cost. /// /// + /// + public bool CanWriteFromAnOffset => false; + public Task OpenWriteAsync(string path, long offset, CancellationToken cancellationToken) { ArgumentNullException.ThrowIfNull(path); diff --git a/src/DodoSSH.Client.Shell/ViewModels/TransfersViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/TransfersViewModel.cs index 42cb583..ad7058f 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/TransfersViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/TransfersViewModel.cs @@ -879,6 +879,23 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo [ObservableProperty] private ObjectStoreRowViewModel? selectedBucket; + /// The host and bucket somebody chose, as opposed to whatever the pickers show this instant. + /// + /// re-finds the selection by id, and it runs on every change to the vault's + /// lists — including the Clear() a reload starts with, at which point there is nothing to re-find + /// and the picker's own selection becomes null. Re-finding from that null on the next Add moved the + /// picker to the first host on every sync pass, a minute after somebody chose another one, and CONNECT then + /// dialled the first host with the password typed for theirs. So the choice is remembered here, written + /// only when a person changes the picker, and is what every refresh looks for. + /// + private Guid? chosenHost; + + /// + private Guid? chosenBucket; + + /// Whether a selection change is putting things back, not a choice. + private bool refreshingPickers; + /// /// Whether there is a bucket to open at all. /// @@ -969,6 +986,17 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo [ObservableProperty] private string typedPassword = string.Empty; + /// The host showing when was typed, which is the only one it is for. + /// + /// The picker can move without anybody touching it — a host deleted elsewhere falls back to the first — + /// and the box does not empty when it does, because it cannot tell that from a reload refilling the list. + /// So the password is checked against this at the moment it would be sent instead, and never goes to a + /// host it was not typed for. + /// + private Guid? typedPasswordFor; + + partial void OnTypedPasswordChanged(string value) => typedPasswordFor = SelectedHost?.EntityId; + /// /// It opens saying what the state is rather than what to do about it, and that is a v3 change the /// desktop forced. "Choose a host and connect to browse its files" was this line for two versions, and @@ -1780,8 +1808,10 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo return; } + var password = typedPasswordFor == row.EntityId ? TypedPassword : string.Empty; + if (!open.TryBuildConnectionRequest( - row.Host, TypedPassword, out var request, out var identityLabel, out var refusal)) + row.Host, password, out var request, out var identityLabel, out var refusal)) { Status = refusal; return; @@ -2677,9 +2707,15 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo foreach (var row in rows) { + if (LocalDirectory.ChildPath(LocalPath, row.Name) is not { } local) + { + plan.Unsafe++; + continue; + } + if (row.IsFile) { - plan.Downloads.Add((Path.Combine(LocalPath, row.Name), row.Entry)); + plan.Downloads.Add((local, row.Entry)); continue; } @@ -2695,11 +2731,19 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo await WalkRemoteAsync( row.FullPath, - Path.Combine(LocalPath, row.Name), + local, plan, cancellationToken).ConfigureAwait(true); } + EnqueueDownloads(plan); + + Status = Describe(plan, "download into", LocalPath); + } + + /// Makes the directories a download plan needs here, then hands its files to the queue. + private void EnqueueDownloads(TransferPlan plan) + { foreach (var directory in plan.LocalDirectories.OrderBy(each => each.Length)) { try @@ -2719,8 +2763,6 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo plan.Queued++; } - - Status = Describe(plan, "download into", LocalPath); } /// Adds every file under a remote directory to the plan, with where each will land here. @@ -2774,30 +2816,48 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo return; } - switch (entry.Kind) - { - case SftpEntryKind.File: - plan.Downloads.Add((Path.Combine(toDirectory, entry.Name), entry)); - break; - - case SftpEntryKind.Directory: - pending.Enqueue((entry.FullPath, Path.Combine(toDirectory, entry.Name))); - break; - - case SftpEntryKind.SymbolicLink: - plan.Links++; - break; - - default: - // A socket, a device, a pipe. There is nothing to copy and no local equivalent to - // create, and counting it as a refusal would overstate what went wrong. - plan.Skipped++; - break; - } + PlanRemoteEntry(plan, pending, entry, toDirectory); } } } + /// Adds one entry of a remote listing to a download plan. + private static void PlanRemoteEntry( + TransferPlan plan, + Queue<(string Remote, string Local)> pending, + SftpEntry entry, + string toDirectory) + { + var here = entry.Kind is SftpEntryKind.File or SftpEntryKind.Directory + ? LocalDirectory.ChildPath(toDirectory, entry.Name) + : null; + + switch (entry.Kind) + { + case SftpEntryKind.File or SftpEntryKind.Directory when here is null: + plan.Unsafe++; + break; + + case SftpEntryKind.File: + plan.Downloads.Add((here!, entry)); + break; + + case SftpEntryKind.Directory: + pending.Enqueue((entry.FullPath, here!)); + break; + + case SftpEntryKind.SymbolicLink: + plan.Links++; + break; + + default: + // A socket, a device, a pipe. There is nothing to copy and no local equivalent to create, and + // counting it as a refusal would overstate what went wrong. + plan.Skipped++; + break; + } + } + /// What an upload of this local file should carry to the host with it. /// /// @@ -2893,6 +2953,10 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo /// Sockets, devices and pipes, which have nothing to copy. public int Skipped { get; set; } + /// Remote names that would land outside the directory they were meant for. + /// See . + public int Unsafe { get; set; } + /// Whether the walk stopped at rather than at the end of the tree. public bool Truncated { get; set; } @@ -2937,6 +3001,12 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo said += $" {other} not a file."; } + if (plan.Unsafe > 0) + { + var named = plan.Unsafe == 1 ? "1 name points outside the folder and was" : $"{plan.Unsafe} names point outside the folder and were"; + said += $" {named} left on the host."; + } + if (plan.Truncated) { said += " The walk stopped at 5000 files; queue the rest a folder at a time."; @@ -3162,13 +3232,18 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo $"Fetching {row.Name}…", async () => { - var copy = Path.Combine( + var folder = Path.Combine( Path.GetTempPath(), "dodossh-edits", - Guid.CreateVersion7().ToString("N", CultureInfo.InvariantCulture), - row.Name); + Guid.CreateVersion7().ToString("N", CultureInfo.InvariantCulture)); - Directory.CreateDirectory(Path.GetDirectoryName(copy)!); + if (LocalDirectory.ChildPath(folder, row.Name) is not { } copy) + { + Status = $"{row.Name} is not a name this machine can open as a file."; + return; + } + + Directory.CreateDirectory(folder); await FetchAsync(row.FullPath, copy, cancellationToken).ConfigureAwait(true); @@ -3922,31 +3997,37 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo /// private void RefreshHosts() { - var host = SelectedHost?.EntityId; - var bucket = SelectedBucket?.EntityId; + refreshingPickers = true; - Hosts.Clear(); - - if (vault is not { } open) + try { - return; - } + Hosts.Clear(); - foreach (var row in open.Hosts) + if (vault is not { } open) + { + return; + } + + foreach (var row in open.Hosts) + { + Hosts.Add(row); + } + + SelectedHost = Hosts.FirstOrDefault(row => row.EntityId == chosenHost) ?? Hosts.FirstOrDefault(); + + Buckets.Clear(); + + foreach (var row in open.ObjectStores) + { + Buckets.Add(row); + } + + SelectedBucket = Buckets.FirstOrDefault(row => row.EntityId == chosenBucket) ?? Buckets.FirstOrDefault(); + } + finally { - Hosts.Add(row); + refreshingPickers = false; } - - SelectedHost = Hosts.FirstOrDefault(row => row.EntityId == host) ?? Hosts.FirstOrDefault(); - - Buckets.Clear(); - - foreach (var row in open.ObjectStores) - { - Buckets.Add(row); - } - - SelectedBucket = Buckets.FirstOrDefault(row => row.EntityId == bucket) ?? Buckets.FirstOrDefault(); } /// The session, or a failure a queue row can carry. @@ -4106,8 +4187,30 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo await SettleRemoteListingAsync().ConfigureAwait(true); } - partial void OnSelectedHostChanged(HostRowViewModel? value) => + partial void OnSelectedHostChanged(HostRowViewModel? value) + { + if (!refreshingPickers) + { + chosenHost = value?.EntityId; + + // A person moving to another host gets an empty box rather than one still showing a password + // that, by typedPasswordFor, would no longer be sent. + if (value is not null && typedPasswordFor is not null && value.EntityId != typedPasswordFor) + { + TypedPassword = string.Empty; + } + } + OnPropertyChanged(nameof(SelectedHostAsksForAPassword)); + } + + partial void OnSelectedBucketChanged(ObjectStoreRowViewModel? value) + { + if (!refreshingPickers) + { + chosenBucket = value?.EntityId; + } + } /// /// The password box follows this as well as the host, because it is shown only for a host that asks for diff --git a/src/DodoSSH.Client.Ssh/SftpSession.cs b/src/DodoSSH.Client.Ssh/SftpSession.cs index cc2e56a..1c7c729 100644 --- a/src/DodoSSH.Client.Ssh/SftpSession.cs +++ b/src/DodoSSH.Client.Ssh/SftpSession.cs @@ -523,6 +523,14 @@ public interface IRemoteFileStore : IAsyncDisposable /// Abandons the open. Task OpenWriteAsync(string path, long offset, CancellationToken cancellationToken); + /// Whether can start anywhere but the beginning. + /// + /// True for SFTP, which is a file system. False for a bucket, where an object is written whole: an + /// interrupted upload there starts again rather than resuming, and the transfer queue reads this to know + /// — asking for an offset would be refused on every retry, leaving a row that can never finish. + /// + bool CanWriteFromAnOffset => true; + /// Creates one directory, whose parent must exist. Task CreateDirectoryAsync(string path, CancellationToken cancellationToken); diff --git a/src/DodoSSH.Client.Transfer/FileTransferQueue.cs b/src/DodoSSH.Client.Transfer/FileTransferQueue.cs index d694b6a..d1c2a1b 100644 --- a/src/DodoSSH.Client.Transfer/FileTransferQueue.cs +++ b/src/DodoSSH.Client.Transfer/FileTransferQueue.cs @@ -701,7 +701,10 @@ public sealed class FileTransferQueue : IAsyncDisposable var part = destination + PartSuffix; var existing = await session.StatAsync(part, cancellationToken).ConfigureAwait(false); - var offset = ResumableLength(entry, existing); + + // A bucket writes an object whole, so a part left by a failed attempt is started over rather than + // resumed — and replaced, since writing an object replaces it. + var offset = session.CanWriteFromAnOffset ? ResumableLength(entry, existing) : 0; if (offset == 0 && existing is not null) { diff --git a/src/DodoSSH.Client.Transfer/LocalDirectory.cs b/src/DodoSSH.Client.Transfer/LocalDirectory.cs index 93bfb6e..aae8d6c 100644 --- a/src/DodoSSH.Client.Transfer/LocalDirectory.cs +++ b/src/DodoSSH.Client.Transfer/LocalDirectory.cs @@ -94,6 +94,49 @@ public static class LocalDirectory /// public static string? Parent(string path) => Path.GetDirectoryName(Path.TrimEndingDirectorySeparator(path)); + /// + /// Where a file another machine named should land inside , or null when that + /// name cannot be a file directly inside it. + /// + /// + /// + /// ◆ The name is the remote's, and the remote is not trusted to have chosen one that stays put. + /// A POSIX file name may contain a backslash, a colon or a leading drive letter, and on Windows any of + /// those turns Path.Combine(directory, name) into somewhere else: ..\..\AppData\…\Startup\x.bat + /// climbs out, and C:\… is rooted, so drops the + /// directory altogether. An S3 key is worse, because ../ is an ordinary prefix there on every + /// platform. Whoever can write a name into a shared directory or bucket would otherwise choose where a + /// download writes on this machine. + /// + /// + /// Characters this platform cannot put in a file name are replaced rather than refused, so a Linux file + /// called backup:2026-10-02 still downloads to Windows, as backup_2026-10-02. What is + /// refused is a name that is nothing but a reference to here or above. The final check resolves the path + /// and requires its parent to be the directory, which also catches what Windows does to a trailing dot + /// or space — .. is .. by the time it reaches the disk. + /// + /// + public static string? ChildPath(string directory, string name) + { + ArgumentNullException.ThrowIfNull(directory); + ArgumentNullException.ThrowIfNull(name); + + var cleaned = string.Join('_', name.Split(Path.GetInvalidFileNameChars())); + + if (cleaned.Trim() is "" or "." or "..") + { + return null; + } + + var root = Path.TrimEndingDirectorySeparator(Path.GetFullPath(directory)); + var child = Path.GetFullPath(Path.Combine(root, cleaned)); + + return string.Equals(Path.GetDirectoryName(child), root, StringComparison.Ordinal) + && !string.Equals(child, root, StringComparison.Ordinal) + ? child + : null; + } + /// /// Where the local pane can start from: the drives on Windows, and the root elsewhere. /// diff --git a/tests/DodoSSH.Client.App.Tests/LocalEditorTests.cs b/tests/DodoSSH.Client.App.Tests/LocalEditorTests.cs new file mode 100644 index 0000000..dd78fde --- /dev/null +++ b/tests/DodoSSH.Client.App.Tests/LocalEditorTests.cs @@ -0,0 +1,70 @@ +using DodoSSH.Client.App.Platform; + +namespace DodoSSH.Client.App.Tests; + +/// +/// LocalEditor: a remote file opened for editing is never run. +/// +/// +/// The file's name was chosen by whoever can write to the host, so the question for every name here is +/// whether the desktop's default action would execute it. Only the Windows rule is exercised directly — it +/// is a pure function of the name and PATHEXT — while what each platform starts is checked on the +/// platform the suite is running on. +/// +public sealed class LocalEditorTests +{ + [Theory] + [InlineData("deploy.bat")] + [InlineData("DEPLOY.CMD")] + [InlineData("notes.hta")] + [InlineData("setup.js")] + [InlineData("link.lnk")] + [InlineData("run.ps1")] + [InlineData("settings.reg")] + [InlineData("deploy.bat.")] + [InlineData("deploy.bat ")] + public void AFileWindowsWouldRun_IsRecognised(string name) => + LocalEditor.WouldRunOnWindows(name, pathExt: string.Empty).ShouldBeTrue(); + + [Theory] + [InlineData("nginx.conf")] + [InlineData("authorized_keys")] + [InlineData(".bashrc")] + [InlineData("app.json")] + [InlineData("bat")] + public void AnOrdinaryConfigFile_IsLeftToItsAssociation(string name) => + LocalEditor.WouldRunOnWindows(name, pathExt: string.Empty).ShouldBeFalse(); + + [Fact] + public void WhateverPathExtAdds_IsTreatedAsRunnableToo() => + LocalEditor.WouldRunOnWindows("tool.foo", pathExt: ".COM;.EXE;.FOO").ShouldBeTrue(); + + [Fact] + public void ARunnableFile_IsNeverHandedToTheDefaultOpenAction() + { + var path = Path.Combine(Path.GetTempPath(), "deploy.bat"); + + if (OperatingSystem.IsLinux()) + { + // No portable editor to name, and a .bat is not something a Linux desktop runs. + LocalEditor.StartInfoFor(path).UseShellExecute.ShouldBeTrue(); + Should.Throw(() => LocalEditor.StartInfoFor("/tmp/app.desktop")); + return; + } + + var start = LocalEditor.StartInfoFor(OperatingSystem.IsMacOS() ? "/tmp/run.command" : path); + + // Either the type's own "edit" verb, or a named editor with the file as an argument — never the + // file itself under its default verb. + (start.Verb == "edit" || !start.UseShellExecute).ShouldBeTrue(); + } + + [Fact] + public void AnOrdinaryFile_StillOpensWithItsAssociation() + { + var start = LocalEditor.StartInfoFor(Path.Combine(Path.GetTempPath(), "nginx.conf")); + + start.UseShellExecute.ShouldBeTrue(); + start.Verb.ShouldBeEmpty(); + } +} diff --git a/tests/DodoSSH.Client.App.Tests/TransferQueueingTests.cs b/tests/DodoSSH.Client.App.Tests/TransferQueueingTests.cs index 3e79f53..3417f9b 100644 --- a/tests/DodoSSH.Client.App.Tests/TransferQueueingTests.cs +++ b/tests/DodoSSH.Client.App.Tests/TransferQueueingTests.cs @@ -131,6 +131,35 @@ public sealed class TransferQueueingTests : IDisposable /// SFTP fake that a real connect produces. /// /// + /// + /// A name the host chose cannot choose where on this machine the download lands. + /// + /// + /// Backslashes and colons are ordinary in a POSIX file name and are separators and drive letters on + /// Windows, and a slash arrives in a name from an S3 key — so every one of these would have escaped the + /// folder through Path.Combine somewhere. They are queued inside it under a cleaned name, and the + /// bare .., which no cleaning can make a file name, is left on the host and said so. + /// + [Fact] + public void ADownloadNamedToEscapeTheFolder_LandsInsideItOrNotAtAll() + { + Connected(); + + transfers.QueueDownloads( + [ + RemoteFile(@"..\..\AppData\Roaming\Microsoft\Windows\Start Menu\Programs\Startup\x.bat"), + RemoteFile(@"C:\Windows\evil.dll"), + RemoteFile("../../escaped.txt"), + RemoteFile(".."), + ]); + + var landed = Queued().Select(row => row.Transfer.LocalPath).ToList(); + + landed.Count.ShouldBe(3); + landed.ShouldAllBe(path => Path.GetDirectoryName(path) == directory); + transfers.Status.ShouldContain("1 name points outside the folder"); + } + [Fact] public void TheDownloadButton_GoesThroughTheSamePathAsADrop() { diff --git a/tests/DodoSSH.Client.Transfer.Tests/FakeSftpSession.cs b/tests/DodoSSH.Client.Transfer.Tests/FakeSftpSession.cs index 8134da3..8f3689e 100644 --- a/tests/DodoSSH.Client.Transfer.Tests/FakeSftpSession.cs +++ b/tests/DodoSSH.Client.Transfer.Tests/FakeSftpSession.cs @@ -93,6 +93,15 @@ internal sealed class FakeSftpSession : ISftpSession /// public int? FailReadAfter { get; set; } + /// + /// Whether a write may start anywhere but zero. False makes this behave like a bucket, which refuses an + /// offset — and, like a bucket, still keeps whatever an abandoned write had sent when it is closed. + /// + public bool CanWriteFromAnOffset { get; set; } = true; + + /// Fails the next write after this many bytes, once, the way a dropped upload does. + public int? FailWriteAfter { get; set; } + /// The offsets was asked to start at, in order. /// /// The only direct evidence that a resume resumed. A test can see the right bytes on disk at the end @@ -195,11 +204,19 @@ internal sealed class FakeSftpSession : ISftpSession /// public Task OpenWriteAsync(string path, long offset, CancellationToken cancellationToken) { + if (!CanWriteFromAnOffset && offset != 0) + { + throw new SftpPathException(path, "An object cannot be written to from the middle."); + } + lock (gate) { var existing = files.GetValueOrDefault(path, []); + var failAfter = FailWriteAfter; + FailWriteAfter = null; return Task.FromResult(new CommittingStream( + failAfter, existing.AsSpan(0, (int)Math.Min(offset, existing.Length)).ToArray(), content => { @@ -300,10 +317,42 @@ internal sealed class FakeSftpSession : ISftpSession } /// A write that lands on the fake host when it is disposed. - private sealed class CommittingStream(byte[] prefix, Action commit) : MemoryStream() + private sealed class CommittingStream(int? failAfter, byte[] prefix, Action commit) : MemoryStream() { private bool committed; + public override void Write(ReadOnlySpan buffer) + { + FailIfDue(buffer.Length); + base.Write(buffer); + } + + public override void Write(byte[] buffer, int offset, int count) + { + FailIfDue(count); + base.Write(buffer, offset, count); + } + + public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationToken cancellationToken = default) + { + FailIfDue(buffer.Length); + return base.WriteAsync(buffer, cancellationToken); + } + + public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) + { + FailIfDue(count); + return base.WriteAsync(buffer, offset, count, cancellationToken); + } + + private void FailIfDue(int count) + { + if (failAfter is { } limit && Length + count > limit) + { + throw new IOException("The connection dropped mid-upload."); + } + } + public override void Close() { if (!committed) diff --git a/tests/DodoSSH.Client.Transfer.Tests/FileTransferQueueTests.cs b/tests/DodoSSH.Client.Transfer.Tests/FileTransferQueueTests.cs index 855590c..d6119fd 100644 --- a/tests/DodoSSH.Client.Transfer.Tests/FileTransferQueueTests.cs +++ b/tests/DodoSSH.Client.Transfer.Tests/FileTransferQueueTests.cs @@ -145,6 +145,37 @@ public sealed class FileTransferQueueTests : IDisposable host.Exists(Remote("artefact.tar") + FileTransferQueue.PartSuffix).ShouldBeFalse(); } + /// + /// A bucket writes an object whole, so a retried upload starts again rather than resuming. + /// + /// + /// The failed attempt leaves a part object behind — a bucket keeps what was sent when the stream closes — + /// and the retry used to ask for the offset that part had reached. A bucket refuses any offset, so every + /// retry failed until the row was discarded, under a message saying it would start again. + /// + [Fact] + public async Task ARetriedUploadToAStoreThatCannotResume_StartsAgainAndLands() + { + var content = Bytes(300_000); + await File.WriteAllBytesAsync(Local("artefact.tar"), content, Token); + + host.CanWriteFromAnOffset = false; + host.FailWriteAfter = 100_000; + + await using var queue = NewQueue(); + + var broken = await RunToCompletionAsync( + queue, TransferDirection.Upload, Local("artefact.tar"), Remote("artefact.tar"), content.Length); + + broken.State.ShouldBe(TransferState.Failed); + host.Exists(Remote("artefact.tar") + FileTransferQueue.PartSuffix).ShouldBeTrue("what a bucket keeps"); + + var retried = await AwaitFinishAsync(queue, () => queue.Retry(broken.Id)); + + retried.State.ShouldBe(TransferState.Completed, retried.Failure); + host.Read(Remote("artefact.tar")).ShouldBe(content); + } + [Fact] public async Task AnUploadOntoAFileThatIsAlreadyThere_IsRefusedAndLeavesItAlone() { diff --git a/tests/DodoSSH.Client.Transfer.Tests/LocalDirectoryTests.cs b/tests/DodoSSH.Client.Transfer.Tests/LocalDirectoryTests.cs new file mode 100644 index 0000000..d4e6660 --- /dev/null +++ b/tests/DodoSSH.Client.Transfer.Tests/LocalDirectoryTests.cs @@ -0,0 +1,68 @@ +namespace DodoSSH.Client.Transfer.Tests; + +/// +/// LocalDirectory.ChildPath: where a name another machine chose may land on this one. +/// +/// +/// The assertion that matters is the same in every row — the result's parent is the directory asked about, +/// or there is no result — because that is the property a download relies on, and it is checked against the +/// resolved path rather than the string so that what the file system does to a name counts too. +/// +public sealed class LocalDirectoryTests +{ + private static readonly string Directory = Path.Combine(Path.GetTempPath(), "dodossh-child-path"); + + [Theory] + [InlineData("notes.txt")] + [InlineData("backup:2026-10-02")] + [InlineData(@"..\..\AppData\Roaming\Microsoft\Windows\Start Menu\Programs\Startup\x.bat")] + [InlineData(@"C:\Windows\evil.dll")] + [InlineData("../../escaped.txt")] + [InlineData("/etc/passwd")] + [InlineData("...")] + [InlineData(".hidden")] + public void ANameThatCanBeAFileHere_LandsDirectlyInsideTheDirectory(string name) + { + var child = LocalDirectory.ChildPath(Directory, name); + + if (child is null) + { + // Only acceptable for "...", which Windows reduces to nothing; everywhere else it is a name. + OperatingSystem.IsWindows().ShouldBeTrue($"'{name}' was refused"); + name.ShouldBe("..."); + return; + } + + Path.GetDirectoryName(child).ShouldBe(Path.GetFullPath(Directory)); + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + [InlineData(".")] + [InlineData("..")] + public void ANameThatIsOnlyHereOrAbove_IsRefused(string name) => + LocalDirectory.ChildPath(Directory, name).ShouldBeNull(); + + /// + /// Windows drops a trailing dot or space from a path component, so .. is .. by the time + /// it reaches the disk — which a check of the string alone would have let through. + /// + [Fact] + public void ATrailingSpaceOrDot_DoesNotSmuggleAParentReferencePastTheCheck() + { + foreach (var name in new[] { ".. ", "...", ".. .", ". " }) + { + var child = LocalDirectory.ChildPath(Directory, name); + + if (child is not null) + { + Path.GetDirectoryName(child).ShouldBe(Path.GetFullPath(Directory), name); + } + } + } + + [Fact] + public void AnOrdinaryName_IsKeptAsItIs() => + Path.GetFileName(LocalDirectory.ChildPath(Directory, "nginx.conf")).ShouldBe("nginx.conf"); +}