fix(pdf): a PDF pypdf can't read is a 400, not a 500 — LimitReachedError too - #186
Open
MrChengLen wants to merge 1 commit into
Open
MrChengLen wants to merge 1 commit into
MrChengLen wants to merge 1 commit into
Conversation
…ror too pypdf raises LimitReachedError when a crafted file trips one of its resource limits. It derives from PyPdfError, not PdfReadError, so _open_reader's catch missed it; and pypdf parses lazily, so most limits (and a damaged page object's ValueError) fire in add_page/write or in extract_text, after the one guarded open. Result: generic 500 plus a logged traceback on /pdf/extract, /pdf/split and /convert pdf->pdf, and for every unreadable or password-protected PDF on pdf->txt, which caught nothing. No detail reached the client; status and log were wrong. - pdf_pages.py: one _reading_pdf() guard around every pypdf step that reads the input (open, add_page/write in extract, the split loop) maps PyPdfError and ValueError to UnreadablePdfError, a PageSelectionError with a fixed message, and logs the error's class (never its message, which can quote the file). OSError is no longer mapped: pypdf reads the whole file into memory first, so it can only be our own disk, a 500. PageSelectionError is now an InvalidInputError (and no longer a ValueError, which nothing relied on), so /convert and /convert/batch answer it through their existing 400 invalid_input / per-file-message branches. - document.py: PdfToTxtConverter maps the same errors from open and text extraction to InvalidInputError, and writes an unpaired surrogate (from a broken ToUnicode map) as "?" instead of failing the UTF-8 write with a 500. - /pdf/extract sends invalid_pdf for an unreadable file. It sent invalid_page_selection, which made pdf-tools.js tell the user to fix a page selection that was fine; invalid_pdf already maps to the localized "Could not read the PDF" text. Docs table updated. Not widened: pypdf also raises AttributeError, KeyError, TypeError, NotImplementedError and IndexError on some damaged files (mutation fuzz, review); adding them is a policy change beyond this finding, left for a follow-up. Tests (tests/test_pdf_unreadable.py): tiny in-process PDFs fail at each stage (page tree deeper than 100 levels, an 80 MB /Length, a page object without /Type, a 100 000-glyph /W range, bad ASCII85); every route answers 400 with the fixed message and logs no traceback; the log names the class, not pypdf's message. 24 of the 30 new tests fail on the old code (the other 6 check the fixtures); narrowing the catch to PyPdfError fails the 8 ValueError cases. Full suite 1519 green, 72 skipped (pypdf 6.19.0 as in requirements.lock; the new tests also pass on 6.16.2); ruff + format clean; gitleaks clean; i18n and dependencies untouched. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
A PDF that pypdf can't read now gets a clean
400with the fixed message "Could not read the PDF. Verify the file is valid." on every pypdf path — instead of the generic500plus a logged traceback./pdf/extractinvalid_page_selection(open failures)invalid_pdf/pdf/splitinvalid_pdf(open failures)invalid_pdf/convertpdf → txtinvalid_input/convertpdf → pdf (pass-through)invalid_input/convert/batch(both)Why
pypdf's
LimitReachedError(a crafted file tripping a resource limit) derives fromPyPdfError, not from thePdfReadErrorthat_open_readercaught. And pypdf parses lazily: most limits — and a damaged page object'sValueError("Invalid page object"), the most common failure in a 3000-case mutation fuzz — fire inadd_page/writeorextract_text, after the one guarded open.PdfToTxtConvertercaught nothing. No detail reached the client; status and log were wrong.How
app/converters/pdf_pages.py: one_reading_pdf()guard around every pypdf step that reads the input (open, copy/write, split loop), mappingPyPdfError+ValueErrortoUnreadablePdfError(aPageSelectionError) and logging only the error's class.OSErroris no longer mapped — pypdf reads the whole file into memory first, so it can only be our own disk (stays a 500).PageSelectionErroris now anInvalidInputError, so/convertand/convert/batchuse their existing 400 branches.app/converters/document.py: same mapping for pdf → txt; an unpaired surrogate from a broken ToUnicode map is written as?instead of failing the UTF-8 write (another 500).app/api/routes/pdf_pages.py:/pdf/extractanswersinvalid_pdffor an unreadable file (the web page showed "Invalid page selection" for it); selection errors keepinvalid_page_selection.docs/api-reference.md), batch message list (docs/api-usage-guide.md), CHANGELOG.Deliberately not widened (follow-up):
AttributeError,KeyError,TypeError,NotImplementedError,IndexError,OverflowErrorthat pypdf raises on some other damaged files.Tests
tests/test_pdf_unreadable.py(30 tests): tiny in-process PDFs fail at each stage — page tree deeper than 100 levels (open), 80 MB/Length(copy), page object without/Type(copy,ValueError), 100 000-glyph/Wrange (text), invalid ASCII85 (text,ValueError) — plus the surrogate case. Every route answers 400 with the fixed message and logs no traceback; the log names the class, never pypdf's message.PyPdfErrorfails exactly the 8ValueErrorcases.requirements.lock; the new tests also pass on 6.16.2). ruff + format clean, gitleaks clean. Reviewed by the security-auditor (PASS) and code-reviewer (ready) agents.🤖 Generated with Claude Code