Opened 13 years ago
Closed 13 years ago
#22690 closed defect (bug) (fixed)
Twenty Twelve: twentytwelve_content_nav $nav_id is not validated.
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Milestone: | 3.5 | Priority: | normal |
| Severity: | minor | Version: | |
| Component: | Bundled Theme | Keywords: | has-patch |
| Focuses: | Cc: |
Description
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)
Change History (11)
#1
@
13 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.
#4
@
13 years ago
22690.2.patch uses sanitize_html_class().
#5
@
13 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 function could 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.
Would something like
sanitize_key()make sense? Since you can't have spaces/multiple IDs anyway.