Repository navigation
Conversation
Registra login fallido, respuestas 403 y cambios de estado en pedidos y asignaciones, sanitizando tokens y contraseñas en los logs.
Normaliza claves sensibles camelCase, protege metadatos reservados, evita log injection, audita asignaciones fuera de la transacción y evita que fallos de logging afecten respuestas.
Usa replaceAll, omite claves reservadas sin variables sin usar, valida la línea JSON y escribe por stdout para mitigar log injection.
Add a dedicated security-audit job that fails on high/critical vulnerabilities and document how to manage approved exceptions.
|
Warning Review limit reached
More reviews will be available in 33 minutes and 25 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (20)
📝 WalkthroughWalkthroughSe implementa un sistema integral de auditoría de seguridad que sanitiza automáticamente eventos sensibles, captura fallos de autenticación y cambios de estado en órdenes/asignaciones, y valida dependencias de seguridad en CI con soporte para excepciones documentadas. ChangesSistema Integral de Auditoría de Seguridad
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthCtrl as session.controllers
participant Logger as logSecurityEvent
participant Sanitize as sanitizeForLog
participant Stdout as [SECURITY] log
Client->>AuthCtrl: login(email, password)
alt missing credentials
AuthCtrl->>Logger: LOGIN_FAILED<br/>reason: missing_credentials
else invalid user
AuthCtrl->>Logger: LOGIN_FAILED<br/>reason: invalid_credentials
else password mismatch
AuthCtrl->>Logger: LOGIN_FAILED<br/>reason: invalid_credentials
end
Logger->>Sanitize: sanitizeForLog(details)
Sanitize->>Sanitize: redact password/token fields
Sanitize->>Sanitize: remove control chars
Sanitize-->>Logger: sanitized
Logger->>Stdout: [SECURITY] {event, details, timestamp}
Logger-->>AuthCtrl: logged
AuthCtrl-->>Client: UnauthorizedError
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/modules/delivery/delivery-assignments/delivery-assignments.service.js (1)
290-305:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFalta el evento
ORDER_STATUS_CHANGEDal completar la entrega.Dentro de la transacción la orden pasa a
DELIVERED(líneas 290-293), pero solo se registraASSIGNMENT_STATUS_CHANGED. Esto es inconsistente con la ramaACCEPTderespondToAssignmentService, donde sí se emiten ambos eventos (asignación y orden). Para mantener la cobertura de auditoría uniforme, conviene registrar también el cambio de estado de la orden.♻️ Posible ajuste para auditar el cambio de orden
logSecurityEvent("ASSIGNMENT_STATUS_CHANGED", { assignmentId: id_delivery_assignment, orderId: assignment.fk_order, deliveryId: assignment.fk_delivery, previousStatus: "ACCEPTED", newStatus: "DELIVERED", actorUserId: id_user, }); + + logSecurityEvent("ORDER_STATUS_CHANGED", { + orderId: assignment.fk_order, + newStatus: "DELIVERED", + actorUserId: id_user, + actorRole: "DELIVERY", + source: "assignment_complete", + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/delivery/delivery-assignments/delivery-assignments.service.js` around lines 290 - 305, La transacción actual actualiza la orden a DELIVERED pero no emite el evento ORDER_STATUS_CHANGED; antes de llamar a tx.orders.update() lee el estado actual de la orden (por ejemplo con tx.orders.findUnique({ where: { id_order: assignment.fk_order } })) para capturar previousStatus, luego después de la transacción invoca logSecurityEvent("ORDER_STATUS_CHANGED", { orderId: assignment.fk_order, previousStatus: <valor_obtenido>, newStatus: "DELIVERED", actorUserId: id_user }) — mantén además el evento ASSIGNMENT_STATUS_CHANGED existente (identificadores relevantes: tx.orders.update, assignment.fk_order, logSecurityEvent, id_delivery_assignment, id_user).src/modules/users/orders/order.service.js (1)
16-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winArreglar los mocks de Prisma: agregá
orders.counten e2e (paragenerateOrderNumber)El CI falla porque
generateOrderNumber()llamaprismaOrTx.orders.count(...)(líneas ~16-17) y en los mocks de Prisma usados por los tests e2eprisma.ordersno definecount, causandoTypeError: prismaOrTx.orders.count is not a functionpropagado pormapOrderResponseycreateOrderService. En los unit tests (tests/unit/order/order.test.js)orders.countsí existe; el faltante está en e2e (por ejemplotests/e2e/stock.test.jsy otros comotests/e2e/orders-available-deliveries.test.js,tests/e2e/notifications.test.js,tests/e2e/delivery-assignments-*.test.js, etc.).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/users/orders/order.service.js` around lines 16 - 17, La falla viene de que generateOrderNumber usa prismaOrTx.orders.count(...) pero los mocks de Prisma en los tests e2e no exponen orders.count; actualizar los mocks e2e para definir orders.count (método que acepta el mismo filtro que en producción) y devolver un número apropiado (p. ej. 0 o el valor esperado por los tests). Localizá los mocks compartidos usados por los e2e (donde se crea el objeto prisma/mockPrisma) y añadí una implementación para orders.count; esto garantiza que generateOrderNumber, mapOrderResponse y createOrderService no lancen TypeError durante los tests.
🧹 Nitpick comments (2)
.github/workflows/vitest-supertest.yml (1)
8-21: ⚡ Quick winConsiderar hardening de seguridad del job según análisis estático.
El análisis estático (zizmor) identificó varias oportunidades de hardening de seguridad en el job
security-audit:
Acciones sin pinning a commit hash: Las acciones
actions/checkout@v4yactions/setup-node@v4usan tags en lugar de hashes de commit específicos, lo que podría permitir ataques de supply chain si el tag se modifica.Permisos por defecto amplios: El job no especifica un bloque
permissions:, heredando permisos por defecto que podrían ser más amplios de lo necesario.Credenciales persistentes: No se establece
persist-credentials: falseen el checkout, lo que podría exponer credenciales en artifacts.🔒 Hardening propuesto
security-audit: name: Security audit (high/critical) timeout-minutes: 10 runs-on: ubuntu-latest + permissions: + contents: read steps: - uses: actions/checkout@v4 + with: + persist-credentials: false - uses: actions/setup-node@v4 with: node-version: lts/* cache: 'npm'Para pinning a commit hash (opcional según política organizacional):
- - uses: actions/checkout@v4 + - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4 - - uses: actions/setup-node@v4 + - uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/vitest-supertest.yml around lines 8 - 21, The security-audit job should be hardened: replace tag-based action references (actions/checkout@v4, actions/setup-node@v4) with pinned commit SHAs or organization-approved digests; add a minimal permissions: block for the job to grant only the least privileges required (e.g., restrict to read-only where possible) instead of inheriting defaults; and set persist-credentials: false on the actions/checkout step to avoid leaking runner credentials to subsequent steps or artifacts; update the job named security-audit and the checkout/setup-node steps accordingly.src/modules/delivery/delivery-assignments/delivery-assignments.service.js (1)
94-100: 💤 Low valueTransiciones intermedias dentro de la transacción no quedan auditadas.
El evento nuevo cubre la creación de la asignación pendiente, pero dentro de la transacción también se cambian estados que no se registran: el
pendingAssignmentpasado aREJECTED(líneas 44-47) y el cierre/reasignación víaclosePendingAndReassign(..., "EXPIRED")(línea 51). Si el objetivo de la auditoría es trazar todas las transiciones de estado, considerá emitir eventos también para esos casos.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/delivery/delivery-assignments/delivery-assignments.service.js` around lines 94 - 100, La creación actual solo emite logSecurityEvent("ASSIGNMENT_STATUS_CHANGED") para newDeliveryAssignment; además debe emitirse un evento cuando el pendingAssignment se marca como REJECTED y cuando se invoca closePendingAndReassign(..., "EXPIRED"). Añadí llamadas a logSecurityEvent("ASSIGNMENT_STATUS_CHANGED") justo después de la actualización que pone pendingAssignment a "REJECTED" (usando pendingAssignment.id_delivery_assignment / fk_order / fk_delivery y previousStatus como su estado previo), y otra llamada dentro o inmediatamente después de closePendingAndReassign(...) para registrar la transición a "EXPIRED" (usando el id retornado o el assignment afectado y newStatus "EXPIRED"), asegurando consistencia en los campos previousStatus/newStatus y que los eventos se emitan dentro de la misma transacción.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/security-logger.js`:
- Around line 20-42: Reemplaza el uso de String.prototype.replaceAll por
alternativas compatibles con Node versiones LTS en normalizeKey y
sanitizeLogString: en normalizeKey intercambia replaceAll(/([a-z0-9])([A-Z])/g,
...) por String.prototype.replace con la misma expresión global, y cambia
replaceAll("-", "_") por split("-").join("_") (o replace con /-/g); en
sanitizeLogString usa replace con patrones globales (o split/join) en lugar de
replaceAll para CONTROL_CHARS_PATTERN, JWT_PATTERN y BEARER_PATTERN; además, si
prefieres garantizar la API, añade engines.node: ">=15" en package.json como
alternativa documentada. Asegúrate de que SENSITIVE_KEYS, isSensitiveKey y el
flujo de normalización sigan usando normalizeKey tras los cambios.
---
Outside diff comments:
In `@src/modules/delivery/delivery-assignments/delivery-assignments.service.js`:
- Around line 290-305: La transacción actual actualiza la orden a DELIVERED pero
no emite el evento ORDER_STATUS_CHANGED; antes de llamar a tx.orders.update()
lee el estado actual de la orden (por ejemplo con tx.orders.findUnique({ where:
{ id_order: assignment.fk_order } })) para capturar previousStatus, luego
después de la transacción invoca logSecurityEvent("ORDER_STATUS_CHANGED", {
orderId: assignment.fk_order, previousStatus: <valor_obtenido>, newStatus:
"DELIVERED", actorUserId: id_user }) — mantén además el evento
ASSIGNMENT_STATUS_CHANGED existente (identificadores relevantes:
tx.orders.update, assignment.fk_order, logSecurityEvent, id_delivery_assignment,
id_user).
In `@src/modules/users/orders/order.service.js`:
- Around line 16-17: La falla viene de que generateOrderNumber usa
prismaOrTx.orders.count(...) pero los mocks de Prisma en los tests e2e no
exponen orders.count; actualizar los mocks e2e para definir orders.count (método
que acepta el mismo filtro que en producción) y devolver un número apropiado (p.
ej. 0 o el valor esperado por los tests). Localizá los mocks compartidos usados
por los e2e (donde se crea el objeto prisma/mockPrisma) y añadí una
implementación para orders.count; esto garantiza que generateOrderNumber,
mapOrderResponse y createOrderService no lancen TypeError durante los tests.
---
Nitpick comments:
In @.github/workflows/vitest-supertest.yml:
- Around line 8-21: The security-audit job should be hardened: replace tag-based
action references (actions/checkout@v4, actions/setup-node@v4) with pinned
commit SHAs or organization-approved digests; add a minimal permissions: block
for the job to grant only the least privileges required (e.g., restrict to
read-only where possible) instead of inheriting defaults; and set
persist-credentials: false on the actions/checkout step to avoid leaking runner
credentials to subsequent steps or artifacts; update the job named
security-audit and the checkout/setup-node steps accordingly.
In `@src/modules/delivery/delivery-assignments/delivery-assignments.service.js`:
- Around line 94-100: La creación actual solo emite
logSecurityEvent("ASSIGNMENT_STATUS_CHANGED") para newDeliveryAssignment; además
debe emitirse un evento cuando el pendingAssignment se marca como REJECTED y
cuando se invoca closePendingAndReassign(..., "EXPIRED"). Añadí llamadas a
logSecurityEvent("ASSIGNMENT_STATUS_CHANGED") justo después de la actualización
que pone pendingAssignment a "REJECTED" (usando
pendingAssignment.id_delivery_assignment / fk_order / fk_delivery y
previousStatus como su estado previo), y otra llamada dentro o inmediatamente
después de closePendingAndReassign(...) para registrar la transición a "EXPIRED"
(usando el id retornado o el assignment afectado y newStatus "EXPIRED"),
asegurando consistencia en los campos previousStatus/newStatus y que los eventos
se emitan dentro de la misma transacción.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 420207fd-80e2-4ba1-8316-544b11e5fabd
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
.github/workflows/vitest-supertest.ymlsecurity/AUDIT_EXCEPTIONS.mdsecurity/audit-ci.jsonsrc/app.jssrc/lib/security-logger.jssrc/middlewares/security-log.middleware.jssrc/modules/delivery/delivery-assignments/delivery-assignments.service.jssrc/modules/session/controllers/session.controllers.jssrc/modules/users/orders/order.service.jstests/unit/lib/security-logger.test.jstests/unit/middlewares/security-log.middleware.test.js
Address valid review findings with minimal changes: replace replaceAll for LTS compatibility, add missing order status audit logging in assignment completion flow, harden security-audit job permissions, and update e2e Prisma mocks for orders.count.
Evita sobrescribir req.params al validar varios DTOs y actualiza aserciones al nuevo formato de errores de validacion.
|
2 participants
Objetivo
Implementar control de seguridad en CI para que el pipeline falle cuando se detecten vulnerabilidades
highocritical.Cambios
security-audital workflow de backend.audit-cicon política desdesecurity/audit-ci.json.security/AUDIT_EXCEPTIONS.md.package-lock.jsonpara reducir hallazgos de seguridad.Política de excepciones
allowlistvacío).security/audit-ci.json,security/AUDIT_EXCEPTIONS.mdcon justificación, responsable y vencimiento.Criterio de aceptación
critical/highsin excepción documentada.Validación
npx --yes audit-ci@7 --config security/audit-ci.jsonSummary by CodeRabbit
Notas de Lanzamiento
Seguridad
Tests