From 3c61e66fe3d24f183628a7396a3fcd720a95abeb Mon Sep 17 00:00:00 2001 From: Frédéric Buclin Date: Thu, 8 Jul 2010 18:58:33 +0200 Subject: Bug 490930: Always store attachments locally if they are over X size (and below some threshold!), don't ever display "Big File" checkbox r=mkanat a=LpSolit --- Bugzilla/Attachment.pm | 116 ++++++++++++++++--------------------------------- 1 file changed, 37 insertions(+), 79 deletions(-) (limited to 'Bugzilla/Attachment.pm') diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm index ddce1f593..39afe5df1 100644 --- a/Bugzilla/Attachment.pm +++ b/Bugzilla/Attachment.pm @@ -59,6 +59,9 @@ use Bugzilla::Util; use Bugzilla::Field; use Bugzilla::Hook; +use File::Copy; +use List::Util qw(max); + use base qw(Bugzilla::Object); ############################### @@ -108,7 +111,6 @@ use constant VALIDATORS => { isprivate => \&_check_is_private, isurl => \&_check_is_url, mimetype => \&_check_content_type, - store_in_file => \&_check_store_in_file, }; use constant UPDATE_VALIDATORS => { @@ -538,52 +540,29 @@ sub _check_content_type { sub _check_data { my ($invocant, $params) = @_; - my $data; + my $data = $params->{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; + $params->{filesize} = length($data); } 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}; - } - else { - # The file will be stored in the DB. We need the content of the file. - local $/; - my $fh = $params->{data}; - $data = <$fh>; - } + $params->{filesize} = ref $data ? -s $data : length($data); } Bugzilla::Hook::process('attachment_process_data', { data => \$data, attributes => $params }); - # Do not validate the size if we have a filehandle. It will be checked later. - return $data if ref $data; - - $data || ThrowUserError('zero_length_file'); + $params->{filesize} || ThrowUserError('zero_length_file'); # Make sure the attachment does not exceed the maximum permitted size. - my $len = length($data); - my $max_size = $params->{store_in_file} ? Bugzilla->params->{'maxlocalattachment'} * 1048576 - : Bugzilla->params->{'maxattachmentsize'} * 1024; - if ($len > $max_size) { - my $vars = { filesize => sprintf("%.0f", $len/1024) }; - if ($params->{ispatch}) { - ThrowUserError('patch_too_large', $vars); - } - elsif ($params->{store_in_file}) { - ThrowUserError('local_file_too_large'); - } - else { - ThrowUserError('file_too_large', $vars); - } + my $max_size = max(Bugzilla->params->{'maxlocalattachment'} * 1048576, + Bugzilla->params->{'maxattachmentsize'} * 1024); + + if ($params->{filesize} > $max_size) { + my $vars = { filesize => sprintf("%.0f", $params->{filesize}/1024) }; + ThrowUserError('file_too_large', $vars); } return $data; } @@ -642,15 +621,6 @@ sub _check_is_url { return $is_url ? 1 : 0; } -sub _check_store_in_file { - my ($invocant, $store_in_file) = @_; - - if ($store_in_file && !Bugzilla->params->{'maxlocalattachment'}) { - ThrowCodeError('attachment_local_storage_disabled'); - } - return $store_in_file ? 1 : 0; -} - =pod =head2 Class Methods @@ -815,9 +785,6 @@ Params: takes a hashref with the following keys: 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. @@ -833,53 +800,44 @@ sub create { # 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}; + my $data = delete $params->{data}; + my $size = delete $params->{filesize}; 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. - 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 ($store_in_file) { + # The file is too large to be stored in the DB, so we store it locally. + if ($size > Bugzilla->params->{'maxattachmentsize'} * 1024) { my $attachdir = bz_locations()->{'attachdir'}; 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; - if (ref $fh) { - my $limit = Bugzilla->params->{"maxlocalattachment"} * 1048576; - 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; + if (ref $data) { + copy($data, "$attachdir/$hash/attachment.$attachid"); + close $data; } else { - print AH $fh; + open(AH, '>', "$attachdir/$hash/attachment.$attachid"); + binmode AH; + print AH $data; + close AH; } - close AH; + $data = ''; # Will be stored in the DB. + } + # If we have a filehandle, we need its content to store it in the DB. + elsif (ref $data) { + local $/; + $data = <$data>; } + my $sth = $dbh->prepare("INSERT INTO attach_data + (id, thedata) VALUES ($attachid, ?)"); + + trick_taint($data); + $sth->bind_param(1, $data, $dbh->BLOB_TYPE); + $sth->execute(); + # Return the new attachment object. return $attachment; } -- cgit v1.2.3-24-g4f1b