741996 - approval-mozilla-esr10 flag request mails show real subject for security bugs

Status: RESOLVED FIXED

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

Description

[Reed Loden [:reed]]

khuey reported that flag mails still have the full bug summary in the subject for security bugs.

Comment 1

[Daniel Veditz [:dveditz]]

:mccr8 reported the same, although he added the intriguing detail that it looked like approval-mozilla-beta+ and approval-mozilla-aurora+ mail was secure but approval-mozilla-esr10+ mail was not. I can't think of any reason bugzilla would treat those flags differently. The only difference I see is that beta/aurora have flag comments and esr10 does not.

I can confirm myself-- for bug 724781 I got two mails sent at the same time for the same change:
  approval-mozilla-esr10 granted: [Bug 724781] Java related <rest of subject>
  [Bug 724781] (Secure bug updated)

The full content is unencrypted, it's not just the subject.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yes, its just esr, it just happened to be the only one I looked at.

Comment 3

[Daniel Veditz [:dveditz]]

The esr flag specificity seems particularly odd.

Comment 4

[David Lawrence [:dkl]]

Can you attach the full text of the email (headers too) to this bug so I can track down the differences?

Thanks
dkl

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: ESR10 mail with headers, sensitive text elided

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: beta mail

No substantive differences between the two that I can see.

Comment 8

[David Lawrence [:dkl]]

Thanks I have found the problem. In the extension we grep the subject for the bug id and the regex used was finding the id incorrectly due to the esr flag having a number in the name. Also the code was making the body not secure due to not error checking the bug load was successful after getting the id. Patch ready and coming.

dkl

Comment 9

[David Lawrence [:dkl]]

Attachment: Patch to fix encryption of flag emails with certain flag names (v1)

gerv this changes the regex used to pull the bug id from the email subject so that flags with a number in the name do not cause the wrong id to be pulled. Also it now checks for $bug->{'error'} before making the body insecure. Please review so I can get this out in the next update. Glob is out til thursday.

Thanks
dkl

Comment 11

[Gervase Markham [:gerv]]

Comment on attachment 612232 [details] [diff] [review]
Patch to fix encryption of flag emails with certain flag names (v1)

r=gerv. Although is there a reason you go from the first digit to the closing bracket, rather than looking for a run of digits inside the brackets? 

([^\]]+)
vs
([\d]+)

With the current header, they are equivalent, but I just wondered...

Gerv

Comment 12

[David Lawrence [:dkl]]

(In reply to Gervase Markham [:gerv] from comment #11)
> Comment on attachment 612232 [details] [diff] [review]
> Patch to fix encryption of flag emails with certain flag names (v1)
> 
> r=gerv. Although is there a reason you go from the first digit to the
> closing bracket, rather than looking for a run of digits inside the
> brackets? 
> 
> ([^\]]+)
> vs
> ([\d]+)
> 
> With the current header, they are equivalent, but I just wondered...
> 
> Gerv

Moving to /\[\D+(\d+)\]/ which is much clearer as you pointed out. Retesting and will commit with the new regex if all goes well.

dkl

Comment 13

[David Lawrence [:dkl]]

Comitted.

Committing to: bzr+ssh://[e-mail]/bmo/4.0             
modified extensions/SecureMail/Extension.pm
Committed revision 8121. 

Committing to: bzr+ssh://[e-mail]/bmo/4.2             
modified extensions/SecureMail/Extension.pm
Committed revision 8107

dkl