WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#11605 closed defect (bug) (wontfix)

wpdb::_weak_escape() is an alias to addslashes only

Reported by: hakre Owned by: ryan
Milestone: Priority: normal
Severity: normal Version: 2.9
Component: Database Keywords: has-patch tested featured
Focuses: Cc:

Description

esc_sql has been introduced for completeness (esc_* named functions; [11490]). time has showed that next to the function naming game (offering kind of an API without an API definition) nothing more has evolved since then. so it's a fact that it does not properly escape SQL, it's just an alias (one with many lines of code for such a thing) for addslashes with it's meaning to add slashes.

the code can better reflect that with a simple change.

Attachments (5)

11605.patch (374 bytes) - added by hakre 5 years ago.
same API but easier to read and to improve in the future.
11605.2.patch (581 bytes) - added by hakre 5 years ago.
11605.3.patch (779 bytes) - added by hakre 5 years ago.
And the next iteration. I do not see much more this function can benefit from.…
11605.4.patch (749 bytes) - added by hakre 5 years ago.
11605.5.patch (1.0 KB) - added by hakre 5 years ago.

Download all attachments as: .zip

Change History (33)

@hakre5 years ago

same API but easier to read and to improve in the future.

comment:1 @nacin5 years ago

  • Keywords has-patch removed
  • Milestone 3.0 deleted
  • Resolution set to invalid
  • Status changed from new to closed
  • Version 2.9 deleted

esc_sql() is not an alias for addslashes(). It is an alias for $wpdb->escape(), big difference:

Anyone using a drop-in wpdb replacement would suddenly find their SQL unescaped.

wpdb->escape() supports arrays.

This change might cause some developers to simply use addslashes() instead of esc_sql(). $wpdb->escape() used to be set up to do more than addslashes(), and may evolve in the future. (Also take a look at the _escape and _real_escape methods.) Thus, definitely not easier to improve in the future.

comment:2 @nacin5 years ago

Anyone using a drop-in wpdb replacement would suddenly find their SQL unescaped.

Should be:
... would suddenly find their SQL escaped in what may be a method that differs from what their drop-in specifies.

comment:3 @dd325 years ago

Side note: Array support was added to wpdb::escape() in WordPress 2.8

comment:4 @nacin5 years ago

Additional side note:

mysql_escape_string was removed in [1545]. mysql_real_escape_string was added in [2684], removed in [2737], conditionally added in [10597], partially removed in [10604].

comment:5 @hakre5 years ago

  • Keywords has-patch added
  • Milestone set to 3.0
  • Resolution invalid deleted
  • Status changed from closed to reopened
  • Summary changed from esc_sql does not escape sql, it just adds slashes to wpdb::_weak_escape() is an alias to addslashes only
  • Version set to 2.9

FYI 1: from wordpress codex:

esc_sql( $text ) (since 2.8) 
$wpdb->escape( $text ) 
Escapes a single string for use in a SQL query. Glorified addslashes().

FYI 2: from esc_sql function documentation:

/**
 * Escapes data for use in a MySQL query
 *
 * This is just a handy shortcut for $wpdb->escape(), for completeness' sake
 *
 * @since 2.8.0
 * @param string $sql Unescaped SQL data
 * @return string The cleaned $sql
 */

I must admit that I did not whink about the possibility to replace WPDB with an own implementation right. So the error is "only" in the default implementation and therefore changing esc_sql() is wrong.

It is naturally in the default (not overwritten) implementation, function wpdb::_weak_escape() is the alias to addslashes(). So that's the place where it must be corrected then.

@hakre5 years ago

comment:6 @hakre5 years ago

Updated patch, reflects the correct position where the alias was in place. Preserves compability with third-party WPDB implementations.

Additionally the function has benefitted from the re-work, unnecessary if clause could be removed.

@hakre5 years ago

And the next iteration. I do not see much more this function can benefit from....

comment:7 @hakre5 years ago

Updated patch, I know it would be easier to read to have this in 4 lines, but it's so long ago we had a oneliner in core... .

comment:8 @nacin5 years ago

  • Keywords close added

-1.

Escaping in wpdb is abstracted into escape, _escape, _weak_escape and _real_escape for very good reasons.

It is naturally in the default (not overwritten) implementation, function wpdb::_weak_escape() is the alias to addslashes().

You said it yourself.

As an example, you are again adversely affecting drop-ins, which can extend and rewrite the wpdb class and replace methods. i.e. bbPress extends wpdb and adds one method. A drop-in could extend wpdb and replace _weak_escape, which suddenly would cease to be called by wpdb::escape. The point here is the concept of abstraction, not even whether there exists a drop-in that does this.

Suggest closing as invalid.

comment:9 @Denis-de-Bernardy5 years ago

Escaping in wpdb is abstracted into escape, _escape, _weak_escape and _real_escape for very good reasons.

Err, I'd say that these various functions exist for extremely bad reasons. We've banged our head into the table, for years, in order to support completely obsolete versions of PHP and MySQL. We end up with workarounds and workarounds around the workarounds.

It's like, heck, escape() should do exactly that: escape. Not weak, or real, or maybe, or maybe not, or anything else; plain, simple escape. There should only be a single method and it should do its job properly.

WP 3.0 probably isn't the right time to clean this mess up, though. Maybe when we switch to PHP 5.1 and decide whether we start using PDO?

comment:10 @Denis-de-Bernardy5 years ago

  • Keywords close removed

comment:11 follow-up: @hakre5 years ago

I'm pretty shure those function-names start with _ to signal that they are inteded for private use. That is an idiom in PHP 4 code in the lack of private members. Since the accesibility of the functions you refrain is not documented in any way, the extending class can not rely on the call from escape() to _weak_escape.

@Nacin: Just ask, I can provide you references for that _ in function names and PHP 4.

I'll clean the patch that it is better readable. Maybe it was too hard to read :)

@Denis: This ticket is not about the jam-packed treasure chest by the god of escape WP has to offer (this word seems to have some special magic, just do a fulltext search, if there is a problem, you only need to escape it :) ). I've just run over this optimization, I'm sure there pretty much more but when it's left to be fixed, why not improve it? Otherwise this will never happen... (<- I'm ready-minded to be proven wrong).

@hakre5 years ago

comment:12 @hakre5 years ago

@nacin: I'm not pedantic (than I would take a look when that comment went in), but if a developer writes it's own implementation of wpdb without taking the default function definition into account, I think that guy is pretty lost.

* Escapes content for insertion into the database using addslashes(), for security

Guess where this line is taken from. But that only as a sidenote, I do not want to argue about that any longer. You can update the patch that it does the additional function call to the private member _weak_escape (which otherwise could just be deleted, because it's not needed any longer) and then at least the escape function benefitted from the other improvements the patch has.

comment:13 @hakre5 years ago

  • Keywords tested added

comment:14 in reply to: ↑ 11 ; follow-up: @nacin5 years ago

Replying to hakre:

I'm pretty shure those function-names start with _ to signal that they are inteded for private use.

In this case, I would argue that if anything, they are for protected use, not private. Many drop-ins replace wpdb::_real_escape() with a method that calls, say, pg_escape_string() or sqllite_escape_string().

Can we simplify this? Sure, we can change all references of wpdb::_weak_escape() to addslashes(), and maybe even remove wpdb::_weak_escape() when we're done. But unless we take it further as Denis said and overhaul how wpdb escapes SQL, what is truly necessary?

comment:15 in reply to: ↑ 14 @hakre5 years ago

Replying to nacin:

Replying to hakre:

I'm pretty shure those function-names start with _ to signal that they are inteded for private use.

In this case, I would argue that if anything, they are for protected use, not private. Many drop-ins replace wpdb::_real_escape() with a method that calls, say, pg_escape_string() or sqllite_escape_string().

You are so damn smart that I want you to get even smarter. Please tell me which programming related book you would like to get, I'll send it to you. PHP 4 an protected function, I've never been so amused lately.

Can we simplify this? Sure, we can change all references of wpdb::_weak_escape() to addslashes(), and maybe even remove wpdb::_weak_escape() when we're done. But unless we take it further as Denis said and overhaul how wpdb escapes SQL, what is truly necessary?

Well, not much. I've updated the patch to reflect the needed changes for the whole core code: 11605.5.patch

@hakre5 years ago

comment:16 follow-up: @Denis-de-Bernardy5 years ago

I like 11605.5.patch. does the same thing, except that it's simpler to read. could we get this checked in?

comment:17 @hakre5 years ago

I added those changes of 11605.5.patch to #11799 as well.

comment:18 @hakre5 years ago

Related: #11819

comment:19 @Denis-de-Bernardy5 years ago

  • Keywords bug-hunt added

comment:20 @Denis-de-Bernardy5 years ago

  • Keywords featured added; bug-hunt removed

comment:21 in reply to: ↑ 16 @miqrogroove5 years ago

Replying to Denis-de-Bernardy:

I like 11605.5.patch. does the same thing, except that it's simpler to read. could we get this checked in?

I like the simplification of escape(), but I'm confused by what's going on with the deletes in that patch?

comment:22 @miqrogroove5 years ago

Also, pardon the dumb questions, why does escape() allow array input?

comment:24 follow-ups: @dd325 years ago

  • Milestone changed from 3.0 to Future Release

We're close to a beta, This is no longer the time to be making database escaping decisions/changes.

comment:25 in reply to: ↑ 24 @hakre5 years ago

  • Milestone changed from Future Release to 3.0

Replying to dd32:

We're close to a beta, This is no longer the time to be making database escaping decisions/changes.

Last patch is not changing database escaping. It calls the exact same functions as before, it only does it better. Please review the code again.

comment:26 in reply to: ↑ 24 @nacin5 years ago

  • Milestone changed from 3.0 to Future Release

Replying to dd32:

We're close to a beta, This is no longer the time to be making database escaping decisions/changes.

Those concerns are still valid. We've also made more than enough structural changes to 3.0 for this cycle. Database dropins will have a hard time adapting as it is.

comment:27 @nacin3 years ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

comment:28 @nacin3 years ago

  • Component changed from Security to Database
Note: See TracTickets for help on using tickets.