Shiny plugin updates fails if slug has special characters

[khag7]

The shiny plugin updates pass around the plugin slug via ajax. Here is how the slug is used: The esc_attr'd slug gets sent to the browser when the plugins page loads. The user tries to update the plugin and the slug (maybe with special characters, because esc_attr allows some) is sent to the server via ajax. wp_ajax_update_plugin will pass it through sanitize_key which strips out special characters. When the slug gets passed back to the user's browser, the javascript fails to find the right element to post the update message to because the slug has changed.

The problem is, sometimes the slug is santized with esc_attr, sometimes with sanitize_html_class, sometimes with sanitize_key. By standardizing on one method of escaping/sanitizing the slug anywhere its used for shiny updates, failures due to slug mismatches will stop.

I think it is necessary to continue to use sanitize_key because the incoming ajax parameter must be sanitized in the strictest way. The easy solution is to use sanitize_key instead of esc_attr and sanitize_html_class in the HTML. It will look odd to have sanitize_key used for escaping HTML attributes, so a comment is recommended to explain why it is being used.

You may be wondering how a plugin slug would have special characters. Plugins which are not part of the wordpress plugin repo sometimes make use of the wordpress plugin update mechanism. The results of the api call to the wordpress plugin repo are stored in a site transient, and many 3rd party plugins inject their data into that transient. Since the standardization of plugin slugs is currently only really enforced on wordpress's plugin repo, any 3rd party plugins can have slugs with special characters. Those plugins can still be updated via a separate page load, but when done via ajax the update fails and the user experience is poor. Users don't blame the 3rd party plugin, they blame core wordpress.

This should fix shiny plugin updates for all woothemes plugins/extensions, which has been broken for some time because woothemes uses special characters in its plugin slugs.

Ticket #32465 was along the same lines, but they seemed to have trouble tracking down the right solution. Or maybe this is not the same problem.

[khag7]

Patch to use sanitize_key in place of sanitize_html_class and esc_attr for plugin slugs as values of data-slug attributes

[swissspidy]

Can you give us an example of some plugin slugs with special characters in it?

[khag7]

The slugs provided by the WP plugin repo are all sanitized by the repo API before they are returned.

WooThemes premium plugins have an API for updates which returns instead of a sanitized plug, a string that is the filename of the plugin.

Example: WooCommerce Smart Coupons gets returned from the WooThemes API as woocommerce-smart-coupons/woocommerce-smart-coupons.php

There are about 350 other WooThemes extensions that might all behave the same way. I don't own all of them but I assume their API works consistently in that manner.

On one hand, I think the burden should be on 3rd parties to make their plugin update API service interact with WordPress in a way that is functional. On the other hand, updates have always worked even when the slug contains special characters, its just the shiny updates that are broken. If we want to disallow special characters in plugin slugs then we should do so consistently: we should prohibit regular updates from working as well.

If you're looking to test and need a copy of a WooThemes plugin that does this, I can provide. Alternatively, just for testing you could write a filter on site_transient_update_plugins that adds special characters to the plugin slugs stored there. The slug doesn't have to be "correct" for updates to work, its just a string that is used as an HTML data attribute.

[adamsilverstein]

@khag7 - Thanks for your bug report. Your detailed explanation pinpoints the underlying issue of relying on the plugin slug as the identifying mechanism for the plugin being updated, especially considering the inconsistent escaping being applied, as you point out.

While your patch approach looks reasonable, I wanted to point out that we've already resolved this issue as work on shiny updates continues: a group of us are working on a feature plugin we hope to merge into core in the 4.5 release cycle. The goals for a merge include those mentioned in this ticket: #31531. Development on this plugin is happening in github, and we welcome contributions! ​https://github.com/obenland/shiny-updates.

Can you please give the current plugin a try to see if it resolves the issue you are having? it would be good to verify the new approach fixes the underlying issue.

In the new version of the plugin, we are allowing bulk updates and discovered that users can have multiple plugins with the *same slug* which also breaks the ajax return/selection mechanism. For this reason we have switched to using the plugin field (not the name, the file path, usually plugin/plugin.php). It would be worth exploring whether this could break in similar situations for example where emoji or special characters are used in a plugin file name. is that even possible?

Thanks!

[khag7]

@adamsilverstein I'll look into this ASAP and test against some oddball plugin slugs.

I think moving away from using the slug is a good idea. The slug isn't even generated by the WP core, its returned from an API. And if you have 3rd party plugins from different update providers (Envato, WooThemes, WordPress) there is inconsistency between slugs and collisions are possible, as you described. The file path is seems ideal, in my opinion. That is what WooThemes is using. But the / and . in the path are considered special characters which brings forward the issue at hand.

[SergeyBiryukov]

It will look odd to have sanitize_key used for escaping HTML attributes, so a comment is recommended to explain why it is being used.

I'd suggest keeping the esc_attr() for clarity: esc_attr( sanitize_key( $plugin['slug'] ) ).

[khag7]

@adamsilverstein I installed the plugin you shared. Updates are failing if the value of the data-slug attribute contains special characters. This is because the shiny-updates.js as well as the default wordpress admin script updates.js are both making use of the value in the data-slug attribute as a jQuery selector string without escaping it first. jQuery selector strings with special characters have to prefix the special char with a \\ before it can be used in the string. If shiny-updates.js and the core admin script updates.js were changed to let the jQuery handle special characters, it might work.

That would involve doing something like slug = slug.replace(/([^A-Za-z0-9_\-])/g,"\\$1" ) throughout the js anywhere a slug is used in a jQuery selector string.

At that point, the jQuery wouldn't throw a syntax error.

Even if that does work, not sure that it will make a difference in the way that the server handles the ajax request and returns a response. I haven't looked at the code of your plugin in depth. But even if it can handle it, I think the patch I submitted which removes the special characters before they ever show up in the HTML is a better option than having to fix all the javascript to handle the special chars.

[adamsilverstein]

Replying to khag7:

That would involve doing something like slug = slug.replace(/([^A-Za-z0-9_\-])/g,"\\$1" ) throughout the js anywhere a slug is used in a jQuery selector string.

This is a reasonable suggestion if we are going to continue using slug. we have switched to using the plugin name in some cases already, however i see we are still using slug so this is still an issue. lets keep working on this in the plugin.

Does your existing patch resolve the issue in core? can you please refresh with @SergeyBiryukov 's feedback? also, can you offer some steps to reproduce? how can i create a plugin with a slug using special characters?

Thanks.

[khag7]

The patch I gave does solve the problem in core. I still see a better long term solution to be getting away from using plugin name or slug as identifiers for the reasons we discussed. But for now the patch I submitted does fix the more immediate issue of shiny updates failing for some plugins.

I will resubmit a patch using esc_attr( sanitize_key ( $slug ) ) although I think it's redundant to have both. Or maybe I'm confused about what each of those does.

I threw together a ​little plugin that might help you reproduce this issue. It adds itself to the 'update_plugins' transient (that's where available plugin updates are stored) and it uses its filename (including special chars) as the slug. It also always shows as having an update available, so its ideal for testing this kind of thing.

[revaxarts]

I just came through this bug and found already this open ticket. I've started a question on Stackoverflow if anyone is interested
​http://wordpress.stackexchange.com/questions/211803/changing-plugin-slug-with-update

[khag7]

I'm most of the way through reworking the updates.js to not rely on slugs at all. At that point, we can actually ignore the patch I already submitted and I will submit a new one.

One thing I'm waiting on... there is a patch on ticket #18974 which will add the data-plugin attribute to solve a different issue. We can make use of that same attribute in updates.js to have better jQuery selectors. Do I wait for the patch on #18974 to be committed or do I use that patch as part of this patch? I think it would cause a conflict later when someone tries to commit and the changes were already made.

[adamsilverstein]

@khag7

Thanks for pointing out #18974. We need to look at the HTML of this page carefully and clean it up.

Ultimately much of this will be replaced by the code we are developing in ​https://github.com/obenland/shiny-updates - it would be great to have your contributions there. We already switched from embedding all the plugin data in the html, instead passing it to the JS directly, I think we only keep the unique plugin name, and use that data element for the lookup.

No telling exactly if/when the plugin will get merged into core, so in the mean time I think its worth getting your patches into core to fix the issue for everyone running the current version.

[khag7]

@adamsilverstein
Thanks for reminding me about that github repo. I do intend on contributing there, I think shiny updates can be improved. Like you mentioned, in the meantime I'm hopeful that the patches in this ticket and #18974 can go through so that those issues are fixed. If your feature repo gets merged into core thats great, but if not at least we fixed the small issues that exist already.

[adamsilverstein]

Replying to khag7:

My thoughts exactly! Thanks for your contribution.

I see you are in the shineyupdates slack channel already :) We will likely start regular meetings there after the new year.

[obenland]

In 37714:

Update/Install: Shiny Updates v2.

Gone are the days of isolation and feelings of "meh", brought on by The Bleak Screen of Sadness. For a shiny knight has arrived to usher our plugins and themes along their arduous journey of installation, updates, and the inevitable fate of ultimate deletion.

Props swissspidy, adamsilverstein, mapk, afragen, ocean90, ryelle, j-falk, michael-arestad, melchoyce, DrewAPicture, AdamSoucie, ethitter, pento, dd32, kraftbj, Ipstenu, jorbin, afercia, stephdau, paulwilde, jipmoors, khag7, svovaf, jipmoors, obenland.
Fixes #22029, #25828, #31002, #31529, #31530, #31773, #33637, #35032.