Skip to content
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

Move the Password property from DeflaterOutputStream into ZipOutputStream #604

Merged
merged 3 commits into from
May 13, 2021

Conversation

Numpsy
Copy link
Contributor

@Numpsy Numpsy commented Mar 14, 2021

refs #590

This would be a breaking API change, but - is there actually a need for the Password property to be on DeflaterOutputStream, given that it only applies to creating encrypted Zip files? (the property isn't actually referenced by DeflaterOutputStream anyway, only from ZipOutputStream)

Related:
The InitializePassword and InitializeAESPassword functions in DeflaterOutputStream only seem to be called from ZipOutputStream as well, though they set the (presently private) cryptoTransform_ member as well, so moving them as well would need a further change.

I certify that I own, and have sufficient rights to contribute, all source code and related material intended to be compiled or integrated with the source code for the SharpZipLib open source product (the "Contribution"). My Contribution is licensed under the MIT License.

@codecov
Copy link

codecov bot commented Mar 14, 2021

Codecov Report

Merging #604 (c15cb91) into master (d9fb8a4) will increase coverage by 0.06%.
The diff coverage is 93.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #604      +/-   ##
==========================================
+ Coverage   73.24%   73.30%   +0.06%     
==========================================
  Files          68       68              
  Lines        8712     8710       -2     
==========================================
+ Hits         6381     6385       +4     
+ Misses       2331     2325       -6     
Impacted Files Coverage Δ
...ib/Zip/Compression/Streams/DeflaterOutputStream.cs 85.36% <ø> (-0.64%) ⬇️
src/ICSharpCode.SharpZipLib/Zip/ZipOutputStream.cs 92.19% <93.75%> (+0.07%) ⬆️
...Code.SharpZipLib/Zip/Compression/DeflaterEngine.cs 87.45% <0.00%> (+0.33%) ⬆️
...ICSharpCode.SharpZipLib/BZip2/BZip2OutputStream.cs 79.19% <0.00%> (+0.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9fb8a4...c15cb91. Read the comment docs.

@Numpsy
Copy link
Contributor Author

Numpsy commented Apr 28, 2021

@piksel would you consider the public API change as acceptable, before I see about doing anything else with this?

@piksel
Copy link
Member

piksel commented Apr 28, 2021

Yeah, I think it's good idea. It's a breaking change on a technical level, but if there is any code out there relying on it, it's probably not doing what the author intended anyway (as shown by the referenced issue).

@Numpsy Numpsy marked this pull request as ready for review April 28, 2021 18:18
@Numpsy
Copy link
Contributor Author

Numpsy commented Apr 28, 2021

hmm, moved the code that sets up the crypto transforms and didn't notice till looking at the coverage results that the existing code initialises the static _aesRnd member where it's declared and also checks it for null and tries to set it up where it's used, can have a look at tidying that up as well.

@Numpsy Numpsy added encryption zip Related to ZIP file format labels Apr 28, 2021
@piksel
Copy link
Member

piksel commented May 8, 2021

Will still push this to v1.4.0 due to the "breakage".

@piksel piksel added this to the v1.4 milestone May 8, 2021
@piksel piksel merged commit 08c40e8 into icsharpcode:master May 13, 2021
@Numpsy Numpsy deleted the rw/590 branch May 14, 2021 10:56
HowToDoThis added a commit to HowToDoThis/SharpZipLib that referenced this pull request Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
encryption zip Related to ZIP file format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants