-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Normalizing slashes in entrynames #119248
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Fixes path normalization in ZIP archive entry creation to ensure forward slashes are used as required by the ZIP specification. This resolves interoperability issues with Java and PHP tools that expect standard ZIP path separators.
- Normalizes backslashes to forward slashes in entry names
- Trims leading slashes from entry names to prevent invalid paths
- Adds comprehensive test coverage for both direct entry creation and file-based entry creation
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs | Adds path normalization logic to DoCreateEntry method |
| src/libraries/System.IO.Compression.ZipFile/src/System/IO/Compression/ZipFileExtensions.ZipArchive.Create.cs | Adds path normalization logic to InitializeDoCreateEntryFromFile method |
| src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs | Adds test methods to verify path normalization behavior |
src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/tests/ZipArchive/zip_CreateTests.cs
Outdated
Show resolved
Hide resolved
|
Hey @dotnet/area-system-io-compression @rzikm Now that I think again, maybe a better way to deal with this would be adding an extra parameter to coose to normalize entrynames, like specified in this comment here: #98247 (comment), so it wouldn't result in any breaking change, what do you think? |
|
I am personally in favor of taking the breaking change. I am also okay with adding an option to override the normalization, but I would consider the normalization enabled to be the better default, as you cannot create a filename containing backslashes on Windows, and on Linux such names - while supported - are considered gross malpractice (as are names containing whitespace and control characters). |
|
#113058 this also seems to be related |
|
I also came across this issue: #1553, which seems related |
|
|
||
| FileStream fs = new FileStream(sourceFileName, FileMode.Open, FileAccess.Read, FileShare.Read, ZipFile.FileStreamBufferSize, useAsync); | ||
|
|
||
| entryName = entryName.TrimStart('/', '\\').Replace('\\', '/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we know we're dealing with a path, so manipulating the string to normalize separators is consistent with other API at this layer. Consider sharing the logic that already exists for this normalization in https://github.com/dotnet/runtime/blob/0d1523350e6df0f18405a1d1c8bfcf8c0699d750/src/libraries/Common/src/System/IO/Archiving.Utils.Windows.cs#L53C37-L53C50
There is still a concern here that the user might actually have a file name that contains a backslash on linux. I wonder how you can deal with that?
(edit) Actually, upon closer inspection, the entryName is passed as a separate parameter from the file path in the host system, so while it may contain separators those came directly from the user and not from us and we don't know if they meant those as path separators (most-likely) or not.
|
|
||
| ThrowIfDisposed(); | ||
|
|
||
| entryName = entryName.TrimStart('/', '\\').Replace('\\', '/'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can "know" we're dealing with a path and that the user wants this replacement done? What's going to happen for archives that might already use \ elsewhere? What if the user actually needs to preserve the slash for whatever reason?
It looks like this method is called for all calls to CreateEntry so any other logic that tries to do this slash replacement is probably redundant to this. We shouldn't be duplicating this replacement at multiple layers (if we do decide to keep it at this layer).
|
Closing the PR as we need to reconsider more things for these changes |
Fix ZipFileExtensions.CreateEntryFromFile writing backslashes () into ZipArchive entries.
Per the ZIP spec, file paths stored in the central directory MUST use forward slashes (/). Using backslashes causes interoperability issues with some tools (Java, PHP).
This change normalizes entry names to use / only when creating new entries.
Fixes #41914