790143 - Ensure committers have thought about bug description for security bugs when landing

Status: RESOLVED WONTFIX

(Developer Services :: Mercurial: hg.mozilla.org, defect)

Description

[Jason Duell]

I've done this one too many time, forgetful me...

We should automatically strip anything bug "bug 45677: r=bz, a=akeybl" from security bugs when they are landed.  Right now we tell devs to manually do this.  I've forgotten now a bunch of times, and each time I get a reprimand and the hackers still get the hint in our hg logs.

Should just be a matter of querying bugzilla for isSecure?

Comment 1

[Shyam Mani [:fox2mike]]

Where do you want this stripped from? Bugzilla?

Comment 2

[Daniel Veditz [:dveditz]]

Might be better than the status quo, but would prevent the use of comments that are useful and not revealing. For many fixes that's probably not possible (you can't obscure a simple bounds check without lying) but for more involved fixes it's often possible to describe the code restructuring without saying which part of it precisely led to some bad thing elsewhere.

In fact, comment-less check-ins might be even more of a red-flag than one with a comment :-(

Comment 3

[Andrew McCreight [:mccr8]]

I generally like to give descriptions that are essentially mechanical descriptions of bugs. For instance, for bug 708825, I just said "set mIdentity to null if compartment enter fails", which doesn't reveal what the horrible consequence of not doing it, nor the weird tricky way you hit that problem. I feel like that at least gives some information in the hg log for later archaeology. But maybe it doesn't help much.

You can always just never put a descriptive description in the patch, though maybe that interferes with your work flow.

Comment 4

[Jason Duell]

> would prevent the use of comments

We could add some sort of DONTDELETE magic so that people could add comments if they want one.  If nothing else this would be a reminder that they should stop and think about what they write, if anything...

Comment 5

[Jason Duell]

So I propose we morph this into the following:  for secure bugs we barf at push time unless the commit message contains (ends with?) "SECURE".   If the message does contain "SECURE" than we s/SECURE// and commit the rest of the message.

That will at least make sure that people don't land patches without thinking of the text that they're putting into the public record.

Comment 6

[Jason Duell]

The barf message could also point to a URI that describes best practices for what sort of commit message to provide--on IRC I got the advice to use a blank message, and it seems that's not actually the best way to do it.

Comment 7

[Shyam Mani [:fox2mike]]

This sounds like an hg hook.

Comment 8

[(not currently active) Ted Mielczarek]

FYI I don't think we can change commit messages on the server without really breaking the hg workflow. Changing the commit message changes the changeset's hash value.

I don't really know how you'd programmatically determine that the commit message has been carefully chosen, so I'm not sure how we'd implement this.

Comment 9

[Jason Duell]

> Changing the commit message changes the changeset's hash value.

Ah, that blows up my idea in comment 5.   I'm out of ideas here--if someone wants to pursue, reopen or file a new bug.