PR #142 — Revisión de Código

🔒 Seguridad 3 hallazgos

CRÍTICO SQL Injection — interpolación directa de parámetros en query raw leads/views.py · L8–L12

El código construye una query SQL concatenando directamente user_id y status desde la query string sin ninguna sanitización. Un atacante puede enviar ?user_id=1 OR 1=1 para exfiltrar todos los leads de todos los usuarios, o incluso ejecutar queries destructivas.

            − Código problemático
            
            + Corrección propuesta
          
−query = f"SELECT * FROM leads WHERE user_id = {user_id}"
−if status:
−    query += f" AND status = '{status}'"
−leads = Lead.objects.raw(query)
···
+leads = Lead.objects.filter(owner=request.user)
+if status:
+    leads = leads.filter(status=status)
+leads = leads.select_related('contact', 'owner')\
+              .prefetch_related('tags')

Sugerencia: Usa siempre el ORM de Django o, si necesitas raw SQL, usa cursor.execute(sql, [params]) con parámetros separados. Nunca interp oles variables de usuario en strings SQL.

CRÍTICO IDOR — cualquier usuario puede ver leads de otros usuarios leads/views.py · L6

El endpoint acepta user_id como parámetro GET y filtra por ese valor. Cualquier usuario autenticado puede cambiar ?user_id=999 para ver los leads de cualquier otro usuario. Insecure Direct Object Reference (IDOR) clásico — viola el control de acceso básico.

− Sin control de autorización + Filtrar siempre por request.user

−user_id = request.GET.get('user_id') # ← parámetro controlado por el cliente

+leads = Lead.objects.filter(owner=request.user) # ← siempre el usuario en sesión

Sugerencia: ¿Hay un caso de uso legítimo donde un admin vea los leads de otro usuario? Si sí, implementa una comprobación explícita de rol: if request.user.is_staff and target_user_id: ...

CRÍTICO XSS — dangerouslySetInnerHTML con datos no saneados de la API LeadsDashboard.tsx · L17–L19

La función renderNotes inyecta el campo lead.notes directamente en el DOM via dangerouslySetInnerHTML. Si un comercial guarda notas con contenido como <script>fetch('/api/leads?export=all')</script>, este código se ejecutará en el navegador de cualquier otro usuario que vea el dashboard.

            − XSS potencial
            
            + Renderizado seguro
          
−const renderNotes = (notes: string) => (
−  <div dangerouslySetInnerHTML={{ __html: notes }} />
−);
···
+// Opción A: texto plano (sin HTML)
+const renderNotes = (notes: string) => <p>{notes}</p>;
+
+// Opción B: si necesitas HTML, sanea con DOMPurify
+import DOMPurify from 'dompurify';
+const renderNotes = (notes: string) => (
+  <div dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(notes) }} />
+);

⚡ Rendimiento 2 hallazgos

ALTO N+1 Queries — acceso a relaciones en bucle sin prefetch leads/views.py · L14–L20

Por cada lead en el resultado, el código lanza 3 queries adicionales: una para contact.email, otra para owner.username y otra para tags.all(). Con 200 leads, esto genera 601 queries a PostgreSQL en lugar de 4. En producción, con carga concurrente, esto puede colapsar la base de datos.

− 1 + 3N queries + 4 queries (con SELECT IN)

−for lead in leads: # N queries de prefetch

− 'contact': lead.contact.email, # +1 query cada vez

− 'assigned_to': lead.owner.username, # +1 query cada vez

− 'tags': [t.name for t in lead.tags.all()] # +1 query cada vez

···

+leads = Lead.objects\

+ .filter(owner=request.user)\

+ .select_related('contact', 'owner')\ # JOIN en 1 query

+ .prefetch_related('tags') # SELECT IN separado

ALTO Exportación CSV sin límite ni paginación — DoS potencial leads/export.py · L7–L11

La función export_leads_csv carga todos los registros en memoria y los escribe en un buffer en RAM. Un usuario con 500.000 leads podría generar una respuesta de varios GB, agotando la memoria del servidor. Además, los N+1 del punto anterior se aplican también aquí.

Sugerencia: Usa StreamingHttpResponse con un generador para escribir el CSV en chunks, y añade un límite configurable (ej. 10.000 filas por exportación, o un job async de Celery para exportaciones grandes). ¿Has considerado que esto podría necesitar revisión de un senior dado el impacto en producción?

⚛️ Diseño & React 2 hallazgos

MEDIO useEffect — userId ausente del array de dependencias (stale closure) LeadsDashboard.tsx · L10

Si el componente recibe un userId diferente (ej. cambio de usuario en una vista admin), el efecto no se re-ejecuta y el dashboard muestra datos obsoletos. React/eslint-plugin-react-hooks debería haber capturado esto como warning.

− Dependencia faltante + Corrección

− }, [filter]); // userId faltante → stale closure

+ }, [filter, userId]); // correcto

MEDIO Sin manejo de errores ni estado de carga en el componente LeadsDashboard.tsx · L12–L15

El fetch no tiene .catch(). Si la API falla (500, timeout, red caída), el componente se queda en blanco silenciosamente. El usuario no recibe ningún feedback. Tampoco hay estado de carga (spinner/skeleton) mientras se obtienen los datos.

Sugerencia: Añade estados loading y error. Considera usar React Query / SWR para cacheo y reintento automático — ya lo usáis en otros componentes del proyecto.

🧪 Cobertura de Tests 1 hallazgo

BAJO 0% de cobertura — ni un test para la vista ni el componente tests/ (ausente)

El PR añade una vista Django (get_leads), una función de exportación y un componente React sin ningún test. Mínimo esperado por las guías de Sentry:

Tests mínimos necesarios antes de aprobar:

1. Backend: test_get_leads_only_returns_own_leads() — verifica que un usuario no puede ver leads de otro.
2. Backend: test_get_leads_filters_by_status() — verifica que el filtro funciona correctamente.
3. Frontend: render del componente con datos mock + snapshot o assertions sobre el DOM.
4. Frontend: caso de error de API → mensaje de error visible para el usuario.