Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#22690 closed defect (bug) (fixed)

Twenty Twelve: twentytwelve_content_nav $nav_id is not validated.

Reported by: ounziw Owned by: lancewillett
Milestone: 3.5 Priority: normal
Severity: minor Version:
Component: Bundled Theme Keywords: has-patch
Focuses: Cc:


In functions.php of TwentyTwelve Theme, function "twentytwelve_content_nav" is defined.

twentytwelve_content_nav takes a parameter called $nav_id. $nav_id is echoed without validated nor escaped.

When careless people write a code like twentytwelve_content_nav( 'nav below' ), it breaks HTML's rule.

function twentytwelve_content_nav( $nav_id ) {
	global $wp_query;

	if ( $wp_query->max_num_pages > 1 ) : ?>
		<nav id="<?php echo $nav_id; ?>" class="navigation" role="navigation">

I propose to add

$nav_id = esc_attr( str_replace(' ','',$nav_id ) );

at the beginning of the function definition.

Attachments (3)

22690.patch (1.0 KB) - added by SergeyBiryukov 5 years ago.
22690.2.patch (1.0 KB) - added by SergeyBiryukov 5 years ago.
22690.diff (1.1 KB) - added by nacin 5 years ago.

Download all attachments as: .zip

Change History (11)

#1 @SergeyBiryukov
5 years ago

  • Component changed from Themes to Bundled Theme
  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.5
  • Summary changed from TwentyTwelve twentytwelve_content_nav $nav_id is not validated. to Twenty Twelve: twentytwelve_content_nav $nav_id is not validated.

#2 @helenyhou
5 years ago

Would something like sanitize_key() make sense? Since you can't have spaces/multiple IDs anyway.

#3 @ryan
5 years ago

Or sanitize_html_class()?

#4 @SergeyBiryukov
5 years ago

22690.2.patch uses sanitize_html_class().

5 years ago

#5 @nacin
5 years ago

Looks like the only difference between sanitize_html_class() and sanitize_key() is that the former A) allows for a fallback value, B) has a filter, C) strips octets. They use the same sanitization.

It's possible that in the future, sanitize_html_class() is expanded to all characters possible in a class, which is slightly different than what is allowed in an ID.

sanitize_key() seems fine here. But, either functioncould break a hypothetically valid ID already in use. "nav below" is not a valid ID. Perhaps we rename the argument from $nav_id to $html_id and then just drop esc_attr() in. There is only so much we should do to prevent someone from shooting themselves in the foot. Eventually they're just going to do it.

Version 0, edited 5 years ago by nacin (next)

#6 @lancewillett
5 years ago

Note: Twenty Eleven needs the change, also.

#7 @lancewillett
5 years ago

In 22999:

Twenty Twelve: escape navigation ID output, props nacin. See #22690.

#8 @lancewillett
5 years ago

  • Owner set to lancewillett
  • Resolution set to fixed
  • Status changed from new to closed

In 23000:

Twenty Eleven: escape navigation ID output, closes #22690.

Note: See TracTickets for help on using tickets.