diff options
| -rw-r--r-- | Bugzilla/CGI.pm | 24 | ||||
| -rw-r--r-- | Bugzilla/Config/Attachment.pm | 7 | ||||
| -rw-r--r-- | Bugzilla/Util.pm | 14 | ||||
| -rwxr-xr-x | attachment.cgi | 104 | ||||
| -rw-r--r-- | template/en/default/admin/params/attachment.html.tmpl | 18 | ||||
| -rw-r--r-- | template/en/default/attachment/edit.html.tmpl | 35 |
6 files changed, 162 insertions, 40 deletions
diff --git a/Bugzilla/CGI.pm b/Bugzilla/CGI.pm index 68d3ef69d..d7934f89b 100644 --- a/Bugzilla/CGI.pm +++ b/Bugzilla/CGI.pm @@ -71,6 +71,18 @@ sub new { # Send appropriate charset $self->charset(Bugzilla->params->{'utf8'} ? 'UTF-8' : ''); + # Redirect to urlbase/sslbase if we are not viewing an attachment. + if (use_attachbase() && i_am_cgi()) { + my $cgi_file = $self->url('-path_info' => 0, '-query' => 0, '-relative' => 1); + $cgi_file =~ s/\?$//; + my $urlbase = Bugzilla->params->{'urlbase'}; + my $sslbase = Bugzilla->params->{'sslbase'}; + my $path_regexp = $sslbase ? qr/^(\Q$urlbase\E|\Q$sslbase\E)/ : qr/^\Q$urlbase\E/; + if ($cgi_file ne 'attachment.cgi' && $self->self_url !~ /$path_regexp/) { + $self->redirect_to_urlbase; + } + } + # Check for errors # All of the Bugzilla code wants to do this, so do it here instead of # in each script @@ -351,6 +363,14 @@ sub require_https { exit; } +# Redirect to the urlbase version of the current URL. +sub redirect_to_urlbase { + my $self = shift; + my $path = $self->url('-path_info' => 1, '-query' => 1, '-relative' => 1); + print $self->redirect('-location' => correct_urlbase() . $path); + exit; +} + 1; __END__ @@ -421,6 +441,10 @@ If the client is using XMLRPC, it will not retain the QUERY_STRING since XMLRPC It takes an optional argument which will be used as the base URL. If $baseurl is not provided, the current URL is used. +=item C<redirect_to_urlbase> + +Redirects from the current URL to one prefixed by the urlbase parameter. + =back =head1 SEE ALSO diff --git a/Bugzilla/Config/Attachment.pm b/Bugzilla/Config/Attachment.pm index 72ad29a2d..17dbe4068 100644 --- a/Bugzilla/Config/Attachment.pm +++ b/Bugzilla/Config/Attachment.pm @@ -40,6 +40,13 @@ $Bugzilla::Config::Attachment::sortkey = "025"; sub get_param_list { my $class = shift; my @param_list = ( + { + name => 'attachment_base', + type => 't', + default => '', + checker => \&check_urlbase + }, + { name => 'allow_attachment_deletion', type => 'b', diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm index 01f824c5b..951c4df3c 100644 --- a/Bugzilla/Util.pm +++ b/Bugzilla/Util.pm @@ -36,7 +36,7 @@ use base qw(Exporter); html_quote url_quote xml_quote css_class_quote html_light_quote url_decode i_am_cgi get_netaddr correct_urlbase - lsearch ssl_require_redirect + lsearch ssl_require_redirect use_attachbase diff_arrays diff_strings trim wrap_hard wrap_comment find_wrap_point format_time format_time_decimal validate_date @@ -294,6 +294,13 @@ sub correct_urlbase { return Bugzilla->params->{'urlbase'}; } +sub use_attachbase { + my $attachbase = Bugzilla->params->{'attachment_base'}; + return ($attachbase ne '' + && $attachbase ne Bugzilla->params->{'urlbase'} + && $attachbase ne Bugzilla->params->{'sslbase'}) ? 1 : 0; +} + sub lsearch { my ($list,$item) = (@_); my $count = 0; @@ -803,6 +810,11 @@ cookies) to only some addresses. Returns either the C<sslbase> or C<urlbase> parameter, depending on the current setting for the C<ssl> parameter. +=item C<use_attachbase()> + +Returns true if an alternate host is used to display attachments; false +otherwise. + =back =head2 Searching diff --git a/attachment.cgi b/attachment.cgi index 35829343f..f1753261d 100755 --- a/attachment.cgi +++ b/attachment.cgi @@ -27,6 +27,7 @@ # Greg Hendricks <ghendricks@novell.com> # Frédéric Buclin <LpSolit@gmail.com> # Marc Schumann <wurblzap@gmail.com> +# Byron Jones <bugzilla@glob.com.au> ################################################################################ # Script Initialization @@ -51,8 +52,6 @@ use Bugzilla::Attachment::PatchReader; use Bugzilla::Token; use Bugzilla::Keyword; -Bugzilla->login(); - # For most scripts we don't make $cgi and $template global variables. But # when preparing Bugzilla for mod_perl, this script used these # variables in so many subroutines that it was easier to just @@ -73,13 +72,27 @@ local our $vars = {}; # Determine whether to use the action specified by the user or the default. my $action = $cgi->param('action') || 'view'; +# You must use the appropriate urlbase/sslbase param when doing anything +# but viewing an attachment. +if ($action ne 'view') { + my $urlbase = Bugzilla->params->{'urlbase'}; + my $sslbase = Bugzilla->params->{'sslbase'}; + my $path_regexp = $sslbase ? qr/^(\Q$urlbase\E|\Q$sslbase\E)/ : qr/^\Q$urlbase\E/; + if (use_attachbase() && $cgi->self_url !~ /$path_regexp/) { + $cgi->redirect_to_urlbase; + } + Bugzilla->login(); +} + # Determine if PatchReader is installed eval { require PatchReader; $vars->{'patchviewerinstalled'} = 1; }; -if ($action eq "view") +# When viewing an attachment, do not request credentials if we are on +# the alternate host. Let view() decide when to call Bugzilla->login. +if ($action eq "view") { view(); } @@ -131,7 +144,8 @@ exit; # Validates an attachment ID. Optionally takes a parameter of a form # variable name that contains the ID to be validated. If not specified, # uses 'id'. -# +# If the second parameter is true, the attachment ID will be validated, +# however the current user's access to the attachment will not be checked. # Will throw an error if 1) attachment ID is not a valid number, # 2) attachment does not exist, or 3) user isn't allowed to access the # attachment. @@ -139,8 +153,8 @@ exit; # Returns an attachment object. sub validateID { - my $param = @_ ? $_[0] : 'id'; - my $user = Bugzilla->user; + my($param, $dont_validate_access) = @_; + $param ||= 'id'; # If we're not doing interdiffs, check if id wasn't specified and # prompt them with a page that allows them to choose an attachment. @@ -164,6 +178,14 @@ sub validateID { my $attachment = new Bugzilla::Attachment($attach_id) || ThrowUserError("invalid_attach_id", { attach_id => $attach_id }); + return $attachment if ($dont_validate_access || check_can_access($attachment)); +} + +# Make sure the current user has access to the specified attachment. +sub check_can_access { + my $attachment = shift; + my $user = Bugzilla->user; + # Make sure the user is authorized to access this attachment's bug. Bugzilla::Bug->check($attachment->bug_id); if ($attachment->isprivate && $user->id != $attachment->attacher->id @@ -172,7 +194,19 @@ sub validateID { ThrowUserError('auth_failure', {action => 'access', object => 'attachment'}); } - return $attachment; + return 1; +} + +# Determines if the attachment is public -- that is, if users who are +# not logged in have access to the attachment +sub attachmentIsPublic { + my $attachment = shift; + + return 0 if Bugzilla->params->{'requirelogin'}; + return 0 if $attachment->isprivate; + + my $anon_user = new Bugzilla::User; + return $anon_user->can_see_bug($attachment->bug_id); } # Validates format of a diff/interdiff. Takes a list as an parameter, which @@ -223,8 +257,60 @@ sub validateCanChangeBug # Display an attachment. sub view { - # Retrieve and validate parameters - my $attachment = validateID(); + my $attachment; + + if (use_attachbase()) { + $attachment = validateID(undef, 1); + # Replace %bugid% by the ID of the bug the attachment belongs to, if present. + my $attachbase = Bugzilla->params->{'attachment_base'}; + my $bug_id = $attachment->bug_id; + $attachbase =~ s/%bugid%/$bug_id/; + my $path = 'attachment.cgi?id=' . $attachment->id; + + # Make sure the attachment is served from the correct server. + if ($cgi->self_url !~ /^\Q$attachbase\E/) { + # We couldn't call Bugzilla->login earlier as we first had to make sure + # we were not going to request credentials on the alternate host. + Bugzilla->login(); + if (attachmentIsPublic($attachment)) { + # No need for a token; redirect to attachment base. + print $cgi->redirect(-location => $attachbase . $path); + exit; + } else { + # Make sure the user can view the attachment. + check_can_access($attachment); + # Create a token and redirect. + my $token = url_quote(issue_session_token($attachment->id)); + print $cgi->redirect(-location => $attachbase . "$path&t=$token"); + exit; + } + } else { + # No need to validate the token for public attachments. We cannot request + # credentials as we are on the alternate host. + if (!attachmentIsPublic($attachment)) { + my $token = $cgi->param('t'); + my ($userid, undef, $token_attach_id) = Bugzilla::Token::GetTokenData($token); + unless ($userid + && detaint_natural($token_attach_id) + && ($token_attach_id == $attachment->id)) + { + # Not a valid token. + print $cgi->redirect('-location' => correct_urlbase() . $path); + exit; + } + # Change current user without creating cookies. + Bugzilla->set_user(new Bugzilla::User($userid)); + # Tokens are single use only, delete it. + delete_token($token); + } + } + } else { + # No alternate host is used. Request credentials if required. + Bugzilla->login(); + $attachment = validateID(); + } + + # At this point, Bugzilla->login has been called if it had to. my $contenttype = $attachment->contenttype; my $filename = $attachment->filename; diff --git a/template/en/default/admin/params/attachment.html.tmpl b/template/en/default/admin/params/attachment.html.tmpl index ef3363bbb..7c0b52472 100644 --- a/template/en/default/admin/params/attachment.html.tmpl +++ b/template/en/default/admin/params/attachment.html.tmpl @@ -24,6 +24,24 @@ %] [% param_descs = { + attachment_base => "It is possible for a malicious attachment to steal your " _ + "cookies or access other attachments to perform an attack " _ + "on the user.<p>" _ + "If you would like additional security on attachments " _ + "to avoid this, set this parameter to an alternate URL " _ + "for your $terms.Bugzilla that is not the same as " _ + "<tt>urlbase</tt> or <tt>sslbase</tt>. That is, a different " _ + "domain name that resolves to this exact same $terms.Bugzilla " _ + "installation.<p>" _ + "For added security, you can insert <tt>%bugid%</tt> into " _ + "the URL, which will be replaced with the ID of the current " _ + "$terms.bug that the attachment is on, when you access " _ + "an attachment. This will limit attachments to accessing " _ + "only other attachments on the same ${terms.bug}. " _ + "Remember, though, that all those possible domain names " _ + "(such as <tt>1234.your.domain.com</tt>) must point to " _ + "this same $terms.Bugzilla instance." + allow_attachment_deletion => "If this option is on, administrators will be able to delete " _ "the content of attachments.", diff --git a/template/en/default/attachment/edit.html.tmpl b/template/en/default/attachment/edit.html.tmpl index 2550d4f46..48137e76a 100644 --- a/template/en/default/attachment/edit.html.tmpl +++ b/template/en/default/attachment/edit.html.tmpl @@ -39,6 +39,9 @@ doc_section = "attachments.html" %] +[%# No need to display the Diff button and iframe if the attachment is not a patch. %] +[% patchviewerinstalled = (patchviewerinstalled && attachment.ispatch) %] + <script type="text/javascript"> <!-- var prev_mode = 'raw'; @@ -47,37 +50,7 @@ var has_viewed_as_diff = 0; function editAsComment() { - // Get the content of the document as a string. - var viewFrame = document.getElementById('viewFrame'); - var aSerializer = new XMLSerializer(); - var contentDocument = viewFrame.contentDocument; - var theContent = aSerializer.serializeToString(contentDocument); - - // If this is a plaintext document, remove cruft that Mozilla adds - // because it treats it as an HTML document with a big PRE section. - // http://bugzilla.mozilla.org/show_bug.cgi?id=86012 - var contentType = '[% attachment.contenttype FILTER js %]'; - if ( contentType == 'text/plain' ) - { - theContent = theContent.replace( /^<html><head\/?><body><pre>/i , "" ); - theContent = theContent.replace( /<\/pre><\/body><\/html>$/i , "" ); - theContent = theContent.replace( /</gi , "<" ); - theContent = theContent.replace( />/gi , ">" ); - theContent = theContent.replace( /&/gi , "&" ); - } - - // Add mail-style quote indicators (>) to the beginning of each line. - // ".*\n" matches lines that end with a newline, while ".+" matches - // the rare situation in which the last line of a file does not end - // with a newline. - theContent = theContent.replace( /(.*\n|.+)/g , ">$1" ); - switchToMode('edit'); - - // Copy the contents of the diff into the textarea - var editFrame = document.getElementById('editFrame'); - editFrame.value = theContent + "\n\n"; - has_edited = 1; } function undoEditAsComment() @@ -306,6 +279,8 @@ minrows = 10 cols = 80 wrap = 'soft' + defaultcontent = (attachment.contenttype.match('^text\/')) ? + attachment.data.replace('(.*\n|.+)', '>$1') : undef %] <iframe id="viewFrame" src="attachment.cgi?id=[% attachment.id %]" style="height: 400px; width: 100%;"> <b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. |