# 18 — Problemas Conocidos > Hallazgos del análisis del código, **no corregidos**. Este documento solo describe. > Cada hallazgo indica el fichero y la línea o el método exacto para que pueda verificarse. > > **Clasificación:** 🔴 CRÍTICO · 🟠 ALTO · 🟡 MEDIO · 🔵 BAJO · ⚙️ DEUDA TÉCNICA --- ## Resumen | Severidad | Cantidad | |---|---| | 🔴 Crítico | 6 | | 🟠 Alto | 9 | | 🟡 Medio | 11 | | 🔵 Bajo | 7 | | ⚙️ Deuda técnica | 14 | | **Total** | **47** | --- ## 🔴 CRÍTICOS ### HR-SQL-1 — Las cláusulas WHERE no escapan los valores **Ficheros:** `core/db/MysqlPDO.php::criteriaToSql()` (línea 254) · `core/Model.php` (todos los métodos de lectura) ```php $condition .= " " . $criteria["name"]; $condition .= " " . $criteria["operator"]; $condition .= " " . $criteria["separatorValues"] . $criteria["value"] . $criteria["separatorValues"] . " "; ``` El valor se concatena **sin ningún escapado**. `insert()`, `update()` y `updateCriteria()` sí aplican `addslashes()` a los valores, pero **`criteriaToSql()` no**. La única defensa es que quien construye el criterio castee la entrada. El proyecto ha ido introduciendo ese saneamiento (método privado `cleanText()`, duplicado en 21 controladores, y casts `(int)`), pero **no es sistemático ni verificable**. **Impacto:** inyección SQL en cualquier punto que pase entrada de usuario a un criterio sin castear. **Mitigación real:** sentencias preparadas en `criteriaToSql()`, o un escapado obligatorio en el punto de construcción. --- ### HR-SQL-2 — `criteriaExt`: el cliente puede inyectar criterios arbitrarios **Fichero:** `core/ListaAjax.php::loadCriteria()` (línea 449) ```php if (isset($_REQUEST['criteriaExt']) && !empty($_REQUEST['criteriaExt'])) { $this->Criteria = (array) ($_REQUEST['criteriaExt']); } ``` El array de criterio llega **directamente de la petición** y se pasa a `criteriaToSql()`, que no escapa (HR-SQL-1). **Alcance:** **todos** los endpoints `dataListAjax`, es decir, prácticamente todos los módulos. **Agravante:** `'dataListAjax'` está en `Config::$actionsGeneral`, la lista de acciones exentas de permiso. **Cualquier usuario autenticado puede invocar el endpoint de datos de cualquier módulo**, tenga o no permiso `list` sobre él, e inyectar un criterio arbitrario. **Impacto:** exfiltración de datos de cualquier tabla del sistema por parte de un usuario autenticado con el mínimo privilegio. --- ### HR-SQL-3 — El término de búsqueda se concatena sin escapar **Fichero:** `core/ListaAjax.php::loadCriteria()` (línea ~497) ```php $CadenaBusqueda .= $this->FieldsShow[$j]." LIKE '%".$SeparadorBusqueda[$z]."%' OR "; ``` `$SeparadorBusqueda` proviene de `$_REQUEST['search']['value']` partido por espacios. **Sin `addslashes` ni sentencias preparadas.** **Alcance:** el buscador de todos los listados del sistema. --- ### HR-AUTH-1 — Contraseñas en MD5 sin sal **Fichero:** `core/helpers/PassHelper.php` ```php public static function encode($pass) { return md5($pass); } public static function verify($pass, $hash) { return md5($pass) == $hash; } ``` MD5 es un algoritmo **roto para contraseñas**: es rápido de calcular (miles de millones de intentos por segundo en GPU), no tiene sal y existen tablas rainbow públicas. Además la comparación con `==` no es de tiempo constante. **Agravante — la incoherencia interna:** `LineaEticaCasosModel` **sí** usa `password_hash(PASSWORD_DEFAULT)` y `password_verify()` para el PIN de seguimiento de denuncias. El mecanismo correcto ya está en el proyecto, pero no se aplica al login. **Facilitador de la corrección:** `usuarios.Contrasena` y `colaboradores.Contrasena` son `varchar(500)`. **La migración a `password_hash()` no requiere cambio de esquema.** --- ### HR-LEAK-1 — La traza de excepción se imprime al usuario, siempre **Fichero:** `core/AutoLoad.php` (líneas ~210-215) ```php try { call_user_func(array($app, 'process')); } catch (Error $exception) { echo " {$exception}"; } catch (Exception $exception) { echo " {$exception}"; } ``` **No depende de `APP_DEBUG`.** En producción, cualquier excepción no capturada muestra al usuario la traza completa: rutas absolutas del servidor, nombres de clase, números de línea y, según el caso, fragmentos de SQL con datos. --- ### HR-EVAL-1 — `eval()` sobre datos de la base en el motor de nómina **Fichero:** `app/models/NominaCalculoModel.php::safeEvalFormula()` (línea 246) ```php $expr = strtoupper((string)$formula); foreach ($variables as $key => $value) { $expr = preg_replace('/\b'.preg_quote(strtoupper($key),'/').'\b/', (string)((float)$value), $expr); } if (!preg_match('/^[0-9\.\+\-\*\/\(\)\s]+$/', $expr)) { return 0.0; } $result = eval('return ' . $expr . ';'); ``` La lista blanca de caracteres es estricta y bloquea la ejecución de código arbitrario en la práctica. **Aun así**, es un `eval()` alimentado por la columna `nomina_conceptos.Formula`, editable desde la interfaz por cualquier usuario con permiso sobre el módulo de nómina. Cualquier relajación futura del regex se convierte en ejecución remota de código. **Alternativa:** una biblioteca de evaluación de expresiones matemáticas, o un intérprete propio con parser. --- ## 🟠 ALTOS ### HR-CSRF-1 — `CSRF_STRICT` no tiene efecto **Ficheros:** `app/config/Config.php` (líneas 43, 162) · `.env.example` ```bash grep -rF '$CSRF_STRICT' app core services # → solo app/config/Config.php ``` La variable se declara y se rellena desde el `.env`, pero **ninguna comprobación de CSRF la consulta**. El `.env.example` la recomienda en `true` (*"Recomendado true: exige token CSRF en todas las peticiones POST"*), lo que da una **falsa sensación de seguridad**. --- ### HR-CSRF-2 — La validación de CSRF no está centralizada **Fichero:** `core/Controller.php::process()` El framework solo impone CSRF en: 1. La acción `remove` (para todos los controladores). 2. `Model::save()` cuando la petición es POST. **Toda otra mutación** (`aprobar`, `rechazar`, `cancelar`, `cambiarEstado`, `publicar`, `archivar`, `asignar`, `entregar`, `cerrar`, …) depende de que el autor recuerde escribir la comprobación a mano. Es un patrón repetido decenas de veces y fácil de olvidar. **Mitigación parcial existente:** la cookie de sesión es `SameSite=Strict`, lo que bloquea las peticiones cross-site. No protege frente a un XSS almacenado. --- ### HR-IDOR-1 — No hay verificación centralizada de propiedad de registro Ninguna capa comprueba que el registro al que se accede pertenezca al usuario de la sesión. `Controller::$PUBLIC_COLLABORATOR_ROUTES` autoriza **la ruta**, no **el registro**. Que un colaborador pueda ejecutar `?c=Tickets&a=ver` no significa que solo pueda ver sus propios tickets: eso depende de que la acción aplique `listarConFiltros($filtros, $soloColaboradorId)`. **Existe un mecanismo parcial**: el permiso con valor `2` ("solo mis registros") añade `WHERE UsuarioRegistro = `, **pero solo lo implementa `ListaAjax`**. Una acción `view`/`edit` invocada con un `Id` ajeno no aplica esa restricción. `tests/MutationSecurityRegressionTest::testUserProfileOperationsValidateOwnershipAndUploadedImages` cubre el caso de `UsuariosController`, pero no el resto. --- ### HR-PERM-1 — `dataListAjax` está exento de permisos **Fichero:** `app/config/Config.php` ```php public static $actionsGeneral = array('dataListAjax', 'testDB'); ``` `Controller::validateAccess()` concede `ACCESS` a cualquier acción de esta lista **para cualquier usuario autenticado**, sin consultar la tabla `permisos`. Combinado con HR-SQL-2 (`criteriaExt`), un colaborador del portal puede consultar el listado de nómina, salarios o casos de la línea de ética. `'testDB'` también está exenta: es la acción de `Controller::testDB($modelName)` que renderiza `_test/db`. --- ### HR-PERM-2 — El portal concede permisos por código, eludiendo la base de datos **Fichero:** `app/models/ColaboradoresModel.php::validateUser()` (líneas 464-600+) Tras cargar los permisos del rol 5 desde la base, el método **sobrescribe la sesión** con asignaciones incondicionales: ```php $_SESSION[APP_ID]['Permissions']['Colaboradores']['list'] = 1; $_SESSION[APP_ID]['Permissions']['Colaboradores']['view'] = 1; $_SESSION[APP_ID]['Permissions']['Colaboradores']['edit'] = 1; // … y bloques if(!isset(…)) para ~12 módulos más ``` **Consecuencias:** - Quitar un permiso al rol 5 en la base **no tiene efecto** sobre los asignados incondicionalmente. - `Colaboradores` es el mismo `$MODULE_NAME` que el CRUD administrativo de colaboradores. Sin la lista blanca de rutas, un colaborador tendría acceso al CRUD completo. - La única barrera efectiva es `Controller::$PUBLIC_COLLABORATOR_ROUTES`, una lista **hardcodeada en `core/Controller.php`**: mezcla la política de autorización con el framework. --- ### HR-TX-1 — Los flujos multi-tabla no usan transacciones `grep -rn "beginTransaction"` sobre `app/`, `core/` y `services/` devuelve **una sola ocurrencia**: `ColaboradoresModel::deleteCascade()`. Flujos afectados: | Flujo | Riesgo | Fichero | |---|---|---| | Aprobar vacaciones | Cambia el estado y **después** descuenta el saldo. Si lo segundo falla, la solicitud queda aprobada sin descontar días. El código lo reconoce: *"La solicitud cambió de estado, pero hubo un problema actualizando el saldo"* | `VacacionesController::aprobarSolicitudInterna()` | | Aprobar beneficio | Cambia el estado y **después** consume cupo y presupuesto | `BeneficiosSolicitudesController` + `BeneficiosModel::aplicarConsumoAprobacion()` | | Recalcular nómina | Guarda la liquidación, **borra** el detalle y lo regenera fila a fila | `NominaCalculoModel::recalcularColaborador()` | | Contratar candidato | Cambia el estado, escribe historial y actualiza el colaborador | `RecruitmentApplicationsModel::moverEtapa()` | | Desembolsar adelanto | Cambia el estado, crea desembolso, plan y N cuotas | `AdelantosNominaController::registrarDesembolso` | --- ### HR-RACE-1 — Consumo de cupo de beneficios sin bloqueo **Fichero:** `app/models/BeneficiosModel::aplicarConsumoAprobacion()` ```php $fresh = static::getById($id); // READ if (!static::tieneDisponibilidad($fresh, $monto)) { // CHECK return false; } // … calcula el nuevo cupo y presupuesto … return static::editFromParameters(…); // WRITE ``` Patrón read-check-write **sin transacción ni `SELECT … FOR UPDATE`**. Dos aprobaciones concurrentes del último cupo pueden pasar ambas y dejar `CupoUsado > CupoTotal`. El método hace lo correcto al releer el registro fresco, pero eso no basta sin bloqueo. --- ### HR-014 — El DocumentRoot puede exponer el código fuente **Fichero:** `.htaccess` de la raíz (documentado en el propio fichero) > *NOTA: esto es una mitigación, no la solución. La corrección real es que el DocumentRoot del vhost apunte a `public/` y no a la raíz del proyecto; mientras no sea así, `app/`, `core/`, `database/` y `.git/` siguen siendo alcanzables.* Si el DocumentRoot apunta a la raíz, `.htaccess` bloquea `.env`, `.gitignore`, `composer.json/lock` y `sftp.json`, y desactiva los listados. **No bloquea `app/`, `core/`, `database/` ni `.git/`.** **Complicación:** mover el DocumentRoot a `public/` deja fuera `files/` (que el layout referencia como `URL::base_url().'/../files/…'`) y `core/ajax.js`. Requiere un `Alias`. --- ### HR-DEAD-1 — 20 clases referenciadas que no existen Ver el inventario completo en [12_INTEGRATIONS.md](12_INTEGRATIONS.md) §12. ``` AsignaturasModel · DebugModel · DirectoresEvaluacionesModel · DirectoresModel DirectorioActivoModel · DocentesAsignaturasModel · DocentesEvaluacionesModel DocentesModel · EstudiantesAsignaturasModel · EstudiantesEvaluacionesModel EstudiantesModel · EstudiantesRespuestasModel · FacultadesModel · LaborDocenteModel OdsDocentesModel · PostulacionesModel · ProduccionIntelectualModel · ProgramasModel ProyectosDocentesModel · ResumenEvaluacionModel ``` Cualquier ruta que las alcance produce **error fatal**, y por HR-LEAK-1 la traza se muestra al usuario. **Caso más peligroso — `DirectorioActivoModel`:** con `DIRECTORIO_ACTIVO=true`, `UsuariosModel::validateUser()` la invoca cuando la contraseña local falla → error fatal en el flujo de autenticación. --- ## 🟡 MEDIOS ### HR-CONF-1 — Cinco variables de configuración sin efecto Verificado con `grep -rF` sobre `app/`, `core/` y `services/`: | Variable | Documentada como | Efecto real | |---|---|---| | `CSRF_STRICT` | *"Exige token CSRF en todas las peticiones POST"* | **Ninguno** | | `IP_BLOCKING` | Bloqueo por IP (con tabla `ips_autorizadas` y CRUD completos) | **Ninguno**: nada comprueba la IP | | `NO_COPY` | Impedir copiar contenido | **Ninguno** | | `CAMBIO_ROL` | Cambio de rol en caliente | **Ninguno** | | `LOGIN_AUTOMATICO` | Login automático | **Ninguno** | | `MAIL_DIARIOS_MAX` / `$correos_diarios` | Tope diario de correos | **Ninguno** | | `APP_NO_PAYMENT` / `$noPayment` | Cliente moroso | **Ninguno** | Cada una de ellas es una **falsa expectativa** para quien configura el sistema. --- ### HR-DOC-1 — La documentación técnica preexistente describe otro producto **Ficheros:** `docs/01_GETTING_STARTED.md` … `docs/06_STANDARDS.md` y `docs/AI_MODULE_DEVELOPMENT_GUIDE.md` (3 543 líneas) Estos documentos describen una arquitectura que **no existe en este repositorio**: | Documentado | Realidad | |---|---| | `core/Request.php`, `core/Response.php` | ❌ No existen | | `core/MailService.php`, `core/EventDispatcher.php` | ❌ No existen | | `core/TenantContext.php`, `core/App.php` | ❌ No existen | | `MiddlewarePipeline`, `AuthMiddleware`, `CsrfMiddleware`, `RateLimitMiddleware` | ❌ No existen | | Traits `HasValidation`, `HasTenant`, `HasCache`, `HasAuditLog` | ❌ `grep "trait Has"` → 0 resultados | | `app/config/Modules.php` con `$registry` | ❌ No existe | | `bin/make module Producto` | ❌ Existen `bin/make-migration` y `bin/make-seeder` | | `appId = 'KleePlanner2'`, `MODULES_OVERRIDE`, `TENANTS` | ❌ Es otro producto | | `Model::find()`, `findAll()`, `deleteFromParameters()` | ❌ No existen en `core/Model.php` | | `$_SERVER['KLEE_CSRF_VALIDATED']` | ❌ No se usa en ninguna parte | **Riesgo:** una IA o un desarrollador nuevo que lea esos documentos escribirá código que no compila ni se ejecuta. **Recomendación:** archivarlos o marcarlos como obsoletos, apuntando a `docs/knowledge/`. `docs/guias_modulos/` (manuales funcionales) y `database/README.md` **sí son correctos y vigentes**. --- ### HR-VIEW-1 — `vista_colaboradores` expone la columna `Contrasena` **Verificado:** `SHOW CREATE VIEW vista_colaboradores` incluye `c.Contrasena AS Contrasena`. `ColaboradoresModel::getAll()`, `getById()` y `getByCriteria()` están **sobreescritos para leer siempre de la vista**, y su uso habitual es `array('*')`. El hash de contraseña acaba en memoria en cualquier listado de colaboradores, y en la respuesta de cualquier endpoint que serialice la fila completa. --- ### HR-VIEW-2 — El docblock de `ColaboradoresModel` describe columnas inexistentes **Fichero:** `app/models/ColaboradoresModel.php` (líneas ~390, ~400) ```php /** * Sobrescribe getAll para leer siempre desde la vista. * La vista incluye aliases de compatibilidad (Nombre, Apellido, NoIdentificacion, Cargo, Dependencia). */ ``` **Falso.** Las únicas columnas de `vista_colaboradores` con esos nombres son `Nombres`, `Apellidos` e `IdCargo`; el único alias añadido es **`NombreCompleto`**. No existen `Nombre`, `Apellido`, `NoIdentificacion`, `Cargo` ni `Dependencia`. Cualquier criterio que filtre por esos alias falla con `Column not found`. --- ### HR-VIEW-3 — Las 62 vistas usan `DEFINER = root@localhost` Todas se crean con `SQL SECURITY DEFINER` y ese definidor. Al restaurar un volcado en un servidor donde `root@localhost` no exista, **todas las vistas fallan** con `ERROR 1449`, y con ellas la mayor parte de la aplicación. --- ### HR-SESS-1 — `logout` no destruye la sesión **Fichero:** `app/controllers/LoginController.php::logoutAction()` ```php public function logoutAction() { unset($_SESSION[Controller::getAppId()]); ROUTER::redirect_to_action(); } ``` No llama a `session_destroy()`, no regenera el identificador y no invalida la cookie. La rama `$_SESSION['public']` (usada por `Model::log()`) sobrevive al cierre de sesión. --- ### HR-RATE-1 — El rate limiter vive en la sesión **Fichero:** `core/RateLimiter.php` ```php $_SESSION[Controller::getAppId()][self::SESSION_KEY] ``` Un atacante que descarte la cookie en cada intento **reinicia el contador**. No protege contra fuerza bruta automatizada. **Mitigación posible:** almacenar los intentos en base de datos o en fichero, indexados por IP + usuario. --- ### HR-NOM-1 — `BaseCalculo = 'NETO'` se comporta como `TOTAL_DEVENGADO` **Fichero:** `app/models/NominaCalculoModel::resolverBase()` ```php if ($baseCalculo === 'TOTAL_DEVENGADO') { return (float)$totalDevengadoParcial; } if ($baseCalculo === 'NETO') { return (float)$totalDevengadoParcial; } // ← idéntico ``` Un concepto configurado con base `NETO` produce un valor **incorrecto**: usa el devengado parcial en lugar del neto. --- ### HR-NOM-2 — El resultado de la nómina depende del orden de los conceptos `BaseCalculo = 'TOTAL_DEVENGADO'` usa el acumulado **parcial** hasta ese punto del bucle. El resultado depende del orden en que `NominaConceptosModel::listarActivos()` devuelva las filas, y **`nomina_conceptos` no tiene columna de orden explícita**. Cambiar el orden de inserción de los conceptos cambia el resultado de la liquidación. --- ### HR-CSP-1 — La CSP es incoherente con los recursos que se cargan **Fichero:** `core/AutoLoad.php` ``` Content-Security-Policy: default-src 'self' 'unsafe-inline' data:; ``` - **No declara** `fonts.googleapis.com`, `fonts.gstatic.com` ni `cdn.datatables.net`, que sí se cargan (fuente Poppins y traducción de DataTables). - Incluye `'unsafe-inline'`, necesario porque `ListaAjax` genera `