Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#48199 closed defect (bug) (fixed)

Check for whether the themes directory writable is incorrect when the current theme is symlinked into the theme directory

Reported by: pbiron's profile pbiron Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.5 Priority: normal
Severity: normal Version: 5.2
Component: Site Health Keywords: has-patch commit
Focuses: Cc:

Description

To test whether the theme directory is writable,WP_Debug_Data::debug_data(), L322 does:

$is_writable_template_directory = wp_is_writable( get_template_directory() . '/..' );

If the current theme exists outside of ABSPATH and is symlink'd into ABSPATH . 'wp-content/themes (which I do for all the bundled themes for all sites on my local dev machine) then what is actually being checked is whether that directory outside of ABSPATH is writable, not whether the theme directory is writable.

Changing that line to:

$is_writable_template_directory = wp_is_writable( dirname( get_template_directory() ) );

correctly tests whether the theme directory is writable.

Additionally, when running on a Windows machine, wp_is_writable() calls win_is_writable(), which creates and then unlinks a tmp file. Because of the directory traversal in the path, the unlink fails when the template directory is a symlink...which leaves the tmp file in the parent directory of the symlink'd theme resides outside of ABSPATH. Changing the argument to wp_is_writable() to use dirname() instead of directory traversal also fixes that problem.

Attachments (1)

48199.diff (1.2 KB) - added by pbiron 4 years ago.

Download all attachments as: .zip

Change History (13)

#1 @pbiron
5 years ago

  • Keywords 2nd-opinion added

Can someone check whether the use of directory traversal on a linux box when the current theme is symlink'd is actually checking the writability of the directory outside of ABSPATH as it is on a Windows machine?

#2 follow-up: @Clorith
4 years ago

  • Keywords needs-patch added; 2nd-opinion removed
  • Milestone changed from Awaiting Review to 5.5
  • Version set to 5.2

Good catch, we should probably simplify this even further by just using wp_is_writable( get_theme_root() ), are you up for writing a patch for it?

#3 in reply to: ↑ 2 @pbiron
4 years ago

Replying to Clorith:

Good catch, we should probably simplify this even further by just using wp_is_writable( get_theme_root() ), are you up for writing a patch for it?

sure, I'll work one up first part of next week.

This ticket was mentioned in Slack in #core-site-health by pbiron. View the logs.


4 years ago

@pbiron
4 years ago

#5 @pbiron
4 years ago

48199.diff corrects the immediate problem:

  • the check is done on a sub-directory within WP_CONTENT_DIR
  • the temp file is correctly deleted even when the theme's directory is symlink'd (on Windows)

#6 @pbiron
4 years ago

Related: #50018

#7 @pbiron
4 years ago

  • Keywords has-patch added; needs-patch removed

This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.


4 years ago

This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.


4 years ago

This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.


4 years ago

#11 @afragen
4 years ago

  • Keywords commit added

#12 @SergeyBiryukov
4 years ago

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

In 48125:

Site Health: Correct the check of whether the theme directory is writable when the current theme is symlinked into the theme directory.

This ensures that the check is done on a sub-directory within WP_CONTENT_DIR, rather than outside of ABSPATH.

Props pbiron, Clorith.
Fixes #48199.

Note: See TracTickets for help on using tickets.