diff --git a/extensions/BMO/Extension.pm b/extensions/BMO/Extension.pm index 6365dcf02f..ed6005424e 100755 --- a/extensions/BMO/Extension.pm +++ b/extensions/BMO/Extension.pm @@ -1197,6 +1197,12 @@ sub attachment_process_data { } if (my $detected = _detect_attached_url($url)) { + + # Some detected content types are reserved for automation/non-user flows + # (for example, specialized URL attachments) and must not be auto-assigned + # from this user-submitted plain-text URL path. + return if !$detected->{can_user_set}; + $attributes->{mimetype} = $detected->{content_type}; $attributes->{ispatch} = 0; } diff --git a/extensions/BMO/lib/Data.pm b/extensions/BMO/lib/Data.pm index 777f5eb67e..05e6696dbe 100644 --- a/extensions/BMO/lib/Data.pm +++ b/extensions/BMO/lib/Data.pm @@ -46,6 +46,16 @@ our $flag_pronoun_re = do { qr/^cf_(status|tracking)_(firefox|thunderbird)_($channels_re)$/; }; +sub phabricator_url_re { + my $params = Bugzilla->params; + return qr/(?!)/ unless $params->{phabricator_enabled}; + + my $phab_uri = $params->{phabricator_base_uri}; + return qr/(?!)/ unless $phab_uri; + + return qr/^\Q${phab_uri}\ED\d+$/i; +} + # Creating an attachment whose contents is a URL matching one of these regexes # will result in the user being redirected to that URL when viewing the # attachment. @@ -55,6 +65,14 @@ our %autodetect_attach_urls = ( regex => qr#^https://github\.com/[^/]+/[^/]+/pull/\d+/?$#i, content_type => 'text/x-github-pull-request', can_review => 1, + can_user_set => 1, + }, + phabricator => { + title => 'Phabricator', + regex => \&phabricator_url_re, + content_type => 'text/x-phabricator-request', + can_review => 1, + can_user_set => 0, }, google_docs => { title => 'Google Doc', @@ -62,6 +80,7 @@ our %autodetect_attach_urls = ( qr#^https://docs\.google\.com/(?:document|spreadsheets|presentation)/d/#i, content_type => 'text/x-google-doc', can_review => 0, + can_user_set => 1, }, ); diff --git a/qa/config/generate_test_data.pl b/qa/config/generate_test_data.pl index a08f598c46..e0e9acc7f6 100644 --- a/qa/config/generate_test_data.pl +++ b/qa/config/generate_test_data.pl @@ -93,6 +93,8 @@ BEGIN defaultseverity => '--', # BMO CHANGE timetrackinggroup => 'editbugs', # BMO CHANGE letsubmitterchoosepriority => 1, # BMO CHANGE + phabricator_enabled => 1, + phabricator_base_uri => 'https://phabricator.test.example.com/', ); my $params_modified; diff --git a/qa/t/rest_attachment_redirect.t b/qa/t/rest_attachment_redirect.t new file mode 100644 index 0000000000..44d3490d30 --- /dev/null +++ b/qa/t/rest_attachment_redirect.t @@ -0,0 +1,172 @@ +#!/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. +# +# Tests that attachments whose data is a recognized URL (GitHub PR, Google +# Doc, Phabricator revision) cause attachment.cgi to issue an HTTP redirect +# to that URL rather than serving the raw text. Regression guard for the +# security fix that added the can_user_set / phabricator URL-detection changes. +use strict; +use warnings; +use 5.10.1; +use lib qw(lib ../../lib ../../local/lib/perl5); + +use Bugzilla; +use Bugzilla::Attachment; +use Bugzilla::Bug; +use Bugzilla::User; +use QA::Util qw(get_config); +use Bugzilla::Extension::PhabBugz::Constants qw(PHAB_CONTENT_TYPE); + +use MIME::Base64 qw(encode_base64); +use Test::Mojo; +use Test::More; + +my $config = get_config(); +my $admin_api_key = $config->{admin_user_api_key}; +my $url = Bugzilla->localconfig->urlbase; + +# Do not follow redirects so we can assert the 302 and Location header +# directly rather than chasing the remote URL. +my $t = Test::Mojo->new(); +$t->ua->max_redirects(0); + +########################################################################## +# Section 1: Create a bug to host the test attachments +########################################################################## + +my $new_bug = { + product => 'Firefox', + component => 'General', + summary => 'Test bug for attachment URL redirect', + type => 'defect', + version => 'unspecified', + severity => 'normal', + description => 'This bug exists to test URL-attachment redirect behaviour.', +}; + +$t->post_ok( + $url . 'rest/bug' => {'X-Bugzilla-API-Key' => $admin_api_key} => json => + $new_bug)->status_is(200)->json_has('/id'); + +my $bug_id = $t->tx->res->json->{id}; +ok($bug_id, "Created test bug $bug_id"); + +########################################################################## +# Helper: create an attachment via REST and return its id +########################################################################## + +sub create_url_attachment { + my ($raw_url, $label) = @_; + + my $att = { + summary => "URL attachment: $label", + content_type => 'text/plain', + data => encode_base64($raw_url), + file_name => 'url-attachment.txt', + is_patch => 0, + is_private => 0, + }; + + $t->post_ok( + $url . "rest/bug/$bug_id/attachment" => + {'X-Bugzilla-API-Key' => $admin_api_key} => json => $att) + ->status_is(201); + + my ($id) = keys %{$t->tx->res->json->{attachments}}; + return $id; +} + +########################################################################## +# Section 2: GitHub PR URL attachment → must redirect to GitHub +########################################################################## + +my $github_url = 'https://github.com/mozilla/firefox/pull/1234'; +my $github_att_id = create_url_attachment($github_url, 'GitHub PR'); + +ok($github_att_id, "GitHub PR attachment created (id $github_att_id)"); + +$t->get_ok("${url}attachment.cgi?id=${github_att_id}" => + {'X-Bugzilla-API-Key' => $admin_api_key}) + ->status_is(302, 'GitHub PR attachment.cgi returns 302') + ->header_is('Location', $github_url, + 'GitHub PR redirect Location is the PR URL'); + +# Explicit content_type param must suppress the redirect (raw serve path) +$t->get_ok( + "${url}attachment.cgi?id=${github_att_id}&content_type=text/plain" => + {'X-Bugzilla-API-Key' => $admin_api_key}) + ->status_is(200, 'Explicit content_type param suppresses redirect'); + +########################################################################## +# Section 3: Google Docs URL attachment → must redirect to Google Docs +########################################################################## + +my $gdoc_url = 'https://docs.google.com/document/d/abc123def456/edit'; +my $gdoc_att_id = create_url_attachment($gdoc_url, 'Google Doc'); + +ok($gdoc_att_id, "Google Doc attachment created (id $gdoc_att_id)"); + +$t->get_ok("${url}attachment.cgi?id=${gdoc_att_id}" => + {'X-Bugzilla-API-Key' => $admin_api_key}) + ->status_is(302, 'Google Doc attachment.cgi returns 302') + ->header_is('Location', $gdoc_url, + 'Google Doc redirect Location is the doc URL'); + +$t->get_ok( + "${url}attachment.cgi?id=${gdoc_att_id}&content_type=text/plain" => + {'X-Bugzilla-API-Key' => $admin_api_key}) + ->status_is(200, 'Explicit content_type param suppresses redirect'); + +########################################################################## +# Section 4: Phabricator URL attachment → must redirect to Phabricator +# +# Users cannot create text/x-phabricator-request attachments via the REST +# API (blocked by PhabBugz::Extension::object_before_create). Mirror the +# production code path from PhabBugz::Util which sets +# allow_phab_revision_attachment in the request cache. +########################################################################## + +SKIP: { + my $phab_enabled = Bugzilla->params->{phabricator_enabled}; + my $phab_base_uri = Bugzilla->params->{phabricator_base_uri}; + + skip 'phabricator_enabled or phabricator_base_uri not set', 2 + unless $phab_enabled && $phab_base_uri; + + (my $phab_base = $phab_base_uri) =~ s{/?$}{/}; + my $phab_url = "${phab_base}D9999"; + + my $admin_user = Bugzilla::User->check($config->{admin_user_login}); + Bugzilla->set_user($admin_user); + + my $bug = Bugzilla::Bug->new($bug_id); + my $request_cache = Bugzilla->request_cache; + + local $request_cache->{allow_phab_revision_attachment} = 1; + + my $phab_att = Bugzilla::Attachment->create({ + bug => $bug, + data => $phab_url, + description => 'Phabricator revision D9999', + filename => 'phabricator-D9999-url.txt', + ispatch => 0, + isprivate => 0, + mimetype => PHAB_CONTENT_TYPE, + }); + + ok($phab_att->id, + "Phabricator attachment created directly (id " . $phab_att->id . ")"); + + $t->get_ok("${url}attachment.cgi?id=" . $phab_att->id => + {'X-Bugzilla-API-Key' => $admin_api_key}) + ->status_is(302, 'Phabricator attachment.cgi returns 302') + ->header_is('Location', $phab_url, + 'Phabricator redirect Location is the revision URL'); +} + +done_testing();