#24941 closed defect (bug) (invalid)
esc_sql in 3.6 changes how /r /n and maybe other characters handled
Reported by: | sc0ttkclark | Owned by: | |
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | 3.6 |
Component: | Formatting | Keywords: | |
Focuses: | Cc: |
Description
In Pods, we've been using esc_sql (maybe wrongly, after reading #21767) in 3.x, in conjunction with stripslashes_deep, when working with $_POST data.
By default, WP slashes all $_POST data on load, so we come back around and set a scoped var with the $_POST data, stripslashes_deep on the array, and use the values in the code. Once ready to save, in certain areas we've been using esc_sql.
Now I'm not yet sure what's going on well enough to fix it on my side for 3.6+, but it appears that this change 3 weeks ago may have a big part to do with it: 24718
It seems now at some point in 3.6, \r and \n become just "r" and "n".
We've been sanitizing data before it hits wp_update_post and other areas it looks like, so there's bound to be issues on our side here that we'll need to resolve now.
I'm posting this ticket at the request of @markjaquith who wanted to better understand what we're dealing with right now. I'll post updates as I determine what fixes I needed to make in case it helps other plugin developers who come across this issue.
If you read anything that would appear as a WP bug in this ticket, feel free to jump in and help figure out what needs to be done, otherwise you can close this as invalid if it ends up being a pilot error.
Code to reproduce:
$content = "Testing it out\n\nTest"; $postdata = array( 'ID' => $post_ID, 'post_content' => esc_sql( $content ) ); /* WP 3.x: $postdata[ 'post_content' ] = "Testing it out\n\nTest"; WP 3.6: $postdata[ 'post_content' ] = "Testing it out\\n\\nTest"; */ wp_update_post( $postdata );
Change History (14)
#2
@
11 years ago
Scott, just tested a sample use case in lastest 3.6:
$content = "Testing it out\n\nTest"; $content = esc_sql( $content );
There is no extra escaping there, i.e. the new content is exactly "Testing it out\n\nTest"
. Can you still confirm the issue on your end?
#3
@
11 years ago
$content = "Testing it out\n\nTest"; var_dump( $content ); var_dump( esc_sql( $content ) ); var_dump( addslashes( $content ) );
Outputs:
string(20) "Testing it out Test" string(22) "Testing it out\n\nTest" string(20) "Testing it out Test"
#4
@
11 years ago
I see now, thanks - wasn't clear from the code above. So why have you been using esc_sql
for the post_content?
#5
@
11 years ago
$content = "Testing's it outs yo\r\nyahhhhh\r\n\"Yes\" `yeohoho` !@po`j-9j-``!#~!~Testing it out\n\nTest"; var_dump( $content ); var_dump( esc_sql( $content ) ); var_dump( addslashes( $content ) );
Outputs:
string(84) "Testing's it outs yo yahhhhh "Yes" `yeohoho` !@po`j-9j-``!#~!~Testing it out Test" string(93) "Testing\'s it outs yo\r\nyahhhhh\r\n\"Yes\" `yeohoho` !@po`j-9j-``!#~!~Testing it out\n\nTest" string(87) "Testing\'s it outs yo yahhhhh \"Yes\" `yeohoho` !@po`j-9j-``!#~!~Testing it out Test"
#6
@
11 years ago
I removed the esc_sql from the three wp_update_post calls in the code. Not sure why it was using it there, this is just in a side part of Pods itself, a few of the components had been doing that. Core of Pods doesn't do this for wp_*_post.
There are still issues with the data elsewhere, I'm tracking those down right now.
#8
@
11 years ago
Maybe the core ninjas could give a reason for esc_sql to be used there, but I can't find any.
I see your point of testing with addslashes (even if we have functions like esc_html
), just FYI if you think that esc_sql
is using addslashes - it does only if no SQL connection is available in the $wpdb object when the esc_sql call is executed. Or moreover, mysql_real_escape_string
is being called instead which could escape the things differently (in SQL it makes sense to parse input in a different way to prevent injections).
http://core.trac.wordpress.org/browser/trunk/wp-includes/wp-db.php#L879
#9
@
11 years ago
@nofearinc This was comparing WP 3.x to 3.6 -- 3.6 is _doing_it_right but 3.x had been using addslashes all along:
3.x: esc_sql >> wpdb::escape >> wpdb::_weak_escape >> addslashes
3.6: esc_sql >> wpdb::_escape >> wpdb::_real_escape >> mysql_real_escape_string (if db connection active), addslashes (plus _doing_it_wrong if no connection)
#10
@
11 years ago
@sc0ttkclark, I understand your concern and I still have no idea how esc_sql
would be useful in this scenario. The post insert/update process is reusing the $wpdb->insert() function which is calling a prepared statement, escaping eventual dangerous SQL code.
The only possible issue I see is with injecting markup/JavaScript when not needed, but that is not related to the esc_sql
.
#11
@
11 years ago
Sorry for the miscommunication, I stated above that it was *our* mistake that we used esc_sql in a few wp_update_post calls we made, which weren't part of the main Pods core package (they were part of a few components we included). The esc_sql change continues to affect other areas of our code right now though, which is what I'm looking into next.
#12
@
11 years ago
I think what might be happening here is we were sanitizing certain arrays at first, then in certain areas down the line in functions, it would unsanitize the data using stripslashes_deep, which with the 3.6 change in slashing for \r \n and other characters, means that it will egregiously remove those backslashes incorrectly.
Deeper issue for use to solve, but I think that's the primary issue right now.
#13
@
11 years ago
- Resolution set to invalid
- Status changed from new to closed
OK, going to close this as invalid, looks like we've got a lot of work to get Pods sanitization back under control for the 3.6 slashing changes. Some of this stuff just "worked" because it was legacy and it was the way it was, so it ended up spreading across the codebase. We were able to slash and unslash data willy nilly because esc_sql mapped to addslashes, but there's no quick fix for this because stripslashes_deep isn't a proper anti-mysql_real_escape_string in 3.6
OK so looks like our wp_update_post usage was unnecessarily sanitizing, removed that. However, the original code to reproduce shows the main difference between esc_sql's functionality pre 3.6 and now in 3.6 -- which is causing the other issues across our codebase. I'm digging into those areas to see where things go south.