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
#29 file-validate-size-1468210-26.patch2.54 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 40,897 pass(es).
[ View ]
#26 file-validate-size-1468210-26.patch2.54 KBDevin Carlson
FAILED: [[SimpleTest]]: [MySQL] 40,928 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#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.

marthinal’s picture

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.

marthinal’s picture

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.

dagmar’s picture

Status:Needs work» Needs review
quicksketch’s picture

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.

Dries’s picture

Status:Reviewed & tested by the community» Fixed

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

Eric_A’s picture

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.

Eric_A’s picture

Status:Needs work» Needs review
StatusFileSize
new899 bytes
PASSED: [[SimpleTest]]: [MySQL] 41,333 pass(es).
[ View ]
Tor Arne Thune’s picture

Status:Needs review» Reviewed & tested by the community
tstoeckler’s picture

Status:Reviewed & tested by the community» Needs work

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

marthinal’s picture

Assigned:Unassigned» marthinal
marthinal’s picture

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

Sintax revised.

marthinal’s picture

Status:Needs review» Reviewed & tested by the community
tstoeckler’s picture

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!

marthinal’s picture

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.

tstoeckler’s picture

Status:Needs review» Reviewed & tested by the community

Yup, looks good! :-)

Dries’s picture

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.

David_Rothstein’s picture

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.

David_Rothstein’s picture

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).

Tor Arne Thune’s picture

Status:Needs review» Reviewed & tested by the community

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

alexpott’s picture

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.

Dave Reid’s picture

Issue summary:View changes
Issue tags:+Media Initiative
Devin Carlson’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new2.54 KB
FAILED: [[SimpleTest]]: [MySQL] 40,928 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

A combined backport of #4, #17 and #21.

dcam’s picture

Status:Needs review» Reviewed & tested by the community

I checked #26 and it looks good to me. It is a backport of #4, #17, and #21. After applying the patch I could no longer bypass the file size validation check.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 26: file-validate-size-1468210-26.patch, failed testing.

tstoeckler’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new2.54 KB
PASSED: [[SimpleTest]]: [MySQL] 40,897 pass(es).
[ View ]

Random fail, so re-uploading.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 29: file-validate-size-1468210-26.patch, failed testing.

dcam’s picture

Status:Needs work» Needs review
mgifford’s picture

Status:Needs review» Reviewed & tested by the community

Back to RTBC.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 29: file-validate-size-1468210-26.patch, failed testing.

Status:Needs work» Needs review
dcam’s picture

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 29: file-validate-size-1468210-26.patch, failed testing.

Status:Needs work» Needs review
dcam’s picture

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 29: file-validate-size-1468210-26.patch, failed testing.

Status:Needs work» Needs review
dcam’s picture

Status:Needs review» Reviewed & tested by the community
David_Rothstein’s picture

Status:Reviewed & tested by the community» Fixed
Issue tags:+7.32 release notes

Committed to 7.x - thanks!

  • David_Rothstein committed 99c124d on 7.x
    Issue #1468210 by marthinal, quicksketch, tstoeckler, Devin Carlson,...
David_Rothstein’s picture

Simple followup cleanup patch, by the way (relevant for both Drupal 7 and Drupal 8): #2331151: Remove leftover code in testFileValidateSize() which runs the tests as a specific user

Status:Fixed» Closed (fixed)

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

David_Rothstein’s picture

Drupal 7.32 was a security release, so this is now scheduled to be in 7.33.