WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#35083 closed defect (bug) (duplicate)

Unusual plugin names can cause CSS to be unintentionally applied -- also duplicate element ID's are forbidden in HTMl

Reported by: khag7 Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Plugins Keywords: has-patch
Focuses: administration Cc:
PR Number:

Description

The list of installed plugins, /wp-admin/plugins.php is set up such that each <tr> in the table represents a single plugin. The value of the id attribute is simply the sanitized version of the plugin title. That means that a plugin with a specific name can cause some visual damage to the interface if the sanitized name of the plugin matches an id in the stylesheet.

Here's a fun example -- Create a plugin named "Plugin Information" (or just rename an existing plugin for the purposes of this example) and drop it in your plugins dir. Don't even need to activate it. Then go to the plugins list page and be amazed! Here's a php to help you out:

<?php
/* Plugin Name: Plugin Information */
?>

Some other fun plugin names to try: Contextual Help Back, WPAdminBar, Screen Meta, WPFooter, WPBody Content, Post Body Content, the list goes on.

Now, its unlikely that someone creates a plugin named that, but there's also no reason our admin interface should be coded in such a way that the name of a plugin is enough to cause this kind of ugliness. The point is, we should come up with a better way of giving those <tr> elements a value for their id attribute.

My first thought of a solution: we could change the value of id to be something like #plugin-akismet instead of just #akismet. That solves the problem, but since we're here lets talk about another issue...

What happens if a user has two plugins with the same title? We end up with more than one HTML element with the same exact id. That's a HTML no-no!

Before you say "Two plugins with the same name? That never happens!": I can think of a few clients of mine who have multiple old versions of the same plugin for backup reasons. Each of those <tr> elements have the same ID. Not the end of the world, but its not right. Our interface should be generating HTML with unique id's for elements, or no id at all.

So, long story short, using the plugin title to make the id (as we've been doing for probably very long) is not working. Slugs won't work either: slugs can be the same for multiple plugins, and non wp-repo plugins have no slug at all in cases where the 3rd party plugin developer doesn't specifically provide one.

One thing that is always unique is the plugin's file path, usually in the pattern of the-plugin/the-plugin.php. Unfortunately, the id attribute can't have / or . in it so we'd have to sanitize that string in a standard way.

Alternatively, we could ditch the id attribute entirely and use a data element, since they can have special characters. <tr data-plugin="some-plugin/the-plugin.php"> looks pretty nice. Is there any downside to losing the id attribute from all <tr>'s in the plugin list?

Attachments (2)

35083.patch (689 bytes) - added by khag7 4 years ago.
use data-plugin attribute instead of id attribute
35083-2.patch (834 bytes) - added by khag7 4 years ago.
ditch the id attribute in favor of the data-plugin attribute; now with esc_attr

Download all attachments as: .zip

Change History (7)

@khag7
4 years ago

use data-plugin attribute instead of id attribute

#1 @swissspidy
4 years ago

  • Keywords has-patch added

+1 for using the-plugin/the-plugin.php for the attribute, as this indeed is the only real thing making a plugin unique. Since there's already data-slug I see no harm in using data-plugin, as long as it doesn't break anything.

Besides that, what about using esc_attr() for all these <tr> attributes?

#2 @afercia
4 years ago

+1 :D

@khag7
4 years ago

ditch the id attribute in favor of the data-plugin attribute; now with esc_attr

#3 @jdgrimes
4 years ago

Related: #18974 (possibly a duplicate?)

#4 @SergeyBiryukov
4 years ago

  • Component changed from Administration to Plugins
  • Focuses administration added
  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #18974.

@khag7, could you add your patch to the other ticket? Thanks!

Last edited 4 years ago by SergeyBiryukov (previous) (diff)

#5 @khag7
4 years ago

@SergeyBiryukov will do. Thanks for finding that older ticket, I didn't find that when I searched, I guess I need better search-fu!

Note: See TracTickets for help on using tickets.