dedlfix: Links am Ende werden nicht angezeigt

Beitrag lesen

echo $begrüßung;

//Var-Sicherheit
if(isset($_GET['fid'])) { $fid = mysql_real_escape_string(htmlentities($_GET['fid'],ENT_QUOTES)); } else { echo 'Keine Forums-ID empfangen.'; exit; }
if(isset($_GET['tid'])) { $tid = mysql_real_escape_string(htmlentities($_GET['tid'],ENT_QUOTES)); } else { echo 'Keine Thread-ID empfangen.'; exit; }

Was ist denn das für ein Unfug mit den geschachtelten mysql_real_escape_string(htmlentities())? Du behandelst hier in einem Aufwasch Werte für zwei völlig verschiedenen Kontexte.

//Lesen aus DB
echo '<a href="newanswer_form.php?fid='.$fid.'&tid='.$tid.'">Neue Antwort</a><br><br>';

Abgesehen vom für diese Stelle unpassenden Kommentar, hast du hier einen der beiden Kontexte, nämlich HTML. Für den ist eine Behandlung gemäß MySQL-Strings-Kontext wenig sinnvoll.

//Antworten
$res = mysql_query("select * from ".$tableanswers." where fid=".$fid." AND tid=".$tid);

Und hier ist der zweite Kontext. Für den wirkt sich die HTML-Behandlung im Allgemeinen eher störend aus, lassen sich doch keine sinnvollen Stringbearbeitungen mehr durchführen, wenn solcherart behandelten Werte in den Tabellen stehen.

Wenn fid und tid Zahlen sein sollen, dann wäre eher eine Überprüfung auf Zahlen sinnvoll, beziehungsweise mit intval() eine erzwungene Konvertierung zu einer Zahl.

Außerdem schützt dich deine obige Behandlung im Zusammenhang mit diesem Statement nicht die Bohne vor SQL-Injection. Angenommen der Wert von $tid wäre »42 OR 1«. Weder mysql_real_escape_string() noch htmlentities() erwirkt eine Änderung. mysql_real_escape_string() ist für Strings gedacht, also Werte, die im Statement zwischen "" oder '' zu stehen kommen sollen. Es ergibt sich damit ein Statement

... where fid=23 AND tid=42 OR 1

Gemäß Operator-Rangfolge findet zuerst die AND-Verkknüpfung statt, dann die mit OR. Zur Verdeutlichung mal mit Klammern geschrieben ergäbe das

... where (fid=23 AND tid=42) OR 1

Die beiden ersten Einschränkungen werden durch die Verknüpfung mit dem OR außer Kraft gesetzt.

Vielleicht ist diese Stelle unkritisch, ergibt das doch "nur" eine Ausgabe aller Antworten, doch wenn du das auch so notiert hast, wenn es um beispielsweise Administrator-Funktionen geht, ...

Glücklicherweise schützt sich MySQL selbst vor mehrfachen Statements, so dass sich kein

; DELETE FROM ...

unterschieben lässt, doch auch ein SELECT-Statement lässt sich zur Abfrage sensibler Daten erweitern

SELECT user, text, createdToShow FROM answers WHERE fid = 23 AND tid=42
  UNION SELECT user, passwort AS text, 0 AS createdToShow FROM user

Damit hängt es den Inhalt der Tabelle user an die Ergebnismenge und so gelangen alle Passwörter (oder was auch immer davon in der Tabelle) gelandet ist zur Anzeige.

Deine SQL-Injection-Lücke in diesem Fall kannst du schließen indem du die Eingabewerte für fid und tid mittels intval() garantiert in Zahlen umwandelst. Im Fehlerfall (also bei Nicht-Zahlen-Strings) gibt die Funktion 0 zurück. Alternativ müsstest du den mit mysql_real_escape_string() behandelten Stringwert auch in String-Begrenzer einrahmen ("" oder '').

//ausgeben
while($row = mysql_fetch_array($res) or die(mysql_error())) {
$text = nl2br($row["text"]); //Zeilenumbrüche
echo '<fieldset style="background: gray; color: white;">
        <legend><font color="chartreuse"><b>'.$row["user"].'</b> schrieb am '.$row["createdToShow"].':</font></legend>'.
        $text.'</fieldset>';
}

An dieser Stelle übergibst du wieder Werte in den HTML-Kontext. Erst dabei ist eine Behandlung gemäß der HTML-Regeln sinnvoll, nicht (wie ich vermute) bereits beim Eintragen in die DB.

Die Sache mit dem die() ist ja bereits geklärt. Es fehlt noch eine Auswertung des Rückgabewertes von mysql_query() inklusive angemesser Reaktion. Das Script sterben zu lassen und dem Anwender den mysql_error()-Text zu präsentieren ist meist nicht die feine englische Art, geschweige denn, dass die dabei veröffentlichten Interna jemand anderes zu sehen bekommen sollte als der Entwickler. Auch der am Anfang zu sehende Abbruch mit »exit;« ist nicht besser.

Vielleicht solltest du dir mal grundlegende Gedanken machen, wie du Scripte besser strukturieren kannst, so dass Eingabe, Verarbeitung und Ausgabe getrennte Bestandteile werden (EVA-Prinzip). exit und die() sind dann "verbotene" Dinge. Ein Script sollte nach einer vollständig ausgegebenen Seite enden. Fehlerzustände, so sie dem Anwender mitgeteilt werden sollen, finden darin ihren Platz und zerhackstücken nicht das Layout durch plötzlichen Abbruch. Außerdem sollte der Text einer sein, mit dem der Anwender etwas anfangen kann. Wenn beispielsweise die Datenbank defekt ist, kann er nichts dafür. Die genaue Fehlerursache ist nicht für seine Augen bestimmt, nur für die des Administrators.

echo "$verabschiedung $name";