El que me dio más vergüenza fue el primero. El buscador de recuperación de una API de transcripción que llevaba meses en producción recibía la lista de identificadores a los que el usuario tenía acceso y filtraba el corpus con ella. Si la lista llegaba vacía, el código entendía "no hay filtro" en vez de "este usuario no puede ver nada". Y devolvía todo.
Lo había escrito yo, lo había revisado yo, y tenía tests. Los tests pasaban antes de arreglarlo y siguieron pasando después, porque en ninguno había un usuario sin acceso a nada. ¿Para qué iba a escribir ese caso? En mi cabeza ese usuario no existía.
Ese día cerré seis fallos de seguridad en la misma API. No usé ninguna herramienta especial. Estaba documentando decisiones de diseño, al releer me empezaron a chirriar cosas, y cambié la pregunta con la que leía: dejé de preguntarme si el código hacía lo que yo quería y empecé a preguntarme cómo lo rompería si fuera otra persona con ganas de joderme.
Suena a consejo de póster motivacional, lo sé. Pero en la práctica cambia qué valores te imaginas. Cuando programas piensas en datos razonables. Cuando atacas piensas en el vacío, el cero, el negativo, el enorme, el que llega dos veces. Y esos casi nunca están en la suite.
El vacío es el sitio donde falla abierto
Lo del ACL tiene nombre: fallar abierto. La ausencia de permisos se traducía en permitirlo todo. Y es fácilísimo escribirlo sin querer, porque el código natural para "filtra si hay filtro" es algo así:
// ilustrativo: el patrón, no el código real
const where = allowedIds.length > 0 ? inArray(docs.id, allowedIds) : undefined;
Ese undefined parece inocente. En la mayoría de query builders significa "sin condición", o sea, todas las filas. Para un filtro de búsqueda por texto está bien. Para un filtro de autorización es un agujero.
Ahora, cada vez que veo un filtro de permisos, lo primero que miro es qué hace con la colección vacía. La pregunta de "¿y si el usuario no tiene permiso?" suele estar cubierta; la del vacío casi nunca.
Lo demás, más o menos por familias
Los otros hallazgos no eran tan espectaculares, pero tenían un parecido incómodo entre sí.
Había un endpoint de exportación que dejaba sacar datos sin exigir la autoridad de gestión de estructura. Y un borrado en cascada de un dominio que comprobaba el permiso una vez, al principio, en lugar de comprobar la autoridad de borrado duro sobre cada elemento que se llevaba por delante. Las cascadas son el sitio clásico para esto: autorizas la operación de arriba y luego el bucle de dentro hace cosas que nadie ha autorizado.
Los listados de administración pasaban el limit que llegara directamente a la consulta, sin techo. Desde la UI siempre valía diez, así que nunca lo vi. Desde un script, un valor grande materializaba la tabla entera en la memoria del worker. El arreglo cabe en una línea, Math.min(Math.max(limit, 1), 1000), y aun así no estaba.
Luego estaba authenticateApiKey, que validaba el hash, la expiración y el flag isActive. Todo correcto, en teoría. El problema era que la fila de la clave vive más que la cuenta de su dueño: si el registro de facturación se limpiaba al dar de baja a alguien, nadie tocaba ese flag y la clave seguía autenticando. La revalidación la puse dentro de la propia función (que el propietario siga teniendo registro de facturación), para que todos los endpoints que autentican por clave la hereden sin que cada uno tenga que acordarse. Si dependo de acordarme, ya sé cómo acaba.
Una inyección sin una línea de JavaScript
Este fue el que me cambió la forma de pensar, no solo el código.
El saneador de markdown permitía style y class libres en div, span y svg. La cadena va de raw a sanitize a harden, y ninguna de las tres etapas mira dentro del CSS: la librería de saneado trata style como una cadena opaca, y el endurecedor solo reescribe URLs en a e img. El estilo llegaba intacto al render.
Yo tenía en la cabeza que "inyección" equivale a ejecutar script, y como no se ejecutaba script, tranquilo. Pero con CSS puedes tapar un botón con otro, poner un texto encima de otro o cambiar lo que parece que dice un mensaje. Si la vista pinta contenido que ha escrito otro usuario, eso es un ataque de un usuario a otro, con o sin JavaScript.
La tentación fue escribir un parser de CSS con lista blanca de propiedades. No lo hice. Comprobé que ninguna vista legítima necesitaba style y quité los dos atributos de la lista permitida. Un filtro de CSS casero es superficie nueva que mantener para cubrir un caso de uso que no existía. Si algún día aparece ese caso, lo pensaré entonces.
Cosas que viven más de lo que deberían
Los dos últimos no eran bugs de lógica, sino de ciclo de vida, y creo que son los que más se escapan en cualquier revisión porque el código hace exactamente lo que dice.
Cuando la autenticación por API key fallaba por una clave mal formada, el catch metía la clave en bruto en el contexto del log, y el logger lo serializaba con JSON.stringify sin redactar nada. Una clave válida mandada a un endpoint equivocado acababa en texto claro en los logs. La redacción que puse mira por dos lados: el nombre del campo (/rawkey|token|authorization|password/i) y la forma del valor (los prefijos dk_live_ y dk_test_). Deja visibles los ocho primeros caracteres seguidos de ***REDACTED***, el mismo formato con el que la UI reesconde la clave después de enseñarla una vez. Lo de mirar también la forma del valor me parece importante: los nombres de campo se los inventa cada uno, pero el prefijo de tus claves lo controlas tú.
Y las URLs firmadas de S3 del reproductor de audio duraban siete días. Siete días para un enlace que solo tiene que aguantar a que cargue un <audio>. Cualquiera con el link (reenviado, en un historial, cacheado por un proxy corporativo) conservaba acceso al audio original todo ese tiempo, y borrar la transcripción no revocaba nada porque la cascada solo borraba la fila. Bajé la expiración a 600 segundos, con renovación desde el cliente a los 480, y añadí un DeleteObjectCommand sobre el objeto al borrar la transcripción.
Lo único que se rompió fue reenviar el enlace del audio por correo a un compañero. Alguien me dirá que eso era una funcionalidad. Yo diría que era un agujero que la gente usaba, que no es lo mismo: nunca estuvo soportado ni tenía ningún control de acceso. Entiendo que haya quien lo vea distinto si tiene usuarios que dependían de ello.
"Para eso paga un pentest"
Es el comentario que espero, y en parte lo comparto. Una auditoría externa ve cosas que tú no vas a ver nunca, porque no tiene tus puntos ciegos. Pero no la vas a pagar cada vez que tocas un endpoint, y la mayoría de lo que encontré no necesitaba a un experto. Necesitaba que alguien leyera el código con mala leche. Puedo ser yo, un rato, si me obligo.
Lo de obligarme es la parte que he intentado resolver, porque esta auditoría salió de casualidad y no quiero depender de otra casualidad. Cada hallazgo acaba en un fichero corto en docs/security/ con el vector y por qué el arreglo es ese y no otro. De esos ficheros sale la lista con la que reviso lo demás.
Cuando pasé esa lista por el boilerplate con el que arranco proyectos salieron treinta y nueve puntos entre seguridad, SEO y arquitectura. Muchos eran variaciones de las mismas cuatro preguntas, que son las que me he quedado:
- ¿Qué hace esto con la lista vacía?
- ¿Quién comprueba el permiso dentro del bucle, y no solo antes?
- ¿Cuál es el techo de este límite si no viene de mi UI?
- ¿Cuánto vive esto (la clave, el enlace, el log) después de que ya no debería?
No son todas las preguntas posibles, ni de lejos. Seguro que la próxima vez que me siente sale una quinta que ahora mismo no se me ocurre, y probablemente será por algo que también escribí yo.