WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#12780 closed defect (bug) (fixed)

get_search_query() can be confusing as it doesn't sanitize

Reported by: Viper007Bond Owned by: ryan
Milestone: 3.0 Priority: high
Severity: normal Version: 3.0
Component: Template Keywords:
Focuses: Cc:

Description

the_search_query() is the recommended way to display what a user searched for. But what if you need the_search_query()'s output for use in PHP, i.e. the value returned? get_search_query() seems like the correct function to use, but they differ in one very important way -- get_search_query() does not escape it's output at all.

It's an easy mistake as most get_ functions are identical to their echo'ing counterparts and most users don't realize the difference. This can easily result in a XSS attack.

I'm not sure what the solution to this is, but there should be an easier way to get a safe search query than having the user call esc_attr(), get_search_query(), etc.

Perhaps deprecated get_search_query() and introduce get_the_search_query or something.

Attachments (1)

get_search_query.diff (5.4 KB) - added by nacin 5 years ago.
Making sure we're at least back compat with filters, and never passing already-escaped values to a filter. Also modify instances in core.

Download all attachments as: .zip

Change History (9)

comment:1 @Viper007Bond5 years ago

  • Component changed from General to Template

comment:2 @Viper007Bond5 years ago

As proof, this common mistake was even made in the Twenty Ten theme: #12781

comment:3 follow-up: @nacin5 years ago

  • Milestone changed from Unassigned to 3.0
  • Priority changed from normal to high

Deprecating it for get_the_search_query() doesn't do much good. We can't even get plugin authors to obey the deprecated API.

I suggest we break back compat here and escape it. The Codex is wrong, Twenty Ten is wrong, many many themes are inviting XSS.

If anyone wants the unescaped value, they can call the query var themselves.

comment:4 in reply to: ↑ 3 @Viper007Bond5 years ago

Replying to nacin:

Deprecating it for get_the_search_query() doesn't do much good. We can't even get plugin authors to obey the deprecated API.

I suggest we break back compat here and escape it. The Codex is wrong, Twenty Ten is wrong, many many themes are inviting XSS.

If anyone wants the unescaped value, they can call the query var themselves.

That sounds like an acceptable solution to me.

comment:5 @Viper007Bond5 years ago

  • Owner set to ryan
  • Status changed from new to assigned

@nacin5 years ago

Making sure we're at least back compat with filters, and never passing already-escaped values to a filter. Also modify instances in core.

comment:6 @nacin5 years ago

(In [13978]) Have get_search_query() escape by default, like it's echoing counterpart the_search_query(). see #12780

comment:7 @nacin5 years ago

Checking this in and leaving open for now. Since esc_attr and attribute_escape don't double-escape, this won't have any adverse effects when the API was used. We're only breaking htmlspecialchars here.

comment:8 @nacin5 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.