Skip to content

Commit 39ea9d9

Browse files
trishortsclaude
andcommitted
fix(usefulproteomicsdatabases): close leaf-guard gap and reorder resume skip
Address the four remaining findings from the PR #1093 re-review: - DownloadFileAsync now also rejects a bare "." or ".." FileName. Path.GetFileName returns these unchanged, so they slipped past the leaf-name guard despite the documented "no '..'" contract. - Move the overwrite==false && File.Exists short-circuit ahead of GetHttpsDownloadUrl and Directory.CreateDirectory, so a resume (overwrite: false) skips an already-present file that has no HTTPS location (e.g. Aspera-only) instead of throwing, and does no needless URL resolution or filesystem work. - Fix the path-traversal test to assert against the real escape target derived from the fileName parameter (parent dir for "../evil.raw", a nested subdir for "sub/dir/file.raw") rather than a fixed _tempDir/evil.raw that neither case ever occupies; add "." and ".." cases. - Exercise TryGetHttpsDownloadUrl(out ...) in the explicit-HTTPS-preference test, and add a resume-skip test for an already-present Aspera-only file. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent dec927e commit 39ea9d9

2 files changed

Lines changed: 35 additions & 6 deletions

File tree

mzLib/Test/DatabaseTests/PrideArchiveDownloadTests.cs

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,8 @@ public void TryGetHttpsDownloadUrl_PrefersExplicitHttpsOverFtp()
121121
Ftp("pride/data/x/run1.raw"),
122122
("PRIDE:0000000", "https://example.org/direct/run1.raw"));
123123

124+
Assert.That(file.TryGetHttpsDownloadUrl(out string url), Is.True);
125+
Assert.That(url, Is.EqualTo("https://example.org/direct/run1.raw"));
124126
Assert.That(file.GetHttpsDownloadUrl(), Is.EqualTo("https://example.org/direct/run1.raw"));
125127
}
126128

@@ -227,6 +229,24 @@ public async Task DownloadFileAsync_NotOverwrite_ExistingFile_SkipsRequest()
227229
Assert.That(File.ReadAllText(path), Is.EqualTo("old")); // untouched
228230
}
229231

232+
[Test]
233+
public async Task DownloadFileAsync_NotOverwrite_ExistingFile_NoHttpsLocation_SkipsWithoutThrowing()
234+
{
235+
// resume must skip an already-present file before resolving its URL, so an Aspera-only entry
236+
// (no HTTPS location) that is already on disk returns untouched instead of throwing NotSupportedException.
237+
Directory.CreateDirectory(_tempDir);
238+
string existing = Path.Combine(_tempDir, "run1.raw");
239+
File.WriteAllText(existing, "old");
240+
var handler = new StubHandler(_ => Bytes(Encoding.UTF8.GetBytes("new")));
241+
using var client = new PrideArchiveClient(new HttpClient(handler));
242+
var file = MakeFile("run1.raw", "RAW", Aspera("pride/data/x/run1.raw")); // no HTTPS-reachable location
243+
244+
string path = await client.DownloadFileAsync(file, _tempDir, overwrite: false);
245+
246+
Assert.That(handler.RequestedUris, Is.Empty); // skipped before any URL resolution or network call
247+
Assert.That(File.ReadAllText(path), Is.EqualTo("old")); // left untouched
248+
}
249+
230250
[Test]
231251
public async Task DownloadFileAsync_Overwrite_ReplacesExistingFile()
232252
{
@@ -391,6 +411,8 @@ public void WhereCategory_NullCategory_Skipped()
391411

392412
[TestCase("../evil.raw")]
393413
[TestCase("sub/dir/file.raw")]
414+
[TestCase(".")] // bare "." survives Path.GetFileName unchanged but is a non-leaf the contract rejects
415+
[TestCase("..")] // bare ".." likewise resolves to the parent directory; must be rejected
394416
public void DownloadFileAsync_FileNameNotBareLeaf_Throws_WritesNothingOutside(string fileName)
395417
{
396418
// REGRESSION: failed before the fix — a path-bearing FileName escaped destinationDirectory via Path.Combine.
@@ -400,7 +422,10 @@ public void DownloadFileAsync_FileNameNotBareLeaf_Throws_WritesNothingOutside(st
400422

401423
Assert.That(async () => await client.DownloadFileAsync(file, _tempDir), Throws.ArgumentException);
402424
Assert.That(handler.RequestedUris, Is.Empty); // rejected before any network call
403-
Assert.That(File.Exists(Path.Combine(_tempDir, "evil.raw")), Is.False);
425+
// the location an un-prevented Path.Combine would have written to (parent for "../evil.raw",
426+
// a nested subdir for "sub/dir/file.raw") — derived from the input so each case checks its own escape target
427+
string wouldBeTarget = Path.GetFullPath(Path.Combine(_tempDir, fileName));
428+
Assert.That(File.Exists(wouldBeTarget), Is.False);
404429
}
405430

406431
[Test]

mzLib/UsefulProteomicsDatabases/PrideArchiveClient.cs

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -158,18 +158,22 @@ public async Task<string> DownloadFileAsync(PrideArchiveFile file, string destin
158158
// leaf name is allowed, so a value carrying a directory separator, a ".." segment, or a rooted
159159
// path cannot escape destinationDirectory when combined below (Path.Combine does not sanitize).
160160
string safeFileName = Path.GetFileName(file.FileName);
161-
if (safeFileName != file.FileName)
161+
if (safeFileName != file.FileName || safeFileName == "." || safeFileName == "..")
162162
throw new ArgumentException(
163163
$"The PRIDE file name '{file.FileName}' is not a bare file name; refusing to write outside the destination directory.",
164164
nameof(file));
165165

166-
string url = file.GetHttpsDownloadUrl(); // throws NotSupportedException if unreachable over HTTPS
167-
168-
Directory.CreateDirectory(destinationDirectory);
169166
string destinationPath = Path.Combine(destinationDirectory, safeFileName);
170167

168+
// Cheap resume: an already-present destination is left untouched. This runs before URL
169+
// resolution and directory creation so skipping a downloaded file never fails on a file
170+
// that has no HTTPS location (e.g. Aspera-only) or does needless filesystem work.
171171
if (!overwrite && File.Exists(destinationPath))
172-
return destinationPath; // already downloaded; leave it untouched
172+
return destinationPath;
173+
174+
string url = file.GetHttpsDownloadUrl(); // throws NotSupportedException if unreachable over HTTPS
175+
176+
Directory.CreateDirectory(destinationDirectory);
173177

174178
using HttpResponseMessage response =
175179
await _httpClient.GetAsync(url, HttpCompletionOption.ResponseHeadersRead, cancellationToken).ConfigureAwait(false);

0 commit comments

Comments
 (0)