WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 2 months ago

#32315 reopened defect (bug)

$wpdb->insert fails without error msg

Reported by: dlt101 Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.8
Component: Database Keywords: needs-patch needs-unit-tests
Focuses: Cc:

Description

Given a field in MySQL defined as char(8), but the string you attempt to insert in that field has a length of 20.
If you use $wpdb->query($wpdb->prepare(...)) syntax, it will insert the record (and possibly truncate the string).
If you use $wpdb->insert(...) syntax, no record is created, and no error is reported in any manner. If you edit the MySQL def to char(20) or shorten the string to 8 chars, $wpdb->insert then inserts the record.

Preferably, both forms of syntax should work the same and either (a) provide an understandable error and fail; (b) provide a warning and succeed.

Change History (30)

#1 @zoiec
2 years ago

I also found this bug.

MySQL varchar length set to 20
string being inserted was 29

tried
$wpdb->update and $wpdb->insert
both fail without error message

#2 follow-up: @kwisatz
22 months ago

Confirming this issue. $wpdb->last_query will output the previous query and $wpdb->last_error will output an empty array in this case. In my case, this occurred on 4.3.1

#3 @NettSite
17 months ago

The problem is here, in /includes/wp-db.php:

<?php
/**
   * For string fields, record the maximum string length that field can safely save.
   *
   * @since 4.2.1
   * @access protected
   *
   * @param array  $data  As it comes from the wpdb::process_field_charsets() method.
   * @param string $table Table name.
   * @return array|false The same array as $data with additional 'length' keys, or false if
   *                     any of the values were too long for their corresponding field.
   */
  protected function process_field_lengths($data, $table) {
    foreach ($data as $field => $value) {
      if ('%d' === $value['format'] || '%f' === $value['format']) {
        /*
         * We can skip this field if we know it isn't a string.
         * This checks %d/%f versus ! %s because its sprintf() could take more.
         */
        $value['length'] = false;
      } else {
        $value['length'] = $this->get_col_length($table, $field);
        if (is_wp_error($value['length'])) {
          return false;
        }
      }

      $data[$field] = $value;
    }

    return $data;
  }

This function simply returns "FALSE" if a data field is too long.

There should be some means of discovering which field is too long, e.g. logging an error, or displaying one if WP_DEBUG is TRUE.

This would have saved me a good few hours of barking up wrong trees this morning.

#5 @jrusinek
11 months ago

  • Severity changed from normal to major
  • Version 4.2.2 deleted

I cannot believe this is still an issue. I suffer from it in 4.5.2, but I am pretty sure 4.6.1 still has it. I got here through Social Login plugin issues.

#6 @datainterlock
11 months ago

  • Keywords needs-patch added
  • Resolution set to invalid
  • Severity changed from major to critical
  • Status changed from new to closed
  • Version set to 4.6.1

Confirmed. It still exists in 4.6.1 after a year and a half! I just spent HOURS trying to figure out why my database was being flooded with bad data after trying to get a $wpdb->insert_id and having it return false without any sql error. The insert fails with no error or record produced. $wpdb->last_query shows the query BEFORE the insert and $wpdb->last_error is empty.

Bumping this to critical as having a truncated insert is one thing. Having no error produced after a failed insert is just unconscionable.

Last edited 11 months ago by datainterlock (previous) (diff)

#7 @datainterlock
11 months ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

#8 follow-up: @pento
11 months ago

  • Keywords needs-unit-tests added
  • Severity changed from critical to normal
  • Version changed from 4.6.1 to 4.2.1

I'm cool with adding an error message if this method fails, it'll make it into 4.7 if the patch is ready before November 15. I'm happy to provide feedback on any patches uploaded.

#9 in reply to: ↑ 8 @datainterlock
11 months ago

  • Version 4.2.1 deleted

Replying to pento:

I'm cool with adding an error message if this method fails, it'll make it into 4.7 if the patch is ready before November 15. I'm happy to provide feedback on any patches uploaded.

And error message would be an excellent start for when there are syntax errors and such. IMHO though, when the syntax is correct, the insert should perform the expected, truncate any entries that are too large for the field and perform the insert. The only reason I stumbled on this bug is because I assumed that's what would happen, and didn't.

Last_query should also show that the insert query was the last query attempted.

I'd offer to help but I barely have time to play with plugins.

Thanks!

#10 @procodewp
11 months ago

I`ll work on a patch for this, I will get it uploaded over the weekend.

#11 @procodewp
11 months ago

Just to confirm - error messages should be set on failure, but should it also truncate values and carry on with the insert if necessary, or fail and return an error?

#12 @mnelson4
11 months ago

should it also truncate values and carry on with the insert if necessary, or fail and return an error?

I think it should just fail and return an error. Truncating values somehow introduced a security problem, which is how this secure-but-unhelpful code was introduced in the first place (see my earlier comment where I pointed to the security release where this bug was introduced and the exact commit). Right @pento ?

#13 @dd32
11 months ago

I think it should just fail and return an error. Truncating values somehow introduced a security problem, which is how this secure-but-unhelpful code was introduced in the first place (see my earlier comment where I pointed to the security release where this bug was introduced and the exact commit). Right @pento ?

Correct. Silently truncating data as it goes into a Database can very easily cause a security vulnerability (as strange as it seems).
We should add an error message, but wpdb should continue to abort.

#14 @mnelson4
11 months ago

So if an insert fails because a value is too long for its column, where should the error message go?
$wpdb->last_error just contains MySQL errors, but this error would be generated by WordPress itself and so no MySQL query is actually ran (or generated, in fact).

I'm inclined to suggest we should just use $wpdb->print_error(), with text like "You attempted to put x characters into a column that only accepts y, so your query was not performed." The error will then be recorded on the global $EZSQL_ERROR too, an error will be printed if WP_DEBUG is on, and $wpdb->last_error will continue to only be errors generated by MySQL.

Thoughts?

#15 follow-up: @datainterlock
11 months ago

Php Mysqli truncates and inserts. That's how wp should work. Most plugin devs will assume this and not even perform an error check. I did and I'm betting a lot of current plugins still do. My advise, find another way to fix the security hole instead of changing php conventional methods.

#16 @datainterlock
11 months ago

You realize how much extra code an insert would require if you error? Every insert var would have to be checked for length prior to insert. Why would I use it then instead of a general query?

#17 @procodewp
11 months ago

I`ve been looking into this over the weekend and found the following:

When you call the insert() or update() routine in wpdb, it's calling process_fields().

Inside of process_fields(), it calls strip_invalid_text() which removes invalid characters, and also truncates the data to the field length.

When strip_invalid_text() returns the amended $data, the following code is run:

if ( $data !== $converted_data ) {
  return false;
}

This compares the original $data with the $converted_data returned from the strip_invalid_text() function, and if anything has been changed it returns false.

This stops the operation from occuring, without an error message. So if any invalid characters are removed, or the field is truncated, everything stops.

The problem from the error side is that the existing error variables are set up to log errors returned by MySQL. But in this case, the query is never sent to the database, it's an internal error rather than a database error, so it doesn't get logged anywhere.

You realize how much extra code an insert would require if you error? Every insert var would have to be checked for length prior to insert.

This is already happening, each insert var string passes through process_field_lengths() which gets the size of the database field. strip_invalid_text() then uses this to truncate the field if necessary.

I'm not not sure of the best way to handle this. It would be possible to run through the $data array and compare it to the $converted_data array returned from strip_invalid_text(), which would allow a list of any fields with data that's been changed. But what would be the best way to display this information to the user?

Would it be worth creating a variable to hold internal errors, for errors in functions that occur before a database operation?

Last edited 11 months ago by procodewp (previous) (diff)

#18 in reply to: ↑ 15 @dd32
11 months ago

Replying to mnelson4:

So if an insert fails because a value is too long for its column, where should the error message go?
$wpdb->last_error just contains MySQL errors, but this error would be generated by WordPress itself and so no MySQL query is actually ran (or generated, in fact).

I would just re-use it myself. An error coming from MySQL or from WordPress should be handled the same, if print_error() is called with a MySQL error this should also hit it. At the end of the day simplicity above all, plugins and code should just be able to check the property for "did an error occur?" regardless of what triggered it.

Replying to datainterlock:

Php Mysqli truncates and inserts. That's how wp should work. Most plugin devs will assume this and not even perform an error check. I did and I'm betting a lot of current plugins still do. My advise, find another way to fix the security hole instead of changing php conventional methods.

The alternatives to what we currently do is to put MySQL into strict mode, which would cause the queries to fail. Unfortunately during investigation we realised this wasn't a viable solution and had to handle it ourselves. Simply truncating data isn't an option, and any application which relies upon that behaviour and inserts user-provided data into a table is potentially vulnerable to a range of issues (feel free to ping me directly on Slack: @dd32 if you want to discuss what those are).

#19 follow-up: @mnelson4
11 months ago

thanks for the reply @dd32; ya putting the error in $wpdb->last_error would be conveniently simple for client code.

Now what about $wpdb->last_query: should something be put in there when this error occurs? No query is ran, and the SQL statement doesn't even get generated. But I suspect most folks will still want to check it to see what went wrong with their insert or update query (in addition to $wpdb->last_error).

So when this error occurs, should we

  1. finish generating the SQL, put it in $wpdb->last_query etc, but not run the query? (I think this would be the most helpful to folks, but it might be confusing for someone reading the code that we detect a error but wait to abort the process.) Or
  1. set $wpdb->last_query to a empty string (indicating no query ran)?
  1. set $wpdb->last_query to be some sort of descriptive phrase like "WordPress was about to insert values(x, y, z) into columns (w,v,r), but column 't' was too small" (this has the advantage of allowing us to abort the execution right away, but we're not creating actual SQL like we normally would)?

Or something else?

Last edited 11 months ago by mnelson4 (previous) (diff)

#20 in reply to: ↑ 19 ; follow-up: @dd32
11 months ago

Replying to mnelson4:

Now what about $wpdb->last_query: should something be put in there when this error occurs? No query is ran, and the SQL statement doesn't even get generated. But I suspect most folks will still want to check it to see what went wrong with their insert or update query (in addition to $wpdb->last_error).

This one is a little more difficult, as in some cases a query hasn't been provided.

  • For the case in wpdb::query() it makes sense IMHO to set last_query to the aborted query.
  • For the cases in wpdb::_insert_replace_helper(), wpdb::delete() and wpdb::update() it seems like not setting a query would be OK, continuing to build the query seems like it'll add significant code for small benefit. Could also set it to something non-query-like such as \$wpdb->update( $table )

Based on the above, I'm indifferent, If the developer provided a query, set as last_query (which is what would happen with a DB based failure anyway) if they didn't and they've simply called a helper, maybe not?

I'll be interested on what @pento's thoughts are here though. He's afk for the day though AFAIK.

#21 @mnelson4
11 months ago

For the case in wpdb::query() it makes sense IMHO to set last_query to the aborted query.

I agree

For the cases in wpdb::_insert_replace_helper(), wpdb::delete() and wpdb::update() it seems like not setting a query would be OK, continuing to build the query seems like it'll add significant code for small benefit. Could also set it to something non-query-like such as \$wpdb->update( $table )

Yeah not setting anything wouldn't be any worse than what's in there right now.

And setting it to something non-query-like, that's basically a printout of how they would have called the method, would certainly be informative, which I think is the main point. But I'm pretty sure there are plugins that record all the queries ran etc (like Query Monitor), and it might be unexpected for them to see non-SQL in $wpdb->last_query.

@johnbillion, your Query Monitor plugin uses $wpdb->last_query doesn't it? what would you think of putting non-sql into $wpdb->last_query, or $wpdb->queries etc?

#22 follow-up: @datainterlock
11 months ago

If you're dead set on making this error, tell me. What's the purpose of having insert and update when wp-query WILL truncate and insert? Why would i even waste my time coding a wp-insert when a wp-query wouldnt fail with the same exact query? In order to keep wp-insert from erroring, the vars will have to be checked for size prior to inserting. A huge waste and will still get truncated by the dev anyway. I say again, it should truncate and insert or update. That's how php works.

#23 in reply to: ↑ 22 ; follow-up: @pento
11 months ago

Replying to datainterlock:

If you're dead set on making this error, tell me. What's the purpose of having insert and update when wp-query WILL truncate and insert?

::query() is generally used for more complex queries that don't fit into the CRUD model. It has a valid purpose, but it's not necessary for basic INSERT and UPDATE queries.

Why would i even waste my time coding a wp-insert when a wp-query wouldnt fail with the same exact query? In order to keep wp-insert from erroring, the vars will have to be checked for size prior to inserting. A huge waste and will still get truncated by the dev anyway. I say again, it should truncate and insert or update. That's how php works.

You're welcome to do whatever you like on your own site, but I'd strongly recommend against it.

As has been mentioned several times on this ticket, allowing the database to truncate the string will almost certainly introduce significant security issues, as any data sanitisation you've run prior to insert (for example, using KSES to remove invalid HTML), will no longer be valid.

#24 in reply to: ↑ 23 ; follow-up: @datainterlock
11 months ago

Replying to pento:

Replying to datainterlock:

If you're dead set on making this error, tell me. What's the purpose of having insert and update when wp-query WILL truncate and insert?

::query() is generally used for more complex queries that don't fit into the CRUD model. It has a valid purpose, but it's not necessary for basic INSERT and UPDATE queries.

Why would i even waste my time coding a wp-insert when a wp-query wouldnt fail with the same exact query? In order to keep wp-insert from erroring, the vars will have to be checked for size prior to inserting. A huge waste and will still get truncated by the dev anyway. I say again, it should truncate and insert or update. That's how php works.

You're welcome to do whatever you like on your own site, but I'd strongly recommend against it.

As has been mentioned several times on this ticket, allowing the database to truncate the string will almost certainly introduce significant security issues, as any data sanitisation you've run prior to insert (for example, using KSES to remove invalid HTML), will no longer be valid.

Ok, then what's the solution then when the data you're inserting is of unknown length yet you don't want to make every field a blob? So far, I've been criticized and accused of crying in this thread when I'm trying to point out that in the REAL WORLD of data, you don't always have the luxury of knowing the exact length of what you're trying to insert and giving the end user truncated data is more valuable than no data at all.

You have my opinion. I'm done here.

#25 in reply to: ↑ 24 @pento
11 months ago

Replying to datainterlock:

Ok, then what's the solution then when the data you're inserting is of unknown length yet you don't want to make every field a blob? So far, I've been criticized and accused of crying in this thread when I'm trying to point out that in the REAL WORLD of data, you don't always have the luxury of knowing the exact length of what you're trying to insert and giving the end user truncated data is more valuable than no data at all.

Most fields that you'd store data of unknown length in are LONGTEXT fields, which allow 4GB of data. Unless you expect to go over that, you don't really need to check the length.

For shorter fields, it should be part of your standard data sanitisation process to check the length, the same as you'd check for unsafe HTML, or SQL injection attacks (note that WPDB's CRUD methods will take care of SQL injection prevention for you).

#26 @mnelson4
11 months ago

@pento do you have thoughts on the issue discussed earlier: https://core.trac.wordpress.org/ticket/32315#comment:20 ?

#27 in reply to: ↑ 20 @pento
11 months ago

Thanks for the reminder, @mnelson4. :-)

Replying to dd32:

This one is a little more difficult, as in some cases a query hasn't been provided.

  • For the case in wpdb::query() it makes sense IMHO to set last_query to the aborted query.

Yup.

  • For the cases in wpdb::_insert_replace_helper(), wpdb::delete() and wpdb::update() it seems like not setting a query would be OK, continuing to build the query seems like it'll add significant code for small benefit. Could also set it to something non-query-like such as \$wpdb->update( $table )

I'm inclined to leave last_query blank, and have last_error include which table/column it failed on, in which CRUD method. As @dd32 mentioned, generating the actual query is a lot of effort, and the query has minimal relation to the method call.

#28 in reply to: ↑ 2 @dpegasusm
6 months ago

Can confirm this issue still exists in 4.7.3 - just spent an hour debugging why a form was not inserting a row. text was 15o characters and field was varchar(100)

#29 @Asif2BD
2 months ago

  • Version set to 4.8

VentureBeat made a big news about it here: https://venturebeat.com/2017/07/19/your-wordpress-plugins-might-be-silently-losing-business-data/

And here is the public data set they prepared to show number of WordPress plugin affected in this: https://docs.google.com/spreadsheets/d/12pn2tEzEW4tGUjRUWZY7WuwvJpDOaHtBRzgCLTOQVE8/edit#gid=1224222312

This ticket was mentioned in Slack in #core by asif2bd. View the logs.


2 months ago

Note: See TracTickets for help on using tickets.