From d03eec5251c99394724423f5e15a2c7d0cf28b67 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Wed, 23 Sep 2026 16:14:01 -0400 Subject: [PATCH 1/2] Bug 2055580 --- Bugzilla/API/V1/Github.pm | 160 +++++-- Dockerfile | 1 - Makefile.PL | 1 - docs/en/rst/api/core/v1/github.rst | 74 +++- qa/t/rest_github_pull_request.t | 28 ++ qa/t/rest_github_push_comment.t | 18 +- t/github-webhook-bug-visibility.t | 419 ++++++++++++++++++ .../en/default/admin/params/github.html.tmpl | 7 +- 8 files changed, 659 insertions(+), 49 deletions(-) create mode 100644 t/github-webhook-bug-visibility.t diff --git a/Bugzilla/API/V1/Github.pm b/Bugzilla/API/V1/Github.pm index 7e71c465a9..e11f03d097 100644 --- a/Bugzilla/API/V1/Github.pm +++ b/Bugzilla/API/V1/Github.pm @@ -14,9 +14,10 @@ use Bugzilla::Bug; use Bugzilla::BugMail; use Bugzilla::Constants; use Bugzilla::Group; +use Bugzilla::Logging; use Bugzilla::Milestone; use Bugzilla::User; -use Bugzilla::Util qw(fetch_product_versions); +use Bugzilla::Util qw(clean_text fetch_product_versions remote_ip); use Bugzilla::Extension::TrackingFlags::Flag; use Bugzilla::Extension::TrackingFlags::Flag::Bug; @@ -49,7 +50,8 @@ sub pull_request { } # Verify that signature is correct based on shared secret - if (!$self->_verify_signature) { + my $webhook_auth = $self->_verify_signature; + if (!$webhook_auth) { return $self->code_error('github_mismatch_signatures'); } @@ -59,7 +61,7 @@ sub pull_request { return $self->render(json => {error => 0}); } - # Validate JSON input + # Validate JSON input my $payload = $self->req->json; my @errors = joi->object->props( action => joi->string->required, @@ -112,10 +114,26 @@ sub pull_request { } } - # Create new attachment using pull request URL as attachment content + # Record which webhook credential authenticated this privileged request so + # that abuse of a leaked bot key remains attributable after the fact. + INFO(sprintf( + 'github pull_request webhook authenticated as %s (%s) from %s: bug %s, repo %s', + $webhook_auth->{login}, $webhook_auth->{via}, remote_ip(), + $bug->id, clean_text($repository) + )); + + # The identity whose bug visibility scopes this request (see _verify_signature). + my $webhook_user = $webhook_auth->{user}; + + # Create new attachment using pull request URL as attachment content. + # /rest has no login middleware, so there is no authenticated user to write + # as; we adopt the shared github-automation account. The bug was already + # gated above against the caller -- anonymous here -- so this only ever runs + # for public bugs. Only the groups (not bless_groups) are elevated because + # the webhook never manages group memberships: it only files attachments, + # comments, and flags. my $auto_user = Bugzilla::User->check({name => 'github-automation@bmo.tld'}); - $auto_user->{groups} = [Bugzilla::Group->get_all]; - $auto_user->{bless_groups} = [Bugzilla::Group->get_all]; + $auto_user->{groups} = [Bugzilla::Group->get_all]; Bugzilla->set_user($auto_user); my $timestamp = Bugzilla->dbh->selectrow_array("SELECT NOW()"); @@ -131,9 +149,11 @@ sub pull_request { mimetype => 'text/x-github-pull-request', }); - # Insert a comment about the new attachment into the database. + # Insert a comment about the new attachment into the database. Attribute it to + # the webhook credential that triggered the action so the identity behind the + # github-automation change is visible in bug history, not just server logs. $bug->add_comment( - '', + "(via GitHub webhook, authenticated as $webhook_auth->{login})", { type => CMT_ATTACHMENT_CREATED, extra_data => $attachment->id, @@ -154,13 +174,26 @@ sub pull_request { # data doesn't match this URL, skip it next if $attachment->data ne $html_url; + # Bugzilla->user is the all-groups automation user at this point, so it can + # see every bug. Check visibility as the original caller instead, otherwise + # we would obsolete attachments and comment on bugs the caller cannot see. + if (!$webhook_user->can_see_bug($attachment->bug_id)) { + WARN( 'github pull_request: not obsoleting attachment ' + . $attachment->id + . ' on bug ' + . $attachment->bug_id + . ': caller cannot see that bug'); + next; + } + $other_bugs{$attachment->bug_id}++; my $moved_comment = "GitHub pull request attachment was moved to bug " . $bug->id . ". Setting attachment " . $attachment->id - . " to obsolete."; + . " to obsolete.\n" + . "(via GitHub webhook, authenticated as $webhook_auth->{login})"; $attachment->set_is_obsolete(1); $attachment->bug->add_comment( $moved_comment, @@ -194,7 +227,8 @@ sub push_comment { } # Verify that signature is correct based on shared secret - if (!$self->_verify_signature) { + my $webhook_auth = $self->_verify_signature; + if (!$webhook_auth) { return $self->code_error('github_mismatch_signatures'); } @@ -204,7 +238,7 @@ sub push_comment { return $self->render(json => {error => 0}); } - # Validate JSON input + # Validate JSON input my $payload = $self->req->json; my @errors = joi->object->props( ref => joi->string->required, @@ -278,15 +312,51 @@ sub push_comment { push @{$update_bugs{$bug_id}}, {text => $comment_text}; } - # If no bugs were found, then we return an error + # Restrict this request to the bugs the authenticated webhook credential can + # actually see. The bug ids above come from commit messages, which anyone able + # to land on a monitored branch controls, and the write loop below runs as + # github-automation@bmo.tld with every group -- so without this gate a commit + # message naming a confidential bug id would comment on and resolve that bug. + # The scoping identity is resolved by _verify_signature. + my $webhook_user = $webhook_auth->{user}; + + my @denied_bugs = grep { !$webhook_user->can_see_bug($_) } keys %update_bugs; + if (@denied_bugs) { + delete @update_bugs{@denied_bugs}; + WARN(sprintf( + 'github push_comment webhook authenticated as %s (%s) from %s: ' + . 'ignoring bug(s) %s not visible to that account, repo %s', + $webhook_auth->{login}, $webhook_auth->{via}, + remote_ip(), join(',', sort { $a <=> $b } @denied_bugs), + clean_text($repository) + )); + } + + # If no bugs were found, then we return an error. Bugs dropped by the + # visibility gate above land here too, so a bug the credential cannot see is + # indistinguishable from one that does not exist -- the response cannot be + # used as an oracle for confidential bug ids. if (!keys %update_bugs) { return $self->code_error('github_push_comment_bug_not_found'); } - # Set current user to automation so we can add comments to private bugs + # Record which webhook credential authenticated this privileged request so + # that abuse of a leaked bot key remains attributable after the fact. + INFO(sprintf( + 'github push_comment webhook authenticated as %s (%s) from %s: bugs %s, repo %s', + $webhook_auth->{login}, $webhook_auth->{via}, + remote_ip(), join(',', sort { $a <=> $b } keys %update_bugs), + clean_text($repository) + )); + + # /rest has no login middleware, so there is no authenticated user to write + # as; we adopt the shared github-automation account. %update_bugs was already + # narrowed to bugs the signing bot can see, so this elevation only spares the + # bot from needing edit rights -- it does not widen which bugs are reachable. + # Only the groups (not bless_groups) are elevated because the webhook never + # manages group memberships: it only adds comments and sets flags. my $auto_user = Bugzilla::User->check({name => 'github-automation@bmo.tld'}); - $auto_user->{groups} = [Bugzilla::Group->get_all]; - $auto_user->{bless_groups} = [Bugzilla::Group->get_all]; + $auto_user->{groups} = [Bugzilla::Group->get_all]; Bugzilla->set_user($auto_user); my $dbh = Bugzilla->dbh; @@ -304,6 +374,11 @@ sub push_comment { $comment_text .= $comment->{text} . "\n\n"; } + # Attribute the automated comment to the webhook credential that triggered + # it, so the bot behind a github-automation action is visible in bug history + # (not just server logs) and any leaked-key abuse is traceable on the bug. + $comment_text .= "(via GitHub webhook, authenticated as $webhook_auth->{login})"; + # Set all parameters my $set_all = { comment => { @@ -379,38 +454,61 @@ sub push_comment { return $self->render(json => {error => 0, bugs => \%update_bugs}); } +# Verify the request signature and identify which webhook credential signed it. +# Returns a hashref describing the authenticated credential on success, or undef +# on failure: +# user - the Bugzilla::User whose bug visibility scopes this request +# login - identity string for logs and bug comments +# via - which credential matched, for logs +# Callers still perform the privileged bug mutations as the shared +# github-automation account, but must gate bug access on {user} and log the +# identity so that abuse of a leaked bot key remains attributable after the fact. +# +# This must be called *before* set_user($auto_user): the legacy-secret path +# captures Bugzilla->user, which afterwards is the all-groups automation account. sub _verify_signature { my ($self) = @_; my $payload = $self->req->body; my $received_signature = $self->req->headers->header('X-Hub-Signature-256'); - return 0 if !$received_signature; + return undef if !$received_signature; # Fast path: check legacy shared secret first during migration period. # Operators should migrate to per-bot API keys and clear this parameter. + # The legacy secret carries no bot identity, so it is scoped to the + # (unauthenticated) request user and is limited to public bugs. my $legacy_secret = Bugzilla->params->{github_pr_signature_secret}; if ($legacy_secret) { my $expected = 'sha256=' . hmac_sha256_hex($payload, $legacy_secret); - return 1 if secure_compare($expected, $received_signature); + return { + user => Bugzilla->user, + login => 'shared-secret', + via => 'legacy shared secret', + } + if secure_compare($expected, $received_signature); } - # Fetch all non-revoked, non-sticky API keys for users in the github-webhook-bot group. - # Each bot account uses its own Bugzilla API key as the GitHub webhook secret, - # so individual keys can be revoked without affecting other integrations. - # Sticky keys are excluded because they are IP-bound and not appropriate for - # webhook use from GitHub's IP ranges. + # Fetch all non-revoked, non-sticky API keys for enabled users in the + # github-webhook-bot group. Each bot account uses its own Bugzilla API key as + # the GitHub webhook secret, so individual keys can be revoked without + # affecting other integrations. Sticky keys are excluded because they are + # IP-bound and not appropriate for webhook use from GitHub's IP ranges. + # Disabled accounts are excluded to match Bugzilla::Auth, which rejects a + # disabled user after credential verification; without this, disabling a + # compromised bot would leave its webhook keys working. my $dbh = Bugzilla->dbh; my $keys = $dbh->selectall_arrayref( - "SELECT uak.id, uak.api_key + "SELECT uak.id, uak.api_key, uak.user_id, p.login_name FROM user_api_keys uak INNER JOIN user_group_map ugm ON ugm.user_id = uak.user_id + INNER JOIN profiles p ON p.userid = uak.user_id INNER JOIN " . $dbh->quote_identifier('groups') . " g ON g.id = ugm.group_id WHERE g.name = 'github-webhook-bot' + AND p.is_enabled = 1 AND uak.revoked = 0 AND uak.sticky = 0 AND ugm.isbless = 0 - AND ugm.grant_type = " . GRANT_DIRECT, - {Slice => {}} + AND ugm.grant_type = " . GRANT_DIRECT, {Slice => {}} ); foreach my $key_row (@{$keys}) { @@ -419,13 +517,17 @@ sub _verify_signature { # Track key usage so operators can see which key last authenticated a webhook request $dbh->do( "UPDATE user_api_keys SET last_used = LOCALTIMESTAMP(0), last_used_ip = ? WHERE id = ?", - undef, $self->tx->remote_address, $key_row->{id} + undef, remote_ip(), $key_row->{id} ); - return 1; + return { + user => Bugzilla::User->new({id => $key_row->{user_id}, cache => 1}), + login => $key_row->{login_name}, + via => 'api key id ' . $key_row->{id}, + }; } } - return 0; + return undef; } # If the ref matches a certain branch pattern for the repo we are interested @@ -435,7 +537,7 @@ sub _set_status_flag { # In order to determine the appropriate status flag for the default # branch, we have to find out what the current *nightly* Firefox version is. - # fetch_product_versions() calls an API endpoint maintained by rel-eng that + # fetch_product_versions() calls an API endpoint maintained by rel-eng that # returns all of the current product versions so we can use that. my $version; if ($branch eq 'main' || $branch eq 'master') { diff --git a/Dockerfile b/Dockerfile index c0d01f3917..b282254b66 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,4 +1,3 @@ - # Generate the third-party front-end libraries (jQuery, Prism, mermaid, ...) # from the versions pinned in package-lock.json. These files are not committed # to the repository; this stage is their only source. Node lives only here, so diff --git a/Makefile.PL b/Makefile.PL index b556efcc1a..4266084b6f 100755 --- a/Makefile.PL +++ b/Makefile.PL @@ -88,7 +88,6 @@ my %requires = ( 'Moo' => '2.002004', 'MooX::StrictConstructor' => '0.008', 'Mozilla::CA' => '20160104', - 'Net::CIDR' => '0', 'Net::DNS' => '0', 'Package::Stash' => '0.37', 'Parse::CPAN::Meta' => '1.44', diff --git a/docs/en/rst/api/core/v1/github.rst b/docs/en/rst/api/core/v1/github.rst index 833618e6ab..79ed274cbb 100644 --- a/docs/en/rst/api/core/v1/github.rst +++ b/docs/en/rst/api/core/v1/github.rst @@ -10,11 +10,22 @@ and be automatically redirected to the pull request. **Github Setup Instructions** -* Create or identify a Bugzilla bot account to own this webhook. The bot - account should be least-privileged — grant it only the permissions needed - for the integration. +* Create or identify a Bugzilla bot account to own this webhook. * A BMO admin must add that bot account to the ``github-webhook-bot`` group - via the Users admin UI (``/editusers.cgi``). + via the Users admin UI (``/editusers.cgi``). Membership in this group is a + privileged grant: it lets any of the account's (non-sticky) API keys drive + this endpoint, so only add bots whose owners are trusted. +* The attachment and its comment are written as the shared + ``github-automation@bmo.tld`` account, but **this endpoint only acts on + publicly visible bugs**. The target bug is checked against the + (unauthenticated) request user before the automation account is adopted, so a + pull request title naming a confidential bug is rejected with + ``github_pr_bug_not_found`` no matter which bot key signed it. +* The one exception is the cleanup pass that obsoletes the same pull request + attachment on *other* bugs. That pass is scoped to the bugs the signing bot + account can see, so a bot with security group memberships can obsolete an + attachment on (and add the accompanying comment to) a confidential bug it + has access to. * Log in as the bot account and go to Preferences > API Keys. * Create a new API key with a descriptive label (e.g. ``github-webhook-mozilla-bteam-bmo``). Copy the key value — it will only @@ -36,10 +47,21 @@ and be automatically redirected to the pull request. * Make sure at the bottom that "Active" is checked on. * Save the webhook. +.. warning:: + The bug id, title, and repository all come from the (signed) request body, so + a leaked key is a general attachment-creation credential, not one scoped to a + single repository: it can attach a pull request link to, and comment on, any + publicly visible bug. Its reach into confidential bugs is limited to + obsoleting an existing pull request attachment on a bug the signing bot + account can see. Keep webhook bots out of security groups they do not need, + and treat every such key as a credential. + .. note:: If a webhook secret is ever compromised, revoke the affected API key from the - bot account's Preferences > API Keys page. Only that single webhook is affected — - all other bot accounts' webhooks continue to work without any changes. + bot account's Preferences > API Keys page. Revoking one key does not affect any + other bot's webhook. Every authenticated webhook action is logged with, and the + resulting bug comment attributes, the bot account whose key signed the request, + so misuse of a leaked key is traceable. .. note:: Past pull requests will not automatically get a link created in the bug. New pull @@ -132,11 +154,23 @@ repositories, a Firefox status flag may be set to FIXED. **Github Setup Instructions** -* Create or identify a Bugzilla bot account to own this webhook. The bot - account should be least-privileged — grant it only the permissions needed - for the integration. +* Create or identify a Bugzilla bot account to own this webhook. * A BMO admin must add that bot account to the ``github-webhook-bot`` group - via the Users admin UI (``/editusers.cgi``). + via the Users admin UI (``/editusers.cgi``). Membership in this group is a + privileged grant: it lets any of the account's (non-sticky) API keys drive + this endpoint, so only add bots whose owners are trusted. +* The bug changes themselves are made as the shared + ``github-automation@bmo.tld`` account, but **only for bugs the signing bot + account can see**. A bug referenced in a commit message that the bot cannot + see is silently skipped. So the bot account's own visibility decides which + bugs its webhook can touch: a bot that must comment on bugs in a given + security group has to be a member of that group. +* Note that group membership is not the only thing that grants visibility. A + confidential bug is also visible to its assignee, its QA contact, and (when + the product allows it) its reporter or a user on its CC list. A bot with no + group memberships is therefore not strictly limited to public bugs, so avoid + leaving webhook bot accounts as the assignee, QA contact, or a CC of + confidential bugs. * Log in as the bot account and go to Preferences > API Keys. * Create a new API key with a descriptive label (e.g. ``github-webhook-mozilla-bteam-bmo-push``). Copy the key value — it will only @@ -159,10 +193,26 @@ repositories, a Firefox status flag may be set to FIXED. * Make sure at the bottom that "Active" is checked on. * Save the webhook. +.. warning:: + The bug ids and repository name come from the (signed) request body, and the + commit messages that supply those bug ids can be written by anyone able to + land on a monitored branch. A leaked key is therefore a general + bug-modification credential, not one scoped to a single repository — but its + reach is bounded by what the signing bot account can see. Keep webhook bots + out of security groups they do not need. + .. note:: If a webhook secret is ever compromised, revoke the affected API key from the - bot account's Preferences > API Keys page. Only that single webhook is affected — - all other bot accounts' webhooks continue to work without any changes. + bot account's Preferences > API Keys page. Revoking one key does not affect any + other bot's webhook. Every authenticated webhook action is logged with, and the + resulting bug comment attributes, the bot account whose key signed the request, + so misuse of a leaked key is traceable. + +.. note:: + The legacy global ``github_pr_signature_secret`` parameter identifies no bot + account, so requests signed with it are limited to public bugs. Deployments + that need this endpoint to comment on confidential bugs must migrate to + per-bot API keys and clear that parameter. .. note:: The API endpoint looks at the commit messages for the bug ID so diff --git a/qa/t/rest_github_pull_request.t b/qa/t/rest_github_pull_request.t index cc6360bf71..4feb3ad78b 100644 --- a/qa/t/rest_github_pull_request.t +++ b/qa/t/rest_github_pull_request.t @@ -26,6 +26,22 @@ my $secret = $config->{github_automation_user_api_key}; my $t = Test::Mojo->new(); +# pull_request attributes both the attachment-created comment and the +# moved/obsoleted comment to the webhook bot account whose API key signed the +# request. In this test that is the github-automation account itself. +my $attribution + = "(via GitHub webhook, authenticated as $config->{github_automation_user_login})"; + +# Return the text of the most recent comment on a bug. +sub last_comment_text { + my ($bug_id) = @_; + $t->get_ok( + $url . "rest/bug/$bug_id/comment" => {'X-Bugzilla-API-Key' => $api_key}) + ->status_is(200); + my $comments = $t->tx->res->json->{bugs}->{$bug_id}->{comments}; + return $comments->[-1]->{text}; +} + # Create a new test bug for linking to PR my $new_bug = { product => 'Firefox', @@ -156,6 +172,11 @@ my $attach_data = $t->tx->res->json->{attachments}->{$attach_id}->{data}; $attach_data = decode_base64($attach_data); ok($attach_data eq 'https://github.com/mozilla-bteam/bmo/pull/1'); +# The attachment-created comment must name the webhook credential so the +# identity behind the github-automation change is visible in bug history. +like(last_comment_text($bug_id), qr/\Q$attribution\E/, + 'attachment-created comment is attributed to the webhook credential'); + # Bug already had the same github attachment so don't add twice $t->post_ok( $url @@ -214,6 +235,13 @@ $t->get_ok( $url . "rest/bug/attachment/$attach_id" => {'X-Bugzilla-API-Key' => $api_key}) ->status_is(200)->json_is("/attachments/$attach_id/is_obsolete", true); +# The new bug's attachment comment and the old bug's moved/obsoleted comment +# must both carry the attribution trailer. +like(last_comment_text($bug_id_2), qr/\Q$attribution\E/, + 'attachment-created comment on the second bug is attributed'); +like(last_comment_text($bug_id), qr/\Q$attribution\E/, + 'moved/obsoleted attachment comment is attributed'); + # Test that ping events (when the webhook is first created) are successful # a valid signature is also provided # Post the valid GitHub event to the rest/github/pull_request API endpoint diff --git a/qa/t/rest_github_push_comment.t b/qa/t/rest_github_push_comment.t index a29261fafd..d7c0fe8ac0 100644 --- a/qa/t/rest_github_push_comment.t +++ b/qa/t/rest_github_push_comment.t @@ -22,6 +22,12 @@ my $api_key = $config->{admin_user_api_key}; my $url = Bugzilla->localconfig->urlbase; my $secret = $config->{github_automation_user_api_key}; +# push_comment appends a trailer attributing the comment to the webhook bot +# account whose API key signed the request. In this test that is the +# github-automation account itself (it owns the signing key above). +my $comment_trailer + = "\n\n(via GitHub webhook, authenticated as $config->{github_automation_user_login})"; + my $t = Test::Mojo->new(); # Create a new test bug for linking to PR @@ -138,7 +144,8 @@ my $comment_text = 'Authored by ' . $payload->{commits}->[0]->{author}->{name} . "\n" . $payload->{commits}->[0]->{url} . "\n[releases_v110] " - . $payload->{commits}->[0]->{message}; + . $payload->{commits}->[0]->{message} + . $comment_trailer; # Retrieve the new comment from the bug to make sure it was created correctly $t->get_ok( @@ -201,7 +208,8 @@ $comment_text . 'Authored by https://github.com/' . $payload->{commits}->[1]->{author}->{username} . "\n" . $payload->{commits}->[1]->{url} . "\n[master] " - . $payload->{commits}->[1]->{message}; + . $payload->{commits}->[1]->{message} + . $comment_trailer; # Retrieve the new comment from the bug to make sure it was created correctly $t->get_ok( @@ -258,7 +266,8 @@ $comment_text = 'Authored by https://github.com/' . $payload->{commits}->[0]->{author}{username} . "\n" . $payload->{commits}->[0]->{url} . "\n[master] " - . $payload->{commits}->[0]->{message}; + . $payload->{commits}->[0]->{message} + . $comment_trailer; # Retrieve the new comment from the bug to make sure it was created correctly $t->get_ok( @@ -312,7 +321,8 @@ $comment_text = 'Authored by https://github.com/' . $payload->{commits}->[0]->{author}->{username} . "\n" . $payload->{commits}->[0]->{url} . "\n[master] " - . $payload->{commits}->[0]->{message}; + . $payload->{commits}->[0]->{message} + . $comment_trailer; # Retrieve the new comment from the bug to make sure it was created correctly $t->get_ok( diff --git a/t/github-webhook-bug-visibility.t b/t/github-webhook-bug-visibility.t new file mode 100644 index 0000000000..2d71c9687e --- /dev/null +++ b/t/github-webhook-bug-visibility.t @@ -0,0 +1,419 @@ +#!/usr/bin/env perl +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. +# +# This Source Code Form is "Incompatible With Secondary Licenses", as +# defined by the Mozilla Public License, v. 2.0. +# +# Both github webhook endpoints perform their writes as +# github-automation@bmo.tld, an account elevated into every group, while the bug +# ids they act on come from attacker-influenced request data. push_comment scopes +# referenced bugs to the signing bot's visibility; pull_request keeps its target +# public-only and scopes only stale-attachment cleanup to bot visibility. These +# tests pin those boundaries: +# +# * push_comment must not comment on, resolve, or even acknowledge a bug the +# signing bot cannot see; +# * pull_request must not obsolete -- or comment on -- an existing pull +# request attachment sitting on a bug the signing bot cannot see. +use strict; +use warnings; +use 5.10.1; +use lib qw( . lib local/lib/perl5 ); + +BEGIN { + $ENV{LOG4PERL_CONFIG_FILE} = 'log4perl-t.conf'; + $ENV{BUGZILLA_DISABLE_HOSTAGE} = 1; + $ENV{BUGZILLA_ALLOW_INSECURE_HTTP} = 1; +} + +use Bugzilla::Test::MockLocalconfig (urlbase => 'http://bmo.test'); +use Bugzilla::Test::MockDB; +use Bugzilla::Test::MockParams ( + github_push_comment_enabled => 1, + github_pr_linking_enabled => 1, + github_pr_signature_secret => '', +); +use Bugzilla::Test::Util qw(create_bug create_user issue_api_key); + +use Bugzilla::Attachment; +use Bugzilla::Bug; +use Bugzilla::Constants; +use Bugzilla::Group; +use Bugzilla::User; + +use Digest::SHA qw(hmac_sha256_hex); +use Mojo::JSON qw(encode_json); +use Test2::V0; +use Test::Mojo; + +# super_user() resolves the 'automation@bmo.tld' account, which the mock DB +# does not ship. Without it we would silently run as the anonymous default +# user, and creating the fixture bugs below would die with login_required. +create_user('automation@bmo.tld', '*'); +Bugzilla->set_user(Bugzilla::User->super_user); +my $dbh = Bugzilla->dbh; + +# --------------------------------------------------------------------------- +# Fixtures: the impersonated automation account, a webhook bot with a signing +# key, and a public and a group-restricted bug. +# --------------------------------------------------------------------------- + +# push_comment does Bugzilla::User->check on this account before writing. +create_user('github-automation@bmo.tld', '*'); + +my $BOT_KEY = 'AbCdEfGhIjKlMnOpQrStUvWxYz0123456789abcd'; # varchar(40) +my $bot = create_user('webhook-bot@bmo.test', '*'); +$dbh->do( + 'INSERT INTO user_group_map (user_id, group_id, isbless, grant_type) + VALUES (?, ?, 0, ?)', undef, $bot->id, + Bugzilla::Group->new({name => 'github-webhook-bot'})->id, GRANT_DIRECT +); +issue_api_key('webhook-bot@bmo.test', $BOT_KEY); + +my $sec_group = Bugzilla::Group->create({ + name => 'webhook-visibility-test-sec', + description => 'Group the webhook bot is not a member of', + isbuggroup => 1, +}); + +sub make_bug { + my ($desc, $group_id) = @_; + my $bug = create_bug( + short_desc => $desc, + comment => 'Bug for the push_comment visibility gate tests', + bug_type => 'defect', + assigned_to => 'nobody@mozilla.org', + ); + + # Inserted directly so the bug lands in the group regardless of the product's + # group controls -- only the resulting visibility matters here. + $dbh->do('INSERT INTO bug_group_map (bug_id, group_id) VALUES (?, ?)', + undef, $bug->id, $group_id) + if $group_id; + + return $bug->id; +} + +my $public_bug = make_bug('Public bug the webhook bot can see'); +my $private_bug = make_bug('Restricted bug the webhook bot cannot see', + $sec_group->id); + +# pull_request fixtures. Every bug here has to be created before the first +# request below: creating a bug runs BMO's object_end_of_create hook, which +# calls remote_ip(), and that needs a live Mojo transaction. Once a request has +# completed, the controller left in the request cache has no transaction and +# remote_ip() dies. +# +# A group of its own, so this section does not depend on the membership the +# push_comment tests grant partway through. +my $pr_group = Bugzilla::Group->create({ + name => 'webhook-visibility-test-pr-sec', + description => 'Group the webhook bot is not a member of', + isbuggroup => 1, +}); + +# A second group, for the target-side test at the bottom of the file. Kept +# separate from $pr_group so that test does not depend on the membership the +# cleanup-pass tests grant partway through. +my $pr_target_group = Bugzilla::Group->create({ + name => 'webhook-visibility-test-pr-target', + description => 'Group only the webhook bot is granted', + isbuggroup => 1, +}); + +my $PR_URL = 'https://github.com/x/y/pull/42'; + +my $pr_private_bug + = make_bug('Restricted bug holding a stale PR attachment', $pr_group->id); +my $pr_bug_one = make_bug('Public bug the PR is attached to first'); +my $pr_bug_two = make_bug('Public bug the PR is attached to second'); +my $pr_restricted_target + = make_bug('Restricted bug named in a pull request title', + $pr_target_group->id); + +# The stale attachment the cleanup pass will try to obsolete. Attachment->match() +# keys off the mimetype plus the filename the endpoint derives from the repo name +# and PR number, so these have to line up with the payload posted below. +my $pr_stale_attach_id = Bugzilla::Attachment->create({ + bug => Bugzilla::Bug->check({id => $pr_private_bug}), + creation_ts => $dbh->selectrow_array('SELECT NOW()'), + data => $PR_URL, + description => '[x/y] Some pull request (#42)', + filename => 'github-x_y-42-url.txt', + ispatch => 0, + isprivate => 0, + mimetype => 'text/x-github-pull-request', +})->id; + +# Sanity check the fixture itself: without it the "denied" cases below would +# pass for the wrong reason. +my $bot_user = Bugzilla::User->new({id => $bot->id}); +ok($bot_user->can_see_bug($public_bug), 'bot can see the public bug'); +ok(!$bot_user->can_see_bug($private_bug), 'bot cannot see the restricted bug'); + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +my $t = Test::Mojo->new('Bugzilla::App'); + +# 'x/y' rather than mozilla-mobile/firefox-android so the milestone and status +# flag paths (which call out to the product-versions API) stay out of the way. +sub push_payload { + my (@bug_ids) = @_; + return { + ref => 'refs/heads/main', + repository => {full_name => 'x/y', default_branch => 'main'}, + commits => [ + map { + { + author => {name => 'Foo Bar', username => 'foobar'}, + url => "https://github.com/x/y/commit/abc$_", + message => "Bug $_ - land a fix", + } + } @bug_ids + ], + }; +} + +# Sign and send the exact bytes, since _verify_signature HMACs the raw body. +# no-qe-verify=1 because the qe-verify flag type does not exist in the mock DB. +sub post_push { + my ($payload) = @_; + my $body = encode_json($payload); + return $t->post_ok( + '/rest/github/push_comment?no-qe-verify=1', + { + 'X-Hub-Signature-256' => 'sha256=' . hmac_sha256_hex($body, $BOT_KEY), + 'X-GitHub-Event' => 'push', + 'Content-Type' => 'application/json', + }, + $body + ); +} + +sub bug_status { + my ($bug_id) = @_; + return scalar Bugzilla->dbh->selectrow_array('SELECT bug_status FROM bugs WHERE bug_id = ?', + undef, $bug_id); +} + +sub comment_count { + my ($bug_id) = @_; + return scalar Bugzilla->dbh->selectrow_array( + 'SELECT COUNT(*) FROM longdescs WHERE bug_id = ?', undef, $bug_id); +} + +# The rendered message is wrapped by the template, so match across whitespace. +my $BUG_NOT_FOUND = qr/did\s+not\s+contain\s+a\s+valid/; + +# --------------------------------------------------------------------------- +# A bug the signing bot can see is processed exactly as before -- the gate must +# not break the normal case. +# --------------------------------------------------------------------------- +post_push(push_payload($public_bug))->status_is(200) + ->json_is('/error' => 0) + ->json_has("/bugs/$public_bug/id", 'visible bug is commented on'); + +is(bug_status($public_bug), 'RESOLVED', 'visible bug was resolved'); + +# The write still happens as the elevated automation account. +is( + scalar Bugzilla->dbh->selectrow_array( + 'SELECT p.login_name FROM longdescs l JOIN profiles p ON p.userid = l.who + WHERE l.bug_id = ? ORDER BY l.comment_id DESC LIMIT 1', undef, $public_bug + ), + 'github-automation@bmo.tld', + 'comment is still attributed to the automation account' +); + +# --------------------------------------------------------------------------- +# A bug the signing bot cannot see is left completely alone, and the response +# is the same one an unparseable bug id produces -- so it cannot be used as an +# oracle for the existence of confidential bug ids. +# --------------------------------------------------------------------------- +my $private_comments = comment_count($private_bug); +my $private_status = bug_status($private_bug); + +my $denied = post_push(push_payload($private_bug)); +$denied->status_is(400)->json_is('/error' => 1); +$denied->json_like('/message' => $BUG_NOT_FOUND, 'invisible bug is rejected'); +$denied->json_hasnt("/bugs/$private_bug", + 'response does not name the invisible bug'); + +is(comment_count($private_bug), $private_comments, + 'no comment was added to the invisible bug'); +is(bug_status($private_bug), $private_status, + 'invisible bug status is unchanged'); + +# --------------------------------------------------------------------------- +# One push touching both bugs: the visible one is processed, the invisible one +# is dropped. A single unreadable bug id must not fail the whole delivery, or +# GitHub would retry it forever. +# --------------------------------------------------------------------------- +$private_comments = comment_count($private_bug); + +# Reopen so there is an observable change to make on the public bug. +Bugzilla->dbh->do('UPDATE bugs SET bug_status = ?, resolution = ? WHERE bug_id = ?', + undef, 'CONFIRMED', '', $public_bug); + +my $mixed = post_push(push_payload($public_bug, $private_bug)); +$mixed->status_is(200)->json_is('/error' => 0); +$mixed->json_has("/bugs/$public_bug/id", 'mixed push: visible bug is processed'); +$mixed->json_hasnt("/bugs/$private_bug", 'mixed push: invisible bug is dropped'); + +is(comment_count($private_bug), $private_comments, + 'mixed push left the invisible bug untouched'); +is(bug_status($public_bug), 'RESOLVED', 'mixed push resolved the visible bug'); + +# --------------------------------------------------------------------------- +# The gate is scoped to the signing bot, not hardcoded to public bugs: granting +# the bot the group makes the same request succeed. This is how an operator +# deliberately gives a webhook reach into confidential bugs. +# --------------------------------------------------------------------------- +Bugzilla->dbh->do( + 'INSERT INTO user_group_map (user_id, group_id, isbless, grant_type) + VALUES (?, ?, 0, ?)', undef, $bot->id, $sec_group->id, GRANT_DIRECT +); +Bugzilla->memcached->clear_all; + +post_push(push_payload($private_bug))->status_is(200) + ->json_is('/error' => 0) + ->json_has("/bugs/$private_bug/id", + 'bot in the group can now reach the restricted bug'); + +is(bug_status($private_bug), 'RESOLVED', + 'restricted bug was resolved once the bot had access'); + +# --------------------------------------------------------------------------- +# pull_request: the cleanup pass that obsoletes the same pull request +# attachment on other bugs runs as the all-groups automation account, so it is +# gated on the signing bot too. A bug the bot cannot see must keep its +# attachment and gain no comment. +# --------------------------------------------------------------------------- +ok(!Bugzilla::User->new({id => $bot->id})->can_see_bug($pr_private_bug), + 'bot cannot see the bug holding the stale PR attachment'); + +sub post_pull_request { + my ($bug_id) = @_; + my $body = encode_json({ + action => 'opened', + repository => {full_name => 'x/y'}, + pull_request => + {html_url => $PR_URL, title => "Bug $bug_id - do a thing", number => 42}, + }); + return $t->post_ok( + '/rest/github/pull_request', + { + 'X-Hub-Signature-256' => 'sha256=' . hmac_sha256_hex($body, $BOT_KEY), + 'X-GitHub-Event' => 'pull_request', + 'Content-Type' => 'application/json', + }, + $body + ); +} + +sub is_obsolete { + my ($attach_id) = @_; + return scalar Bugzilla->dbh->selectrow_array( + 'SELECT isobsolete FROM attachments WHERE attach_id = ?', undef, $attach_id); +} + +sub pr_attachment_count { + my ($bug_id) = @_; + return scalar Bugzilla->dbh->selectrow_array( + 'SELECT COUNT(*) FROM attachments WHERE bug_id = ? AND mimetype = ?', + undef, $bug_id, 'text/x-github-pull-request'); +} + +my $pr_private_comments = comment_count($pr_private_bug); + +post_pull_request($pr_bug_one)->status_is(200)->json_is('/error' => 0); + +ok(!is_obsolete($pr_stale_attach_id), + 'attachment on the invisible bug was not obsoleted'); +is(comment_count($pr_private_bug), $pr_private_comments, + 'no "moved to bug" comment was added to the invisible bug'); + +# Sanity check that the request did its normal work, otherwise the assertions +# above would pass even if the endpoint had bailed out before the cleanup pass. +is(pr_attachment_count($pr_bug_one), 1, + 'the pull request was still attached to the visible bug'); + +# --------------------------------------------------------------------------- +# As with push_comment, the gate follows the signing bot: once the bot can see +# the bug, the same cleanup pass does obsolete the stale attachment. +# --------------------------------------------------------------------------- +$dbh->do( + 'INSERT INTO user_group_map (user_id, group_id, isbless, grant_type) + VALUES (?, ?, 0, ?)', undef, $bot->id, $pr_group->id, GRANT_DIRECT +); +Bugzilla->memcached->clear_all; + +post_pull_request($pr_bug_two)->status_is(200)->json_is('/error' => 0); + +ok(is_obsolete($pr_stale_attach_id), + 'attachment is obsoleted once the bot can see the bug'); +cmp_ok(comment_count($pr_private_bug), '>', $pr_private_comments, + 'the "moved to bug" comment is added once the bot can see the bug'); + +# --------------------------------------------------------------------------- +# The two visibility gates in pull_request are deliberately asymmetric, and +# this pins the half the tests above do not reach. +# +# The *cleanup* pass (tested above) is gated on the signing bot, because it +# touches bugs the request never named. The *target* bug -- the one in the pull +# request title -- is gated on Bugzilla->user, which is anonymous on /rest, so +# a pull request can only ever be attached to a publicly visible bug. Widening +# that to the signing bot would let a GitHub PR title pull a confidential bug +# into an externally-visible attachment, so it is not a check the bot's group +# membership should be able to unlock. +# +# Granting the bot the group is the point: every assertion below must hold even +# though the signing bot can see the bug perfectly well. +# --------------------------------------------------------------------------- +$dbh->do( + 'INSERT INTO user_group_map (user_id, group_id, isbless, grant_type) + VALUES (?, ?, 0, ?)', undef, $bot->id, $pr_target_group->id, GRANT_DIRECT +); +Bugzilla->memcached->clear_all; + +ok(Bugzilla::User->new({id => $bot->id})->can_see_bug($pr_restricted_target), + 'signing bot can see the restricted target bug'); + +my $target_comments = comment_count($pr_restricted_target); + +my $target = post_pull_request($pr_restricted_target); +$target->status_is(200)->json_is('/error' => 1); +$target->json_like('/message' => qr/not\s+publicly\s+visible/, + 'restricted target bug is rejected even though the bot can see it'); + +is(pr_attachment_count($pr_restricted_target), + 0, 'no pull request was attached to the restricted target bug'); +is(comment_count($pr_restricted_target), + $target_comments, 'no comment was added to the restricted target bug'); + +# --------------------------------------------------------------------------- +# Disabling the bot account must disable its webhook keys too. _verify_signature +# queries user_api_keys directly rather than going through Bugzilla::Auth, so it +# has to apply the is_enabled check itself -- otherwise the usual way to shut +# off a compromised or retired bot (disable the account) would leave every one +# of its keys signing valid webhooks. Kept last because it makes $BOT_KEY dead. +# --------------------------------------------------------------------------- +my $before_comments = comment_count($public_bug); + +$dbh->do('UPDATE profiles SET is_enabled = 0, disabledtext = ? WHERE userid = ?', + undef, 'retired bot', $bot->id); +Bugzilla->memcached->clear_all; + +my $disabled = post_push(push_payload($public_bug)); +$disabled->status_is(400)->json_is('/error' => 1); +$disabled->json_like('/message' => qr/signature/i, + 'key of a disabled bot no longer authenticates'); + +is(comment_count($public_bug), $before_comments, + 'disabled bot request made no change to the bug'); + +done_testing(); diff --git a/template/en/default/admin/params/github.html.tmpl b/template/en/default/admin/params/github.html.tmpl index f7b34b21a1..a8fa671c95 100644 --- a/template/en/default/admin/params/github.html.tmpl +++ b/template/en/default/admin/params/github.html.tmpl @@ -35,8 +35,11 @@ "under Preferences > API Keys, and use that API key as the GitHub " _ "webhook secret. Individual keys can then be revoked without affecting " _ "other integrations. Clear this field once all webhooks are migrated. " _ - "Note: Bugzilla API keys also grant full REST API access as the bot " _ - "account, so the bot should be least-privileged and the key treated as a credential." + "Note: a 'github-webhook-bot' API key lets these endpoints write as " _ + "'github-automation@bmo.tld'. push_comment is limited to the bugs the " _ + "signing bot account can see; pull_request only attaches to publicly " _ + "visible bugs. So only add trusted bots to the group, keep them out of " _ + "security groups they do not need, and treat each key as a credential." github_push_comment_enabled => "Enable the ability for Github pushes to add a comment to a related bug report." From f04238549fbabb61c039dce34fdbc653ae06b4b3 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Wed, 23 Sep 2026 16:21:55 -0400 Subject: [PATCH 2/2] Potential fix for pull request finding 'Batch bug visibility checks to avoid N+1 queries' Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- Bugzilla/API/V1/Github.pm | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Bugzilla/API/V1/Github.pm b/Bugzilla/API/V1/Github.pm index e11f03d097..0dd7d0aa80 100644 --- a/Bugzilla/API/V1/Github.pm +++ b/Bugzilla/API/V1/Github.pm @@ -320,7 +320,9 @@ sub push_comment { # The scoping identity is resolved by _verify_signature. my $webhook_user = $webhook_auth->{user}; - my @denied_bugs = grep { !$webhook_user->can_see_bug($_) } keys %update_bugs; + my %visible_bugs + = map { $_ => 1 } @{$webhook_user->visible_bugs([keys %update_bugs])}; + my @denied_bugs = grep { !$visible_bugs{$_} } keys %update_bugs; if (@denied_bugs) { delete @update_bugs{@denied_bugs}; WARN(sprintf(