From 719303d9928bf0a727478c32f83a39b015a25136 Mon Sep 17 00:00:00 2001 From: "lpsolit%gmail.com" <> Date: Thu, 9 Apr 2009 11:37:56 +0000 Subject: Bug 454251: Implement Bugzilla::Attachment->create() and $attachment->update() - Patch by Frédéric Buclin a=LpSolit (module owner) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Bugzilla/Attachment.pm | 635 ++++++++++++------------ Bugzilla/Constants.pm | 29 +- Bugzilla/Flag.pm | 40 ++ attachment.cgi | 233 ++++----- post_bug.cgi | 51 +- template/en/default/global/code-error.html.tmpl | 8 + template/en/default/global/user-error.html.tmpl | 13 +- 7 files changed, 515 insertions(+), 494 deletions(-) diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm index 4aa74dcdf..33c761cf1 100644 --- a/Bugzilla/Attachment.pm +++ b/Bugzilla/Attachment.pm @@ -86,6 +86,39 @@ sub DB_COLUMNS { $dbh->sql_date_format('attachments.creation_ts', '%Y.%m.%d %H:%i') . ' AS creation_ts'; } +use constant REQUIRED_CREATE_FIELDS => qw( + bug + data + description + filename + mimetype +); + +use constant UPDATE_COLUMNS => qw( + description + filename + isobsolete + ispatch + isprivate + mimetype + modification_time +); + +use constant VALIDATORS => { + bug => \&_check_bug, + description => \&_check_description, + isprivate => \&_check_is_private, + isurl => \&_check_is_url, + store_in_file => \&_check_store_in_file, +}; + +use constant UPDATE_VALIDATORS => { + filename => \&_check_filename, + isobsolete => \&Bugzilla::Object::check_boolean, + ispatch => \&Bugzilla::Object::check_boolean, + mimetype => \&_check_content_type, +}; + ############################### #### Accessors ###### ############################### @@ -123,7 +156,7 @@ sub bug { my $self = shift; require Bugzilla::Bug; - $self->{bug} = Bugzilla::Bug->new($self->bug_id); + $self->{bug} ||= Bugzilla::Bug->new($self->bug_id); return $self->{bug}; } @@ -393,6 +426,13 @@ sub datasize { return $self->{datasize}; } +sub _get_local_filename { + my $self = shift; + my $hash = ($self->id % 100) + 100; + $hash =~ s/.*(\d\d)$/group.$1/; + return bz_locations()->{'attachdir'} . "/$hash/attachment." . $self->id; +} + =over =item C @@ -439,23 +479,121 @@ sub flag_types { #### Validators ###### ############################### -# Instance methods; no POD documentation here yet because the only ones so far -# are private. +sub set_content_type { $_[0]->set('mimetype', $_[1]); } +sub set_description { $_[0]->set('description', $_[1]); } +sub set_filename { $_[0]->set('filename', $_[1]); } +sub set_is_obsolete { $_[0]->set('isobsolete', $_[1]); } +sub set_is_patch { $_[0]->set('ispatch', $_[1]); } +sub set_is_private { $_[0]->set('isprivate', $_[1]); } -sub _get_local_filename { - my $self = shift; - my $hash = ($self->id % 100) + 100; - $hash =~ s/.*(\d\d)$/group.$1/; - return bz_locations()->{'attachdir'} . "/$hash/attachment." . $self->id; +sub _check_bug { + my ($invocant, $bug) = @_; + my $user = Bugzilla->user; + + $bug = ref $invocant ? $invocant->bug : $bug; + ($user->can_see_bug($bug->id) && $user->can_edit_product($bug->product_id)) + || ThrowUserError("illegal_attachment_edit_bug", { bug_id => $bug->id }); + + return $bug; } -sub _validate_filename { - my ($throw_error) = @_; - my $cgi = Bugzilla->cgi; - defined $cgi->upload('data') - || ($throw_error ? ThrowUserError("file_not_specified") : return 0); +sub _check_content_type { + my ($invocant, $content_type) = @_; + + $content_type = 'text/plain' if (ref $invocant && ($invocant->isurl || $invocant->ispatch)); + my $legal_types = join('|', LEGAL_CONTENT_TYPES); + if ($content_type !~ /^($legal_types)\/.+$/) { + ThrowUserError("invalid_content_type", { contenttype => $content_type }); + } + trick_taint($content_type); + + return $content_type; +} + +sub _check_data { + my ($invocant, $params) = @_; + + my $data; + if ($params->{isurl}) { + $data = $params->{data}; + ($data && $data =~ m#^(http|https|ftp)://\S+#) + || ThrowUserError('attachment_illegal_url', { url => $data }); + + $params->{mimetype} = 'text/plain'; + $params->{ispatch} = 0; + $params->{store_in_file} = 0; + } + else { + if ($params->{store_in_file} || !ref $params->{data}) { + # If it's a filehandle, just store it, not the content of the file + # itself as the file may be quite large. If it's not a filehandle, + # it already contains the content of the file. + $data = $params->{data}; + + # We don't compress BMP images stored locally, nor do we check + # their size. No need to go further. + return $data if $params->{store_in_file}; + } + else { + # The file will be stored in the DB. We need the content of the file. + local $/; + my $fh = $params->{data}; + $data = <$fh>; + } + + $data || ThrowUserError('zero_length_file'); + + # This should go away, see bug 480986. + # Windows screenshots are usually uncompressed BMP files which + # makes for a quick way to eat up disk space. Let's compress them. + # We do this before we check the size since the uncompressed version + # could easily be greater than maxattachmentsize. + if (Bugzilla->params->{'convert_uncompressed_images'} + && $params->{mimetype} eq 'image/bmp') + { + require Image::Magick; + my $img = Image::Magick->new(magick=>'bmp'); + $img->BlobToImage($data); + $img->set(magick=>'png'); + my $imgdata = $img->ImageToBlob(); + $data = $imgdata; + $params->{mimetype} = 'image/png'; + # $hr_vars->{'convertedbmp'} = 1; + } + + # Make sure the attachment does not exceed the maximum permitted size. + my $max_size = Bugzilla->params->{'maxattachmentsize'} * 1024; # Convert from K + my $len = length($data); + if ($len > $max_size) { + my $vars = { filesize => sprintf("%.0f", $len/1024) }; + if ($params->{ispatch}) { + ThrowUserError('patch_too_large', $vars); + } + else { + ThrowUserError('file_too_large', $vars); + } + } + } + return $data; +} + +sub _check_description { + my ($invocant, $description) = @_; + + $description = trim($description); + $description || ThrowUserError('missing_attachment_description'); + return $description; +} + +sub _check_filename { + my ($invocant, $filename, $is_url) = @_; + + $is_url = $invocant->isurl if ref $invocant; + # No file is attached, so it has no name. + return '' if $is_url; - my $filename = $cgi->upload('data'); + $filename = trim($filename); + $filename || ThrowUserError('file_not_specified'); # Remove path info (if any) from the file name. The browser should do this # for us, but some are buggy. This may not work on Mac file names and could @@ -467,64 +605,38 @@ sub _validate_filename { # Truncate the filename to 100 characters, counting from the end of the # string to make sure we keep the filename extension. $filename = substr($filename, -100, 100); + trick_taint($filename); return $filename; } -sub _validate_data { - my ($throw_error, $hr_vars) = @_; - my $cgi = Bugzilla->cgi; +sub _check_is_private { + my ($invocant, $is_private) = @_; - my $fh; - # Skip uploading into a local variable if the user wants to upload huge - # attachments into local files. - if (!$cgi->param('bigfile')) { - $fh = $cgi->upload('data'); + if (((!ref $invocant && $is_private) + || (ref $invocant && $invocant->isprivate != $is_private)) + && !Bugzilla->user->is_insider) { + ThrowUserError('user_not_insider'); } - my $data; + return $is_private ? 1 : 0; +} - # We could get away with reading only as much as required, except that then - # we wouldn't have a size to print to the error handler below. - if (!$cgi->param('bigfile')) { - # enable 'slurp' mode - local $/; - $data = <$fh>; - } +sub _check_is_url { + my ($invocant, $is_url) = @_; - $data - || ($cgi->param('bigfile')) - || ($throw_error ? ThrowUserError("zero_length_file") : return 0); - - # Windows screenshots are usually uncompressed BMP files which - # makes for a quick way to eat up disk space. Let's compress them. - # We do this before we check the size since the uncompressed version - # could easily be greater than maxattachmentsize. - if (Bugzilla->params->{'convert_uncompressed_images'} - && $cgi->param('contenttype') eq 'image/bmp') { - require Image::Magick; - my $img = Image::Magick->new(magick=>'bmp'); - $img->BlobToImage($data); - $img->set(magick=>'png'); - my $imgdata = $img->ImageToBlob(); - $data = $imgdata; - $cgi->param('contenttype', 'image/png'); - $hr_vars->{'convertedbmp'} = 1; + if ($is_url && !Bugzilla->params->{'allow_attach_url'}) { + ThrowCodeError('attachment_url_disabled'); } + return $is_url ? 1 : 0; +} - # Make sure the attachment does not exceed the maximum permitted size - my $maxsize = Bugzilla->params->{'maxattachmentsize'} * 1024; # Convert from K - my $len = $data ? length($data) : 0; - if ($maxsize && $len > $maxsize) { - my $vars = { filesize => sprintf("%.0f", $len/1024) }; - if ($cgi->param('ispatch')) { - $throw_error ? ThrowUserError("patch_too_large", $vars) : return 0; - } - else { - $throw_error ? ThrowUserError("file_too_large", $vars) : return 0; - } - } +sub _check_store_in_file { + my ($invocant, $store_in_file) = @_; - return $data || ''; + if ($store_in_file && !Bugzilla->params->{'maxlocalattachment'}) { + ThrowCodeError('attachment_local_storage_disabled'); + } + return $store_in_file ? 1 : 0; } =pod @@ -587,105 +699,6 @@ sub get_attachments_by_bug { =pod -=item C - -Description: validates the "patch" flag passed in by CGI. - -Returns: 1 on success. - -=cut - -sub validate_is_patch { - my ($class, $throw_error) = @_; - my $cgi = Bugzilla->cgi; - - # Set the ispatch flag to zero if it is undefined, since the UI uses - # an HTML checkbox to represent this flag, and unchecked HTML checkboxes - # do not get sent in HTML requests. - $cgi->param('ispatch', $cgi->param('ispatch') ? 1 : 0); - - # Set the content type to text/plain if the attachment is a patch. - $cgi->param('contenttype', 'text/plain') if $cgi->param('ispatch'); - - return 1; -} - -=pod - -=item C - -Description: validates the description passed in by CGI. - -Returns: 1 on success. - -=cut - -sub validate_description { - my ($class, $throw_error) = @_; - my $cgi = Bugzilla->cgi; - - $cgi->param('description') - || ($throw_error ? ThrowUserError("missing_attachment_description") : return 0); - - return 1; -} - -=pod - -=item C - -Description: validates the content type passed in by CGI. - -Returns: 1 on success. - -=cut - -sub validate_content_type { - my ($class, $throw_error) = @_; - my $cgi = Bugzilla->cgi; - - if (!defined $cgi->param('contenttypemethod')) { - $throw_error ? ThrowUserError("missing_content_type_method") : return 0; - } - elsif ($cgi->param('contenttypemethod') eq 'autodetect') { - my $contenttype = - $cgi->uploadInfo($cgi->param('data'))->{'Content-Type'}; - # The user asked us to auto-detect the content type, so use the type - # specified in the HTTP request headers. - if ( !$contenttype ) { - $throw_error ? ThrowUserError("missing_content_type") : return 0; - } - $cgi->param('contenttype', $contenttype); - } - elsif ($cgi->param('contenttypemethod') eq 'list') { - # The user selected a content type from the list, so use their - # selection. - $cgi->param('contenttype', $cgi->param('contenttypeselection')); - } - elsif ($cgi->param('contenttypemethod') eq 'manual') { - # The user entered a content type manually, so use their entry. - $cgi->param('contenttype', $cgi->param('contenttypeentry')); - } - else { - $throw_error ? - ThrowCodeError("illegal_content_type_method", - { contenttypemethod => $cgi->param('contenttypemethod') }) : - return 0; - } - - if ( $cgi->param('contenttype') !~ - /^(application|audio|image|message|model|multipart|text|video)\/.+$/ ) { - $throw_error ? - ThrowUserError("invalid_content_type", - { contenttype => $cgi->param('contenttype') }) : - return 0; - } - - return 1; -} - -=pod - =item C Description: validates if the user is allowed to view and edit the attachment. @@ -709,7 +722,7 @@ sub validate_can_edit { || ((!$attachment->isprivate || $user->is_insider) && $user->in_group('editbugs', $product_id))); - # If we come here, then this attachment cannot be seen by the user. + # If we come here, then this attachment cannot be edited by the user. ThrowUserError('illegal_attachment_edit', { attach_id => $attachment->id }); } @@ -727,14 +740,13 @@ Returns: 1 on success. Else an error is thrown. =cut sub validate_obsolete { - my ($class, $bug) = @_; - my $cgi = Bugzilla->cgi; + my ($class, $bug, $list) = @_; # Make sure the attachment id is valid and the user has permissions to view # the bug to which it is attached. Make sure also that the user can view # the attachment itself. my @obsolete_attachments; - foreach my $attachid ($cgi->param('obsolete')) { + foreach my $attachid (@$list) { my $vars = {}; $vars->{'attach_id'} = $attachid; @@ -771,192 +783,147 @@ sub validate_obsolete { =pod -=item C +=item C -Description: inserts an attachment from CGI input for the given bug. +Description: inserts an attachment into the given bug. -Params: C<$bug> - Bugzilla::Bug object - the bug for which to insert +Params: takes a hashref with the following keys: + C - Bugzilla::Bug object - the bug for which to insert the attachment. - C<$user> - Bugzilla::User object - the user we're inserting an - attachment for. - C<$timestamp> - scalar - timestamp of the insert as returned - by SELECT NOW(). - C<$hr_vars> - hash reference - reference to a hash of template - variables. - -Returns: the ID of the new attachment. + C - Either a filehandle pointing to the content of the + attachment, or the content of the attachment itself. + C - string - describe what the attachment is about. + C - string - the name of the attachment (used by the + browser when downloading it). If the attachment is a URL, this + parameter has no effect. + C - string - a valid MIME type. + C - string (optional) - timestamp of the insert + as returned by SELECT NOW(). + C - boolean (optional, default false) - true if the + attachment is a patch. + C - boolean (optional, default false) - true if + the attachment is private. + C - boolean (optional, default false) - true if the + attachment is a URL pointing to some external ressource. + C - boolean (optional, default false) - true + if the attachment must be stored in data/attachments/ instead + of in the DB. + +Returns: The new attachment object. =cut -# FIXME: needs to follow the way Object->create() works. sub create { - my ($class, $throw_error, $bug, $user, $timestamp, $hr_vars) = @_; - - my $cgi = Bugzilla->cgi; + my $class = shift; my $dbh = Bugzilla->dbh; - my $attachurl = $cgi->param('attachurl') || ''; - my $data; - my $filename; - my $contenttype; - my $isurl; - $class->validate_is_patch($throw_error) || return; - $class->validate_description($throw_error) || return; - - if (Bugzilla->params->{'allow_attach_url'} - && ($attachurl =~ /^(http|https|ftp):\/\/\S+/) - && !defined $cgi->upload('data')) - { - $filename = ''; - $data = $attachurl; - $isurl = 1; - $contenttype = 'text/plain'; - $cgi->param('ispatch', 0); - $cgi->delete('bigfile'); - } - else { - $filename = _validate_filename($throw_error) || return; - # need to validate content type before data as - # we now check the content type for image/bmp in _validate_data() - unless ($cgi->param('ispatch')) { - $class->validate_content_type($throw_error) || return; - - # Set the ispatch flag to 1 if we're set to autodetect - # and the content type is text/x-diff or text/x-patch - if ($cgi->param('contenttypemethod') eq 'autodetect' - && $cgi->param('contenttype') =~ m{text/x-(?:diff|patch)}) - { - $cgi->param('ispatch', 1); - $cgi->param('contenttype', 'text/plain'); - } - } - $data = _validate_data($throw_error, $hr_vars); - # If the attachment is stored locally, $data eq ''. - # If an error is thrown, $data eq '0'. - ($data ne '0') || return; - $contenttype = $cgi->param('contenttype'); - - # These are inserted using placeholders so no need to panic - trick_taint($filename); - trick_taint($contenttype); - $isurl = 0; - } - - # Check attachments the user tries to mark as obsolete. - my @obsolete_attachments; - if ($cgi->param('obsolete')) { - @obsolete_attachments = $class->validate_obsolete($bug); - } - # The order of these function calls is important, as Flag::validate - # assumes User::match_field has ensured that the - # values in the requestee fields are legitimate user email addresses. - my $match_status = Bugzilla::User::match_field($cgi, { - '^requestee(_type)?-(\d+)$' => { 'type' => 'multi' }, - }, MATCH_SKIP_CONFIRM); + $class->check_required_create_fields(@_); + my $params = $class->run_create_validators(@_); - $hr_vars->{'match_field'} = 'requestee'; - if ($match_status == USER_MATCH_FAILED) { - $hr_vars->{'message'} = 'user_match_failed'; - } - elsif ($match_status == USER_MATCH_MULTIPLE) { - $hr_vars->{'message'} = 'user_match_multiple'; - } + # Extract everything which is not a valid column name. + my $bug = delete $params->{bug}; + $params->{bug_id} = $bug->id; + my $fh = delete $params->{data}; + my $store_in_file = delete $params->{store_in_file}; - # Escape characters in strings that will be used in SQL statements. - my $description = $cgi->param('description'); - trick_taint($description); - my $isprivate = $cgi->param('isprivate') ? 1 : 0; - - # Insert the attachment into the database. - my $sth = $dbh->do( - "INSERT INTO attachments - (bug_id, creation_ts, modification_time, filename, description, - mimetype, ispatch, isurl, isprivate, submitter_id) - VALUES (?,?,?,?,?,?,?,?,?,?)", undef, ($bug->bug_id, $timestamp, $timestamp, - $filename, $description, $contenttype, $cgi->param('ispatch'), - $isurl, $isprivate, $user->id)); - # Retrieve the ID of the newly created attachment record. - my $attachid = $dbh->bz_last_key('attachments', 'attach_id'); + my $attachment = $class->insert_create_data($params); + my $attachid = $attachment->id; # We only use $data here in this INSERT with a placeholder, # so it's safe. - $sth = $dbh->prepare("INSERT INTO attach_data - (id, thedata) VALUES ($attachid, ?)"); + my $sth = $dbh->prepare("INSERT INTO attach_data + (id, thedata) VALUES ($attachid, ?)"); + + my $data = $store_in_file ? "" : $fh; trick_taint($data); $sth->bind_param(1, $data, $dbh->BLOB_TYPE); $sth->execute(); # If the file is to be stored locally, stream the file from the web server # to the local file without reading it into a local variable. - if ($cgi->param('bigfile')) { + if ($store_in_file) { + my $limit = Bugzilla->params->{"maxlocalattachment"} * 1048576; + # If $fh is not a filehandle, we already know its size. + ThrowUserError("local_file_too_large") if (!ref($fh) && length($fh) > $limit); + my $attachdir = bz_locations()->{'attachdir'}; - my $fh = $cgi->upload('data'); my $hash = ($attachid % 100) + 100; $hash =~ s/.*(\d\d)$/group.$1/; mkdir "$attachdir/$hash", 0770; chmod 0770, "$attachdir/$hash"; open(AH, ">$attachdir/$hash/attachment.$attachid"); binmode AH; - my $sizecount = 0; - my $limit = (Bugzilla->params->{"maxlocalattachment"} * 1048576); - while (<$fh>) { - print AH $_; - $sizecount += length($_); - if ($sizecount > $limit) { - close AH; - close $fh; - unlink "$attachdir/$hash/attachment.$attachid"; - $throw_error ? ThrowUserError("local_file_too_large") : return; + if (ref $fh) { + my $sizecount = 0; + while (<$fh>) { + print AH $_; + $sizecount += length($_); + if ($sizecount > $limit) { + close AH; + close $fh; + unlink "$attachdir/$hash/attachment.$attachid"; + ThrowUserError("local_file_too_large"); + } } + close $fh; + } + else { + print AH $fh; } close AH; - close $fh; } - # Make existing attachments obsolete. - my $fieldid = get_field_id('attachments.isobsolete'); + # Return the new attachment object. + return $attachment; +} + +sub run_create_validators { + my $class = shift; + my $params = $class->SUPER::run_create_validators(@_); + + $params->{data} = $class->_check_data($params); + # We couldn't call these checkers earlier as _check_data() could alter values. + $params->{ispatch} = $params->{ispatch} ? 1 : 0; + $params->{filename} = $class->_check_filename($params->{filename}, $params->{isurl}); + $params->{mimetype} = $class->_check_content_type($params->{mimetype}); + $params->{creation_ts} ||= Bugzilla->dbh->selectrow_array('SELECT NOW()'); + $params->{modification_time} = $params->{creation_ts}; + $params->{submitter_id} = Bugzilla->user->id || ThrowCodeError('invalid_user'); - foreach my $obsolete_attachment (@obsolete_attachments) { - # If the obsolete attachment has request flags, cancel them. - # This call must be done before updating the 'attachments' table. - Bugzilla::Flag->CancelRequests($bug, $obsolete_attachment, $timestamp); + return $params; +} + +sub update { + my $self = shift; + my $dbh = Bugzilla->dbh; + my $user = Bugzilla->user; + my $bug = $self->bug; - $dbh->do('UPDATE attachments SET isobsolete = 1, modification_time = ? - WHERE attach_id = ?', - undef, ($timestamp, $obsolete_attachment->id)); + my $timestamp = shift || $dbh->selectrow_array('SELECT NOW()'); + $self->{modification_time} = $timestamp; - $dbh->do('INSERT INTO bugs_activity (bug_id, attach_id, who, bug_when, - fieldid, removed, added) - VALUES (?,?,?,?,?,?,?)', - undef, ($bug->bug_id, $obsolete_attachment->id, $user->id, - $timestamp, $fieldid, 0, 1)); + my ($changes, $old_self) = $self->SUPER::update(@_); + # Ignore this change. + delete $changes->{modification_time}; + + # Record changes in the activity table. + my $sth = $dbh->prepare('INSERT INTO bugs_activity (bug_id, attach_id, who, bug_when, + fieldid, removed, added) + VALUES (?, ?, ?, ?, ?, ?, ?)'); + + foreach my $field (keys %$changes) { + my $change = $changes->{$field}; + my $fieldid = get_field_id("attachments.$field"); + $sth->execute($bug->id, $self->id, $user->id, $timestamp, + $fieldid, $change->[0], $change->[1]); } - my $attachment = new Bugzilla::Attachment($attachid); - - # 1. Add flags, if any. To avoid dying if something goes wrong - # while processing flags, we will eval() flag validation. - # This requires errors to die(). - # XXX: this can go away as soon as flag validation is able to - # fail without dying. - # - # 2. Flag::validate() should not detect any reference to existing flags - # when creating a new attachment. Setting the third param to -1 will - # force this function to check this point. - my $error_mode_cache = Bugzilla->error_mode; - Bugzilla->error_mode(ERROR_MODE_DIE); - eval { - Bugzilla::Flag::validate($bug->bug_id, -1, SKIP_REQUESTEE_ON_ERROR); - Bugzilla::Flag->process($bug, $attachment, $timestamp, $hr_vars); - }; - Bugzilla->error_mode($error_mode_cache); - if ($@) { - $hr_vars->{'message'} = 'flag_creation_failed'; - $hr_vars->{'flag_creation_error'} = $@; + if (scalar(keys %$changes)) { + $dbh->do('UPDATE bugs SET delta_ts = ? WHERE bug_id = ?', + undef, $timestamp, $bug->id); } - # Return the new attachment object. - return $attachment; + return $changes; } =pod @@ -985,4 +952,50 @@ sub remove_from_db { $dbh->bz_commit_transaction(); } +############################### +#### Helpers ##### +############################### + +# Extract the content type from the attachment form. +sub get_content_type { + my $cgi = Bugzilla->cgi; + + return 'text/plain' if ($cgi->param('ispatch') || $cgi->param('attachurl')); + + my $content_type; + if (!defined $cgi->param('contenttypemethod')) { + ThrowUserError("missing_content_type_method"); + } + elsif ($cgi->param('contenttypemethod') eq 'autodetect') { + defined $cgi->upload('data') || ThrowUserError('file_not_specified'); + # The user asked us to auto-detect the content type, so use the type + # specified in the HTTP request headers. + $content_type = + $cgi->uploadInfo($cgi->param('data'))->{'Content-Type'}; + $content_type || ThrowUserError("missing_content_type"); + + # Set the ispatch flag to 1 if the content type + # is text/x-diff or text/x-patch + if ($content_type =~ m{text/x-(?:diff|patch)}) { + $cgi->param('ispatch', 1); + $content_type = 'text/plain'; + } + } + elsif ($cgi->param('contenttypemethod') eq 'list') { + # The user selected a content type from the list, so use their + # selection. + $content_type = $cgi->param('contenttypeselection'); + } + elsif ($cgi->param('contenttypemethod') eq 'manual') { + # The user entered a content type manually, so use their entry. + $content_type = $cgi->param('contenttypeentry'); + } + else { + ThrowCodeError("illegal_content_type_method", + { contenttypemethod => $cgi->param('contenttypemethod') }); + } + return $content_type; +} + + 1; diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm index 0150f3124..91a97b7a2 100644 --- a/Bugzilla/Constants.pm +++ b/Bugzilla/Constants.pm @@ -148,6 +148,7 @@ use File::Basename; MAX_LOGINCOOKIE_AGE SAFE_PROTOCOLS + LEGAL_CONTENT_TYPES MIN_SMALLINT MAX_SMALLINT @@ -236,18 +237,6 @@ use constant LOGOUT_ALL => 0; use constant LOGOUT_CURRENT => 1; use constant LOGOUT_KEEP_CURRENT => 2; -use constant contenttypes => - { - "html"=> "text/html" , - "rdf" => "application/rdf+xml" , - "atom"=> "application/atom+xml" , - "xml" => "application/xml" , - "js" => "application/x-javascript" , - "csv" => "text/csv" , - "png" => "image/png" , - "ics" => "text/calendar" , - }; - use constant GRANT_DIRECT => 0; use constant GRANT_REGEXP => 2; @@ -377,6 +366,22 @@ use constant SAFE_PROTOCOLS => ('afs', 'cid', 'ftp', 'gopher', 'http', 'https', 'irc', 'mid', 'news', 'nntp', 'prospero', 'telnet', 'view-source', 'wais'); +# Valid MIME types for attachments. +use constant LEGAL_CONTENT_TYPES => ('application', 'audio', 'image', 'message', + 'model', 'multipart', 'text', 'video'); + +use constant contenttypes => + { + "html"=> "text/html" , + "rdf" => "application/rdf+xml" , + "atom"=> "application/atom+xml" , + "xml" => "application/xml" , + "js" => "application/x-javascript" , + "csv" => "text/csv" , + "png" => "image/png" , + "ics" => "text/calendar" , + }; + # Usage modes. Default USAGE_MODE_BROWSER. Use with Bugzilla->usage_mode. use constant USAGE_MODE_BROWSER => 0; use constant USAGE_MODE_CMDLINE => 1; diff --git a/Bugzilla/Flag.pm b/Bugzilla/Flag.pm index 130756459..66c392198 100644 --- a/Bugzilla/Flag.pm +++ b/Bugzilla/Flag.pm @@ -1057,6 +1057,46 @@ sub FormToNewFlags { return \@flags; } +# This is a helper to set flags on a new bug or attachment. +# For existing bugs and attachments, errors must be reported. +sub set_flags { + my ($class, $bug, $attachment, $timestamp, $vars) = @_; + my $cgi = Bugzilla->cgi; + + # The order of these function calls is important, as Flag::validate + # assumes User::match_field has ensured that the + # values in the requestee fields are legitimate user email addresses. + my $match_status = Bugzilla::User::match_field($cgi, { + '^requestee(_type)?-(\d+)$' => { 'type' => 'multi' }, + }, MATCH_SKIP_CONFIRM); + + $vars->{'match_field'} = 'requestee'; + if ($match_status == USER_MATCH_FAILED) { + $vars->{'message'} = 'user_match_failed'; + } + elsif ($match_status == USER_MATCH_MULTIPLE) { + $vars->{'message'} = 'user_match_multiple'; + } + + # 1. Add flags, if any. To avoid dying if something goes wrong + # while processing flags, we will eval() flag validation. + # + # 2. Flag::validate() should not detect any reference to existing flags + # when creating a new attachment. Setting the third param to -1 will + # force this function to check this point. + my $error_mode_cache = Bugzilla->error_mode; + Bugzilla->error_mode(ERROR_MODE_DIE); + eval { + validate($bug->bug_id, $attachment ? -1 : undef, SKIP_REQUESTEE_ON_ERROR); + $class->process($bug, $attachment, $timestamp, $vars); + }; + Bugzilla->error_mode($error_mode_cache); + if ($@) { + $vars->{'message'} = 'flag_creation_failed'; + $vars->{'flag_creation_error'} = $@; + } +} + =pod =over diff --git a/attachment.cgi b/attachment.cgi index 38793ec45..53d189b93 100755 --- a/attachment.cgi +++ b/attachment.cgi @@ -237,20 +237,6 @@ sub validateContext return $context; } -sub validateCanChangeBug -{ - my ($bugid) = @_; - my $dbh = Bugzilla->dbh; - my ($productid) = $dbh->selectrow_array( - "SELECT product_id - FROM bugs - WHERE bug_id = ?", undef, $bugid); - - Bugzilla->user->can_edit_product($productid) - || ThrowUserError("illegal_attachment_edit_bug", - { bug_id => $bugid }); -} - ################################################################################ # Functions ################################################################################ @@ -316,12 +302,8 @@ sub view { # Bug 111522: allow overriding content-type manually in the posted form # params. - if (defined $cgi->param('content_type')) - { - $cgi->param('contenttypemethod', 'manual'); - $cgi->param('contenttypeentry', $cgi->param('content_type')); - Bugzilla::Attachment->validate_content_type(THROW_ERROR); - $contenttype = $cgi->param('content_type'); + if (defined $cgi->param('content_type')) { + $contenttype = $attachment->_check_content_type($cgi->param('content_type')); } # Return the appropriate HTTP response headers. @@ -392,7 +374,7 @@ sub enter { # Retrieve and validate parameters my $bug = Bugzilla::Bug->check(scalar $cgi->param('bugid')); my $bugid = $bug->id; - validateCanChangeBug($bugid); + Bugzilla::Attachment->_check_bug($bug); my $dbh = Bugzilla->dbh; my $user = Bugzilla->user; @@ -434,8 +416,7 @@ sub insert { # Retrieve and validate parameters my $bug = Bugzilla::Bug->check(scalar $cgi->param('bugid')); my $bugid = $bug->id; - validateCanChangeBug($bugid); - my ($timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()"); + my ($timestamp) = $dbh->selectrow_array("SELECT NOW()"); # Detect if the user already used the same form to submit an attachment my $token = trim($cgi->param('token')); @@ -461,8 +442,48 @@ sub insert { } } - my $attachment = - Bugzilla::Attachment->create(THROW_ERROR, $bug, $user, $timestamp, $vars); + # Check attachments the user tries to mark as obsolete. + my @obsolete_attachments; + if ($cgi->param('obsolete')) { + my @obsolete = $cgi->param('obsolete'); + @obsolete_attachments = Bugzilla::Attachment->validate_obsolete($bug, \@obsolete); + } + + # Must be called before create() as it may alter $cgi->param('ispatch'). + my $content_type = Bugzilla::Attachment::get_content_type(); + + my $attachment = Bugzilla::Attachment->create( + {bug => $bug, + creation_ts => $timestamp, + data => scalar $cgi->param('attachurl') || $cgi->upload('data'), + description => scalar $cgi->param('description'), + filename => $cgi->param('attachurl') ? '' : scalar $cgi->upload('data'), + ispatch => scalar $cgi->param('ispatch'), + isprivate => scalar $cgi->param('isprivate'), + isurl => scalar $cgi->param('attachurl'), + mimetype => $content_type, + store_in_file => scalar $cgi->param('bigfile'), + }); + + Bugzilla::Flag->set_flags($bug, $attachment, $timestamp, $vars); + + my $fieldid = get_field_id('attachments.isobsolete'); + + foreach my $obsolete_attachment (@obsolete_attachments) { + # If the obsolete attachment has request flags, cancel them. + # This call must be done before updating the 'attachments' table. + Bugzilla::Flag->CancelRequests($bug, $obsolete_attachment, $timestamp); + + $dbh->do('UPDATE attachments SET isobsolete = 1, modification_time = ? + WHERE attach_id = ?', + undef, ($timestamp, $obsolete_attachment->id)); + + $dbh->do('INSERT INTO bugs_activity (bug_id, attach_id, who, bug_when, + fieldid, removed, added) + VALUES (?,?,?,?,?,?,?)', + undef, ($bug->bug_id, $obsolete_attachment->id, $user->id, + $timestamp, $fieldid, 0, 1)); + } # Insert a comment about the new attachment into the database. my $comment = "Created an attachment (id=" . $attachment->id . ")\n" . @@ -538,25 +559,28 @@ sub edit { || ThrowTemplateError($template->error()); } -# Updates an attachment record. Users with "editbugs" privileges, (or the -# original attachment's submitter) can edit the attachment's description, -# content type, ispatch and isobsolete flags, and statuses, and they can -# also submit a comment that appears in the bug. +# Updates an attachment record. Only users with "editbugs" privileges, +# (or the original attachment's submitter) can edit the attachment. # Users cannot edit the content of the attachment itself. sub update { my $user = Bugzilla->user; my $dbh = Bugzilla->dbh; + # Start a transaction in preparation for updating the attachment. + $dbh->bz_start_transaction(); + # Retrieve and validate parameters my $attachment = validateID(); - my $bug = new Bugzilla::Bug($attachment->bug_id); - $attachment->validate_can_edit($bug->product_id); - validateCanChangeBug($bug->id); - Bugzilla::Attachment->validate_description(THROW_ERROR); - Bugzilla::Attachment->validate_is_patch(THROW_ERROR); - Bugzilla::Attachment->validate_content_type(THROW_ERROR) unless $cgi->param('ispatch'); - $cgi->param('isobsolete', $cgi->param('isobsolete') ? 1 : 0); - $cgi->param('isprivate', $cgi->param('isprivate') ? 1 : 0); + my $bug = $attachment->bug; + $attachment->_check_bug; + $attachment->validate_can_edit($bug->product_id); # FIXME: allow comments anyway. + + $attachment->set_description(scalar $cgi->param('description')); + $attachment->set_is_patch(scalar $cgi->param('ispatch')); + $attachment->set_content_type(scalar $cgi->param('contenttypeentry')); + $attachment->set_is_obsolete(scalar $cgi->param('isobsolete')); + $attachment->set_is_private(scalar $cgi->param('isprivate')); + $attachment->set_filename(scalar $cgi->param('filename')); # Now make sure the attachment has not been edited since we loaded the page. if (defined $cgi->param('delta_ts') @@ -588,17 +612,6 @@ sub update { my $token = $cgi->param('token'); check_hash_token($token, [$attachment->id, $attachment->modification_time]); - # If the submitter of the attachment is not in the insidergroup, - # be sure that he cannot overwrite the private bit. - # This check must be done before calling Bugzilla::Flag*::validate(), - # because they will look at the private bit when checking permissions. - # XXX - This is a ugly hack. Ideally, we shouldn't have to look at the - # old private bit twice (first here, and then below again), but this is - # the less risky change. - unless ($user->is_insider) { - $cgi->param('isprivate', $attachment->isprivate); - } - # If the user submitted a comment while editing the attachment, # add the comment to the bug. Do this after having validated isprivate! if ($cgi->param('comment')) { @@ -607,7 +620,7 @@ sub update { my $comment = "(From update of attachment " . $attachment->id . ")\n" . $cgi->param('comment'); - $bug->add_comment($comment, { isprivate => $cgi->param('isprivate') }); + $bug->add_comment($comment, { isprivate => $attachment->isprivate }); } # The order of these function calls is important, as Flag::validate @@ -618,111 +631,35 @@ sub update { }); Bugzilla::Flag::validate($bug->id, $attachment->id); - # Start a transaction in preparation for updating the attachment. - $dbh->bz_start_transaction(); - # Quote the description and content type for use in the SQL UPDATE statement. - my $description = $cgi->param('description'); - my $contenttype = $cgi->param('contenttype'); - my $filename = $cgi->param('filename'); - # we can detaint this way thanks to placeholders - trick_taint($description); - trick_taint($contenttype); - trick_taint($filename); - - # Figure out when the changes were made. - my ($timestamp) = $dbh->selectrow_array("SELECT NOW()"); + # Figure out when the changes were made. + my ($timestamp) = $dbh->selectrow_array("SELECT NOW()"); - # Update flags. We have to do this before committing changes - # to attachments so that we can delete pending requests if the user - # is obsoleting this attachment without deleting any requests - # the user submits at the same time. - Bugzilla::Flag->process($bug, $attachment, $timestamp, $vars); - - # Update the attachment record in the database. - $dbh->do("UPDATE attachments - SET description = ?, - mimetype = ?, - filename = ?, - ispatch = ?, - isobsolete = ?, - isprivate = ?, - modification_time = ? - WHERE attach_id = ?", - undef, ($description, $contenttype, $filename, - $cgi->param('ispatch'), $cgi->param('isobsolete'), - $cgi->param('isprivate'), $timestamp, $attachment->id)); - - my $updated_attachment = new Bugzilla::Attachment($attachment->id); - # Record changes in the activity table. - my $sth = $dbh->prepare('INSERT INTO bugs_activity (bug_id, attach_id, who, bug_when, - fieldid, removed, added) - VALUES (?, ?, ?, ?, ?, ?, ?)'); - # Flag for updating Last-Modified timestamp if record changed - my $updated = 0; - - if ($attachment->description ne $updated_attachment->description) { - my $fieldid = get_field_id('attachments.description'); - $sth->execute($bug->id, $attachment->id, $user->id, $timestamp, $fieldid, - $attachment->description, $updated_attachment->description); - $updated = 1; - } - if ($attachment->contenttype ne $updated_attachment->contenttype) { - my $fieldid = get_field_id('attachments.mimetype'); - $sth->execute($bug->id, $attachment->id, $user->id, $timestamp, $fieldid, - $attachment->contenttype, $updated_attachment->contenttype); - $updated = 1; - } - if ($attachment->filename ne $updated_attachment->filename) { - my $fieldid = get_field_id('attachments.filename'); - $sth->execute($bug->id, $attachment->id, $user->id, $timestamp, $fieldid, - $attachment->filename, $updated_attachment->filename); - $updated = 1; - } - if ($attachment->ispatch != $updated_attachment->ispatch) { - my $fieldid = get_field_id('attachments.ispatch'); - $sth->execute($bug->id, $attachment->id, $user->id, $timestamp, $fieldid, - $attachment->ispatch, $updated_attachment->ispatch); - $updated = 1; - } - if ($attachment->isobsolete != $updated_attachment->isobsolete) { - my $fieldid = get_field_id('attachments.isobsolete'); - $sth->execute($bug->id, $attachment->id, $user->id, $timestamp, $fieldid, - $attachment->isobsolete, $updated_attachment->isobsolete); - $updated = 1; - } - if ($attachment->isprivate != $updated_attachment->isprivate) { - my $fieldid = get_field_id('attachments.isprivate'); - $sth->execute($bug->id, $attachment->id, $user->id, $timestamp, $fieldid, - $attachment->isprivate, $updated_attachment->isprivate); - $updated = 1; - } + # Update flags. We have to do this before committing changes + # to attachments so that we can delete pending requests if the user + # is obsoleting this attachment without deleting any requests + # the user submits at the same time. + Bugzilla::Flag->process($bug, $attachment, $timestamp, $vars); - if ($updated) { - $dbh->do("UPDATE bugs SET delta_ts = ? WHERE bug_id = ?", undef, - $timestamp, $bug->id); - } - - # Commit the transaction now that we are finished updating the database. - $dbh->bz_commit_transaction(); + $attachment->update($timestamp); + # Commit the comment, if any. + $bug->update(); - # Commit the comment, if any. - $bug->update(); + # Commit the transaction now that we are finished updating the database. + $dbh->bz_commit_transaction(); - # Define the variables and functions that will be passed to the UI template. - $vars->{'mailrecipients'} = { 'changer' => Bugzilla->user->login }; - $vars->{'attachment'} = $attachment; - # We cannot reuse the $bug object as delta_ts has eventually been updated - # since the object was created. - $vars->{'bugs'} = [new Bugzilla::Bug($bug->id)]; - $vars->{'header_done'} = 1; - $vars->{'use_keywords'} = 1 if Bugzilla::Keyword::keyword_count(); + # Define the variables and functions that will be passed to the UI template. + $vars->{'mailrecipients'} = { 'changer' => $user->login }; + $vars->{'attachment'} = $attachment; + $vars->{'bugs'} = [$bug]; + $vars->{'header_done'} = 1; + $vars->{'use_keywords'} = 1 if Bugzilla::Keyword::keyword_count(); - print $cgi->header(); + print $cgi->header(); - # Generate and return the UI (HTML page) from the appropriate template. - $template->process("attachment/updated.html.tmpl", $vars) - || ThrowTemplateError($template->error()); + # Generate and return the UI (HTML page) from the appropriate template. + $template->process("attachment/updated.html.tmpl", $vars) + || ThrowTemplateError($template->error()); } # Only administrators can delete attachments. @@ -742,7 +679,7 @@ sub delete_attachment { # Make sure the administrator is allowed to edit this attachment. my $attachment = validateID(); - validateCanChangeBug($attachment->bug_id); + Bugzilla::Attachment->_check_bug($attachment->bug); $attachment->datasize || ThrowUserError('attachment_removed'); diff --git a/post_bug.cgi b/post_bug.cgi index b450a8691..9a933fc1a 100755 --- a/post_bug.cgi +++ b/post_bug.cgi @@ -85,12 +85,10 @@ if ($token) { } # do a match on the fields if applicable - -&Bugzilla::User::match_field ($cgi, { +Bugzilla::User::match_field ($cgi, { 'cc' => { 'type' => 'multi' }, 'assigned_to' => { 'type' => 'single' }, 'qa_contact' => { 'type' => 'single' }, - '^requestee_type-(\d+)$' => { 'type' => 'multi' }, }); if (defined $cgi->param('maketemplate')) { @@ -194,10 +192,35 @@ if (defined $cgi->param('version')) { # Add an attachment if requested. if (defined($cgi->upload('data')) || $cgi->param('attachurl')) { $cgi->param('isprivate', $cgi->param('commentprivacy')); - my $attachment = Bugzilla::Attachment->create(!THROW_ERROR, - $bug, $user, $timestamp, $vars); + + # Must be called before create() as it may alter $cgi->param('ispatch'). + my $content_type = Bugzilla::Attachment::get_content_type(); + my $attachment; + + # If the attachment cannot be successfully added to the bug, + # we notify the user, but we don't interrupt the bug creation process. + my $error_mode_cache = Bugzilla->error_mode; + Bugzilla->error_mode(ERROR_MODE_DIE); + eval { + $attachment = Bugzilla::Attachment->create( + {bug => $bug, + creation_ts => $timestamp, + data => scalar $cgi->param('attachurl') || $cgi->upload('data'), + description => scalar $cgi->param('description'), + filename => $cgi->param('attachurl') ? '' : scalar $cgi->upload('data'), + ispatch => scalar $cgi->param('ispatch'), + isprivate => scalar $cgi->param('isprivate'), + isurl => scalar $cgi->param('attachurl'), + mimetype => $content_type, + store_in_file => scalar $cgi->param('bigfile'), + }); + }; + Bugzilla->error_mode($error_mode_cache); if ($attachment) { + # Set attachment flags. + Bugzilla::Flag->set_flags($bug, $attachment, $timestamp, $vars); + # Update the comment to include the new attachment ID. # This string is hardcoded here because Template::quoteUrls() # expects to find this exact string. @@ -222,22 +245,8 @@ if (defined($cgi->upload('data')) || $cgi->param('attachurl')) { }; } -# Add flags, if any. To avoid dying if something goes wrong -# while processing flags, we will eval() flag validation. -# This requires errors to die(). -# XXX: this can go away as soon as flag validation is able to -# fail without dying. -my $error_mode_cache = Bugzilla->error_mode; -Bugzilla->error_mode(ERROR_MODE_DIE); -eval { - Bugzilla::Flag::validate($id, undef, SKIP_REQUESTEE_ON_ERROR); - Bugzilla::Flag->process($bug, undef, $timestamp, $vars); -}; -Bugzilla->error_mode($error_mode_cache); -if ($@) { - $vars->{'message'} = 'flag_creation_failed'; - $vars->{'flag_creation_error'} = $@; -} +# Set bug flags. +Bugzilla::Flag->set_flags($bug, undef, $timestamp, $vars); # Email everyone the details of the new bug $vars->{'mailrecipients'} = {'changer' => $user->login}; diff --git a/template/en/default/global/code-error.html.tmpl b/template/en/default/global/code-error.html.tmpl index 9d182d25f..a2863336a 100644 --- a/template/en/default/global/code-error.html.tmpl +++ b/template/en/default/global/code-error.html.tmpl @@ -47,6 +47,14 @@ Attachment #[% attach_id FILTER html %] ([% description FILTER html %]) is already obsolete. + [% ELSIF error == "attachment_local_storage_disabled" %] + [% title = "Local Storage Disabled" %] + You cannot store attachments locally. This feature is disabled. + + [% ELSIF error == "attachment_url_disabled" %] + [% title = "Attachment URL Disabled" %] + You cannot attach a URL. This feature is currently disabled. + [% ELSIF error == "auth_invalid_email" %] [% title = "Invalid Email Address" %] We received an email address ([% addr FILTER html %]) diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl index 338088178..879e62ade 100644 --- a/template/en/default/global/user-error.html.tmpl +++ b/template/en/default/global/user-error.html.tmpl @@ -211,6 +211,11 @@ [% title = "Attachment Deletion Disabled" %] Attachment deletion is disabled on this installation. + [% ELSIF error == "attachment_illegal_url" %] + [% title = "Illegal Attachment URL" %] + [% url FILTER html %] is not a legal URL for attachments. + It must start either with http://, https:// or ftp://. + [% ELSIF error == "attachment_removed" %] [% title = "Attachment Removed" %] The attachment you are attempting to access has been removed. @@ -875,8 +880,7 @@ [% title = "Invalid Content-Type" %] The content type [% contenttype FILTER html %] is invalid. Valid types must be of the form foo/bar where foo - is either application, audio, image, message, model, multipart, - text, or video. + is one of [% constants.LEGAL_CONTENT_TYPES.join(', ') FILTER html %]. [% ELSIF error == "invalid_context" %] [% title = "Invalid Context" %] @@ -1615,6 +1619,11 @@ [% name FILTER html %] does not exist or you are not allowed to see that user. + [% ELSIF error == "user_not_insider" %] + [% title = "User Not In Insidergroup" %] + Sorry, but you are not allowed to (un)mark comments or attachments + as private. + [% ELSIF error == "votes_must_be_nonnegative" %] [% title = "Votes Must Be Non-negative" %] [% admindocslinks = {'voting.html' => 'Setting up the voting feature'} %] -- cgit v1.2.3-24-g4f1b