Public Access
Keep downloads inside the folder they were sent to, never run a file opened for editing, and let a bucket upload be retried
- A remote name went straight into Path.Combine. A POSIX name may hold backslashes, a colon or a drive letter, and an S3 key may hold "../", so a name chosen by whoever could write to the host decided where on this machine a download landed. LocalDirectory.ChildPath cleans the name and requires the resolved parent to be the target folder; every download path and EDIT use it, and a name that cannot be a file there is left on the host and reported. - EDIT handed the copy to the shell's default action, which for .bat, .hta, .js and friends is to run it. LocalEditor opens such a file with its own "edit" verb or Notepad on Windows and `open -t` on macOS, and refuses a .desktop launcher on Linux. - A failed bucket upload leaves its part object, and a retry asked for an offset no bucket accepts, so the row could never finish. IRemoteFileStore.CanWriteFromAnOffset is false for buckets and the queue starts those over. - The SFTP host picker re-found its selection from the empty moment of every vault reload and landed on the first host once a minute; the chosen host is now remembered, and a typed password is only sent to the host it was typed for.
This commit is contained in:
1 parent
4c7f47c026
commit
03c8e8589a
12 files changed
+586
-83
No files matched your search
@@ -93,6 +93,15 @@ internal sealed class FakeSftpSession : ISftpSession
|
||||
/// </remarks>
|
||||
public int? FailReadAfter { get; set; }
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
public bool CanWriteFromAnOffset { get; set; } = true;
|
||||
|
||||
/// <summary>Fails the next write after this many bytes, once, the way a dropped upload does.</summary>
|
||||
public int? FailWriteAfter { get; set; }
|
||||
|
||||
/// <summary>The offsets <see cref="OpenReadAsync"/> was asked to start at, in order.</summary>
|
||||
/// <remarks>
|
||||
/// 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
|
||||
/// <inheritdoc />
|
||||
public Task<Stream> 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<Stream>(new CommittingStream(
|
||||
failAfter,
|
||||
existing.AsSpan(0, (int)Math.Min(offset, existing.Length)).ToArray(),
|
||||
content =>
|
||||
{
|
||||
@@ -300,10 +317,42 @@ internal sealed class FakeSftpSession : ISftpSession
|
||||
}
|
||||
|
||||
/// <summary>A write that lands on the fake host when it is disposed.</summary>
|
||||
private sealed class CommittingStream(byte[] prefix, Action<byte[]> commit) : MemoryStream()
|
||||
private sealed class CommittingStream(int? failAfter, byte[] prefix, Action<byte[]> commit) : MemoryStream()
|
||||
{
|
||||
private bool committed;
|
||||
|
||||
public override void Write(ReadOnlySpan<byte> 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<byte> 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)
|
||||
|
||||
@@ -145,6 +145,37 @@ public sealed class FileTransferQueueTests : IDisposable
|
||||
host.Exists(Remote("artefact.tar") + FileTransferQueue.PartSuffix).ShouldBeFalse();
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A bucket writes an object whole, so a retried upload starts again rather than resuming.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// 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.
|
||||
/// </remarks>
|
||||
[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()
|
||||
{
|
||||
|
||||
@@ -0,0 +1,68 @@
|
||||
namespace DodoSSH.Client.Transfer.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// <c>LocalDirectory.ChildPath</c>: where a name another machine chose may land on this one.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// 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.
|
||||
/// </remarks>
|
||||
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();
|
||||
|
||||
/// <summary>
|
||||
/// Windows drops a trailing dot or space from a path component, so <c>.. </c> is <c>..</c> by the time
|
||||
/// it reaches the disk — which a check of the string alone would have let through.
|
||||
/// </summary>
|
||||
[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");
|
||||
}
|
||||
Reference in new issue
Block a user