db_query arg replacement and variables
robertDouglass - August 21, 2007 - 10:01
| Project: | AutoPilot |
| Version: | 5.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Jump to:
Description
Just for clarification, why is the query argument replacement done manually instead of using the db abstraction layer for the same?
<?php
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?
}
?>
#1
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.
#2
Oop.. on closer inspection, db_query auto-handles this.. Cancel the second argument.. just force of habit :).
#3
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 =)
#4
#5
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.
#6
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.
#7
These concerns are no longer an issue now that AP has been redone from the ground up.