diff options
| -rw-r--r-- | Bugzilla/Attachment/PatchReader.pm | 2 | ||||
| -rwxr-xr-x | attachment.cgi | 129 | ||||
| -rw-r--r-- | template/en/default/global/user-error.html.tmpl | 5 |
3 files changed, 106 insertions, 30 deletions
diff --git a/Bugzilla/Attachment/PatchReader.pm b/Bugzilla/Attachment/PatchReader.pm index cfc7610f4..01a624a8f 100644 --- a/Bugzilla/Attachment/PatchReader.pm +++ b/Bugzilla/Attachment/PatchReader.pm @@ -37,6 +37,7 @@ sub process_diff { $last_reader->sends_data_to(new PatchReader::DiffPrinter::raw()); # Actually print out the patch. print $cgi->header(-type => 'text/plain', + -x_content_type_options => "nosniff", -expires => '+3M'); disable_utf8(); $reader->iterate_string('Attachment ' . $attachment->id, $attachment->data); @@ -118,6 +119,7 @@ sub process_interdiff { $last_reader->sends_data_to(new PatchReader::DiffPrinter::raw()); # Actually print out the patch. print $cgi->header(-type => 'text/plain', + -x_content_type_options => "nosniff", -expires => '+3M'); disable_utf8(); } diff --git a/attachment.cgi b/attachment.cgi index f612815e2..5eba13611 100755 --- a/attachment.cgi +++ b/attachment.cgi @@ -74,10 +74,13 @@ local our $vars = {}; # Determine whether to use the action specified by the user or the default. my $action = $cgi->param('action') || 'view'; +my $format = $cgi->param('format') || ''; # You must use the appropriate urlbase/sslbase param when doing anything -# but viewing an attachment. -if ($action ne 'view') { +# but viewing an attachment, or a raw diff. +if ($action ne 'view' + && (($action !~ /^(?:interdiff|diff)$/) || $format ne 'raw')) +{ do_ssl_redirect_if_required(); if ($cgi->url_is_attachment_base) { $cgi->redirect_to_urlbase; @@ -167,11 +170,12 @@ sub validateID { # non-natural, so use the original value from $cgi in our exception # message here. detaint_natural($attach_id) - || ThrowUserError("invalid_attach_id", { attach_id => $cgi->param($param) }); + || ThrowUserError("invalid_attach_id", + { attach_id => scalar $cgi->param($param) }); # Make sure the attachment exists in the database. my $attachment = new Bugzilla::Attachment($attach_id) - || ThrowUserError("invalid_attach_id", { attach_id => $attach_id }); + || ThrowUserError("invalid_attach_id", { attach_id => $attach_id }); return $attachment if ($dont_validate_access || check_can_access($attachment)); } @@ -187,7 +191,8 @@ sub check_can_access { && !$user->is_insider) { ThrowUserError('auth_failure', {action => 'access', - object => 'attachment'}); + object => 'attachment', + attach_id => $attachment->id}); } return 1; } @@ -230,34 +235,51 @@ sub validateContext return $context; } -################################################################################ -# Functions -################################################################################ +# Gets the attachment object(s) generated by validateID, while ensuring +# attachbase and token authentication is used when required. +sub get_attachment { + my @field_names = @_ ? @_ : qw(id); -# Display an attachment. -sub view { - my $attachment; + my %attachments; if (use_attachbase()) { - $attachment = validateID(undef, 1); - my $path = 'attachment.cgi?id=' . $attachment->id; - # The user is allowed to override the content type of the attachment. - if (defined $cgi->param('content_type')) { - $path .= '&content_type=' . url_quote($cgi->param('content_type')); + # Load each attachment, and ensure they are all from the same bug + my $bug_id = 0; + foreach my $field_name (@field_names) { + my $attachment = validateID($field_name, 1); + if (!$bug_id) { + $bug_id = $attachment->bug_id; + } elsif ($attachment->bug_id != $bug_id) { + ThrowUserError('attachment_bug_id_mismatch'); + } + $attachments{$field_name} = $attachment; } + my @args = map { $_ . '=' . $attachments{$_}->id } @field_names; + my $cgi_params = $cgi->canonicalise_query(@field_names, 't', + 'Bugzilla_login', 'Bugzilla_password'); + push(@args, $cgi_params) if $cgi_params; + my $path = 'attachment.cgi?' . join('&', @args); # Make sure the attachment is served from the correct server. - my $bug_id = $attachment->bug_id; if ($cgi->url_is_attachment_base($bug_id)) { # No need to validate the token for public attachments. We cannot request # credentials as we are on the alternate host. - if (!attachmentIsPublic($attachment)) { + if (!all_attachments_are_public(\%attachments)) { 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)) - { + my ($userid, undef, $token_data) = Bugzilla::Token::GetTokenData($token); + my %token_data = unpack_token_data($token_data); + my $valid_token = 1; + foreach my $field_name (@field_names) { + my $token_id = $token_data{$field_name}; + if (!$token_id + || !detaint_natural($token_id) + || $attachments{$field_name}->id != $token_id) + { + $valid_token = 0; + last; + } + } + unless ($userid && $valid_token) { # Not a valid token. print $cgi->redirect('-location' => correct_urlbase() . $path); exit; @@ -283,15 +305,17 @@ sub view { # Replace %bugid% by the ID of the bug the attachment # belongs to, if present. $attachbase =~ s/\%bugid\%/$bug_id/; - if (attachmentIsPublic($attachment)) { + if (all_attachments_are_public(\%attachments)) { # 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); + foreach my $field_name (@field_names) { + check_can_access($attachments{$field_name}); + } # Create a token and redirect. - my $token = url_quote(issue_session_token($attachment->id)); + my $token = url_quote(issue_session_token(pack_token_data(\%attachments))); print $cgi->redirect(-location => $attachbase . "$path&t=$token"); exit; } @@ -300,8 +324,48 @@ sub view { do_ssl_redirect_if_required(); # No alternate host is used. Request credentials if required. Bugzilla->login(); - $attachment = validateID(); + foreach my $field_name (@field_names) { + $attachments{$field_name} = validateID($field_name); + } + } + + return wantarray + ? map { $attachments{$_} } @field_names + : $attachments{$field_names[0]}; +} + +sub all_attachments_are_public { + my $attachments = shift; + foreach my $field_name (keys %$attachments) { + if (!attachmentIsPublic($attachments->{$field_name})) { + return 0; + } + } + return 1; +} + +sub pack_token_data { + my $attachments = shift; + return join(' ', map { $_ . '=' . $attachments->{$_}->id } keys %$attachments); +} + +sub unpack_token_data { + my @token_data = split(/ /, shift || ''); + my %data; + foreach my $token (@token_data) { + my ($field_name, $attach_id) = split('=', $token); + $data{$field_name} = $attach_id; } + return %data; +} + +################################################################################ +# Functions +################################################################################ + +# Display an attachment. +sub view { + my $attachment = get_attachment(); # At this point, Bugzilla->login has been called if it had to. my $contenttype = $attachment->contenttype; @@ -352,9 +416,14 @@ sub view { sub interdiff { # Retrieve and validate parameters - my $old_attachment = validateID('oldid'); - my $new_attachment = validateID('newid'); my $format = validateFormat('html', 'raw'); + my($old_attachment, $new_attachment); + if ($format eq 'raw') { + ($old_attachment, $new_attachment) = get_attachment('oldid', 'newid'); + } else { + $old_attachment = validateID('oldid'); + $new_attachment = validateID('newid'); + } my $context = validateContext(); Bugzilla::Attachment::PatchReader::process_interdiff( @@ -363,8 +432,8 @@ sub interdiff { sub diff { # Retrieve and validate parameters - my $attachment = validateID(); my $format = validateFormat('html', 'raw'); + my $attachment = $format eq 'raw' ? get_attachment() : validateID(); my $context = validateContext(); # If it is not a patch, view normally. diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl index 3e1b8748e..af2fc7b36 100644 --- a/template/en/default/global/user-error.html.tmpl +++ b/template/en/default/global/user-error.html.tmpl @@ -109,6 +109,11 @@ [% terms.Bug %] aliases cannot be longer than 20 characters. Please choose a shorter alias. + [% ELSIF error == "attachment_bug_id_mismatch" %] + [% title = "Invalid Attachments" %] + You tried to perform an action on attachments from different [% terms.bugs %]. + This operation requires all attachments to be from the same [% terms.bug %]. + [% ELSIF error == "auth_cant_create_account" %] [% title = "Can't create accounts" %] This site is using an authentication scheme which does not permit |