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

Update the number of property blocks in the POIFS header block #462

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

ebourg
Copy link
Member

@ebourg ebourg commented May 3, 2023

@centic9
Copy link
Member

centic9 commented May 6, 2023

Thanks for the changes. Looks good on a brief look.

Can you add a sample .msi-file to directory test-data/poi-fs and a small unit-test which verifies opening it so we verify the intended feature as well?

@ebourg
Copy link
Member Author

ebourg commented May 6, 2023

I did modify the existing tests to ensure the header is properly updated. I don't think adding an actual msi file will improve the test coverage, it's just a CFB file like any other one.

@pjfanning
Copy link
Contributor

I don't think we should merge this without an MSI test. We have zero test coverage of MSIs.

-1 from me

@ebourg
Copy link
Member Author

ebourg commented May 6, 2023

I can contribute one for the tests (I've built one for the Jsign tests: https://github.com/ebourg/jsign/blob/master/jsign-core/src/test/resources/minimal.msi), but a msi file is really not different from other formats handled by POIFS, and this issue is mostly related to the correctness of the header information, that's not MSI specific.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants