829358 - Changing the name of a private attachment in an unhidden bug results in the name change being sent unencrypted

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, defect)

Description

[Andrew McCreight [:mccr8]]

I think that if a attachment that is marked "private (only visible to core-security)" (that I can view) gets its description changed, in a non-private bug, then the old and new description are sent out in plain text in bug mail. I noticed this during the stream of bug bounty updates that happened today, and I think those are all marked private, but I could see the change made in plain text.

Comment 1

[Gervase Markham [:gerv]]

This seems like a Bugzilla bug, not a SecureMail bug, because it relates to the "insidergroup" feature.

Gerv

Comment 2

[Gervase Markham [:gerv]]

Oh, I see. No, I'm wrong. Obviously you should see the change, as you are in the group! The issue is that the mail is not encrypted.

Gerv

Comment 3

[Andrew McCreight [:mccr8]]

Yeah, sorry, I should have been more explicit about that.

Comment 4

[David Lawrence [:dkl]]

Sorry for sitting on this one so long. I finally see why this is failing and am working on a solution.

Basically the SecureMail code is doing the following:
                
                # Encrypt if updating a private attachment without a comment
                if ($email->header('X-Bugzilla-Changed-Fields')
                    && $email->header('X-Bugzilla-Changed-Fields') =~ /Attachment #(\d+)/)
                {
                    my $attachment = Bugzilla::Attachment->new($1);
                    if ($attachment && $attachment->isprivate) {
                        $make_secure = SECURE_BODY;
                    }
                }  

But looking at the changed field names we are actually putting in the header for attachment changes, we do not include the attachment id such as "Attachment #1234 [details] description" for a description change. We just have "Attachment description" only which will not match.

So we need to either do one of two things:

1) Convert to "Attachment #1234 [details] description" format
2) or include a new "X-Bugzilla-Attach-ID:" header and store the attachment id there to pull from in the SecureMail code. We normally never update more than one attachment at a time so this should not be an issue.

I personally think #2 is better as changing the field names in the current header may break a lot of filter rules already in place. Will work on a patch.

dkl

Comment 5

[David Lawrence [:dkl]]

Attachment: 829358_1.patch

Comment 6

[:glob ✱]

Comment on attachment 8526283 [details] [diff] [review]
829358_1.patch

Review of attachment 8526283 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

::: extensions/SecureMail/Extension.pm
@@ +308,5 @@
>                  }
>                  # Encrypt if updating a private attachment without a comment
>                  if ($email->header('X-Bugzilla-Changed-Fields')
> +                    && $email->header('X-Bugzilla-Attach-ID')
> +                    && $email->header('X-Bugzilla-Changed-Fields') =~ /Attachment/)

searching x-bugzilla-changed-fields for attachment is redundant and can be removed

Comment 7

[David Lawrence [:dkl]]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   57c9998..66da5ae  master -> master