WordPress.org

Make WordPress Core

Opened 23 months ago

Closed 18 months ago

Last modified 16 months ago

#20778 closed enhancement (fixed)

get_temp_dir should use the system tmp directory as first choice

Reported by: simonwheatley Owned by: dd32
Milestone: 3.5 Priority: normal
Severity: normal Version: 2.5
Component: Filesystem API Keywords: has-patch commit
Focuses: Cc:

Description

I believe WordPress should use the system tmp directory as first preference temp directory in get_temp_dir if WP_TEMP_DIR, is not defined. It just seems… odd… to use wp-content in preference to the proper, managed, temp directory.

Some historical context on introducing get_temp_dir: #5984

Attachments (5)

207778-sw.diff (1.3 KB) - added by simonwheatley 23 months ago.
Switch sys_get_temp_dir and WP_CONTENT_DIR within get_temp_dir
20778.patch (1.7 KB) - added by kurtpayne 19 months ago.
20778.diff (1.9 KB) - added by dd32 19 months ago.
20778.warning.patch (743 bytes) - added by ocean90 19 months ago.
20778.2.patch (493 bytes) - added by SergeyBiryukov 18 months ago.

Download all attachments as: .zip

Change History (40)

simonwheatley23 months ago

Switch sys_get_temp_dir and WP_CONTENT_DIR within get_temp_dir

comment:1 scribu23 months ago

Agreed.

If WP_TEMP_DIR isn't defined, the first fallback should be sys_get_temp_dir(), not WP_CONTENT_DIR.

Last edited 19 months ago by scribu (previous) (diff)

comment:2 nacin23 months ago

I tracked down this history in IRC with simonwheatley. The original order was introduced in #5984 and made sense at the time, but in [14016] an is_writeable() check was added to the result of sys_get_temp_dir(), at which point the order stopped making sense.

It makes sense to first check sys_get_temp_dir() as long as we are still keeping the is_writeable().

comment:3 nacin23 months ago

  • Version changed from 3.4 to 2.5

comment:4 dd3222 months ago

Does it make sense to use WP_CONTENT_DIR in preference to upload_tmp_dir here? I'd have thought that WP_CONTENT_DIR should be relegated to the absolute last preference?

comment:5 bpetty19 months ago

  • Type changed from defect (bug) to enhancement

comment:6 nacin19 months ago

  • Milestone changed from Awaiting Review to 3.5

I agree, seems like WP_CONTENT_DIR should go last.

kurtpayne19 months ago

comment:7 kurtpayne19 months ago

  • Cc kurtpayne added

Patch updated with:

  • Put WP_CONTENT_DIR last
  • Use win_is_writable when host system is windows
  • Don't save sys_get_temp_dir to static $temp var until it passes the is_writable check

comment:8 nacin19 months ago

Is win_is_writable() needed anymore? Seems like the ACL issues have been fixed since 5.0.x.

comment:9 kurtpayne19 months ago

The comments on this bug don't inspire confidence.

Official view on this problem is "wont fix", since we lack the time and inclination to mess with NT security API (it's an ugly beast), but would accept a patch that implements it.

comment:10 nacin19 months ago

  • Owner set to dd32
  • Status changed from new to assigned

comment:11 follow-up: dd3219 months ago

  • Status changed from assigned to accepted

I completely agree with switching the priorities here, however, the latest patch has a small logic error in the case of win_is_writable().

If win_is_writable() is designed to work around cases where is_writable() returns true when it can't actually write to a directory, the current logic goes as follows:
If ( ( is windows AND windows is writable ) OR is writable )
On affected windows hosts that would boil down to If ( ( 1 AND 0 ) OR 1 )

I'm not sure I understand the want to avoid writing to the $temp variable, as if a path that can't be written to is stored in it, It'll be overwritten by the next location test anyway, and failing all tests, overwritten with /tmp/. Micro-optimizations definitely isn't needed here, so that just seems to increase the complexity of the function.

I haven't tested on a Windows environment with ACL's in place yet, but attached is my take on the patch, Anyone see anything wrong with this approach?

dd3219 months ago

comment:12 in reply to: ↑ 11 simonwheatley19 months ago

Replying to dd32:

Anyone see anything wrong with this approach?

The logic in the patch makes sense to me.

comment:13 kurtpayne19 months ago

Looks good to me.

comment:14 nacin19 months ago

  • Keywords commit added; dev-feedback removed

comment:15 knutsp19 months ago

  • Cc knut@… added

+1
wp-content easily becomes polluted by uploaded zip files from an upload that didn't complete (zip file not extracted and installed). They have to be deleted manually. This should not happen.

comment:16 follow-up: dd3219 months ago

+1 wp-content easily becomes polluted by uploaded zip files from an upload that didn't complete (zip file not extracted and installed).

That sounds like a separate bug that should be reported if you can reliably duplicate it, the uploaded files should always be cleaned up (Only case I can think of right now would be a fatal or similar during the installation process).

Thanks for the sanity checks, I'll commit this later on

comment:17 in reply to: ↑ 16 knutsp19 months ago

Replying to dd32:

That sounds like a separate bug that should be reported if you can reliably duplicate it, the uploaded files should always be cleaned up (Only case I can think of right now would be a fatal or similar during the installation process).

I think it happens when there is a request timeout. The file, or part of the file, is uploaded, but then the process aborts. If this file is just left in a temp dir, then there is no fixable WordPress bug involved.

comment:18 dd3219 months ago

Tested OK on a Windows 7 box..

However, is_writable() works as expected for me on windows 7 with PHP 5.4.4, Denying write causes is_writable to return false, Allowing Write but not Delete causes both is_writable() and is_win_writable() to return true (although the latter makes a PHP Warning (Unable to delete the test file)). I'd be interested in finding the configurations which cause it to fail..

win_is_writable() will also need to move from wp-admin/includes/misc.php to wp-includes/functions.php, although core doesn't use get_temp_dir() outside of the admin, if it's available to plugins so it should work.

comment:19 dd3219 months ago

In [22008]:

Rearrange the order that we check for temporary directories in get_temp_dir(). This change causes us to use System temporary directories in preference to WP_CONTENT_DIR, for better windows compatibility, we use win_is_writable() as well. Props simonwheatley and kurtpayne for initial patches, See #20778

comment:20 dd3219 months ago

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

In [22009]:

Move win_is_writable() from wp-admin/includes to wp-includes so that it's always available for get_temp_dir(). Fixes #20778

comment:21 markoheijnen19 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

It seems it is broken on Mac OS since a lot of unit tests are broken now. Somehow /private/ got added.

comment:22 DH-Shredder19 months ago

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

Looks like this is only happening because the test in question is realpath()ing the result.
Working with kurtpayne to fix the test.

comment:23 follow-up: dd3219 months ago

Looks like this is only happening because the test in question is realpath()ing the result.

That sounds like something that someone would do to work around symlinks or something.

I'll also note this here, I am somewhat unconvinced that we *actually* need to use win_is_writable() at all in core, however, I'm unsure of the latest PHP version which is affected by the bugs which require it's use (I would not be surprised if PHP 5.2 is affected, while 5.3/5.4 are not).

Since this change means that a system directory outside of the normal web directories may be used, I thought it was the safest bet to use win_is_writable() to prevent any edge cases. A new ticket will need to circle back on it, but I'd really like to define the conditions which make it needed.

comment:24 in reply to: ↑ 23 kurtpayne19 months ago

Replying to dd32:

Looks like this is only happening because the test in question is realpath()ing the result.

The code in the #6821 patch is using realpath(). The test wasn't. On OS X, the /tmp folder is linked to /private/tmp and it was causing a mismatch on the assertion. The test was updated to call realpath() on both the $acutal an $expected before the assertion.

comment:25 ocean9019 months ago

  • Keywords commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Not sure if we want to fix it, but through the change a warning appears on updates.

Warning: is_dir() [function.is-dir]: open_basedir restriction in effect.
File(/tmp) is not within the allowed path(s): (
/var/www/web39/html/:
/var/www/web39/phptmp/:
/var/www/web39/files/:
/var/www/web39/atd/
) in /var/www/web39/html/wp-includes/functions.php on line 1394

See #12866 for background. Patch uses @is_dir.

ocean9019 months ago

comment:26 dd3219 months ago

  • Keywords commit added

Yup, we should fix warnings like that from safemode. It looks like that should be the only directory which needs the @is_dir() check.

comment:27 dd3219 months ago

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

In [22082]:

Silence a PHP Warning in Safe Mode from get_temp_dir(). Props ocean90. Fixes #20778

comment:28 dd3218 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This could have caused http://wordpress.org/support/topic/unable-to-update-2?replies=1

Looking at the report, 'C:\WINDOWS\TEMP\/wordpress-latest.tmp' contains a \/ as a path delimiter, that condition seems to be causing the problem here.

SergeyBiryukov18 months ago

comment:29 nacin18 months ago

  • Status changed from reopened to assigned

comment:30 SergeyBiryukov18 months ago

Could not reproduce the "Missing archive file" error from the forum thread on Windows XP.

I could, however, reproduce the wrong path delimiter. 20778.2.patch fixes that.

comment:31 dd3218 months ago

I've seen windows fail with a double slash in a path , or \/ before, but I think it's limited to a certain filesystem or ACL set.

In this case, The fact that trailingslashit() and untrailingslashit() don't support windows paths seems like a good new ticket, but for now, that patch looks good to me. I don't have a windows system to test on right now, but will follow up later

comment:33 dd3218 months ago

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

In 22331:

Prevent an issue on windows systems which can cause the temporary directory from get_temp_dir() from returning a path containing \/. Props to SergeyBiryukov for the initial patch. Fixes #20778

comment:34 dd3218 months ago

I really don't like that commit, but until #22267 is patched to fix untrailingslashit(), I'd prefer to avoid the potential issues which this could cause.

comment:35 Otto4216 months ago

Possibly related:
#22900

Note: See TracTickets for help on using tickets.