Repository navigation
fix(security): suite de la remédiation de l'audit de sécurité V1 - #4720
Conversation
« false », « 0 » ou une valeur vide valent non, une valeur ambiguë bloque le démarrage. Les valeurs déployées (envoi des emails, TLS SMTP, mode lecture seule) gardent leur sens actuel.
… entrants L'app n'utilise pas WebSocket : les requêtes Upgrade sont refusées avant Next.js. Les en-têtes Content-Security-Policy et X-Nonce venant du client ne sont jamais transmis à l'app.
Montée 8 → 11 (corrige @opentelemetry/core). Cookies, en-têtes, corps de requête, query strings et données utilisateur ne sont pas collectés. La configuration serveur et edge est chargée depuis instrumentation.ts, mode debug coupé.
script-src n'autorise plus 'unsafe-inline' : chaque réponse porte un nonce aléatoire, et les en-têtes CSP ou nonce envoyés par le client sont écrasés. La page d'accueil est rendue par requête pour recevoir le nonce.
Une PR hebdomadaire groupe les montées mineures et correctives des dépendances directes, les majeures restent manuelles.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![iterion-forge-egapro[bot]](https://avatars.githubusercontent.com/in/4896217?s=60&v=4){kind=small}
There was a problem hiding this comment.
Revi — 1 problème à corriger.
1 medium
À confirmer
- Was client-side Sentry verified after the v11 bump (a production browser session showing the client SDK still initializes)? The build plugin's injection of
sentry.client.config.tsis the legacy mechanism on Next 14 (instrumentation-client.ts needs Next 15.3+), and since debug is now off a silent injection failure would mean client error monitoring disappears with no visible symptom.
Détails du run IA · Iterion
Run : 01a10b14-1975-7878-8b4d-82d00c9de0aa
Tokens du run au moment de publier : 118 595.
Périmètre revu : correctness: packages/api/egapro/config.py (parse_bool + init bool branch), packages/api/test/test_config.py ; correctness: packages/app/src/middleware.ts (nonce generation, request/response header propagation, CSP construction) ; correctness: packages/app/src/app/(default)/page.tsx force-dynamic vs root layout headers() usage (no other force-static route remains) ; security: packages/nginx/nginx.conf (upgrade guard, CSP/X-Nonce header stripping, coverage of all proxy locations) ; security: packages/app/src/common/sentryDataCollection.ts + sentry.{client,edge,server}.config.ts (PII collection off, tunnel kept) ; security/maintainability: packages/app/src/instrumentation.ts (per-runtime config import paths), packages/app/next.config.js (withSentryConfig v11 shape, hidden source maps + delete-after-upload) ; dependencies: pnpm-lock.yaml diff (Sentry 8→11 tree swap only, @opentelemetry/api 1.9.0→1.9.1), packages/app/package.json ; tests: packages/app/src/tests/middleware.test.ts (nonce uniqueness, forged CSP/nonce overwrite, dev-only unsafe-eval) ; maintainability: .github/dependabot.yml (target-branch master matches this PR's base branch)
Revue mono (single model family).
Identifiants des findings : Rcb2640.
| Étape | Modèle | Harness | Effort demandé | Tokens |
|---|---|---|---|---|
| Revue GLM | glm-5.3 | claude_code | max | 82 918 |
| Synthèse · GLM | glm-5.3 | claude_code | medium | 35 677 |
Tokens cumulés des appels IA, pas la taille du contexte. L’effort indiqué est le réglage demandé au modèle.
maxgfr
left a comment
maxgfr
left a comment
There was a problem hiding this comment.
Rien de bloquant côté CSP, Sentry, nginx ou API. Vérifié en lecture seule sur la review app, sur 12 pages (accueil, recherche, simulateur, déclaration, login, /stats, une 404) :
- tous les
<script>portent le nonce, et il change à chaque requête ; - un en-tête
Content-Security-PolicyouX-Nonceenvoyé par le client n'est pas repris ; - une requête
Upgrade: websocketreçoit un 426,/healthzrépond 200 ; - aucun
.mapn'est servi.
Important
Dependabot npm reste inactif (.github/dependabot.yml:11-13). La branche par défaut du dépôt est alpha, et Dependabot ne lit sa configuration que sur la branche par défaut : ce fichier sur master est ignoré. Il faut plutôt ajouter, dans le dependabot.yml d'alpha, une entrée npm avec target-branch: master (deux entrées pour le même écosystème et le même dossier sont acceptées si leur target-branch diffère), sans appliquer le groupe "*" à l'entrée qui cible alpha (cohérence de version ultra11y).
https://*.gouv.fr dans script-src limite l'apport du nonce (middleware.ts:29 et :35). Non exploité, mais plausible : une injection HTML peut charger un script depuis n'importe quel sous-domaine gouv.fr qui sert du JS contrôlable, ou poser un <base href> vers un tel hôte (autorisé par base-uri), et les chunks Next noncés seraient alors chargés depuis là. Suggestion : 'strict-dynamic' (tous les scripts rendus côté serveur sont déjà noncés, et Matomo comme les chunks sont injectés par du JS de confiance) ou la seule origine Matomo à la place de *.gouv.fr, et base-uri 'self'.
À confirmer : EGAPRO_SMTP_SSL en prod (config.py:83-84). Si la valeur stockée était "False" ou "0", l'ancien bool() activait STARTTLS (emails/__init__.py:58) et le nouveau parseur le coupe : le login SMTP partirait en clair. Dev, preprod et docker-compose gardent bien leur sens, mais je n'ai pas pu vérifier la prod.
Check « Test e2e » : les deux échecs précédents viennent du délai de l'étape « Wait for app to be ready » (180 s écoulées avant la fin du déploiement), pas d'un test. Le run actuel est encore en cours.
Mineurs
config.py:58-59:TRUE_VALUES/FALSE_VALUESsont en majuscules, doncinit()les traite comme des réglages, et une variableEGAPRO_TRUE_VALUESpeut les écraser. Le préfixe_ne suffit pas ("_X".isupper()vautTrue) : les passer en minuscules ou les définir dans la fonction.test_config.py:104-113:config.init()modifie l'état global, etSMTP_SSLreste àTruepour les tests suivants. Restaurer avecmonkeypatch.setattr.nginx.conf:113-120:- le 426 et la suppression d'en-têtes ne s'appliquent qu'à
location /(pas exploitable, nginx ne transmet pasUpgradepar défaut), etproxy_set_header Upgrade ""est redondant ; - Next lit aussi
Content-Security-Policy-Report-Onlyen repli : autant le vider aussi.
- le 426 et la suppression d'en-têtes ne s'appliquent qu'à
middleware.ts:50:x-nonceest aussi renvoyé dans la réponse, alors que seul l'en-tête de requête sert.- Sentry :
dataCollectionne couvre que la collecte automatique.captureError(..., headers: safeHeaders(req.headers))(middleware.ts:97) envoie toujoursx-forwarded-for/x-real-ip. C'était déjà le cas avant, mais ça contredit « en-têtes non collectés » ; une liste blanche serait plus sûre. next.config.js:116:debug: trueest resté sur le plugin de build Sentry.
Nits
page.tsx:8:force-dynamicest inutile, le layout racine appelle déjàheaders().common/config.ts:11(hors diff) : le getternoncerenvoie le SHA git, un « nonce » public et statique, utilisé nulle part. À supprimer pour éviter le piège.
Bien vu : nonce via crypto.randomUUID() posé sur la requête et la réponse, en-têtes entrants écrasés côté middleware et supprimés côté nginx, nonce repris par react-dsfr, Matomo et les scripts Next, pas de double CSP, migration Sentry 11 propre (options obsolètes retirées, sourcemaps masquées), tests sur l'en-tête falsifié et les booléens invalides.
…les flavours de dev
…utes les locations
… en liste blanche
|
Merci pour la relecture. Tout est traité dans 8c86241…3f8f94a9f. Important
Mineurs
Nits
|
|
Review actualisée après lecture des changements jusqu’au commit Contexte de maintenance : 1. Dependabot V1 — point résoluL’ajout des mises à jour npm hebdomadaires sur 2. E2E — correction relue, résultat CI encore attenduLe lancement précédent sur La route AvisReview de code approuvée. Aucun autre défaut bloquant démontré dans le périmètre relu, y compris les derniers ajustements CSP, Nginx, Sentry et configuration API. Cette approbation ne vaut pas constat de réussite des contrôles encore en cours. Analyse fondée sur le diff, les sources des dépendances et les logs/captures CI précédents ; suites complètes non relancées localement. |
Viczei
left a comment
Viczei
left a comment
There was a problem hiding this comment.
Review de code approuvée après relecture du commit 3f8f94a9f1687e53ab557a7f611cdcb937594d92 et prise en compte du gel de la V1. Les deux points et leur état sont détaillés ici : #4720 (comment)
L’ajout Dependabot V1 a été retiré et la correction du nettoyage E2E a été relue. Les contrôles CI encore en cours restent à vérifier avant fusion.
maxgfr
left a comment
maxgfr
left a comment
There was a problem hiding this comment.
Je valide, mais j'ai peur que la montée de version de Sentry casse des trucs au niveau des source maps ou autre, à vérifier
|
🎉 This PR is included in version 3.18.7 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This branch was successfully deployed
3 participants
Contexte
Suite de la remédiation de l'audit de sécurité de la V1 (#4681).
Changements
API : réglages booléens lus strictement
EGAPRO_SEND_EMAILS,EGAPRO_SMTP_SSLetEGAPRO_READONLYétaient convertis avecbool(), donc"false"valait vrai. Ils acceptent maintenant1/true/yes/onet0/false/no/off/vide, et toute autre valeur bloque le démarrage. Les valeurs déployées en prod, en preprod et sur les review apps ont été vérifiées : elles gardent leur sens actuel. Le vocabulaire du parseur et la liste des flavours autorisées à garder les secrets de dev ne peuvent plus être redéfinis par une variableEGAPRO_*.nginx : connexions upgradées et en-têtes CSP entrants
L'app n'utilise pas WebSocket : les requêtes
Upgradesont refusées (426) sur toutes les locations, avant d'atteindre Next.js. Les en-têtesContent-Security-Policy,Content-Security-Policy-Report-OnlyetX-Nonceenvoyés par le client ne sont jamais transmis à l'app.Sentry 8 → 11
@sentry/nextjspasse en v11, ce qui corrige@opentelemetry/core.dataCollectioncoupe la collecte des cookies, en-têtes, corps de requête, query strings et données utilisateur, et les en-têtes joints aux erreurs du middleware passent en liste blanche. Les configurations serveur et edge sont chargées depuisinstrumentation.ts, le SDK ne les injectant plus. Le mode debug est coupé, retiré des bundles et du plugin de build.CSP à nonce pour les scripts
script-srcn'autorise plus'unsafe-inline'ni aucun hôte : le middleware génère un nonce par requête, écrase tout en-tête CSP ou nonce venant du client, et'strict-dynamic'ne fait confiance qu'aux scripts noncés et à ceux qu'ils chargent. Next.js, react-dsfr et Matomo reprennent le nonce, qui n'est jamais renvoyé au navigateur.base-uriest limité à'self'. La page d'accueil n'est plus forcée en statique, une page pré-rendue ne pouvant pas porter de nonce.style-srcgarde'unsafe-inline', que le DSFR exige pour ses attributsstyle.E2E
test-loginrattache le compte de test à son SIREN, comme une connexion ProConnect. Sans ce rattachement, le nettoyage entre deux tests ne supprimait aucune déclaration, et la première déclaration créée faisait échouer tous les tests suivants.Vérification
tsc,lint, 509 tests Jest,next build.nginx -t) et testée en conteneur, 426 et en-têtes vidés sur chaque location.