Closed Bug 1164063 Opened 5 years ago Closed 4 years ago

show a warning near the attachments table for sec-high/sec-crit bugs without sec-approval? on patches

Categories

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

Production
defect
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: sfink, Assigned: glob)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

I often forget to request sec-approval (from bug 786836) on security bugs before landing. It would be very helpful for me if setting sec-high or sec-crit would autocomment something like

"This bug has been marked security sensitive. Please request sec-approval? on any patches before landing, in order to avoid prematurely exposing security vulnerabilities."

(Just an example; I imagine the security folks are the ones who would be writing the text.)
Summary: Setting sec-high or sec-crit should emit boilerplate → Setting sec-high or sec-crit should emit sec-approval boilerplate
We do have bug 1158178 but it hasn't gone anywhere.
sec-high and sec-crit are keywords which we do not currently have the capability to auto-fill the comment textarea when added. We have that for normal flags such as sec-review and sec-approval. We could do either:

1. Add hook code on the backend to add the comment when a bug change is submitted containing the keywords mentioned.
2. Add functionality to auto-fill the comment text in the UI when a keyword is added similar to the flags. 
3. Migrate from using sec-* keywords to use flags and take advantage of what is already there.

1 and 3 are the easiest depending on which path you want to go. 2 would be the most work and is least likely to happen soon.

dkl
i have concerns about the usefulness of adding a comment at the time the keyword is added.
in the sec bugs i've seen the security rating has been set early in the bug's life, so it's likely by the time the patches are ready to be committed the comment with the reminder may be lost/forgotten.


instead i propose adding a warning message near the attachments table for bugs with a sec-high/sec-critical keywords, patches, but do not have sec-approval requested or set.

eg. https://www.dropbox.com/s/btc1gzw55v8v4gx/Screenshot%202015-05-13%2011.28.03.png
(i would expect "sec-approval" to link to a wiki page describing the process).
(In reply to Byron Jones ‹:glob› from comment #3)
> i have concerns about the usefulness of adding a comment at the time the
> keyword is added.
> in the sec bugs i've seen the security rating has been set early in the
> bug's life, so it's likely by the time the patches are ready to be committed
> the comment with the reminder may be lost/forgotten.
> 
> instead i propose adding a warning message near the attachments table for
> bugs with a sec-high/sec-critical keywords, patches, but do not have
> sec-approval requested or set.

That's fair, though it wouldn't work for my personal pattern, since I normally do most of my patch manipulation from the command line and therefore do not see the attachments table. (I do read the bugmail, though.) Then again, I think abillings is right and bug 1158178 is the real fix for scumbags like me.

> eg.
> https://www.dropbox.com/s/btc1gzw55v8v4gx/Screenshot%202015-05-13%2011.28.03.
> png
> (i would expect "sec-approval" to link to a wiki page describing the
> process).

I like it, and think it would be independently useful. Though I'd make it a somewhat scarier color. Orange or something.
(In reply to Steve Fink [:sfink, :s:] from comment #4)
> Then again, I think abillings is right and bug 1158178 is the real fix for
> scumbags like me.

+1

> I like it, and think it would be independently useful.

the framework for that is happening in bug 1135164, updating summary and marking dependency.
Depends on: 1135164
Summary: Setting sec-high or sec-crit should emit sec-approval boilerplate → show a warning near the attachments table for sec-high/sec-crit bugs without sec-approval? on patches
I like this too. The release management team is talking about similar ideas in bug 1158178. Could we do this (warning developers) and also make it impossible to set approval flags for uplifts to + on these bugs, when they don't have sec-approval:+ set?
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #6)
> I like this too. The release management team is talking about similar ideas
> in bug 1158178. Could we do this (warning developers) and also make it
> impossible to set approval flags for uplifts to + on these bugs, when they
> don't have sec-approval:+ set?

can you point me to a wiki page that describe the use of the flags and when these limitations should apply?
i'd probably want to break the flag limiting into a separate bug from this, but that documentation helps both causes.
sylvestre, are you able to provide the details for comment 7?
Flags: needinfo?(sledru)
Sure, this should be this page:
https://wiki.mozilla.org/Security/Bug_Approval_Process
Flags: needinfo?(sledru)
Assignee: nobody → glob
more questions ..

if a sec-critical bug has 2 non-obsolete patches, do both patches require sec-approval, or is it ok if any of the patches have approval?


the docs has:
> 1. The bug has a sec-low, sec-moderate, sec-other, or sec-want rating.
> OR
> 2. The bug is a recent regression on mozilla-central (this means that
> the specific regressing check-in has been identified on mozilla-central)

i'd like a sanity check on the logic i'd need to implement... we'd warn if

- the bug has 'sec-high' or 'sec-critical' keywords set
AND
- there aren't any status-firefox tracking flags set
  OR
  all status-firefox tracking flags except the latest are set to 'unaffected'
AND
- there is at least one patch without sec-approval set (to any value)

?
Flags: needinfo?(sledru)

(In reply to Byron Jones ‹:glob› from comment #10)

> i'd like a sanity check on the logic i'd need to implement... we'd warn if

> - there aren't any status-firefox tracking flags set
>   OR
>   all status-firefox tracking flags except the latest are set to 'unaffected'


I think this should be 

OR
any of the current status-firefox tracking flags that aren't the latest are set to 'affected'

As I understand it, if we've said that all the other current branches are unaffected, and only one branch is affected, the patch doesn't need sec-approval.  If we know more than one branch is affected, or if we aren't sure if multiple branches are affected or not (ie, status is not set) then we should be determining which branches are affected before landing anywhere.     

The problem with this logic is that sometimes we set 'affected' for the current nightly, and maybe unaffected for the one before it, but no one bothers to say that every current previous branch is unaffected, because we only need to establish a demarcation point for when the regression happened. So we may need to add in another OR like so,

OR 
a branch is marked 'affected' and the one preceding it (one number lower) has no value set

Hope that makes sense, it's late and I'm tired !
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #11)
> any of the current status-firefox tracking flags that aren't the latest are
> set to 'affected'

ok, that sounds easier.

> The problem with this logic is that sometimes we set 'affected' for the
> current nightly, and maybe unaffected for the one before it, but no one
> bothers to say that every current previous branch is unaffected[...]

that's fine -- by your logic as long as any of the not-nightly flags are set to 'affected' then we'll warn.

it doesn't have to be perfect initially as it's just a warning; we can always refine the logic over time to match real world usage.
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #11)

> As I understand it, if we've said that all the other current branches are
> unaffected, and only one branch is affected, the patch doesn't need
> sec-approval. 

No, this is not the rule. The rule is if ONLY trunk is affected, then it doesn't need sec-approval.

https://wiki.mozilla.org/Security/Bug_Approval_Process
(In reply to Byron Jones ‹:glob› from comment #10)
> if a sec-critical bug has 2 non-obsolete patches, do both patches require
> sec-approval, or is it ok if any of the patches have approval?

i'll go with "we're good as long as at least one patch has sec-approval"
Flags: needinfo?(sledru)
Attached patch 1164063_1.patchSplinter Review
Attachment #8668807 - Flags: review?(dkl)
Comment on attachment 8668807 [details] [diff] [review]
1164063_1.patch

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

r=dkl

::: extensions/BMO/Extension.pm
@@ +851,5 @@
> +        WHERE       => {
> +            'name like ?' => 'cf_status_firefox%',
> +        },
> +    });
> +    $flags = [ grep { $_->name =~ /^cf_status_firefox/ } @$flags ];

This line seems redundant since you already have the WHERE above
Attachment #8668807 - Flags: review?(dkl) → review+
(In reply to David Lawrence [:dkl] from comment #16)
> ::: extensions/BMO/Extension.pm
> @@ +851,5 @@
> > +        WHERE       => {
> > +            'name like ?' => 'cf_status_firefox%',
> > +        },
> > +    });
> > +    $flags = [ grep { $_->name =~ /^cf_status_firefox/ } @$flags ];
> 
> This line seems redundant since you already have the WHERE above

it does seem that way, but set flags are added after the sql query is executed so we need that grep.
i'll add a comment.
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   42270ff..bbd091b  master -> master
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
"sec-approval required on patches before landing"
Excellent! Congrat to the bmo team! :)
Status: RESOLVED → VERIFIED
Blocks: 1404173
You need to log in before you can comment on or make changes to this bug.