Audit de sécurité — merge requests (2026-07-29, suite)

Périmètre : uniquement la fonctionnalité merge requests (internal/store/merge_requests.go, la plomberie git ajoutée dans internal/gitexec pour les branches/fusions, admin.Ops/adminrpc correspondants, cmd/gitfed-web/handlers_merge_requests.go).
Nature : revue de code adversariale, avec exploitation réelle en bac à sable (pas seulement une lecture de code) — chaque faille listée ci-dessous a été effectivement déclenchée contre une instance locale avant d'être corrigée, pas seulement supposée théorique.
État : les correctifs identifiés ci-dessous sont déjà appliqués dans ce commit, avec un test de régression qui reproduit l'exploit exact pour C1 (internal/gitexec/gitexec_test.go) et pour L3 (internal/admin/admin_test.go).

1. Synthèse

ID	Sévérité	Titre	Statut
C1	🔴 Critical	Injection d'argument via les hash de commit et noms de branche	✅ corrigé
L3	🟡 Low	Fermer une MR déjà fusionnée écrase silencieusement son statut	✅ corrigé
L4	🟡 Low	Titre/description/commentaire non plafonnés côté serveur	✅ corrigé

C1 est la découverte importante de cet audit — un contournement complet du modèle de menace habituel (aucune authentification requise, aucun accès en écriture nécessaire) menant à une écriture de fichier arbitraire, avec un chemin réaliste vers l'exécution de code. Le reste de la surface merge requests (autorisation, isolation git, gestion des conflits/races) a résisté à l'examen — voir §3.

2. Détail

🔴 C1 — Injection d'argument via les hash de commit et noms de branche

Fichiers : internal/gitexec/gitexec.go (ShowCommit, CommitDiff, BranchDiff, branchFiles, commitFiles, withScratchWorktree, CheckMergeable, MergeBranches)

Le problème. Chacune de ces fonctions passe un hash de commit ou un nom de branche comme dernier argument d'une commande git : git show --format=... <hash>, git diff --no-color <target>...<source>, git worktree add --detach <dir> <base>, git merge --no-ff -m <msg> <source>. Le porcelain git (git branch, git checkout -b) refuse côté client tout nom de ref commençant par - (check-ref-format) — mais ce n'est qu'une validation client. Un git update-ref direct sur le dépôt bare, ou un push utilisant directement une refspec, crée la ref sans broncher. Une fois cette ref créée, elle est renvoyée par ListBranches exactement comme n'importe quelle autre — rien ne la distingue dans l'interface.

Preuve de faisabilité (réellement exécutée, pas seulement décrite).

# Contourne check-ref-format en écrivant la ref directement sur le bare repo :
git --git-dir=demo.git update-ref 'refs/heads/--output=/tmp/poc.txt' <commit>

Puis, sans authentification, sans accès en écriture, juste une requête GET vers un dépôt public :

GET /repo-commit/testuser/demo?hash=--output=/tmp/gitfed-commit-poc.txt

hash provient directement du paramètre de requête, sans aucune validation qu'il ressemble à un hash. git show a interprété --output=/tmp/gitfed-commit-poc.txt comme son propre indicateur -o/--output=<fichier> ("Output to a specific file instead of stdout.") plutôt que comme une révision — et a effectivement créé le fichier /tmp/gitfed-commit-poc.txt sur le disque du serveur.

Impact. Une écriture de fichier arbitraire à un chemin choisi par l'attaquant, à l'endroit où gitfed-server (uid 1000, propriétaire de tout /data) a le droit d'écrire — ce qui inclut hooks/post-receive dans n'importe quel dépôt bare de l'instance. Un hook git s'exécute automatiquement à chaque git push suivant sur ce dépôt : c'est un chemin réaliste vers l'exécution de code arbitraire en tant que processus gitfed-server, déclenchable par le tout premier visiteur anonyme d'une instance publique, aucun compte, aucune clé SSH, aucun accès en écriture nécessaire. La variante par branche (nécessite un accès en écriture pour créer la ref malveillante, puis ouvrir une merge request avec comme branche source ou cible) a été vérifiée de la même manière via branchFiles/BranchDiff/CheckMergeable/MergeBranches.

Correctif. --end-of-options ajouté juste avant chaque argument de ce type — un indicateur générique reconnu par les commandes basées sur parse-options de git (2.24+, largement disponible), qui force tout ce qui suit à être traité comme un argument positionnel, jamais comme une option. Vérifié explicitement que ça préserve le comportement correct pour une révision légitimement préfixée par un tiret (contrairement à --, qui aurait réinterprété l'argument comme un chemin de fichier au lieu d'une révision, cassant la sémantique de plage A...B). Testé en conditions réelles :

Un hash/nom de branche légitime continue de fonctionner exactement comme avant (diff/merge produits correctement).
Le même hash malveillant qu'en preuve de faisabilité ne crée plus aucun fichier ; la page correspondante répond 200 proprement au lieu d'un 500 (git rejette la révision, gitfed le gère normalement).
Test de régression TestArgumentInjectionIsBlocked (internal/gitexec/gitexec_test.go) : reproduit l'exploit exact contre chacune des fonctions concernées et échoue si le correctif est retiré (vérifié en désactivant temporairement --end-of-options sur CommitDiff — le test échoue bien en détectant le fichier créé).

🟡 L3 — Fermer une MR déjà fusionnée écrase silencieusement son statut

Fichier : internal/admin/admin.go (CloseMergeRequest)

Le problème. CloseMergeRequest fixait Status = MRClosed inconditionnellement, sans vérifier le statut courant. Une MR déjà fusionnée pouvait donc être "fermée" — le commit de fusion reste bien réel dans la branche cible, mais l'enregistrement de gitfed se met à afficher « Fermée sans fusion », un mensonge pur et simple sur l'historique.

Preuve de faisabilité. MR réellement fusionnée (branche cible avancée, vérifié par git log), puis POST /repo-mr-close/{repo} avec son numéro : la page de détail affiche ensuite le badge « fermée » et « Fermée sans fusion », alors que le commit de fusion est toujours présent dans l'historique de la branche.

Impact. Intégrité de la piste d'audit, pas d'élévation de privilège — n'importe quel collaborateur en écriture peut se donner un faux historique (« ma MR a été rejetée » alors qu'elle a bien été intégrée), utile pour tromper une revue ou masquer qu'un changement est passé.

Correctif. CloseMergeRequest est maintenant un no-op si le statut n'est plus MROpen, à l'image de la même garde déjà présente sur MergeMergeRequest. Test de régression TestCloseMergeRequestDoesNotOverwriteMerged (internal/admin/admin_test.go).

🟡 L4 — Titre/description/commentaire non plafonnés côté serveur

Fichier : cmd/gitfed-web/handlers_merge_requests.go

Les attributs maxlength du formulaire (200 pour le titre, 4000 pour la description/le commentaire) ne sont que côté client — un POST direct les contourne trivialement, comme vérifié par exploitation réelle (titre de 300 caractères accepté avant correctif). Contrairement au plafond déjà en place sur les notifications fédérées (accessible à n'importe quelle instance distante sans relation préalable), créer une MR ou commenter exige déjà d'être authentifié et d'avoir accès en lecture/écriture au dépôt — le risque est donc plus faible (abus par un collaborateur déjà de confiance, pas par un tiers anonyme), ce qui justifie un correctif proportionné : une limite de longueur serveur, sans plafond sur le nombre de MR/commentaires par dépôt.

Correctif. mrTitleMaxLen/mrTextMaxLen (200/4000, identiques aux attributs maxlength) revérifiés côté serveur dans handleMRCreate et handleMRComment. Vérifié : un titre de 300 caractères est maintenant rejeté avec le message d'erreur habituel, un titre normal continue de fonctionner.

3. Contrôles vérifiés et jugés sains

Surface	Constat
Autorisation de fusion/fermeture	Passe par `CheckAccess(repo, principal, RoleWrite)`, le même contrôle qu'un `git push` par SSH — vérifié qu'un utilisateur sans accès en écriture ne voit ni bouton Fusionner ni bouton Fermer, et qu'un `POST` direct sans cet accès reçoit un 404.
Fuite de MR d'un dépôt privé	`canView` revérifié dans `handleMRDetail`/`handleMRList` avant tout accès, même 404 uniforme que le reste de l'app — vérifié anonyme + utilisateur sans accès.
XSS via titre/description/commentaire	Tout passe par `html/template` en échappement automatique, jamais de `template.HTML` sur ces champs — vérifié qu'aucun de ces trois n'est enrobé différemment des autres champs utilisateur déjà couverts par l'audit précédent.
CSRF sur les routes de mutation (`/repo-mr-new`, `/repo-mr-merge`, `/repo-mr-close`, `/repo-mr-comment`)	Aucune de ces routes n'est dans la liste d'exemption de `sameOriginPOST` (seule `/git-upload-pack` l'est) — protégées par défaut.
Isolation de la fusion elle-même	`MergeBranches` tourne dans un worktree jetable (`git worktree add --detach`), jamais sur l'état réel de la branche cible ; nettoyage (`worktree remove --force`) garanti par `defer`, vérifié qu'aucun worktree ne survit après une fusion en conflit.
Mise à jour du ref cible	`git update-ref refs/heads/<target> <nouveau> <ancien>` est un compare-and-swap — un push concurrent qui déplace `<target>` entre le calcul de la fusion et cet appel fait échouer la mise à jour au lieu de l'écraser silencieusement. Le commit de fusion perdant devient orphelin (récupéré par un futur `git gc`), pas de corruption de données.
Double clic / fusion concurrente sur la même MR	La vérification `Status == MROpen` se fait avant l'opération git (lente), mais comme la mise à jour du ref est elle-même atomique côté git, un seul des deux appels concurrents peut réellement faire avancer la branche ; le perdant reçoit une erreur serveur générique au lieu d'un message clair — une aspérité d'UX notée mais pas une faille de sécurité, la donnée stockée reste correcte.
Numérotation des MR (`#1`, `#2`, ...)	Lecture + incrément + écriture dans une seule transaction `bbolt.Update` — bbolt garantit une seule transaction d'écriture à la fois sur toute la base, donc pas de race possible sur l'attribution du numéro même avec des créations concurrentes.

4. Note

C1 n'est pas une régression d'un contrôle qui existait avant et aurait été retiré — c'est une fonctionnalité (l'affichage détaillé de commit, puis les merge requests) ajoutée dans cette même session sans que ce chemin d'argument-injection ait été anticipé au moment de l'écrire. L'audit du 29/07 précédent (AUDIT-2026-07-29.md) avait bien vérifié l'absence d'injection d'argument sur repo.Path dans le clone HTTP — ce constat reste vrai, il portait simplement sur une surface différente de celle touchée ici, ajoutée après coup.