feat: quick-search fill modes + editable custom-field combobox + JS build gate
Quick search (Ctrl+Shift+Q fill mode) - Enter / left-click → full autofill (username + Tab + password), like Ctrl+Shift+L. - Shift+Enter / right-click → username only (new Delphi username-only SendInput path via field=user; ExecuteAutofill AUsernameOnly param). - Ctrl+Enter / Ctrl+click → password only. - Copy mode (tray / palette) unchanged: Enter/left = password, Shift+Enter/right = username. - Clipboard fix: copy-then-minimise no longer wipes the just-copied password — MinimizeToTray takes an AClearClipboard flag (False on the quick-search copy path, driven by app/minimize?keepclip=1). The 30s auto-clear still guards it. - Right-click on a result row suppresses the native/custom context menu (preventDefault + stopPropagation). Editable custom-field combobox - Option-backed custom fields (card brand, expiry year/month, etc.) now render a custom editable combobox instead of a locked <select>: an arrow drops a menu of ALL options (a native <datalist> filtered to the typed text, which confused users), while the input stays freely typeable for values not in the list. Storage shape unchanged. - Outside-click closes the menu via the existing slideover mousedown handler; item mousedown + preventDefault so blur doesn't race the pick. Build safety - BuildAssets.ps1 runs `node --check` on every embedded .js before generating assets.res. A syntax error now aborts the asset build (exit 1, file + line logged) instead of shipping a dead bundle that only surfaces after a full Delphi rebuild. Node is optional: absent → warn and continue. Docs - CODE_AUDIT.md: full static-analysis report (security, latent bugs, maintainability, future features, prioritized action plan). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
r-zakarya
+277
@@ -0,0 +1,277 @@
|
||||
# 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
|
||||
|
||||
`applyRemoteSnapshot()` compare `entry.updated_at > tombstone.deleted_at`
|
||||
pour décider résurrection vs suppression. Ces timestamps viennent de
|
||||
`FormatDateTime('yyyy-mm-dd hh:nn:ss', Now)` — **heure locale du device
|
||||
qui a écrit**. Entre deux machines avec des horloges décalées (ou fuseaux
|
||||
différents), l'arbitrage last-write-wins peut se tromper :
|
||||
|
||||
- Device A (horloge en avance) supprime → deleted_at "futur"
|
||||
- Device B édite (horloge correcte) → updated_at "passé" vs deleted_at A
|
||||
- B croit que la suppression est plus récente → tue l'édition de B
|
||||
|
||||
**Recommandation** : stocker les timestamps en **UTC ISO 8601** partout
|
||||
(serveur ET snapshot), et idéalement un compteur logique (Lamport) en
|
||||
complément pour les cas d'égalité. À minima, documenter que les horloges
|
||||
des devices doivent être synchronisées (NTP).
|
||||
|
||||
### 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é
|
||||
|
||||
Toute la validation est manuelle (TEST_PLAN.md, TEST_REGRESSION.md). Les
|
||||
zones à haut risque de régression (crypto round-trip, merge de sync,
|
||||
arbitrage tombstone, dirty-check) sont exactement celles qui bénéficieraient
|
||||
de tests unitaires.
|
||||
|
||||
**Recommandations** :
|
||||
- Tests unitaires JS (Vitest/Jest) sur : `encryptPwd`/`decryptPwd` round-trip,
|
||||
`deriveKeyAndVerifier` (vecteurs connus), `applyRemoteSnapshot` (merge +
|
||||
résurrection), `isSoDirty`, `parseEntriesFromCSV/JSON`.
|
||||
- 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) — fiabilise l'arbitrage tombstone.
|
||||
5. **Tests unitaires crypto + merge** (§3.2) — filet avant d'ajouter des features.
|
||||
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.
|
||||
Reference in New Issue
Block a user