WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 5 months ago

#47210 new enhancement

Allow html on site health titles and description

Reported by: juliobox Owned by:
Milestone: Awaiting Review Priority: normal
Severity: minor Version: 5.2
Component: Site Health Keywords: needs-patch dev-feedback site-health 2nd-opinion
Focuses: Cc:
PR Number:

Description

Hello there,

In /wp-admin/site-health-info.php#L115 we have this:

<?php echo esc_html( $details['label'] ); ?>

So we don't allow HTML content ? why!?
I propose the usage of wp_kses_* to allow clean html content.

Also line#141 we have this:

printf( '<p>%s</p>', $details['description'] );

We clearly allow any html, so I propose to sanitize using wp_kses_* too, we don't want embed/iframe/script here right?

Thank you for your feedback.

Change History (6)

#1 @audrasjb
6 months ago

  • Keywords site-health added

Related: #46878

#2 @SergeyBiryukov
6 months ago

  • Component changed from Security to Administration

#3 @azaozz
6 months ago

  • Keywords 2nd-opinion added

Couple of things:

  • KSES is not intended (not a good way) to sanitize things on displaying. It is a heavy/slow filtering of HTML tags and attributes best suited for saving to the DB.
  • What are the "user cases" for allowing HTML in item titles and collected data? I.e. how would that improve the user experience? :)

As far as I see it may be helpful to add a link to the "explanations" of some of the results, however keep in mind these will have to be translated and the link would (ideally) point to a "get more info" web page in the same language.

Seems displaying only "basic" info there and leaving it to the user to decide if they want to know more, and providing them with enough to "get them started" is a pretty good UX/workflow. There is a lot of (new/unfamiliar) info on that screen, we don't want to overwhelm them right from the start :)

Also, no HTML tags should be in the info that is for copying.

#4 @juliobox
6 months ago

Hey @azaozz !
Thanks for your interest and point of view :bow:, to answer to "how would that improve the user experience? :)"
Well, as a user I would rather prefer to have a quick view of all this stuff without having to (1) click on each to get more info, and (2) click on a link get … more info!
I agree to stay basic, however, too basic means I'll force the user to open the scan to really know what is the real issue.
For my case, nothing will be translated since all is already done, I'm just placing my security scan in this page, with is design to receive this kind of info.
Some examples of what I actually have:
https://www.dropbox.com/s/2uglxr7yiis1e5l/Capture%20d%27%C3%A9cran%202019-05-11%2000.13.51.png?dl=0
And what I would like to have:
https://www.dropbox.com/s/0w4mrvryzpmlk37/Capture%20d%27%C3%A9cran%202019-05-11%2000.24.33.png?dl=0
Also a screenshot when open:
https://www.dropbox.com/s/dvm2gfnbfivm2z8/Capture%20d%27%C3%A9cran%202019-05-11%2000.25.30.png?dl=0
I explain what is this scan, then explain more deeply + 1 link to the plugin page related to this scan + 1 link to my external documentation on that scan.

edit: quick test with switching infos:
https://www.dropbox.com/s/5y7to6hzf0teiyh/Capture%20d%27%C3%A9cran%202019-05-11%2000.29.51.png?dl=0
Is this what you think is the best ux practice?

voilà !

Last edited 6 months ago by juliobox (previous) (diff)

#5 @palmiak
5 months ago

I fully understand @juliobox point of view. Using some html tags can improve the readability of the titles and his screenshots has shown it.

I see three ways to go:

  • we leave as it is
  • we remove the esc_html just as it's made in with $details['description']
  • we remove the esc_html and use strip_tags with a list of tags we can use (everything but not style or script tags. Also we can use it for $details['description'] to keep it the same as the title.

I would go with the third version - it's adds some sanitation, but also gives a chance to use extra tags.

#6 @desrosj
5 months ago

  • Component changed from Administration to Site Health

Moving Site Health tickets into their lovely new home, the Site Health component.

Note: See TracTickets for help on using tickets.