Just for clarification, why is the query argument replacement done manually instead of using the db abstraction layer for the same?

function autopilot_save_target($target) {
  $values = array(db_escape_string($target['name']), 
		  db_escape_string($target['host']), 
		  db_escape_string($target['path']), 
		  db_escape_string($target['active']));
  $query = "INSERT INTO {autopilot_targets} (name, host, path, active) values('%s', '%s', '%s', '%s')";
  if ($target['tid']) {
    $query = "UPDATE {autopilot_targets} set name='%s', host='%s', path='%s', active='%s' where tid=%s";
    $values[] = $target['tid'];
  }

  $resultset = db_query(vsprintf($query, $values));  <---- why vsprintf instead of passing values into db_query?
}

Comments

IncrediblyKenzi’s picture

IncrediblyKenzi commented

Partially force of habit, and partially because depending on how it's called (either a new target or an existing) there's a different number of arguments passed into the query.

IncrediblyKenzi’s picture

IncrediblyKenzi commented

Oop.. on closer inspection, db_query auto-handles this.. Cancel the second argument.. just force of habit :).

robertdouglass’s picture

robertdouglass commented

Cool. While I'm nitpicking... Drupal SQL is much nicer (and will play better with alternate drivers like Oracle) if the SQL keywords are capitalized: SET, WHERE, INSERT, DELETE etc.

Looking forward to the full release of this stuff =)

robertdouglass’s picture

robertdouglass commented
Category: support » bug
Status: Postponed (maintainer needs more info) » Active
greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Title: Why do arg replacement yourself? » db_query arg replacement and variables

I also noticed that in _autopilot_admin_log_view_page($build_id)

277 : 	  	  	 $query = "SELECT al.bid as build_id, al.name as build_name, at.name as target_name, rid, from_unixtime(timestamp) as timestamp, status, output from {autopilot_log} al, {autopilot_targets} at where at.tid=al.tid AND al.bid=" . $build_id . " order by timestamp desc";
278 : 	  	  	$resultset = db_query($query);

By concatenating the build_id into the query you allow user submitted data (from the url, right?) into the query which is a potentially bad idea.

Granted only admins should be able to access this page, but this iss combined with account escalation could lead to further problems.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

autopilot_get_target also does query building in a manner that skips the internal Drupal query protections.

I think that all of the queries (and the text handling, especially since this module doesn't use t()) could use a review.

akalsey’s picture

akalsey commented
Status: Active » Closed (fixed)

These concerns are no longer an issue now that AP has been redone from the ground up.