nicht angemeldet
Moin!
Wenn du deinen Code hier so freundlich reinstellst, will ich die Gelegenheit mal nutzen, um ein paar Kommentare abzugeben.
error_reporting(0);
Es ist nie eine gute Idee, das Error-Reporting abzuschalten! Es ist hingegen immer eine gute Idee, das eingeschaltete Error-Reporting in ein passendes Logfile zu leiten, und auf produktiven Systemen dem Besucher keinerlei PHP-Fehlermeldung zu zeigen (display_errors = off).
if ( count($_GET) == 3 ) {
if ( isset($_GET["v"],$_GET["s"],$_GET["h"]) ) {
Das ist eine eher unkonventionelle Art, eine Sammlung von Eingabeparametern zu validieren, aber sie funktioniert. Verbesserungswürdig ist die überflüssige zweite IF-Ebene an dieser Stelle. Tatsächlich weitergearbeitet werden soll ja nur, wenn beiden Bedingungen gleichzeitig zutreffen, die bessere Vorgehensweise wäre an dieser Stelle eine logische Verknüpfung mit "&&" und eben nur ein IF.
function unr_gl() { foreach (func_get_args() as $name) { foreach ($GLOBALS[$name] as $key=>$value){ if (isset($GLOBALS[$key])) unset($GLOBALS[$key]); } } }
Erstens: Funktionsdefinitionen gehören irgendwo an zentraler Stelle gesammelt und dann einmalig definiert, nicht mitten in den Code eingestreut. Ansonsten könnte es passieren, dass der Code noch ein zweites Mal durchlaufen wird und er dabei die Funktion ein zweites Mal versucht zu definieren - das wäre dann ein PHP-Fehler, den man auf diese Weise leicht vermeiden könnte.
Mir gefällt an der Funktion nicht, dass sie unendlich viele Parameter erlaubt, ohne einen einzigen davon aufzulisten. Ich hätte die Funktion mit genau einem Parameter definiert, und diesen dann als Array formuliert. Dann wird beim Betrachten der Funktion sofort klar, dass man da ein Array übergeben soll.
Abgesehen davon ist die Funktion aber komplett überflüssig.
unr_gl('_POST', '_COOKIE', '_REQUEST', '_SERVER', '_ENV', '_FILES');
Keine der hier angeführten Variablen kann auch nur den geringsten schädlichen Einfluss ausüben, wenn man nicht auf sie zugreift.
Function remote_status ($get_v, $get_s, $get_h) { $get_h = strtolower($get_h); if ( $get_v == 'A') { $insert = "update table set ".$get_v." = ".$get_v." + '".$get_s."' where col = '".$get_h."';"; } else { $insert = "update table set ".$get_v." = '".$get_s."' where col = '".$get_h."';"; } mysql_query($insert); }
Über die Definition von Funktionen mitten im Code siehe oben.
Dass diese Funktion das Escaping nicht durchführt, ist ein eklatanter Mangel. Dass die Funktion im Zweifel das mysql_query() auf der Mysql-Standarddatenbank ausführt, weil die zu benutzende Mysql-Connection nicht als Parameter übergeben wird, ist ebenfalls eine Unschönheit, die die Fehlersuche im Zweifel extrem verkompliziert.
$link = mysql_connect ("localhost", "user", "passwd"); mysql_select_db("database"); // $get_v = mysql_real_escape_string(trim($_GET["v"])); $get_s = mysql_real_escape_string(trim($_GET["s"])); $get_h = mysql_real_escape_string(trim($_GET["h"]));
Das Escaping gehört in die Funktion!
Zu beachten ist auch, dass mysql_real_escape_string() eine existierende Mysql-Connection benötigt und diese im Bedarfsfall selbst herstellt - mit Standardparametern, wenn vorher kein mysql_connect() ausgeführt wurde.
$get_v_chk_array = array('A','B','C','D','E','F','G','H'); // if ( in_array($get_v, $get_v_chk_array) ) { if ( strlen($get_s) == 1 && preg_match('[w|x|y|z]', $get_s) == 1 ) {
Validierung ist ja nicht verkehrt, aber warum für dasselbe zwei unterschiedliche Methoden verwenden?
$get_v darf einen der in $get_v_chk_array enthaltenen Werte annehmen (Großbuchstaben von A bis H).
$get_s darf einen der im preg_match angegebenen Buchstaben enthalten, also w, x, y oder z.
Das ist zweimal dasselbe Schema: Eine Variable darf genau einen Wert aus einer Liste von Werten annehmen. Warum wird das unterschiedlich gecoded?
Warum wird für die zweite Variable nicht komplett ein regulärer Ausdruck verwendet, der die strlen-Prüfung überflüssig macht, indem mit Regex-Ankern (^ und $) gearbeitet wird?
Außerdem gilt für das verschachteln von IFs das weiter oben gesagte: Und-Verknüpfen der beiden Bedingungen reduziert den Code um eine Einrückungsebene!
remote_status($get_v, $get_s, $get_h); unset($insert,$_GET["v"]);
Das unset() ist überflüssig. Erstens: Es gibt an dieser Stelle keine Variable $insert - die ist schon gelöscht worden, als die Funktion beendet wurde.
Und das Löschen von $_GET['v'] ist ebenso überflüssig wie das Löschen der weiteren superglobalen Variablen weiter oben.
} else exit; } else exit; mysql_close($link);
Die Datenbank-Connection wird nicht geschlossen, wenn eines der oben angegebenen IFs fehlschlägt. ;)
Nein, doch, natürlich wird die Connection geschlossen - dann, wenn alle Connections automatisch geschlossen werden, nämlich am Skriptende. Das explizite Schließen der Connection ist überflüssig. Ich würde darauf verzichten.
} else exit;} else exit;
Es ist außerdem überlegenswert, sich hinsichtlich der diversen validierenden IF-Abfragen mal zu überlegen, ob "fail fast" nicht die bessere Taktik wäre. Anstatt "if (alles ist gut) { mach; } else {exit}" würde das bedeuten, die Abfrage logisch umzudrehen und im if-Code das Skript zu beenden. Wenn es nicht beendet wurde, ist alles in Ordnung, und man kann weitermachen: "if (!alles ist gut) {exit;} mach;"
Das spart in der Regel lange IF-Konstrukte, an deren Ende noch dieses kleine Exit-Anhängsel dransteht. Der relevante Code, der die Arbeit leistet, ist nicht so tief eingerückt - durchaus ein Vorteil.
Außerdem sollte man "teure" Dinge wie das Herstellen einer DB-Verbindung möglichst spät und nur dann erledigen, wenn dieser Code tatsächlich benötigt wird, weil tatsächlich etwas in die Datenbank hineingeschrieben werden muss - und das nicht kurz vorher aufgrund einer scheiternden Prüfung noch abgeblasen wird.
Also zusammengefasst wäre das:
~~~php
function remote_status ($connection, $get_v, $get_s, $get_h) {
$get_h = strtolower($get_h);
if ( $get_v == 'A') {
$insert = "update table set ".mysql_real_escape_string($get_v, $connection)." = ".mysql_real_escape_string($get_v, $connection)." + '".mysql_real_escape_string($get_s, $connection)."' where col = '".mysql_real_escape_string($get_h, $connection)."';";
} else {
$insert = "update table set ".mysql_real_escape_string($get_v, $connection)." = '".mysql_real_escape_string($get_s, $connection)."' where col = '".mysql_real_escape_string($get_h, $connection)."';";
}
mysql_query($insert, $connection);
}
if (! (count($_GET) == 3 && isset($_GET["v"], $_GET["s"], $_GET["h"]))) {
exit();
}
$get_v = trim($_GET["v"]);
$get_s = trim($_GET["s"]);
$get_h = trim($_GET["h"]);
if (! in_array($get_v, array('A', 'B', 'C', 'D', 'E', 'F', 'G', 'H'))) {
exit();
}
if (! in_array($get_s, array('w', 'x', 'y', 'z'))) {
exit();
}
$link = mysql_connect ("localhost", "user", "passwd");
mysql_select_db("database", $link);
remote_status($link, $get_v, $get_s, $get_h);
Das ist aber nur ein Zwischenschritt. Denn wie zu sehen ist, wird der SQL-String nicht mit reinen Daten befüllt. mysql_real_escape_string() ist aber nur dafür da, Daten zu escapen, nicht Tabellenfeldnamen!
Andererseits darf man nicht beliebige Feldnamen verwenden, diese Prüfung gehört aber mit zur Validierung der Eingabeparameter in der Funktion remote_status().
Also:
function remote_status ($connection, $get_v, $get_s, $get_h) {
if (! in_array($get_v, array('A', 'B', 'C', 'D', 'E', 'F', 'G', 'H'))) {
return false;
}
if (! in_array($get_s, array('w', 'x', 'y', 'z'))) {
return false;
}
$get_h = strtolower($get_h);
if ($get_v == 'A') {
$insert = "update table set ".$get_v." = ".$get_v." + '".mysql_real_escape_string($get_s, $connection)."' where col = '".mysql_real_escape_string($get_h, $connection)."';";
} else {
$insert = "update table set ".$get_v." = '".mysql_real_escape_string($get_s, $connection)."' where col = '".mysql_real_escape_string($get_h, $connection)."';";
}
return mysql_query($insert, $connection);
}
if (! (count($_GET) == 3 && isset($_GET["v"], $_GET["s"], $_GET["h"]))) {
exit();
}
$get_v = trim($_GET["v"]);
$get_s = trim($_GET["s"]);
$get_h = trim($_GET["h"]);
$link = mysql_connect ("localhost", "user", "passwd");
mysql_select_db("database", $link);
remote_status($link, $get_v, $get_s, $get_h);
Ich habe das exit() in den beiden IFs durch "return false" ersetzt - das beendet die Funktion an dieser Stelle. Außerdem wird das Ergebnis des Aufrufs von mysql_query() jetzt als Funktionsergebnis zurückgegeben - aufgrund der UPDATE-Statements kann das nur true oder false sein. Am Ende gibt die Funktion also false zurück, wenn irgendwas nicht geklappt hat, oder true, wenn das UPDATE funktioniert hat.
- Sven Rautenberg
MudGuard