Repository navigation
M9: sign every page with one digital signature, repeat Fill & Sign signatures, trust Windows roots - #2087
M9: sign every page with one digital signature, repeat Fill & Sign signatures, trust Windows roots#2087mendax-iitm wants to merge 9 commits into
Conversation
…ignature on pages - pdfcraft-sign: SignOptions::also adds a widget per extra page to one signature field (one signature, one PIN), with tests that fail without the change - Sign dialog: 'Show on other pages too' with a page range; Refresh button lists a USB token or smart card plugged in after the dialog opened - Fill & Sign: 'Repeat on pages' places the last signature or initials in the same place on a page range as one undo step - automation: sign_document also_pages; parity note and translations
Windows root certificates (where a DSC token driver installs its CA) are kept apart from the user's trusted list and never saved. Adds an end-to-end test for signing one signature onto every page through the Sign dialog.
|
Tested on Windows with a real DSC USB token [token model, e.g. ePass2003] on Windows11, using an x64 MSI built from this branch:
This covers the two items the PR description lists as not yet confirmed by hand on Windows (the multi-page signature and the root-store trust). |
|
Hey @echelon Can you have a look on this PR and run workflow, made some contribution which i felt is missing from pdfcraft. |
# Conflicts: # ATTRIBUTION.toml # contributors/people.toml # crates/automation/src/tools.rs # crates/engine/src/lib.rs # crates/engine/src/tests.rs # crates/sign/tests/pdf.rs # crates/ui-egui/src/i18n/ar.tsv # crates/ui-egui/src/i18n/bg.tsv # crates/ui-egui/src/i18n/cs.tsv # crates/ui-egui/src/i18n/de.tsv # crates/ui-egui/src/i18n/el.tsv # crates/ui-egui/src/i18n/es.tsv # crates/ui-egui/src/i18n/fr.tsv # crates/ui-egui/src/i18n/hu.tsv # crates/ui-egui/src/i18n/it.tsv # crates/ui-egui/src/i18n/ja.tsv # crates/ui-egui/src/i18n/kk.tsv # crates/ui-egui/src/i18n/lt.tsv # crates/ui-egui/src/i18n/pl.tsv # crates/ui-egui/src/i18n/pt-br.tsv # crates/ui-egui/src/i18n/ru.tsv # crates/ui-egui/src/i18n/sl.tsv # crates/ui-egui/src/i18n/te.tsv # crates/ui-egui/src/i18n/tr.tsv # crates/ui-egui/src/i18n/uk.tsv # crates/ui-egui/src/i18n/zh-hans.tsv # crates/ui-egui/src/i18n/zh-hant.tsv # crates/ui-egui/src/sign_ui.rs
Upstream added vi.tsv after this branch started; its coverage tests require every UI label, including the four this PR adds.
# Conflicts: # ATTRIBUTION.toml # crates/ui-egui/src/i18n/bg.tsv # crates/ui-egui/src/i18n/cs.tsv # crates/ui-egui/src/i18n/de.tsv # crates/ui-egui/src/i18n/fr.tsv # crates/ui-egui/src/i18n/hu.tsv # crates/ui-egui/src/i18n/it.tsv # crates/ui-egui/src/i18n/ja.tsv # crates/ui-egui/src/i18n/kk.tsv # crates/ui-egui/src/i18n/lt.tsv # crates/ui-egui/src/i18n/pt-br.tsv # crates/ui-egui/src/i18n/ru.tsv # crates/ui-egui/src/i18n/sl.tsv # crates/ui-egui/src/i18n/tr.tsv # crates/ui-egui/src/i18n/uk.tsv # crates/ui-egui/src/i18n/zh-hans.tsv
|
Hey @marcoarnulfo, @jonathanwxh-cell, @Silvertree2010, @cyfung1031, @bartalos86, @jcl-bit Can you have a look on this PR and run workflow, made some contribution which i felt is missing from pdfcraft. |
jonathanwxh-cell
left a comment
There was a problem hiding this comment.
Reviewed at commit b649ed3d28ef17de419064ab22e9789dcfcfbfa9 on Windows (x86_64-pc-windows-msvc).
(Note: as a community contributor I don't have repository maintainer permissions to click "Approve and run" on the GitHub Actions workflow queue, so I ran the verification suite locally on Windows to help move this review along.)
Local Verification Summary (Windows)
git diff --check upstream/main..HEAD: cleancargo fmt --all -- --check: cleancargo clippy -p pdfcraft-sign -p pdfcraft-engine -p pdfcraft-automation -p pdfcraft-ui-egui -p pdfcraft --all-targets -- -D warnings: 0 warnings / 0 errors (including the#[cfg(windows)]rustls_cng::store::CertStore::openpath incrates/sign/src/windows.rs)cargo xtask layers&cargo xtask assets: passedcargo test -p pdfcraft-sign: 118 passed (includingone_signature_shows_on_several_pages,other_pages_need_a_visible_new_signature_on_existing_pages,windows_store_enumeration_and_missing_identity, andstore_keys)cargo test -p pdfcraft-engine --lib:system_roots_validate_signatures_but_are_not_the_users_listand all signing/trust tests passedcargo test -p pdfcraft-ui-egui --lib fill_sign&cargo test -p pdfcraft-ui-egui --test signing: all 15 tests passed (includingsignatures_repeat_in_the_same_place_on_other_pagesanda_digital_signature_shows_on_every_page_in_the_same_place)
Findings
1. [Medium / Correctness] Multi-page counter-signatures (also non-empty) are classified as "form fill" by classify_later_changes (crates/sign/src/pdf.rs)
When opts.also is non-empty (placed.len() > 1), sign_inner (crates/sign/src/pdf.rs:1927-1938) creates a separated parent/child hierarchy per ISO 32000-2 §12.7 / §12.8:
- The parent field dictionary
fr(<< /FT /Sig /T ... /V ... /Kids [...] >>, without/Subtype /Widget) is added to/AcroForm /Fields. - Each child widget dictionary
wr(<< /Type /Annot /Subtype /Widget /Rect ... /P ... /F 132 /Parent fr /AP ... >>, without a direct/FT /Sig) is added to its page's/Annots.
However, the post-signing modification classifier in crates/sign/src/pdf.rs still assumes every newly added signature field/widget is a merged single dictionary carrying both /Subtype /Widget and /FT /Sig:
added_annots(crates/sign/src/pdf.rs:1444-1460): checks(d.name(b"Subtype") == Some(b"Widget"), d.name(b"FT") == Some(b"Sig"))directly onrwithout resolvingd.get(b"Parent")for split widget children.- When
page_change_kind(line 1596) orchange_kindfor/Type /Page(line 1633) inspects a page's added/Annots, each child widgetwrhas/Subtype /Widgetand/Parent fr(no direct/FT /Sig), soadded_annotsreturnskind::FORM_FILLinstead ofkind::SIGNATURE.
- When
acroform_kind(crates/sign/src/pdf.rs:1486-1488): callsadded_annots(doc, &added)on new entries in/AcroForm /Fields. Because the parent fieldfrhas/FT /Sigand/Kids(no/Subtype /Widget),added_annotsreturnskind::COMMENTS, causingacroform_kindto returnkind::FORM_FILLinstead ofkind::SIGNATURE.change_kind(crates/sign/src/pdf.rs:1641-1643):WhenSome(b"Widget") => return if d.name(b"FT") == Some(b"Sig") && signature_ok { kind::SIGNATURE } else { kind::FORM_FILL },
classify_later_changesiterates over the newly added child widget objectswr,d.name(b"FT")isNone(it lives on/Parent), so eachwris also classified askind::FORM_FILLinstead ofkind::SIGNATURE.
Effect: If a document already has a signature and is then counter-signed with a multi-page signature (also non-empty), verify() on the earlier signature reports "form fill" alongside "signature" (Modification::Allowed(vec!["signature", "form fill"]), or Modification::Disallowed(vec!["form fill"]) if /P 1), whereas a single-page counter-signature (also empty) reports only "signature".
2. [Low / Edge Case] Cross-page rectangle mapping when pages have different /Rotate values
- In
crates/ui-egui/src/fill_sign.rs:487-501(same_place/same_rect), the normalized coordinates(nx, ny, nw, nh)are computed directly infrom.crop(raw PDF user space) and projected ontoto.cropwithout accounting forfrom.rotationvsto.rotation. - In
crates/automation/src/signing.rs:226-237, the center point is mapped throughto.view_to_user(ux, uy)(which accounts forto.rotation), whilehwandhhare scaled fromfrom's raw user-space width/height ((rect[2] - rect[0]).abs() * 0.5 * sx) and applied directly into's user space. - For homogeneous documents (all pages sharing the same rotation) both behave as expected; on mixed-rotation PDFs (e.g. a portrait page at
/Rotate 0and a landscape page at/Rotate 90), the repeated box placement/aspect ratio will shift.
3. [Nit / UX] Default to page when toggling "Pages from" in Fill & Sign repeat bar (crates/ui-egui/src/fill_sign.rs:620-626)
app.fill_repeat initializes PageRange::default() (from: 1, to: 1). Unlike PageRange::ui in crates/ui-egui/src/marks_ui.rs:56-58 (which clamps/initializes range.to to page_count when opening), switching the Fill & Sign repeat radio from "All pages" to "Pages from" starts with 1 to 1 rather than 1 to count until the user increments the second DragValue.
4. [Nit / Follow-up] Canvas signature status badges on multi-page signatures (crates/sign/src/pdf.rs:578-584)
SignatureInfo stores page and rect from f.widgets.first(), so crates/ui-egui/src/canvas.rs:332-344 renders the on-canvas status badge only on the first widget's page (though all pages render the signature appearance stream and are clickable via form_fields). Keeping SignatureInfo's single (page, rect) for API simplicity makes sense here, just noting in case multi-widget badge rendering is desired later.
Overall the architecture, sig_fields /Kids traversal, user_trust vs system_trust separation in Session, and rustls-cng ROOT store integration are very clean and well-tested.
Kid widgets of a split signature field carry /FT /Sig only through /Parent, and the parent field is not a widget, so counter-signing with a multi-page signature reported "form fill" on earlier signatures. Resolve /FT through /Parent (guarded against relabelled parents) and judge added /AcroForm /Fields as fields. Fill & Sign repeat range now opens as the whole document, like PageRange::ui. Review feedback from @jonathanwxh-cell on storytold#2087.
What this changes
Three signing additions, aimed at people who sign every page of a document, as is common with Indian DSC USB tokens (ePass2003, ProxKey, mToken) and as Foxit's paid tier does.
/Kids, each with/Parent), and every widget shares the same/AP /Nstream. Each extra widget sits in the same place relative to its page's crop box and keeps the size of the first one. Without the option, the single-page output is byte-for-byte the same merged field-and-widget as before.pdfcraft-sign:SignOptions::also: Vec<(usize, [f64; 4])>. It skips repeated pages and the signed page itself, refuses pages that don't exist, refuses an invisible signature, and refuses an existing field.automation:sign_documenttakesalso_pages(1-based).Edit::Batch, so one Undo removes them.trusted_certificates()still returns only the user's list, which is what gets persisted, so OS roots are never saved. The Sign dialog's ID chooser also gets a Refresh button, which lists a token that was plugged in after the dialog opened.How you verified it
cargo fmt --check-D warningsonpdfcraft-sign,pdfcraft-engine,pdfcraft-ui-egui,pdfcraft-automation,pdfcraft(macOS)cargo test --no-fail-fast -p pdfcraft-sign -p pdfcraft-engine -p pdfcraft-ui-egui -p pdfcraft-automationon macOS after rebasing ontomain, with 0 failures.cargo xtask layersandcargo xtask assetscfg(windows)code compiles). Tested by hand with a real DSC USB token through the Windows store: one PIN prompt, the stamp on every page of the range, a single signature listed, and the signature shown as valid against the Windows trusted roots. Fill & Sign repeat was checked by hand on macOS and on Windows.New tests, each seen failing with its change reverted:
crates/sign/tests/pdf.rs::one_signature_shows_on_several_pages: one signed field, a widget on each of 3 pages (including a larger page), all with/Parentand the appearance.crates/sign/tests/pdf.rs::other_pages_need_a_visible_new_signature_on_existing_pages: no page 4, an invisible signature, or an existing field is an error.crates/ui-egui/tests/signing.rs::a_digital_signature_shows_on_every_page_in_the_same_place: end to end through the real Sign dialog.crates/ui-egui/src/fill_sign.rs::signatures_repeat_in_the_same_place_on_other_pages: mapping across crop boxes, skipping the source page and missing pages, and a degenerate crop box.crates/engine/src/tests.rs::system_roots_validate_signatures_but_are_not_the_users_listNever-crash
unwrap/expect/panic!/unreachable!/todo!outside testsget. Crop-box mapping guards zero-width boxes and non-finite input. The[0, 1]/i + 2indices into[f32; 4]are constants.unsafeProvenance
ATTRIBUTION.tomlare re-recorded; the added strings are contributor-original, as each entry states.Security
--rootconfinement unchanged (also_pagesis page numbers only)rustls-cngdependency.Parity and reach
parity/acrobat-features.toml:sign.visible-signaturenotes and evidence, andsign.os-trustmoved topartialwith what is still missingsign_documentalso_pagescrates/sign/README.mdcontributors/people.toml: my own credit entry, committed by me (AGENTS.md, Contributor credits)Anything a reviewer should look at first
Certificate::signing_problemstill requires a document-signing purpose, so a TLS-only certificate can't validate as a document signer. Happy to put it behind a preference if you'd rather.sign::windows::trusted_roots) has no automated test, because CI has no store fixtures. It was checked by hand with a real token.tl!literal to be translated. They're machine-quality and worth a native speaker's look.