Soyez paranos avec les codes de suppression 14


L’informatique est généralement une discipline rationnelle (sauf bien entendu dans le cas de l’administration d’un système Windows), et pondre un bout de code pour un cas d’utilisation qui n’aura jamais lieu n’est pas vraiment la qualité première qu’on demande à un développeur.

Sauf. Sauf dans un cas.

Dans le cadre d’un code de suppression.

Au-delà de RM

Un code de suppression peut se cacher derrière bien plus qu’un remove :

  • Renommer ou déplacer une donnée ou un fichier.
  • Écrire par dessus ou à la suite du conteneur de données (tel qu’un fichier ou une table).
  • Mettre à jour la donnée elle-même.

C’est valable dans les scripts shell, mais également dans les scripts Python (ou tout langage qui vous sert à faire des opérations sur des systèmes critiques) : os.remove, os.rename, shutil.move, open('file', 'w').write('...'), etc.

Mais c’est aussi toutes les requêtes SQL en écriture.

Les codes à surveiller le plus sont ceux qui peuvent entrainer une suppression massive :

  • SELECT * UPDATE ou DROP table
  • rm -fr ou tout code manipulant un fichier matché avec un glob (*)
  • os.removes, shutil.rmtree, tout code qui manipule un fichier dans une boucle ou un appel récursif

Mais tous les autres restent importants si tant est que la donnée cible est critique (contenu de l’utilisateur, donnée non récupérable par un autre moyen ou alors de manière lente et couteuse). Même quand vous avez un backup dont vous êtes certains de la perfection – et franchement je me méfie toujours des backups – une restauration, c’est long, ça implique souvent une interruption de service ou au moins une dégradation, et de toute façon c’est du taff dont vous n’aviez vraiment pas besoin.

Pensez au petit fils de Murphy et François Perrin

Quand on a un petit site ou un logiciel en local de taille moyenne, on ne pense pas à ce genre de choses. Mais quand on travaille avec des grosses DB qui sont tout le temps sollicitées et des systèmes de fichiers qui sont blindés de Tera octets de contenus qui prennent des semaines à uploader sur un serveur (j’ai bien dis des SEMAINES), on ne veut pas que ça disparaisse.

La réplication et le load balancing peuvent vous sauver d’un plantage matériel ou d’un bug logique. Mais une corruption de données en cascade, ça se propage à tout un système. Et un RM massif, c’est exactement ça, surtout à l’heure des surcouches d’abstraction d’outils de haut niveau automatiques.

Alors vous allez me dire que les chances que ça arrive sont minimes. En fait votre code est propre, vous faites des checks normaux, il n’y a pas d’injections possibles, tout est sanitizé, bref, il n’y a pas de raison que ça chie.

Et vous n’auriez pas tort, si il n’y avait pas 2 importantes particularités du monde de l’informatique :

  • En informatique tout est multiplié par des milliards en quelque seconde. Une opération qui à très peu de chance d’arriver (une chance sur 10 milliards, c’est plus bas que de gagner au loto), peut donc survenir très facilement puisque l’opération est répétée un très grand nombre de fois en un temps très court.
  • Vous n’avez pas la main sur tout le système. Ce n’est pas sous votre contrôle. La complexité matérielle est logiciel est telle que vous n’en comprenez qu’un centième, et ce blob néo punk dont vous devez vous occuper est en interaction avec un environnement technique et humain gigantesque, parfaitement imprévisible, qui se renouvèle sans cesse.

Et comme la gravité des conséquences d’une merde de suppression massive est du genre “Code Rouge” dans “28 semaines plus tard”, vous ne pouvez pas vous permettre de ne pas blinder les suppressions.

Quelques scénarios à la con qui peuvent vous tuer sur un code aussi con que ça :

from fabric import cd, sudo
 
from django.conf import settings
 
 
...
 
for content in contents:
 
    if not content.published:
 
        with cd(settings.MEDIA_ROOT):
            sudo('rm -fr {}'.format(content.path))

A priori, rien de dangereux : vous contrôlez vos settings, vous contrôlez votre base de données, pas de malveillances, vous vérifiez que le contenu à expiré. Que peut-il arriver de mauvais ?

Plein de choses :

  • Vous avez merdé dans un merge Git octopussien entre votre fichier, celui de votre collègue qui vient de partir en retraite et celui de la démo de l’open coffee dans laquelle vous avez fait un quick fix après un effet démo bien humiliant. Il se glisse un early return est inséré dans la propriété content.published et comme Python retourne None par défaut, tous les contenu sont supprimés.
  • Vous êtes fatigués, vous avez oublié de changer le local_settings et MEDIA_ROOT n’a pas la même valeur. Vous effacez dans le répertoire de prod ou lieu du répertoire de test.
  • content.path est éditable dans l’admin privée de Django. En restaurant un formulaire avec une extension Firefox, il se glisse une étoile que vous ne remarquez pas dans le nom du contenu automatiquement utilisé pour créer le chemin de fichier, et vous faites un rm -fr avec un beau * dedans.
  • Vous discutez avec votre voisin et tapez du code dans le shell en prod pour faire une migration bénin après vous êtres dit que “c’est pas la peine de lancer south pour ça”. Grâce à la magie du cerveau humain vous tapez ce qu’il vous dit au lieu ce la commande que vous vouliez taper, et la moitié de votre try est excécuté, mais pas le code du except, par contre le finally a bien tout défoncé. Pardon, nettoyé.
  • Vous avez un stagiaire.

Le plus drôle, c’est quand plusieurs trucs improbables se cumulent, là on peut faire des combos qui déchirent bien toute l’infrastructure.

Les tests unitaires et la sanitization ne vous protègeront pas de la fatigue, l(inattention, l’éternuement en pleine session SSH, le chat qui saute sur le clavier… Souvenez-vous de la mouche dans Brazil.

Vous protéger contre vous même

Sur des codes comme ça, il faut donc prendre des mesures supplémentaires. Faire les vérifications qu’on ne fait jamais, de ce qui ne peut pas arriver.

 
import re
 
from fabric import cd, sudo
 
from django.conf import settings
 
 
...
 
for content in contents:
 
    # stylistiquement on ne fait jamais ça en Python, mais ici, on ne prend
    # pas le risque
    if content.expired == False: # None != False
 
        # on évite autant que possible les CD qui peuvent merder grave
        path = os.path.join(settings.MEDIA_ROOT, content.path)
 
        # on check strictement le chemin qu'on va dégommer avec une regex qui
        # ne laisse pas la place "/", "/bidule/*" et autre ".."
        # et qui réplique au plus prêt la structure de l'arbo qu'on vise
        if re.match(r'^regex_de_nazi$', path):
            sudo('rm -fr {}'.format(path))

Idéalement il faudrait avoir les droits nécessaires pour éviter le sudo. On peut même exceptionnellement casser le DRY en mettant MEDIA_ROOT en dur dans cette fonction. C’est à double tranchant, mais on ne vit pas dans un monde idéal.

Une regex qu’on a en production ressemble à ça:

re.match(r’^/[\w-]+/[\w-]+/\d/\d/\d/\d/\d/\d+$’, path):

Qui ne laissera passer qu’une suppression d’un truc qui à la forme “/1nom-de_dossier/una_autre-nom/8/1/9/3/5/890709″. Pas une arbo plus courte ou plus longue. Pas d’étoiles ou de guillemets. Pas un dossier qui ne contienne pas exactement cet enchainement de sous-dossiers numériques.

Et ce, malgré le fait que path est généré depuis un ID extrait d’un auto ID en DB (non accessible dans l’admin). Parce qu’on ne sait jamais. Et on ne peut pas se permettre que ça pète.

Pareil pour le SQL, n’hésitez pas à faire des requêtes en plus avant le DELETE pour voir si les données sont cohérentes. La plupart du temps on s’en branle si une suppression est lente.

Bref, les bonnes pratiques que sont de bien nettoyer ses données, de bien setter ses permissions et de ne pas tout lancer à 3 heures du matin après une session de debugging de 6 heures au Red Bull sont toujours des priorités. Cependant souvenez-vous que vous êtes humain, vous allez faire des conneries, les autres aussi.

Vous allez avoir un système imparfait (non, vous n’avez pas mis des checks et des conversions partout, le 100% ça ne marche que dans les labos de recherche, pas quand on a une dead line dans 15 jours).

Alors soyez paranos avec les codes de suppression.

Ce n’est pas une grosse partie du code, ça ne va pas tuer votre productivité ou plomber votre maintenance, mais contrairement aux autres snippets, vous n’aurez probablement pas de seconde chance pour le debug.

C’est l’histoire d’un mec…

Bon ok, de 3 mecs dans un bagnole : un matheux, un physicien et un informaticien.

A la sortie d’un virage, la voiture dérape et s’arrête deux roues dans le vide.

Les 3 compères sortent de la voiture, un peu choqués, et commencent à discuter :

Le mathématicien : “Il faudrait qu’on calcule la probabilité que ça nous arrive sur ce virage, avec cette voiture, dans ces conditions climatiques.”

Le physicien : “Il faudrait qu’on calcule le coefficient de friction et notre énergie cinétique pour mieux comprendre ce qui nous est arrivé.”

L’informaticien : “On reprend le virage avec la caisse pour voir si c’est reproductible ?”

14 thoughts on “Soyez paranos avec les codes de suppression

  • roro

    La prog, c’est des émotions… Je l’ai toujours dit.
    …mais là tu parle de commotions…

  • Morgotth

    +1 avec tout ce qui est dit. De toute manière toute info en entrée (BDD, fichiers, user input) est potentiellement dangereuse (chaine vide, etc).
    Et toutes les verifs du monde n’éviteront pas la sueur froide du premier deploiement en prod … Qui a dit qu’être un informaticien était de tout repos ?! ;)

    Je réagis quand même sur la regex_de_nazi qui n’est pas très “portable”. Pour éviter les * en bash, je préfère toujours mettre la chaine entre quotes :

    rm -fr 'dossier*'

    Car dossier* peut être un nom valide.
    Et sur le moment, pour check qu’on remonte pas dans l’arborescence, j’utiliserais peut être path.py et son relpathto.

  • Sam Post author

    Ouai mais t’évite pas le double quote insérer dans le nom, la faute de frappe et autres joyeuseté. Quand c’est possible, toujours réduire au plus petit dénominateur. De toute manière un nom de fichier pour être sans danger doit contenir uniquement des lettres ascii minuscules, des tirets et des underscores. Le reste, c’est bien sur le desktop, mais pas sur un serveur.

  • kontre

    Dans le second exemple, il faut remplacer des content.path par path tout court (désolé, la flemme de corriger…)

    Ça me fait stresser rien que de lire cet article ! D’autant que bossant dans un labo de recherche, le 100% on ne l’y trouve pas non plus… :( Y’a peut-être plus de chance dans un labo de maths fondamentales ?

  • Bertrand

    Pour la vérification du préfix, ce genre de check peut-être pas mal aussi:

    path = os.path.join(settings.MEDIA_ROOT, content.path)
    path = os.path.abspath(path)

    assert os.path.commonprefix([settings.MEDIA_ROOT, path]) == settings.MEDIA_ROOT

    ça évite les construction malignes de content.path (et complète l’utilisation de la regex si on veut limiter à un sous-ensemble de settings.MEDIA_ROOT).

  • policier.moustachu

    Les chasseurs de miel du NEPAL !!!!!!
    Toi aussi tu regardes arte le matin en te grattant les couilles?

  • KiKine

    aller question débile…
    “c’est pas la peine de lancer south pour ça”
    kesako ?

  • Sam Post author

    @Bertrand : pas mal, je connaissais pas.

    @Kikine : c’est un outil de migration de base de données pour Django. Tu modifies ta classe de modèle, et il applique la modification automatiquement sur la base de données. Ca permet aussi de faire des migrations de format de données, mais là c’est pas aussi automatisé.

  • Vincseize

    dangerous SQL missing:

    SELECT * UPDATE ou DROP table

    —> ALTER

    cheers

    PS: excellent WebSite, really

  • Sam Post author

    Oui d’ailleurs c’est probablement le plus dangereux quand on y pense, car il y a un risque de corruption de données qui peut se propager.

  • Frantret

    La complexité matérielle est logiciel > La complexité matérielle et logiciel

    Les tests unitaires et la sanitization en vous protégera pas de la fatigue, inattention, l’éternuement au dessus > Les tests unitaires et la sanitization ne vous protégeront pas de la fatigue, l’inattention, l’éternuement au-dessus

    Cependant souvenez vous que vous êtes humains > Cependant souvenez-vous que vous êtes humain

    vous n’avez pas mis des checks et des convertions partout > vous n’avez pas mis des checks et des conversions partout

    Merci pour ce post !

Leave a comment

Your email address will not be published. Required fields are marked *

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <pre> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>

Des questions Python sans rapport avec l'article ? Posez-les sur IndexError.