WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 7 weeks ago

Last modified 3 days ago

#47580 closed defect (bug) (fixed)

Prevent output of invalid HTML in `_default_wp_die_handler()`

Reported by: jeremyfelt Owned by: audrasjb
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch has-dev-note
Focuses: accessibility Cc:
PR Number:

Description

The default handler for wp_die() wraps (by default) error messages with a paragraph tag.

For a large number of wp_die() calls in WordPress, a plain text string is passed and the HTML displayed is valid: <p>This is an error message.</p>.

Also for a large number of wp_die() calls in WordPress, an HTML string is passed and the HTML displayed is invalid: <p><h1>You need a higher level of permission.</h1><p>Sorry, you are not allowed to manage terms in this taxonomy.</p></p>

It seems that both use cases are valid and should be supported. wp_die() already has $args that are passed to the handlers. It seems like we could add key there for whether the handler should wrap in its own HTML or display the message as is.

Supporting a new argument would make it possible to have a slightly more complex error message while also maintaining the simple output of general error messages.

Related #47413, which addresses the issue when filling out a comment.

Attachments (2)

47580.diff (1.6 KB) - added by dinhtungdu 2 months ago.
Use div for wrapper instead of p
47580.2.diff (1.7 KB) - added by dinhtungdu 8 weeks ago.

Download all attachments as: .zip

Change History (22)

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


3 months ago

#2 @afercia
3 months ago

  • Component changed from General to Administration
  • Milestone changed from Awaiting Review to Future Release

Discussed during today's accessibility bug-scrub and agreed ensuring valid HTML output is always valuable. Moving to Future Release.

#3 in reply to: ↑ description @SergeyBiryukov
3 months ago

Replying to jeremyfelt:

wp_die() already has $args that are passed to the handlers. It seems like we could add key there for whether the handler should wrap in its own HTML or display the message as is.

A key in $args might be cleaner, but also seems a bit like an overkill. Maybe a simple strpos( $message, '<p>' ) check before adding the <p> tag would suffice here?

Another option suggested by @audrasjb is to use <div> instead of <p> to allow for nested elements.

#4 @afercia
2 months ago

One more example of invalid HTML:

https://core.trac.wordpress.org/browser/trunk/src/index.php?rev=45649&marks=33-55#L32

The passed string avoids to set the initial opening tag <p>, which is smart, but then forgets to not pass the last closing tag </p>. Actual result in the HTML: </p></p>.

#5 @SergeyBiryukov
2 months ago

  • Milestone changed from Future Release to 5.3

@dinhtungdu
2 months ago

Use div for wrapper instead of p

#6 @dinhtungdu
2 months ago

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

After considering checking for tags in the $message and replacing p by div, I think it's better to use div for the die message wrapper.

We can check if $message contains <p>, but it means that we need to check for other tags as well. h1 for example, can't be a child element of p. A full covered check can make the current function more complex.

On the other hand, replacing p by div doesn't increase the complexity of _default_wp_die_handler.

What I did:

This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.


2 months ago

#8 @audrasjb
2 months ago

  • Keywords needs-testing added
  • Owner set to audrasjb
  • Status changed from new to accepted

#9 @audrasjb
2 months ago

  • Keywords dev-feedback needs-testing removed

Good to go on my side. As said during the meeting, I like the <div> approach because it‘s more secure concerning backward compatibility :)

#10 @audrasjb
8 weeks ago

  • Keywords needs-dev-note added

#11 follow-up: @audrasjb
8 weeks ago

  • Keywords needs-refresh added

@dinhtungdu thanks for the patch, but after a full review, I think we should separate p and .wp-die-message in two CSS rules (even if they have the same CSS declarations) in src/wp-admin/css/common.css.

@dinhtungdu
8 weeks ago

#12 @dinhtungdu
8 weeks ago

  • Keywords dev-feedback added; needs-refresh removed

Thank @audrasjb, I updated the patch with your comment :)

#13 @audrasjb
8 weeks ago

  • Keywords dev-feedback removed

Thanks! 47580.2.diff is looking great to me 👍

This ticket was mentioned in Slack in #core by pento. View the logs.


7 weeks ago

#15 @mikeschroder
7 weeks ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 45909:

Administration: Output valid HTML when wp_die() is called.

To better support HTML and string calls to wp_die() without
outputting invalid HTML, wraps error messages in <div> rather than <p>.

Adds .wp-die-message CSS class for styling.

Props dinhtungdu, jeremyfelt, audrasjb, SergeyBiryukov, afercia, audrasjb, noisysocks.
Fixes #47580.

#16 in reply to: ↑ 11 ; follow-up: @SergeyBiryukov
7 weeks ago

Replying to audrasjb:

a full review, I think we should separate p and .wp-die-message in two CSS rules (even if they have the same CSS declarations) in src/wp-admin/css/common.css.

Just curious, what was the reason for separating the rules? It seems that would make it harder to keep them consistent in the future.

#17 in reply to: ↑ 16 ; follow-up: @dinhtungdu
7 weeks ago

Replying to SergeyBiryukov:

Replying to audrasjb:

a full review, I think we should separate p and .wp-die-message in two CSS rules (even if they have the same CSS declarations) in src/wp-admin/css/common.css.

Just curious, what was the reason for separating the rules? It seems that would make it harder to keep them consistent in the future.

IMO, we don't need to keep them consistent in the future, they are two separated elements. We have their CSS rules identical for now because I want to keep the style as it was. In the future, it's more likely that we'll change the style for .wp-die-message.

#18 in reply to: ↑ 17 ; follow-up: @SergeyBiryukov
7 weeks ago

Replying to dinhtungdu:

IMO, we don't need to keep them consistent in the future, they are two separated elements. We have their CSS rules identical for now because I want to keep the style as it was. In the future, it's more likely that we'll change the style for .wp-die-message.

It just seems inconsistent to me that they're combined in _default_wp_die_handler() styles, but separated in common.css. I would think it's better to reduce duplication for now and separate them in the future if needed.

#19 in reply to: ↑ 18 @dinhtungdu
7 weeks ago

Replying to SergeyBiryukov:

It just seems inconsistent to me that they're combined in _default_wp_die_handler() styles, but separated in common.css. I would think it's better to reduce duplication for now and separate them in the future if needed.

Yeah, it makes sense now :). What do you think @audrasjb ?

#20 @audrasjb
3 days ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.