Password-Manager/CODE_AUDIT.md

# Audit de code — PMServer (Password Manager)

> Analyse statique + revue architecture au 2026-07-03.
> Portée : crypto, auth, HTTP lockdown, sync, frontend, robustesse.
> Basé sur la lecture du code, pas sur un pentest dynamique.

Légende sévérité : 🔴 critique · 🟠 important · 🟡 moyen · 🔵 mineur / cosmétique

---

## 0. Résumé exécutif

Le produit est **solide pour un gestionnaire perso offline** : chiffrement
client-side AES-GCM, zero-knowledge login (le serveur ne voit jamais le
master pw), défense en profondeur sur le serveur loopback (token + PID
check + CSP stricte), DPAPI pour la persistance device-bound. La majorité
des "bugs" trouvés cette session ont été corrigés.

Les points qui méritent attention avant de considérer le produit "durci" :

1. 🔴 **Le verifier envoyé au serveur EST la clé de chiffrement** (en hex).
2. 🟠 **Sync WebDAV sans concurrence atomique** → lost update possible.
3. 🟠 **Arbitrage tombstone sensible au décalage d'horloge** entre devices.
4. 🟠 **PBKDF2-SHA256** (GPU-friendly) au lieu d'Argon2id.
5. 🟡 **`app.js` monofichier ~14k lignes** + aucun test automatisé.

---

## 1. Sécurité

### 1.1 🔴 Verifier = matériel de clé (couplage clé/authentifiant)

`deriveKeyAndVerifier()` ([js/app.js](js/app.js)) dérive **un seul** output
PBKDF2 et l'utilise pour DEUX rôles :

- `cryptoKey` = les 32 octets bruts importés en AES-GCM (chiffre les entries)
- `verifier` = **ces mêmes 32 octets en hex**, envoyés au serveur à `/login`

Le serveur stocke `SHA-256(verifier)`, donc une fuite de la **DB** ne donne
pas la clé (préimage SHA-256). MAIS :

- Le `verifier` transite en clair vers `/login` (loopback HTTP). Toute
  fuite du **corps de requête** (log, proxy de debug, extension, futur
  bug XSS contournant la CSP) expose **directement la clé du vault**.
- Conceptuellement, l'authentifiant et le secret de chiffrement ne
  devraient jamais être le même matériel.

**Recommandation** : dériver le verifier dans un **domaine séparé** —
p.ex. `verifier = HKDF(keyBytes, info="auth")` ou une 2ᵉ dérivation
PBKDF2 avec un `info`/salt distinct. Ainsi le verifier transmis n'est
pas la clé. Migration possible sans re-chiffrer les entries (seul le
verifier stocké côté serveur change).

### 1.2 🟠 PBKDF2-SHA256 vs Argon2id

600k itérations SHA-256 pour les nouveaux comptes (correct, conforme
OWASP 2023 ≥ 600k). Mais PBKDF2-SHA256 reste **GPU/ASIC-friendly**.
Un master pw faible tombe vite sur du matériel dédié si la DB fuit.

**Recommandation** : migrer vers **Argon2id** (memory-hard). Coût :
WebAssembly côté JS (argon2-browser) + implémentation Delphi, avec
migration progressive (comme le passage 100k→600k déjà en place).

### 1.3 🟡 Métadonnées en clair

Documenté mais à rappeler pour un futur modèle de menace :

- `entry_attachments` : `filename`, `mime`, `size_bytes` **non chiffrés**
- `users.avatar_b64` : image **non chiffrée** (cosmétique, assumé)
- `vault_entries` : `site`, `title`, `username`, `folder`, `tags`, `kind`,
  `template` **non chiffrés** (nécessaire pour recherche/tri sans déchiffrer)

Un attaquant avec accès disque voit la liste des sites et usernames. Pour
un vault perso c'est un compromis acceptable (recherche instantanée), mais
à documenter clairement pour l'utilisateur.

### 1.4 🟡 Snapshot de sync = tout le vault en clair sous le sync password

`buildSyncSnapshot()` déchiffre chaque entry puis re-chiffre le tout sous
`syncEncPwd`. Si l'utilisateur choisit un sync password faible, le fichier
WebDAV distant devient le maillon faible (le master pw fort ne protège
plus rien sur le remote). **Recommandation** : imposer une politique de
force minimale sur le sync password (déjà ≥ 6 chars — trop peu ; viser
≥ 12 + zxcvbn).

### 1.5 🔵 Points positifs (à conserver)

- ✅ AES-GCM 256 (AEAD, intégrité incluse) — pas de mode ECB/CBC nu
- ✅ IV aléatoire par blob (`crypto.getRandomValues(12)`) — pas de réutilisation
- ✅ Zero-knowledge : le master pw plaintext ne quitte jamais le client
- ✅ CSP stricte (`default-src 'self'; img-src 'self' data:`) — bloque
  l'exfiltration même en cas d'injection
- ✅ Serveur loopback + token + PID-on-socket check (defense in depth)
- ✅ DevTools / menu contextuel Edge désactivés
- ✅ Rendu via `textContent` / `el()` (pas d'`innerHTML` sur données user)
  → surface XSS minimale
- ✅ Clipboard sécurisé (exclusion clipboard history + auto-clear 30s)
- ✅ Recovery code à usage limité (5), consommé, supprimé après rotation

---

## 2. Bugs / risques latents (non encore observés)

### 2.1 🟠 Sync : lost update (pas de concurrence atomique)

`runSyncNow()` fait `GET` → merge → `PUT`. Deux devices qui syncent
**en même temps** sur le même fichier WebDAV : le 2ᵉ `PUT` écrase le 1ᵉʳ
sans détecter le conflit → perte de la fenêtre de merge de l'un des deux.

**Recommandation** : concurrence optimiste via **ETag / If-Match** WebDAV.
Récupérer l'ETag au `GET`, l'envoyer en `If-Match` au `PUT` ; si 412
Precondition Failed → re-pull + re-merge + re-push. La plupart des serveurs
WebDAV (Nextcloud, Apache mod_dav) supportent les ETags.

### 2.2 🟠 Arbitrage tombstone sensible à l'horloge

### 2.2 🟠 Arbitrage tombstone sensible à l'horloge — **corrigé (2026-07-04)**

`applyRemoteSnapshot()` compare `entry.updated_at > tombstone.deleted_at`
pour décider résurrection vs suppression.

**Bug réel trouvé** (pire que décrit) : deux sources d'horloge coexistaient
**sur un même device**. `created_at`/`updated_at` des entries étaient écrits
via Delphi `FormatDateTime(..., Now)` = **heure locale**, tandis que
`deleted_at` (soft-delete + tombstones) venait de SQLite
`CURRENT_TIMESTAMP` / `datetime('now')` = **UTC**. L'arbitrage comparait donc
un `updated_at` local à un `deleted_at` UTC → décalage = l'offset UTC de la
machine, **même en solo mono-device** (pas seulement entre devices décalés).

**Fix** : helper `NowUTC` / `NowUTCStr` dans `PM.Database`, substitué à
`FormatDateTime(..., Now)` à tous les sites d'écriture d'entries/attachments
([PM.Handler.Entries.pas](delphi-backend/Handlers/PM.Handler.Entries.pas)
create/update/bulk, [PM.Handler.Attachments.pas](delphi-backend/Handlers/PM.Handler.Attachments.pas)).
Tout est désormais UTC (les paths SQLite l'étaient déjà). Côté JS aucun
changement nécessaire : `buildSyncSnapshot` utilise `toISOString()` (UTC) et
l'arbitrage compare deux valeurs du **même** fuseau → `Date.parse` applique
le même offset local aux deux, ordre relatif préservé.

**Rows existantes** : décision « laisser se soigner » — les anciennes lignes
gardent leur `updated_at` local jusqu'à leur prochain edit (qui le réécrit en
UTC). Exposition étroite pour un vault solo ; pas de migration destructive
(un shift par l'offset courant serait irréversible et approximatif : DST /
changement de fuseau historique).

**Reste (optionnel)** : compteur logique (Lamport) pour les cas d'égalité
stricte ; incohérence de **format** pré-existante (serveur `'yyyy-mm-dd
hh:nn:ss'` vs export JS `toISOString()` avec `T…Z`) sur les rares rows sans
`created_at`.

### 2.3 🟡 `catch (_) {}` silencieux en cascade

Nombreux `catch (_) {}` dans `applyRemoteSnapshot`, `buildSyncSnapshot`,
restauration d'attachments, création de folders. Une erreur réseau/serveur
transitoire est avalée → l'entry/folder/attachment est silencieusement
sauté. Le garde-fou "failed count → abort push" atténue la perte côté
entries, mais **pas** côté attachments ni folders.

**Recommandation** : logger chaque `catch` (au moins `console.warn` +
compteur), et étendre le garde-fou "n échecs → warn utilisateur" aux
attachments.

### 2.4 🟡 Pas de pagination serveur sur `GET /entries`

Tout le vault est chargé + déchiffré à chaque unlock. Sur un gros vault
(5k-10k entries), `buildSyncSnapshot` (déchiffre chaque entry + fetch
attachments un par un) et `computeHealthCache` deviennent lents (O(n)
séquentiel, `await` en boucle). Acceptable pour usage perso (< 500
entries), problématique au-delà.

**Recommandation** : batcher les déchiffrements (Promise.all par lots),
et si besoin paginer côté serveur pour la vue grille.

### 2.5 🟡 500 au lieu de 401 sur session expirée

`Authenticate` écrit 401 puis `raise ESessionRejected` → le `try/except`
global de `PM.HTTPServer` réécrit un **500**. Pré-existant, tous les
handlers concernés. Cosmétique (le client voit un non-2xx) mais brouille
le debug.

**Recommandation** : attraper `ESessionRejected` spécifiquement dans le
dispatcher et ne pas réécrire la réponse déjà envoyée.

### 2.6 🔵 CSV re-import crée des doublons

Documenté/assumé (les lignes CSV sans uuid mint un nouvel uuid à chaque
import). Le dedup uuid ne couvre que le JSON. Acceptable si documenté à
l'utilisateur.

### 2.7 🔵 `navigator.clipboard` fallback échoue silencieusement si caché

Quand le Bridge Delphi est absent, le fallback `navigator.clipboard.
writeText` échoue si le document n'a pas le focus (WebView caché). Cas
rare (le Bridge est quasi toujours actif en prod) mais le `catch (_) {}`
masque l'échec → l'utilisateur croit avoir copié.

---

## 3. Qualité de code / maintenabilité

### 3.1 🟡 `app.js` monofichier ~14 000 lignes

Tout le frontend en un seul fichier. Difficile à naviguer, à tester, et
un edit malencontreux casse tout le parse (déjà arrivé cette session : une
déclaration de fonction supprimée → app entièrement morte, découvert
seulement au runtime).

**Recommandations** :
- Découper en modules ES (`crypto.js`, `sync.js`, `slideover.js`,
  `quicksearch.js`, `settings.js`…) + un bundle simple (esbuild) dans
  `BuildAssets.cmd`.
- **Ajouter `node --check` (ou eslint) en pré-étape de `BuildAssets.cmd`**
  → aurait attrapé le syntax error avant le rebuild. Gain immédiat, coût
  quasi nul.

### 3.2 🟡 Aucun test automatisé — **partiellement adressé (2026-07-04)**

Toute la validation était manuelle (TEST_PLAN.md, TEST_REGRESSION.md). Les
zones à haut risque de régression (crypto round-trip, merge de sync,
arbitrage tombstone) sont exactement celles qui bénéficient d'un filet
unitaire.

**Fait** : suite `js/tests/` (35 tests, Node `node:test`, zéro dépendance,
~1.7 s) — cf. [js/tests/README.md](js/tests/README.md). Harness `node:vm`
qui charge `app.js` (monofichier) avec globals navigateur stubbés :
- `crypto.test.js` — `deriveKeyAndVerifier` (clé AES == PBKDF2 brut,
  cross-checké vs `pbkdf2Sync` Node), découplage verifier legacy vs `-v2`,
  `encryptPwd`/`decryptPwd` round-trip, unicité IV, tamper/mauvaise clé → `[ERROR]`.
- `csv.test.js` — `parseCSV`, `findColumn`, `parseEntriesFromCSV` (Bitwarden/
  KeePass, classification note vs login).
- `merge.test.js` — `applyRemoteSnapshot` : add/update/skip (LWW), tombstone
  delete, arbitrage résurrection (2 branches `NaN`), veto tombstone local,
  merge additif de folders. Seule `api()` est stubbée (fake server mémoire) ;
  `loadEntries`/`encryptImportEntry` tournent en vrai.

Intégré comme **gate de build** dans `BuildAssets.ps1` (après `node --check`,
bypass `PM_SKIP_TESTS=1`).

**Reste à faire** :
- `isSoDirty` (couplé DOM/`soState` — nécessite plus de stubbing).
- Tests Delphi (DUnitX) sur les handlers critiques (bulk-import + tombstone
  purge, rotation master pw).

### 3.3 🔵 Duplication de constantes cross-langage

`600000`, formats de timestamp, noms de colonnes, shapes JSON sont
dupliqués entre JS et Delphi. Un changement d'un côté sans l'autre =
bug silencieux (déjà vu avec les champs oubliés dans la rotation/duplicate,
cf. la checklist "Entry payload" de CLAUDE.md).

---

## 4. Améliorations UX (rapides)

- 🔵 **Force du sync password** : afficher un indicateur zxcvbn au réglage,
  refuser < 12 chars.
- 🔵 **Indicateur de force du master pw** à l'enregistrement (zxcvbn).
- 🔵 **Feedback d'échec sync partiel** : "3 attachments non synchronisés"
  au lieu d'un silence.
- 🔵 **Combobox custom fields** : navigation clavier (flèches ↑↓ + Enter)
  dans le menu déroulant, pas seulement souris.
- 🔵 **Auto-lock** : afficher le temps restant avant lock dans un coin.
- 🔵 **Avatar** : l'inclure dans le sync snapshot + auto-backup (actuellement
  seulement dans l'export manuel) pour cohérence multi-device.

---

## 5. Fonctionnalités futures (par valeur / effort)

| Feature | Valeur | Effort | Note |
|---|---|---|---|
| **Argon2id** (KDF memory-hard) | 🔴 Haute | Moyen | argon2-browser + Delphi, migration progressive |
| **ETag/If-Match sur sync** | 🟠 Haute | Faible | Évite les lost updates multi-device |
| **Extension navigateur** | 🟠 Haute | Élevé | Autofill in-page, feature #1 demandée |
| **Windows Hello (biométrie)** | 🟠 Moyenne | Moyen | `Windows.Security.Credentials` — déverrouillage biométrique |
| **Tests automatisés** | 🟠 Haute | Moyen | Filet de sécurité contre les régressions |
| **Import KeePass XML / 1PUX** | 🟡 Moyenne | Moyen | Complète l'écosystème d'import |
| **Password strength par entry** | 🟡 Moyenne | Faible | zxcvbn dans le slideover + vault health |
| **Verifier découplé de la clé** | 🔴 Haute | Faible | Cf. §1.1 — fix de sécurité prioritaire |
| **i18n (FR/EN propre)** | 🔵 Basse | Moyen | Actuellement FR/EN mélangés dans l'UI |
| **Timestamps UTC + Lamport** | 🟠 Moyenne | Moyen | Fiabilise l'arbitrage sync (§2.2) |
| **Emergency access / partage** | 🔵 Basse | Élevé | Hors scope "perso offline" |

---

## 6. Plan d'action recommandé (ordre)

1. **`node --check` dans `BuildAssets.cmd`** — 5 min, évite les JS cassés en prod.
2. **Découpler verifier ↔ clé** (§1.1) — fix sécu prioritaire, effort faible.
3. **ETag/If-Match sur sync** (§2.1) — évite la perte de données multi-device.
4. ~~**Timestamps UTC partout** (§2.2)~~ — ✅ fait (`NowUTCStr`, going-forward ; rows existantes self-heal).
5. ~~**Tests unitaires crypto + merge** (§3.2)~~ — ✅ fait (35 tests, `js/tests/`, gate de build).
6. **Argon2id** (§1.2) — durcissement KDF, migration progressive.
7. Découpage `app.js` en modules (§3.1) — maintenabilité long terme.

---

## 7. Verdict

Pour un usage **personnel, local, mono-utilisateur**, le produit est
**sûr et fonctionnel** aujourd'hui. Les défenses en profondeur (loopback,
token, PID check, CSP, zero-knowledge, AES-GCM) sont bien pensées et
au-dessus de la moyenne des projets perso.

Les axes de durcissement (verifier découplé, Argon2id, sync atomique,
timestamps UTC) deviennent importants **dès qu'on vise le multi-device
sérieux ou un modèle de menace où la DB / le fichier de sync peut fuiter**.
Aucun de ces points n'est un trou béant immédiat en usage loopback solo,
mais §1.1 (verifier=clé) est celui que je corrigerais en premier car il
est peu coûteux et supprime un couplage dangereux.