WordPress.org
Get WordPress

Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#27466 closed defect (bug) (worksforme)

WordPress 3.9 issue - database connection lost on mysql_real_escape_string() / AJAX?

Reported by: harmr's profile harmr Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Query Keywords: close
Focuses: Cc:

Description

My plugin "Leaflet Maps Marker" adds a button to the TinyMCE editor which opens a popup (current version still currently wpdialog why retest is more difficult as wpdialog has been depreciated) and allows user to start an AJAX search for markers.

Here is part of the code that is used to load the search window when the users clicks on the tinymce button:

info.html('<iframe width=\'450\' height=\'407\' scrolling=\'no\' src=\'" . $admin_url . "admin-ajax.php?action=get_mm_list&mode=html\' />')

Here is the code for the function get_mm_list that executes the sql query:

function get_mm_list()
global $wpdb;
$table_name_markers = $wpdb->prefix.'leafletmapsmarker_markers';
$table_name_layers = $wpdb->prefix.'leafletmapsmarker_layers';

$l_condition = isset($_GET['q']) ? "AND l.name LIKE '%" . mysql_real_escape_string($_GET['q']) . "%'" : '';
$m_condition = isset($_GET['q']) ? "AND m.markername LIKE '%" . mysql_real_escape_string($_GET['q']) . "%'" : '';

$marklist = $wpdb->get_results("
	(SELECT l.id, 'icon-layer.png' as 'icon', l.name as 'name', l.updatedon, l.updatedby, 'layer' as 'type' FROM $table_name_layers as l WHERE l.id != '0' $l_condition)
	UNION
	(SELECT m.id, m.icon as 'icon', m.markername as 'name', m.updatedon, m.updatedby, 'marker' as 'type' FROM $table_name_markers as m WHERE  m.id != '0' $m_condition)
	order by updatedon DESC LIMIT 50", ARRAY_A);
if (isset($_GET['q'])) 
	buildMarkersList($marklist);
	exit();

?>
<!DOCTYPE html>
<html><!--result output starting here-->
....

Since WordPress 3.9 the AJAX search is broken - if you start typing into the search field (which fires get_mm_list), the following error is displayed:


Warning: mysql_real_escape_string(): Access denied for user 'xxx'@'localhost' (using password: NO) in /wp-content/plugins/leaflet-maps-marker/inc/tinymce-plugin.php on line 57 Warning: mysql_real_escape_string(): A link to the server could not be established in /wp-content/plugins/leaflet-maps-marker/inc/tinymce-plugin.php on line 57 Warning: mysql_real_escape_string(): Access denied for user 'xxx'@'localhost' (using password: NO) in /wp-content/plugins/leaflet-maps-marker/inc/tinymce-plugin.php on line 58 Warning: mysql_real_escape_string(): A link to the server could not be established in /wp-content/plugins/leaflet-maps-marker/inc/tinymce-plugin.php on line 58

when I remove the mysql_real_escape_string() function

$l_condition = isset($_GET['q']) ? "AND l.name LIKE '%" . $_GET['q'] . "%'" : '';
$m_condition = isset($_GET['q']) ? "AND m.markername LIKE '%" . $_GET['q'] . "%'" : '';

results are displayed as usual. Anyway I am not sure if I can go without mysql_real_escape_string() as I need the user input to be sanitized.

Is there a change with mysql_real_escape_string() I am not aware of or could this be a bug?
Thanks for any help!

Change History (6)

#1 @ocean90
11 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed
  • Version trunk deleted

You shouldn't use mysql_real_escape_string() directly. Use $wpdb->prepare() instead.

$wpdb now uses mysqli if available (#21663), means there is no MySQL instance, which is required by mysql_real_escape_string() (see PHP docs).

#2 @harmr
11 years ago

  • Keywords close added

important info, thanks! using that function pretty often in my plugin.

#3 @harmr
11 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

sorry, just a short follow up question - just tried to replace

$l_condition = isset($_GETq?) ? "AND l.name LIKE '%" . $_GETq? . "%'" : ;

with

$l_condition = isset($_GETq?) ? "AND l.name LIKE '%" . $wpdb->prepare($_GETq?) . "%'" : ;

but that through a warning as second argument is needed. so I tried

$l_condition = isset($_GETq?) ? "AND l.name LIKE '%" . $wpdb->prepare("%s", $_GETq?) . "%'" : ;

but that didnt work either - now I have a SQL syntax error:

(SELECT l.id, 'icon-layer.png' as 'icon', l.name as 'name', l.updatedon, l.updatedby, 'layer' as 'type' FROM wp_leafletmapsmarker_layers as l WHERE l.id != '0' AND l.name LIKE '%'s'%') UNION (SELECT m.id, m.icon as 'icon', m.markername as 'name', m.updatedon, m.updatedby, 'marker' as 'type' FROM wp_leafletmapsmarker_markers as m WHERE m.id != '0' AND m.markername LIKE '%'s'%') order by updatedon DESC LIMIT 50

could you please help & tell what the proper replacement would be here?
thx

#4 @harmr
11 years ago

ok, I think I was too fast - I think I will take esc_sql() as a replacement here ;-)

#5 @nacin
11 years ago

While esc_sql() is indeed the 1:1 replacement for mysqli_real_escape_string(), you should be preparing your queries whenever possible. Not preparing queries is about a decade behind best practices in web development.

#6 @harmr
11 years ago

  • Resolution set to worksforme
  • Status changed from reopened to closed

thanks for the clarification Nacin! I use $wpdb->prepare() wherever possible - anyway due to the design of my plugin there are a few cases (e.g. where input values can be 1,2,3 or ALL) where I need this replacement. Will check if I can reduce these cases in in future releases.

Now closing ticket for certain ;-)

Note: See TracTickets for help on using tickets.