Skip to content

fix: sanitize ZIP entry names to prevent path traversal #42

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
dobby-coder wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix: sanitize ZIP entry names to prevent path traversal #42

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/Zip/ZipHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,17 @@ public static byte[] CreateZip(IReadOnlyList<PgFile> files)
{
foreach (var file in files)
{
var entry = archive.CreateEntry(file.Name, CompressionLevel.Optimal);
// Strip any directory components so traversal sequences (e.g. "../../")
// cannot be embedded as ZIP entry names in produced archives.
var entryName = Path.GetFileName(file.Name);

@dobby-coder dobby-coder Bot Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: Path.GetFileName is platform-dependent — on non-Windows runtimes it only treats / as a separator, so a Windows-style traversal name like ..\..\etc\passwd passes through UNCHANGED here. Verified on this Linux runtime: Path.GetFileName("..\\..\\etc\\passwd") returns the string unchanged. Because these ZIPs are consumed cross-platform (extracted on Windows, where \ IS a separator), the fix does not prevent embedded traversal for backslash sequences when the archive is created on Linux/macOS. Strip BOTH / and \ explicitly (split on both separator chars / take the last segment) instead of relying on Path.GetFileName, and add a ..\..\-style test case.

@dobby-coder dobby-coder Bot Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug] Path.GetFileName is platform-dependent: on non-Windows runtimes it only treats / as a separator, so Windows-style traversal names like ..\..\etc\passwd pass through UNCHANGED. Verified on this Linux runtime: Path.GetFileName("..\\..\\etc\\passwd") returns the input unchanged. Since produced ZIPs are consumed cross-platform (extracted on Windows, where \ IS a separator), the fix does not actually prevent embedded traversal for backslash sequences when the archive is created on Linux/macOS. Strip BOTH / and \ explicitly (split on both separator chars / take the last segment after any of them) rather than relying on Path.GetFileName.

if (string.IsNullOrEmpty(entryName))
{
throw new ArgumentException(
$"File name '{file.Name}' does not resolve to a valid entry name.",
nameof(files));
}

var entry = archive.CreateEntry(entryName, CompressionLevel.Optimal);

@dobby-coder dobby-coder Bot Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: flattening directory components can silently collapse distinct inputs (a/x.txt and b/x.txt, or x.txt and dir/x.txt) to the same entry name. ZipArchive.CreateEntry permits duplicate names without throwing, so on extraction one entry overwrites the other → silent data loss. Acceptable given the security intent, but worth a collision guard or de-dup.

@dobby-coder dobby-coder Bot Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Flattening directory components can silently collapse distinct inputs (e.g. a/x.txt and b/x.txt, or x.txt and dir/x.txt) to the same entry name. ZipArchive.CreateEntry permits duplicate names without throwing, so on extraction one entry overwrites the other → silent data loss. Acceptable given the security intent, but worth a collision guard or de-dup.

using var entryStream = entry.Open();
file.Content.CopyTo(entryStream);
}
Expand Down
27 changes: 26 additions & 1 deletion tests/E4A.PostGuard.Tests/ZipHelperTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,35 @@ public void MultipleFiles_AllPresentWithContent()
var entries = ReadZip(zip);
Assert.Equal(3, entries.Count);
Assert.Equal("alpha", entries["a.txt"]);
Assert.Equal("beta", entries["dir/b.txt"]);
// Directory components are stripped, so "dir/b.txt" is stored as "b.txt".
Assert.Equal("beta", entries["b.txt"]);
Assert.Equal("gamma", entries["c.bin"]);
}

[Theory]
[InlineData("../../etc/passwd", "passwd")]
[InlineData("dir/nested/file.txt", "file.txt")]
[InlineData("/absolute/path.txt", "path.txt")]
public void SanitizesEntryNames_StrippingDirectoryComponents(string name, string expected)
{
var zip = ZipHelper.CreateZip([File(name, "payload")]);

var entries = ReadZip(zip);
var entryName = Assert.Single(entries.Keys);
Assert.Equal(expected, entryName);
Assert.DoesNotContain("..", entryName);
Assert.Equal("payload", entries[entryName]);
}

[Theory]
[InlineData("")]
[InlineData("dir/")]
[InlineData("../")]
public void ThrowsArgumentException_WhenNameHasNoFileComponent(string name)
{
Assert.Throws<ArgumentException>(() => ZipHelper.CreateZip([File(name, "payload")]));
}

[Fact]
public void EmptyFileList_ProducesValidEmptyZip()
{
Expand Down
Loading