Code Review — NutriTrack SaaS PR #142
feature/patient-search-and-import  ·  3 archivos modificados  ·  +312 / −47 líneas
🚫 BLOQUEADO
Diagnóstico previo
$ dotnet build NutriTrack.sln Build succeeded. 0 Error(s)    3 Warning(s) $ dotnet format --verify-no-changes warning: PatientSearchController.cs:28 — line length exceeds 120 chars warning: NutritionPlanImporter.cs:61 — inconsistent brace style $ git diff -- '*.cs' diff --git a/Controllers/PatientSearchController.cs diff --git a/Services/NutritionPlanImporter.cs diff --git a/Services/ReportExportService.cs
Resumen de hallazgos
3
Crítico
4
Alto
3
Medio
2
Bajo
12
Total
Problemas críticos
Crítico Inyección SQL por interpolación de cadena PatientSearchController.cs:34
Problema
El parámetro searchTerm recibido de la query string se concatena directamente en la consulta SQL mediante interpolación de cadena ($"..."). Un atacante puede inyectar SQL arbitrario para extraer, modificar o eliminar datos de pacientes.
Corrección
Usar consultas parametrizadas con SqlParameter o migrar a EF Core con LINQ donde la parametrización es automática. Nunca incluir input del usuario en el texto de la consulta.
Antes — VULNERABLE
var query = $"SELECT * FROM Patients
  WHERE Name LIKE '%{searchTerm}%'
     OR DNI = '{searchTerm}'";

var cmd = new SqlCommand(query, conn);
var results = await cmd.ExecuteReaderAsync();
Después — SEGURO
var query = @"SELECT * FROM Patients
  WHERE Name LIKE @search
     OR DNI = @dni";

var cmd = new SqlCommand(query, conn);
cmd.Parameters.AddWithValue("@search",
    $"%{searchTerm}%");
cmd.Parameters.AddWithValue("@dni", searchTerm);
var results = await cmd.ExecuteReaderAsync();
Crítico Path Traversal en exportación de informes ReportExportService.cs:67
Problema
La ruta de exportación outputPath proviene directamente del body de la petición HTTP y se usa sin validar en File.WriteAllBytesAsync. Un atacante puede escribir archivos fuera del directorio permitido usando secuencias ../../../ o rutas absolutas.
Corrección
Normalizar con Path.GetFullPath y verificar que la ruta resultante empieza con el directorio base permitido. Idealmente, no recibir rutas del cliente: generar el nombre del archivo en el servidor.
Antes — VULNERABLE
public async Task ExportAsync(
    string outputPath,
    ReportData data)
{
    var pdf = GeneratePdf(data);
    // ⚠️ sin validar outputPath
    await File.WriteAllBytesAsync(
        outputPath, pdf);
}
Después — SEGURO
private const string _allowedBase =
    "/var/app/exports";

public async Task ExportAsync(
    string fileName, ReportData data)
{
    var fullPath = Path.GetFullPath(
        Path.Combine(_allowedBase, fileName));
    if (!fullPath.StartsWith(_allowedBase))
        throw new UnauthorizedAccessException();
    await File.WriteAllBytesAsync(fullPath, pdf);
}
Crítico Deserialización insegura con BinaryFormatter NutritionPlanImporter.cs:112
Problema
BinaryFormatter está obsoleto en .NET 5+ y deshabilitado por defecto en .NET 7+. Deserializar un archivo binario del usuario con él permite ejecución remota de código (RCE). Además, hay un aviso en el compilador que se ignoró con #pragma warning disable.
Corrección
Eliminar completamente BinaryFormatter. Para CSV, usar CsvHelper o StreamReader con parseo manual. Para datos binarios, usar System.Text.Json o MessagePack.
Antes — VULNERABLE
#pragma warning disable SYSLIB0011
var formatter = new BinaryFormatter();
using var stream = File.OpenRead(filePath);
var plan = (NutritionPlan)formatter
    .Deserialize(stream); // ⚠️ RCE
#pragma warning restore SYSLIB0011
Después — SEGURO
using var reader = new StreamReader(filePath);
using var csv = new CsvReader(reader,
    CultureInfo.InvariantCulture);
var records = csv
    .GetRecords<NutritionPlanDto>()
    .ToList();
Problemas altos
Alto Bloqueo síncrono sobre async (.Result) — riesgo de deadlock PatientSearchController.cs:58
Problema
GetPatientsAsync().Result bloquea el hilo del pool de threads. En ASP.NET Core con SynchronizationContext activo puede causar deadlock; en todos los casos desperdicia el hilo durante la espera de I/O.
Corrección
Hacer el método async Task<IActionResult> y usar await.
Antes
public IActionResult Search(string q)
{
    var results = _service
        .GetPatientsAsync(q).Result; // ⚠️
    return Ok(results);
}
Después
public async Task<IActionResult> Search(
    string q, CancellationToken ct)
{
    var results = await _service
        .GetPatientsAsync(q, ct);
    return Ok(results);
}
Alto API async pública sin soporte de CancellationToken NutritionPlanImporter.cs:23, ReportExportService.cs:14
Problema
Los métodos ImportAsync y ExportAsync no aceptan CancellationToken. Si el cliente cancela la petición HTTP, las operaciones de archivo y base de datos seguirán ejecutándose, consumiendo recursos innecesariamente.
Corrección
Añadir CancellationToken ct = default como último parámetro y propagarlo a todas las llamadas async internas (ReadToEndAsync(ct), ExecuteReaderAsync(ct)...).
Alto Suppressión agresiva de nullable warnings con operador ! PatientSearchController.cs:41, 44, 51
Problema
El código usa patient!.Name, result!.Data y record!.DNI con el operador ! de supresión de nullable en tres puntos. Esto silencia las advertencias del compilador sin resolver el problema subyacente; si cualquiera de esos objetos es null en runtime, se producirá un NullReferenceException.
Corrección
Sustituir por guardas explícitas: if (patient is null) return NotFound(); o usar el patrón patient?.Name ?? string.Empty según corresponda.
Alto Catch vacío que traga excepciones silenciosamente NutritionPlanImporter.cs:88
Problema
catch (Exception) { return null; } alrededor del loop de importación CSV. Si falla la lectura de una línea, se devuelve null sin registrar nada. El dietista nunca sabe que la importación fue parcial.
Corrección
Capturar excepciones específicas (CsvHelperException, FormatException), loguear con contexto (_logger.LogError(ex, "Fila {Row}", rowIndex)) y relanzar o devolver un resultado de error tipado.
Problemas medios
Medio Consultas N+1 — lazy loading en loop de resultados PatientSearchController.cs:72
Problema
Por cada paciente retornado se accede a patient.NutritionPlans dentro de un foreach, disparando una query adicional por paciente. Con 100 resultados → 101 queries a base de datos.
Corrección
Usar .Include(p => p.NutritionPlans) en la query EF Core original. Añadir .AsNoTracking() ya que es una consulta de solo lectura.
Medio Concatenación de string en bucle — usar StringBuilder ReportExportService.cs:134
Problema
El método que genera el HTML del informe usa htmlContent += row dentro de un loop de hasta 1.000 filas de datos nutricionales. Cada concatenación crea un nuevo objeto string en el heap → O(n²) en memoria y tiempo.
Corrección
Usar var sb = new StringBuilder(); sb.Append(row); y sb.ToString() al final del loop.
Medio Magic strings para claves de configuración ReportExportService.cs:19, 22, 31
Problema
Claves como "ReportSettings:OutputPath" y "ReportSettings:MaxSizeMB" están duplicadas como literales en 3 sitios. Un typo en una de ellas es silencioso en compilación.
Corrección
Definir una clase ReportSettings con string OutputPath y registrarla con services.Configure<ReportSettings>(config.GetSection("ReportSettings")). Inyectar IOptions<ReportSettings>.
Recomendaciones menores
Bajo Clases no selladas — añadir sealed para claridad y perf NutritionPlanImporter.cs:8   ReportExportService.cs:11
Problema
Dos clases de servicio que no están diseñadas para ser heredadas no tienen el modificador sealed. El compilador puede optimizar mejor los métodos virtuales en clases sealed.
Corrección
Añadir sealed: public sealed class NutritionPlanImporter.
Bajo DTO mutable — convertir a record para inmutabilidad NutritionPlanImporter.cs:6
Problema
NutritionPlanDto es una clase con setters públicos. Al ser un DTO de solo lectura (solo se lee del CSV), debería ser inmutable.
Corrección
Convertir a public record NutritionPlanDto(string Name, decimal Calories, ...).
Checks de framework
⚡ ASP.NET Core
Falta [ValidateAntiForgeryToken] en endpoints de mutación
Sin atributo [Authorize] en PatientSearchController
Model validation via DataAnnotations presente
~ Middleware order: autenticación antes de autorización (OK)
🗄️ EF Core
Lazy loading activo por defecto — usar Include explícito
~ Falta AsNoTracking() en queries de solo lectura
Migrations en carpeta correcta y sin breaking changes
DbContext con lifetime Scoped correcto
🔒 Seguridad
SQL Injection (PatientSearchController:34)
Path Traversal (ReportExportService:67)
Insecure Deserialization (NutritionPlanImporter:112)
HTTPS enforced en pipeline
⚙️ Async / Rendimiento
.Result blocking (PatientSearchController:58)
Sin CancellationToken en 2 servicios
~ String concat en loop (ReportExportService:134)
No hay async void fuera de event handlers
Criterio de aprobación
Sin problemas CRÍTICOS — 3 problemas críticos encontrados
Sin problemas ALTOS — 4 problemas altos encontrados
~ Problemas MEDIOS solo (puede mergear con cautela) — hay críticos y altos: no aplica
🚫
PR BLOQUEADO — No se puede mergear
Resolver los 3 problemas críticos (SQL injection, path traversal, deserialización insegura) y los 4 altos antes de solicitar nueva revisión. Los problemas medios pueden resolverse en el mismo PR o en un follow-up.