is_writable function fails on Windows systems causing spurious http_request_failed error

[Otto42]

In class-http.php, in request(), there is a double check to be sure a file is writable using this code:

$r['filename'] = get_temp_dir() . basename( $url );

...

if ( ! is_writable( dirname( $r['filename'] ) ) )
	return new WP_Error( 'http_request_failed', __( 'Destination directory for file streaming does not exist or is not writable.' ) );

Problem is that is_writable can return false incorrectly on Windows systems, due to a PHP bug:
​https://bugs.php.net/bug.php?id=30931

Now, get_temp_dir() already performs an is_writable check on the resulting temp directories, and it uses a win_is_writable() call to work around this problem on Windows systems.

Thus, this secondary check is seemingly unnecessary anyway, anything returned by get_temp_dir() really should be writable.

So either the writable check in class-http.php needs to be made Windows compatible, or removed entirely.

A few reports of the problem on support forums:

• ​http://wordpress.org/support/topic/plugin-upgrades-fail-after-update-to-wp-35?replies=6
• ​http://wordpress.org/support/topic/plugin-update-not-writeable?replies=14
• ​http://wordpress.org/support/topic/download-failed-destination-directory-for-file-streaming-does-not-exist?replies=2

This problem may have been exacerbated by #20778.

The is_writable() check here is redundant when no path is passed to WP_HTTP to save the file to, in that case, it uses get_temp_dir(). In the case where a path is specifically passed, we still need to do a is_writable() check on it.

Perhaps we should rename win_is_writable() to wp_is_writable(), and short-circuit it with is_writable() when the server isn't windows, and then just use that everywhere?

It would reduce the logic elsewhere too.

I've just had someone email me the test debug from this thread: ​http://wordpress.org/support/topic/35-on-iis7-unable-to-update-plug-ins-and-themes?replies=5

As expected, the results in this case are:

get_temp_dir: C:\Windows\TEMP/  is_dir: true  win_writable: true   writable: false
WP_CONTENT_DIR: G:\....\wwwroot/wp-content  is_dir: true  win_writable: true   writable: true

An unfortunate side effect of this, and I'm rather annoyed that I didn't catch it in #20778, is that this will block people on affected systems from being able to upgrade to 3.5.1, or being able to install a plugin from the admin to fix it.

A temporary work around for those affected, would be to add this to their wp-config.php file, which will revert it to 3.4's behaviour (most likely) and get them unstuck..

define( 'WP_TEMP_DIR', ABSPATH . 'wp-content/' );

22900.diff​ is the minimal changes I'd like to make for 3.5.1 here, I haven't yet tested it on an affected install (my IIS7 install doesn't trigger the issue yet)

for 3.6, I'd like to extend that to us using wp_is_writable() elsewhere where we use is_writable(). @nacin?

Avoid error condition on IIS7 via Hotfix plugin

In addition to the above hotfix.22900.diff​ updates hotfix to restore 3.4 behaviour for WP_HTTP.

The Hotfix plugin will have to be installed before 3.5 is upgraded to, or manually via FTP/directly installing the plugin.

(Once again, untested on an affected environment, but a passes on a simulated environment that previously failed)

In hotfix.22900.diff​, pre_http_request is called as an action, but should be called as a filter.

Okay, Tested on a IIS7 server that experiences the problem - Now I remember why my permission set is so lax.. file permissions are a PITA!

attachment:22900.diff​​ confirmed working.

attachment:hotfix.22900.diff​​ fails - not sure why it worked in my testing, download_url() ignores the filename returned from wp_remote_get() and thus can't be changed on the filter naughty.. naughty..

attachment:22900.hotfix.diff​​ takes a different approach (just defining WP_TEMP_DIR on upgrade requests), and confirmed working.

pre_http_request is called as an action, but should be called as a filter.

They're just aliases of each other, but fixed up in the new patch.

22900.diff​ looks good.

Not sure if we should Hotfix this, or simply point people to wp-config.php. Unless they installed/updated Hotfix before updating to 3.5 (which is unlikely, unless they know about the order of operations, which is further unlikely), they'll need to go in through FTP anyway. I suppose uploading a file could be considered easier than changing a file, though.

Something less invasive for 3.5.1.

22900.2.diff​ is a bit less invasive, for 3.5.1.

22900.2.diff​ is a bit less invasive, for 3.5.1.

Looks good to me, Tested A-OK as well. Logic seems correct.

In 23250:

HTTP API: Use win_is_writable() on Windows when checking the directory to which we are streaming.

props dd32.
see #22900.
for the 3.5 branch only.

Looking at 22900.diff​ for 3.6:

• is_writable() should have error suppression, so we do not need to error-suppress when calling wp_is_writable().

• At first, it looked like win_is_writable() has a requirement that a directory must be checked with a trailing slash, which would make wp_is_writable() an imperfect drop-in replacement for is_writable(). But it turns out thanks to the is_dir() call, the comment "NOTE: use a trailing slash for folders!!!" is not accurate. We should clean this up and make sure the docs for wp_is_writable() and win_is_writable() are solid.

Putting this ticket back on report:4 so we may include the Hotfix plugin.

In 23254:

Clarify the Documentation in win_is_writable() and move an inline comment to the Docblock, reduces confusion about what the function actually does. See #22900

In 23255:

HTTP API: Introduce wp_is_writable() to wrap win_is_writable() and is_writable() to work around PHP Windows ACL issues. See #22900 for trunk

In 23256:

Correct an incorrect PHPDoc s/@see/@link/ introduced in r23254. See #22900

Does this affect only IIS on Windows, or any server on Windows?

Does this affect only IIS on Windows, or any server on Windows?

I'm going to go with: It affects PHP when running on windows in locked down user accounts, IIS specifically respects windows permissions to the dot, in addition to that, it has it's own set of permissions on top of the filesystem permissions, and runs PHP within a special user account.

Apache + windows on the other hand, is often configured differently, often running in a user account which has read access to a lot more than it needs to, since it's often configured as a regular user on the system.

Since it's a PHP bug of not respecting permissions (and instead, relying upon the read only flags) it will apply to any account which has permission limitations applied to it, which can't be expressed in that boolean read only flag.