Bug 2050896 - Add secure bug link to PGP email body - #2735
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The non-wrapped PGP path declares a boundary belonging to another MIME object, producing inconsistent MIME output.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a plaintext secure-bug link alongside PGP/MIME-encrypted bugmail.
Changes:
- Wraps encrypted PGP/MIME content in
multipart/mixedwith a plaintext link. - Adds URL and wrapping helpers.
- Adds tests for the new MIME structure.
File summaries
| File | Description |
|---|---|
extensions/SecureMail/Extension.pm |
Builds and wraps the PGP/MIME payload. |
t/903-securemail-link.t |
Tests link and nested encryption parts. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
dklawren
left a comment
dklawren
left a comment
There was a problem hiding this comment.
Code review — structural core verified correct against the pinned Email::MIME 1.954 (parts_set/fill_parts/walk_parts/create) plus an end-to-end run of _make_secure with a realistic multipart/alternative bugmail, replaying Bugzilla::Mailer's post-hook walk_parts and diffing against master.
The wrapped output is a well-formed multipart/mixed → (text/plain, multipart/encrypted; protocol="application/pgp-encrypted" → the two required PGP parts). The non-SECURE_ALL path (private-comment / whine / password mail) is byte-for-byte structurally identical to master. Boundary reuse is safe — the outer message inherits the original multipart/alternative boundary, which can never appear inside base64 PGP armour or the encryption-failure fallback text. No inverted conditions, null derefs, or dropped guards.
3 findings inline.
| _set_pgp_content_type($encrypted_part); | ||
|
|
||
| if ($sanitize_subject && $bug_id) { | ||
| _wrap_pgp_bugmail($email, $encrypted_part, _bug_url($bug_id)); |
There was a problem hiding this comment.
medium — PGP/MIME is no longer the top-level content type for secure bugmail.
RFC 3156 clients that only auto-detect multipart/encrypted at message level (GpgOL/Outlook, some webmail plugins) will stop decrypting and show two attachments (noname version part + encrypted.asc) instead of a body. Thunderbird 91+ decrypts nested parts but renders the "parts of this message are not encrypted" downgrade banner on every secure bugmail.
This is the deliberate trade-off of the PR, but it changes the shape of all fully-secure bugmail for all PGP users in order to fix a Gmail-only presentation problem. Worth confirming against the MUAs BMO's secure-mail users actually run before landing.
There was a problem hiding this comment.
Yeah, this makes the tradeoff unacceptable for this change. Adding some preference that controls this seems like more work than it's worth. A Firefox extension that adds a link would probably be easier (but only benefits those who install it). I've actually installed Flowcrypt (Gmail GPG plugin) again. :-/
I had hoped we could expose a Gmail-friendly cleartext link while preserving normal behavior for existing PGP/MIME clients, but that does not appear possible within the expected MIME structure. I don’t want to change secure-mail behavior for every PGP user, or risk degraded handling in clients such as Outlook/GpgOL, solely to improve Gmail’s presentation.
I’m going to abandon this approach and close the PR. Thank you for the detailed compatibility analysis.
| # multipart/mixed message with a plaintext link so clients which do not | ||
| # expose the OpenPGP armour comment (notably Gmail) provide a usable | ||
| # route back to the bug. | ||
| my $encrypted_part = Email::MIME->create( |
There was a problem hiding this comment.
low — $encrypted_part is built unconditionally, then discarded on the else path.
Email::MIME->create(parts => \@new_parts) runs a full parts_set (serialize every part) plus fill_parts (re-parse the result) — for a bug with large attachments that is a multi-MB round trip — and the object is thrown away for every private-comment notification, whine mail, password mail, and X-Bugzilla-Encrypt mail.
Move the create + _set_pgp_content_type($encrypted_part) inside the if ($sanitize_subject && $bug_id) branch.
| $unwrapped_email->parts_set([$control_part, $data_part]); | ||
| Bugzilla::Extension::SecureMail::_set_pgp_content_type($unwrapped_email); | ||
| my ($boundary) = $unwrapped_email->header('Content-Type') =~ /boundary="([^"]+)"/; | ||
| is($boundary, $unwrapped_email->{ct}{attributes}{boundary}, |
There was a problem hiding this comment.
low — this assertion cannot fail.
is($boundary, $unwrapped_email->{ct}{attributes}{boundary}) reads the header back and compares it to the exact value _set_pgp_content_type just used to write it, so it passes for any implementation.
More importantly, the test file never calls _make_secure, so the branch this PR actually adds ($sanitize_subject && $bug_id) and its interaction with Bugzilla::Mailer's post-hook walk_parts are untested — a regression that broke branch selection or the else path would ship green. One test that calls _make_secure with a stubbed _tct_encrypt and asserts the resulting top-level content type for $sanitize_subject 1 vs 0 would cover the real logic.
Adds a plaintext link to the secure bug.