Andrew McCreight [:mccr8] Reporter
	Description • 11 years ago

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.

	Gervase Markham [:gerv]
	Comment 1 • 11 years ago

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

Gerv

	Gervase Markham [:gerv]
	Comment 2 • 11 years ago

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

	Andrew McCreight [:mccr8] Reporter
	Comment 3 • 11 years ago

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

	David Lawrence [:dkl] Assignee
	Comment 4 • 10 years ago

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

	:glob ✱
	Comment 6 • 10 years ago

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

	David Lawrence [:dkl] Assignee
	Comment 7 • 10 years ago

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   57c9998..66da5ae  master -> master