47aec30934
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
986 lines
46 KiB
Markdown
986 lines
46 KiB
Markdown
# 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](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 ».
|
||
|
||
<details><summary>Analyse d'origine</summary>
|
||
|
||
`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` 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é).
|
||
</details>
|
||
|
||
### 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](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](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`.
|
||
|
||
> ⚠️ **MAJ 2026-07-16 — « Tout est désormais UTC » était inexact, puis corrigé.**
|
||
> La passe de substitution avait raté **`PM.Session.CreateSession`**
|
||
> (`FormatDateTime(..., IncHour(Now, 24))`, heure locale). ✅ **Corrigé en
|
||
> §6c-2** (`IncHour(NowUTC, 24)` + `Authenticate` sur `NowUTC`). C'était le
|
||
> dernier `FormatDateTime(..., Now)` du backend → §2.2 est maintenant réellement
|
||
> clos.
|
||
|
||
### 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](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 (multi-langue)** | 🔵 Basse | Moyen | UI 100% EN (vérifié 2026-07-13) ; i18n = feature future, pas un bug |
|
||
| **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).
|
||
|
||
---
|
||
|
||
## 6c. Revue backend Delphi (2026-07-16) — correctifs proposés, NON appliqués
|
||
|
||
Passe manuelle sur les ~10 500 lignes Delphi (`Source/` + `Handlers/` + `.dpr`),
|
||
axée crypto/auth/session/HTTP/SQL. Les correctifs ci-dessous sont **écrits mais
|
||
pas appliqués** — chacun est autonome.
|
||
|
||
**Sain, vérifié (pour ne pas re-chasser)** : aucune injection SQL (les seuls SQL
|
||
concaténés sont `PRAGMA table_info(...)` / `ALTER TABLE` dans `PM.Database`, sur
|
||
constantes internes) ; autorisation scopée par `user_id` **partout** sans
|
||
exception ; le `SET` dynamique de [Folders.pas:199](delphi-backend/Handlers/PM.Handler.Folders.pas:199)
|
||
est assemblé de littéraux fixes, pas d'input ; les routes non authentifiées sont
|
||
exactement les bonnes (register, login, challenge, ping, redeem, stub passkey) ;
|
||
`ValidateConnectingProcess` échoue **fermé** si le lookup PID rate ; primitives
|
||
crypto correctes + auto-testées à l'init. La fuite d'énumération de comptes de
|
||
`/login/challenge` est **documentée et assumée** ([Auth.pas:1189](delphi-backend/Handlers/PM.Handler.Auth.pas:1189))
|
||
→ pas comptée comme finding.
|
||
|
||
---
|
||
|
||
### 6c-1. 🔴 `/migrate-kdf` ne re-chiffre que 1 blob sur 8 — perte de données
|
||
|
||
> ✅ **RÉSOLU 2026-07-16 — chemin désactivé (option ultra).** Les 2 call-sites
|
||
> qui déclenchaient l'auto-migration au login/reauth
|
||
> ([app.js:1246, 1443](js/app.js)) sont retirés + la fonction morte
|
||
> `runKdfMigration` supprimée. Sans le client qui orchestre le swap de clé,
|
||
> l'endpoint serveur ne peut plus corrompre (il ne fait qu'un UPDATE de ce
|
||
> qu'on lui envoie) → laissé inerte, non touché. **DB vérifiée avant fix : les 2
|
||
> comptes sont déjà à 600k**, le chemin n'a donc jamais mordu. Un compte legacy
|
||
> <600k s'upgrade désormais via **Change Master Password** (qui re-chiffre les
|
||
> 8 blobs + attachments correctement et passe à Argon2id). §6c-4 (brick d'un
|
||
> compte -v2 par ce même endpoint) devient également inatteignable.
|
||
|
||
**C'est le seul vrai bug de perte de données de cette passe.**
|
||
|
||
La migration KDF passe les itérations 100k → 600k, donc **change la clé AES
|
||
dérivée** — puis [app.js:1035](js/app.js:1035) bascule `state.cryptoKey = newKey`.
|
||
Or [`runKdfMigration`](js/app.js:961) ne re-chiffre que `encrypted_password`/`iv`,
|
||
et [`HandleMigrateKdf`](delphi-backend/Handlers/PM.Handler.Auth.pas:795) n'UPDATE
|
||
que ces deux colonnes.
|
||
|
||
Restent chiffrés sous l'**ancienne** clé, donc définitivement illisibles :
|
||
`totp_secret`, `custom_fields`, `username_enc`, `site_enc`, `title_enc`,
|
||
`tags_enc`, `template_enc` (+ leurs IV), et les attachments.
|
||
|
||
Le contraste est net avec [`doChangeMasterPassword`](js/app.js:6106) /
|
||
[`HandleChangeMasterPassword`](delphi-backend/Handlers/PM.Handler.Auth.pas:1038),
|
||
qui affrontent **exactement la même situation** (changement de clé) et traitent
|
||
correctement les 8 colonnes. C'est une nouvelle instance de la classe de bug
|
||
décrite en **§3.3** — et la checklist « Entry payload » du CLAUDE.md cite
|
||
`doChangeMasterPassword` mais **pas** `runKdfMigration` : voilà par où c'est passé.
|
||
|
||
**Symptôme** : sournois, pas bruyant. [`decryptEntryMeta`](js/app.js:1502) ignore
|
||
silencieusement les champs qui ne déchiffrent pas (`if (v !== '[ERROR]') e[f] = v`)
|
||
→ titres/usernames deviennent `undefined` à l'écran, puis le premier save réécrit
|
||
ce vide par-dessus l'ancien ciphertext. Là c'est irréversible.
|
||
|
||
**Portée** : uniquement les comptes à `kdf_iterations < 600000`. Les inscriptions
|
||
partent directement à 600k ([app.js:1283](js/app.js:1283)) → legacy uniquement.
|
||
**Non vérifié** s'il en reste : aucun `vault.db` sur la machine de revue, pas de
|
||
CLI sqlite3. À trancher par `SELECT username, kdf_iterations, hash_algo FROM users;`
|
||
— si tout est à 600k, le bug n'a jamais mordu, mais le chemin reste **armé** et
|
||
auto-déclenché par signal serveur.
|
||
|
||
#### Correctif recommandé — extraire un helper partagé
|
||
|
||
Recopier la boucle de `doChangeMasterPassword` dans `runKdfMigration` serait
|
||
reproduire la cause (duplication, §3.3). La boucle
|
||
[app.js:6115-6179](js/app.js:6115) est **déjà exactement** la logique voulue :
|
||
l'extraire et l'appeler des deux côtés.
|
||
|
||
```js
|
||
// js/app.js — nouveau helper, unique source de vérité du re-chiffrement.
|
||
// Prend la clé courante (oldKey) et la cible (newKey), retourne le payload
|
||
// serveur. N'écrit RIEN dans state : l'appelant décide quand basculer.
|
||
async function reEncryptEntriesForNewKey(oldKey, newKey, onProgress) {
|
||
const out = [];
|
||
let done = 0;
|
||
for (const e of state.entries) {
|
||
done++;
|
||
if (onProgress) onProgress(done, state.entries.length);
|
||
|
||
state.cryptoKey = oldKey;
|
||
const plain = await decryptPwd(e.encrypted_password, e.iv);
|
||
if (plain === '[ERROR]') throw new Error('Could not decrypt entry id=' + e.id);
|
||
|
||
try {
|
||
state.cryptoKey = newKey;
|
||
const re = await encryptPwd(plain);
|
||
|
||
let totpEnc = '', totpIv = '';
|
||
if (e.totp_secret && e.totp_iv) {
|
||
state.cryptoKey = oldKey;
|
||
const plainTotp = await decryptTotpSecret(e.totp_secret, e.totp_iv);
|
||
state.cryptoKey = newKey;
|
||
if (plainTotp !== '[ERROR]') {
|
||
const t = await encryptPwd(plainTotp);
|
||
totpEnc = t.encrypted; totpIv = t.iv;
|
||
}
|
||
}
|
||
|
||
let cfEnc = '', cfIv = '';
|
||
if (e.custom_fields && e.custom_fields_iv) {
|
||
state.cryptoKey = oldKey;
|
||
const plainCf = await decryptCustomFields(e.custom_fields, e.custom_fields_iv);
|
||
state.cryptoKey = newKey;
|
||
if (plainCf.length > 0) {
|
||
const c = await encryptCustomFields(plainCf);
|
||
cfEnc = c.encrypted; cfIv = c.iv;
|
||
}
|
||
}
|
||
|
||
state.cryptoKey = newKey;
|
||
const meta = {};
|
||
for (const f of ENCRYPTED_META_FIELDS) {
|
||
if (e[f]) {
|
||
const c = await encryptPwd(e[f]);
|
||
meta[f + '_enc'] = c.encrypted;
|
||
meta[f + '_iv'] = c.iv;
|
||
} else {
|
||
meta[f + '_enc'] = '';
|
||
meta[f + '_iv'] = '';
|
||
}
|
||
}
|
||
|
||
out.push({
|
||
id: e.id,
|
||
encrypted_password: re.encrypted, iv: re.iv,
|
||
totp_secret: totpEnc, totp_iv: totpIv,
|
||
custom_fields: cfEnc, custom_fields_iv: cfIv,
|
||
...meta,
|
||
});
|
||
} finally {
|
||
state.cryptoKey = oldKey; // restauré tant que le serveur n'a pas confirmé
|
||
}
|
||
}
|
||
return out;
|
||
}
|
||
```
|
||
|
||
`doChangeMasterPassword` remplace sa boucle par un appel ; `runKdfMigration`
|
||
remplace la sienne :
|
||
|
||
```js
|
||
// js/app.js — dans runKdfMigration, branche `if (kdfChange)`
|
||
newKey = await deriveKey(masterPwd, state.salt, toIters);
|
||
newCiphertexts = await reEncryptEntriesForNewKey(state.cryptoKey, newKey);
|
||
```
|
||
|
||
⚠️ **Les attachments restent à traiter** : `doChangeMasterPassword` les gère dans
|
||
une boucle séparée *après* le round-trip serveur (GET blob → décrypt oldKey →
|
||
encrypt newKey → `PUT /attachments/:id`). `runKdfMigration` ne l'a pas du tout.
|
||
Même extraction à faire (`reEncryptAttachmentsForNewKey(oldKey, newKey)`), appelée
|
||
des deux côtés après le commit.
|
||
|
||
#### Côté Delphi — aligner l'UPDATE sur celui de la rotation
|
||
|
||
```pascal
|
||
// PM.Handler.Auth.pas — HandleMigrateKdf, remplace l'UPDATE de la ligne ~795.
|
||
// Colonnes identiques à HandleChangeMasterPassword (ligne ~1039) : c'est le
|
||
// même événement (la clé change), donc le même jeu de blobs doit voyager.
|
||
LQ.SQL.Text :=
|
||
'UPDATE vault_entries SET ' +
|
||
' encrypted_password = :ep, iv = :iv, ' +
|
||
' totp_secret = :ts, totp_iv = :tiv, ' +
|
||
' custom_fields = :cf, custom_fields_iv = :cfiv, ' +
|
||
' username_enc = :uenc, username_iv = :uiv, ' +
|
||
' site_enc = :senc, site_iv = :siv, ' +
|
||
' title_enc = :tenc, title_iv = :tiv2, ' +
|
||
' tags_enc = :genc, tags_iv = :giv, ' +
|
||
' template_enc = :tplenc, template_iv = :tpliv, ' +
|
||
' updated_at = CURRENT_TIMESTAMP ' +
|
||
'WHERE id = :id AND user_id = :uid';
|
||
```
|
||
|
||
…avec la même lecture de champs + binding que
|
||
[Auth.pas:1051-1077](delphi-backend/Handlers/PM.Handler.Auth.pas:1051).
|
||
|
||
#### Alternative stratégique (plus propre, plus de risque)
|
||
|
||
Un « migrate-kdf » **est** une rotation de master pw vers le même mot de passe.
|
||
Faire déléguer `runKdfMigration` à `/change-master-password` (nouveau salt +
|
||
`argon2id-v2`) supprimerait ce chemin dupliqué *et* ferait basculer les comptes
|
||
legacy sur Argon2id d'un coup — ce que §1.2 attend de toute façon. Contrepartie :
|
||
`HandleChangeMasterPassword` fait `DeleteAllUserSessions` + `CreateSession`
|
||
([Auth.pas:1161](delphi-backend/Handlers/PM.Handler.Auth.pas:1161)) et le CLAUDE.md
|
||
impose le wipe du blob PIN → une migration silencieuse de fond deviendrait
|
||
visible. À arbitrer ; le correctif chirurgical ci-dessus est le chemin bas risque.
|
||
|
||
#### Test de non-régression suggéré
|
||
|
||
La suite `js/tests/` couvre déjà le round-trip crypto. Ajouter : *entry avec TOTP
|
||
+ custom_fields + méta chiffrée → `reEncryptEntriesForNewKey` → tout déchiffre
|
||
sous newKey*. C'est le filet qui aurait attrapé ça.
|
||
|
||
---
|
||
|
||
### 6c-2. 🟠 Expiry de session écrite en local, lue en UTC (corrige §2.2)
|
||
|
||
> ✅ **RÉSOLU 2026-07-16.** `CreateSession` écrit `IncHour(NowUTC, 24)` et
|
||
> `Authenticate` compare `LExpires < NowUTC` ([PM.Session.pas](delphi-backend/Source/PM.Session.pas)).
|
||
> Les 3 lecteurs (Authenticate, RequireCSRF, CleanupExpired) sont désormais tous
|
||
> sur UTC. `NowUTC` + `System.DateUtils` déjà dans les uses → aucun uses touché.
|
||
> §2.2 est maintenant réellement clos (c'était le dernier `FormatDateTime(...,
|
||
> Now)` du backend). Sessions existantes : expirent sous 24 h, purgées au boot.
|
||
|
||
[`CreateSession`](delphi-backend/Source/PM.Session.pas:155) écrit
|
||
`FormatDateTime('yyyy-mm-dd hh:nn:ss', IncHour(Now, 24))` — heure **locale**.
|
||
C'est le dernier `FormatDateTime(..., Now)` du backend et il contredit la règle
|
||
du CLAUDE.md (« Timestamps = UTC partout … JAMAIS `FormatDateTime(..., Now)` »).
|
||
`NowUTCStr` existe et est utilisé partout ailleurs.
|
||
|
||
Trois lecteurs, deux bases de temps :
|
||
|
||
| Lecteur | Base | Cohérent ? |
|
||
|---|---|---|
|
||
| [`Authenticate`](delphi-backend/Source/PM.Session.pas:89) | `Now` (local) | oui |
|
||
| [`RequireCSRF`](delphi-backend/Source/PM.Session.pas:122) | `datetime('now')` (UTC) | **non** |
|
||
| [`CleanupExpired`](delphi-backend/Source/PM.Database.pas:469) | `datetime('now')` (UTC) | **non** |
|
||
|
||
**Pourquoi ça ne s'est jamais vu** : en UTC+1/+2 le décalage va dans le sens
|
||
inoffensif — `Authenticate` coupe à 24 h avant que la fenêtre CSRF (26 h) ne
|
||
serve. En **UTC-5** la fenêtre CSRF tombe à **19 h** : entre 19 h et 24 h la
|
||
session s'authentifie normalement, les GET passent, et **toute écriture répond
|
||
403 « Invalid CSRF token »**. Scénario plausible : « Start with Windows » + quick
|
||
unlock, machine laissée allumée plus d'une journée.
|
||
|
||
```pascal
|
||
// PM.Session.pas — CreateSession (~ligne 155)
|
||
// UTC comme partout ailleurs : RequireCSRF et CleanupExpired comparent contre
|
||
// SQLite datetime('now') qui EST UTC.
|
||
LExpires := FormatDateTime('yyyy-mm-dd hh:nn:ss', IncHour(NowUTC, 24));
|
||
```
|
||
|
||
```pascal
|
||
// PM.Session.pas — Authenticate (~ligne 89)
|
||
// Doit suivre : expires_at est désormais UTC.
|
||
if (LExpires <> 0) and (LExpires < NowUTC) then
|
||
```
|
||
|
||
`PM.Database` (qui expose `NowUTC`) est déjà dans le `uses` de l'interface,
|
||
`System.DateUtils` (pour `IncHour`) déjà dans celui de l'implémentation → aucun
|
||
`uses` à toucher. Les trois lecteurs s'alignent alors sur UTC.
|
||
|
||
**Note migration** : les sessions déjà en base gardent leur `expires_at` local.
|
||
Sans importance — elles expirent sous 24 h, et `CleanupExpired` les purge au
|
||
prochain démarrage.
|
||
|
||
---
|
||
|
||
### 6c-3. 🟡 Le CSRF est validé contre la mauvaise session
|
||
|
||
> ✅ **RÉSOLU 2026-07-16.** `RequireCSRF` lit désormais `csrf_token WHERE
|
||
> token_hash = :th AND user_id = :uid` (la session présentée), plus de
|
||
> `ORDER BY created_at DESC LIMIT 1`. `ExtractBearerToken`/`SHA256Hex` déjà
|
||
> utilisés par `Authenticate` dans la même unité ; signature inchangée, aucun
|
||
> call-site touché. Le re-test d'expiry est retiré (Authenticate tourne avant
|
||
> et l'a déjà fait, désormais sur la même base UTC après §6c-2).
|
||
|
||
[`RequireCSRF`](delphi-backend/Source/PM.Session.pas:120) cherche le
|
||
`csrf_token` par `user_id ORDER BY created_at DESC LIMIT 1` — la session la plus
|
||
récente, **pas celle réellement présentée**. Deux conséquences :
|
||
|
||
- `created_at` a une résolution d'**une seconde** : deux sessions créées dans la
|
||
même seconde donnent une égalité que SQLite tranche arbitrairement → il peut
|
||
lire le token de l'ancienne → **403 fantôme**.
|
||
- Conceptuellement, le contrôle devient « connais-tu *un* CSRF valide pour cet
|
||
utilisateur » au lieu de « celui lié à ta session ».
|
||
|
||
`Authenticate` a **déjà** localisé la bonne ligne par `token_hash` : lire
|
||
`csrf_token` depuis cette ligne-là est plus simple *et* correct. La signature ne
|
||
change pas → aucun call-site à toucher.
|
||
|
||
```pascal
|
||
// PM.Session.pas — RequireCSRF
|
||
var
|
||
LSubmitted, LStored, LTokenHash: string;
|
||
LQ: TFDQuery;
|
||
begin
|
||
if SameText(ARequest.Command, 'GET') then Exit;
|
||
|
||
LSubmitted := ARequest.RawHeaders.Values['X-CSRF-Token'];
|
||
if LSubmitted = '' then
|
||
begin
|
||
TJSONHelper.SendError(AResponse, 403, 'Missing CSRF token');
|
||
raise ESessionRejected.Create('missing csrf');
|
||
end;
|
||
|
||
// Lier le CSRF à la session PRÉSENTÉE, pas à la plus récente du user.
|
||
// AUserId reste en filtre (defense in depth).
|
||
LTokenHash := SHA256Hex(ExtractBearerToken(ARequest));
|
||
|
||
DB.Lock;
|
||
try
|
||
LQ := TFDQuery.Create(nil);
|
||
try
|
||
LQ.Connection := DB.Connection;
|
||
LQ.SQL.Text :=
|
||
'SELECT csrf_token FROM sessions ' +
|
||
'WHERE token_hash = :th AND user_id = :uid';
|
||
LQ.ParamByName('th').AsString := LTokenHash;
|
||
LQ.ParamByName('uid').AsInteger := AUserId;
|
||
LQ.Open;
|
||
// ... suite inchangée
|
||
```
|
||
|
||
L'expiry n'a plus à être re-testée ici : `Authenticate` tourne toujours avant et
|
||
l'a déjà fait (et après **6c-2** les deux sont sur la même base de temps).
|
||
|
||
---
|
||
|
||
### 6c-4. 🟡 `/migrate-kdf` rétrograderait un compte `-v2` (verrouillage définitif)
|
||
|
||
> ⛔ **SANS OBJET depuis 6c-1 (2026-07-16).** Le chemin migrate-kdf est
|
||
> désactivé côté client (fonction `runKdfMigration` supprimée, triggers
|
||
> retirés). L'endpoint reste inerte (rien ne l'appelle) → ce scénario de brick
|
||
> n'est plus atteignable. Garde à ré-ajouter uniquement si migrate-kdf revenait
|
||
> un jour.
|
||
|
||
Le garde d'idempotence ([Auth.pas:722](delphi-backend/Handlers/PM.Handler.Auth.pas:722))
|
||
ne court-circuite que si l'algo vaut `pbkdf2-sha256`. Un compte `argon2id-v2` ou
|
||
`pbkdf2-sha256-v2` **passe au travers** et se fait réécrire en
|
||
`HASH_ALGO_CURRENT` ([Auth.pas:763](delphi-backend/Handlers/PM.Handler.Auth.pas:763)
|
||
et [785](delphi-backend/Handlers/PM.Handler.Auth.pas:785)) → la formule de
|
||
verifier change → **lockout définitif**.
|
||
|
||
Non atteignable aujourd'hui : le register envoie `kdfIterations: 600000`
|
||
([app.js:1296](js/app.js:1296)) donc le login ne signale jamais de migration pour
|
||
un compte v2 — et le commentaire [app.js:1016](js/app.js:1016) montre que le cas a
|
||
été raisonné. Mais l'endpoint est authentifié, CSRF'é et **armé** ; le garde coûte
|
||
4 lignes et rend la protection locale au lieu de dépendre de l'appelant.
|
||
|
||
```pascal
|
||
// PM.Handler.Auth.pas — HandleMigrateKdf, juste avant le garde d'idempotence
|
||
// existant (~ligne 722).
|
||
// Les schemes -v2 ont un verifier découplé : migrate-kdf ne sait pas les
|
||
// réécrire sans changer la formule → refuser au lieu de bricker le compte.
|
||
if LAlgo.EndsWith('-v2', True) then
|
||
begin
|
||
TJSONHelper.SendOK(AResponse, 'Already at target');
|
||
Exit;
|
||
end;
|
||
```
|
||
|
||
---
|
||
|
||
### 6c-5. 🔵 Fallback silencieux sur params Argon2 invalides
|
||
|
||
[Auth.pas:319](delphi-backend/Handlers/PM.Handler.Auth.pas:319) (register) et
|
||
[Auth.pas:994](delphi-backend/Handlers/PM.Handler.Auth.pas:994) (rotation) :
|
||
|
||
```pascal
|
||
if SameText(LReqAlgo, HASH_ALGO_ARGON2) and LArgon.Valid then ... else
|
||
LRegAlgo := HASH_ALGO_DECOUPLED; // ← fallback muet
|
||
```
|
||
|
||
Un client qui déclare `argon2id-v2` avec des params hors bornes est stocké en
|
||
**PBKDF2** sans un mot, alors qu'il a dérivé sa clé en **Argon2id** → au login
|
||
suivant `/login/challenge` renvoie `pbkdf2-sha256-v2`, le client dérive en PBKDF2,
|
||
verifier différent → **compte brické**. Le JS envoie toujours
|
||
`ARGON2_DEFAULT_PARAMS` (valides) donc ce n'est pas live ; échouer fort coûte
|
||
5 lignes.
|
||
|
||
```pascal
|
||
// Aux DEUX endroits : un client qui DEMANDE Argon2id avec des params hors
|
||
// bornes doit être refusé, pas silencieusement rétrogradé en PBKDF2.
|
||
if SameText(LReqAlgo, HASH_ALGO_ARGON2) and (not LArgon.Valid) then
|
||
begin
|
||
TJSONHelper.SendError(AResponse, 400, 'Invalid argon2 params');
|
||
Exit;
|
||
end;
|
||
```
|
||
|
||
---
|
||
|
||
### 6c-6. 🔵 Le port « aléatoire » dépend d'un `Randomize` qui n'est pas le nôtre
|
||
|
||
[PM.HTTPServer.pas:136](delphi-backend/Source/PM.HTTPServer.pas:136) utilise
|
||
`Random()`, et le projet n'appelle **jamais** `Randomize` (vérifié : 0 occurrence
|
||
dans tout le `.dpr` + `.pas`). `RandSeed` vaut 0 par défaut → séquence
|
||
déterministe.
|
||
|
||
Empiriquement le port **varie** bien (le CLAUDE.md documente le wipe de
|
||
`localStorage` à chaque lancement, ce qui le prouve) → une unité RTL/Indy sème
|
||
`RandSeed` en initialisation. Ça marche donc **par accident** : dépendance
|
||
implicite, sur ce que le design traite comme une mesure de sécurité, alors que
|
||
`RandomBytes` (CNG) est déjà utilisé **6 lignes plus haut** pour le token.
|
||
|
||
```pascal
|
||
// PM.HTTPServer.pas — Start (~ligne 136)
|
||
// CSPRNG (CNG) plutôt que Random() : ne dépend pas d'un Randomize appelé par
|
||
// une unité tierce. 49152..65535 = 16384 valeurs, et 65536 mod 16384 = 0
|
||
// → pas de biais modulo.
|
||
if LRequestedPort = 0 then
|
||
begin
|
||
var LRnd := PM.Crypto.RandomBytes(2);
|
||
LRequestedPort := EphemeralPortMin +
|
||
(((LRnd[0] shl 8) or LRnd[1]) mod (EphemeralPortMax - EphemeralPortMin + 1));
|
||
end;
|
||
```
|
||
|
||
---
|
||
|
||
### 6c-7. 🔵 Comparaison du token d'accès : sous-chaîne + non constant-time
|
||
|
||
[PM.HTTPServer.pas:258](delphi-backend/Source/PM.HTTPServer.pas:258) :
|
||
|
||
```pascal
|
||
if (CookieHeader <> '') and (Pos(CookieName + '=' + FAccessToken, CookieHeader) > 0)
|
||
```
|
||
|
||
`Pos` matche **n'importe où** dans le header et accepte les **préfixes** :
|
||
`pm_token=<token>junk` passe, et un cookie nommé `xpm_token=<token>` aussi. Ni ça
|
||
ni le `=` de la ligne 262 ne sont constant-time, alors que `ConstantTimeEquals`
|
||
existe et sert déjà pour le CSRF. Impact faible derrière le PID check (il faut
|
||
déjà connaître le token), mais c'est une incohérence gratuite.
|
||
|
||
```pascal
|
||
// PM.HTTPServer.pas — parse réel du header Cookie plutôt qu'un Pos() sur la
|
||
// chaîne brute, + comparaison constant-time (comme RequireCSRF).
|
||
function TryGetCookie(const AHeader, AName: string; out AValue: string): Boolean;
|
||
var
|
||
LPairs: TArray<string>;
|
||
LPair, LName: string;
|
||
LEq: Integer;
|
||
begin
|
||
Result := False;
|
||
AValue := '';
|
||
LPairs := AHeader.Split([';']);
|
||
for LPair in LPairs do
|
||
begin
|
||
LEq := Pos('=', LPair);
|
||
if LEq <= 0 then Continue;
|
||
LName := Trim(Copy(LPair, 1, LEq - 1));
|
||
if SameText(LName, AName) then
|
||
begin
|
||
AValue := Trim(Copy(LPair, LEq + 1, MaxInt));
|
||
Exit(True);
|
||
end;
|
||
end;
|
||
end;
|
||
```
|
||
|
||
```pascal
|
||
// ValidateAccessToken — remplace les deux comparaisons
|
||
if TryGetCookie(CookieHeader, CookieName, LCookieVal) and
|
||
ConstantTimeEquals(LCookieVal, FAccessToken) then
|
||
Exit(True);
|
||
|
||
QueryToken := ARequest.Params.Values[QueryParamName];
|
||
if (QueryToken <> '') and ConstantTimeEquals(QueryToken, FAccessToken) then
|
||
begin
|
||
// ... Set-Cookie inchangé
|
||
```
|
||
|
||
(`PM.Crypto` est déjà dans le `uses` de `PM.HTTPServer` → rien à ajouter.)
|
||
|
||
---
|
||
|
||
### 6c-8. 🔵 Fuite mémoire sur chemin d'erreur
|
||
|
||
[Attachments.pas:292](delphi-backend/Handlers/PM.Handler.Attachments.pas:292) :
|
||
`LArr := TJSONArray.Create` est créé **avant** le `try/finally` → une exception
|
||
dans le bloc DB le fuit. `SendJSON` ne le libère que sur le chemin nominal.
|
||
|
||
```pascal
|
||
// PM.Handler.Attachments.pas — HandleListAllAttachments
|
||
LArr := TJSONArray.Create;
|
||
try
|
||
DB.Lock;
|
||
try
|
||
// ... remplissage inchangé
|
||
finally
|
||
DB.Unlock;
|
||
end;
|
||
except
|
||
LArr.Free;
|
||
raise;
|
||
end;
|
||
TJSONHelper.SendJSON(AResponse, LArr); // libère LArr (AOwnsObj=True)
|
||
```
|
||
|
||
---
|
||
|
||
### 6c-9. ❓ `PRAGMA foreign_keys` — à vérifier, non tranché
|
||
|
||
> ✅ **RÉSOLU 2026-07-16.** `FConn.Params.Values['ForeignKeys'] := 'On'` ajouté
|
||
> à la config de connexion FireDAC ([PM.Database.pas](delphi-backend/Source/PM.Database.pas)).
|
||
> Correct dans les deux cas (no-op si c'était déjà le défaut). **Vérif base
|
||
> avant fix : 0 attachment / 0 history orphelin** — soit le défaut FireDAC était
|
||
> déjà On, soit aucun delete-permanent-avec-attachments n'a encore eu lieu ;
|
||
> l'explicite lève le doute pour de bon.
|
||
|
||
`PRAGMA foreign_keys` n'est défini **nulle part**. Le schéma s'appuie pourtant sur
|
||
`ON DELETE CASCADE` : [Entries.pas:704](delphi-backend/Handlers/PM.Handler.Entries.pas:704)
|
||
(delete permanent) ne nettoie **ni** `entry_attachments` **ni**
|
||
`entries_password_history` à la main, et le CLAUDE.md affirme « FK cascade on user
|
||
+ entry delete ».
|
||
|
||
SQLite met `foreign_keys` à **OFF** par défaut. Le paramètre `ForeignKeys` de
|
||
FireDAC est *censé* défauter à `On` (ce qui sauverait la mise), mais **je n'ai pas
|
||
pu le confirmer** : les DCU ne sont pas greppables et il n'y avait pas de base pour
|
||
tester. Si c'était Off, chaque suppression définitive laisserait des blobs chiffrés
|
||
orphelins — exactement ce que `CompactIfBloated` tente de rattraper.
|
||
|
||
Deux façons de lever le doute :
|
||
|
||
```sql
|
||
-- Sur un vault.db réel, après avoir supprimé définitivement une entry
|
||
-- qui avait des attachments :
|
||
SELECT COUNT(*) FROM entry_attachments a
|
||
WHERE NOT EXISTS (SELECT 1 FROM vault_entries e WHERE e.id = a.entry_id);
|
||
-- > 0 → la cascade ne s'applique pas.
|
||
```
|
||
|
||
```pascal
|
||
// PM.Database.pas — TPMDatabase.Create, avec les autres Params (~ligne 82).
|
||
// Explicite = plus de dépendance à un défaut FireDAC non documenté chez nous.
|
||
FConn.Params.Values['ForeignKeys'] := 'On';
|
||
```
|
||
|
||
Le second est correct **dans les deux cas** (no-op si c'était déjà le défaut) →
|
||
à appliquer sans attendre la vérification.
|
||
|
||
---
|
||
|
||
### Ordre d'application suggéré
|
||
|
||
| # | Sévérité | Effort | État (2026-07-16) |
|
||
|---|---|---|---|
|
||
| **6c-1** | 🔴 | — | ✅ **Fait** — chemin migrate-kdf désactivé (option ultra) au lieu de réparé. |
|
||
| **6c-2** | 🟠 | — | ✅ **Fait** — `expires_at` en UTC. Ferme §2.2. |
|
||
| **6c-3** | 🟡 | — | ✅ **Fait** — CSRF lié à la session présentée (token_hash). |
|
||
| **6c-9** | ❓ | — | ✅ **Fait** — `ForeignKeys=On` explicite. |
|
||
| **6c-4** | 🟡 | — | ⛔ **Sans objet** — l'endpoint migrate-kdf est inatteignable depuis 6c-1. |
|
||
| **6c-5** | 🔵 | Trivial | À faire — 5 lignes × 2 endroits (fail-fast params Argon2). |
|
||
| **6c-6/7/8** | 🔵 | Faible | À faire — cohérence / hygiène, aucune urgence. |
|
||
|
||
Rappel build : tout edit JS impose `delphi-backend\assets\BuildAssets.cmd` **puis**
|
||
le build Delphi, sinon l'exe garde les anciens assets.
|
||
|
||
---
|
||
|
||
## 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 ».
|
||
|
||
**MAJ 2026-07-16** : la revue backend Delphi (**§6c**) ne remet pas en cause ce
|
||
verdict sur le **durcissement** — aucune injection, autorisation scopée partout,
|
||
routes non authentifiées correctes, crypto saine. Le **🔴 de perte de données**
|
||
(`/migrate-kdf` re-chiffrait 1 blob sur 8, **§6c-1**) est **traité** : chemin
|
||
désactivé (les 2 comptes étaient déjà à 600k, jamais mordu). Faits aussi :
|
||
expiry session UTC (**§6c-2**, clôt §2.2), CSRF lié à la bonne session
|
||
(**§6c-3**), `foreign_keys` explicite (**§6c-9**). Reste du confort/hygiène
|
||
(**§6c-5/6/7/8**), sans urgence.
|