Digital Signature and Certificate verification#21247
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #21247 +/- ##
===========================================
+ Coverage 54.57% 65.89% +11.32%
===========================================
Files 216 250 +34
Lines 58860 64603 +5743
===========================================
+ Hits 32120 42572 +10452
+ Misses 26740 22031 -4709
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
They were consulted and they didn't agree with this approach (the main reason being what you said here), so it's not going to be enabled in Firefox but only in Enterprise Firefox. |
| const refKey = fieldRef instanceof Ref ? fieldRef.toString() : "inline"; | ||
| const id = `${refKey}:${a}-${b}-${c}-${d}`; | ||
|
|
||
| const fileLength = stream.end || 0; |
There was a problem hiding this comment.
I don't know if it's correct in general.
@Snuffleupagus could it be wrong if the pdf is from a ranged request ?
There was a problem hiding this comment.
Given that PDFDocument is initialized either with a Stream or a ChunkedStream, where the latter extends the former, this part ought to be fine.
However, looking at this now I wonder why stream.end is being used and if that's actually intended?
What is fileLength actually supposed to represent here, is it either:
-
The length of the entire raw file passed to PDF.js?
If this is the answer, then I suppose thatstream.endis appropriate (although quite unexpected, see below). -
The length of the actual PDF-data contained within the raw file?
That can be shorter if there's "garbage" in the file before the%PDF-header, which marks the start of the actual PDF-data (in which casestream.moveStart()will update the start-position of the stream).Given that all "normal" data in a PDF document is always referenced from the
%PDF-header position, naively I'd thus expect that you actually wantstream.lengthhere (which is fine as long as you're only dealing withStreaminstances).
| banner.className = `sigBanner ${severity}`; | ||
| banner.setAttribute( | ||
| "data-l10n-id", | ||
| `pdfjs-signature-properties-banner-${worst}` |
There was a problem hiding this comment.
Can you avoid generating l10n ids, similarly to what we did in #21263 ?
| for (const sig of this.#signatures) { | ||
| if (sig.parentId) { | ||
| if (!byParent.has(sig.parentId)) { | ||
| byParent.set(sig.parentId, []); |
There was a problem hiding this comment.
nit: getOrInsertComputed
| #render() { | ||
| const list = this.#appConfig.signaturePropertiesList; | ||
| const banner = this.#appConfig.signaturePropertiesBanner; | ||
| list.replaceChildren(); |
There was a problem hiding this comment.
We can probably use a documentFragment and use list.replaceChildren(fragment).
| this.#updateButtonState(); | ||
| } | ||
|
|
||
| #updateButtonState() { |
There was a problem hiding this comment.
This function looks a bit similar to bannerStateForResults, maybe you could factorize a bit.
| from pathlib import Path | ||
|
|
||
| CORPUS_DIR = Path(__file__).resolve().parent | ||
| FIREFOX_DIR = Path("/opt/mozilla/firefox") |
There was a problem hiding this comment.
It's a bad idea to have this hardcoded path.
| `security/manager/tools/pycms.py` and reuses its vendored Python | ||
| modules under `third_party/python/{ecdsa,rsa,pyasn1,pyasn1_modules,six}`. | ||
| 2. The Firefox build at | ||
| `/opt/mozilla/firefox/obj-aarch64-apple-darwin25.4.0/dist/Nightly.app` |
There was a problem hiding this comment.
Can we have something more generic ?
| Launch the dev Firefox: | ||
|
|
||
| ```sh | ||
| /opt/mozilla/firefox/obj-aarch64-apple-darwin25.4.0/dist/Nightly.app/Contents/MacOS/firefox \ |
There was a problem hiding this comment.
Can we something more generic ?
| &.status--revoked::before, | ||
| &.cert--trusted::before { | ||
| background-color: transparent; | ||
| background-image: url("data:image/svg+xml;utf8,<svg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 16 16'><circle cx='8' cy='8' r='7' fill='white' stroke='%23969696' stroke-width='1'/><path fill='none' stroke='%23969696' stroke-width='1.8' stroke-linecap='round' stroke-linejoin='round' d='M5 8.4l2.4 2.4L11.6 6'/></svg>"); |
There was a problem hiding this comment.
It should probably live in svg file and again take care about the colors depending on the theme.
| * class is set in #render. */ | ||
| .sigCard--top-allfine > .sigCard__row.status--verified::before, | ||
| .sigCard--top-allfine > .sigCard__row.cert--trusted::before { | ||
| background-image: url("data:image/svg+xml;utf8,<svg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 16 16'><circle cx='8' cy='8' r='7' fill='%231d8e3d'/><path fill='none' stroke='white' stroke-width='2' stroke-linecap='round' stroke-linejoin='round' d='M4.5 8.4l2.4 2.4L11.6 6'/></svg>"); |
| background: var(--field-bg-color, white); | ||
| } | ||
|
|
||
| .sigCard__signer { |
There was a problem hiding this comment.
You can probably have class "signer" and and use nested css instead.
The same comment applied for other similar pattern (foo__bar).
| } | ||
| } | ||
|
|
||
| @keyframes signaturePropertiesDots { |
There was a problem hiding this comment.
Maybe I'm wrong, but I've the feeling we can do better here...
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.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7 participants
No description provided.