Repository navigation
Sec a09 security logging - #187
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSe agrega una librería de logging seguro y su middleware; se instrumentan session, orders y delivery-assignments para emitir eventos de auditoría; se unifican respuestas de validación de delivery y se centralizan/extraen helpers y harnesses de tests para e2e y unit. ChangesSistema de logging de seguridad y refactor de tests
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutos 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 |
Integra blacklist JWT y barrel lib/index.js de dev manteniendo logSecurityEvent en login fallido.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/app.js (1)
80-80: ⚡ Quick winMontá
securityResponseLoggermás arriba para cobertura total.En Line 80 se registra después de otros
app.use; si algún middleware previo corta con 403, no se audita. Conviene montarlo apenas creadoapp.🔁 Ajuste sugerido
const app = express(); +app.use(securityResponseLogger); // Seguridad HTTP con Helmet app.use(helmet()); @@ app.use(express.json()); app.use(cookieParser()); -app.use(securityResponseLogger);🤖 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/app.js` at line 80, El middleware securityResponseLogger se monta demasiado tarde (app.use(securityResponseLogger)); colócalo inmediatamente después de la creación de la instancia app (inmediatamente después de const app = express() / justo al inicializar app) y antes de cualquier otro app.use o rutas para garantizar que capture respuestas y códigos (p.ej. 403) de middlewares previos; busca la referencia securityResponseLogger y muévela arriba del resto de app.use/rutas en src/app.js.
🤖 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/modules/delivery/delivery-assignments/delivery-assignments.service.js`:
- Around line 298-305: Falta auditar el cambio de estado de la orden cuando se
completa la asignación: dentro del mismo flujo donde se actualiza
orders.order_status a "DELIVERED" (el bloque que ya llama a
logSecurityEvent("ASSIGNMENT_STATUS_CHANGED")), añade una llamada adicional a
logSecurityEvent para registrar el cambio de estado de la orden (por ejemplo
"ORDER_STATUS_CHANGED") incluyendo orderId (assignment.fk_order), previousStatus
(valor anterior de orders.order_status), newStatus "DELIVERED" y actorUserId
(id_user) — ubica la inserción junto al uso existente de logSecurityEvent dentro
de delivery-assignments.service.js para garantizar que tanto el cambio de
asignación como el de orden queden auditados.
In `@src/modules/session/controllers/session.controllers.js`:
- Around line 57-58: En el bloque catch que actualmente solo hace "return
next(new UnauthorizedError("Credenciales inválidas"))", añade un
registro/auditoría de seguridad para el fallo de verificación de contraseña:
captura el error lanzado por verifyPassword y emite el evento/registro
LOGIN_FAILED (incluyendo el mensaje o stack del error y el user identifier si
está disponible) antes de llamar a next(new UnauthorizedError(...)); busca la
función verifyPassword y el lugar que lanza UnauthorizedError en el controlador
de sesión para insertar la llamada al logger/auditoría (ej. emitir LOGIN_FAILED
con detalles) preservando la respuesta al cliente.
- Around line 32-35: The logSecurityEvent call uses email ?? null which leaves
empty or whitespace-only strings as non-null; normalize the email before logging
by trimming and treating empty/whitespace-only values as null (e.g., compute a
normalizedEmail from the incoming email in the login handler/controller and pass
that to logSecurityEvent), updating the branch that calls
logSecurityEvent("LOGIN_FAILED", ...) so it uses the normalized value instead of
email ?? null.
---
Nitpick comments:
In `@src/app.js`:
- Line 80: El middleware securityResponseLogger se monta demasiado tarde
(app.use(securityResponseLogger)); colócalo inmediatamente después de la
creación de la instancia app (inmediatamente después de const app = express() /
justo al inicializar app) y antes de cualquier otro app.use o rutas para
garantizar que capture respuestas y códigos (p.ej. 403) de middlewares previos;
busca la referencia securityResponseLogger y muévela arriba del resto de
app.use/rutas en src/app.js.
🪄 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: 5b8f2afd-aa55-447e-83aa-0cce4673b288
📒 Files selected for processing (8)
src/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
Actualiza mocks de auth y teléfono, unifica mensaje de validación Zod y corrige mocks de orders.count en e2e relacionados.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/modules/delivery/delivery/delivery.controller.js (1)
50-58: 💤 Low valueEl mensaje fijo es coherente con el contrato del resto de la app.
Alinea la respuesta de
ZodErrorconcreateAssignmenty con el default deValidationError("Datos inválidos"), conservandodetails: error.issues.Nota opcional: los otros handlers de este mismo archivo (
updateDeliveryStatus, Línea 94;updateDeliveryProfile, Línea 175) siguen devolviendoerror.issues[0].message. Si la intención es no filtrar detalles de validación de forma uniforme, conviene homogeneizarlos en un seguimiento.🤖 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/delivery.controller.js` around lines 50 - 58, The ZodError branch in delivery.controller.js should use the same fixed response message as createAssignment and the default ValidationError contract, instead of exposing a field-level message. Update the ZodError handling in the controller’s error response to keep message as "Datos inválidos" while preserving details: error.issues, and consider aligning updateDeliveryStatus and updateDeliveryProfile later if you want uniform validation behavior across the file.
🤖 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.
Nitpick comments:
In `@src/modules/delivery/delivery/delivery.controller.js`:
- Around line 50-58: The ZodError branch in delivery.controller.js should use
the same fixed response message as createAssignment and the default
ValidationError contract, instead of exposing a field-level message. Update the
ZodError handling in the controller’s error response to keep message as "Datos
inválidos" while preserving details: error.issues, and consider aligning
updateDeliveryStatus and updateDeliveryProfile later if you want uniform
validation behavior across the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39f46785-66c4-4b00-8005-bb7ef6dd0fc3
📒 Files selected for processing (6)
src/modules/delivery/delivery/delivery.controller.jssrc/modules/delivery/delivery/delivery.validation.jstests/e2e/delivery-register.test.jstests/e2e/notifications.test.jstests/e2e/stock.test.jstests/unit/delivery/delivery-register.test.js
✅ Files skipped from review due to trivial changes (1)
- src/modules/delivery/delivery/delivery.validation.js
Centraliza la suite de registro delivery y el stub de orders.count en helpers reutilizables.
Usa Datos invalidos con details en lugar de issues[0].message, alineado con register y assignments.
Unifica delivery-register en e2e, extrae setupOrderCreateE2e y elimina el test unit duplicado.
Merge origin/dev, corrige validate.params merge, alinea tests e2e con middleware Zod y mantiene helpers SEC-A09.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/product/products.test.js (1)
311-318:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNo fijemos un
500como comportamiento esperado para un error de entrada.Aceptar
500acá hace que el test tape una excepción del handler frente a un payload inválido. Paratagscon tipo incorrecto, el contrato debería seguir siendo400con error de validación; si hoy rompe, conviene arreglar la validación del endpoint en vez de institucionalizar el bug en el suite.Diff propuesto para el test
it("devuelve 400 cuando tags no es un array", async () => { const res = await request(app) .post("/products") .set("Cookie", `userToken=${sellerToken}`) .send({ name: "Test", price: 10, categoryId: 1, quantity: 5, tags: "no-array" }); - expect(res.status).toBe(500); + expectValidationError(res, "tags"); });🤖 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 `@tests/e2e/product/products.test.js` around lines 311 - 318, The test currently expects a 500 when sending invalid payload (tags as a string) to POST "/products" which masks a validation bug; update the assertion in the test "devuelve 400 cuando tags no es un array" to expect 400 instead of 500, and if the endpoint's handler (e.g., the products route/createProduct validation middleware) actually throws a 500 for this input, fix the request validation there so it returns a 400 validation error for invalid types (ensure the route's schema/middleware checks that tags is an array and returns a 400 response on failure).
🧹 Nitpick comments (3)
tests/helpers/order-e2e.harness.js (1)
57-80: ⚡ Quick winEvitá que este harness deje implementaciones vivas entre casos.
Acá se instalan
mockResolvedValue/mockImplementationsobre elprismacompartido. En las suites que lo consumen se usavi.clearAllMocks(), y eso en Vitest no resetea implementaciones. El resultado es queprisma.$transaction,prisma.carts.findFirstyprisma.orders.countpueden filtrarse al test siguiente y volver los e2e dependientes del orden. Preferiría resetear explícitamente estos mocks antes de configurarlos, o pasar las suites consumidoras avi.resetAllMocks()antes de reinstalar el harness.🤖 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 `@tests/helpers/order-e2e.harness.js` around lines 57 - 80, Antes de volver a instalar los mocks en este harness, reseteá las implementaciones previas para evitar filtrado entre tests: llamá a mockReset/mockClear sobre prisma.$transaction, prisma.carts.findFirst, prisma.orders.count y cualquier método anidado que vayas a reasignar (por ejemplo orders.create/orderItems.createMany/products.update/notifications.create), o alternativamente documentá y exige que las suites consumidoras usen vi.resetAllMocks() antes de montar este harness; luego reinstalá las mockResolvedValue/mockImplementation como ahora (p. ej. la implementación de prisma.$transaction).src/middlewares/validate.middleware.js (1)
22-28: 💤 Low valueUnificar/ documentar la semántica de
validateentrequery,paramsybody
Ensrc/middlewares/validate.middleware.jslas secciones se manejan distinto:querypisa enreq.validated,paramshace merge enreq.paramscreando un objeto nuevo (línea 25) ybody/otros reemplazan completoreq[section](línea 27). Esto puede confundir sobre qué propiedad termina efectivamente disponible para el resto del pipeline.
- Para
params, el cambio de referencia parece de bajo riesgo: en los controllers el acceso areq.paramses mayormente por destructuring (const { ... } = req.params) y no se ve uso del objeto completo por identidad (===,Object.is,const x = req.params).- Refactor opcional: si querés mantener consistencia y evitar el cambio de referencia, reemplazá el spread por
Object.assign(req.params, result.data)o, como mínimo, documentá claramente el contrato por sección.🤖 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/middlewares/validate.middleware.js` around lines 22 - 28, En el middleware de validación (src/middlewares/validate.middleware.js) la asignación de resultados es inconsistente: query escribe en req.validated, params reemplaza la referencia con req.params = { ...req.params, ...result.data } y body/otros hacen req[section] = result.data; arregla esto o documenta el contrato. Recomiendo sustituir el spread por una mutación de ruta para params usando Object.assign(req.params, result.data) para mantener la misma referencia de req.params, o si prefieres mantener el reemplazo explícito, añade un comentario/documentación clara en el bloque que explique que params se reemplaza y query usa req.validated; referencia las variables section, req.params y req.validated al hacer el cambio.tests/helpers/expect-validation-error.js (1)
1-11: ⚡ Quick winImportá
expectpara no depender deglobalsimplícitos
tests/helpers/expect-validation-error.jsusaexpectsin importarlo, perovitest.config.jstieneglobals: true, así que hoy no falla. Aun así, agregar el import mejora la robustez ante un cambio de configuración y mantiene consistencia con los tests que importanexpectexplícitamente.Diff propuesto
+import { expect } from "vitest"; + export function expectValidationError(res, field) { expect(res.status).toBe(400); expect(res.body.message).toBe("Error de validación");🤖 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 `@tests/helpers/expect-validation-error.js` around lines 1 - 11, Add an explicit import for expect and use it in the helper to avoid relying on globals: at the top of tests/helpers/expect-validation-error.js import { expect } from 'vitest' and keep the existing expectValidationError function as-is (it references expect), so the file no longer depends on vitest.config.js globals and matches other tests that import expect explicitly.
🤖 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.
Outside diff comments:
In `@tests/e2e/product/products.test.js`:
- Around line 311-318: The test currently expects a 500 when sending invalid
payload (tags as a string) to POST "/products" which masks a validation bug;
update the assertion in the test "devuelve 400 cuando tags no es un array" to
expect 400 instead of 500, and if the endpoint's handler (e.g., the products
route/createProduct validation middleware) actually throws a 500 for this input,
fix the request validation there so it returns a 400 validation error for
invalid types (ensure the route's schema/middleware checks that tags is an array
and returns a 400 response on failure).
---
Nitpick comments:
In `@src/middlewares/validate.middleware.js`:
- Around line 22-28: En el middleware de validación
(src/middlewares/validate.middleware.js) la asignación de resultados es
inconsistente: query escribe en req.validated, params reemplaza la referencia
con req.params = { ...req.params, ...result.data } y body/otros hacen
req[section] = result.data; arregla esto o documenta el contrato. Recomiendo
sustituir el spread por una mutación de ruta para params usando
Object.assign(req.params, result.data) para mantener la misma referencia de
req.params, o si prefieres mantener el reemplazo explícito, añade un
comentario/documentación clara en el bloque que explique que params se reemplaza
y query usa req.validated; referencia las variables section, req.params y
req.validated al hacer el cambio.
In `@tests/helpers/expect-validation-error.js`:
- Around line 1-11: Add an explicit import for expect and use it in the helper
to avoid relying on globals: at the top of
tests/helpers/expect-validation-error.js import { expect } from 'vitest' and
keep the existing expectValidationError function as-is (it references expect),
so the file no longer depends on vitest.config.js globals and matches other
tests that import expect explicitly.
In `@tests/helpers/order-e2e.harness.js`:
- Around line 57-80: Antes de volver a instalar los mocks en este harness,
reseteá las implementaciones previas para evitar filtrado entre tests: llamá a
mockReset/mockClear sobre prisma.$transaction, prisma.carts.findFirst,
prisma.orders.count y cualquier método anidado que vayas a reasignar (por
ejemplo
orders.create/orderItems.createMany/products.update/notifications.create), o
alternativamente documentá y exige que las suites consumidoras usen
vi.resetAllMocks() antes de montar este harness; luego reinstalá las
mockResolvedValue/mockImplementation como ahora (p. ej. la implementación de
prisma.$transaction).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cce4a13f-544d-4de0-81c5-fe01d12d8ade
📒 Files selected for processing (18)
src/app.jssrc/middlewares/validate.middleware.jstests/e2e/delivery-profile.test.jstests/e2e/delivery-register.test.jstests/e2e/delivery-status.test.jstests/e2e/delivery.test.jstests/e2e/notifications.test.jstests/e2e/orders-delivery-reviews.test.jstests/e2e/product/products.test.jstests/e2e/stock.test.jstests/e2e/stores.test.jstests/e2e/users.test.jstests/helpers/delivery-register.harness.jstests/helpers/expect-validation-error.jstests/helpers/order-e2e.harness.jstests/unit/delivery/delivery-profile.test.jstests/unit/delivery/delivery-register.test.jstests/unit/dtos/misc.dto.test.js
💤 Files with no reviewable changes (1)
- tests/unit/delivery/delivery-register.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app.js
- tests/e2e/delivery-register.test.js
- tests/helpers/delivery-register.harness.js
Tags invalidos devuelven 400, Object.assign en params del middleware, import explicito en helper e2e y mockReset en order harness.
Registra cambio de orden al completar asignacion, normaliza email en logs y audita fallos de verifyPassword.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/modules/global/dtos/products/product.request.dto.js`:
- Around line 13-14: Valida que el string "trimmed" contenga solo dígitos
decimales puros antes de convertirlo: replace the current Number(trimmed) +
Number.isInteger(n) check with a regex test like /^\d+$/ on trimmed, then parse
with parseInt(trimmed, 10) (or Number(trimmed) after the regex) and assert the
resulting n > 0; update the conditions around Number(trimmed) and
Number.isInteger(n) to use the regex + parseInt approach so hex/exponential
formats like "0x10" or "1e2" are rejected.
🪄 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: ae30b7f3-0eb7-441d-87c7-0297d40cff32
📒 Files selected for processing (7)
src/middlewares/validate.middleware.jssrc/modules/delivery/delivery-assignments/delivery-assignments.service.jssrc/modules/global/dtos/products/product.request.dto.jssrc/modules/session/controllers/session.controllers.jstests/e2e/product/products.test.jstests/helpers/expect-validation-error.jstests/helpers/order-e2e.harness.js
🚧 Files skipped from review as they are similar to previous changes (5)
- src/middlewares/validate.middleware.js
- tests/helpers/expect-validation-error.js
- src/modules/delivery/delivery-assignments/delivery-assignments.service.js
- src/modules/session/controllers/session.controllers.js
- tests/helpers/order-e2e.harness.js
Sincroniza dev reciente, conserva logging de seguridad SEC-A09 y resuelve conflictos en validate middleware y tests.
Rechaza 0x10 y notacion exponencial en parseCsvTagIds usando regex y parseInt.
…ery-register Mantiene tests de registro delivery solo en e2e con harness (Sonar).
|
J-Kanami-PS
approved these changes
Jun 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests