Signatures: defer validation without weakening signed-file guards

[cyfung1031]

Review direction

Review the exact signed-presence and rewrite-safety behavior first. The timing and RSS figures are performance context; assess correctness from the pre-validation guard tests and the matching signature-list outputs.

What changed

• Document::is_signed() uses an exact structural presence check before signature validation. It covers adopted page-only signature fields and standalone document timestamps, including malformed signature dictionaries that must still block a full rewrite.
• Session::open defers full signature discovery and cryptographic validation. Document::signatures() validates on first detail use and reuses the result until a document edit or trust-store change invalidates it.
• The Signatures panel and signature tools request validated details. Ordinary canvas and rail rendering use only already-loaded details.
• No network validation is added.

Refs #307.

Validation

• cargo test --locked --profile low-disk -p pdfcraft-engine signed_state_ — 2 passed.
• cargo test --locked --profile low-disk -p pdfcraft-engine signing_saving_trusting_and_commenting_afterwards — passed; covers trust changes, incremental saves, and full-rewrite refusal.
• cargo check --locked --profile low-disk --tests -p pdfcraft-engine -p pdfcraft-ui-egui -p pdfcraft-automation — passed.
• Focused Clippy for the same packages and test targets passed with -A clippy::collapsible_match for the existing main-branch warning.
• cargo fmt --all and git diff --check passed.
• Measured on 2026-10-13 on macOS 14.7.1 (23H222), Darwin 23.6.0 arm64, 8 cores and 16 GiB RAM, with other applications running. Base: storytold/pdfcraft:main at 87b448b664e1b3c960b479cc0f2c2e7a82c7ac71, the merge base of this branch. Head: cyfung1031/pdfcraft:codex/open-scale-pr-03 at b9cd81fad8d67d30dd8b45c2ec28b26df48c7260. Both pdfcraft-cli binaries use the low-disk profile. Inputs: demo_heavy_engineering_A1.pdf, vmware-cloud-foundation-9-1.pdf, and pdf_engine_test.pdf. Each row has one unrecorded warm-up and then 15 measured rounds, with base and head alternating order. CPU was at least 60% idle before each round. Stdout is compared by SHA-256 for doc_open and doc_open + sign_list. Base and head run back to back in each round, so each round gives one paired comparison; a row is called consistent only when one side wins at least 12 of 15 pairs (two-sided exact sign test, p < 0.05).

None of the three files is signed (sign_list count 0 on each), so the signed-file validation path is not measured here. The tables cover the unsigned open path and the first signature-list call.

Inputs

File	Size	Pages	Links	Signatures
demo_heavy_engineering_A1.pdf	37.7 MiB	1	0	0
vmware-cloud-foundation-9-1.pdf	184.0 MiB	9,156	17,548	0
pdf_engine_test.pdf	0.3 MiB	11	2	0

Elapsed time

File	Workload	Base elapsed ms, median (min–max)	PR elapsed ms, median (min–max)	Change	Paired rounds PR faster	Verdict	Output identical
demo_heavy_engineering_A1.pdf	open	14.7 (13.8–57.1)	15.1 (13.8–42.3)	+3.2%	7/15	no consistent difference	yes (5c2a942361c4)
vmware-cloud-foundation-9-1.pdf	open	8,041 (7,848–8,605)	2,702 (2,635–3,452)	-66.4%	15/15	faster, consistent	yes (6f510de5299a)
pdf_engine_test.pdf	open	17.4 (16.6–22.1)	13.8 (13.4–15.6)	-20.7%	15/15	faster, consistent	yes (7684c4060a9d)
demo_heavy_engineering_A1.pdf	open + sign_list	15.0 (13.6–58.8)	15.4 (14.2–41.0)	+2.9%	7/15	no consistent difference	yes (da1b19944370)
vmware-cloud-foundation-9-1.pdf	open + sign_list	8,374 (7,725–9,276)	8,399 (7,866–8,869)	+0.3%	6/15	no consistent difference	yes (548eadb755f4)
pdf_engine_test.pdf	open + sign_list	17.3 (16.8–21.9)	17.2 (17.0–18.8)	-0.2%	5/15	no consistent difference	yes (bd2494beb80a)

Peak memory footprint and max RSS

File	Workload	Base peak footprint MiB, median (min–max)	PR peak footprint MiB, median (min–max)	Change	Paired rounds PR footprint lower	Base max RSS MiB, median (min–max)	PR max RSS MiB, median (min–max)	Paired rounds PR RSS lower
demo_heavy_engineering_A1.pdf	open	42.8 (42.8–42.9)	42.8 (42.7–42.9)	+0.0%	6/15 lower, no consistent difference	48.0 (47.9–48.2)	47.9 (47.8–48.0)	13/15 lower, consistent
vmware-cloud-foundation-9-1.pdf	open	475.4 (433.2–535.2)	465.9 (435.2–504.6)	-2.0%	8/15 lower, no consistent difference	572.8 (387.9–618.9)	565.6 (354.8–587.2)	9/15 lower, no consistent difference
pdf_engine_test.pdf	open	8.3 (7.9–9.1)	8.1 (8.0–8.9)	-2.8%	12/15 lower, consistent	14.2 (13.8–14.9)	13.8 (13.7–14.5)	14/15 lower, consistent
demo_heavy_engineering_A1.pdf	open + sign_list	42.8 (42.8–43.0)	42.9 (42.7–43.0)	+0.0%	8/15 lower, no consistent difference	48.1 (48.0–48.3)	48.2 (48.0–48.4)	5/15 lower, no consistent difference
vmware-cloud-foundation-9-1.pdf	open + sign_list	463.9 (443.6–505.3)	464.0 (430.4–506.8)	+0.0%	7/15 lower, no consistent difference	535.7 (395.4–586.2)	442.4 (384.1–589.6)	9/15 lower, no consistent difference
pdf_engine_test.pdf	open + sign_list	8.2 (8.0–9.5)	8.3 (8.0–9.2)	+0.9%	12/15 higher, consistent	14.2 (14.0–15.4)	14.3 (14.0–15.1)	12/15 higher, consistent

The one consistent large change is doc_open on vmware-cloud-foundation-9-1.pdf (9,156 pages, unsigned): faster in 15 of 15 paired rounds, median 8,041 ms to 2,702 ms. The PR's open + sign_list on that file (8,399 ms) matches base's open + sign_list (8,374 ms; faster in 6 of 15 rounds, no consistent difference), so the saving applies only to sessions that never request signature details. pdf_engine_test.pdf also opens faster in 15 of 15 rounds (17.4 ms to 13.8 ms, a 3.6 ms gap). Its max RSS is lower in 14 of 15 rounds on open (14.2 to 13.8 MiB), and its peak footprint is lower in 12 of 15 (8.3 to 8.1 MiB). On open + sign_list, its footprint and max RSS are higher in 12 of 15 rounds each (about 0.1 MiB). demo_heavy_engineering_A1.pdf max RSS is lower in 13 of 15 rounds on open (48.0 to 47.9 MiB), with no consistent footprint change. The consistent memory changes are all under 0.5 MiB. The benchmark does not profile where the open-time saving comes from.

Hosted CI is not used as a validation gate.

Related work

This change overlaps signature and trust paths in #63, #622, and #760. It is based directly on main and is not stacked on those branches.

Summary by CodeRabbit

• Bug Fixes
  • Signed documents now show their signature status on the first render, including when a signature needs validation.
  • Improved signature notices distinguish documents with detected signatures from documents with no signatures.
• Performance
  • Signature validation is deferred until signature details are needed, helping avoid unnecessary validation when opening unsigned documents.

[parinya-ao]

I have a question?

• Would you mind sharing whether this is expected to show a larger benefit on larger or real-world PDFs?

• The current benchmark looks fairly similar, so I’m curious where the main performance gain is expected.

[cyfung1031]

I have a question?

• Would you mind sharing whether this is expected to show a larger benefit on larger or real-world PDFs?
• The current benchmark looks fairly similar, so I’m curious where the main performance gain is expected.

as the benchmark target was not specified, synthetic data were used to present that measurement table.

Now replaced the measurement with three actual PDF files.

Three Actual PDFs:

Issue number	File name	File URL
#260	pdf_engine_test.pdf	https://github.com/user-attachments/files/33201942/pdf_engine_test.pdf
#623	demo_heavy_engineering_A1.pdf	https://raw.githubusercontent.com/cyfung1031/BergPDF/a9f4bac2a88290d72eb123bf1b0aa5b854e2aff2/sample/demo_heavy_engineering_A1.pdf
#1483	vmware-cloud-foundation-9-1.pdf	https://techdocs.broadcom.com/content/dam/broadcom/techdocs/us/en/pdf/vmware/vcf/vcf-90/vmware-cloud-foundation-9-1.pdf

[FredApps]

Changes requested. I did not run tests (branch is CONFLICTING/DIRTY, so no host run was queued); hosted CI is green on this head (ubuntu, windows, wasm32, licences), and I reviewed the full diff by reading.

The engine side looks right: is_signed() is now an exact structural check (signature fields with a dict /V, adopted page-only fields, standalone DocTimeStamp), the SignedRewrite guard still runs before any rewrite, and validation caches are invalidated on every refresh and on trust changes.

Problem: opening a signed file no longer shows that it is signed.

• canvas.rs now builds the banner from doc.validated_signatures().unwrap_or_default(), and chrome.rs computes has_signatures from validated_signatures() too. Nothing on the open or render path calls signatures(), so on a freshly opened signed PDF the "Signed ..." / "signatures have problems" banner and the Signatures rail button are absent until something else happens to trigger validation. The rail button is also how the user opens the panel that would trigger it.
• The tests mask this: in ui-egui/tests/signing.rs the code calls doc.signatures() (which fills the cache) before get_by_label("At least one signature has problems."), and the engine test only asserts the accessors.

Fix: use the new cheap doc.is_signed() for has_signatures in right_rail, and in document_area compute the banner from doc.signatures() only when doc.is_signed() is true (unsigned files keep the fast open the PR is after). Add a UI test that opens a signed fixture without touching signatures() and asserts the banner text and the Signatures rail button.

Also rebase onto main (conflicts).

[cyfung1031]

Thanks for catching the fresh-open regression. I rebased onto current main (3f5a784c) and fixed the first-render behavior:

• Structurally signed documents load signature details for the banner on first render, while unsigned documents keep validation deferred.
• The Signatures rail indicator uses structural presence, so malformed signed documents remain visible even when validation returns no details.
• Page badges use only already-validated details, so ordinary unsigned rendering does not trigger validation.

Added UI coverage for signed, unsigned, and malformed fresh-open cases. Local verification on the rebased head: the 14 UI signing tests pass; both signed_state_ engine tests and signing_saving_trusting_and_commenting_afterwards pass; the engine/UI/automation test check and UI Clippy pass. Hosted CI is running on d2cb046f.

[FredApps]

Reviewed d2cb046. The fresh-open blocker from my previous review is addressed: structurally signed documents populate the banner on first render, unsigned documents retain deferred validation, and malformed signed documents keep their visible signature warning/rail presence without inventing validated details. Page badges consume only cached validation results.

Hosted CI run 39082116328 is successful for Ubuntu, Windows, wasm32 and licences; fuzz was skipped. I verified that both OS logs pass the three new fresh-open regressions, both signed_state_ tests, and signing_saving_trusting_and_commenting_afterwards. Those jobs tested synthetic merge 44f2ec5, combining this head with main 3f5a784, rather than a literal head checkout.

No remaining blocking finding from this review. I did not run a separate local desktop test or independently reproduce the performance benchmarks.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: c8872f73-0efa-4b2d-92f5-5b890d20788e

📥 Commits

Reviewing files that changed from the base of the PR and between ec0a965 and 13a7c68.

📒 Files selected for processing: 12

• ROADMAP.md
• crates/automation/src/lib.rs
• crates/automation/src/signing.rs
• crates/engine/src/lib.rs
• crates/engine/src/tests.rs
• crates/sign/src/pdf.rs
• crates/ui-egui/src/canvas.rs
• crates/ui-egui/src/panels.rs
• crates/ui-egui/src/sign_ui.rs
• crates/ui-egui/tests/signing.rs
• crates/ui-egui/tests/trust_signer.rs
• docs/roadmap.md

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

The document now detects signature presence separately from validating signature details. Automation and UI code use document accessors, and UI messages distinguish documents with reported signatures from documents with none. Roadmap notes also record candidate and progress updates.

Changes: Signature state and display

Layer / File(s)	Summary
Signature detection and accessors crates/engine/src/lib.rs, crates/sign/src/pdf.rs	The document caches signature presence and validated signature details separately. Signed-state detection includes populated signature fields and standalone document timestamps.
Cache invalidation and signed rewrites crates/engine/src/lib.rs	Edits, refreshes, and trust changes clear signature caches. The signed-rewrite guard computes signature presence when needed.
Automation and signature UI crates/automation/src/*, crates/ui-egui/src/*	Automation and UI code use document accessors. The banner and panel distinguish an empty validated signature list from a document that reports signatures.
Signature state and UI tests crates/engine/src/tests.rs, crates/ui-egui/tests/*	Tests cover deferred validation, cache reuse and invalidation, signed-state detection, and warnings for malformed signatures.

Roadmap status notes

Layer / File(s)	Summary
Candidate and progress records ROADMAP.md, docs/roadmap.md	Roadmap entries record candidate #1465 signature-status observations and Linux/Unix fallback-font discovery details.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Refactor

Sequence Diagram(s)

sequenceDiagram
  participant Canvas
  participant Document
  participant Banner as sign_ui::banner
  Canvas->>Document: is_signed()
  alt Document is signed
    Canvas->>Document: signatures()
    Document->>Document: validate and cache signature details
  else Document is not signed
    Canvas->>Document: validated_signatures()
  end
  Canvas->>Banner: pass signature data and signed status

Loading

Suggested reviewers: echelon

Merge Risk: ⚪ Minimal · up to 13a7c

No merge-blocking signature-state or display issue remains identified; the change is ready for normal merge checks.

Security architecture risk: 🟡 Moderate · up to 13a7c

The deferred-validation design generally preserves cache freshness, but signature detection no longer covers every structure recognized by validation. Unusual signed files can consequently be treated as unsigned, allowing rewrites that were previously refused. The exposure concerns document integrity during editing or saving, not signature forgery or newly granted system privileges.

Retained concerns

• Medium · security · inferred: The new signed-presence policy can return false for signatures that validation still recognizes. Direct dictionaries in AcroForm /Fields are skipped by form enumeration, and fields beyond its depth limit are omitted. The previous signature-list-based policy recognized these cases. Consequently, a crafted document without a separate timestamp can bypass signed-rewrite refusal and full-save refusal; before details are requested, its signature banner can also be absent. Loading validated details does not reconcile the independent presence cache.

Security review details

Security Blast Radius

• inferred — The affected boundary is the integrity policy for each supplied PDF and its saved output. A document producer can control the field structure that drives presence detection. Writing an affected output still requires an edit/save request and an allowed destination; no cross-document privilege gain is established.

Security Findings and Attack Paths

• inferred — An inline /Fields entry containing /FT /Sig, /T, and dictionary-valued /V is recognized as signed by the unchanged validation traversal but skipped by form enumeration. A sufficiently deep referenced hierarchy produces another omission. Without a separate timestamp, presence can cache false, permitting full-save serialization and suppressing the initial canvas signature banner. The writer rewrites object layout without independently enforcing signed-file refusal. This weakens preservation controls; it does not make rewritten signatures cryptographically valid.

Trust Boundaries and Controls

• observed — Structural presence is not a trust verdict. Validated details still use current document bytes and the session trust store through list_cached. Trust changes invalidate details, and edit permission checks remain ahead of mutation. These controls remain distinct from the weakened signed-presence guard.

Resilience and Maintainability Implications

• observed — The lifecycle test covers deferred validation, repeated-read reuse, signing after an unsigned presence check, trust invalidation, post-sign edits, incremental saves, and refused sanitization. It provides counterevidence against general cache-staleness failures, but does not establish coverage for inline fields or hierarchies beyond the form reader's limit.

Hardening Proposals

• proposed — Make structural presence a conservative superset of signature discovery, sharing traversal semantics or treating incomplete traversal as unsafe for rewriting. Preserve resource limits without interpreting skipped structures as unsigned, and exercise the invariant through both rewrite-capable edits and full saves.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 69.81% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 53 functions across 10 files. (2 skipped:…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title clearly and concisely describes the main change: deferring signature validation while preserving signed-file guards.
Description check		The description explains the behavior change, motivation, affected paths, issue reference, verification commands, test results, and performance context. It does not explicitly include the template's N…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Description check

Explanation

The description explains the behavior change, motivation, affected paths, issue reference, verification commands, test results, and performance context. It does not explicitly include the template's Never-crash, Provenance, Security, Parity and reach, or reviewer-focus sections, but the available information is sufficient and directly relevant.

Full details: Docstring Coverage

Explanation

Docstring coverage is 69.81% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 53 functions across 10 files. (2 skipped: 2 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

• Fix all pre-merge checks with AI

✨ Finishing Touches: 💡 1

🛠️ Fix failing CI checks: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}