fix: critical vulnerabilities in all packages#1797
Conversation
Signed-off-by: Yash Oswal <yoswal@redhat.com>
AI Code ReviewReviewer: Auto (Cursor agent router)
Executive Summary
The PR makes real progress reducing npm audit findings across the workspace, but it introduces CI regressions, leaves critical vulnerabilities in several packages, and includes upgrades incompatible with the repo’s Node 16 engine constraint. What This PR Does Well
Blockers1. CI failures (8 packages)
The PR checklist marks “Did tests pass?” as unchecked, which aligns with CI. 2. PR claims do not fully match outcomesTitle: “fix: critical vulnerabilities in all packages” The included audit table still shows 29 critical vulnerabilities remaining:
Lockfile migration claim: “All lockfiles are migrated to v2 (~node 16)” The diff shows 9 lockfiles moving from v1→v2 and 9 remaining at v1. This is a partial migration, not a complete one. Detailed FindingsA. Mongoose 5 → 6 migration (5 services)Affected: Good: Deprecated connection options were removed correctly. Missing:
Risk: Mongoose 6 has additional behavioral changes (e.g. B. Node 16 engine incompatibilityRoot "engines": { "node": "^16.18.0", "npm": "^8.1.0" }Several upgrades appear to require Node 18+:
Recommendation: Either revert these to Node-16-compatible versions or bump repo engines + CI Node version in the same PR. C. TypeScript /
|
| Change | Concern |
|---|---|
Root cypress 6.5.0 → 15.16.0 |
Major version; may need config migration |
feedback-service nodemailer 6.7.0 → ^8.0.10 |
Major version; verify email sending still works |
feedback-service lodash 4.17.21 → ^4.18.1 |
Unusual — lodash 4.17.x is the stable line; confirm 4.18.x exists and is intended |
api-catalog-spa @one-platform/opc-feedback prerelease → alpha |
Verify API compatibility with consuming code |
Recommendations
Must fix before merge
- Resolve all 8 CI failures
- Revert or align Jest 30 / Cypress 15 / happy-dom 20 with Node 16, or bump engines to Node 18+ with CI workflow updates
- Update Mongoose 6 tests (
insertOne/insertMany, review query patterns) - Pin
@types/nodeto TS-4.x-compatible versions; removeskipLibCheckworkaround - Fix peer dependency conflicts in Angular, Apollo, and ESLint stacks
Should fix / document
- Update PR title and description to reflect remaining critical counts honestly
- Correct the “all lockfiles migrated to v2” claim
- Add a manual test plan for Mongoose 6 services (CRUD, search indexing, feedback queues, user-group LDAP)
- Address or document remaining critical vulns in
api-gateway-service(11) and SPAs
Consider for follow-up PRs
- Split into smaller PRs: (a) lockfile-only patch bumps, (b) Mongoose 6 per-service, (c) major dev-tool upgrades with Node bump
Checklist Assessment
| Item | Status | Notes |
|---|---|---|
| All files related to one feature | ✅ | Cohesive audit remediation |
| Unit tests | Existing tests broken in apps-service; no new tests added |
|
| Tests pass | ❌ | 8/19 CI jobs failing |
| Documentation updated | ❌ | Claims inaccurate (lockfiles, “all” criticals) |
| Breaking change assessment | “Might be needs testing” — underspecified for Mongoose 6 + major tool bumps |
Final Verdict
Request changes. The vulnerability reduction effort is valuable, but the PR is not ready to merge due to widespread CI failures, remaining critical vulnerabilities in several packages, Node 16 incompatibilities, and incomplete Mongoose 6 migration. Recommend fixing blockers or splitting into smaller, reviewable PRs.
Generated by Auto (Cursor agent router), powered by Composer. Assisted-by: Composer
deshmukhmayur
left a comment
There was a problem hiding this comment.
See my previous AI review comment^
Resolves
Resolves multiple Critical vulns reported by npm audit
Previous
After fixing vulnerabilities
Does this PR introduce a breaking change
Might be needs testing
Ready-for-merge Checklist