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.

MAJ 2026-07-09 : les 5 axes de durcissement ci-dessous sont tous traités.

1. 🔴 Verifier = clé → ✅ découplé (-v2 : verifier = SHA256(keyHex+domaine)). §1.1
2. 🟠 Sync sans concurrence atomique → ✅ ETag/If-Match + 412 re-merge. §2.1
3. 🟠 Arbitrage tombstone / horloge → ✅ timestamps UTC partout. §2.2
4. 🟠 PBKDF2 GPU-friendly → ✅ Argon2id (argon2id-v2). §1.2
5. 🟡 monofichier + zéro test → ✅ 8 modules + 65 tests (gate de build). §3.1/§3.2

---

1. Sécurité

1.1 ✅ Verifier = matériel de clé — corrigé (schemes -v2)

Fait : les schemes pbkdf2-sha256-v2 et argon2id-v2 transmettent verifier = SHA256(keyHex + "pmserver/auth-verifier/v2") au lieu de keyHex (isDecoupledVerifierAlgo/verifierFromKeyHex, js/app.crypto.js). Le verifier est désormais une fonction one-way de la clé → intercepter /login ne donne plus la clé AES. Adoption register + rotation ; comptes pré-v2 basculent à leur prochaine rotation. La clé AES reste la sortie brute du KDF (entries déchiffrables). Détails CLAUDE.md « Auth-hash schemes ».

Analyse d'origine

deriveKeyAndVerifier() (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 transitait en clair vers /login = directement la clé du vault si le corps de requête fuit. C'est exactement ce que -v2 corrige (verifier = one-way de la clé).

1.2 🟠 PBKDF2-SHA256 vs Argon2id — corrigé (2026-07-05)

PBKDF2-SHA256 (600k) reste GPU/ASIC-friendly → un master pw faible tombe vite sur matériel dédié si la DB fuit.

Fait : nouveau scheme argon2id-v2 (Argon2id memory-hard + verifier décplé). Détails dans le CLAUDE.md « Auth-hash schemes ».

• JS pur (js/argon2.js, bundle @noble/hashes@2.2.0), pas WASM : la CSP script-src 'self' n'accorde pas wasm-unsafe-eval. Vérifié contre le vecteur RFC 9106 §5.3 (test unitaire).
• Zéro Argon2 côté Delphi (l'audit se trompait sur ce point) : le serveur ne dérive jamais la clé, il stocke/échoie juste les params (argon2_m/t/p) et SHA256-wrappe le verifier comme tout -v2.
• Params OWASP m=19 MiB, t=2, p=1 (~0.65 s/unlock), stockés par compte.
• Adoption : register + change-master-pw (nouveau défaut). Comptes existants restent PBKDF2 jusqu'à rotation. Passer à Argon2id re-chiffre tout le vault (la clé dérivée change) — c'est le flow rotation existant.
• Tests : 8 tests crypto Argon2 (vecteur RFC, branche KDF, contrat de params register↔login, sensibilité aux params). 42/42.

Validé runtime : un compte ayant tourné sa master pw affiche hash_algo=argon2id-v2 (m=19456, t=2, p=1) et se reconnecte/déchiffre.

✅ Async (2026-07-05) : deriveKeyBytes utilise argon2idAsync (bundle re-vendé pour l'exposer) — cède la main à l'event loop pour que le spinner s'anime au lieu de figer ~0.65 s. Parité sync/async testée sur le vecteur RFC 9106. Plus rien d'ouvert sur §1.2.

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 : username, site, title, tags, template chiffrés (username/site/title/tags 2026-07-09 ; template 2026-07-11) ; folder, kind restent en clair (folder = requête serveur de réassignation ; kind = 2 valeurs, ~0 entropie).

username + site + title + tags chiffrés au repos (✅ 2026-07-09) : colonnes <f>_enc/<f>_iv (AES-GCM sous la clé du vault). Clé de l'approche : recherche/tri sont côté client → on déchiffre au loadEntries en mémoire, donc AES-GCM plein (IV aléatoire), pas de searchable-encryption. Choke-point withEncryptedMeta sur tous les writes ; decryptEntryMeta au load ; migration migrateMetadataAtRest (live + corbeille) ; rotation re-chiffre les 4. Validation serveur « Site required » retirée + ?q= neutralisé (LIKE inutile sur ciphertext). Détails CLAUDE.md « Entry payload ». Validé runtime le 2026-07-09 : après rebuild + unlock du compte test, la base montre 0 en clair sur les 4 champs (username/site/title/tags) pour ce compte, _enc remplies, affichage/ recherche/favicons OK. La migration est par utilisateur (tourne au unlock sur les entries du compte connecté) — un compte non connecté garde son clair jusqu'à sa prochaine connexion (comportement normal, pas une régression). template chiffré au repos (✅ 2026-07-11) : même pattern (template_enc/template_iv, ajouté à ENCRYPTED_META_FIELDS). Migration vérifiée (0 clair restant, 2 comptes) → refs colonne clair retirées du code + AddColumnIfMissing supprimés (template, et tags/title qui étaient revenus en fantômes après le drop) + CREATE TABLE corrigé (site/username NOT NULL cassait une DB neuve). Guard dans withEncryptedMeta : les re-ships partiels (add-tag, move-to-folder) omettent template → le serveur le préserve via LHasTemplate (clé absente), pas de wipe. 3 tests unitaires. Pièces jointes : le contenu est déjà chiffré (AES-GCM, encryptBlobBytes) ; seuls filename/mime/size_bytes restent en clair (pour lister sans tout déchiffrer). Résiduel : folder, kind, métadonnées d'attachments, nombre de lignes, timestamps.

1.4 ✅ Sync password faible — corrigé (2026-07-09)

Le snapshot WebDAV n'est protégé que par le sync password. Fait : syncSetEncPwdFlow exige maintenant ≥12 chars ET computeStrength ≥ 50 (réutilise computeStrength, pas de dép zxcvbn). Le maillon faible est fermé.

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 — corrigé (ETag/If-Match)

Fait : concurrence optimiste implémentée. runSyncNow capture l'ETag au GET, l'envoie en If-Match au PUT (js/app.sync.js) ; un 412 → re-pull + re-merge + re-push (borné à 3). Serveur sans ETag → etag vide → pas d'If-Match → fallback last-write-wins. Détails CLAUDE.md « Sync ».

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 create/update/bulk, 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 — corrigé (2026-07-09)

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

En cours (2026-07-05) : découpage incrémental en fichiers classic-script chargés dans l'ordre via <script> séparés — pas de bundler ES/esbuild (les classic scripts partagent l'environnement lexical global, donc zéro réécriture des call-sites, risque quasi nul vs conversion en modules ES).

• ✅ js/app.crypto.js extrait (KDF, verifier, AES-GCM) — byte-for-byte identique, 42 tests verts, pas de const dupliqué. Chargé AVANT app.js.
• ✅ js/app.sync.js extrait (WebDAV, snapshot, merge applyRemoteSnapshot) — byte-for-byte identique, couvert par les tests merge, chargé APRÈS app.js (effet de bord top-level Bridge.onWebdavResult).
• ✅ js/app.import.js extrait (export container + parsing CSV/JSON + doImport/doExport) — byte-for-byte identique, couvert par les 14 tests CSV, chargé AVANT app.js (pures déclarations). Pattern + règles d'ordre documentés dans CLAUDE.md « Découpage frontend ».
• ✅ js/app.totp.js extrait (TOTP RFC 6238 + crypto TOTP/custom-fields) — byte-for-byte identique, chargé AVANT app.js. +13 tests TOTP (vecteurs RFC 6238) ajoutés en même temps → extraction et nouvelle couverture.
• ✅ js/app.backup.js extrait (auto-backup planifié) — byte-for-byte identique, chargé AVANT app.js (pures déclarations).
• ✅ js/app.favicon.js extrait (fetch/cache favicons) — chargé AVANT app.js. +7 tests faviconHost (extraction hostname → décide quel domaine part vers DDG). A révélé un commentaire faux (IPs « valides » alors qu'elles sont rejetées par la règle TLD [a-z]{2,}) — corrigé.
• ✅ js/app.health.js extrait (vault health dashboard) — byte-for-byte identique, chargé AVANT app.js (auditCache/auditFilter viennent avec, résolus cross-fichier).
• ✅ js/app.overlays.js extrait (quick search + cheatsheet + password history) — quicksearch était entrelacé avec ces 2 overlays, donc extraits ensemble en un bloc contigu byte-for-byte. Chargé AVANT app.js.
• app.js : 11 936 → 9 170 lignes (8 modules sortis, ~2 770 lignes).
• Suite de tests : 42 → 65 tests.
• Reste : sections très couplées au state/DOM (slideover, settings, folders, attachments, autofill, PIN/recovery/quick-unlock) — rendement/risque faible, à faire au fil de l'eau. Le socle §3.1 (pattern + modules à risque isolés + filet de tests) est en place.
• ✅ node --check en pré-étape de BuildAssets.ps1 : déjà fait (cf. §3.2).

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/ (65 tests, Node node:test, zéro dépendance, ~1.7 s) — cf. 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 — ✅ fait (§1.4 : ≥12 + computeStrength≥50).
• 🔵 Indicateur de force du master pw — ✅ déjà là (regStrengthBar + computeStrength au register ; barre aussi dans le slideover + entry modal).
• 🔵 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 — ✅ déjà là : popup #idleWarning + countdown WARNING_SECONDS avant lock (showIdleWarning, bouton « Stay unlocked »).
• 🔵 Avatar : l'inclure dans le sync snapshot + auto-backup — ✅ fait : avatar_b64 dans buildSyncSnapshot + payload auto-backup ; restauré additivement dans applyRemoteSnapshot (adopté seulement si le device n'a pas d'avatar, jamais écrasé). +2 tests merge.

---

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

Feature	Valeur	Effort	Note
Argon2id ✅	🔴	—	Fait : argon2id-v2, JS pur (noble). §1.2
ETag/If-Match sur sync ✅	🟠	—	Fait. §2.1
Verifier découplé de la clé ✅	🔴	—	Fait : -v2. §1.1
Tests automatisés ✅	🟠	—	Fait : 65 tests, gate de build. §3.2
Password strength par entry ✅	🟡	—	Fait : barre slideover + vault health (computeStrength, pas zxcvbn)
Timestamps UTC ✅ (Lamport optionnel)	🟠	—	Fait. §2.2
Extension navigateur	🟠 Haute	Élevé	Autofill in-page, feature #1 demandée
Windows Hello (biométrie)	🟠 Moyenne	Moyen	Windows.Security.Credentials
Import KeePass XML / 1PUX	🟡 Moyenne	Moyen	Complète l'écosystème d'import
i18n (FR/EN propre)	🔵 Basse	Moyen	FR/EN mélangés dans l'UI
Emergency access / partage	🔵 Basse	Élevé	Hors scope "perso offline"

---

6. Plan d'action recommandé (ordre)

Tout le plan initial est fait :

1. node --check dans BuildAssets ✅ (+ suite de tests en gate).
2. Découpler verifier ↔ clé ✅ (-v2, §1.1).
3. ETag/If-Match sur sync ✅ (§2.1).
4. Timestamps UTC ✅ (§2.2).
5. Tests unitaires crypto + merge ✅ (65 tests, §3.2).
6. Argon2id ✅ (§1.2).
7. Découpage app.js ✅ socle (8 modules, §3.1) — reste sections UI couplées, au fil de l'eau.

Bonus fait hors plan : métadonnées chiffrées au repos (§1.3), sync pw fort (§1.4), 401 session (§2.5), avatar sync (§4).

---

6b. Revue source complète (2026-07-11) — noté, NON corrigé

Passe manuelle JS + Delphi + HTML + CSS à la recherche d'illogismes, bugs invisibles, perf et code mort. Rien de bloquant ; classé par type.

Code mort / nettoyage

• js/app-legacy.js (1549 lignes) + index-legacy.html + css/style-legacy.css — ✅ supprimés (2026-07-11). Ancien monolithe pré-découpage §3.1 (le trio HTML+JS+CSS). Ni embarqués (absents du whitelist BuildAssets.ps1) ni référencés sauf entre eux. 68/68 tests OK après suppression.
• #passkeyBtn (index.html:134) — bouton display:none jamais câblé en JS. Stub passkey/WebAuthn abandonné. → supprimer le markup (ou implémenter).
• #passkeyBtn (index.html:134) — ✅ supprimé (2026-07-11). Bouton display:none jamais câblé (stub passkey/WebAuthn abandonné).
• IDs inutilisés #reauthTitle, #historyTitle, #reauthConfirm — laissés volontairement : un id non référencé ne coûte rien et pourrait servir à un titre dynamique plus tard. Les retirer = churn à bénéfice nul.

Performance

• Search sans debounce (app.js:8515) — won't-do (YAGNI, 2026-07-11). Pour < 500 entries le render est instantané ; un debounce ajouterait de la latence au retour de recherche (régression du feel) pour un gros vault hypothétique. À revisiter seulement si un profiler montre du lag réel.
• N+1 attachments séquentiel (buildSyncSnapshot, auto-backup, rotation) — déféré (2026-07-11) : paralléliser touche le chemin sync/rotation (zone la plus délicate) pour un gain spéculatif ; < 1 s sur loopback à < 500 entries. Trigger pour revisiter : un backup/sync perceptiblement lent.

Design / cohérence (tranché)

• favorite / pinned device-local — ✅ décidé device-local (2026-07-11), comme l'avatar. Absents de buildSyncSnapshot → leurs toggles serveur (Entries.pas:804 favorite, 851 pin) ne bumpent volontairement PAS updated_at. Asymétrie assumée (favorite est dans backup/export mais pas sync). Documenté dans CLAUDE.md « Settings sync ».

Vérifié sain (pour ne pas re-chasser)

• DB.Lock/DB.Unlock équilibrés (les écarts au grep étaient des mentions en commentaire, pas des appels).
• Aucun except … end muet côté Delphi (pas de swallow silencieux serveur).
• Aucun == laxiste suspect en JS ; setInterval/clearInterval appariés.
• Restore d'entry (Entries.pas:759) et set-icône (965) bumpent bien updated_at.
• Soft-delete (712) ne bump pas mais sort du snapshot live → OK (trash non synced).

---

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.

MAJ 2026-07-09 : tous les axes de durcissement identifiés sont traités — verifier découplé (§1.1), Argon2id (§1.2), métadonnées chiffrées au repos (§1.3), sync password fort (§1.4), sync atomique ETag (§2.1), timestamps UTC (§2.2), tests + découpage (§3). Reste uniquement du confort/cosmétique (§2.3/2.4/2.6/2.7, §3.3) et des features hors durcissement (§5 : extension navigateur, Windows Hello, import KeePass, i18n). Le produit est passé de « sûr pour du solo loopback » à « durci pour un modèle de menace DB/sync qui fuit ».