Bug 454251: Implement Bugzilla::Attachment->create() and $attachment->update() - Patch by Frédéric Buclin <LpSolit@gmail.com> a=LpSolit (module owner)

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<flags>
@@ -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<validate_is_patch()>
-
-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<validate_description()>
-
-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<validate_content_type()>
-
-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<validate_can_edit($attachment, $product_id)>
 
 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<create($throw_error, $bug, $user, $timestamp, $hr_vars)>
+=item C<create>
 
-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<bug> - 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<data> - Either a filehandle pointing to the content of the
+            attachment, or the content of the attachment itself.
+            C<description> - string - describe what the attachment is about.
+            C<filename> - 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<mimetype> - string - a valid MIME type.
+            C<creation_ts> - string (optional) - timestamp of the insert
+            as returned by SELECT NOW().
+            C<ispatch> - boolean (optional, default false) - true if the
+            attachment is a patch.
+            C<isprivate> - boolean (optional, default false) - true if
+            the attachment is private.
+            C<isurl> - boolean (optional, default false) - true if the
+            attachment is a URL pointing to some external ressource.
+            C<store_in_file> - 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 (<b>[% addr FILTER html %]</b>)
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" %]
+    <em>[% url FILTER html %]</em> 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 <em>[% contenttype FILTER html %]</em> is invalid.
     Valid types must be of the form <em>foo/bar</em> where <em>foo</em>
-    is either <em>application, audio, image, message, model, multipart,
-    text,</em> or <em>video</em>.
+    is one of <em>[% constants.LEGAL_CONTENT_TYPES.join(', ') FILTER html %]</em>.
     
   [% ELSIF error == "invalid_context" %]
     [% title = "Invalid Context" %]
@@ -1615,6 +1619,11 @@
     <tt>[% name FILTER html %]</tt> 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'} %]