👊🏻 😓 🔒 Analyse des schlechtesten Codes der Welt 👕 😼 👨🏼‍🔬

Es gibt eine italienische Facebook-Seite. Es heißt "Il Programmatore di Merda", was "Shitty Programmer" bedeutet. Ich liebe diese Seite.

Sie veröffentlichen oft ekelhaften Code und Memes über das Programmieren. Aber eines Tages habe ich dort etwas absolut Erstaunliches gesehen.

Dieser Code hat den Ehrentitel "Beste Arbeit" der Woche erhalten.

Ich habe beschlossen, diesen Code zu zerlegen, aber es gibt so viele falsche Dinge, dass es mir selbst schwer fällt, das erste Problem für die Analyse auszuwählen.

Wenn Sie ein Anfängerprogrammierer sind, hilft Ihnen mein Material zu verstehen, welche schrecklichen Fehler von denen gemacht wurden, die diesen Code geschrieben haben.

28 Zeilen Codefehler

Um es bequemer zu machen, geben wir diesen Code in den Editor ein:

<script>
function authenticateUser(username, password) {
  var accounts = apiService.sql(
    "SELECT * FROM users"
  );

  for (var i = 0; i < accounts.length; i++) {
    var account = accounts [i];
    if (account.username === username &&
        account.password === password)
    {
      return true;
    }
  }
  if ("true" === "true") {
    return false;
  }
}

$('#login').click(function() {
  var username = $("#username").val();
  var password = $("#password").val();

  var authenticated = authenticateUser(username, password);

  if (authenticated === true) {
    $.cookie('loggedin', 'yes', { expires: 1 });
  } else if (authenticated === false) {
    $("error_message").show(LogInFailed);
  }
});
</script>

Und ... Nun, wirklich - ich weiß nicht, wo ich anfangen soll, es zu analysieren. Aber du musst noch anfangen. Ich habe beschlossen, die Mängel dieses Codes in drei Kategorien zu unterteilen:

Sicherheitsprobleme.
Probleme mit grundlegenden Programmierkenntnissen.
Probleme bei der Codeformatierung.

Sicherheitsprobleme

Dieser Code wird wahrscheinlich auf dem Client ausgeführt. Dies wird durch die Tatsache angezeigt, dass es in ein Tag eingeschlossen ist <script>(und hier auch jQuery verwendet).

Aber versteh mich nicht falsch. Dieser Code würde genauso schrecklich aussehen, wenn er für den Server wäre. Die Ausführung auf dem Client bedeutet jedoch, dass jeder, der diesen Code lesen kann, Zugriff auf die darin verwendete Datenbank hat.

Werfen wir einen Blick auf die Funktion authenticateUser:

function authenticateUser(username, password) {
  var accounts = apiService.sql(
    "SELECT * FROM users"
  );

  for (var i = 0; i < accounts.length; i++) {
    var account = accounts [i];
    if (account.username === username &&
        account.password === password)
    {
      return true;
    }
  }
  if ("true" === "true") {
    return false;
  }
}

Aus der Analyse des Codes können wir schließen, dass es irgendwo ein Objekt apiServicegibt, das uns eine Methode gibt, .sqlmit der wir SQL-Abfragen an die Datenbank ausführen können. Dies bedeutet, dass Sie beim Öffnen der Entwicklerkonsole beim Anzeigen der Seite, die diesen Code enthält, alle Abfragen für die Datenbank ausführen können.

Sie können beispielsweise eine Abfrage wie folgt ausführen:

apiService.sql("show tables;");

Als Antwort wird eine vollständige Liste der Datenbanktabellen zurückgegeben. Und das alles über die projekteigene API. Aber was schon da ist, stellen wir uns vor, dass dies kein wirkliches Problem ist. Schauen wir uns das genauer an:

if (account.username === username &&
    account.password === password)

Dieser Code sagt mir, dass Passwörter ohne Hashing im Projekt gespeichert werden. Toller Zug! Dies bedeutet, dass ich den Debugger nehmen und die Passwörter der Projektbenutzer sehen kann. Ich glaube auch, dass ein großer Prozentsatz der Benutzer das gleiche Paar aus Benutzername und Passwort in sozialen Medien, E-Mail-Diensten, Bank-Apps und anderen ähnlichen Orten verwendet.

Ich sehe das Problem auch darin, wie Cookies in diesem Code gesetzt werden loggedin:

$.cookie('loggedin', 'yes', { expires: 1 });

Es stellt sich heraus, dass jQuery verwendet wird, um ein Cookie zu setzen, das der Webanwendung mitteilt, ob der Benutzer authentifiziert ist.

Denken Sie daran: Setzen Sie diese Cookies niemals mit JavaScript.

Wenn Sie diese Art von Informationen auf dem Client speichern müssen, werden hierfür am häufigsten Cookies verwendet. Ich kann zu einer solchen Idee nichts Schlechtes sagen. Das Setzen eines Cookies mit JavaScript bedeutet jedoch, dass das Attribut nicht konfiguriert werden kann httpOnly. Dies bedeutet wiederum, dass jedes böswillige Skript auf diese Cookies zugreifen kann.

Und ja, ich weiß, dass hier nur so etwas wie ein Schlüssel-Wert-Paar gespeichert ist.'loggedin': 'yes'Selbst wenn das Skript eines anderen so etwas liest, wird es nicht viel Schaden anrichten. Aber das ist trotzdem eine sehr schlechte Praxis.

Durch Öffnen der Chrome-Konsole kann ich außerdem jederzeit einen Befehl wie eingeben $.cookie('loggedin', 'yes', { expires: 1000000000000 });. Infolgedessen stellt sich heraus, dass ich mich für immer auf der Website angemeldet habe, auch ohne ein Konto zu haben.

Probleme mit grundlegenden Programmierkenntnissen

Wir können über ähnliche Probleme sprechen, die in diesem Code zu finden sind, und wir haben nicht viel Zeit.

Eine Funktion authenticateUserist also offensichtlich ein Beispiel für sehr schlecht geschriebenen Code. Es zeigt, dass der Person, die es geschrieben hat, einige Grundkenntnisse in der Programmierung fehlen.

function authenticateUser(username, password) {
  var accounts = apiService.sql(
    "SELECT * FROM users"
  );

  for (var i = 0; i < accounts.length; i++) {
    var account = accounts [i];
    if (account.username === username &&
        account.password === password)
    {
      return true;
    }
  }
  if ("true" === "true") {
    return false;
  }
}

Vielleicht reicht es aus, einen Benutzer mit einem bestimmten Namen und Passwort auszuwählen, anstatt eine vollständige Liste der Benutzer aus der Datenbank zu erhalten? Was passiert, wenn Millionen von Benutzern in dieser Datenbank gespeichert sind?

Ich habe bereits darüber gesprochen, aber ich werde mich wiederholen und die folgende Frage stellen: "Warum haben sie keine Passwörter in ihrer Datenbank?"

Schauen wir uns nun an, was die Funktion zurückgibt authenticateUser. Aus dem, was ich sehen kann, kann ich schließen, dass zwei Typargumente verwendet werden stringund ein einzelner Typwert zurückgegeben wird boolean.

Daher kann der folgende Code, obwohl er schrecklich geschrieben ist, nicht als bedeutungslos bezeichnet werden:

for (var i = 0; i < accounts.length; i++) {
  var account = accounts [i];
  if (account.username === username &&
      account.password === password)
  {
    return true;
  }
}

Wenn Sie es in eine gewöhnliche Sprache übersetzen, erhalten Sie ungefähr Folgendes: „Gibt es einen Benutzer mit dem Namen X und dem Kennwort Y? Wenn ja, geben Sie true zurück. "

Schauen wir uns nun diesen Code an:

if ("true" === "true") {
  return false;
}

Unsinn. Warum kehren Funktionen nicht einfach zurück false? Warum braucht sie eine Bedingung, die immer wahr ist?

Lassen Sie uns nun den folgenden Code analysieren:

$('#login').click(function() {
  var username = $("#username").val();
  var password = $("#password").val();

  var authenticated = authenticateUser(username, password);

  if (authenticated === true) {
    $.cookie('loggedin', 'yes', { expires: 1 });
  } else if (authenticated === false) {
    $("error_message").show(LogInFailed);
  }
});

Der jQuery-Teil dieses Codes sieht gut aus. Das Problem hierbei ist jedoch die Organisation der Arbeit mit Cookies loggedin.

Wir haben bereits gesagt, dass Sie auch ohne Konto einfach die Chrome-Konsole öffnen und den Befehl ausführen $.cookie('loggedin', 'yes', { expires: 1 });und sich für einen Tag authentifizieren lassen können.

Woher weiß diese Seite, wer der authentifizierte Benutzer ist? Vielleicht zeigt es nur etwas Besonderes an, das nur für diejenigen gedacht ist, die die Überprüfung von Benutzername und Passwort erfolgreich bestanden haben und keine persönlichen Daten anzeigen? Wir werden das nicht wissen.

Probleme bei der Codeformatierung

Das Styling ist wahrscheinlich das kleinste und unbedeutendste Problem in diesem Code. Es zeigt jedoch deutlich, dass die Person, die diesen Code erstellt hat, seine einzelnen Fragmente von irgendwoher kopiert und eingefügt hat.

Hier ist ein Beispiel für die Verwendung von doppelten Anführungszeichen beim Formatieren von Zeichenfolgen:

var username = $("#username").val();
var password = $("#password").val();

Und anderswo werden einfache Anführungszeichen verwendet:

$.cookie('loggedin', 'yes', { expires: 1 });

Dies mag unwichtig klingen, sagt uns jedoch, dass der Entwickler möglicherweise Code aus beispielsweise StackOverflow kopiert hat, ohne ihn gemäß dem im Projekt verwendeten Styleguide neu zu schreiben. Dies ist natürlich ein kleines Detail, aber es zeigt an, dass es dem Entwickler nicht wirklich wichtig ist, zu verstehen, wie der Code funktioniert. Dieser Entwickler braucht nur den Code, um irgendwie zu funktionieren.

Ich möchte hier meinen Standpunkt zu diesem Problem klarstellen. Ich suche jeden Tag bei Google nach Code-Inhalten, aber ich denke, es ist viel wichtiger zu verstehen, wie man beispielsweise ein Cookie setzt, als einen Code gedankenlos von irgendwoher kopieren zu lassen. Was ist, wenn das Programm aus irgendeinem Grund nicht mehr funktioniert? Wie kann ein Programmierer, der nicht versteht, was in seinem Code vor sich geht, einen Fehler finden?

Ergebnis

Ich bin absolut sicher, dass dieser Code eine Fälschung ist. Hier habe ich zum ersten Mal ein Beispiel für die synchrone Ausführung einer SQL-Abfrage gesehen:

var accounts = apiService.sql(
  "SELECT * FROM users"
);

Normalerweise werden solche Aufgaben wie folgt gelöst:

var accounts = apiService.sql("SELECT * FROM users", (err, res) => {
  console.log(err); // 
  console.log(res); //   
});

Sie werden auch so gelöst:

var accounts = await apiService.sql(
  "SELECT * FROM users"
);

Selbst wenn die Methode apiService.sqldas Ergebnis im synchronen Modus zurückgibt (was ich bezweifle), muss sie eine Verbindung zur Datenbank herstellen, eine Anforderung ausführen und das Ergebnis an den Aufrufpunkt senden. Und diese Operationen können (wie Sie vielleicht vermuten) nicht synchron ausgeführt werden.

Aber selbst wenn dies völlig realer Code ist, bin ich sicher, dass er von einem unerfahrenen Programmierer, Junior, geschrieben wurde. Ich bin mir sicher, dass ich in den ersten Wochen als Programmierer auch schrecklichen Code geschrieben habe (sorry: D).

Dies ist jedoch nicht die Schuld des Junior-Programmierers.

Angenommen, dies ist echter Code, der irgendwo ausgeführt wird. Einige Junioren würden alles daran setzen, dass dieser Code funktioniert. Dieser Entwickler muss noch lernen, wie er mit SQL-Abfragen, Cookies und allem anderen richtig umgeht. Und in diesem Licht ist es völlig normal.

Aber der Leiter dieses Programmierers muss die Arbeit kontrollieren, auf Fehler hinweisen und den Anfänger dazu bringen, seine Fehler zu verstehen. Dies wird dazu führen, dass solch schrecklicher Code es nicht in die Produktion schafft.

Ich weiß, dass es Unternehmen gibt, denen die Qualität des Codes, der in die Produktion geht, egal ist. Löst der Code das Problem? In diesem Fall wird es ohne Fragen bereitgestellt. Wird der Code von einem Junior geschrieben und nicht einmal von einem erfahrenen Programmierer getestet? In Produktion ist es!

Im Allgemeinen gibt es Trauer im Leben.

Aktualisierung ab 8. August 2020

Als ich diesen Artikel schrieb, glaubte ich, dass der Code, den ich analysierte, eine Fälschung war. Nachdem ich das Material auf Reddit besprochen hatte, erhielt ich einen Link zu diesem Thread. Wie sich herausstellte, wird dieser Code in einigen internen Systemen verwendet, die 1.500 Benutzer unterstützen. Also habe ich mich offensichtlich geirrt. Dies ist völlig realer Code.

Sind Sie in der Produktion auf ehrlich gesagt schlechte Codefragmente gestoßen, die voller allerlei Probleme sind?

Analyse des schlechtesten Codes der Welt