🍻 🙅🏾 🚶🏻 Analyser le pire morceau de code au monde 👌🏾 👨🏼‍🔧 🏝️

Il existe une page Facebook italienne. Il s'appelle «Il Programmatore di Merda», ce qui signifie «Shitty Programmer». J'adore cette page.

Ils publient souvent des morceaux de code dégoûtant et des mèmes sur la programmation. Mais un jour, j'ai vu quelque chose d'absolument incroyable là-bas.

Ce morceau de code a remporté le titre honorifique de «meilleur travail» de la semaine.

J'ai décidé de démonter ce code, mais il y a tellement de choses fausses qu'il m'est même difficile de choisir le premier problème à analyser.

Si vous êtes un programmeur débutant, mon matériel vous aidera à comprendre quelles terribles erreurs ont été commises par ceux qui ont écrit ce code.

28 lignes d'erreurs de code

Pour le rendre plus pratique, tapons ce code dans l'éditeur:

<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>

Et ... Eh bien, vraiment - je ne sais pas par où commencer à l'analyser. Mais vous devez encore commencer. J'ai décidé de décomposer les lacunes de ce code en trois catégories:

Les problèmes de sécurité.
Problèmes avec les connaissances de base en programmation.
Problèmes de formatage du code.

Les problèmes de sécurité

Ce code est probablement en cours d'exécution sur le client. Ceci est indiqué par le fait qu'il est enveloppé dans une balise <script>(et, en plus, jQuery est utilisé ici).

Mais ne vous méprenez pas. Ce code aurait l'air tout aussi horrible s'il s'agissait du serveur. Mais l'exécuter sur le client signifie que toute personne capable de lire ce code aura accès à la base de données qui y est utilisée.

Jetons un coup d'œil à la fonction 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;
  }
}

De l'analyse de son code, nous pouvons conclure qu'il existe quelque part un objet apiServicequi nous donne une méthode .sqlavec laquelle nous pouvons exécuter des requêtes SQL dans la base de données. Cela signifie que si vous ouvrez la console développeur tout en affichant la page qui contient ce code, vous pouvez exécuter toutes les requêtes sur la base de données.

Par exemple, vous pouvez exécuter une requête comme celle-ci:

apiService.sql("show tables;");

En réponse, une liste complète des tables de la base de données sera renvoyée. Et tout cela se fait en utilisant la propre API du projet. Mais, ce qui est déjà là, imaginons que ce n'est pas un vrai problème. Jetons un meilleur regard à ceci:

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

Ce code m'indique que les mots de passe sont stockés dans le projet sans hachage. Grand mouvement! Cela signifie que je peux prendre le débogueur et voir les mots de passe des utilisateurs du projet. Je pense également qu'un grand pourcentage d'utilisateurs utilisent la même paire nom d'utilisateur / mot de passe sur les réseaux sociaux, les services de messagerie, les applications bancaires et d'autres endroits similaires.

Je vois également le problème dans la façon dont les cookies sont définis dans ce code loggedin:

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

Il s'avère qu'il utilise jQuery pour définir un cookie qui indique à l'application Web si l'utilisateur est authentifié.

N'oubliez pas: ne définissez jamais ces cookies en utilisant JavaScript.

Si vous avez besoin de stocker ce type d'informations sur le client, les cookies sont le plus souvent utilisés à cette fin. Je ne peux rien dire de mal à propos d'une telle idée. Mais la définition d'un cookie à l'aide de JavaScript signifie que l'attribut ne peut pas être configuré httpOnly. Cela signifie à son tour que tout script malveillant peut accéder à ces cookies.

Et oui, je sais que seul quelque chose comme une paire clé: valeur est stocké ici,'loggedin': 'yes', donc même si le script de quelqu'un d'autre lit quelque chose comme ça, il n'y aura pas beaucoup de mal. Mais c'est de toute façon une très mauvaise pratique.

De plus, lorsque j'ouvre la console Chrome, je peux toujours entrer une commande comme $.cookie('loggedin', 'yes', { expires: 1000000000000 });. En conséquence, il s'avère que je me suis connecté au site pour toujours, même sans avoir de compte dessus.

Problèmes avec les connaissances de base en programmation

Nous pouvons parler et parler de problèmes similaires qui peuvent être trouvés dans ce morceau de code, et nous n'avons pas beaucoup de temps.

Donc, évidemment, une fonction authenticateUserest un exemple de code très mal écrit. Cela démontre que la personne qui l'a écrit n'a pas de connaissances de base en programmation.

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;
  }
}

Peut-être qu'au lieu d'obtenir une liste complète des utilisateurs de la base de données, il suffit de sélectionner un utilisateur avec un nom et un mot de passe donnés? Que se passe-t-il si des millions d'utilisateurs sont stockés dans cette base de données?

J'en ai déjà parlé, mais je vais me répéter en posant la question suivante: "Pourquoi ne hachent-ils pas les mots de passe dans leur base de données?"

Voyons maintenant ce que la fonction renvoie authenticateUser. D'après ce que je peux voir, je peux conclure qu'il prend deux arguments de type stringet renvoie une seule valeur de type boolean.

Par conséquent, le morceau de code suivant, bien qu'il soit écrit horriblement, ne peut pas être qualifié de dénué de sens:

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

Si vous le traduisez dans un langage ordinaire, vous obtenez quelque chose comme ceci: «Y a-t-il un utilisateur avec le nom X et avec le mot de passe Y? Si oui, retournez vrai. "

Jetons maintenant un coup d'œil à ce morceau de code:

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

Absurdité. Pourquoi les fonctions ne reviennent-elles pas simplement false? Pourquoi a-t-elle besoin d'une condition qui est toujours vraie?

Analysons maintenant le code suivant:

$('#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);
  }
});

La partie jQuery de ce code semble correcte. Mais le problème ici est l'organisation du travail avec les cookies loggedin.

Nous avons déjà dit que, même sans compte, vous pouvez simplement ouvrir la console Chrome et exécuter la commande $.cookie('loggedin', 'yes', { expires: 1 });et être authentifié pendant un jour.

Comment cette page sait-elle qui est l'utilisateur authentifié? Peut-être qu'il affiche simplement quelque chose de spécial, destiné uniquement à ceux qui réussissent la vérification du nom d'utilisateur et du mot de passe et n'affiche aucune donnée personnelle? Nous ne le saurons pas.

Problèmes de formatage du code

Le style est probablement le problème le plus petit et le plus insignifiant de ce code. Mais cela indique clairement que la personne qui a créé ce code a copié et collé ses fragments individuels de quelque part.

Voici un exemple d'utilisation de guillemets doubles lors du formatage de chaînes:

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

Et ailleurs, des guillemets simples sont utilisés:

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

Cela peut sembler sans importance, mais cela nous dit en fait que le développeur a peut-être copié du code à partir de, par exemple, StackOverflow, sans même le réécrire, en suivant le guide de style utilisé dans le projet. Bien sûr, c'est un petit détail, mais cela indique que le développeur ne se soucie pas vraiment de comprendre comment le code fonctionne. Ce développeur a juste besoin du code pour fonctionner d'une manière ou d'une autre.

Je voudrais clarifier ici mon point de vue sur ce problème. Je recherche tous les jours sur Google des trucs liés au code, mais je pense qu'il est beaucoup plus important de comprendre comment définir un cookie, par exemple, plutôt que de faire un morceau de code copié sans réfléchir à partir d'un travail. Et si, pour une raison quelconque, le programme cesse de fonctionner? Comment un programmeur qui ne comprend pas ce qui se passe dans son code peut-il trouver une erreur?

Résultat

Je suis absolument sûr que ce morceau de code est un faux. Ici, j'ai vu pour la première fois un exemple d'exécution synchrone d'une requête SQL:

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

Habituellement, ces tâches sont résolues comme suit:

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

Ils sont également résolus comme ceci:

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

Même si la méthode apiService.sqlrenvoie le résultat en mode synchrone (ce dont je doute), elle doit ouvrir une connexion à la base de données, exécuter une requête, envoyer le résultat au point d'appel. Et ces opérations (comme vous pouvez le deviner) ne peuvent pas être effectuées de manière synchrone.

Mais même s'il s'agit d'un code complètement réel, je suis sûr qu'il a été écrit par un programmeur novice, junior. Je suis sûr que lorsque j'ai travaillé en tant que programmeur pendant les premières semaines, j'ai également écrit un code terrible (désolé: D).

Mais ce n'est pas la faute du programmeur junior.

Disons que c'est du vrai code qui s'exécute quelque part. Certains juniors feraient tout pour que ce code fonctionne. Ce développeur doit encore apprendre à gérer correctement les requêtes SQL, les cookies et tout le reste. Et dans cette optique, c'est tout à fait normal.

Mais le chef de ce programmeur doit contrôler le travail, signaler les erreurs et amener le débutant à comprendre ses erreurs. Cela conduira au fait qu'un code aussi terrible ne parvient pas à la production.

Je sais avec certitude qu'il y a des entreprises qui ne se soucient pas de la qualité du code qui entre en production. Le code résout-il le problème? Si tel est le cas, il est déployé sans aucune question. Le code est-il écrit par un junior et même pas testé par un programmeur plus expérimenté? En production il!

En général, il y a des chagrins dans la vie.

Mise à jour au 8 août 2020

Quand j'écrivais cet article, je pensais que le code que j'analysais était un faux. Mais après avoir discuté du matériel sur Reddit, on m'a donné un lien vers ce fil. Il s'est avéré que ce code est utilisé dans certains systèmes internes prenant en charge 1 500 utilisateurs. J'avais donc manifestement tort. C'est du code complètement réel.

Avez-vous rencontré des fragments de code franchement mauvais en production, pleins de toutes sortes de problèmes?

Analyser le pire morceau de code au monde