Revisión de Código Java — NutriTrack PR #142

⚠️

Escalación a security-reviewer requerida

Se detectaron 2 issues CRÍTICOS de seguridad (SQL injection + hardcoded secret). Según el protocolo del revisor, este PR queda bloqueado y debe ser revisado por el equipo de seguridad antes de continuar. Ningún HIGH puede mergearse hasta que los CRITICALs estén resueltos.

📁

Archivos auditados (5 ficheros)

Resumen de issues por fichero

Fichero	Capa	Issues	Estado
subscription/SubscriptionController.java	Controller	2 altos	⚡ ALTO
subscription/SubscriptionService.java	Service	1 crítico, 1 alto, 1 medio	🚫 CRÍTICO
user/UserRepository.java	Repository	1 medio	⚠️ MEDIO
auth/AuthController.java	Controller	1 crítico, 1 medio	🚫 CRÍTICO
subscription/SubscriptionServiceTest.java	Test	2 medios	⚠️ MEDIO

🔴

CRÍTICO — Seguridad

2 issues · Bloquean el merge inmediatamente

CRITICAL

SQL Injection — concatenación de string en consulta nativa

subscription/SubscriptionService.java · línea 47

La consulta nativa construye el WHERE con concatenación directa del parámetro companyName recibido del request body, sin sanitización. Un atacante puede inyectar SQL arbitrario y exfiltrar o corromper la base de datos completa. Uso de @Query con parámetro posicional o nombrado es obligatorio.

❌ Código problemático

@Query(value = "SELECT * FROM subscriptions WHERE company = '"
    + companyName + "'", nativeQuery = true)
List<Subscription> findByCompany(String companyName);

// Ejemplo explotación:
// companyName = "' OR 1=1 --"

✅ Corrección requerida

@Query(value = "SELECT * FROM subscriptions WHERE company = :name",
       nativeQuery = true)
List<Subscription> findByCompany(
    @Param("name") String companyName
);

📋 Regla: SQL Injection — Spring @Query con bind parameters 🔗 CWE: CWE-89

CRITICAL

Secreto hardcodeado — JWT signing key en código fuente

auth/AuthController.java · línea 23

La clave privada de firma JWT está embebida directamente como literal de String en el código fuente. Si este repositorio es accesible (incluso internamente), cualquier persona con acceso puede firmar tokens arbitrarios y suplantar a cualquier usuario. La clave debe provenir de application.yml o un secrets manager (Vault / AWS Secrets Manager). El historial de git también debe ser purgado.

❌ Código problemático

private static final String JWT_SECRET =
    "nutritrack-super-secret-2024-x9z!";

public String generateToken(String userId) {
    return Jwts.builder()
        .signWith(Keys.hmacShaKeyFor(
            JWT_SECRET.getBytes()))
        .compact();
}

✅ Corrección requerida

@Value("${app.jwt.secret}")
private String jwtSecret;

// En application.yml (o env var):
# app:
#   jwt:
#     secret: ${JWT_SECRET}

public String generateToken(String userId) {
    return Jwts.builder()
        .signWith(Keys.hmacShaKeyFor(
            jwtSecret.getBytes()))
        .compact();
}

📋 Regla: Hardcoded secrets — Spring @Value / secrets manager 🔗 CWE: CWE-321

🟠

ALTO — Arquitectura & JPA

3 issues · Bloquean el merge

HIGH

Field injection con @Autowired — usar constructor injection

subscription/SubscriptionController.java · línea 15, 16

Dos dependencias se inyectan directamente sobre campos con @Autowired. Esto es un code smell en Spring Boot moderno: dificulta los tests unitarios (no puedes instanciar el objeto sin Spring), oculta las dependencias y hace la clase más frágil. Constructor injection es el estándar idiomático recomendado por el Spring team.

❌ Field injection

@RestController
public class SubscriptionController {

    @Autowired
    private SubscriptionService service;

    @Autowired
    private NotificationService notif;
}

✅ Constructor injection

@RestController
@RequiredArgsConstructor // Lombok OK
public class SubscriptionController {

    private final SubscriptionService service;
    private final NotificationService notif;

    // Spring inyecta via constructor
}

📋 Regla: Dependency injection — [SPRING] constructor injection requerido

HIGH

Entidad JPA expuesta directamente desde el Controller

subscription/SubscriptionController.java · línea 34

El endpoint de creación devuelve directamente la entidad Subscription (anotada con @Entity) como cuerpo del response. Esto expone el modelo de datos interno, puede serializar campos sensibles no intencionados, y acopla el contrato API al esquema de base de datos. Se debe usar un DTO o record de proyección.

❌ Entidad expuesta

@PostMapping("/subscriptions")
public ResponseEntity<Subscription> create(
        @RequestBody SubscriptionRequest req) {
    Subscription saved = service.create(req);
    return ResponseEntity.ok(saved);
}

✅ DTO de respuesta

@PostMapping("/subscriptions")
public ResponseEntity<SubscriptionDto> create(
        @Valid @RequestBody SubscriptionRequest req) {
    SubscriptionDto dto = service.create(req);
    return ResponseEntity.status(201).body(dto);
}

record SubscriptionDto(UUID id, String plan,
    Instant createdAt) {}

📋 Regla: Entity exposed in response — usar DTO/record projection

HIGH

N+1 query — FetchType.EAGER en colección OneToMany

subscription/SubscriptionService.java · línea 61 (via UserRepository)

La relación @OneToMany(fetch = FetchType.EAGER) sobre licenses provoca que al cargar cualquier Subscription, se emitan N+1 queries adicionales para cada elemento de la lista. Con 100 suscripciones activas, esto genera 101 queries por request. Usar FetchType.LAZY con JOIN FETCH o @EntityGraph cuando sea necesario.

❌ EAGER — N+1 queries

@Entity
public class Subscription {

    @OneToMany(fetch = FetchType.EAGER,
               cascade = CascadeType.ALL)
    private List<License> licenses;
}

✅ LAZY + EntityGraph

@Entity
@NamedEntityGraph(name = "sub.licenses",
    attributePaths = {"licenses"})
public class Subscription {

    @OneToMany(fetch = FetchType.LAZY,
               cascade = CascadeType.ALL)
    private List<License> licenses;
}

📋 Regla: N+1 — usar LAZY + JOIN FETCH / @EntityGraph

🟡

MEDIO — Concurrencia, Tests & Idioms

4 issues · No bloquean el merge, pero deben corregirse antes de producción

MEDIUM

Campo mutable no final en @Service singleton

subscription/SubscriptionService.java · línea 31

El campo requestCounter es un int mutable en un bean @Service (singleton). Bajo carga concurrente, múltiples threads lo incrementan simultáneamente sin sincronización — race condition clásica. Usar AtomicInteger o Micrometer Counter.

❌ Race condition

@Service
public class SubscriptionService {
    private int requestCounter = 0;

    public void process() {
        requestCounter++; // NO thread-safe
    }
}

✅ AtomicInteger

@Service
public class SubscriptionService {
    private final AtomicInteger requestCounter
        = new AtomicInteger(0);

    public void process() {
        requestCounter.incrementAndGet();
    }
}

📋 Regla: Mutable singleton fields — usar tipos atómicos o Micrometer

MEDIUM

@SpringBootTest en test unitario de servicio

subscription/SubscriptionServiceTest.java · línea 8

La clase de test usa @SpringBootTest para probar únicamente la lógica del SubscriptionService. Esto levanta el contexto Spring completo (base de datos, beans, servidor), incrementando el tiempo de CI en ~8–15 segundos por test innecesariamente. Usar @ExtendWith(MockitoExtension.class) para tests unitarios de servicios.

📋 Regla: Over-scoped test — [SPRING] @SpringBootTest solo para integración

MEDIUM

PII logging — password logueado en flujo de autenticación

auth/AuthController.java · línea 41

Se llama log.info("Login attempt: user={}, pwd={}", username, password) en el flujo de login. Las contraseñas aparecerán en texto plano en los logs de la aplicación (Logback, Elastic, CloudWatch…). Loguear únicamente el username o un hash no reversible del evento.

❌ Password en log

log.info("Login attempt: user={}, pwd={}",
    username, password);

✅ Solo username

log.info("Login attempt: user={}", username);
// NUNCA loguear password, tokens, PIIs

📋 Regla: PII/token logging — Spring SLF4J cerca de auth code

MEDIUM

Nombre de test débil — sin comportamiento esperado

subscription/SubscriptionServiceTest.java · línea 22

Los métodos de test se llaman testCreate, testFind. Si uno falla, el nombre no comunica qué contrato fue violado. Los nombres deben describir el escenario y el resultado esperado.

❌ Nombre vacío

@Test
void testCreate() { ... }

@Test
void testFind() { ... }

✅ Nombre descriptivo

@Test
void should_create_subscription_when_plan_valid(){}

@Test
void should_throw_404_when_sub_not_found() {}

📋 Regla: Weak test names — should_<comportamiento>_when_<escenario>

✅

Criterios aprobados (6 de 12)

Buenos aspectos del PR que se mantienen

✓ Bean Validation presente — @Valid correctamente aplicado en el request body del endpoint de creación de empresa.
✓ @RestControllerAdvice presente — existe GlobalExceptionHandler.java con manejo centralizado de excepciones; buena práctica.
✓ @Transactional en capa de servicio — las mutaciones están correctamente anotadas en SubscriptionService, no en el controller.
✓ Optional usado correctamente — repository.findById(id).orElseThrow() sin llamadas ciegas a .get().
✓ Paginación en endpoint de listado — el endpoint GET /subscriptions acepta Pageable y devuelve Page<T>.
✓ Framework detectado correctamente — pom.xml confirma Spring Boot 3.2.4; ruleset Spring aplicado íntegramente.

Puntuación de calidad

28 / 100

Issues críticos de seguridad reducen la puntuación a 0 hasta resolución

0 — Bloqueado 100 — Aprobado

Próximos pasos (orden requerido)

1.Escalar CRITICALs al security-reviewer — SQL injection + JWT key
2.Eliminar JWT secret del historial de git (git filter-repo) y rotar la clave
3.Refactorizar controller: constructor injection + DTO de respuesta
4.Corregir FetchType.EAGER → LAZY + @EntityGraph
5.Resolver 4 issues MEDIUM antes de merge a staging
6.Re-solicitar revisión con PR actualizado