Revisión PHP — NutriPaw PR #48

Herramientas automatizadas

✖ PHPStan — 7 errores nivel 8

✖ Psalm — 3 issues unsafe

✖ Pint — 12 violaciones PSR-12

⚠ PHPUnit — 61% cobertura (min 80%)

⚠ composer audit — 1 dep. obsoleta

✖ N+1 detector — 2 consultas N+1

Archivos analizados

🐘

SubscriptionController.php

2 críticos 2 altos

🐘

CustomerExportController.php

1 crítico 1 alto

🐘

PaymentService.php

2 altos 1 medio

🐘

Subscription.php (Model)

2 medios

🐘

User.php (Model)

1 medio

Críticos — Seguridad

CRÍTICO SQL Injection vía whereRaw con input de usuario SubscriptionController.php:87

Problema

Se interpola directamente $request->search en whereRaw() sin parametrizar, permitiendo inyección SQL arbitraria por cualquier usuario autenticado.

// ❌ VULNERABLE
$subs = Subscription::whereRaw(
    "pet_name LIKE '%{$request->search}%'"
)->get();
        

Corrección

Usar la interpolación parametrizada de Eloquent o el helper where() con LIKE y binding seguro.

// ✅ SEGURO
$subs = Subscription::where(
    'pet_name', 'LIKE',
    '%' . $request->validated('search') . '%'
)->get();
        

CRÍTICO Mass Assignment sin $fillable en modelo Subscription SubscriptionController.php:54

Problema

Subscription::create($request->all()) sin que el modelo defina $fillable. Un atacante puede sobrescribir campos como status, price_override o is_admin inyectando parámetros extra en el POST.

// ❌ PELIGROSO
Subscription::create($request->all());
// Subscription.php — sin $fillable definido
        

Corrección

Usar FormRequest para validar y añadir $fillable con los campos permitidos en el modelo.

// ✅ Controller
Subscription::create($request->validated());
// Subscription.php
protected $fillable = [
    'user_id', 'plan_id', 'pet_name',
    'starts_at', 'ends_at',
];
        

CRÍTICO Exportación CSV sin autorización (IDOR) CustomerExportController.php:23

Problema

El endpoint /admin/customers/export retorna todos los usuarios con email, nombre y datos de pago sin verificar que el usuario sea administrador. Cualquier usuario autenticado puede descargar el CSV completo.

// ❌ SIN AUTORIZACIÓN
public function export(Request $request)
{
    $users = User::with('subscriptions')
                 ->get();
    return $this->streamCsv($users);
}
        

Corrección

Proteger con Gate o Policy, y limitar los campos exportados. Registrar en log de auditoría.

// ✅ CON GATE + CHUNKING
public function export(Request $request)
{
    Gate::authorize('export-customers');
    activity()->log('customer-export');
    return $this->streamCsvChunked();
}
        

Altos — Estándares y Rendimiento

ALTO Consulta N+1: relaciones sin eager loading SubscriptionController.php:102

Problema

Se itera sobre $subscriptions accediendo a $sub->user->email dentro del bucle sin haber hecho with('user'). Con 500 suscripciones = 501 queries.

$subs = Subscription::all();
foreach ($subs as $sub) {
    // N+1: query por cada $sub
    echo $sub->user->email;
}
        

Corrección

Añadir with('user') (o definir $with en el modelo si la relación se usa siempre).

$subs = Subscription
    ::with('user')
    ->get(); // 2 queries total
        

ALTO declare(strict_types=1) ausente en 3 archivos PaymentService.php · SubscriptionController.php · CustomerExportController.php

Problema

PHP 8.3 permite tipos estrictos por archivo. Sin declare(strict_types=1) las conversiones implícitas pueden enmascarar bugs de tipo (ej. int "1" → 1 sin error).

<?php
// ❌ Falta declare
namespace App\Services;
        

Corrección

Añadir en la primera línea tras el tag de apertura en todos los archivos que no sean vistas Blade.

<?php
declare(strict_types=1);

namespace App\Services;
        

ALTO Lógica de negocio en el controlador (PaymentService inline) SubscriptionController.php:67-140

Problema

El método store() tiene 74 líneas e incluye cálculo de precio, llamada a Stripe, creación del modelo y envío de email. Viola SRP, imposible de testear unitariamente.

Corrección

Extraer a CreateSubscriptionAction (patrón Action de Laravel). El controlador solo orquesta: valida → action → responde.

// SubscriptionController.php
public function store(
    StoreSubscriptionRequest $req,
    CreateSubscriptionAction $action
) {
    return $action->handle($req->validated());
}
        

ALTO Clave de Stripe hardcodeada en PaymentService PaymentService.php:15

Problema

La clave secreta de Stripe sk_live_… está directamente en el código fuente. Quedará expuesta en el historial de git para siempre.

// ❌ SECRETO EXPUESTO
private string $key =
    'sk_live_4xT9rK…Bz';
        

Corrección

Leer de config('services.stripe.secret') (que a su vez lee de .env). Rotar la clave comprometida inmediatamente.

// ✅ config/services.php
'stripe' => [
    'secret' => env('STRIPE_SECRET'),
],
        

ALTO Exportación CSV sin paginación (memory exhaustion) CustomerExportController.php:35

Problema

User::with('subscriptions')->get() carga todos los usuarios en memoria. Con 50k registros el proceso PHP agotará memoria y el servidor caerá.

Corrección

Usar cursor() o chunk() para streaming del CSV sin saturar memoria.

User::with('subscriptions')
    ->cursor()
    ->each(fn($u) =>
        fputcsv($handle, $u->toCsvRow())
    );
        

Medios — Buenas Prácticas

MEDIO dd() y dump() dejados en código comprometido PaymentService.php:88 · Subscription.php:44

Problema

Dos llamadas a dd($response) y dump($this->attributes) quedaron sin eliminar. En producción interrumpen la respuesta HTTP y exponen datos internos.

Corrección

Eliminar todas las llamadas. Configurar el linter para bloquear dd/dump/var_dump en pre-commit (ya soportado por Pint con la regla no_debug_backtrace).

MEDIO count() sobre colección Eloquent — preferir isEmpty() SubscriptionController.php:119 · User.php:67

Problema

if (count($subscriptions) > 0) cuando solo se quiere comprobar si hay elementos. Menos expresivo y lanza una query adicional en colecciones lazy.

Corrección

Usar $subscriptions->isNotEmpty() para claridad semántica. Solo usar count() cuando el número exacto sea necesario.

MEDIO PSR-12: imports sin ordenar y spacing incorrecto (12 violaciones Pint) Todos los archivos

Problema

Pint detecta 12 violaciones: imports mezclados (clases, interfaces, traits), espaciado alrededor de operadores y llaves de cierre sin línea en blanco.

Corrección

Ejecutar ./vendor/bin/pint (no solo --test) para aplicar correcciones automáticamente. Añadir al pre-commit hook de CI.

MEDIO Modelo Subscription sin $casts para fechas y enums Subscription.php

Problema

starts_at/ends_at retornan strings en lugar de objetos Carbon. El campo status es un string sin enum, perdiendo type-safety.

Corrección

protected $casts = [
    'starts_at' => 'datetime',
    'ends_at'   => 'datetime',
    'status'    => SubscriptionStatus::class,
];
        

Revisión PHP — NutriPaw / PR #48

Problema

Corrección

Problema

Corrección

Problema

Corrección

Problema

Corrección

Problema

Corrección

Problema

Corrección

Problema

Corrección

Problema

Corrección

Problema

Corrección

Problema

Corrección

Problema

Corrección

Problema

Corrección