1605656 - Incorrect parsing can make mach try fail

Status: RESOLVED FIXED

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

Description

[Sylvestre Ledru [:Sylvestre]]

with commit message:

try to debug 2

it fails with:

remote: 
remote: ********************************** ERROR **********************************
remote: Your commit message references a bug that does not exist. Please check your
remote: commit message and try again.
remote: 
remote:     Affected bug: 2
remote: 
remote: 
remote: To push this commit anyway and ignore this warning, include SKIP_BMO_CHECK
remote: in your commit message.
remote: ***************************************************************************

I guess "debug 2" is parsed as "bug 2"

Comment 1

[Zeid [:zeid]]

Related to bug 1605350 and bug 1482236.

The regex matching needs to be reviewed and made more specific in mozautomation.commitparser (however in this particular case looks like the change should be pretty simple, i.e., check for the full word "bug").

Comment 2

[Zeid [:zeid]]

Looks like right now we are matching very liberally any integers as bug IDs. We could (1) match more conservatively (i.e. only if user has specified "bug" etc. before an integer; feature already exists in commitparser) or (2) only warn the user if a bug ID is not found, in which case we will have to potentially search through more bugs until a possibly security bug is found.

Option (1) could cause some problems because it means a security bug ID could sneak its way into a commit quite easily. If we implement option
(2), we would have to specify a tolerance for the maximum number of flagged bugs to check to prevent possibly flooding BMO with requests. The latter option is my preferred option. Additionally, we could introduce a convention to reference external bugs (e.g. numbers preceded with a specific character are ignored in the check, e.g. *9567276 and *928745 would be treated as external bug IDs -- this is a new feature and the instructions could be presented to the user as needed.) Additionaly, we could only check BMO URls and ignore integers present in other URLs.

Three possible scenarios (assume max. threshold of 4 -- arbitrarily set):

1. A user submits a commit message that includes 5 bugs IDs, one which is a security bug, and 2 which are external references to bugs in a different system. In this scenario, the initial check flags 3 bug IDs that can not be immediately resolved, and starts checking each one. Since there are less than 4 flagged bug IDs, all bugs will be checked. Since there is a security bug, the commit will be rejected. The user may be warned about the bugs that could not be found and could be presented with possible solutions.

2. A user submits a commit message that references two numbers that do not exist in BMO as bugs. The user is presented with a warning message but the push is accepted. The user could be presented with an informative message regarding the bugs that were not found.

3. A user submits a commit message that references 7 integers, 5 of which can not be found as bugs on BMO, and one of which is a security bug ID. The system checks the numbers starting with ones that may be BMO IDs (e.g. ones 7 digits or less) in descending order. As it happens, the first 4 bug IDs checked can not be found on BMO, the system stops checking and presents the user with an error and an informative message of how to remedy the situation.

Thoughts?

The regex matching needs to be reviewed and made more specific in mozautomation.commitparser (however in this particular case looks like the change should be pretty simple, i.e., check for the full word "bug").

Upon closer inspection, this would only be relevant if we go the "conservative" route, as integers are still checked anyway in the liberal setting even if not preceded by the word "bug". This should still be fixed, but probably only in the conservative setting.

Comment 4

[Connor Sheehan [:sheehan]]

I think the easiest thing to do would be to use BUG_CONSERVATIVE_RE from the commitparser, either by adding a kwarg to parse_bugs or by writing a new function to use the more conservative regex and using that instead. That's likely the fastest way to get the hook turned back on without blocking pushes egregiously. I think the coverage we would get from using that regex is probably good enough, and is definitely better than having the hook turned off altogether. If things manage to sneak by the hook anyways we can add more conditions to the parser to cover those cases. I think that's probably better than starting from a liberal matcher and squashing conditions that shouldn't be matched.

Comment 5

[Zeid [:zeid]]

Attachment: hooks: more conservative push hook for sec bugs (Bug 1605656) r=sheehan,mhentges,glob

WIP do not land

Comment 6

[Zeid [:zeid]]

Attachment: hooks: re-enable sec bug push hook for try (Bug 1605656) r=sheehan

Depends on D69608

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/fc5ae134725e
hooks: more conservative push hook for sec bugs r=sheehan,mhentges
https://hg.mozilla.org/hgcustom/version-control-tools/rev/1113056f2880
hooks: re-enable sec bug push hook for try r=sheehan