Added test for extracting zip files with invalid characters in Windows #67332

Danyy427 · 2022-03-30T11:26:57Z

Relates to #67201

ghost · 2022-03-30T11:27:04Z

Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

Issue Details

Relates to #67201

Author:	Danyy427
Assignees:	-
Labels:	`area-System.IO.Compression`, `community-contribution`
Milestone:	-

danmoseley · 2022-03-30T16:30:19Z

Thanks. We will need the fix as well before we can merge this 😸

src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Extract.cs

…eNameChars.zip

Danyy427 · 2022-03-30T16:48:44Z

@danmoseley I removed all of the [InlineData] and changed the test to a [Fact] and tested for all three of the files by hardcoding them. I hopefully didn't make a mistake because I wrote the code directly on GitHub. Once the tests are complete and fail, I will start working on the sanitization.

danmoseley · 2022-03-30T17:28:11Z

src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Extract.cs

+ {
+ var testDirectory = GetTestFilePath();
+ ZipFile.ExtractToDirectory(compat("InvalidWindowsFileNameChars.zip"), testDirectory);
+ Assert.True(File.Exists(testDirectory + "Test______________________________________.txt"));


Please use Path.Combine not + - see other tests.

danmoseley · 2022-03-30T17:28:41Z

Test looks good with comments above. Please also push up the fix now, that makes this test pass locally for you.

danmoseley · 2022-03-30T18:23:07Z

BTW, it's fine doing things this way for your first PR 😄 but generally we would push up a PR when it's all done -- meaning tests+fix. We verify that the tests fail without the fix, but we do it locally beforehand.

src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Extract.cs

…act.cs

Danyy427 · 2022-03-30T18:31:33Z

@danmoseley Oh, I thought when you said to write the tests first and verify it fails, I thought I was supposed to verify it fails on the GitHub repo rather than my local computer. That was the reason I asked if I should do it one go or separately.

I was going to start writing the fix but I cannot build the repo now due to some technical issue apparently. Once that is fixed and I can build the repo I will start the fix and probably push it by tomorrow.

BTW, thank you for helping, I am quite unfamiliar with all of this. I must have been a headache with all the questions :)

danmoseley · 2022-03-30T18:38:07Z

thought I was supposed to verify it fails on the GitHub repo rather than my local computer

Yes, I was unclear 😄

What is the technical issue? Your link is to the install script.

Danyy427 · 2022-03-30T18:43:56Z

@danmoseley It doesn't build

GET https://dotnet.microsoft.com/download/dotnet/scripts/v1/dotnet-install.ps1
The remote server returned an error: (421) Misdirected Request.
Retrying. Waiting for 3 seconds before next attempt (2 of 5).
GET https://dotnet.microsoft.com/download/dotnet/scripts/v1/dotnet-install.ps1
The remote server returned an error: (421) Misdirected Request.
Retrying. Waiting for 7 seconds before next attempt (3 of 5).
GET https://dotnet.microsoft.com/download/dotnet/scripts/v1/dotnet-install.ps1
The remote server returned an error: (421) Misdirected Request.
Retrying. Waiting for 15 seconds before next attempt (4 of 5).
GET https://dotnet.microsoft.com/download/dotnet/scripts/v1/dotnet-install.ps1
The remote server returned an error: (421) Misdirected Request.
Retrying. Waiting for 31 seconds before next attempt (5 of 5).
GET https://dotnet.microsoft.com/download/dotnet/scripts/v1/dotnet-install.ps1
The remote server returned an error: (421) Misdirected Request.
Unable to download file in 5 attempts.

The link says the following

<h2>Our services aren't available right now</h2><p>We're working to restore all services as soon as possible. Please check back soon.</p>0wKJEYgAAAACW4qqrJ8M1QK2oV6FN05MfQU1TMDRFREdFMTkxMQBlMWRmMDcwYS1hZTQ0LTRjMGItYTU0Yi1jNDkzODA0ZTRkOWY=

Update: Install script is back

…untime into Danyy427-ZipFile-Test

The tests added pass with this build

…untime into Danyy427-ZipFile-Test

Danyy427 · 2022-04-10T11:26:33Z

@danmoseley I would be more than happy to continue contributing after this one.

danmoseley · 2022-04-10T20:46:52Z

@ViktorHofer thanks. Why would we introduce a PNSE TFM when browser+windows+unix covers everything?

Simplest thing then seems to leave the Windows build as the platform agnostic one. I think that means

revert the TargetFrameworks line
change <ItemGroup Condition="'$(TargetPlatformIdentifier)' == 'windows' or '$(TargetPlatformIdentifier)' == ''"> to <ItemGroup Condition="'$(TargetPlatformIdentifier)' == ''">

Danyy427 · 2022-04-10T20:56:20Z

@danmoseley Asking just out of curiosity, why do we assume windows to be the default when the windows filename rules are the exception? Wouldn't sanitizing only the null character be better when targeting a new platform perhaps?

danmoseley · 2022-04-10T21:08:31Z

why do we assume windows to be the default when the windows filename rules are the exception?

As far as I'm aware, the only target that doesn't match browser and Unix is Windows, so it doesn't matter. If we add a new target, we would need to change other things too. I am not sure what the implications are of changing the default one, although @ViktorHofer would know.

Danyy427 · 2022-04-10T21:20:21Z

@danmoseley I don't know if it would change anything code-wise but I was thinking as a design.

ViktorHofer · 2022-04-11T13:32:35Z

The question is, how would you default if you would handle this during runtime?

Option A

if (IsWindows)
{
// Windows specific code
}
else if (IsUnix)
{
// Unix specific code
}
else if (IsBrowser)
{
// Browser specific code
}
else
{
    throw new PlatformNotSupportedException(...);
}

In our world where we don't use runtime conditions but build for specific platforms we would implement Option A the following way:

<PropertyGroup>
<TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-Unix;$(NetCoreAppCurrent)-Browser;$(NetCoreAppCurrent)</NetCoreAppCurrent>
</PropertyGroup>
<!-- DesignTimeBuild requires all the TargetFramework Derived Properties to not be present in the first property group. -->
<PropertyGroup>
<TargetPlatformIdentifier>$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)'))</TargetPlatformIdentifier>
<GeneratePlatformNotSupportedAssemblyMessage Condition="'$(TargetPlatformIdentifier)' == ''">SR.PNSEMEssage</GeneratePlatformNotSupportedAssemblyMessage>
</PropertyGroup>

Option B

if (IsWindows)
{
// Windows specific code
}
else if (IsBrowser)
{
// Browser specific code
}
else (IsUnix)
{
// Use Unix as the default implementation. The assumption stands that new platforms can leverage that implementation..
}

That would be implemented this way:

<PropertyGroup>
<TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-Browser;$(NetCoreAppCurrent)-Unix</NetCoreAppCurrent>
</PropertyGroup>

Choosing Option A requires an additional inner build (for the PNSE configuration) which is a significant cost compared to runtime checks. We should aim at not over-building as every additional project evaluation, msbuild target execution and compiler invocation adds to the overall build. That said, if a PNSE makes most sense for the library, it makes sense to choose it.

As far as I'm aware, the only target that doesn't match browser and Unix is Windows, so it doesn't matter. If we add a new target, we would need to change other things too. I am not sure what the implications are of changing the default one, although @ViktorHofer would know.

You can always move around the default as long as you make sure that the right platform gets the right implementation (via conditions on the item groups).

Why would we introduce a PNSE TFM when browser+windows+unix covers everything?

It might cover everything today but whenever we add a new platform we want our libraries to have a good default setting. In the past we chose Unix as the default as the most platforms builds on top of it. Choosing Windows as a default doesn't make much sense IMO as there is only a single platform (Windows) that would work with the implementation (assuming that the current Windows implementation is actually platform specific and not agnostic).

danmoseley · 2022-04-11T13:43:12Z

Ok it sounds like we should default to the Unix behavior as you suggested @Danyy427 . Do you want to make that change?

Danyy427 · 2022-04-11T13:59:25Z

@danmoseley I tried, hopefully I understood @ViktorHofer's answer correctly.

src/libraries/System.IO.Compression.ZipFile/src/System.IO.Compression.ZipFile.csproj

src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Create.cs

ViktorHofer

From an infrastructure perspective this looks great. Thanks

…te.cs Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>

Danyy427 · 2022-04-11T20:58:43Z

@danmoseley Are we supposed to do anything else to get this thing to merge?

danmoseley · 2022-04-11T21:02:46Z

@Danyy427 nope, just letting tests run.

@dotnet/dnceng something happened here with Azdo https://dev.azure.com/dnceng/9ee6d478-d288-47f7-aacc-f6e6d082ae6d/_apis/build/builds/1711221/logs/662

ulisesh · 2022-04-11T21:13:30Z

Thanks for reporting the problem @danmoseley. I haven't seen this error before but I'll create an issue to monitor this error.

I see that you already retried the build, hopefully it succeeds this time

danmoseley · 2022-04-11T22:36:39Z

Thanks @Danyy427. You ran into one or two more problems (on our side) than most PR's encounter!

Danyy427 · 2022-04-11T22:41:37Z

@danmoseley It’s okay I guess. Thanks for all the help you have given, it has been most useful. I will most certainly comtinue contributing. And this will certainly not be the last time I will give you a headache from all the questions. :)

danmoseley · 2022-04-11T22:47:25Z

Always happy to help. As I mentioned earlier, from experience I highly recommend new folks get a number of easy, short PR's merged before tackling anything larger. Partly just because it takes time to get used to a repo's style, etc.

Danyy427 · 2022-04-11T22:49:49Z

@danmoseley Yeah, probably that’s foe the best. I browsed a couple of those easy and up-for-grabs. There are some which are like a year old and almost forgotten. I may continue with those perhaps as early as tomorrow

Added test for extracting zip files with invalid characters in Windows

56a49cb

ghost added the community-contribution Indicates that the PR has been added by a community member label Mar 30, 2022

dotnet-issue-labeler bot added the area-System.IO.Compression label Mar 30, 2022

Danyy427 mentioned this pull request Mar 30, 2022

ZipFile.ExtractToDirectory failing with "The filename, directory name, or volume label syntax is incorrect." (works in SharpZipLib) #67201

Closed

Removed faulty test case

682dbe3

danmoseley marked this pull request as draft March 30, 2022 16:30

danmoseley reviewed Mar 30, 2022

View reviewed changes

src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Extract.cs Outdated Show resolved Hide resolved

danmoseley reviewed Mar 30, 2022

View reviewed changes

src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Extract.cs Outdated Show resolved Hide resolved

Removed redundant test cases, Added contents of the InvalidWindowsFil…

ebac56d

…eNameChars.zip

danmoseley reviewed Mar 30, 2022

View reviewed changes

Replaced "+"s to Path.Combine for forming the test file path.

feac6a7

danmoseley reviewed Mar 30, 2022

View reviewed changes

src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Extract.cs Outdated Show resolved Hide resolved

Update src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Extr…

b8afbef

…act.cs

Danyy427 and others added 9 commits April 1, 2022 01:33

Added code to use ValidFullName across several files

1e9ba60

Merge branch 'Danyy427-ZipFile-Test' of https://github.com/Danyy427/r…

d97c6a2

…untime into Danyy427-ZipFile-Test

Removed weird character

eeac178

Added Sanitization

6fbf14d

Fixed Typo

a36d344

Fixed wrong if statement

f55c3a0

Fixed wrong filename in test

db5102b

Added sanitization as a method

dc7a5fa

The tests added pass with this build

Merge branch 'Danyy427-ZipFile-Test' of https://github.com/Danyy427/r…

af95adb

…untime into Danyy427-ZipFile-Test

danmoseley added 2 commits April 10, 2022 14:48

Change agnostic TFM back to Windows

482bddc

spacing

d9b7f9a

Changed default target framework to Unix

f8d75fc

ViktorHofer reviewed Apr 11, 2022

View reviewed changes

src/libraries/System.IO.Compression.ZipFile/src/System.IO.Compression.ZipFile.csproj Outdated Show resolved Hide resolved

Defaulted framework to unix

034466f

ViktorHofer reviewed Apr 11, 2022

View reviewed changes

src/libraries/System.IO.Compression.ZipFile/src/System.IO.Compression.ZipFile.csproj Outdated Show resolved Hide resolved

Removed Browser

3046526

danmoseley approved these changes Apr 11, 2022

View reviewed changes

ViktorHofer reviewed Apr 11, 2022

View reviewed changes

src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Create.cs Outdated Show resolved Hide resolved

ViktorHofer approved these changes Apr 11, 2022

View reviewed changes

Update src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Crea…

90b2e00

…te.cs Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>

danmoseley merged commit a074a2d into dotnet:main Apr 11, 2022

Danyy427 deleted the Danyy427-ZipFile-Test branch April 12, 2022 08:44

carlossanlop mentioned this pull request Apr 19, 2022

Implement Tar APIs #67883

Merged

ghost locked as resolved and limited conversation to collaborators May 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added test for extracting zip files with invalid characters in Windows #67332

Added test for extracting zip files with invalid characters in Windows #67332

Danyy427 commented Mar 30, 2022

ghost commented Mar 30, 2022

danmoseley commented Mar 30, 2022

Danyy427 commented Mar 30, 2022

danmoseley Mar 30, 2022

danmoseley commented Mar 30, 2022

danmoseley commented Mar 30, 2022

Danyy427 commented Mar 30, 2022 •

edited

Loading

danmoseley commented Mar 30, 2022

Danyy427 commented Mar 30, 2022 •

edited

Loading

Danyy427 commented Apr 10, 2022

danmoseley commented Apr 10, 2022

Danyy427 commented Apr 10, 2022 •

edited

Loading

danmoseley commented Apr 10, 2022

Danyy427 commented Apr 10, 2022

ViktorHofer commented Apr 11, 2022 •

edited

Loading

danmoseley commented Apr 11, 2022

Danyy427 commented Apr 11, 2022

ViktorHofer left a comment

Danyy427 commented Apr 11, 2022

danmoseley commented Apr 11, 2022

ulisesh commented Apr 11, 2022

danmoseley commented Apr 11, 2022

Danyy427 commented Apr 11, 2022

danmoseley commented Apr 11, 2022

Danyy427 commented Apr 11, 2022

Added test for extracting zip files with invalid characters in Windows #67332

Added test for extracting zip files with invalid characters in Windows #67332

Conversation

Danyy427 commented Mar 30, 2022

ghost commented Mar 30, 2022

danmoseley commented Mar 30, 2022

Danyy427 commented Mar 30, 2022

danmoseley Mar 30, 2022

Choose a reason for hiding this comment

danmoseley commented Mar 30, 2022

danmoseley commented Mar 30, 2022

Danyy427 commented Mar 30, 2022 • edited Loading

danmoseley commented Mar 30, 2022

Danyy427 commented Mar 30, 2022 • edited Loading

Danyy427 commented Apr 10, 2022

danmoseley commented Apr 10, 2022

Danyy427 commented Apr 10, 2022 • edited Loading

danmoseley commented Apr 10, 2022

Danyy427 commented Apr 10, 2022

ViktorHofer commented Apr 11, 2022 • edited Loading

Option A

Option B

danmoseley commented Apr 11, 2022

Danyy427 commented Apr 11, 2022

ViktorHofer left a comment

Choose a reason for hiding this comment

Danyy427 commented Apr 11, 2022

danmoseley commented Apr 11, 2022

ulisesh commented Apr 11, 2022

danmoseley commented Apr 11, 2022

Danyy427 commented Apr 11, 2022

danmoseley commented Apr 11, 2022

Danyy427 commented Apr 11, 2022

Danyy427 commented Mar 30, 2022 •

edited

Loading

Danyy427 commented Mar 30, 2022 •

edited

Loading

Danyy427 commented Apr 10, 2022 •

edited

Loading

ViktorHofer commented Apr 11, 2022 •

edited

Loading