WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 3 months ago

#20125 new enhancement

Escape output in settings_errors

Reported by: tollmanz Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.0
Component: Plugins Keywords: has-patch close
Focuses: administration Cc:

Description

The Problem

The "settings_errors" function does not escape data when outputting it from the $settings_errors variable, which either comes from the $wp_settings_errors global variable or the "settings_errors" transient. This data is not escaped at any point during retrieval or output. Additionally, the data is not sanitized or validated when adding it via "add_settings_error".

Test Case

In the validation callback function for a setting, adding a settings error with HTML can badly break output:

add_settings_error( 'zdt-setting', '1023', 'An error occurred</div>' );

Solution

Escape the $type, $code, and $message variables on output.

Possible Issues

The $message variable is output wrapped in a 'p' and 'strong' tag. My patch tries to allow common, reasonable tags to be output. I use "wp_kses_data", which will only allow tags defined in the $allowedtags variable. Should a plugin or theme author need another element printed out, it will be stripped; however, there still is the potential that it could cause issues for plugins that were previously able to place anything in the $message variable. With that said, most other tags would lead to invalid HTML and probably shouldn't be allowed in this context anyway.

Attachments (1)

20125.diff (788 bytes) - added by tollmanz 2 years ago.

Download all attachments as: .zip

Change History (6)

tollmanz2 years ago

comment:1 kurtpayne2 years ago

  • Cc kpayne@… added

Here's the way I'm reading this code / documentation: add_settings_error is expecting $message to contain pre-sanitized data and to be formatted exactly as the theme developer intends.

The docs say this:

 * @param string $message The formatted message text to display to the user (will be shown inside styled <div> and <p>)

I would argue that any user data being passed through this function should already be sanitized.

add_settings_error( 'zdt-setting', '1023', 'The value: ' . wp_kses_data( $val ) . ' is not allowed' );

Otherwise WordPress should dump this data out directly.

comment:2 in reply to: ↑ description solarissmoke2 years ago

  • Keywords close added

Replying to tollmanz:

My patch tries to allow common, reasonable tags to be output. I use "wp_kses_data", which will only allow tags defined in the $allowedtags variable.

Generally WordPress does not restrict plugins in this manner - IMO that's what makes the plugin API so powerful. I think it's the responsibility of plugin authors to make sure their code doesn't break things.

comment:3 nacin3 months ago

  • Component changed from General to Plugins

comment:4 nacin3 months ago

  • Component changed from Plugins to Admin APIs
  • Focuses administration added

comment:5 nacin3 months ago

  • Component changed from Admin APIs to Plugins

Sorry for the noise.

Note: See TracTickets for help on using tickets.