From 3cc189c5348c15e46fdf919d73efccb83a21d977 Mon Sep 17 00:00:00 2001 From: Kevin L'Hereec Date: Thu, 23 Jul 2026 10:53:13 +0200 Subject: [PATCH] fix(security): close IDOR on computers/components update & destroy - Scope update/destroy queries by userId (Computers & Components), matching the pattern already used in index/show/edit - Add missing auth.authenticate() calls on destroy actions - handleComponents now requires userId and scopes component lookups to the current user, preventing cross-user component takeover - handleComponents rejects attaching more than one component of the same 'unique' type to a computer (new InvalidComponentSelectionException, 422) - handlePictures scopes picture lookups by computerId - destroy(Computer) now detaches its components (computerId = null) instead of losing them to cascade delete, and removes the computer's uploads dir - Switch store/update actions to request.validateUsing() for consistency - Fix relative redirects to absolute paths (/computers, /computers/:id) - Untrack .env.production and add it to .gitignore (secrets must not be committed); local file kept on disk --- .gitignore | 1 + .kilo/plans/remediation-plan.md | 320 ++++++++++++++++++ app/controllers/components_controller.ts | 14 +- app/controllers/computers_controller.ts | 39 ++- .../invalid_component_selection_exception.ts | 17 + app/services/component_service.ts | 40 ++- app/services/picture_service.ts | 10 +- 7 files changed, 417 insertions(+), 24 deletions(-) create mode 100644 .kilo/plans/remediation-plan.md create mode 100644 app/exceptions/invalid_component_selection_exception.ts diff --git a/.gitignore b/.gitignore index ae28bf2..d8a215f 100644 --- a/.gitignore +++ b/.gitignore @@ -9,6 +9,7 @@ dist # Secrets .env .env.local +.env.production .env.production.local .env.development.local .env.test diff --git a/.kilo/plans/remediation-plan.md b/.kilo/plans/remediation-plan.md new file mode 100644 index 0000000..46c6232 --- /dev/null +++ b/.kilo/plans/remediation-plan.md @@ -0,0 +1,320 @@ +# Plan de remédiation — PC Builder + +Basé sur la revue de code du 2026-07-23. Objectif : corriger les failles de +sécurité critiques avant toute mise en production, puis assainir la base du +code (majeur), puis traiter les points mineurs/qualité. + +--- + +## Phase 1 — CRITIQUE (bloquant avant prod) + +### 1.1 IDOR sur `update`/`destroy` (Computers & Components) + +**Problème** : `computers_controller.ts` (`update` L92-103, `destroy` L105-110) +et `components_controller.ts` (`destroy` L77-82) chargent la ressource via +`findOrFail(id)` sans filtrer par `userId`. `destroy` des deux controllers +n'authentifie même pas l'utilisateur (`auth.authenticate()` absent). N'importe +quel utilisateur connecté peut modifier/supprimer les PC ou composants d'un +autre utilisateur. + +**Remédiation** : +- `computers_controller.ts#update` : ajouter `auth.authenticate()`, remplacer + `Computer.findOrFail(computerId)` par + `Computer.query().where('user_id', user.id).andWhere('id', computerId).firstOrFail()` + (même pattern que `show`/`edit`). +- `computers_controller.ts#destroy` : ajouter `auth.authenticate()`, même + pattern de query scopée par `user_id`. +- `components_controller.ts#destroy` : ajouter `auth.authenticate()`, remplacer + `Component.findOrFail(componentId)` par une query scopée + `where('userId', user.id).andWhere('id', componentId).firstOrFail()`. +- Vérifier les routes (`start/routes.ts`) : `middleware.auth()` est déjà + appliqué sur les groupes `/computers` et `/components`, donc l'utilisateur + est authentifiable dans le contexte — juste manquant l'appel explicite + + le filtre. + +**Fichiers impactés** : +- `app/controllers/computers_controller.ts` +- `app/controllers/components_controller.ts` + +### 1.2 IDOR transitif via `handleComponents` / `handlePictures` + +**Problème** : même après la 1.1, `component_service.ts#handleComponents` +(L11, L18) et `picture_service.ts#handlePictures` (L22, L47) font +`findOrFail(id)` sans vérifier que le composant/l'image appartient à +l'utilisateur courant. Un attaquant authentifié peut poster un `componentsId` +contenant l'ID d'un composant appartenant à un autre utilisateur pour +se l'approprier (`component.computerId = computer.id`). + +**Remédiation** : +- `handleComponents(componentsId, computer, userId)` : ajouter le paramètre + `userId`, et scoper les requêtes `Component.query().where('userId', userId).andWhere('id', id).firstOrFail()` + au lieu de `Component.findOrFail(id)`. Si un ID demandé n'appartient pas à + l'utilisateur, lever une erreur explicite (404 ou 403) plutôt que d'échouer + silencieusement. +- `handlePictures` : scoper `ComputerPicture.findOrFail(picture.id)` par + `computerId` du computer courant (`.where('computerId', computer.id)`), pour + empêcher de manipuler des images d'un autre computer/utilisateur. +- Mettre à jour les appels dans `computers_controller.ts#store` et `#update` + pour passer `user.id`. + +**Fichiers impactés** : +- `app/services/component_service.ts` +- `app/services/picture_service.ts` +- `app/controllers/computers_controller.ts` (appels à `handleComponents`) + +### 1.3 Secrets commités : `.env.production` + +**Problème** : `.env.production` est suivi par git et contient `APP_KEY`, +`DB_PASSWORD`, `DB_USER`, `DB_DATABASE` en clair (commit `c822872`). Le CI +(`.gitea/workflows/build.yml`) le lit et le déploie tel quel sur le NAS. + +**Remédiation** (à faire avec le user — actions sensibles) : +1. **Révoquer/régénérer immédiatement** les secrets réels côté NAS : + nouveau `APP_KEY` (`node ace generate:key`), nouveau mot de passe MySQL, + éventuellement recréer l'utilisateur DB. +2. Ajouter `.env.production` au `.gitignore` (garder uniquement + `.env.production.example` comme gabarit, déjà présent et sans valeurs + réelles). +3. Retirer le fichier du suivi git : `git rm --cached .env.production`. +4. Purger l'historique git du secret (`git filter-repo` ou BFG Repo-Cleaner) + — **à valider avec l'utilisateur** car cela réécrit l'historique partagé + (impact sur les autres clones/branches). +5. Migrer la distribution des secrets de prod vers les **Secrets Gitea Actions** + (comme `secrets.NASPASS`) plutôt qu'un fichier committé : injecter les + variables via `${{ secrets.APP_KEY }}`, `${{ secrets.DB_PASSWORD }}`, etc., + et générer `.env` à la volée dans le job de déploiement. + +**Fichiers impactés** : +- `.gitignore` +- `.gitea/workflows/build.yml` +- `.env.production` (suppression du suivi) +- Historique git (opération destructive, confirmation utilisateur requise) + +### 1.4 Secrets en ligne de commande dans le pipeline CI (NASPASS) + +**Problème** : `.gitea/workflows/build.yml` L46-56 utilise `sshpass -p` et +`sudo -S` avec le secret NAS directement en argument de commande shell — +risque d'exposition dans les logs/process list. + +**Remédiation** : +- Remplacer l'authentification par mot de passe par une **clé SSH dédiée** + stockée en secret Gitea (`secrets.NAS_SSH_KEY`), utilisée via + `ssh -i <(echo "$NAS_SSH_KEY")` ou `webfactory/ssh-agent`-like action. +- Configurer un accès `sudo` sans mot de passe restreint aux commandes + nécessaires (`visudo` avec `NOPASSWD` limité aux scripts `setup-db.sh`, + `docker compose`, `docker exec`) plutôt que de faire transiter le mot de + passe utilisateur via `sudo -S`. +- À défaut de refonte complète immédiate, au minimum masquer la commande + (`set +x` explicite, éviter `-x` en debug) et documenter le risque résiduel. + +**Fichiers impactés** : +- `.gitea/workflows/build.yml` +- Configuration NAS (hors dépôt — à faire manuellement) + +### 1.5 Pas de contrainte d'unicité applicative pour les composants "uniques" + +**Problème** : `ComponentType.unique` (ex: un seul CPU par PC) n'est vérifié +nulle part côté serveur. Un POST direct sur `/computers` ou `/computers/:id` +avec plusieurs composants du même type "unique" dans `componentsId` est accepté. + +**Remédiation** : +- Dans `handleComponents`, après résolution des composants à ajouter, grouper + par `typeId` (en préchargeant `type.unique`) et rejeter (erreur 422/exception + métier) si plus d'un composant du même type "unique" seraient attachés au + computer simultanément. +- Ajouter un test automatisé couvrant ce cas (voir Phase 4). + +**Fichiers impactés** : +- `app/services/component_service.ts` + +--- + +## Phase 2 — MAJEUR (avant la prochaine itération fonctionnelle) + +### 2.1 Redirections relatives fragiles + +**Problème** : `computers_controller.ts#store` (L46) fait +`response.redirect('computers/')` et `#update` (L102) fait +`response.redirect(`${computer.id}`)` — chemins relatifs incohérents avec +le reste du code (`/components`, `/auth/login`, etc.). + +**Remédiation** : remplacer par des chemins absolus explicites : +- `store` → `response.redirect('/computers')` +- `update` → `response.redirect(`/computers/${computer.id}`)` + +### 2.2 Incohérence `request.all()` vs `request.validateUsing()` + +**Problème** : `components_controller.ts` et `computers_controller.ts` +utilisent `request.all()` + `xxxValidator.validate(data)` au lieu du helper +standard `request.validateUsing(validator)` utilisé par +`session_controller.ts`/`new_account_controller.ts`. Risque d'incohérence +sur le flash des erreurs de validation Inertia (`inputErrorsBag`). + +**Remédiation** : uniformiser tous les controllers sur +`await request.validateUsing(xxxValidator)`, supprimer les appels manuels à +`.validate()` sur `request.all()`. + +**Fichiers impactés** : +- `app/controllers/computers_controller.ts` (store, update) +- `app/controllers/components_controller.ts` (store, update) + +### 2.3 Validateurs d'inscription dupliqués et incohérents + +**Problème** : `app/validators/Auth/register.ts` (`registerValidator`) n'est +utilisé nulle part (code mort) ; `new_account_controller.ts` utilise +`signupValidator` de `app/validators/user.ts`. Les deux ont des règles +différentes sur `name` (obligatoire minLength 3 vs nullable sans contrainte). + +**Remédiation** : +- Supprimer `app/validators/Auth/register.ts` (code mort), ou le fusionner + si une seconde route d'inscription est prévue. +- Décider de la règle métier définitive sur `name` (obligatoire ou non) dans + `signupValidator` et l'appliquer. + +### 2.4 Formulaire `computers/create.vue` ne se soumet pas via Enter / pas d'état de chargement + +**Problème** : le bouton "Create" est un `` externe au +`
` (pas de `@submit.prevent`), contrairement à `components/create.vue` +qui utilise `useForm` + `@submit.prevent` correctement. + +**Remédiation** : refactorer `computers/create.vue` pour utiliser +`useForm()` d'Inertia avec `@submit.prevent="form.post('/computers')"`, +afficher `form.processing` sur le bouton (état de chargement), harmoniser +avec le pattern de `components/create.vue`. + +### 2.5 `origin` fantôme dans `components/edit.vue` + +**Problème** : la page lit `props.origin` et le renvoie dans le payload, +mais `components_controller.ts#edit` ne le partage jamais dans +`inertia.render(...)`. + +**Remédiation** : soit supprimer complètement `origin` du formulaire et du +type de props si la fonctionnalité est abandonnée, soit l'implémenter +proprement si un besoin réel existe (ex: retour à la page d'origine après +modification). À trancher avec l'utilisateur — proposition : suppression +(simplification), sauf avis contraire. + +### 2.6 Suppression d'un `Computer` : cascade destructive sur les composants + fichiers orphelins + +**Problème** : `destroy` d'un `Computer` supprime en cascade DB ses +`Component` liés au lieu de les détacher (`computerId = null`), ce qui fait +perdre définitivement les composants de l'utilisateur. De plus, les fichiers +images physiques (`uploads/computer_X/...`) ne sont jamais nettoyés lors +d'un `destroy` de computer (seulement lors d'un retrait explicite via update). + +**Remédiation** : +- Dans `computers_controller.ts#destroy`, avant `computer.delete()` : + détacher les composants (`UPDATE components SET computer_id = NULL WHERE computer_id = ?`) + et supprimer physiquement le dossier `uploads/computer_${id}` (+ les lignes + `computer_pictures`, qui elles peuvent rester en cascade DB puisque les + fichiers sont supprimés en même temps). +- Vérifier/adapter la contrainte `onDelete('CASCADE')` sur `components.computer_id` + dans la migration si on souhaite un `SET NULL` réellement porté par la DB + plutôt qu'un traitement applicatif (nécessiterait une migration corrective). + +**Fichiers impactés** : +- `app/controllers/computers_controller.ts` +- Éventuellement une nouvelle migration si `onDelete` doit changer. + +### 2.7 Duplication `ComponentsViewer.vue` / `ComputersViewer.vue` + +**Remédiation** : extraire un composant générique `FilterDropdown.vue` +(props : liste d'options `{id, label}`, `v-model` sur l'état de filtre, +slot ou prop pour le libellé du bouton) réutilisé dans les deux vues, pour +éliminer la duplication du markup dropdown + logique `toggleDropdown`. + +### 2.8 Composants "widgets" orphelins et fonctionnalité galerie incomplète + +**Problème** : `ComponentEditor.vue`, `ComputerComponentsTable.vue`, +`SimpleGallery.vue`, `AuthErrorAlert.vue` ne sont référencés par aucune page +active. La gestion de galerie photo (`SimpleGallery.vue` + `picture_service.ts`) +n'est appelée par aucun formulaire actuel (`computers/create.vue`/`edit.vue` +n'ont pas de champ pictures). + +**Remédiation** — décision à prendre avec l'utilisateur : +- **Option A (simplification)** : supprimer ces composants orphelins et le + code backend de gestion d'images s'ils ne sont plus dans la roadmap. +- **Option B (finaliser la fonctionnalité)** : intégrer `SimpleGallery.vue` + dans `computers/create.vue`/`edit.vue` pour rendre l'upload d'images + réellement fonctionnel, et réintégrer `ComponentEditor.vue` (gestion + propre des composants uniques via select) à la place des checkboxes brutes + actuelles — ce qui réglerait aussi une partie du problème 1.5 côté UX. + +--- + +## Phase 3 — MINEUR (qualité, hygiène) + +### 3.1 Sécurité de configuration +- `config/session.ts` L45 : remplacer `secure: false` par `secure: app.inProduction`. +- `config/shield.ts` L12 : activer une CSP de base (`csp.enabled: true`) + avec des directives minimales (`default-src 'self'`, autoriser les sources + Vite en dev). + +### 3.2 Typage frontend +- Remplacer les `any` dans `computers/*.vue` et `components/*.vue` par les + types réels (`Computer`, `Component`, `ComputerState`, `ComponentType` + importés depuis `#models/*` comme le fait déjà `ComponentsViewer.vue`). +- Corriger/aligner `inertia/pages/types/UITypes.ts` avec le schéma backend + réel (`typeId` au lieu de `componentTypeId`, retirer `componentStateId` + inexistant) ou supprimer ce fichier si les vrais modèles Lucid suffisent. + +### 3.3 Encodage UTF-8 cassé (mojibake) +- Corriger l'encodage dans `ComponentsViewer.vue`, `ComponentEditor.vue`, + `ComputerComponentsTable.vue`, `TableComponent.vue` (caractères `├®`, `Ôé¼` + etc. à réécrire en UTF-8 propre : é, à, ô, €). + +### 3.4 Accessibilité +- Corriger les doublons d'`id` HTML dans `ComputersViewer.vue` (L82/94, + deux fois `id="used"`). +- Ajouter `aria-expanded`/`aria-haspopup` sur les boutons de dropdown filtres. +- Remplacer `CustomSelect.vue` par un `