$wpdb->insert fails without error msg

[dlt101]

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.

[zoiec]

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

[kwisatz]

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

[NettSite]

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.

[mnelson4]

yeah this is pretty annoying. I'm pretty sure this was added aspart of 4.2.1 security release: ​https://wordpress.org/news/2015/04/wordpress-4-2-1/
on this commit: https://core.trac.wordpress.org/changeset?old_path=%2Fbranches%2F4.2&old=32307&new_path=%2Fbranches%2F4.2&new=32307&sfp_email=&sfph_mail=

[jrusinek]

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.

[datainterlock]

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.

[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.

[datainterlock]

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!

[procodewp]

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

[procodewp]

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?

[mnelson4]

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 ?

[dd32]

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.

[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'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?

[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.

[datainterlock]

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?

[procodewp]

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?

[dd32]

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).

[mnelson4]

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

2. set $wpdb->last_query to a empty string (indicating no query ran)?

3. 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?

[dd32]

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.

[mnelson4]

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?

[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? 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.

[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.

[datainterlock]

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.

[pento]

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).

[mnelson4]

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

[pento]

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.

[dpegasusm]

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)

[Asif2BD]

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

[jdgrimes]

Initial patch

[jdgrimes]

32315.diff​ is a basic patch just to start things off. It just updates process_fields() to set last_error when strip_invalid_text() results in changes to the data (in other words, when the data was invalid and had to be corrected), thus causing process_fields() to return false. I don't think that the other places that it can return false are important to handle here, since those methods only fail when a database error has occurred attempting to get the column info or something, and thus the error will already be set.

I've included some basic unit tests in the patch as well.

I hope this can get the ball rolling on this ticket. Feedback on the general direction of the patch would be appreciated.

[mnelson4]

I don't think that the other places that it can return false are important to handle

+1, if we want to tackle those, let's do those on another ticket (maybe we should rename this ticket to specify its only for failures when the value is too big for the column or contains invalid data?)

I've included some basic unit tests in the patch as well.

All star.

I ran a manual test and the results looked good to me. I ran $wpdb->insert with a value that I knew was too big for the column and got

"WordPress Database Error: Processing the value for the following field failed, the supplied value may have been too long or contained invalid data: {name-of-column}."

Whereas without this patch I just got an empty string for $wpdb->last_error. So I think this is certainly an improvement.

[jdgrimes]

Includes query() too, and additional unit tests

[jdgrimes]

Thanks for testing @mnelson4.

I've uploaded 32315.2.diff​, which also sets last_error and last_query when invalid characters are detected in the query() method (the previous patch only addressed the CRUD methods). I've also included more unit tests.

[dd32]

#42122 was marked as a duplicate.

[SergeyBiryukov]

#43087 was marked as a duplicate.

[nlpro]

#32167 was marked as a duplicate.

[galbaras]

This sure seems like it's important, but it hasn't been fixed in 4 years, despite having a patch :(

[skunkbad]

Yep, wasted almost 2 hours today figuring out that this issue exists. Either let there be an error or do something besides silently fail, please. This is actually the latest version of WordPress, 5.1.

[liammitchell]

Yes still a problem on 5.1, frustrating, please just set the last_error to have information about which table and column was incorrect that way it is more apparent as to what is wrong.

Can confirm that 32315.diff patch cleared up what was wrong and I was able to fix my plugins code.

[travisnorthcutt]

@pento do you have any interest in getting this merged?

[FPCSJames]

@pento Please, PLEASE consider merging the first patch here. It's absolutely bonkers that invalid data should cause a query to just fail silently - it makes tracking down the source of an issue virtually impossible when good code suddenly stops working due to a change somewhere in the hosting environment. I spent hours tracking down why Gravity Forms inserts were failing silently today - the answer is that they were storing the IP address of the submitter in a varchar(39) column, and something in the hosting environment was inserting [] around IPv6 addresses. Without the solution in the patch, which I used, I would have had no leads for far longer.

Silent death is madness. Fix this.

[richardfoley]

Yep, just spent several hours with WP 5.2.1 trying to figure out why my insert was first silently failing and then, after tracking down the debug commands, still failing with the equally mysterious

... SHOW FULL COLUMNS FROM ...

3 years later and this silently failing error from wordpress is STILL biting people.

PLEASE FIX THIS!

[woodyhayday]

For those who've come here looking to specifically overcome $wpdb silently failing when an emoji is passed, and need a temporary workaround. Passing through

wp_encode_emoji()

... lets emoji-ridden strings be successfully passed through $wpdb.

Not a long-term fix. Suspect adding 4byte emoji encoding to the data-conversion would be sensible?

[audrasjb]

Hi,

With 5.4 Beta 3 approaching and the Beta period reserved for bugs introduced during the cycle, this is being moved to Future Release. If any maintainer or committer feels this should be included or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

[justindocanto]

Just ran into this issue today and remembered this ticket existed. This is currently tagged for 5.6 and 5.5 just launched, so hopefully it's time to take a look at finally fixing this 5 year old bug?

[audrasjb]

Hello @justindocanto,

Sure! It would be nice to fix it. For the moment, it's still waiting for someone to propose an up-to-date patch that fixes the issue. Any refreshed patch would be welcome to help this moving forward.

[anand.au14]

Added refreshed patch

[davidbaumwald]

On the most recent patch, I'm thinking that the two strings in the count( $problem_fields ) === 1 conditional should be merged and use _n instead.

[anand.au14]

@davidbaumwald

I liked your idea but upon exploring about it further I came across some recommendations given for when to use _n. So just got little bit confused.

this functions should NOT be use in one item or more than one item scenarios, but for singular form and plural forms, which is not the same thing

Some reference links of initial quotes by @SergeyBiryukov are here in this comment
​https://developer.wordpress.org/reference/functions/_n/#comment-2397

@SergeyBiryukov Can you check if it is valid to use _n in this case? I will update the patch if required.

[anthonyeden]

I just came here to add my voice to the 'I can't believe this hasn't been fixed after 5 years' chorus.

Any effort to get this patched would be appreciated.

[hellofromTonya]

With RC 1 for 5.6 landing tomorrow, punting this ticket to Future Release.

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

[lucasw89]

Can someone please fix this already. I mean it's been 6 years since the ticket was opened...Can't believe this still hasn't been patched.

[datainterlock]

I gave up waiting and arguing my point. I haven't typed $wpdb->insert in years and I'll keep using $wpdb->query('Insert even when this is fixed if for no other reasons than it's become habit and it works.

[lukecarbis]

With not much activity happening here and beta 3 now out for 5.7, moving this to Future Release for now.

[galbaras]

After more than 6 years for something so fundamental, isn't it better to get some activity happening than to move it again?

Seems like https://core.trac.wordpress.org/attachment/ticket/32315/32315.3.diff includes a fix and a test. Can this be tested and progressed if it's better, even if it's not perfect?

[psufan]

I am having this issue too, this needs to be fixed. last_error needs to be populated. Why not truncate data and proceed like normal database inserts...?

[chaion07]

Thanks to @dlt101 for reporting this. We reviewed this ticket during a recent [bug-scrub session]​https://wordpress.slack.com/archives/C02RQBWTW/p16230975713658000. With Beta 1 coming in a day, checking with @SergeyBiryukov to kindly draw some attention to this. Thank you!

[hellofromTonya]

Today is 5.8 Beta 1. No activity in the cycle. Punting to 5.9 for investigation and resolution.

[datainterlock]

Six years of kicking the can.

[FPCSJames]

Replying to chaion07:

Thanks to @dlt101 for reporting this. We reviewed this ticket during a recent [bug-scrub session]​https://wordpress.slack.com/archives/C02RQBWTW/p16230975713658000. With Beta 1 coming in a day, checking with @SergeyBiryukov to kindly draw some attention to this. Thank you!

@SergeyBiryukov et al - what's the holdup here? What's missing? Why are we still talking about this six years after report instead of shipping a fix?