WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 6 months ago

Last modified 4 months ago

#46878 closed enhancement (fixed)

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

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

Description

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

Download all attachments as: .zip

Change History (23)

@kraftbj
6 months ago

#1 @jeherve
6 months ago

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

@jeherve
6 months ago

Add rel to list of authorized attributes

#2 @Clorith
6 months 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
6 months ago

If span is allow then can we add div p ?

@mukesh27
6 months ago

Added span, div and p with class attribute

#4 @Clorith
6 months 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
6 months ago

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

#6 @SergeyBiryukov
6 months ago

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

@audrasjb
6 months ago

Patch refresh, and add hreflang attribute and span element

#7 @audrasjb
6 months ago

Hi,

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

@audrasjb
6 months ago

Adds span element to existing documentation for debug_information filter

#8 @Clorith
6 months ago

  • Keywords commit added

46878.4.diff looks good to me.

#9 @pento
6 months 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
6 months 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
6 months ago

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

#12 @Clorith
6 months 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
6 months 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
6 months 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.

@azaozz
6 months ago

#15 @azaozz
6 months 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
6 months 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 months ago

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