Bug 2035372

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 {

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',

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;

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,

extensions/BugModal/template/en/default/bug_modal/activity_stream.html.tmpl

@@ -440,9 +440,12 @@
 
       CASE 'bug_file_loc';
         %]
+        [% IF is_safe_url(value) %]
         <a href="[% value FILTER html %]" target="_blank" rel="nofollow noreferrer"
-           class="bug-url" data-safe="[% is_safe_url(value) ? 1 : 0 %]"
-        >[% value FILTER truncate(40) FILTER html %]</a>
+           class="bug-url">[% value FILTER truncate(40) FILTER html %]</a>
+        [% ELSE %]
+        <span class="bug-url" title="[% value FILTER html %]">[% value FILTER truncate(40) FILTER html %]</span>
+        [% END %]
         [%
 
       CASE 'see_also';

extensions/BugModal/template/en/default/bug_modal/edit.html.tmpl

@@ -1530,10 +1530,17 @@
   END;
 %]
   <div class="link">
-    <a href="[% bug.bug_file_loc FILTER html %]" target="_blank" rel="nofollow noreferrer"
-      title="[% bug.bug_file_loc FILTER truncate(256) FILTER html %]"
-      class="bug-url" data-safe="[% is_safe_url(bug.bug_file_loc) ? 1 : 0 %]">
-      [%~ link_text FILTER html ~%]
-    </a>
+    [% IF is_safe_url(bug.bug_file_loc) %]
+      <a href="[% bug.bug_file_loc FILTER html %]" target="_blank" rel="nofollow noreferrer"
+        title="[% bug.bug_file_loc FILTER truncate(256) FILTER html %]"
+        class="bug-url">
+        [%~ link_text FILTER html ~%]
+      </a>
+    [% ELSE %]
+      <span class="bug-url"
+        title="[% bug.bug_file_loc FILTER truncate(256) FILTER html %]">
+        [%~ link_text FILTER html ~%]
+      </span>
+    [% END %]
   </div>
 [% END %]

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) {

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' %]
-    [%~%]<a href="[% value FILTER html FILTER js %]" target="_blank" rel="nofollow noreferrer"
-      [%~ ' onclick="return inline_history.confirmUnsafeUrl(this.href)"'
-          UNLESS is_safe_url(value) %]>
+    [%~ IF is_safe_url(value) ~%]
+    [%~%]<a href="[% value FILTER html FILTER js %]" target="_blank" rel="nofollow noreferrer">
     [%~%][% value FILTER ih_short_value FILTER html FILTER js %]</a>
+    [%~ ELSE ~%]
+    [%~%]<span title="[% value FILTER html FILTER js %]">[% value FILTER ih_short_value FILTER html FILTER js %]</span>
+    [%~ END ~%]
   [% ELSIF change.fieldname == 'see_also' %]
     [% FOREACH see_also = value %]
       [% IF see_also.bug_id %]

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,<script>alert(1)</script>',
+    },
+    vbscript => {
+      faultstring => 'is not a valid URL for the URL field',
+      value       => 'vbscript:msgbox(1)',
+    },
+  },
 };
 
 $jsonrpc_get->bz_call_fail(

template/en/default/bug/edit.html.tmpl

@@ -627,19 +627,16 @@
     <td>
       [% IF bug.check_can_change_field("bug_file_loc", 0, 1) %]
         <span id="bz_url_edit_container" class="bz_default_hidden">
-          <a href="[% bug.bug_file_loc FILTER html %]" target="_blank"
-             rel="nofollow noreferrer" title="[% bug.bug_file_loc FILTER html %]"
-            [% IF NOT is_safe_url(bug.bug_file_loc) %]
-              onclick="
-                try {
-                  return confirm('This is considered an unsafe URL and could possibly be harmful. ' +
-                    'The full URL is:\n\n[% bug.bug_file_loc FILTER js FILTER html %]\n\nContinue?');
-                } catch(e) {
-                  return false;
-                }
-              "
-             [% END %]>
-          [% bug.bug_file_loc FILTER truncate(40) FILTER html %]</a>
+          [% IF is_safe_url(bug.bug_file_loc) %]
+            <a href="[% bug.bug_file_loc FILTER html %]" target="_blank"
+               rel="nofollow noreferrer" title="[% bug.bug_file_loc FILTER html %]">
+              [%~ bug.bug_file_loc FILTER truncate(40) FILTER html ~%]
+            </a>
+          [% ELSE %]
+            <span title="[% bug.bug_file_loc FILTER html %]">
+              [%~ bug.bug_file_loc FILTER truncate(40) FILTER html ~%]
+            </span>
+          [% END %]
           (<a href="#" id="bz_url_edit_action">edit</a>)
         </span>
       [% END %]

template/en/default/global/user-error.html.tmpl

@@ -312,6 +312,12 @@
       <strong>[% type FILTER html %]</strong>
     [% END %].
 
+  [% ELSIF error == "bug_file_loc_invalid" %]
+    [% title = "Invalid URL" %]
+    <code>[% url FILTER html %]</code> 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" %]
     <code>[% url FILTER html %]</code> is not a valid URL to [% terms.abug %].