WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

#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 3 years ago.
hotfix.22900.diff (2.7 KB) - added by dd32 3 years ago.
Avoid error condition on IIS7 via Hotfix plugin
22900.hotfix.diff (2.3 KB) - added by dd32 3 years ago.
22900.2.diff (721 bytes) - added by nacin 2 years ago.
Something less invasive for 3.5.1.

Download all attachments as: .zip

Change History (27)

comment:1 @nacin3 years ago

  • Milestone changed from Awaiting Review to 3.5.1

comment:2 @ocean903 years ago

  • Description modified (diff)

comment:3 @dd323 years 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 @dd323 years 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 @knutsp3 years ago

  • Cc knut@… added

comment:6 @phill_brown3 years ago

  • Cc phill@… added

@dd323 years ago

comment:7 @dd323 years 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?

@dd323 years ago

Avoid error condition on IIS7 via Hotfix plugin

comment:8 @dd323 years 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 3 years ago by dd32 (previous) (diff)

comment:9 @kurtpayne3 years ago

  • Cc kurtpayne added

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

@dd323 years ago

comment:10 @dd323 years 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 @nacin3 years 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 @nacin3 years ago

  • Keywords has-patch commit added

@nacin2 years ago

Something less invasive for 3.5.1.

comment:13 @nacin2 years ago

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

comment:14 @dd322 years 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 @nacin2 years 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 @nacin2 years 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 @nacin2 years ago

  • Keywords fixed-minor removed

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

comment:18 @dd322 years 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 @dd322 years 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 @dd322 years ago

In 23256:

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

comment:21 @nacin2 years ago

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

comment:22 @dd322 years 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 @nacin2 years ago

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