Skip to content

Digital Signature and Certificate verification#21247

Open
beurdouche wants to merge 10 commits into
mozilla:masterfrom
beurdouche:master
Open

Digital Signature and Certificate verification#21247
beurdouche wants to merge 10 commits into
mozilla:masterfrom
beurdouche:master

Conversation

@beurdouche

@beurdouche beurdouche commented May 9, 2026

Copy link
Copy Markdown
Member

No description provided.

@beurdouche beurdouche requested a review from a team as a code owner May 9, 2026 17:08
@codecov-commenter

codecov-commenter commented May 9, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 25.11962% with 313 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.23%. Comparing base (e86e9d9) to head (4a77dd4).
⚠️ Report is 330 commits behind head on master.

Files with missing lines Patch % Lines
web/digital_signature_properties_manager.js 1.98% 297 Missing ⚠️
src/core/document.js 89.77% 9 Missing ⚠️
web/app.js 70.58% 5 Missing ⚠️
src/display/api.js 71.42% 2 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           master   #21247       +/-   ##
===========================================
+ Coverage   54.57%   89.23%   +34.66%     
===========================================
  Files         216      261       +45     
  Lines       58860    66485     +7625     
===========================================
+ Hits        32120    59331    +27211     
+ Misses      26740     7154    -19586
Flag Coverage Δ
browsertest 66.60% <5.15%> (?)
fonttest 9.02% <ø> (?)
integrationtest 68.49% <5.74%> (?)
unittest 57.26% <88.65%> (?)
unittestcli 56.22% <88.65%> (+1.65%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

calixteman
calixteman reviewed May 13, 2026
View reviewed changes
Comment thread src/core/document.js Outdated
calixteman
calixteman reviewed May 13, 2026
View reviewed changes
Comment thread web/firefoxcom.js Outdated
calixteman
calixteman reviewed May 13, 2026
View reviewed changes
Comment thread web/signature_properties_manager.js Outdated
calixteman
calixteman reviewed May 13, 2026
View reviewed changes
Comment thread web/signature_properties_manager.js Outdated
calixteman
calixteman reviewed May 13, 2026
View reviewed changes
Comment thread web/digital_signature_properties_manager.js
calixteman
calixteman reviewed May 13, 2026
View reviewed changes
Comment thread web/signature_properties_manager.js Outdated
calixteman
calixteman reviewed May 13, 2026
View reviewed changes
Comment thread web/signature_properties_manager.js Outdated
calixteman
calixteman reviewed May 13, 2026
View reviewed changes
Comment thread web/signature_properties_manager.js Outdated
calixteman
calixteman reviewed May 13, 2026
View reviewed changes
Comment thread web/digital_signature_properties_manager.js
calixteman
calixteman reviewed May 13, 2026
View reviewed changes
Comment thread test/pdfs/sig_corpus/generate.py Outdated
calixteman
calixteman reviewed May 13, 2026
View reviewed changes
Comment thread test/pdfs/sig_corpus/README.md Outdated
calixteman
calixteman reviewed May 13, 2026
View reviewed changes
Comment thread test/pdfs/sig_corpus/README.md Outdated
calixteman
calixteman reviewed May 13, 2026
View reviewed changes
Comment thread web/digital_signature_properties.css
calixteman
calixteman reviewed May 13, 2026
View reviewed changes
Comment thread web/digital_signature_properties.css
calixteman
calixteman reviewed May 13, 2026
View reviewed changes
Comment thread web/signature_properties.css Outdated
calixteman
calixteman reviewed May 13, 2026
View reviewed changes
Comment thread web/signature_properties.css Outdated
calixteman
calixteman reviewed May 13, 2026
View reviewed changes
Comment thread web/signature_properties.css Outdated
calixteman
calixteman reviewed May 13, 2026
View reviewed changes
Comment thread web/signature_properties.css Outdated
calixteman
calixteman reviewed May 13, 2026
View reviewed changes
Comment thread web/digital_signature_properties_manager.js Outdated
calixteman
calixteman reviewed May 13, 2026
View reviewed changes
Comment thread web/firefoxcom.js
calixteman
calixteman reviewed May 13, 2026
View reviewed changes
Comment thread web/signature_properties_manager.js Outdated
beurdouche added 6 commits May 13, 2026 20:44
@beurdouche
Attend to review comments
6a8a397
@beurdouche
Remove the /sig reason which is user provided
85b01ac
@beurdouche
Make Signature Properties icons HCM-aware
25c1b3a 
Switch the panel row icons and the toolbar button state badge from
`background-image:` (full-colour SVG, never themes) to `mask-image:` +
`background-color:`. The colour comes from new `--sig-icon-*` tint
vars that resolve via `light-dark()` in normal mode and collapse to
`CanvasText` in `forced-colors: active`.

- Rewrite `toolbarButton-signaturePropertiesVerified/Warn/Error.svg`
  as mono filled-disc silhouettes with a transparent glyph cutout
  (via `<mask>`).
- New `signature-properties-row-check.svg` — outline check used by the
  default/fine row states (supersedes
  `signature-properties-status-check.svg`).
- Drop `--signatureProperties-statusCheck-icon`, add
  `--signatureProperties-rowCheck-icon`.
- Add `--sig-icon-{default,warn,error,verified}` tint vars with
  forced-colors fallbacks; toolbar + row rules both reference them.
@beurdouche
Rename Signature Properties files to digital_signature_*
e31e013 
Per Calixte's review feedback on web/signature_properties.css:1
(use "digital signature" terminology to avoid confusion with the
handwritten-signature feature), rename:

- web/signature_properties.css            -> web/digital_signature_properties.css
- web/signature_properties_manager.js     -> web/digital_signature_properties_manager.js

Updates references in web/viewer.css (@import), web/viewer.html
(import map), gulpfile.mjs (import alias) and web/app.js (import).
The exported SignaturePropertiesManager class name is left unchanged.
@beurdouche
Attend to review comments
aee9c6a 
- src/core/document.js: short comment explaining why `stream.end` is
  used (raw-buffer end matches the coordinate system of /ByteRange
  offsets, which `stream.length` does not after `moveStart`). Resolves
  the follow-up question from @Snuffleupagus on the document.js:2079
  thread.
- test/unit/document_spec.js: update the two assertions that still
  expected an empty array for the "no signatures" case — the getter
  now returns `null` (per @Snuffleupagus on display/api.js:1082), so
  switch to `.toBeNull()`. These were the unit-test failures in CI
  run 27538005889.
@beurdouche
Improve robustness of the digital signature verification changes
cb65325 
Five hardening fixes from a stack review:

1. `/ByteRange` bounds validation in `#parseSignatureDict` — reject
   signatures that lie about the byte spans they cover (`a == 0`,
   `b > 0`, `d >= 0`, `a + b <= c`, `c + d <= fileLength`), and
   compute `coversWholeDocument` from the trailer-gap rather than
   the absolute last-signed-byte position.

2. Split signature metadata from byte payload across the worker
   boundary. The `signatures` getter now ships small metadata only;
   the PKCS#7 blob and signed-data byte spans live in a worker-side
   `#signatureData` Map and are fetched on demand via a new
   `getSignatureData(id)` worker RPC. The viewer pulls bytes one
   signature at a time, immediately before passing them to the
   verifier, so they don't sit in main-thread memory for the
   document's lifetime.

3. Per-load `Symbol` token in `loadFromDocument` / `reset()`. In-flight
   `#verify()` promises capture the token at start and bail on
   resolve if it has changed, so a verification for doc A can't
   write into doc B's results map after a fast doc-switch.

4. `expirationDateForCert` no longer falls back to a future leaf
   `notAfter` when the chain doesn't contain a past date. Showing
   "Expired (Jan 1, 2027)" was misleading; the caller now renders
   the dateless `Certificate: Expired` label instead.

5. Test update: `pkcs7` / `data` are no longer attached to the
   metadata array, so the unit test fetches bytes via the new
   `getSignatureData(id)` helper. Pad the test stream so
   ByteRanges fit within `stream.end`.
bcolsson
bcolsson requested changes Jun 15, 2026
View reviewed changes
Comment thread l10n/en-US/viewer.ftl Outdated
Comment thread l10n/en-US/viewer.ftl Outdated
Comment thread l10n/en-US/viewer.ftl Outdated
@beurdouche
Attend to localization review feedback
4a77dd4 
Three follow-ups from @bcolsson on viewer.ftl:

- Sentence case: `Digital Signature properties` →
  `Digital signature properties` everywhere (button title, aria-label,
  label, group comment, test corpus README/generator, in-source
  doc comments).
- The status row only ever shows one of three distinct English
  strings, so collapse `-status-untrusted` / `-expired` / `-revoked`
  into the existing `-status-verified` (the cert chain issue lives
  on the cert row below). `STATUS_INFO` in the manager maps all
  cryptographically-verified statuses to the same id, and a group
  comment in viewer.ftl explains the design.
- Rewrite the comment for `pdfjs-...-reason`: the variable holds an
  NSS error message for an invalid signature, not text from the
  PDF's /Sig dictionary.

Also rename every Fluent id from `pdfjs-signature-properties-...` to
`pdfjs-digital-signature-properties-...` so the id namespace matches
the filename and the panel name.
bcolsson
bcolsson approved these changes Jun 15, 2026
View reviewed changes

@bcolsson bcolsson left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks good, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@flodolo flodolo flodolo approved these changes

@bcolsson bcolsson bcolsson approved these changes

@Snuffleupagus Snuffleupagus Awaiting requested review from Snuffleupagus

@calixteman calixteman Awaiting requested review from calixteman

Assignees

No one assigned

Labels

core feature viewer

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Implement digital signature validation

8 participants

@beurdouche @codecov-commenter @flodolo @marco-c @Snuffleupagus @calixteman @bcolsson @timvandermeij