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

Add the vba ppt sample to the find_vba list in the test_macros function. #862

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

Conversation

kijeong
Copy link
Contributor

@kijeong kijeong commented Jun 25, 2024

This pull request is related to #859 and #723.
I made some additional adjustments to the changes from #859.
@christian-intra2net, it would be great if you could review it.

The file 'olevba/sample_with_vba.ppt' contains an actual VBA macro.

And in my opinion, the code page should not vary depending on the test machine.

I referred to the following parts:

  • olefile.py:OleFileIO.getproperties function

    • property_id: 1, value: 949
  • codepages.py:get_codepage_name, CODEPAGE_NAME[949]

…nction.

The file 'olevba/sample_with_vba.ppt' contains an actual VBA macro.
@christian-intra2net
Copy link
Contributor

You are right, of course, the local system settings should not matter. Unfortunately, I had to make the unpleasant experience that sometimes it does matter, at least for system encoding.
Anyway, I checked you code and ran it and this works nicely. Apparently, also on github actions server with US locale this went through. I failed to note down the result I had gotten, that differed from yours. So, I have to assume I made an error and all is fine with the code itself.
I suggest to merge this PR

@decalage2 decalage2 requested review from decalage2 June 26, 2024 13:55
@decalage2 decalage2 self-assigned this Jun 26, 2024
@decalage2 decalage2 modified the milestone: oletools 0.60 Jun 26, 2024
@kijeong
Copy link
Contributor Author

kijeong commented Jun 27, 2024

You are right, of course, the local system settings should not matter. Unfortunately, I had to make the unpleasant experience that sometimes it does matter, at least for system encoding. Anyway, I checked you code and ran it and this works nicely. Apparently, also on github actions server with US locale this went through. I failed to note down the result I had gotten, that differed from yours. So, I have to assume I made an error and all is fine with the code itself. I suggest to merge this PR

Thank you for reviewing my pull request and for running the code on your end.

I appreciate your understanding regarding the system encoding variations. As I don't live in a US locale, I've had my fair share of experiences with encoding-related issues,, and I am also sensitive to these problems. It's a relief to know that the code worked fine on the GitHub Actions server with a US locale.

If you encounter any issues again, please feel free to share them with me.

Thanks again for your feedback and support.

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

Successfully merging this pull request may close these issues.

3 participants