Make WordPress Core

Opened 15 years ago

Closed 13 years ago

Last modified 13 years ago

#11605 closed defect (bug) (wontfix)

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

Reported by: hakre's profile hakre Owned by: ryan's profile 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 15 years ago.
same API but easier to read and to improve in the future.
11605.2.patch (581 bytes) - added by hakre 15 years ago.
11605.3.patch (779 bytes) - added by hakre 15 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 15 years ago.
11605.5.patch (1.0 KB) - added by hakre 15 years ago.

Download all attachments as: .zip

Change History (33)

@hakre
15 years ago

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

#1 @nacin
15 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.

#2 @nacin
15 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.

#3 @dd32
15 years ago

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

#4 @nacin
15 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].

#5 @hakre
15 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.

@hakre
15 years ago

#6 @hakre
15 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.

@hakre
15 years ago

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

#7 @hakre
15 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... .

#8 @nacin
15 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.

#9 @Denis-de-Bernardy
15 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?

#10 @Denis-de-Bernardy
15 years ago

  • Keywords close removed

#11 follow-up: @hakre
15 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).

@hakre
15 years ago

#12 @hakre
15 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.

#13 @hakre
15 years ago

  • Keywords tested added

#14 in reply to: ↑ 11 ; follow-up: @nacin
15 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?

#15 in reply to: ↑ 14 @hakre
15 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

@hakre
15 years ago

#16 follow-up: @Denis-de-Bernardy
15 years ago

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

#17 @hakre
15 years ago

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

#18 @hakre
15 years ago

Related: #11819

#19 @Denis-de-Bernardy
15 years ago

  • Keywords bug-hunt added

#20 @Denis-de-Bernardy
15 years ago

  • Keywords featured added; bug-hunt removed

#21 in reply to: ↑ 16 @miqrogroove
15 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?

#22 @miqrogroove
15 years ago

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

#24 follow-ups: @dd32
15 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.

#25 in reply to: ↑ 24 @hakre
15 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.

#26 in reply to: ↑ 24 @nacin
15 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.

#27 @nacin
13 years ago

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

#28 @nacin
13 years ago

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