Using wrong password to test error case #10

Closed
lordmacintosh opened this Issue Oct 12, 2012 · 9 comments

Comments

Projects
None yet
2 participants

Hi, great work Matt!

I had a question for you about unzipping with a password.
I am able to zip with a password, and unzip with the password without any problem.

I tried an error case using a wrong password to unzip. I expected to receive an indication that the unzip was unsuccessful, but I get a return of success. As expected, the zipped files are not actually unzipped (which is good), however, the directory now holds a bunch of zero byte files with the correct file names that were in the zip. Seems strange.

Is this expected, or should there be a return somewhere indicating an invalid password?

Thx!
Anthony

Owner

mattconnolly commented Oct 13, 2012

I've just reproduced that and wouldn't call it expected behaviour.

mattconnolly was assigned Oct 13, 2012

@mattconnolly mattconnolly added a commit that referenced this issue Oct 14, 2012

@mattconnolly mattconnolly Making unzip only create file when there is valid data to read from z…
…ip file.

Fixes zero length files with wrong password, issue #10
b604d55
Owner

mattconnolly commented Oct 14, 2012

How does this work for you? I made it create the file at the first successful read (which could be for a zero length file, since that is a possibility).

Definitely better. I confirm your change fixed the creation of zero length files.
However, the unzip process still returns success, making my app layer believe the unzip was successful even though it was not. At what point, can it be determined that the unzip has failed due to bad password?
Thanks for looking at this!

Owner

mattconnolly commented Oct 14, 2012

Ah. Now it does call the error callback on the delegate. Do you think it should return failed of one or any files couldn't be extracted?

Lets say I have 100 files that were zipped with password.
If a bad password is used for the unzip, then I receive 100 delegate calls to the delegate method "ErrorMessage:" with a failure of "Failed to reading zip file"

I would prefer one of the following:

  1. I do not implement the delegate method, and instead I rely on the "UnzipFileTo:(NSString_) path overWrite:(BOOL) overwrite" method return NO to indicate that a failure has occurred
  2. or, I implement the delegate so I know about the failure, and have the method "UnzipFileTo:(NSString_) path overWrite:(BOOL) overwrite" abort after the first file fails

What are your thoughts?

Currently, I believe I would do the following with the current implementation: make a call to "UnzipFileTo:(NSString*) path overWrite:(BOOL) overwrite" and if I get a delegate callback then I set an error flag. Later, when the UnzipFileTo method returns I check if the error flag was set or if the method returned a NO. if either is true, then the zip was unsuccessful.

Owner

mattconnolly commented Oct 16, 2012

How about we an "abortOnError" property which when set will cause any operating to stop at the first error.

And fix the return result of "UnzipFileTo:" to be NO if ANY of the files fail. ie: YES means a completely successful operation. I'll do that tonight.

Hi Matt - Here is one solution, I believe that if you put success = NO where the error happens:
[self OutputErrorMessage:@"Failed to reading zip file"];
success = NO;
and then check for success in the top layer loop, it will break out on the first error:
while( ret==UNZ_OK && UNZ_OK!=UNZ_END_OF_LIST_OF_FILE && NO!=success);

Some other thoughts:
I thought about adding an UNZ_BAD_PASSWORD for ret status at the point of failure, but ret gets set after that point, so that isn't very clean.

Perhaps the addition of a new method for passwordIsCorrect that return BOOL Yes on success. This small check would be performed by the app layer before trying to unzip the whole file.

Just a few thoughts..

Thanks again!

@mattconnolly mattconnolly added a commit that referenced this issue Nov 8, 2012

@mattconnolly mattconnolly Updating unzip with wrong password to stop at first file failure; cal…
…ling error callback once; and also return success = NO.

Also adding a test target with some tests to verify behaviour.

See Issue #10
47a0377
Owner

mattconnolly commented Nov 8, 2012

I've expanded a bit on your proposed solution and ensured that it returns NO when it fails. Also added some tests for it too.

Does this work for you now?

Owner

mattconnolly commented Apr 7, 2013

Closing due to no responses for a few months.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment