diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm index 9a84fb4467..9145b73f1b 100644 --- a/Bugzilla/Bug.pm +++ b/Bugzilla/Bug.pm @@ -1605,14 +1605,34 @@ sub _check_bug_file_loc { my ($invocant, $url) = @_; return '' if !defined $url; $url = trim($url); + return '' if $url eq ''; # On bug entry, if bug_file_loc is "http://", the default, use an - # empty value instead. However, on bug editing people can set that - # back if they *really* want to. + # empty value instead. On bug editing, "http://" is not generally + # accepted by the safe URL validation below; it is only allowed if it + # is already stored on the bug and remains unchanged. if (!ref $invocant && $url eq 'http://') { - $url = ''; + return ''; } - return $url; + + # Allowlist scheme check — mirrors the is_safe_url template helper so + # what passes here is exactly what renders as a clickable link. + my $safe_url_regexp = SAFE_URL_REGEXP(); + return $url if $url =~ /^$safe_url_regexp$/; + + # Colon-free relative path / local reference (matches is_safe_url + # second branch in Bugzilla/Template.pm). + return $url if $url =~ /^[^\s<>\":]+[\w\/]$/i; + + # Allow an already-stored unsafe value to pass through unchanged so that + # edits to other fields on the bug are not blocked. Unsafe values are + # rendered as plain text by templates; a DB cleanup script will scrub them. + if (ref $invocant) { + my $current = $invocant->{bug_file_loc} // ''; + return $url if $url eq $current; + } + + ThrowUserError('bug_file_loc_invalid', {url => $url}); } sub _check_bug_status { diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm index 6155d985ff..c4c86a09ac 100644 --- a/Bugzilla/Constants.pm +++ b/Bugzilla/Constants.pm @@ -171,6 +171,7 @@ use Memoize; MAX_STS_AGE SAFE_PROTOCOLS + SAFE_URL_REGEXP LEGAL_CONTENT_TYPES MIN_SMALLINT @@ -490,6 +491,11 @@ use constant SAFE_PROTOCOLS => ( 'telnet', 'wais' ); +sub SAFE_URL_REGEXP { + my $safe_protocols = join('|', SAFE_PROTOCOLS); + return qr/($safe_protocols):[^:\s<>\"][^\s<>\"]+[\w\/]/i; +} + # Valid MIME types for attachments. use constant LEGAL_CONTENT_TYPES => ( 'application', 'audio', 'font', 'image', 'message', diff --git a/Bugzilla/Template.pm b/Bugzilla/Template.pm index 4e7afb4d5e..f51496bce9 100644 --- a/Bugzilla/Template.pm +++ b/Bugzilla/Template.pm @@ -52,11 +52,6 @@ use constant FORMAT_2_SIZE => [19, 55]; our %SHARED_PROVIDERS; our $COLOR_QUOTES = 1; -# Pseudo-constant. -sub SAFE_URL_REGEXP { - my $safe_protocols = join('|', SAFE_PROTOCOLS); - return qr/($safe_protocols):[^:\s<>\"][^\s<>\"]+[\w\/]/i; -} sub is_safe_url { my $url = shift; diff --git a/Bugzilla/WebService/Constants.pm b/Bugzilla/WebService/Constants.pm index 10c42df9d1..349b2d83dc 100644 --- a/Bugzilla/WebService/Constants.pm +++ b/Bugzilla/WebService/Constants.pm @@ -126,9 +126,10 @@ use constant WS_ERROR_CODE => { comment_tag_too_long => 127, comment_tag_too_short => 128, - # See Also errors - bug_url_invalid => 112, - bug_url_too_long => 112, + # Bug Url errors + bug_url_invalid => 112, + bug_url_too_long => 112, + bug_file_loc_invalid => 112, # Insidergroup Errors user_not_insider => 113, diff --git a/extensions/BugModal/template/en/default/bug_modal/activity_stream.html.tmpl b/extensions/BugModal/template/en/default/bug_modal/activity_stream.html.tmpl index aae369b866..c7413b062f 100644 --- a/extensions/BugModal/template/en/default/bug_modal/activity_stream.html.tmpl +++ b/extensions/BugModal/template/en/default/bug_modal/activity_stream.html.tmpl @@ -440,9 +440,12 @@ CASE 'bug_file_loc'; %] + [% IF is_safe_url(value) %] [% value FILTER truncate(40) FILTER html %] + class="bug-url">[% value FILTER truncate(40) FILTER html %] + [% ELSE %] + [% value FILTER truncate(40) FILTER html %] + [% END %] [% CASE 'see_also'; diff --git a/extensions/BugModal/template/en/default/bug_modal/edit.html.tmpl b/extensions/BugModal/template/en/default/bug_modal/edit.html.tmpl index f61783f92f..02b25ac646 100755 --- a/extensions/BugModal/template/en/default/bug_modal/edit.html.tmpl +++ b/extensions/BugModal/template/en/default/bug_modal/edit.html.tmpl @@ -1530,10 +1530,17 @@ END; %]
- - [%~ link_text FILTER html ~%] - + [% IF is_safe_url(bug.bug_file_loc) %] + + [%~ link_text FILTER html ~%] + + [% ELSE %] + + [%~ link_text FILTER html ~%] + + [% END %]
[% END %] diff --git a/extensions/BugModal/web/bug_modal.js b/extensions/BugModal/web/bug_modal.js index bdc0416846..99d62b3bb3 100644 --- a/extensions/BugModal/web/bug_modal.js +++ b/extensions/BugModal/web/bug_modal.js @@ -280,25 +280,6 @@ $(function() { $('#attachments tr.attach-obsolete').toggle(); }); - // URL --> unsafe warning - $('.bug-url') - .click(function(event) { - var that = $(this); - event.stopPropagation(); - if (!that.data('safe')) { - event.preventDefault(); - if (confirm('This is considered an unsafe URL and could possibly be harmful. ' + - 'The full URL is:\n\n' + that.attr('href') + '\n\nContinue?')) - { - try { - window.open(that.attr('href')); - } catch(ex) { - alert('Malformed URL'); - } - } - } - }); - // top btn $('#top-btn') .click(function(event) { diff --git a/extensions/InlineHistory/template/en/default/hook/bug/comments-aftercomments.html.tmpl b/extensions/InlineHistory/template/en/default/hook/bug/comments-aftercomments.html.tmpl index 89e3fc57cf..50efe33d1b 100644 --- a/extensions/InlineHistory/template/en/default/hook/bug/comments-aftercomments.html.tmpl +++ b/extensions/InlineHistory/template/en/default/hook/bug/comments-aftercomments.html.tmpl @@ -137,10 +137,12 @@ [% ELSIF change.buglist %] [% value FILTER bug_list_link FILTER js %] [% ELSIF change.fieldname == 'bug_file_loc' %] - [%~%] + [%~ IF is_safe_url(value) ~%] + [%~%] [%~%][% value FILTER ih_short_value FILTER html FILTER js %] + [%~ ELSE ~%] + [%~%][% value FILTER ih_short_value FILTER html FILTER js %] + [%~ END ~%] [% ELSIF change.fieldname == 'see_also' %] [% FOREACH see_also = value %] [% IF see_also.bug_id %] diff --git a/qa/t/webservice_bug_create.t b/qa/t/webservice_bug_create.t index 4aa5ae1750..58a4c5d9be 100644 --- a/qa/t/webservice_bug_create.t +++ b/qa/t/webservice_bug_create.t @@ -13,7 +13,7 @@ use strict; use warnings; use lib qw(lib ../../lib ../../local/lib/perl5); use Storable qw(dclone); -use Test::More tests => 305; +use Test::More tests => 323; use QA::Util; use QA::Tests qw(create_bug_fields PRIVATE_BUG_USER); @@ -143,6 +143,20 @@ my $fields = { invalid => {faultstring => 'you are not allowed to.+comments.+private', value => 1,} }, + url => { + javascript => { + faultstring => 'is not a valid URL for the URL field', + value => 'javascript:alert(document.domain)//', + }, + data => { + faultstring => 'is not a valid URL for the URL field', + value => 'data:text/html,', + }, + vbscript => { + faultstring => 'is not a valid URL for the URL field', + value => 'vbscript:msgbox(1)', + }, + }, }; $jsonrpc_get->bz_call_fail( diff --git a/template/en/default/bug/edit.html.tmpl b/template/en/default/bug/edit.html.tmpl index db15c5f863..9c09bb6b2d 100644 --- a/template/en/default/bug/edit.html.tmpl +++ b/template/en/default/bug/edit.html.tmpl @@ -627,19 +627,16 @@ [% IF bug.check_can_change_field("bug_file_loc", 0, 1) %] - - [% bug.bug_file_loc FILTER truncate(40) FILTER html %] + [% IF is_safe_url(bug.bug_file_loc) %] + + [%~ bug.bug_file_loc FILTER truncate(40) FILTER html ~%] + + [% ELSE %] + + [%~ bug.bug_file_loc FILTER truncate(40) FILTER html ~%] + + [% END %] (edit) [% END %] diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl index 81878be0c1..bc8f138738 100644 --- a/template/en/default/global/user-error.html.tmpl +++ b/template/en/default/global/user-error.html.tmpl @@ -312,6 +312,12 @@ [% type FILTER html %] [% END %]. + [% ELSIF error == "bug_file_loc_invalid" %] + [% title = "Invalid URL" %] + [% url FILTER html %] is not a valid URL for the URL field. + Only safe protocols (http, https, ftp, and similar) are allowed. + Protocols such as "javascript:", "data:", and "vbscript:" are not permitted. + [% ELSIF error == "bug_url_invalid" %] [% title = "Invalid $terms.Bug URL" %] [% url FILTER html %] is not a valid URL to [% terms.abug %].