WordPress.org

Make WordPress Core

#22900 closed defect (bug) (fixed)

is_writable function fails on Windows systems causing spurious http_request_failed error

Reported by: Otto42 Owned by:
Milestone: 3.5.1 Priority: normal
Severity: normal Version: 3.5
Component: HTTP API Keywords: has-patch commit
Focuses: Cc:

Description (last modified by ocean90)

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:

This problem may have been exacerbated by #20778.

Attachments (4)

22900.diff (2.2 KB) - added by dd32 16 months ago.
hotfix.22900.diff (2.7 KB) - added by dd32 16 months ago.
Avoid error condition on IIS7 via Hotfix plugin
22900.hotfix.diff (2.3 KB) - added by dd32 16 months ago.
22900.2.diff (721 bytes) - added by nacin 16 months ago.
Something less invasive for 3.5.1.

Download all attachments as: .zip

Change History (27)

comment:1 nacin16 months ago

  • Milestone changed from Awaiting Review to 3.5.1

comment:2 ocean9016 months ago

  • Description modified (diff)

comment:3 dd3216 months ago

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.

comment:4 dd3216 months ago

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/' );

comment:5 knutsp16 months ago

  • Cc knut@… added

comment:6 phill_brown16 months ago

  • Cc phill@… added

dd3216 months ago

comment:7 dd3216 months ago

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?

dd3216 months ago

Avoid error condition on IIS7 via Hotfix plugin

comment:8 dd3216 months ago

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)

Last edited 16 months ago by dd32 (previous) (diff)

comment:9 kurtpayne16 months ago

  • Cc kurtpayne added

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

dd3216 months ago

comment:10 dd3216 months ago

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.

comment:11 nacin16 months ago

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.

comment:12 nacin16 months ago

  • Keywords has-patch commit added

nacin16 months ago

Something less invasive for 3.5.1.

comment:13 nacin16 months ago

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

comment:14 dd3216 months ago

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

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

comment:15 nacin16 months ago

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.

comment:16 nacin16 months ago

  • Keywords fixed-minor added

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.

comment:17 nacin16 months ago

  • Keywords fixed-minor removed

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

comment:18 dd3216 months ago

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

comment:19 dd3216 months ago

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

comment:20 dd3216 months ago

In 23256:

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

comment:21 nacin16 months ago

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

comment:22 dd3216 months ago

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.

comment:23 nacin15 months ago

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.