#7674 closed defect (bug) (fixed)
Cover Image & More Cannot be Uploaded on Windows Servers with 2.9.3 Update
Reported by: | andrewteg | Owned by: | r-a-y |
---|---|---|---|
Milestone: | 2.9.4 | Priority: | normal |
Severity: | normal | Version: | 2.9.2 |
Component: | Media | Keywords: | has-patch |
Cc: |
Description
In the 2.9.3 update bp-core/bp-core-attachments.php line 1317 changes from
if ( ! is_dir( $cover_dir ) ) {
to
if ( 0 !== validate_file( $cover_dir ) || ! is_dir( $cover_dir ) ) {
Using validate_file on a directory will always return 2 in a Windows/WAMP environment because a directory is always something like
C:\Apache24\htdocs\buddypress/wp-content/uploads/buddypress/groups/4/cover-image
This produces a general error with the default message on a WAMP install that upgrades to 2.9.3.
I believe it also affects line 450 as that uses validate_file on $type_dir, but I have only confirmed it on line 1317 with $cover_dir.
If I roll just that line back to 2.9.2 version cover update works great.
Thanks,
Andrew
Attachments (2)
Change History (19)
#2
@
7 years ago
- Component changed from Core to Media
- Milestone changed from Awaiting Review to 2.9.3
- Version set to 2.9.2
Thanks for the report, and I'm sorry we broke this. None of the people involved in the 2.9.3 release had Windows environments to test on.
#3
@
7 years ago
No worries here. It's a great product and we appreciate all your hard work on every release. We just found it on a test setup and wanted to let everyone know to help out in whatever little way we can!
#4
follow-up:
↓ 6
@
7 years ago
Discussion on Slack https://wordpress.slack.com/archives/C02RQBYUG/p1517258207000486 has suggested that we could check for a return value of 1
here instead.
We've found some team members with access to Windows servers, so next step is to test validate_function()
on those machines (maybe via WP-CLI) and verify the behaviour.
This ticket was mentioned in Slack in #buddypress by djpaul. View the logs.
7 years ago
#6
in reply to:
↑ 4
@
7 years ago
Replying to DJPaul:
Discussion on Slack https://wordpress.slack.com/archives/C02RQBYUG/p1517258207000486 has suggested that we could check for a return value of
1
here instead.
We've found some team members with access to Windows servers, so next step is to test
validate_file()
on those machines (maybe via WP-CLI) and verify the behaviour.
I think the problem is that, validate_file performs abit another function.
This function should check only relative paths, here its trying to check absolute path, thats why its not working properly.
We should remove it from that conditional or change what we checking.
https://developer.wordpress.org/reference/functions/validate_file/#comment-1494
#7
follow-up:
↓ 8
@
7 years ago
No, we can't remove the check from the attachments class. The point of adding those was to fix and harden security issues. Taking that out will not help.
#8
in reply to:
↑ 7
@
7 years ago
Replying to DJPaul:
No, we can't remove the check from the attachments class. The point of adding those was to fix and harden security issues. Taking that out will not help.
hi there, check patch file, possible move validate function to preupload filter and this will work well.
#9
@
7 years ago
@espellcaste and @r-a-y, you said on Slack you've got Windows development machines. Is it possible one of you could pick this up?
First I think we need some testing of validate_file() on Windows with absolute paths to confirm the behaviour. Then, think about how to fix this (checking for a return type of 1 or 2 is probably the way forward, not sure which is best) and create a patch. Once we know it works on Windows, you/we/I can then test that patch on a Linux server and check it still works there.
Let me know if either of you are able to take this on in the next two weeks (I can volunteer to do a release on 21st February if this is fixed by then).
#10
@
7 years ago
01.patch
fixes this issue by seeing if validate_file()
returns 1
meaning a directory traversal attempt, which is really all we are concerned about.
Tested on a WAMP install and it works.
#12
@
7 years ago
Installed WAMP, saw the issue reported using Chrome, FF, and Edge.
Tested @r-a-y's patch and confirmed patch fixed the issue using 3 browsers above.
Edit - P.S. Patch tested and works on IE 9-11 as well.
#13
@
7 years ago
If this has been tested on non-windows and it’s fine, let’s get committed — you don’t need me to approve a fix!
Confirmed that line 450 does indeed to be updated to get the cover image to save. It will update the display but not save properly without removing validate_file from the check on line 450.
Looks like it was https://buddypress.trac.wordpress.org/changeset/11820/ where it originally got updated.