Skip to content

fix(pdf): a PDF pypdf can't read is a 400, not a 500 — LimitReachedError too - #186

Merged
MrChengLen merged 10 commits into
mainfrom
pr-pypdf-limit-errors
Oct 1, 2026
Merged

fix(pdf): a PDF pypdf can't read is a 400, not a 500 — LimitReachedError too#186
MrChengLen merged 10 commits into
mainfrom
pr-pypdf-limit-errors

Conversation

@MrChengLen

@MrChengLen MrChengLen commented Sep 30, 2026

Copy link
Copy Markdown
Owner

What

A PDF that pypdf can't read now gets a clean 400 with the fixed message "Could not read the PDF. Verify the file is valid." on every pypdf path — instead of the generic 500 plus a logged traceback.

Route Before After
/pdf/extract 500 (lazy failures), 400 invalid_page_selection (open failures) 400 invalid_pdf
/pdf/split 500 (lazy failures), 400 invalid_pdf (open failures) 400 invalid_pdf
/convert pdf → txt 500 for every unreadable / password-protected PDF 400 invalid_input
/convert pdf → pdf (pass-through) 500 400 invalid_input
/convert/batch (both) "Conversion failed. Verify the file is valid." the fixed PDF message

Why

pypdf's LimitReachedError (a crafted file tripping a resource limit) derives from PyPdfError, not from the PdfReadError that _open_reader caught. And pypdf parses lazily: most limits — and a damaged page object's ValueError("Invalid page object"), the most common failure in a 3000-case mutation fuzz — fire in add_page/write or extract_text, after the one guarded open. PdfToTxtConverter caught 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), mapping PyPdfError + ValueError to UnreadablePdfError (a PageSelectionError) and logging only the error's class. OSError is no longer mapped — pypdf reads the whole file into memory first, so it can only be our own disk (stays a 500). PageSelectionError is now an InvalidInputError, so /convert and /convert/batch use 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/extract answers invalid_pdf for an unreadable file (the web page showed "Invalid page selection" for it); selection errors keep invalid_page_selection.
  • Docs: error-code table (docs/api-reference.md), batch message list (docs/api-usage-guide.md), CHANGELOG.

Deliberately not widened (follow-up): AttributeError, KeyError, TypeError, NotImplementedError, IndexError, OverflowError that pypdf raises on some other damaged files.

Tests

  • New 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 /W range (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.
  • Against the old code 24 of the 30 fail (the other 6 check the fixtures); narrowing the catch to PyPdfError fails exactly the 8 ValueError cases.
  • Full suite 1519 passed, 72 skipped (pypdf 6.19.0 as in 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

MrChengLen and others added 10 commits September 30, 2026 15:23
@MrChengLen @claude
fix(pdf): a PDF pypdf can't read is a 400, not a 500 — LimitReachedEr…
b46dd5f 
…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>
@MrChengLen @claude
chore(changelog): take the entry out to merge main in — back in two c…
9159fac 
…ommits

#183 added its entry on top of the same changelog section, so this PR
conflicts there. The entry goes out here, main is merged in without a
conflict, and the entry comes back on top of the merged changelog.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@MrChengLen
Merge branch 'main' into pr-pypdf-limit-errors
0e216e7
@MrChengLen @claude
test(pdf): QA fixtures for unreadable PDFs — changelog entry back on top
18bb36d 
The changelog entry returns above #183's, with one more paragraph for the
new scripts/make_testdata_pdf_unreadable.py: it writes the files the
manual checklist uses (a valid three-page control file, an untyped page
object, an oversized declared /Length, a 120-level page tree, a huge CID
/W range, a font map yielding an unpaired surrogate, and the control file
locked with a test password) to the gitignored
docs-internal/testdata/pdf-unreadable/. Two runs are byte-identical; the
locked copy uses RC4-128, which needs no random salt. Each file was run
through the real extract, split and convert-to-txt routes and answers as
the checklist says.

Full suite 1530 green, 72 skipped, on main b2deb14 plus this PR (pypdf 6.19.0
as in requirements.lock); ruff + format clean; gitleaks clean.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@MrChengLen @claude
chore(changelog): take the entry out again to merge main in — back in…
65a4be8 
… two commits

#190 (urllib3 2.8.0) added its Security entry on top of the same
changelog section. The entry goes out here, main is merged in without a
conflict, and the entry comes back on top of the merged changelog.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@MrChengLen
Merge branch 'main' into pr-pypdf-limit-errors
5977360
@MrChengLen @claude
docs(changelog): entry back on top of the merged changelog
615de79 
The entry returns unchanged, now above #190's urllib3 Security entry.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@MrChengLen @claude
chore(changelog): take the entry out again to merge main in — back in…
7437eff 
… two commits

#185 added its entries on top of the same changelog section. The entry
goes out here, main is merged in without a conflict, and the entry comes
back on top of the merged changelog.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@MrChengLen
Merge branch 'main' into pr-pypdf-limit-errors
6efd26c
@MrChengLen @claude
docs(changelog): entry back on top of the merged changelog
7eb8e60 
The entry returns unchanged, now above #185's entries.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@MrChengLen
MrChengLen merged commit bd273d4 into main Oct 1, 2026
7 checks passed
@MrChengLen
MrChengLen deleted the pr-pypdf-limit-errors branch October 1, 2026 12:27
@MrChengLen MrChengLen mentioned this pull request Oct 6, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@MrChengLen