Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#46878 closed enhancement (fixed)

Site Health: Allow 'target' in 'a' tags in the debug data description

Reported by: kraftbj's profile kraftbj Owned by: azaozz's profile azaozz
Milestone: 5.2 Priority: normal
Severity: normal Version: 5.2
Component: Site Health Keywords: site-health has-patch
Focuses: Cc:


When extending the debug data to include custom information for a plugin, it would be handy to include a link offsite to the plugin's support page. In this setup, I would like to open in a new tab instead of taking someone off of their site without a clean way to return.

Suggest adding target as an acceptable attribute for the a tag when running the description through wp_kses.

Attachments (6)

46878.diff (490 bytes) - added by kraftbj 4 years ago.
46878-2.diff (510 bytes) - added by jeherve 4 years ago.
Add rel to list of authorized attributes
46878.2.diff (739 bytes) - added by mukesh27 4 years ago.
Added span, div and p with class attribute
46878.3.diff (571 bytes) - added by audrasjb 4 years ago.
Patch refresh, and add hreflang attribute and span element
46878.4.diff (1.6 KB) - added by audrasjb 4 years ago.
Adds span element to existing documentation for debug_information filter
46878.5.diff (2.1 KB) - added by azaozz 4 years ago.

Download all attachments as: .zip

Change History (23)

4 years ago

#1 @jeherve
4 years ago

Should we add rel in there too, so one can set rel="noopener noreferrer" for their links?

4 years ago

Add rel to list of authorized attributes

#2 @Clorith
4 years ago

If we do this, we also need to allow span and class for screen reader text informing of new tabs opening and such.

#3 @mukesh27
4 years ago

If span is allow then can we add div p ?

4 years ago

Added span, div and p with class attribute

#4 @Clorith
4 years ago

Div and p are excessive and block elements, so I say no, it's overcomplicating things and making uneven outputs I would say, if your debug information is this excessive then you may not be categorising it properly.

#5 @mukesh27
4 years ago

@Clorith thanks for your information so needs add span with class right? and remove div and p from attached patch

#6 @SergeyBiryukov
4 years ago

  • Keywords site-health added
  • Milestone changed from Awaiting Review to 5.2

4 years ago

Patch refresh, and add hreflang attribute and span element

#7 @audrasjb
4 years ago


In 46878.3.diff:

  • I refreshed the previous patch
  • I added hreflang attribute since I think it could be helpful to provide information about the language of any linked document when different from the website language
  • I added span element with class attribute

4 years ago

Adds span element to existing documentation for debug_information filter

#8 @Clorith
4 years ago

  • Keywords commit added

46878.4.diff looks good to me.

#9 @pento
4 years ago

  • Keywords dev-feedback added; commit removed

@Clorith: Can you provide some context for why it's being run through KSES? Anything that's able to hook into the debug_information filter will be able to print its own arbitrary HTML, so it doesn't seem like there'd be any security benefit.

If a plugin wants to add ridiculous HTML, I don't think there's much we can realistically do about that. Instead of adding it directly to the description, they'd just have to write a little bit of JS to alter the HTML in the browser, instead.

#10 @Clorith
4 years ago

That was the intention, yes, to limit the potential for crazy markup and such.

This was mostly to maintain the sanity of the copy-paste output, since excessive markup would risk making that much less usable.

#11 @pento
4 years ago

WP_Debug_Data::format() should probably just run wp_strip_all_tags() on everything to avoid that problem.

#12 @Clorith
4 years ago

True, we can do that, my concern is someone adds a set of li elements, and when stripped by the formater it ends up being one long string with no separation for example.

Sure, this would then be on whomever added the list items for not making the consideration, but I also like to make it harder to mess up if possible, for everyone's benefit.

#13 follow-up: @Clorith
4 years ago

  • Keywords needs-patch added; has-patch dev-feedback removed

I've been checking further on this, and after looking over the source I see some new elements have gone in that means this isn't as big of a concern any more after the introduction of the debug entry, so one can differentiate between the visual, and copied data.

Let's roll with what @pento suggested then as that solves the concerns I have been stuck with.

#14 in reply to: ↑ 13 @azaozz
4 years ago

Replying to Clorith:

Yep, I'm with pento on this as well. Generally WordPress lets plugins and themes do... anything they want in order to provide better user experience. Trying to restrict that would result in either the plugin using more "hacky" way to achieve the same result, or the user experience being not as good as it can be.

As most of the data for the copied info is generated separately, we can strip_tags() only in cases where we are reusing the "view" strings intended for the HTML. There is no point in restricting these strings when added by plugins.

Patch coming up.

4 years ago

#15 @azaozz
4 years ago

  • Keywords has-patch added; needs-patch removed

In 46878.5.diff:

  • Do not use kses to sanitize section descriptions.
  • Add a bit more docs explaining the expected format of the strings.

The section descriptions come directly from plugins, and are not used in the data for copying. There's no need to sanitize them as mentioned above.

The fields names and values are run through esc_html() and all of the data for copying is sanitized with esc_attr(). They do not support HTML tags. Don't think they need additional sanitization.

#16 @azaozz
4 years ago

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

In 45259:

Site Health: Allow some HTML (inline tags only) in the section descriptions. Add some more docs about expected formatting of the gathered data.

Props kraftbj, jeherve, mukesh27, audrasjb, azaozz.
Fixes #46878.

#17 @spacedmonkey
4 years ago

  • Component changed from Administration to Site Health
Note: See TracTickets for help on using tickets.