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.
Comment | File | Size | Author |
---|---|---|---|
#29 | file-validate-size-1468210-26.patch | 2.54 KB | tstoeckler |
#26 | file-validate-size-1468210-26.patch | 2.54 KB | Devin Carlson |
#21 | file-validate-size-1468210-21.patch | 454 bytes | David_Rothstein |
#17 | d8-file_validate_size_user1-1468210-17.patch | 1.42 KB | marthinal |
#14 | d8-file_validate_size_user1-1468210-13.patch | 1.34 KB | marthinal |
Comments
Comment #2
marthinal CreditAttribution: marthinal commentedWe need $user to check the size.So if we want to check every user also 1, I modified a little bit the patch.
Comment #4
marthinal CreditAttribution: marthinal commentedModified test also.
Comment #6
dagmar#4: d8-file_validate_size_user1-1468210-4.patch queued for re-testing.
Comment #7
quicksketchThanks @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.
Comment #8
Dries CreditAttribution: Dries commentedI agree that it makes sense to remove these special cases. Committed to 8.x. Thanks.
Comment #9
Eric_A CreditAttribution: Eric_A commentedThe inline comment indentation needed to be changed as well.
Comment #10
Eric_A CreditAttribution: Eric_A commentedComment #11
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #12
tstoecklerActually the indentation of everything inside the (nested) if statements needs to be changed as well.
Comment #13
marthinal CreditAttribution: marthinal commentedComment #14
marthinal CreditAttribution: marthinal commentedSintax revised.
Comment #15
marthinal CreditAttribution: marthinal commentedComment #16
tstoecklerThanks 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:
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!
Comment #17
marthinal CreditAttribution: marthinal commented@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.
Comment #18
tstoecklerYup, looks good! :-)
Comment #19
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.
Comment #21
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.
Comment #22
David_Rothstein CreditAttribution: David_Rothstein commentedHa, 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:
Comment #23
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedYes, I'm sure there are others (me included) that have come across this.
Comment #24
alexpottCommitted 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.
Comment #25
Dave ReidComment #26
Devin Carlson CreditAttribution: Devin Carlson commentedA combined backport of #4, #17 and #21.
Comment #27
dcam CreditAttribution: dcam commentedI 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.
Comment #29
tstoecklerRandom fail, so re-uploading.
Comment #31
dcam CreditAttribution: dcam commented29: file-validate-size-1468210-26.patch queued for re-testing.
Comment #32
mgiffordBack to RTBC.
Comment #35
dcam CreditAttribution: dcam commentedComment #38
dcam CreditAttribution: dcam commentedComment #41
dcam CreditAttribution: dcam commentedComment #42
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks!
Comment #44
David_Rothstein CreditAttribution: David_Rothstein commentedSimple 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
Comment #46
David_Rothstein CreditAttribution: David_Rothstein commentedDrupal 7.32 was a security release, so this is now scheduled to be in 7.33.
Comment #47
Gábor HojtsyFix media tag.