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.
Comment | File | Size | Author |
---|---|---|---|
#36 | validate_upload_sizes_47_36.txt | 4.9 KB | pwolanin |
#34 | validate_upload_sizes_HEAD_34.txt | 7.38 KB | pwolanin |
#33 | validate_upload_sizes_HEAD_33.txt | 7.3 KB | pwolanin |
#32 | validate_upload_sizes_HEAD_32.txt | 7.27 KB | pwolanin |
#28 | upload.diff.txt | 4.5 KB | drumm |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedimproved title
Comment #2
pwolanin CreditAttribution: pwolanin commentedrelated 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
Comment #3
pwolanin CreditAttribution: pwolanin commentedstarting 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.
Comment #4
pwolanin CreditAttribution: pwolanin commentedInitial patch attached- probably needs a little work in terms of the wording, but seems to work in terms of basic validation of the values.
Comment #5
beginner CreditAttribution: beginner commentedwould you consider providing a patch for cvs, first, as it probably has the same problem, doesn't it?
Comment #6
beginner CreditAttribution: beginner commentedNow 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?
Comment #7
beginner CreditAttribution: beginner commentedYes, 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.
Comment #8
beginner CreditAttribution: beginner commenteduse the $name of the form field in form_set_error:
http://api.drupal.org/api/HEAD/function/form_set_error
Comment #9
pwolanin CreditAttribution: pwolanin commentedYes, 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.
Comment #10
beginner CreditAttribution: beginner commentedyes, head is broken, too.
Comment #11
pwolanin CreditAttribution: pwolanin commentedHEAD 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'
Comment #12
beginner CreditAttribution: beginner commentedYour 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).
Comment #13
pwolanin CreditAttribution: pwolanin commentedOk, 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.
Comment #14
Heine CreditAttribution: Heine commentedThe 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.
Comment #15
pwolanin CreditAttribution: pwolanin commentedYes, 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.
Comment #16
pwolanin CreditAttribution: pwolanin commentedOk, look at the attached. Now makes sure the size limit is <= 50% of post_max_size also.
Comment #17
pwolanin CreditAttribution: pwolanin commentedchanging status
Comment #18
pwolanin CreditAttribution: pwolanin commentedMy 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.
Comment #19
beginner CreditAttribution: beginner commentedYour 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.
Comment #20
beginner CreditAttribution: beginner commenteddarn! 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.
Comment #21
pwolanin CreditAttribution: pwolanin commentedThe '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.phpI'll look at the rest in more detail in a bit.
Comment #22
pwolanin CreditAttribution: pwolanin commentedIn 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):
to something like
Comment #23
pwolanin CreditAttribution: pwolanin commentedOk, 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.
Comment #24
pwolanin CreditAttribution: pwolanin commentedslightly 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.
Comment #25
pwolanin CreditAttribution: pwolanin commentedPatch for HEAD
Comment #26
pwolanin CreditAttribution: pwolanin commentedchanging version to CVS, assigning to me unless anyone else wants to take it!
Comment #27
beginner CreditAttribution: beginner commentedI tested and reviewed the additional code for cvs.
#24 is RTBC for 4.7 and #25 for head.
Comment #28
drummI 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
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- 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.Comment #29
drummI missed another code style issue, there should be a space before the
{
when starting a block with if, for, etc.That should be fixed.
Comment #30
pwolanin CreditAttribution: pwolanin commented@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?
Comment #31
pwolanin CreditAttribution: pwolanin commented@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?
Comment #32
pwolanin CreditAttribution: pwolanin commentedAttached patch for HEAD tried to address the above concerns. Please review. I'll adapt back to 4.7 after it's RTBC.
Comment #33
pwolanin CreditAttribution: pwolanin commentedSilghtly improved patch attached- only shows the extra help text once. Also, doesn't use theme('placeholder') for the term 'default'.
Comment #34
pwolanin CreditAttribution: pwolanin commentedFurther improved error messages.
Comment #35
beginner CreditAttribution: beginner commentedgood.
Comment #36
pwolanin CreditAttribution: pwolanin commentedHere'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.
Comment #37
drummCommitted to HEAD.
Comment #38
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedbackported to 4.7
Comment #39
(not verified) CreditAttribution: commented