Repository navigation
Implement JWT authentication with blacklist and refactor error handling - #190
Merged
Merged
Conversation
… error logs and improve error delegation to middleware
Contributor
📝 WalkthroughWalkthroughSe implementa un mecanismo de blacklist de tokens JWT con almacenamiento interno en ChangesSeguridad de sesión y consolidación de API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 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}
Contributor
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/commerce/products/product.controller.js (1)
13-29:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftInconsistencia con el objetivo del PR: los errores no se delegan al error handler global.
El PR declara "centralizar el logging en el errorHandler global", pero estos endpoints capturan errores localmente y responden directamente con
res.status(...).json(...), evitando que el middleware global de errores los vea o registre.Solo
deleteProduct(línea 79) delega correctamente connext(error). Para cumplir el objetivo del PR, todos los endpoints deberían seguir ese patrón: capturar el error y pasarlo anext(error)en lugar de construir la respuesta manualmente.🔄 Patrón recomendado para alinear con el error handler global
Ejemplo para
createProduct:export const createProduct = async (req, res, next) => { try { if (!req.user?.id_user) { return res.status(401).json({ success: false, message: "Usuario autenticado requerido" }); } const product = await createProductService(req.user.id_user, req.body); return res.status(201).json(product); } catch (error) { - return res.status(error.status || 500).json({ - message: error.message || "Error interno del servidor" - }); + next(error); } };Aplicar el mismo cambio a
updateProduct,getProductsSearch,getProductById,compareProducts,filterProductsygetRelatedProducts.Also applies to: 31-49, 57-65, 84-101, 105-163
🤖 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/commerce/products/product.controller.js` around lines 13 - 29, The endpoints (e.g., createProduct) currently handle errors locally in their catch blocks and send res.status(...).json(...), which prevents the global errorHandler from logging/handling them; update each controller function (createProduct, updateProduct, getProductsSearch, getProductById, compareProducts, filterProducts, getRelatedProducts and any others noted) to accept the third parameter next (req, res, next) if not already present, remove the local res response in the catch, and instead call next(error) so errors are delegated to the global error handler; ensure you do this consistently across all controller functions mentioned and keep successful responses unchanged.src/modules/session/controllers/session.controllers.js (1)
147-155: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUnificá la validación de sesión con la política central de blacklist.
Acá se vuelve a hacer
jwt.verify, pero no se consulta la blacklist que agregaste ensrc/config/jwt.config.js. Eso ya deja dos caminos de autenticación con reglas distintas. Reutilizáauthenticate/un helper compartido y consumíreq.user, o replicá explícitamente el check de blacklist acá.🤖 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/session/controllers/session.controllers.js` around lines 147 - 155, Aquí se está volviendo a llamar a jwt.verify en session.controllers.js creando una segunda vía de autenticación que ignora la blacklist definida en src/config/jwt.config.js; reemplazá esta verificación por la política central: o 1) reutilizá el middleware authenticate (o el helper compartido) y consumí req.user en vez de llamar a jwt.verify aquí, o 2) si necesitás verificar el token aquí, importá y llamá al chequeo de blacklist desde src/config/jwt.config.js (por ejemplo isTokenBlacklisted o la función equivalente) antes de aceptar el token y manejá TokenExpiredError igual que ahora; en cualquiera de los dos casos eliminá la doble verificación con jwt.verify para que todas las rutas usen la misma lógica de blacklist/validación.
🧹 Nitpick comments (5)
src/modules/commerce/commerces/store.service.js (1)
546-548: ⚡ Quick winSe perdió la distinción entre errores esperados e inesperados.
El código anterior distinguía entre errores
ENOENT(archivo no encontrado, esperado) y otros errores inesperados como permisos insuficientes, disco lleno o problemas de red. Ahora todos los errores se silencian indiscriminadamente, lo que elimina visibilidad operativa de problemas reales en el storage.Aunque esto se alinea con el objetivo de eliminar logs, considerar si el error handler global debería ver errores inesperados de storage para monitoreo de infraestructura.
🔍 Alternativa sugerida para mantener visibilidad de errores inesperados
try { await fs.unlink(previousLogoPath); - } catch { - // silencioso + } catch (error) { + // ENOENT es esperado (archivo ya borrado), otros errores podrían indicar problemas + if (error.code !== 'ENOENT') { + // Delegar al error handler global sin interrumpir el flujo + // (requiere contexto de request para usar next, o un logger centralizado) + } }🤖 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/commerce/commerces/store.service.js` around lines 546 - 548, Reemplaza el bloque catch silencioso por uno que capture el error (catch (err)) y distinga errores esperados de inesperados: si err.code === 'ENOENT' simplemente ignora/retorna como antes, pero para cualquier otro error relánzalo (throw err) o pásalo al manejador global para que se registre/monitoree; esto aplica al catch en src/modules/commerce/commerces/store.service.js donde se manejan operaciones de storage.src/modules/images/services/store-image.service.js (2)
80-80: ⚡ Quick winRiesgo de archivos huérfanos en storage.
Si falla
deleteImageaquí, el logo se elimina de la BD pero el archivo permanece en storage, acumulando archivos huérfanos con el tiempo. El catch silencioso impide detectar estos escenarios.Considerar:
- Loguear el error para monitorear la tasa de fallos de limpieza
- Implementar un proceso periódico de limpieza de archivos huérfanos
- O al menos documentar explícitamente que este riesgo es aceptado
🤖 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/images/services/store-image.service.js` at line 80, The silent catch on await deleteImage(BUCKET, filePath) can leave orphaned files in storage when deletion fails; instead of swallowing errors, catch the error and log it (use the module's logger — e.g., processLogger or the existing logger instance) with context including BUCKET and filePath, and emit/record a metric/alert for failed deletions; additionally add a TODO comment referencing a periodic orphaned-file cleanup job (or document acceptance of the risk) so maintainers know to implement background cleanup if needed (update the call site deleteImage(...) to .catch(err => { logger.error(..., { err, bucket: BUCKET, path: filePath }); incrementMetric('storage.delete.failure'); })).
52-54: ⚡ Quick winSe perdió visibilidad de fallos al eliminar el logo anterior.
El catch silencioso oculta todos los errores al borrar el logo antiguo del storage, incluyendo problemas de permisos, cuota excedida o fallos del servicio. Aunque el nuevo logo ya está en BD, estos errores podrían indicar problemas operativos que deberían monitorearse.
Considerar si el error handler global debería recibir estos errores para observabilidad de infraestructura.
🤖 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/images/services/store-image.service.js` around lines 52 - 54, Silent catch in the deletion of the previous logo (in src/modules/images/services/store-image.service.js) hides operational failures; change the empty catch to catch the error (e.g., catch (err)) and either log it with the module's logger (e.g., this.logger.error or logger.error) including err.stack/message and context ("deleting previous logo"), or forward it to the centralized error/monitoring handler (e.g., reportError/monitoringService.capture/globalErrorHandler) so infrastructure issues are observable; do not swallow the error silently.src/modules/images/services/product-image.service.js (1)
54-56: ⚡ Quick winSe perdió visibilidad de fallos al eliminar la imagen anterior.
El catch silencioso oculta todos los errores al borrar la imagen antigua del storage, incluyendo problemas de permisos, cuota excedida o fallos del servicio de storage. Aunque la nueva imagen ya está guardada en BD, estos errores podrían indicar problemas operativos que deberían monitorearse.
Considerar si el error handler global debería recibir estos errores para observabilidad de infraestructura, o documentar explícitamente que los fallos de limpieza se ignoran intencionalmente.
🤖 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/images/services/product-image.service.js` around lines 54 - 56, El bloque catch que actualmente silencia errores al eliminar la imagen anterior en product-image.service.js debe dejar de ocultarlos: reemplaza el catch vacío por un manejo que registre el error con contexto (p. ej. "failed to delete previous product image" + error) y además propague el fallo al mecanismo de observabilidad/global (por ejemplo enviar a reportError/logger de la app o relanzar/forwardear el error) para no perder visibilidad operativa; modifica el catch que envuelve la llamada de eliminación de la imagen anterior para incluir logging detallado y/o invocar el handler global en lugar de ignorar la excepción.src/modules/images/services/user-image.service.js (1)
52-54: ⚡ Quick winSe perdió visibilidad de fallos al eliminar el avatar anterior.
El catch silencioso oculta todos los errores al borrar el avatar antiguo del storage, incluyendo problemas de permisos, cuota excedida o fallos del servicio. Aunque el nuevo avatar ya está en BD, estos errores podrían indicar problemas operativos que deberían monitorearse.
Considerar si el error handler global debería recibir estos errores para observabilidad de infraestructura.
🤖 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/images/services/user-image.service.js` around lines 52 - 54, The empty catch that swallows errors when deleting the previous avatar must be replaced so failures are observable: change the catch to capture the error (catch (err)) and log it with context (e.g., user id, avatar key) using the project's logger (or processLogger), and forward it to your monitoring/exporter (e.g., Sentry.captureException or metrics) or the global error handler (rethrow or call next(err)) instead of silently ignoring it; update the catch in the function that saves the new avatar to DB (the catch following the old-avatar deletion block) to implement these logging/monitoring and propagation steps.
🤖 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/tokenBlacklist.js`:
- Line 1: The in-memory const blacklist = new Set() is not shared across
processes/restarts; replace it with a shared store that supports TTL (e.g.,
Redis) and expose functions like addToBlacklist(token, ttl) and
isBlacklisted(token) in tokenBlacklist.js so logout writes the token with an
expiration and auth checks query the shared store; remove reliance on the local
Set variable (blacklist) and ensure exported APIs accept/return Promises to work
with async Redis calls.
---
Outside diff comments:
In `@src/modules/commerce/products/product.controller.js`:
- Around line 13-29: The endpoints (e.g., createProduct) currently handle errors
locally in their catch blocks and send res.status(...).json(...), which prevents
the global errorHandler from logging/handling them; update each controller
function (createProduct, updateProduct, getProductsSearch, getProductById,
compareProducts, filterProducts, getRelatedProducts and any others noted) to
accept the third parameter next (req, res, next) if not already present, remove
the local res response in the catch, and instead call next(error) so errors are
delegated to the global error handler; ensure you do this consistently across
all controller functions mentioned and keep successful responses unchanged.
In `@src/modules/session/controllers/session.controllers.js`:
- Around line 147-155: Aquí se está volviendo a llamar a jwt.verify en
session.controllers.js creando una segunda vía de autenticación que ignora la
blacklist definida en src/config/jwt.config.js; reemplazá esta verificación por
la política central: o 1) reutilizá el middleware authenticate (o el helper
compartido) y consumí req.user en vez de llamar a jwt.verify aquí, o 2) si
necesitás verificar el token aquí, importá y llamá al chequeo de blacklist desde
src/config/jwt.config.js (por ejemplo isTokenBlacklisted o la función
equivalente) antes de aceptar el token y manejá TokenExpiredError igual que
ahora; en cualquiera de los dos casos eliminá la doble verificación con
jwt.verify para que todas las rutas usen la misma lógica de
blacklist/validación.
---
Nitpick comments:
In `@src/modules/commerce/commerces/store.service.js`:
- Around line 546-548: Reemplaza el bloque catch silencioso por uno que capture
el error (catch (err)) y distinga errores esperados de inesperados: si err.code
=== 'ENOENT' simplemente ignora/retorna como antes, pero para cualquier otro
error relánzalo (throw err) o pásalo al manejador global para que se
registre/monitoree; esto aplica al catch en
src/modules/commerce/commerces/store.service.js donde se manejan operaciones de
storage.
In `@src/modules/images/services/product-image.service.js`:
- Around line 54-56: El bloque catch que actualmente silencia errores al
eliminar la imagen anterior en product-image.service.js debe dejar de
ocultarlos: reemplaza el catch vacío por un manejo que registre el error con
contexto (p. ej. "failed to delete previous product image" + error) y además
propague el fallo al mecanismo de observabilidad/global (por ejemplo enviar a
reportError/logger de la app o relanzar/forwardear el error) para no perder
visibilidad operativa; modifica el catch que envuelve la llamada de eliminación
de la imagen anterior para incluir logging detallado y/o invocar el handler
global en lugar de ignorar la excepción.
In `@src/modules/images/services/store-image.service.js`:
- Line 80: The silent catch on await deleteImage(BUCKET, filePath) can leave
orphaned files in storage when deletion fails; instead of swallowing errors,
catch the error and log it (use the module's logger — e.g., processLogger or the
existing logger instance) with context including BUCKET and filePath, and
emit/record a metric/alert for failed deletions; additionally add a TODO comment
referencing a periodic orphaned-file cleanup job (or document acceptance of the
risk) so maintainers know to implement background cleanup if needed (update the
call site deleteImage(...) to .catch(err => { logger.error(..., { err, bucket:
BUCKET, path: filePath }); incrementMetric('storage.delete.failure'); })).
- Around line 52-54: Silent catch in the deletion of the previous logo (in
src/modules/images/services/store-image.service.js) hides operational failures;
change the empty catch to catch the error (e.g., catch (err)) and either log it
with the module's logger (e.g., this.logger.error or logger.error) including
err.stack/message and context ("deleting previous logo"), or forward it to the
centralized error/monitoring handler (e.g.,
reportError/monitoringService.capture/globalErrorHandler) so infrastructure
issues are observable; do not swallow the error silently.
In `@src/modules/images/services/user-image.service.js`:
- Around line 52-54: The empty catch that swallows errors when deleting the
previous avatar must be replaced so failures are observable: change the catch to
capture the error (catch (err)) and log it with context (e.g., user id, avatar
key) using the project's logger (or processLogger), and forward it to your
monitoring/exporter (e.g., Sentry.captureException or metrics) or the global
error handler (rethrow or call next(err)) instead of silently ignoring it;
update the catch in the function that saves the new avatar to DB (the catch
following the old-avatar deletion block) to implement these logging/monitoring
and propagation steps.
🪄 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: cb3a72b9-69ae-4b97-b4df-25dfd20a1482
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
src/config/jwt.config.jssrc/lib/index.jssrc/lib/store-timezone.jssrc/lib/tokenBlacklist.jssrc/lib/utils/password.utils.jssrc/middlewares/errorHandler.jssrc/modules/commerce/commerces/store.controller.jssrc/modules/commerce/commerces/store.service.jssrc/modules/commerce/product-tags/product-tag.controller.jssrc/modules/commerce/products/product.controller.jssrc/modules/global/categories/product-categories/product-category.controller.jssrc/modules/global/categories/store-categories/store-category.controller.jssrc/modules/global/distances/controllers/distance.controller.jssrc/modules/images/services/product-image.service.jssrc/modules/images/services/store-image.service.jssrc/modules/images/services/user-image.service.jssrc/modules/session/controllers/session.controllers.js
💤 Files with no reviewable changes (8)
- src/modules/global/categories/product-categories/product-category.controller.js
- src/lib/store-timezone.js
- src/modules/global/distances/controllers/distance.controller.js
- src/modules/global/categories/store-categories/store-category.controller.js
- src/middlewares/errorHandler.js
- src/modules/commerce/commerces/store.controller.js
- src/modules/commerce/product-tags/product-tag.controller.js
- src/lib/utils/password.utils.js
Benjakr04
approved these changes
Jun 2, 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
Release Notes
Seguridad
httpOnlyysecure.Refactor