Skip to:
Content

BuddyPress.org

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

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

.patch (2.1 KB) - added by wordplus 2 years ago.
7674.01.patch (1.2 KB) - added by r-a-y 2 years ago.

Download all attachments as: .zip

Change History (19)

#1 @andrewteg
2 years ago

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.

Last edited 2 years ago by andrewteg (previous) (diff)

#2 @DJPaul
2 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 @andrewteg
2 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: @DJPaul
2 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_file() on those machines (maybe via WP-CLI) and verify the behaviour.

Last edited 2 years ago by DJPaul (previous) (diff)

This ticket was mentioned in Slack in #buddypress by djpaul. View the logs.


2 years ago

#6 in reply to: ↑ 4 @wordplus
2 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

Last edited 2 years ago by wordplus (previous) (diff)

@wordplus
2 years ago

#7 follow-up: @DJPaul
2 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 @wordplus
2 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 @DJPaul
2 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).

@r-a-y
2 years ago

#10 @r-a-y
2 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.

#11 @r-a-y
2 years ago

  • Keywords has-patch added

#12 @mercime
2 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.

Last edited 2 years ago by mercime (previous) (diff)

#13 @DJPaul
2 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!

#14 @r-a-y
2 years ago

Just tested on a nginx install and uploading still works.

Going to commit.

#15 @r-a-y
2 years ago

  • Owner set to r-a-y
  • Resolution set to fixed
  • Status changed from new to closed

In 11873:

Attachments: Fix uploading for those on Windows/WAMP installs.

Props andrewteg, r-a-y.

Fixes #7674 (2.9-branch).

#16 @r-a-y
2 years ago

In 11874:

Attachments: Fix uploading for those on Windows/WAMP installs.

Props andrewteg, r-a-y.

Fixes #7674 (trunk).

#17 @DJPaul
2 years ago

  • Milestone changed from 2.9.3 to 2.9.4

Milestone renamed

Note: See TracTickets for help on using tickets.