PR Review #47 — NutriFlow

💥 Análisis de Radio de Impacto (Blast Radius) ALTO

Archivo	Severidad	Consumidores afectados	Riesgo
`src/utils/auth.ts`	CRÍTICO	Todos los endpoints protegidos (12 rutas)	Cambio en `verifyToken()` afecta toda la auth de la app
`src/db/queries/users.ts`	CRÍTICO	PatientService, AdminService, ReportService	SQL injection expone tabla completa de pacientes (PII)
`src/api/routes/admin.ts`	CRÍTICO	Todos los usuarios del sistema	Reset masivo de contraseñas sin autenticación
`src/services/csvParser.ts`	MEDIO	ImportController	Sin validación de tipo/tamaño de archivo; path traversal
`package.json` (multer, csv-parse, nodemailer)	MEDIO	Build pipeline, bundle	3 dependencias nuevas; revisar CVEs en nodemailer
`src/db/migrations/20260612_add_import_log.sql`	BAJO	DB schema	Sin migración DOWN; no reversible en rollback

🔐 Escaneo de Seguridad 4 CRÍTICOS

🔴

API Key de Stripe expuesta en texto plano

src/api/routes/admin.ts:12 — STRIPE_SECRET_KEY = 'sk_live_4eR8mNpQ2vXw...'. Credencial de producción hardcodeada. URGENTE: revocar en dashboard de Stripe inmediatamente.

🔴

SQL Injection — interpolación directa de inputs del usuario

src/db/queries/users.ts:15 — WHERE name LIKE '%${term}%'. Permite extraer o corromper toda la tabla patients.

🔴

Endpoint admin sin autenticación

src/api/routes/admin.ts:3 — POST /api/admin/reset-all-passwords con TODO: add auth check later. Cualquier atacante puede resetear las contraseñas de todos los usuarios.

🔴

JWT fallback inseguro + catch silencioso

src/utils/auth.ts:10 — process.env.JWT_SECRET || 'fallback-secret-123'. Si JWT_SECRET no está configurado en producción, todos los tokens firmados con el fallback son válidos para cualquiera que conozca la cadena.

🟡

Algoritmo de hash inseguro (MD5) para contraseñas

src/api/routes/admin.ts:7 — md5(Math.random().toString()). MD5 no es apto para hashing de contraseñas. Usar bcrypt o argon2.

🟡

Sin validación de tipo MIME en upload de CSV

src/api/routes/patients.ts — multer acepta cualquier archivo. Riesgo de subir scripts maliciosos. Añadir fileFilter para validar text/csv y tamaño máximo.

🟢

Sin prototype pollution detectado

No se encontraron patrones __proto__ ni constructor[ en el diff.

🟢

Sin vectores XSS en frontend

No se encontraron dangerouslySetInnerHTML ni innerHTML = en los archivos modificados del PR.

⛔ Hallazgos — MUST FIX (Bloqueantes) 4 items

1

API Key de Stripe hardcodeada en código fuente

📁 src/api/routes/admin.ts — línea 12

Una clave de producción de Stripe (sk_live_) está presente en texto plano en el repositorio. Cualquier persona con acceso al repo puede usarla para realizar cargos fraudulentos o acceder a datos de clientes de pago.

✅ Acción inmediata

- const STRIPE_SECRET_KEY = 'sk_live_4eR8mNpQ2vXw...';
+ // 1. Revocar AHORA en dashboard.stripe.com/apikeys
+ // 2. Añadir a .env (nunca al repositorio)
+ const STRIPE_SECRET_KEY = process.env.STRIPE_SECRET_KEY;
+ if (!STRIPE_SECRET_KEY) throw new Error('STRIPE_SECRET_KEY not set');

2

SQL Injection en búsqueda de pacientes

📁 src/db/queries/users.ts — línea 15

El parámetro term se interpola directamente en la query SQL. Un atacante puede inyectar ' OR '1'='1 para extraer todos los registros, o '; DROP TABLE patients; -- para destruir datos de PII de pacientes (RGPD/HIPAA).

✅ Usar query parametrizada

- const query = `SELECT * FROM patients WHERE name LIKE '%${term}%' OR email = '${term}'`;
- return db.execute(query);
+ return db.query(
+   'SELECT * FROM patients WHERE name ILIKE $1 OR email = $2',
+   [`%${term}%`, term]
+ );

3

Endpoint de reset masivo sin autenticación ni autorización

📁 src/api/routes/admin.ts — línea 3

POST /api/admin/reset-all-passwords no verifica ningún token ni rol. Un atacante sin credenciales puede resetear las contraseñas de todos los usuarios del sistema y recibir las nuevas por email (que controla el servidor de email). Vectorio de toma completa de la aplicación.

✅ Añadir middleware de auth + verificación de rol

- router.post('/api/admin/reset-all-passwords', async (req, res) => {
-   // TODO: add auth check later
+ router.post('/api/admin/reset-all-passwords',
+   requireAuth,
+   requireRole('SUPER_ADMIN'),
+   async (req, res) => {
+   // audit log obligatorio para operaciones destructivas
+   await auditLog.record({ action: 'bulk_reset', actor: req.user.id });

4

JWT con fallback hardcodeado — autenticación bypasseable

📁 src/utils/auth.ts — línea 10

Si JWT_SECRET no está definida en el entorno (error de configuración en staging/prod), la app firma tokens con 'fallback-secret-123'. Cualquier atacante que conozca esta cadena puede forjar tokens válidos para cualquier usuario.

✅ Fallar en startup si el secreto no está configurado

- return jwt.verify(token, process.env.JWT_SECRET || 'fallback-secret-123');
+ const secret = process.env.JWT_SECRET;
+ if (!secret) throw new Error('JWT_SECRET env var is not configured');
+ return jwt.verify(token, secret);

⚠️ SHOULD FIX (No bloqueantes, pero recomendados) 3 items

5

Contraseñas reseteadas usando MD5 — algoritmo inseguro

📁 src/api/routes/admin.ts — línea 7

MD5 no es apto para hashing de contraseñas. Las tablas rainbow pueden revertirlo en segundos. Además, Math.random() no es criptográficamente seguro.

✅ Usar argon2 o bcrypt + crypto seguro

- const hash = md5(Math.random().toString());
+ import { randomBytes } from 'crypto';
+ import argon2 from 'argon2';
+ const tempPassword = randomBytes(16).toString('hex');
+ const hash = await argon2.hash(tempPassword);

6

N+1 queries en importación CSV de pacientes

📁 src/api/routes/patients.ts — líneas 18-22

db.findOne() y patientService.create() son llamadas dentro de un bucle for...of. Con un CSV de 1.000 pacientes, esto genera 2.000 queries secuenciales. Usar operaciones batch.

✅ Batch upsert

- for (const patient of results) {
-   await db.findOne({ email: patient.email });
-   await patientService.create(patient);
- }
+ await patientService.bulkUpsert(results, { onConflict: 'email' });

7

Migración DB sin DOWN — no reversible en rollback

📁 src/db/migrations/20260612_add_import_log.sql

La migración crea la tabla import_logs pero no define el rollback (DROP TABLE import_logs). Si hay que hacer un rollback del deploy, la migración quedará aplicada y causará inconsistencias.

✅ Añadir sección DOWN

+ -- DOWN
+ DROP TABLE IF EXISTS import_logs;

💡 Sugerencias 3 items

8

suggestion: Añadir validación de tamaño y tipo MIME al upload CSV

📁 src/api/routes/patients.ts

Actualmente multer acepta cualquier archivo. Añadir fileFilter para aceptar solo text/csv y un límite de tamaño (ej. 5MB) para evitar DoS por uploads masivos.

9

nit: Añadir índice en import_logs.imported_at

📁 src/db/migrations/20260612_add_import_log.sql

Si se van a consultar logs por rango de fechas, un índice en imported_at evitará full scans cuando la tabla crezca.

10

question: ¿El scope del PR coincide con NUTRI-218?

NUTRI-218 (Jira)

El ticket NUTRI-218 describe "importación CSV de pacientes". El endpoint /admin/reset-all-passwords parece scope creep. ¿Pertenece a otro ticket? Si no tiene ticket propio, crear NUTRI-224 antes de mergear.

🧪 Cobertura de Tests Insuficiente

Archivos fuente modificados11

Archivos de test añadidos1

Solo csvParser.test.ts fue añadido. Sin tests para:

Admin reset endpoint
searchPatients() query
verifyToken() con fallback
Flujo completo de import CSV

Reglas de cobertura del proyecto:

🔴

Cobertura mínima: 70% — Actual: ~18%

Por debajo del umbral requerido

🔴

Auth/Payments: cobertura 100% requerida

auth.ts sin tests tras modificación

🟢

csvParser.test.ts cubre casos base

28 líneas, happy path y encoding UTF-8

✅ Checklist de Revisión (30+ items)

📋 Scope & Contexto

✅Título del PR describe el cambio
⚠️Descripción explica el WHY (incompleta para reset)
✅Ticket Jira NUTRI-218 existe y está abierto
❌Sin scope creep (reset endpoint sin ticket propio)
❌Breaking changes documentados en PR body

💥 Blast Radius

✅Dependientes directos identificados
✅Límites de servicio comprobados
❌Nuevas env vars en .env.example (STRIPE_SECRET_KEY)
❌Migraciones DB reversibles (sin DOWN)

🔐 Seguridad

❌Sin secretos hardcodeados (Stripe API key)
❌SQL parametrizado (SQL injection en searchPatients)
✅Inputs de frontend validados en csvParser
❌Auth en todos los nuevos endpoints
✅Sin vectores XSS detectados
❌Dependencias sin CVEs conocidos (pendiente audit)
✅Sin datos sensibles en logs
⚠️File uploads validados (MIME + size, pendiente)
✅CORS no modificado

🧪 Tests

❌Nuevas funciones públicas con unit tests
⚠️Edge cases cubiertos (solo en csvParser)
❌Rutas de error testeadas
❌Tests de integración para nuevos endpoints
✅Sin tests eliminados sin justificación

🔀 Cambios Disruptivos

✅Sin endpoints eliminados
✅Sin campos requeridos eliminados de APIs
❌Sin columnas DB eliminadas sin plan dos fases
✅Sin env vars eliminadas de producción

⚡ Rendimiento

❌Sin patrones N+1 (loop con findOne en import)
✅DB índices para nuevas queries (OK en migración)
✅Sin bucles unbounded en datasets grandes
⚠️Nuevas dependencias justificadas (nodemailer, revisar)
✅Operaciones async correctamente awaited

🏗️ Calidad de Código

✅Sin imports no usados
⚠️Manejo de errores (catch vacío en auth.ts)
✅Consistente con patrones existentes
❌TODOs sin resolver (auth check en admin route)

👍 Lo que está bien 3 items

✅ Migración DB bien estructurada: la tabla import_logs tiene tipos correctos, valor por defecto sensato y timestamp automático. Solo falta el DOWN.

✅ csvParser cubierto básicamente: csvParser.test.ts cubre el happy path y el encoding UTF-8, que es un buen inicio para un colaborador nuevo.

✅ .env.example actualizado: se añadieron las 2 nuevas variables de entorno requeridas al ejemplo de configuración. Buen hábito para el equipo.