Skip to content

Bug 2050896 - Add secure bug link to PGP email body #2735

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
aerickson wants to merge 3 commits into from
+132 −11
Closed

Bug 2050896 - Add secure bug link to PGP email body #2735

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
20c4a66
Bug 2050896 - Add secure bug link to PGP email body
aerickson Sep 3, 2026
2b2165f
Bug 2050896 - Preserve PGP MIME boundaries
aerickson Sep 3, 2026
04f5854
Bug 2050896 - Satisfy secure mail test lint
aerickson Sep 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 51 additions & 11 deletions extensions/SecureMail/Extension.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,15 +499,26 @@ sub _make_secure {
body => _tct_encrypt($tct, $to_encrypt, $bug_id)
),
);
$email->parts_set(\@new_parts);
my $new_boundary = $email->{ct}{attributes}{boundary};

# Redo the old content type header with the new boundaries
# and other information needed for PGP
$email->header_set("Content-Type",
"multipart/encrypted; "
. "protocol=\"application/pgp-encrypted\"; "
. "boundary=\"$new_boundary\"");
# Keep the PGP/MIME payload as a standards-compliant two-part
# multipart/encrypted message. For fully secure bugmail, wrap it in a
# 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(

@dklawren dklawren Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

attributes => {content_type => 'multipart/encrypted'},
parts => \@new_parts,
);
_set_pgp_content_type($encrypted_part);

if ($sanitize_subject && $bug_id) {
_wrap_pgp_bugmail($email, $encrypted_part, _bug_url($bug_id));

@dklawren dklawren Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@aerickson aerickson Sep 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

}
else {
# Preserve the original top-level PGP/MIME structure for all other
# encrypted mail, such as private-comment notifications.
$email->parts_set(\@new_parts);
_set_pgp_content_type($email);
}
}
else {
_fix_encoding($email);
Expand Down Expand Up @@ -597,8 +608,7 @@ sub _make_secure {
sub _tct_encrypt {
my ($tct, $text, $bug_id) = @_;

my $comment = Bugzilla->localconfig->urlbase
. ($bug_id ? 'show_bug.cgi?id=' . $bug_id : '');
my $comment = _bug_url($bug_id);
my $encrypted;
my $ok = eval { $encrypted = $tct->encrypt($text, $comment)->get; 1 };
if (!$ok) {
Expand All @@ -614,6 +624,36 @@ sub _tct_encrypt {
return $encrypted;
}

sub _bug_url {
my ($bug_id) = @_;
return Bugzilla->localconfig->urlbase
. ($bug_id ? 'show_bug.cgi?id=' . $bug_id : '');
}

sub _wrap_pgp_bugmail {
my ($email, $encrypted_part, $bug_url) = @_;

my $link_part = Email::MIME->create(
attributes => {
content_type => 'text/plain',
charset => 'UTF-8',
encoding => 'quoted-printable',
},
body_str => "View this bug: $bug_url\n",
);
$email->parts_set([$link_part, $encrypted_part]);
$email->content_type_set('multipart/mixed');
}

sub _set_pgp_content_type {
my ($email) = @_;
my $boundary = $email->{ct}{attributes}{boundary};
$email->header_set("Content-Type",
"multipart/encrypted; "
. "protocol=\"application/pgp-encrypted\"; "
. "boundary=\"$boundary\"");
}

# Insert the subject into the part's body, as the subject of the message will
# be sanitized.
# XXX this incorrectly assumes all parts of the message are the body
Expand Down
81 changes: 81 additions & 0 deletions t/903-securemail-link.t
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at http://mozilla.org/MPL/2.0/.

package main;

use strict;
use warnings;
use 5.10.1;
use lib qw(. lib local/lib/perl5 extensions/SecureMail/lib);

use Test::More;
use Email::MIME;

BEGIN {
*Bugzilla::Extension::SecureMail::NAME = sub { 1 };
}

my $extension = './extensions/SecureMail/Extension.pm';
require $extension;

my $email = Email::MIME->create(
attributes => {content_type => 'multipart/alternative'},
parts => [],
);
my $control_part = Email::MIME->create(
attributes => {
content_type => 'application/pgp-encrypted',
encoding => '7bit',
},
body => "Version: 1\n",
);
my $data_part = Email::MIME->create(
attributes => {
content_type => 'application/octet-stream',
encoding => '7bit',
},
body => 'encrypted data',
);
my $encrypted_part = Email::MIME->create(
attributes => {content_type => 'multipart/encrypted'},
parts => [$control_part, $data_part],
);

Bugzilla::Extension::SecureMail::_wrap_pgp_bugmail(
$email,
$encrypted_part,
'https://bugzilla.example/show_bug.cgi?id=123',
);

like($email->content_type, qr{\Amultipart/mixed(?:;|\z)},
'outer message is multipart/mixed');
my @outer_parts = $email->parts;
is(scalar @outer_parts, 2, 'outer message contains the link and encrypted message');
like($outer_parts[0]->content_type, qr{\Atext/plain(?:;|\z)},
'first part is plaintext');
like($outer_parts[0]->body_str,
qr{\AView this bug: https://bugzilla\.example/show_bug\.cgi\?id=123\r?\n\z},
'plaintext part contains the bug URL');
like($outer_parts[1]->content_type, qr{\Amultipart/encrypted(?:;|\z)},
'second part is the PGP/MIME message');
my @encrypted_parts = $outer_parts[1]->parts;
is(scalar @encrypted_parts, 2, 'PGP/MIME message retains its two required parts');
is($encrypted_parts[0]->content_type, 'application/pgp-encrypted',
'first PGP/MIME part is the control information');
is($encrypted_parts[1]->content_type, 'application/octet-stream',
'second PGP/MIME part is encrypted data');

my $unwrapped_email = Email::MIME->create(
attributes => {content_type => 'multipart/alternative'},
parts => [],
);
$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},

@dklawren dklawren Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

'unwrapped PGP/MIME header declares its own generated boundary');
like($unwrapped_email->as_string, qr/--\Q$boundary\E/,
'unwrapped PGP/MIME body uses its declared boundary');

done_testing;