Right now in filefield_validate_size(), we have a special check that allows user #1 to have an unlimited file quota. This is pretty dumb on a lot of accounts:

- We eliminated almost all user #1 checks in Drupal 7, it's bizarre that we still have this one.
- The help text for the user (as provided by filefield_validate_size_help()) still indicates that there is a file size limit, but it's not enforced for user #1.
- This makes testing while logged in as user #1 quite confusing. I literally had started drafting a somewhat panicked security team e-mail because I thought file size validation had been broken in the most recent D7 release ("because I know it was working last week", so I thought).
- If there are any user file quotas on a site (and there usually aren't) user #1 will be in the admin-role which will likely have an unlimited quota.

I'm sure this check is in place because it also exists in the D6 FileField, from which I'll be more than happy to remove it there also. It's also worth noting that no where in core do we actually utilize the user-limit check anyway, so this is even less likely to have an impact on any existing sites.

Files: 
CommentFileSizeAuthor
#21 file-validate-size-1468210-21.patch454 bytesDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 55,888 pass(es).
[ View ]
#17 d8-file_validate_size_user1-1468210-17.patch1.42 KBmarthinal
PASSED: [[SimpleTest]]: [MySQL] 41,436 pass(es).
[ View ]
#14 d8-file_validate_size_user1-1468210-13.patch1.34 KBmarthinal
PASSED: [[SimpleTest]]: [MySQL] 41,379 pass(es).
[ View ]
#10 d8-file_validate_size_user1-1468210-10.patch899 bytesEric_A
PASSED: [[SimpleTest]]: [MySQL] 41,333 pass(es).
[ View ]
#4 d8-file_validate_size_user1-1468210-4.patch1.88 KBmarthinal
PASSED: [[SimpleTest]]: [MySQL] 41,223 pass(es).
[ View ]
#2 d8-file_validate_size_user1-1468210-2.patch1.12 KBmarthinal
FAILED: [[SimpleTest]]: [MySQL] 41,239 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
file_validate_size_user1-d7.patch1.63 KBquicksketch
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file_validate_size_user1-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
file_validate_size_user1-d8.patch1.65 KBquicksketch
FAILED: [[SimpleTest]]: [MySQL] 35,056 pass(es), 1 fail(s), and 6 exception(s).
[ View ]

Comments

Status:Needs review» Needs work

The last submitted patch, file_validate_size_user1-d7.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.12 KB
FAILED: [[SimpleTest]]: [MySQL] 41,239 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

We need $user to check the size.So if we want to check every user also 1, I modified a little bit the patch.

Status:Needs review» Needs work

The last submitted patch, d8-file_validate_size_user1-1468210-2.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.88 KB
PASSED: [[SimpleTest]]: [MySQL] 41,223 pass(es).
[ View ]

Modified test also.

Status:Needs review» Needs work

The last submitted patch, d8-file_validate_size_user1-1468210-4.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

We need $user to check the size.So if we want to check every user also 1, I modified a little bit the patch.

Thanks @marthinal, an obvious oversight on my part. I'm not sure why I never got back to this patch. The new reroll looks great. Nice work on updating the tests also, I'm not sure they existed at the time that I rolled the first patch.

Status:Reviewed & tested by the community» Fixed

I agree that it makes sense to remove these special cases. Committed to 8.x. Thanks.

Category:task» bug
Status:Fixed» Needs work

     // Save a query by only calling file_space_used() when a limit is provided.
-    if ($user_limit && (file_space_used($user->uid) + $file->filesize) > $user_limit) {
+  if ($user_limit && (file_space_used($user->uid) + $file->filesize) > $user_limit) {

The inline comment indentation needed to be changed as well.

Status:Needs work» Needs review
StatusFileSize
new899 bytes
PASSED: [[SimpleTest]]: [MySQL] 41,333 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

Actually the indentation of everything inside the (nested) if statements needs to be changed as well.

Assigned:Unassigned» marthinal

Status:Needs work» Needs review
StatusFileSize
new1.34 KB
PASSED: [[SimpleTest]]: [MySQL] 41,379 pass(es).
[ View ]

Sintax revised.

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

Thanks for the new patch. That was exactly what I meant. I'll have to send it back to needs work, though, for one tiny little thing:

+++ b/core/modules/file/file.module
@@ -531,13 +531,14 @@ function file_validate_size(File $file, $file_limit = 0, $user_limit = 0) {
+  ¶

Sorry, but this introduces whitespace errors.

Also, it's generally considered bad practice to mark your own patches RTBC. With patches as small and trivial as this one that rule can be irritating, but generally we try to stick by it pretty closely. I'll make sure I'll have another look and mark it RTBC if you post an updated patch. Thanks again!

Status:Needs work» Needs review
StatusFileSize
new1.42 KB
PASSED: [[SimpleTest]]: [MySQL] 41,436 pass(es).
[ View ]

@tstoeckler excuse about RTBC directly ... didn't know that.

Here the patch again, the problem was with my netbeans but now I think everything works fine.

Thanks.

Status:Needs review» Reviewed & tested by the community

Yup, looks good! :-)

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks.

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Title:Remove special $user->uid == 1 check in filefield_validate_size()Remove special $user->uid == 1 check in file_validate_size()
Status:Closed (fixed)» Needs review
Issue tags:+needs backport to D7
StatusFileSize
new454 bytes
PASSED: [[SimpleTest]]: [MySQL] 55,888 pass(es).
[ View ]

I just ran into this in Drupal 7 (almost thought I discovered a security issue because my uploads weren't being validated...)

This is unexpected behavior which borders on broken (a configuration setting in the admin interface is not being respected) and I think we should consider backporting it although it is kind of a behavior change.

However, first let's fix the documentation in Drupal 8 which was never updated for this change.

Ha, just read the issue summary and apparently I'm not the only one who thought they discovered a D7 security hole as a result of this:

- This makes testing while logged in as user #1 quite confusing. I literally had started drafting a somewhat panicked security team e-mail because I thought file size validation had been broken in the most recent D7 release ("because I know it was working last week", so I thought).

Status:Needs review» Reviewed & tested by the community

Yes, I'm sure there are others (me included) that have come across this.

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Committed b4eaea2 and pushed to 8.x. Thanks!

Let the backport discussion begin... from what's being said fixing this is D7 makes sense to me.