WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 4 years ago

#5953 closed defect (bug) (fixed)

Absolute upload_path fails

Reported by: tellyworth Owned by: tellyworth
Milestone: Priority: normal
Severity: normal Version:
Component: Upload Keywords:
Focuses: Cc:

Description

  1. On Options/Misc, enter "/tmp/foo/bar" as a value for the upload path setting.
  2. Write a new post and attach an image.

Expected: the image file should be stored as /tmp/foo/bar/2008/02/file.jpg or similar.

Actual: a 'tmp/foo/bar' subdirectory is created within ABSPATH (if that's possible) and the file is stored there. Both the filesystem path and URL path just append '/tmp/foo/bar', so you get 'ABSPATHtmp/foo/bar/2008/02/file.jpg' and 'http://example.com//tmp/foo/bar/2008/02/file.jpg'

An absolute upload_path should probably be rejected unless you also specify an upload_url_path, because there's no way to know what URL corresponds to an arbitrary filesystem path.

Attachments (1)

absolute-upload-dir-r6957.patch (2.5 KB) - added by tellyworth 10 years ago.

Download all attachments as: .zip

Change History (19)

#1 @tellyworth
10 years ago

Ah. There's another precondition required to reproduce: /tmp, /tmp/foo or /tmp/foo/bar is a symbolic link.

#2 @tellyworth
10 years ago

The attached patch fixes the main issue. It doesn't do anything about the case where upload_path is absolute and upload_url_path is empty - that probably needs a second opinion.

path_is_absolute() and path_join() might be useful elsewhere.

I also snuck in a minor addition: wp_upload_dir() now returns an extra element that just contains the subdir within the upload directory (if any). That might be useful for fixing the file attachment guid situation.

#3 @tellyworth
10 years ago

Unit tests are here:

http://svn.automattic.com/wordpress-tests/wp-testcase/test_uploads.php

test_upload_dir_absolute() covers the bug.

#4 @nbachiyski
10 years ago

In path_join() you can use DIRECTORY_SEPARATOR instead of /.

#5 @ryan
10 years ago

(In [6984]) Upload path fixes from tellyworth. see #5953

#6 @ryan
10 years ago

  • Owner changed from anonymous to tellyworth

Leaving open in case we want to use DIRECTORY_SEPARATOR.

#7 @tellyworth
10 years ago

Initially I wrote that function with DIRECTORY_SEPARATOR, but that constant isn't used anywhere else in WP so I changed it for consistency. Also, using / instead of DIRECTORY_SEPARATOR leaves open the possibility of using path_join() on URL paths.

We could use the constant and perhaps create a second function, url_path_join(), that always uses a /. But I think for that to make sense we'd need to review and change all paths in WP to use DS instead of a hard coded slash.

#8 @nbachiyski
10 years ago

  • Resolution set to fixed
  • Status changed from new to closed

Actually I agree. / works as a separator in Windows and writing path_join or DIRECTORY_SEPERATOR all the time is very tiresome.

#9 @Otto42
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

path_join doesn't appear to work properly for some people.

Reference this support thread:
http://wordpress.org/support/topic/182297

#10 @DD32
9 years ago

path_join doesn't appear to work properly for some people.

Looking at the code, That'd probably be caused by path_is_absolute() => realpath(), I cant see how the regular expressions could be missing it.

#11 @santosj
9 years ago

  • Milestone changed from 2.5 to 2.9

#12 @Denis-de-Bernardy
9 years ago

  • Component changed from General to Upload

#13 @Denis-de-Bernardy
9 years ago

realpath() is buggy on some servers. we could try to use the shell:

echo $PWD

but that wouldn't make much of a difference -- if realpath() fails, exec() probably does as well.

#14 @nbachiyski
9 years ago

$PWD is different than realpath('.'). realpath resolves symlinks and relative references (. and ..). Historically it had caused a lot of problems in the UNIX world, because of strange/buggy implementations.

If realpath fails, we can use dirname to resolve at least . and .. and forget about the symlinks.

#15 @Denis-de-Bernardy
9 years ago

True. For what it's worth, though, I've found this quite useful in various shell scripts:

path=`cd $PWD && pwd`

I've yet to see it fail on a symb links.

#17 @ryan
8 years ago

  • Milestone changed from 2.9 to Future Release

#18 @johnbillion
4 years ago

  • Milestone Future Release deleted
  • Resolution set to fixed
  • Status changed from reopened to closed

The core issue was fixed in r6984 and we no longer have these upload path input fields so the issue is moot.

Note: See TracTickets for help on using tickets.