Make WordPress Core

Opened 7 years ago

Last modified 7 years ago

#40070 new defect (bug)

Cannot remove theme with Javascript if folder name contains periods

Reported by: svanlooy's profile svanlooy Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.7.3
Component: Themes Keywords:
Focuses: javascript Cc:

Description (last modified by SergeyBiryukov)

Hi. This could be vaguely related to #37924 but I've started versioning my themes and producing zip files with version numbers in them, ie foo-2.1.6b.zip - I've noticed that I can't delete them using the admin interface. I press the delete button and the slug name is foo-216b - the periods have been stripped out and the delete fails. So for a theme directory of honw-0.0.1b It fails. The front end gives a message "Deletion failed: The requested theme does not exist". The JSON payload is as follows:

{
    "success": false,
    "data": {
        "delete": "theme",
        "slug": "honw-001b",
        "errorMessage": "The requested theme does not exist."
    }
}

Attachments (1)

40070.diff (500 bytes) - added by MattyRob 7 years ago.

Download all attachments as: .zip

Change History (13)

#1 @MattyRob
7 years ago

  • Focuses javascript added
  • Keywords needs-patch added
  • Summary changed from Cannot remove theme if it contains periods to Cannot remove theme with Javascript if folder name contains periods

@svanlooy

Hello and welcome to the WordPress trac. I've done some testing on your ticket and I think I've narrowed this to an issue in the JavaScript code.

If you copy the 'delete link' into a new browser window it does indeed delete the theme from the server, the link looks something like this:

http://localhost/dev/src/wp-admin/themes.php?action=delete&stylesheet=twentyeleven&_wpnonce=a892cea026

However, in normal operation the click on that link is intercepted and precessed by JavaScript and it seems it is that bit that's broken.

@MattyRob
7 years ago

#2 @MattyRob
7 years ago

  • Keywords has-patch needs-testing reporter-feedback added; needs-patch removed

Patch attached for this, but needs some testing.

#3 @SergeyBiryukov
7 years ago

  • Component changed from Administration to Themes
  • Description modified (diff)
  • Keywords reporter-feedback removed
  • Milestone changed from Awaiting Review to 4.8

#4 @subrataemfluence
7 years ago

@MattyRob,

I tested by modifying regex pattern in function wp_ajax_delete_theme() with your suggestion ([^A-z0-9._\-]/) but it did not work. The original error message ("Deletion failed: The requested theme does not exist") disappeared but the following new message is now coming up:

"Deletion failed: Could not fully remove the theme theme.period." and as expected the theme was neither removed from admin UI nor physically from the disk.

$theme_dir variable however outputs the correct path.

#5 @subrataemfluence
7 years ago

Further investigation tells me that a theme folder (may be a plugin folder as well, but I did not test it) by default gets readonly status for some reason if it has a period in its name. To test status of a theme folder I used the following code snippet in delete_theme function of theme.php.

$themes_dir = trailingslashit( $themes_dir );
$theme_dir = trailingslashit( $themes_dir . $stylesheet );

// Snippet to check readonly status of a theme folder...

if(wp_is_writable($theme_dir)) {
    return new WP_Error("writable_status", sprintf(__('%s is writable'), $theme_dir));
} else {
    return new WP_Error("writable_status", sprintf(__('%s is readonly'), $theme_dir));
}

When I tried to delete a theme (ex. theme.period or customizr-child) the above snippet always enters the else part and outputs /var/www/html/tourplanner/wp-content/themes/customizr-child/ is readonly whereas when I tried to delete a theme like 'noperiod' I get a message like /var/www/html/tourplanner/wp-content/themes/period/ is writable.

#6 @MattyRob
7 years ago

@subrataemfluence

I'm not able to reproduce your issue. Uploading a zipped theme with a period in the directory name creates a fold with 755 permissions that is deleted using my patch above.

How are you uploading the theme?

#7 @subrataemfluence
7 years ago

I uploaded my mentioned two themes (zipped) from /wp-admin/theme-install.php and got the issue.

However... after getting your message I re-checked and saw that the permission for folder with a period is strangely becoming readonly while not for a theme without a period. Not sure why!!

I restarted my PC and now everything works just fine!! So I would say my test is passed as far as the patch is concerned. May be I now need to inspect what went wrong with my PC settings :(

I apologize for the confusion.

#8 @MattyRob
7 years ago

  • Keywords needs-testing removed

@subrataemfluence

No need to apologise - thanks for taking the time to test my patch.

#9 follow-up: @obenland
7 years ago

  • Keywords has-patch removed
  • Milestone changed from 4.8 to Future Release

Unfortunately this is not limited to ., the installer doesn't really strip out any non-word characters.

@dd32 How would you suggest we sanitize these slugs? sanitize_textfield()?

#10 in reply to: ↑ 9 @dd32
7 years ago

Replying to obenland:

Unfortunately this is not limited to ., the installer doesn't really strip out any non-word characters.

@dd32 How would you suggest we sanitize these slugs? sanitize_textfield()?

I'm not sure you should convert it to a slug at all - slugification isn't designed to be used on arbitrary data like this, and causes this exact scenario.

I'd suggest it should return the JS-escaped folder name instead, and should be handled appropriately on the client side - that may mean that items need an extra attribute such as this: class="plugin-{$slug_attempt_of_folder}" data-folder="{$escaped_foldername}" for filter/reference purposes.

Last edited 7 years ago by dd32 (previous) (diff)

#11 follow-up: @obenland
7 years ago

We already send the correct identifier to admin-ajax, trying to sanitize that is what causes the trip up. Do you have any recommendation there?

#12 in reply to: ↑ 11 @dd32
7 years ago

Replying to obenland:

We already send the correct identifier to admin-ajax, trying to sanitize that is what causes the trip up. Do you have any recommendation there?

Probably shouldn't be sanitizing it at all in that case then, merely checking that the provided value is recognised valid value for the request, ie. in_array( $value_provided, wp_list_pluck( get_themes(), 'template' ), true ).

If you did want to sanitize it first - you'd need to allow every character that can appear in a directory name, excluding ../ but including / and ...

Note: See TracTickets for help on using tickets.