• Joined on 2026-05-18
Herald commented on pull request qpismont/herald#7 2026-09-17 20:58:15 +02:00
1.2: Sandboxing

Épingler l'image de base par digest plutôt que par tag flottant, et vérifier que l'image de sandbox ne contient pas de secrets ni d'outils inutiles (surface d'attaque réduite, utilisateur non-root).

Herald commented on pull request qpismont/herald#7 2026-09-17 20:58:15 +02:00
1.2: Sandboxing

Les outils exposés à l'agent doivent valider leurs entrées (chemins relatifs, pas de .., pas de shell arbitraire) et documenter clairement le contrat JSON de chaque outil ; c'est la principale surface d'attaque du sandboxing.

Herald commented on pull request qpismont/herald#7 2026-09-17 18:21:45 +02:00
1.2: Sandboxing

Review Feedback

Herald commented on pull request qpismont/herald#7 2026-09-17 18:21:45 +02:00
1.2: Sandboxing

Passage en workspace multi-crates : vérifier que les dépendances communes sont déclarées dans [workspace.dependencies] et réutilisées avec workspace = true dans les crates, pour éviter les divergences de versions. Ajouter aussi les métadonnées de package communes (edition, license, version) via [workspace.package].

Herald commented on pull request qpismont/herald#7 2026-09-17 18:21:45 +02:00
1.2: Sandboxing

Pipeline CI : vérifier que cargo fmt --check, cargo clippy -- -D warnings et cargo test --all-features couvrent bien le nouveau workspace multi-crates (utiliser --workspace), et que les tests du sandbox qui nécessitent Docker/root sont soit exécutés, soit explicitement ignorés avec justification.

Herald commented on pull request qpismont/herald#7 2026-09-17 18:21:45 +02:00
1.2: Sandboxing

Nouvelles variables d'environnement : valider leur présence et leur format au démarrage (échouer tôt avec un message explicite) plutôt que de laisser une valeur par défaut silencieuse. Prévoir aussi une valeur par défaut sûre (fail-closed) pour tout réglage désactivant l'isolation.

Herald commented on pull request qpismont/herald#7 2026-09-17 18:21:45 +02:00
1.2: Sandboxing

Exécution d'outils dans le sandbox : s'assurer que les commandes passent par une liste blanche explicite (allowlist de binaires + arguments validés) et jamais par un shell (sh -c) avec interpolation de chaînes issues du contenu de la PR. Les entrées utilisateur (diff, messages, noms de fichiers) doivent être passées comme arguments séparés, avec validation/échappement. Vérifier également l'absence de Command::new construit à partir d'une variable non validée.

Herald commented on pull request qpismont/herald#7 2026-09-17 18:21:45 +02:00
1.2: Sandboxing

La documentation du sandbox doit préciser ce qui est isolé et ce qui ne l'est pas (réseau, filesystem, credentials), comment le configurer via l'environnement, et rappeler que l'exécution de code issu d'une PR est une opération à risque. Ajouter si possible un exemple de variable d'environnement sécurisé.

Herald commented on pull request qpismont/herald#7 2026-09-17 18:21:45 +02:00
1.2: Sandboxing

Build de production : privilégier un build multi-étapes pour ne pas embarquer toolchain et sources dans l'image finale, exécuter le binaire avec un utilisateur non privilégié, et épingler les dépendances de l'étape de build. Ajouter un ENTRYPOINT/CMD explicite et un label de version.

Herald commented on pull request qpismont/herald#7 2026-09-17 18:21:45 +02:00
1.2: Sandboxing

Épingler l'image de base par digest (ou au moins une version majeure mineure) pour la reproductibilité, et éviter d'exécuter le conteneur de développement en root si possible. Vérifier que les outils ajoutés dans cette image ne sont pas supposés présents en production (le devcontainer ne doit pas être une dépendance implicite du runtime).

Herald commented on pull request qpismont/herald#7 2026-09-17 18:21:45 +02:00
1.2: Sandboxing

Les nouvelles variables doivent être documentées ici avec un commentaire indiquant leur rôle et une valeur d'exemple non sensible. Vérifier qu'aucune vraie clé/token n'a été committée et que le fichier reste aligné avec les variables réellement lues dans env.rs.

Herald commented on pull request qpismont/herald#7 2026-09-17 18:21:45 +02:00
1.2: Sandboxing

Module OpenRouter : la clé d'API doit être lue depuis l'environnement/state (jamais codée en dur, jamais loggée). Vérifier que les erreurs renvoyées par l'API HTTP ne propagent pas la clé ni le contenu complet des prompts dans les logs ou dans le commentaire publié. Ajouter un timeout et une gestion des codes 429/5xx avec retry borné.

Herald commented on pull request qpismont/herald#7 2026-09-17 18:21:45 +02:00
1.2: Sandboxing

Attention aux chemins : tous les fichiers créés ou montés dans le sandbox doivent être confinés sous une racine contrôlée (canonicalisation + vérification de préfixe) pour empêcher toute traversée de répertoire (.., liens symboliques). Un chemin dérivé d'un nom de fichier d'une PR est une entrée non fiable.

Herald commented on pull request qpismont/herald#7 2026-09-17 18:21:45 +02:00
1.2: Sandboxing

Nouveau module sandbox : ajouter en tête de fichier une documentation (doc-comment) décrivant le modèle de menace et le niveau d'isolation réellement appliqué (namespaces, cgroups, utilisateur non-root, montages read-only, limites CPU/RAM/pid). Sans cela, il est impossible de valider que le nom « sandbox » correspond à une isolation effective et non à un simple répertoire de travail. Préciser aussi le comportement en cas d'échec d'initialisation de l'isolation (échec fermé plutôt qu'ouvert).

Herald commented on pull request qpismont/herald#7 2026-09-17 18:21:45 +02:00
1.2: Sandboxing

Ajouter la configuration Zed tout en supprimant .vscode/settings.json crée une incohérence pour les contributeurs : il vaut mieux conserver les deux (ou documenter l'éditeur recommandé) et s'assurer qu'aucun chemin absolu local ni préférence personnelle n'est committé ici.

Herald commented on pull request qpismont/herald#7 2026-09-17 18:21:45 +02:00
1.2: Sandboxing

Adaptation aux nouveaux chemins de crates : profiter du déplacement pour vérifier que les imports internes utilisent bien crate:: (et non des chemins relatifs fragiles) et qu'aucun use mort ne subsiste après la réorganisation.

Herald commented on pull request qpismont/herald#7 2026-09-17 18:21:45 +02:00
1.2: Sandboxing

La suppression complète de .dockerignore est problématique : le contexte de build inclura désormais target/, .git/, .env et autres fichiers locaux, ce qui alourdit et surtout peut faire fuiter des secrets dans l'image via une copie du contexte. Si le fichier a été remplacé ailleurs, le référencer explicitement ; sinon le restaurer.

Herald commented on pull request qpismont/herald#7 2026-09-17 18:21:45 +02:00
1.2: Sandboxing

Centraliser les valeurs magiques dans un module de constantes est une bonne pratique : s'assurer que les valeurs liées au sandbox (timeouts, limites mémoire, chemins) sont bien documentées par un commentaire indiquant l'unité (secondes, octets) et la raison du choix.

Herald commented on pull request qpismont/herald#7 2026-09-17 18:21:45 +02:00
1.2: Sandboxing

Action de review : ajouter des tests unitaires sur la construction du prompt, la gestion des erreurs et les cas limites (contenu vide, réponse LLM tronquée, échec du sandbox). Prévoir aussi une limite de taille de la réponse publiée dans Gitea pour éviter de dépasser les quotas de l'API.

Herald commented on pull request qpismont/herald#7 2026-09-17 18:21:45 +02:00
1.2: Sandboxing

Le diff du lockfile est volumineux : vérifier qu'il ne contient que des changements attendus (nouvelles dépendances réellement utilisées) et pas d'ajout transitif superflu, puis confirmer que le build est reproductible en CI avec --locked.