Make WordPress Core

Opened 4 months ago

Closed 3 months ago

Last modified 3 months ago

#61488 closed defect (bug) (fixed)

6.5.5 Makes Theme Files Inaccessible

Reported by: script2see's profile script2see Owned by: jorbin's profile jorbin
Milestone: 6.5.6 Priority: normal
Severity: normal Version: 6.5.5
Component: Filesystem API Keywords: has-patch has-unit-tests dev-reviewed commit
Focuses: Cc:

Description

I'm editing a site on the offline testing server, localWP, and, after the 6.5.5 update, I can't access the theme files under the Tools menu. When I try to access those files, I get a screen with the following message: Sorry, that file cannot be edited.

I checked wp-config.php and didn't see any rules that would disallow access to the theme files such as theme.json and style.css.

Attachments (1)

WordPress-error.jpg (11.8 KB) - added by script2see 4 months ago.
Screenshot of Error

Download all attachments as: .zip

Change History (33)

#1 @script2see
4 months ago

I'm using the theme, Twenty Twenty-Four.

@script2see
4 months ago

Screenshot of Error

#2 @jorbin
4 months ago

I've tested this on a live server and also downloaded and installed local Version 9.0.3+6684 and I'm not able to replicate this. Are there any further steps for reproduction available?

#3 @Presskopp
4 months ago

confirmed using 6.6-beta4-58555 and the Twenty Ten theme.

I also tried 2 commercial themes and had the same issue with one of them.

btw: I'm using the very same version of local as in the comment above.

Last edited 4 months ago by Presskopp (previous) (diff)

#4 @audrasjb
4 months ago

  • Component changed from General to Themes
  • Milestone changed from Awaiting Review to 6.6

Milestoning to 6.6 as the issue was reproduced on beta 4.
First, we need to clarify the exact context of the issue. Further reproduction steps are very welcome.

#5 follow-up: @peterwilsoncc
4 months ago

  • Component changed from Themes to Filesystem API

@script2see @Presskopp Are you able to help out by letting me know the following:

  • What operating systems are you using?
  • Are you seeing the same issue on the plugin editor: /wp-admin/plugin-editor.php?
  • Are you testing with a downloaded copy or WordPress or one checked out via SVN or Git?

I've attempted to reproduce this on a Linux Virtual Machine running on a macOS host without success.

As the error message you're seeing is generated in wp-admin/includes/file.php, I've set the component to the Filesystem API.

#6 @siliconforks
4 months ago

It looks like this is probably a Windows-only issue.

The problem is that, in 6.5.5, validate_file() calls wp_normalize_path( $file ) (it didn't in 6.5.4). But $allowed_files does not contain normalized paths. So in_array( $file, $allowed_files, true ) returns false.

For example, on my Windows installation of WordPress, $file is initially

O:\Apache24\htdocs\wordpress/wp-content/themes/twentyten/style.css

which becomes (after calling wp_normalize_path):

O:/Apache24/htdocs/wordpress/wp-content/themes/twentyten/style.css

But $allowed_files contains:

O:\Apache24\htdocs\wordpress/wp-content/themes/twentyten/style.css

#7 in reply to: ↑ 5 @Presskopp
4 months ago

Replying to peterwilsoncc:

  • What operating systems are you using?

Windows 11

  • Are you seeing the same issue on the plugin editor: /wp-admin/plugin-editor.php?

No

  • Are you testing with a downloaded copy or WordPress or one checked out via SVN or Git?

downloaded via beta tester plugin (update-core.php)

@siliconforks is on the right track. In my case the error message is generated in line 752 of file.php (case 3). Setting in_array( $file, $allowed_files, true ) to true manually is re-enabling the editor.

This ticket was mentioned in PR #6899 on WordPress/wordpress-develop by @jorbin.


4 months ago
#8

  • Keywords has-patch has-unit-tests added; needs-patch removed

Allowed files also needs to be normalized in order to ensure that valid paths are checked against each other

Trac ticket: https://core.trac.wordpress.org/ticket/61488

#9 @jorbin
4 months ago

I'm unable to test this since it seems to be only on Windows, but my above patch does include a unit test. I would love some testing from folks running WordPress on Windows hosts or if there is a host that is willing to donate a windows server for testing purposes, happy to test there.

In the meantime, for editing locally I would encourage folks to use a code editor. If you need to edit on a server, many also support editing via ssh or FTP.

#10 @Presskopp
4 months ago

I can confirm that the patch is resolving the issue. I did the change manually on my local windows machine.

#11 @locker17
4 months ago

Same issue with Astra theme on WordPress and localhost Wampserver.
How to apply your patch using Windows 11?

#12 @jorbin
4 months ago

#61496 was marked as a duplicate.

@swissspidy commented on PR #6899:


4 months ago
#13

We might be able to add an e2e test for this too. Even better if we run them on Windows…

This ticket was mentioned in Slack in #core by jorbin. View the logs.


4 months ago

@jorbin commented on PR #6899:


4 months ago
#15

The challenge with an e2e test is that they all run on linux docker containers. We would need to completely refactor the e2e action to get the value for this. Seems worthwhile but also seems like a larger scope than necessary to complete for this fix.

#16 @littler.chicken
4 months ago

Confirmed the reported issue (I'm running Local WP on Windows 11, Twenty Twenty Four theme). I've manually applied the patch and can now edit the theme files under Tools > Theme File Editor.

@hellofromTonya commented on PR #6899:


4 months ago
#17

There's a positive test report using a Windows machine on the Trac ticket that confirms:

  • Can reproduce the reported issue.
  • This patch resolves the issue ✅

@misulicus commented on PR #6899:


4 months ago
#18

Replicated the issue on MAMP locally (Windows).
WP: 6.6-beta4
PHP 8.1.0

PR works good and the File Editor is accessible again!

#19 @hellofromTonya
4 months ago

  • Keywords commit added

Patch: https://github.com/WordPress/wordpress-develop/pull/6899

Thank you @littlerchicken and @misulicus for testing on your Windows machines. Minus a minor nitpick, the patch LGTM for commit.

#20 @jorbin
4 months ago

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

In 58570:

Filesystem: Normalize allowed_files so comparison is apples to apples.

In [58470] a change was made to normalize the filename in validate_file, however this leads to instances where the list of files that are allowed aren't normalized such as in the theme editor. By normalizing the array, the comparison is apples to apples.

Fixes #61488.
Props jorbin, hellofromtonya, swissspidy, misulicus, script2see, Presskopp, audrasjb, peterwilsoncc, siliconforks, littler.chicken, paulkevan,

#21 @jorbin
4 months ago

  • Keywords dev-feedback added; commit removed
  • Milestone changed from 6.6 to 6.5.6
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for consideration to backport to the 6.5 branch.

#22 @hellofromTonya
4 months ago

  • Keywords dev-reviewed commit added; dev-feedback removed

[58570] LGTM for backporting to the 6.5 branch.

@euthelup commented on PR #6899:


4 months ago
#23

I can also confirm that on my Windows machine, on WordPress 6.5.5 I cannot edit theme files, but this branch fixes the issue.

#25 @siliconforks
4 months ago

#61509 was marked as a duplicate.

This ticket was mentioned in Slack in #core by joemcgill. View the logs.


4 months ago

#27 @mukesh27
4 months ago

#61547 was marked as a duplicate.

#28 @audrasjb
4 months ago

  • Milestone changed from 6.5.6 to 6.6.1

Moving to 6.6.1 as we're approaching 6.6 final release.

#29 @peterwilsoncc
4 months ago

@audrasjb [58570] was committed prior to 6.6 being forked so is in the branch already (see code reference).

Are you happy to milestone this for 6.6 or are you asking for it to be reverted and recommitted for 6.6.1?

#30 @audrasjb
4 months ago

  • Milestone changed from 6.6.1 to 6.5.6

My bad. Let's keep it in 6.5.6 then.
Thanks for the clarification.

#31 @jorbin
3 months ago

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

In 58808:

Filesystem: Normalize allowed_files so comparison is apples to apples.

In [58470] a change was made to normalize the filename in validate_file, however this leads to instances where the list of files that are allowed aren't normalized such as in the theme editor. By normalizing the array, the comparison is apples to apples.

Reviewed by hellofromTonya.
Merges 58570 to the 6.5 branch.

Fixes #61488.
Props jorbin, hellofromtonya, swissspidy, misulicus, script2see, Presskopp, audrasjb, peterwilsoncc, siliconforks, littler.chicken, paulkevan,

#32 @jorbin
3 months ago

As [58570] was approved for backport before 6.5 was no longer supported, I've backported this so it can be included if there is a courtesy release in the future.

Note: See TracTickets for help on using tickets.