Make WordPress Core

Opened 9 years ago

Closed 6 years ago

#20125 closed enhancement (worksforme)

Escape output in settings_errors

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


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>' );


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 9 years ago.

Download all attachments as: .zip

Change History (9)

9 years ago

#1 @kurtpayne
9 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.

#2 in reply to: ↑ description @solarissmoke
9 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.

#3 @nacin
7 years ago

  • Component changed from General to Plugins

#4 @nacin
7 years ago

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

#5 @nacin
7 years ago

  • Component changed from Admin APIs to Plugins

Sorry for the noise.

#6 @wonderboymusic
6 years ago

@tollmanz: do you still care?

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

6 years ago

#8 @DrewAPicture
6 years ago

  • Keywords close removed
  • Milestone Awaiting Review deleted
  • Resolution set to worksforme
  • Status changed from new to closed

Per feedback from @tollmanz in the linked Slack logs above, closing as worksforme.

Note: See TracTickets for help on using tickets.