Skip to content

ROB: guard missing /AP appearance state in button form fields#3893

Open
metsw24-max wants to merge 2 commits into
py-pdf:mainfrom
metsw24-max:button-field-appearance-guard
Open

ROB: guard missing /AP appearance state in button form fields#3893
metsw24-max wants to merge 2 commits into
py-pdf:mainfrom
metsw24-max:button-field-appearance-guard

Conversation

@metsw24-max

@metsw24-max metsw24-max commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Uncaught KeyError reading button field appearance states

_build_field collects the checkbox and radio appearance states from /AP /N, but it assumes both are present and are dictionaries, so a button field whose /AP has no /N, or a radio kid with no /AP//N, raises an uncaught KeyError out of the public reader.get_fields() on a malformed document.

Both lookups now go through a small _normal_appearance helper on PdfDocCommon. /N is required in an appearance dictionary, so a malformed appearance (a non-dictionary /AP, or a missing or non-dictionary /N) now raises the documented PdfReadError rather than a bare KeyError. Valid forms are unchanged.

@codecov

codecov Bot commented Jun 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.87%. Comparing base (2266ee8) to head (32a3ccd).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3893      +/-   ##
==========================================
+ Coverage   97.83%   97.87%   +0.03%     
==========================================
  Files          55       57       +2     
  Lines       10609    10753     +144     
  Branches     1985     2010      +25     
==========================================
+ Hits        10379    10524     +145     
  Misses        127      127              
+ Partials      103      102       -1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread pypdf/_doc_common.py Outdated
Comment thread pypdf/_doc_common.py Outdated
Comment thread pypdf/_doc_common.py Outdated
Comment thread tests/test_doc_common.py Outdated
Comment thread tests/test_doc_common.py Outdated
@metsw24-max
metsw24-max force-pushed the button-field-appearance-guard branch from 9cc752c to b454298 Compare June 24, 2026 12:14
@metsw24-max

Copy link
Copy Markdown
Contributor Author

Pushed an update. The helper now lives on PdfDocCommon as a staticmethod and raises PdfReadError for a malformed appearance (non-dictionary /AP, or a missing/non-dictionary /N) rather than skipping silently, so the public get_fields() reports a documented error instead of leaking a bare KeyError. Tests updated to assert the raise and to use an exact /States comparison.

Comment thread pypdf/_doc_common.py
"""Return the /N normal-appearance sub-dictionary of an /AP entry."""
appearance = appearance.get_object()
if not isinstance(appearance, DictionaryObject):
raise PdfReadError(f"Expected appearance dictionary, got {appearance!r}")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is uncovered. Please check.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Added a checkbox case with a non-dictionary /AP, so that raise is exercised now and the branch is covered.

@metsw24-max

Copy link
Copy Markdown
Contributor Author

any update?

@stefan6419846

Copy link
Copy Markdown
Collaborator

See #3900.

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.

2 participants