# 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).**

```sh
# 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.