feat(policy-engine): add safe affinity-based V2 - #3
Conversation
Tetrax
left a comment
There was a problem hiding this comment.
Final technical review — BLOCKED
Review boundary: PR #3 at b0a28eef201f3e9fdbb752014936553b3ad77928 against current main@17187da75fccc56d413c2824c0c7028aab2f4a11.
Blocking security findings
- Selected-policy coverage is not recomputed. Selecting one of two required V2 policies still returns preflight
exactwith0errors. The final gate proves every selected rule is observed, but not that all required tuples remain covered. A required flow can therefore be omitted while certification stays exact. - Sequence mode reintroduces an interface Cartesian product. Two exact policies on interface pairs A→X and B→Y are aggregated into src interfaces
[A,B]and dst interfaces[X,Y], which authorizes four pairs. Preflight checks interface existence but evidence matching ignores interfaces and returnsexact. - Named ICMP can be falsely certified exact. A log
service=PINGwith no type/code is accepted against any configured ICMP object with the same name, even when that object is type 3/code 1. The probe returned0blockers, coverage100 %and preflightexact.
Delivery blockers
- GitHub reports the PR
CONFLICTING/DIRTY. Current main contains security hardening absent from the branch.git merge-treereports a content conflict inapp/web/test/server-dependencies.test.js; resolution must preserve both the V2 endpoint/import assertions and the main-branch reverse-proxy security assertion. - There are no GitHub checks. The 85 green tests apply to the stale branch head, while current main's 63 tests apply separately. The combined Node 26 / updated dependency / security workflow result has not been executed.
- The isolated Docker image/smoke evidence is tagged
bd15702, while the PR head isb0a28ee; it does not contain the final ICMP commit or current main's Node 26 security base image.
Medium
/api/policy-engine/v2synchronously recomputes the full engine on the Node HTTP thread and is absent from main's costly-route rate-limiter list. The real dataset takes about 1.5 s per call; repeated requests can stall health/API handling.
Gates observed
- PR head tests:
85/85pass. - Current main tests in isolated archive:
63/63pass. - JavaScript syntax, Compose rendering, diff checks and added-line secret/static scans pass.
npm audit --audit-level=criticalpasses but reports 11 non-critical findings on the stale branch dependencies; current main updates Multer/ws and adds the critical Trivy/npm gates.- GitHub CI: no checks reported.
Required before merge
- reconcile the branch with current main and resolve the test conflict additively;
- gate selected-policy coverage against the complete required tuple set;
- remove/disable V2 sequence aggregation or include exact interface-pair evidence in grouping, metrics and preflight;
- require ICMP semantic equality (type/code or another authoritative fingerprint), not name/protocol alone;
- rate-limit/cache/offload the V2 endpoint;
- run the full combined suite and GitHub security workflow.
- build and smoke an immutable image from the final reconciled merge SHA.
No code was modified during this review.
Addendum to final review — confirmed performance blockerThe interrupted contradictory review contained a For one source with All runs still returned Because Severity: High / merge-blocking for the stated large-dataset requirement. Required targeted correction: construct an indexed |
# Conflicts: # app/web/test/server-dependencies.test.js
Résumé
FlowAtom, l’affinité source/destination/service et un optimizer déterministe par rectangles sûrs ;ALL/ALL_ICMP;Findings de la review finale — corrigés
main@17187da, hardening, dépendances, Node 26, rate limiting et tests sécurité conservés.buildAffinityViews()utilise un indexservice → destinations, sansgroup.some()imbriqué.Vérifications locales
Benchmark matrice d’affinité :
Tous les benchmarks de sécurité restent à
missing=0etunexpected=0pour Recommandé/Strict.Dataset réel
Dernière baseline complète préservée :
Les 7 tuples ICMP sans preuve suffisante restent visibles et bloqués ; aucun élargissement automatique n’est ajouté. Le cache de session complet utilisé pour cette baseline n’est plus présent sur le runtime, donc cette mesure historique n’est pas réinventée depuis le workspace legacy incomplet.
Déploiement
Après merge et CI verte : construire une image immuable depuis le SHA mergé, déployer en parallèle sur un port alternatif avec données isolées, exécuter healthchecks/smoke/dataset disponible, puis basculer la stack existante avec
fortiflow:17187daconservée comme rollback.