Currently the upload module seems to allow any arbitrary setting for the max upload size without validating against the upload_max_filesize set in the PHP environment. This leads to the unexpected failure of uploads with no warning or watchdog error (as far as I can tell). It seems that upload_max_filesize could be determined by calling ini_get(), and this value could be used to validate the user input and display a warning if the allowed size is exceeded. Alternatively, the size could be displayed on with the settings form.

Searching on Drupal.org, I saw a number of people had this problem, but didn't see any issues proposing a fix.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

Title: setting for upload size are not validated against PHP settings » upload sizes limits are not validated against PHP settings

improved title

pwolanin’s picture

related forum topics (many recent):

http://drupal.org/node/67360
http://drupal.org/node/66718
http://drupal.org/node/19326
http://drupal.org/node/50193

apparently the audio module does check this already (maybe we can reuse the code):

http://drupal.org/node/71524

pwolanin’s picture

starting to explore this further- there seems to be NO validation of the input in this form at all! I can enter a letter or anything for the upload size limit.

pwolanin’s picture

Title: upload sizes limits are not validated against PHP settings » upload sizes limits are not validated
Status: Active » Needs review
FileSize
2.75 KB

Initial patch attached- probably needs a little work in terms of the wording, but seems to work in terms of basic validation of the values.

beginner’s picture

would you consider providing a patch for cvs, first, as it probably has the same problem, doesn't it?

beginner’s picture

Now testing the 4.7 version: with or without the patch, I cannot see a "max upload size" setting in admin/settings/upload, only a 'Maximum resolution for uploaded images".

Am I missing something?

beginner’s picture

Status: Needs review » Needs work

Yes, I was missing something :)
One has first to attribute upload rights to some roles, first.

The patch works well. One thing I would add is set form error for the field where the max upload size is greater than the php limit, so that the collapsed form is expanded and the field in red.

beginner’s picture

use the $name of the form field in form_set_error:
http://api.drupal.org/api/HEAD/function/form_set_error

pwolanin’s picture

Yes, I realized later last night that I need to use the form field name and probalby split more of the validation out per field.

There probably out to be validation for the Maximum resolution for uploaded images input as well.

Has this been fixed in Drupal HEAD? If not, need a patch for HEAD too.

beginner’s picture

Version: 4.7.2 » x.y.z

yes, head is broken, too.

pwolanin’s picture

HEAD may be broken too, but it's a bit different from 4.7 since the settings hook is deprecated and we have an obvious form ID to work with: 'upload_admin_settings'

beginner’s picture

Your patch applies fairly well to head. You have to add your functions manually, and there is one more setting to deal with (default upload size), but your code basically works (it's all the same FAPI validation).

pwolanin’s picture

Status: Needs work » Needs review
FileSize
3.33 KB

Ok, here's a nearly finished version for 4.7 (as you say, HEAD is easy after that). Now aslo validates the image size settings and properly sets the error on the form fields.

The only question is whether to be conservative in terms of translation and re-use t-ified text that isn't exactly right for the error message, or to generate additional novel t-ified text.

Heine’s picture

Status: Needs review » Needs work

The max file size that can be uploaded also depends on post_max_size (all uploads + other form fields + headers), approximately upload_max_filesize as the other components are rather small.

pwolanin’s picture

Yes, that's certainly true, but is it Drupal's job to check that the PHP settings are sane (i.e that post_max_size is greater than upload_max_filesize)? It's certainly not hard to add in so that you limit upload size, to the lesser of upload_max_filesize and 80%(?) of post_max_size.

pwolanin’s picture

Ok, look at the attached. Now makes sure the size limit is <= 50% of post_max_size also.

pwolanin’s picture

Status: Needs work » Needs review

changing status

pwolanin’s picture

My chopped off post above tried to say that I picked an arbitray cutoff of 50% of post_max_size as the upper limit for the size of a single upload. FYI, the PHP defualt values are post_max_size = "8M" and upload_max_filesize = "2M" (corresponding to 25%). However, a value of 80% or 90% might be fine too- please review and provide feedback so this can get finalized.

beginner’s picture

Version: x.y.z » 4.7.3
FileSize
3.87 KB

Your patch was against 4.7, right?
Will you follow up with a cvs patch, once this one is committed?
(an extra check will be needed).

I modified the patch:
1) WIDTHxHEIGHT accepts the values 10x20, 10X20 and 10*20.
2) changed the if ... elseif ... elseif ... elseif into straights if ... if ... if in the _validate function so that we get all validation errors at once, instead of one after the other.

I didn't see anything else that needed change. I am happy with the rest of the patch.
so this patch can be set as RTBC if
1) Peter is happy with my two modifications above,
2) there is no other function in core similar to the function _convert_to_MB().
3) everybody is happy about the sanity check- a single upload should not be more than 50% the size limit of the total post (I am - it seems reasonable).
4) someone can confirm that $usersize (the maximum size of all files a user can have on the site) CAN in practice be SUPERIOR to ini_get('post_max_size').

Peter, if you are fairly sure that the four points above are ok, then set this as RTBC.

beginner’s picture

darn! I am just thinking about this change:

1) WIDTHxHEIGHT accepts the values 10x20, 10X20 and 10*20.

It seemed a logical and simple change to make (when being asked to enter a value "width x height" my first reaction would be to use the programming equivalent of x -multiply by- which is *).
But I don't know how this value is being used elsewhere, so this change may break the code elsewhere,
so you can
a) either revert my change,
b) or modify the rest of the code so that it accepts a value 10*20
c) or change 10*20 into 10x20 on the fly after input.

pwolanin’s picture

The 'x' is used selsewhere in the upload module to parse the size, so my inclination would be just to leave that unchanged (i.e. revert) for now. It cannot be 'X' or '*' unless we change code elsewhere (or convert X or * to x).

I think presenting all the errors at once is a good idea- however if the values fail the is_numeric() test, do you want to be presnting erros based on (for example) $uploadsize > upload_max_size()? Seems like this could generate a meaningless additional error message. http://www.php.net/manual/en/language.operators.comparison.php

I'll look at the rest in more detail in a bit.

pwolanin’s picture

In response to an unanswered question above, I will covert the patch for HEAD as soon as we're satisfied with it.

After looking again I think we should just leave 'x' as the only delimiter. The form is generated using system_settings_form('upload_admin_settings', $form), so the the submit is handled by the system module's code. We'd have to write a submit function also to do a substitution of anything else to 'x'.

The alternative is to convert this line in function _upload_image($file):

    list($width, $height) = explode('x', variable_get('upload_max_resolution', 0));

to something like

preg_match(/^([0-9]+)\D([0-9]+)$/,variable_get('upload_max_resolution', 0),$matches);
$width=$matches[1];
$height=$matches[2];
pwolanin’s picture

Ok, my fears about having the validations non-nested in elseif seems to be unfounded. Apparently only the first error set on a field is displayed to the user.

Attached patch (for 4.7) is a combination of the last two patches- image size only uses 'x' delimiter.

pwolanin’s picture

slightly improved wording.

Working on a patch for HEAD- this module changed quite a bit- additional fields are in the form and the function name and form ID changed.

pwolanin’s picture

Patch for HEAD

pwolanin’s picture

Version: 4.7.3 » x.y.z
Assigned: Unassigned » pwolanin

changing version to CVS, assigning to me unless anyone else wants to take it!

beginner’s picture

Status: Needs review » Reviewed & tested by the community

I tested and reviewed the additional code for cvs.

#24 is RTBC for 4.7 and #25 for head.

drumm’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
4.5 KB

I fixed a munch of code style issues including:
- Spaces around all operators, except . next to '.
- Prefer 'foo'. $bar instead of "foo$bar".
- Remove spaces at ends of lines.
- Avoid breaking in the middle of lines.
- Use [] for string indexing instead of {}.

Here is what needs work:
- Error messages

+     form_set_error('upload_uploadsize_'. $rid, t('error') .': '. t('Maximum file size per upload') .' > '. t('Total file size per user'));

That one is particularly bad. There is no need for the 'error:' prefix and the message itself needs to be a full English sentence. In others: I think we should use 'a number' instead of 'numeric' and the PHP limit needs to be explained a bit more, such as a hint for where it might be changed.
- Use theme('placeholder', ...) to wrap things like $role in

+      form_set_error('upload_usersize_'. $rid, t('%role file size limits must be numeric and greater than zero.', array('%role' => $role)));

- Put upload_max_size() and _convert_to_MB() in file.inc and make sure both are properly named- they must begin with file_ or _file_ and be all lower-cased.

drumm’s picture

I missed another code style issue, there should be a space before the { when starting a block with if, for, etc.

That should be fixed.

pwolanin’s picture

@drumm ok - will work on style and other issues as you suggest. A question on the use of t-ified strings: some of the ackqard constructions were my attempt to avoid adding additional strings to be translated (especially for 4.7). Is this something that should be considered?

pwolanin’s picture

@drumm- in regards to this syle point: - Prefer 'foo'. $bar instead of "foo$bar".
I used the existing upload.module code as a model. would you like to see the code in function upload_admin_settings() changed to conform to this style?

pwolanin’s picture

Status: Needs work » Needs review
FileSize
7.27 KB

Attached patch for HEAD tried to address the above concerns. Please review. I'll adapt back to 4.7 after it's RTBC.

pwolanin’s picture

Silghtly improved patch attached- only shows the extra help text once. Also, doesn't use theme('placeholder') for the term 'default'.

pwolanin’s picture

Further improved error messages.

beginner’s picture

Status: Needs review » Reviewed & tested by the community

good.

pwolanin’s picture

Here's a patch for 4.7 where the help text, etc matches the one for HEAD. I didn't change the double-quoted items, since i'm not sure if this sort of thing qualifies as a "bug" for the stable branch code.

drumm’s picture

Status: Reviewed & tested by the community » Fixed

Committed to HEAD.

killes@www.drop.org’s picture

backported to 4.7

Anonymous’s picture

Status: Fixed » Closed (fixed)