Das Skript zum Montag - Blocklist.de befragen

[1unitedpower]

Blocklist.de bietet eine nette Möglichkeit, von anderen bereits erkannte Angreifer via DNS über deren IP zu identifizieren.

Ich kenne den Dienst nicht, aber es gibt scheinbar auch eine bereits eine HTTP-API dafür. Mir ist nicht so recht klar, welchen Vorteil dein Skript demgegenüber bietet.

Ein paar stilistische Anmerkungen zu deinem Skript hätte ich auch noch:

ini_set('display_errors', 0);

Wieso überschreibst du diese Server-Einstellung? Auf Produktiv-Servern kann man damit rechnen, dass diese Eigenschaft bereits deaktivert ist. Auf Entwicklungs-Servern möchte man Fehler bewusst anzeigen lassen, da erschwert diese Überschreibung nur die Fehlersuche.

if ( empty($_REQUEST['method'])) {
    $_REQUEST['method'] = 'HOST';
}

$_REQUEST ist nicht der Ort um benutzerdefinierte Werte darin zu speichern. Das Array hat eine fest definierte Semantik, die im PHP-Handbuch nachgelesen werden kann. Wenn du dort selber reinschreibst, missachtest du die Semantik und machst deinen Code unverhersehbar. Ich erinnere mich daran, dass diese Praxis auch in diesem Forum schon Nutzer verunsichert hat. Variablen sind keine Mangelware, das einzige Limit für Variablen ist deine Vorstellungskraft beim Ausdenken neuer Bezeichner.

if (
    $_REQUEST['method'] != 'PHP'
    && $_REQUEST['method'] != 'NSLOOKUP'
    && $_REQUEST['method'] != 'HOST'
    ) {
    $_REQUEST['method'] = 'HOST';
}

Die Stelle musste ich 3 Mal lesen, um die Klammernpaare einander zuzuordnen. Dabei sind Codeblöcke bei konventioneller Einrückung etwas, das man schon beim Überfliegen erkennen kann und können sollte. Noch schwieriger wird es, weil du selbst innerhalb dieses Skriptes nicht konsistent vorgehst. Allgemein empfiehlt es sich einen Styleguide zu befolgen.

if ( $return['error_type'] == 'FATAL' ) {
    header('Status: 500 ');
    echo $return['error_string'];
    exit;
}

exit; macht es ebenfalls schwer die Struktur von Quelltexten zu erfassen. Es ist verwirrend, dass der folgende Code abhängig davon ausgeführt wird, ob dieser if-Block betreten wird. Und das obwohl sich die Codeblöcke auf gleicher Höhe befinden. Ein else-Block signalisiert schon beim überfliegen, dass hier eine Abhängigkeit existiert.

    $return['Hint']             = 'Dies ist ein Test! Um eine stabile API zu erhalten fragen Sie beim Autor nach (www.fastix.org)';

Diser Kommentar verunsichert nur. Entweder würde ich hier direkt dokumentieren, worauf du auch hinaus wolltest, oder ich würde den Kommentar gänzlich sparen. Anhand dieses Textes kann ich nicht Mal erkennen, ob es hier um eine kritische Sicherheitslücke geht oder ob die Kontaktaufnahme zu einer bezahlten Service-Leistung führen soll.

    if ( 'NSLOOKUP' == strtoupper($method) && ! is_file(nslookup_exe) ) {
        $method='HOST';
        $return['error_type'] = 'NOTICE';
        $return['error_number'] += 2;
        $return['error_string'] = "'nslookup' nicht verfügbar oder falsch konfiguriert. Es wird versucht 'host' zu benutzen";
        trigger_error($return['error_string'], E_USER_NOTICE);
    }

Du hast vier Codeblöcke dieser Art. Nur der erste Codeblock behandelt dabei einen Fehler, der durch eine Fehlbedienung der Funktion ausgelöst wird. Die anderen Fehlermeldungen beziehen sich auf die Ausführungsumgebung: Existiert dieses Programm? Existiert diese Funktion? Diese Blöcke gehören nicht in diese Funktion, weil sie Fehler aufdecken, die u.U. schon viel früher entstanden sind und weil sie keine Sinneinheit mit der Funktion bilden. Darüberhinaus ist viel Code-Duplikation ein sicheres Zeichen dafür, dass man gemeinsame Code-Fragmente in eine wiederverwendbare eigene Einheit ausgliedern sollte.

    $PATTERN_SERVICE =              '/Service: (.*), Last-Attack/'  ;

Du hast dir sicher etwas dabei gedacht, diese und andere Variablen groß zu schreiben. Für mich als unbeteiligten Leser erschließt sich mir aber nicht, was dahinter stecken könnte. Dies ist ebenfalls ein Thema, das von Styleguides abgedeckt wird und damit zum Konsens beiträgt.