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.

Support from Acquia helps fund testing for Drupal Acquia logo

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
FileSize
1.12 KB

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
FileSize
1.88 KB

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
FileSize
899 bytes
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
FileSize
1.34 KB

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
FileSize
1.42 KB

@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
FileSize
454 bytes

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
FileSize
2.54 KB

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
FileSize
2.54 KB

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

Issue tags: -7.32 release notes +7.33 release notes

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

Gábor Hojtsy’s picture

Issue tags: -Media Initiative +D8Media

Fix media tag.