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

BUG: Correctly handle image mode 1 with FlateDecode #2249

Merged
merged 3 commits into from
Oct 28, 2023

Conversation

stefan6419846
Copy link
Collaborator

Fixes #2248.

@stefan6419846
Copy link
Collaborator Author

I am not sure about the AES failures in this case, which seems to be an issue wit older Python versions and the used test file. In theory we could skip the tests for Python < 3.9(?), but this does not really seem right.

@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (5c3550f) 94.44% compared to head (5b7bbe3) 94.45%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2249   +/-   ##
=======================================
  Coverage   94.44%   94.45%           
=======================================
  Files          43       43           
  Lines        7621     7622    +1     
  Branches     1502     1501    -1     
=======================================
+ Hits         7198     7199    +1     
  Misses        261      261           
  Partials      162      162           
Files Coverage Δ
pypdf/_xobj_image_helpers.py 92.02% <100.00%> (+0.05%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MartinThoma
Copy link
Member

@exiledkingcc Could you please have a look at it?

@stefan6419846
Copy link
Collaborator Author

As another data point: It seems like this only affects Python 3.7 where neither pycryptodome nor cryptography are being installed. Why do we apparently not test Python 3.7 with AES encryption at all?

For reference: https://github.com/stefan6419846/pypdf/actions/runs/6469234606/ uses the same code as the branch of this PR, but with fail-fast disabled compared to the "original" PR run in https://github.com/py-pdf/pypdf/actions/runs/6458194919/

@exiledkingcc
Copy link
Contributor

the test pdf file is encrypted with empty password. so is ok to skip if no AES decrypting.
pytests for Python 3.7 without pycryptodome and cryptography is just to make pytests simple.
no need to test pycryptodome or cryptography or none oth them for every python versions.

@stefan6419846
Copy link
Collaborator Author

@MartinThoma I would argue that we could indeed merge this then. Discussing how to continue with one Python version not being tested with encryption should probably be a separate issue/PR in this case - just adding another matrix entry should be sufficient and if desired, I am open to providing a corresponding PR for it.

@MartinThoma MartinThoma merged commit e9241ac into py-pdf:main Oct 28, 2023
12 checks passed
@MartinThoma
Copy link
Member

Thank you @stefan6419846 for reporting the issue and for fixing it 🙏

Yes, it would be nice to complete our CI to ensure we don't have relevant gaps in test coverage

@MartinThoma
Copy link
Member

@exiledkingcc Thanks for having a look 🙏

MartinThoma added a commit that referenced this pull request Oct 29, 2023
## What's new

### Security (SEC)
-  Infinite recursion when using PdfWriter(clone_from=reader) (#2264) by @Alexhuszagh

### New Features (ENH)
-  Add parameter to select images to be removed (#2214) by @pubpub-zz

### Bug Fixes (BUG)
-  Correctly handle image mode 1 with FlateDecode (#2249) by @stefan6419846
-  Error when filling a value with parentheses #2268 (#2269) by @KanorUbu
-  Handle empty root outline (#2239) by @pubpub-zz

### Documentation (DOC)
-  Improve merging docs (#2247) by @stefan6419846

### Developer Experience (DEV)
-  Test Python 3.7 with cryptopgraphy provider as well (#2276) by @stefan6419846
-  Run CI with windows-latest (#2258) by @MartinThoma
-  Use pytest-xdist (#2254) by @MartinThoma
-  Attribute correct authors in the release notes (#2246) by @stefan6419846

### Maintenance (MAINT)
-  Apply pre-commit hooks (#2277) by @MartinThoma
-  Update requirements + mypy fixes (#2275) by @MartinThoma
-  Explicitly provide Any for IO generic argument (#2272) by @nilehmann

### Testing (TST)
-  Fix test_image_without_pillow in windows environment (#2257) by @pubpub-zz

### Code Style (STY)
-  Remove unused import by @MartinThoma

[Full Changelog](3.16.4...3.17.0)
@stefan6419846 stefan6419846 deleted the flatedecode-imagemode-1 branch December 20, 2023 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect handling of FlateDecode with image mode "1"
3 participants