From bf0695e41fb2ce002dba5e0a5bd9eb9219a694ae Mon Sep 17 00:00:00 2001 From: Ryan VanderMeulen Date: Tue, 11 Aug 2026 21:52:20 -0400 Subject: [PATCH 1/3] Send the Bugzilla api key as a header rather than a query parameter bz.js appended the key to the request URL, putting it into Bugzilla's access logs on every privileged request. BMO accepts X-BUGZILLA-API-KEY and allows it via CORS on both the GET and PUT preflights, so send it there instead. Invalid keys still come back as the same error 306 message the submit path already matches on. --- bugherder/thirdparty/bzjs/bz-0.4.3.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/bugherder/thirdparty/bzjs/bz-0.4.3.js b/bugherder/thirdparty/bzjs/bz-0.4.3.js index ba8d44b..a430bbe 100644 --- a/bugherder/thirdparty/bzjs/bz-0.4.3.js +++ b/bugherder/thirdparty/bzjs/bz-0.4.3.js @@ -306,10 +306,7 @@ var BugzillaClient = (function () { params = params || {}; - if (this.api_key) { - params.api_key = this.api_key; - } - + // The api key goes in a header, to keep it out of Bugzilla's access logs if (this._auth) { params.token = this._auth.token; } else if (this.username && this.password) { @@ -328,6 +325,9 @@ var BugzillaClient = (function () { var req = new XMLHttpRequest(); req.open(method, url, true); req.setRequestHeader("Accept", "application/json"); + if (this.api_key) { + req.setRequestHeader("X-BUGZILLA-API-KEY", this.api_key); + } if (method.toUpperCase() !== "GET") { req.setRequestHeader("Content-Type", "application/json"); } From 24bf20f49f136e59293c9ceb4bb8cd7f9d5aa986 Mon Sep 17 00:00:00 2001 From: Ryan VanderMeulen Date: Tue, 11 Aug 2026 21:45:05 -0400 Subject: [PATCH 2/3] Bug 1154724 - Allow restricted bugs to be loaded and marked with an api key Bugherder loads bug data anonymously, so bugs restricted to a Bugzilla group are silently dropped and can only be reported at the end for someone to mark by hand afterwards. Offer to load them with an api key instead, then rerun the usual flow over just those bugs. The key is reused for the submission, so it is only asked for once, and the sheriff pass is unaffected: without a key nothing loads and the push is presented exactly as before. Loading also skips pushes that reached backedOut without a bug number of their own, and stops the same bug being requested twice, so that the count of bugs that could not be loaded is accurate rather than counting an absent bug number as a restricted bug. --- README.md | 17 ++++ bugherder/css/style.css | 18 ++++ bugherder/index.html | 5 ++ bugherder/js/BugData.js | 12 ++- bugherder/js/Step.js | 95 +++++++++++++++++---- bugherder/js/Summary.js | 52 ++++++++---- bugherder/js/UI.js | 39 +++++++++ bugherder/js/Viewer.js | 18 ++-- bugherder/js/ViewerController.js | 39 ++++++++- bugherder/js/bugherder.js | 140 +++++++++++++++++++++++++++++-- 10 files changed, 389 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index 2861278..8256fa0 100644 --- a/README.md +++ b/README.md @@ -4,6 +4,23 @@ Bugherder is a tool for marking bugs post-merge, created by [Graeme McCutcheon](http://www.graememcc.co.uk/). +RESTRICTED BUGS +--------------- +Bugherder loads bug data anonymously, so bugs that are restricted to a Bugzilla group can't +be loaded, and can't be marked. Sheriffs generally don't have access to them either, so they +are normally left for someone from Release Management to deal with by hand afterwards. + +When a push contains such bugs, bugherder offers to load them with an API key. Supply one +belonging to an account that can see them, and bugherder will restart the normal flow with +everything except those bugs filtered out - the same commenting, resolving and flag setting as +usual, over just the restricted bugs. The key is reused for the submission, so it is only asked +for once. Anything that still can't be loaded with the key given is called out so it can be +followed up manually. + +This doesn't affect the sheriff pass in any way: without a key, nothing loads and the push is +presented exactly as before. + + NOTES ON TESTING ---------------- Adding "?debug=1" shows how all changesets were identified, and shows what changesets Bugherder decided were affected by a backout. diff --git a/bugherder/css/style.css b/bugherder/css/style.css index 30c430b..d5d21d1 100644 --- a/bugherder/css/style.css +++ b/bugherder/css/style.css @@ -335,6 +335,24 @@ div.ctr { color: rgb(51,51,51); } +#restricted { + max-width: 940px; + margin: 10px auto 0; + padding: 0 10px; +} + +#restrictedText { + display: inline-block; + border: 1px solid rgb(255,213,153); + background-color: rgb(255,239,217); + color: rgb(51,51,51); + padding: 2px 5px; +} + +#restrictedButton { + margin-left: 10px; +} + #loadingOverlay { padding-left: 25px; } diff --git a/bugherder/index.html b/bugherder/index.html index 6765b18..85efaf5 100644 --- a/bugherder/index.html +++ b/bugherder/index.html @@ -45,6 +45,11 @@

A tool to help with post-merge Bugzilla administrivia. +
+ + +
+
diff --git a/bugherder/js/BugData.js b/bugherder/js/BugData.js index 26eb01a..2060e86 100644 --- a/bugherder/js/BugData.js +++ b/bugherder/js/BugData.js @@ -9,6 +9,12 @@ var BugData = { loadCallback: null, errorCallback: null, checkComments: false, + apiKey: null, + + setApiKey: function BD_setApiKey(key) { + this.apiKey = key || null; + }, + load: function BD_load(bugs, checkComments, loadCallback, errorCallback) { this.notYetLoaded = bugs; @@ -56,7 +62,7 @@ var BugData = { self.parseData(data); }; - var bugzilla = bz.createClient({timeout: timeout}); + var bugzilla = bz.createClient({timeout: timeout, api_key: this.apiKey}); bugzilla.searchBugs(bugs, callback); }, @@ -111,7 +117,9 @@ var BugData = { bug.intestsuite = ' '; bug.testsuiteFlagID = -1; - bug.canSetTestsuite = ConfigurationData.hasTestsuiteFlag[bug.product][bugObj.component]; + // The configuration is loaded anonymously, so it omits logged-in-only products + var componentFlags = ConfigurationData.hasTestsuiteFlag[bug.product]; + bug.canSetTestsuite = !!(componentFlags && componentFlags[bugObj.component]); if (bug.canSetTestsuite && 'flags' in bugObj && bugObj.flags) { for (var i = 0; i < bugObj.flags.length; i++) { var f = bugObj.flags[i]; diff --git a/bugherder/js/Step.js b/bugherder/js/Step.js index a721523..86c570a 100644 --- a/bugherder/js/Step.js +++ b/bugherder/js/Step.js @@ -5,10 +5,25 @@ // bugs should be commented with the url for a particular push, along with the comment to // be written for each push/bug combination. A step is also responsible for transmitting the // relevant changes to Bugzilla -function Step(name, callbacks, isBackout) { +// bugFilter, if given, holds the only bug numbers this step should concern itself +// with; changesets carrying none of them are left out entirely +function Step(name, callbacks, isBackout, bugFilter) { var self = this; + function isWanted(bugID) { + return !self.bugFilter || (bugID in self.bugFilter); + } + + function hasWantedBug(index) { + var push = PushData.allPushes[index]; + if (push.bug) + return isWanted(push.bug); + if (push.backoutBugs) + return push.backoutBugs.some(isWanted); + return false; + } + function constructAttachedBugs(useBackouts) { var arr = PushData[self.name]; if (useBackouts) @@ -16,22 +31,65 @@ function Step(name, callbacks, isBackout) { var len = arr.length; for (var i = 0; i < len; i++) { + // Anything attached to a changeset we don't draw would submit unseen + if (self.bugFilter && !(arr[i] in self.rendered)) + continue; + var push = PushData.allPushes[arr[i]]; if (push.bug) { + if (!isWanted(push.bug)) + continue; self.attachedBugs[arr[i]] = {}; self.attachBugToCset(arr[i], push.bug); } else if (push.backoutBugs && push.backoutBugs.length > 0) { + var backoutBugs = push.backoutBugs.filter(isWanted); + if (backoutBugs.length == 0) + continue; self.attachedBugs[arr[i]] = {}; - var l2 = push.backoutBugs.length; + var l2 = backoutBugs.length; for (var j = 0; j < l2; j++) - self.attachBugToCset(arr[i], push.backoutBugs[j]); + self.attachBugToCset(arr[i], backoutBugs[j]); } } } + // Unfiltered, that's every changeset in the category, including those without a bug + // so that they can be given one + function constructPushList() { + if (!self.bugFilter) { + self.pushes = PushData[self.name]; + return; + } + + if (!self.hasBackouts) { + self.pushes = PushData[self.name].filter(hasWantedBug); + self.pushes.forEach(function Step_markRendered(index) { + self.rendered[index] = true; + }); + return; + } + + PushData[self.name].forEach(function Step_filterBackout(index) { + var affected = PushData.allPushes[index].affected.filter(hasWantedBug); + if (affected.length == 0 && !hasWantedBug(index)) + return; + + self.pushes.push(index); + self.affected[index] = affected; + self.rendered[index] = true; + affected.forEach(function Step_markAffectedRendered(j) { + self.rendered[j] = true; + }); + }); + } + this.name = name; this.callbacks = callbacks; this.hasBackouts = isBackout; + this.bugFilter = bugFilter || null; + this.pushes = []; + this.affected = {}; + this.rendered = {}; this.attachedBugs = {}; this.bugInfo = {}; @@ -53,6 +111,7 @@ function Step(name, callbacks, isBackout) { } this.unprivilegedLoader = bz.createClient(options); + constructPushList(); constructAttachedBugs(false); if (this.hasBackouts) constructAttachedBugs(true); @@ -64,6 +123,19 @@ Step.prototype.getName = function Step_getName() { }; +Step.prototype.getPushes = function Step_getPushes() { + return this.pushes; +}; + + +Step.prototype.getAffected = function Step_getAffected(index) { + if (index in this.affected) + return this.affected[index]; + + return PushData.allPushes[index].affected; +}; + + Step.prototype.canSubmit = function Step_canSubmit() { for (var bug in this.bugInfo) { var info = this.bugInfo[bug]; @@ -87,11 +159,6 @@ Step.prototype.getSentData = function Step_getSentData() { }; -Step.prototype.hasSecurityBugs = function Step_hasSecurityBugs() { - return this.securityBugs.length > 0; -}; - - Step.prototype.getSecurityBugs = function Step_getSecurityBugs() { var secBugs = []; for (var cset in this.attachedBugs) { @@ -508,12 +575,12 @@ Step.prototype.attachBugToCset = function Step_attachBugToCset(index, bugID) { if (bug) { leaveOpen = bug.leaveOpen; hasMilestone = bug.milestone != '---'; - if (hasMilestone || leaveOpen || !(Config.treeName == 'mozilla-central' || Config.treeName == 'comm-central')) + var productMilestones = ConfigurationData.milestones[bug.product]; + if (hasMilestone || leaveOpen || !productMilestones || + !(Config.treeName == 'mozilla-central' || Config.treeName == 'comm-central')) milestone = bug.milestone; - else { - var defaultMilestone = ConfigurationData.milestones[bug.product].defaultIndex; - milestone = ConfigurationData.milestones[bug.product].values[defaultMilestone]; - } + else + milestone = productMilestones.values[productMilestones.defaultIndex]; } if (!(bugID in this.bugInfo)) { @@ -983,7 +1050,7 @@ Step.prototype.setMaxStepNumber = function Step_setMaxStepNumber(num) { // Return the user-visible step name to be shown for this step Step.prototype.getHeading = function Step_getHeading(addMax) { - addMax = addMax || true; + addMax = addMax !== false; var res = this.name; if (this.name in Step.headings) diff --git a/bugherder/js/Summary.js b/bugherder/js/Summary.js index 4ac7111..27c46d0 100644 --- a/bugherder/js/Summary.js +++ b/bugherder/js/Summary.js @@ -70,17 +70,31 @@ var Summary = { makeSecBugHTML: function summary_makeSecBugHTML(steps) { var sechtml = ''; + + // Steps set aside during this session cover the same changesets as their + // replacements, so the same bug can be reported by more than one of them + var seen = {}; + function unseen(secBug) { + var key = secBug.cset + ':' + secBug.bug; + if (key in seen) + return false; + + seen[key] = true; + return true; + } + for (var i = 0; i < steps.length; i++) { - if (steps[i].hasSecurityBugs()) { - sechtml += '
  • '+steps[i].getHeading(false) + '
    '; - var sb = steps[i].getSecurityBugs(); - sechtml += ''; - for (var j = 0; j < sb.length; j++) { - sechtml += ''; - } - sechtml += '
    ChangesetLinkBug
    ' + sb[j].cset + '' + UI.linkifyRevURL(sb[j].link); - sechtml += '' + UI.linkifyBug(sb[j].bug) + '
    '; - } + var sb = steps[i].getSecurityBugs().filter(unseen); + if (sb.length == 0) + continue; + + sechtml += '
  • '+steps[i].getHeading(false) + '
    '; + sechtml += ''; + for (var j = 0; j < sb.length; j++) { + sechtml += ''; + } + sechtml += '
    ChangesetLinkBug
    ' + sb[j].cset + '' + UI.linkifyRevURL(sb[j].link); + sechtml += '' + UI.linkifyBug(sb[j].bug) + '
    '; } if (sechtml == '') @@ -104,24 +118,34 @@ var Summary = { }, - view: function summary_View(steps, onPrevious, onNext) { + view: function summary_View(steps, onPrevious, onNext, priorSteps) { + priorSteps = priorSteps || []; + + // Only replaced steps that submitted something belong in the activity list and the + // unsubmitted warning. All of them still know which bugs could not be loaded, so the + // security bug table gets the lot + var submitted = priorSteps.filter(function summary_hasSubmitted(step) { + return step.getSentData().length > 0; + }); + var activeSteps = submitted.concat(steps); + // Hide any previous viewer output UI.hide('viewerOutput'); UI.clearErrorMessage(); $('#viewerOutput').empty(); $('#viewerOutput').append(this.makeButtonHTML(onPrevious.label, onNext.label)); - var subHTML = this.makeUnsubmittedHTML(steps); + var subHTML = this.makeUnsubmittedHTML(activeSteps); if (subHTML != '') $('#viewerOutput').append(subHTML + '
    '); - var secHTML = this.makeSecBugHTML(steps); + var secHTML = this.makeSecBugHTML(priorSteps.concat(steps)); if (secHTML != '') $('#viewerOutput').append(secHTML + '
    '); $('#viewerOutput').append('

    Summary of activity

    '); - steps.forEach(function step_viewSummaryMaker(step){ + activeSteps.forEach(function step_viewSummaryMaker(step){ $('#viewerOutput').append(this.makeSummaryForStep(step)); }, this); diff --git a/bugherder/js/UI.js b/bugherder/js/UI.js index f4c30c0..b2cf502 100644 --- a/bugherder/js/UI.js +++ b/bugherder/js/UI.js @@ -169,6 +169,10 @@ var UI = { showModalForm: function UI_showModalForm(id, formID, submitAction, cancelID, cancelAction) { + // The toggles below would put an already-open form straight back down again + if (UI.modalID) + return; + UI.modalID = '#' + id; if (formID) { UI.modalForm = '#' + formID; @@ -223,6 +227,7 @@ var UI = { onCredentialsCancel: function UI_onCredentialsCancel(e) { $('#apikey').val(''); + ViewerController.credentialsCallback = null; }, @@ -234,6 +239,40 @@ var UI = { }, + hideRestricted: function UI_hideRestricted() { + this.hide('restricted'); + }, + + + showRestrictedOffer: function UI_showRestrictedOffer(count, onClick) { + var them = count == 1 ? 'it' : 'them'; + var text = count + ' bug' + (count == 1 ? '' : 's') + ' in this push could not be'; + text += ' loaded, most likely because ' + (count == 1 ? 'it is' : 'they are') + ' restricted.'; + text += ' If you have access to ' + them + ', you can load and mark ' + them; + text += ' with your api key.'; + + $('#restrictedText').text(text); + $('#restrictedButton').off('click.restricted').on('click.restricted', onClick); + $('#restrictedButton').show(); + this.show('restricted'); + }, + + + showRestrictedStatus: function UI_showRestrictedStatus(loaded, stillUnloaded) { + var text = 'Showing the ' + loaded + ' restricted bug' + (loaded == 1 ? '' : 's'); + text += ' from this push only. Everything else has been left out.'; + if (stillUnloaded.length > 0) { + text += stillUnloaded.length == 1 ? ' Bug ' : ' Bugs '; + text += stillUnloaded.join(', ') + ' could not be loaded with your api key,'; + text += ' and will still need marking by hand.'; + } + + $('#restrictedText').text(text); + $('#restrictedButton').off('click.restricted').hide(); + this.show('restricted'); + }, + + onAddBugSubmit: function UI_onAddBugSubmit(e) { var index = $('#addBugForm').attr('data-index'); ViewerController.onAddBug(parseInt(index), $('#loadBug').val()); diff --git a/bugherder/js/Viewer.js b/bugherder/js/Viewer.js index a93dcc6..a383eb6 100644 --- a/bugherder/js/Viewer.js +++ b/bugherder/js/Viewer.js @@ -37,10 +37,12 @@ var Viewer = { 'testsuite': this.decorateWithRequired(this.onTestsuiteChange, indexBug, 'Testsuite') }; + // init runs again when the steps are rebuilt, so don't stack a second handler set var self = this; - $('#viewerOutput').click(bindListener(self, clickListeners)); - $('#viewerOutput').on('input', bindListener(self, inputListeners)); - $('#viewerOutput').on('change', bindListener(self, changeListeners)); + $('#viewerOutput').off('.viewer'); + $('#viewerOutput').on('click.viewer', bindListener(self, clickListeners)); + $('#viewerOutput').on('input.viewer', bindListener(self, inputListeners)); + $('#viewerOutput').on('change.viewer', bindListener(self, changeListeners)); }, @@ -337,6 +339,11 @@ var Viewer = { makeMilestoneSelectHTML: function viewer_makeMilestoneSelectHTML(cset, index, id) { + var product = BugData.bugs[id].product; + // Milestones are already encoded by the time they reach bugInfo + if (!(product in ConfigurationData.milestones)) + return this.step.getMilestone(id); + var html = '