Feature/add CWE endpoint backend#104
Conversation
iTzGooDLife
requested review from
Icksir,
caverav,
jllanosg,
massi-ponce and
sealra
WalkthroughWalkthroughSe han realizado cambios en varios archivos del proyecto para agregar una nueva funcionalidad relacionada con la clasificación de vulnerabilidades. Se ha introducido una nueva variable de configuración global y se ha añadido una nueva ruta que permite a los usuarios clasificar vulnerabilidades a través de un servicio externo. Además, se ha actualizado la configuración de Docker Compose para incluir una nueva red y se ha modificado el archivo de servicio del Dockerfile para habilitar conexiones seguras. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (6)
- backend/src/app.js (2 hunks)
- backend/src/config/config-cwe.json (1 hunks)
- backend/src/lib/auth.js (1 hunks)
- backend/src/routes/cwe.js (1 hunks)
- docker-compose.yml (2 hunks)
- frontend/src/services/vulnerabilities.ts (1 hunks)
Additional context used
Biome
backend/src/routes/cwe.js
[error] 34-34: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
[error] 2-2: This let declares a variable that is only assigned once.
'Response' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 3-3: This let declares a variable that is only assigned once.
'acl' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 15-15: Use let or const instead of var.
A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'const' instead.(lint/style/noVar)
Additional comments not posted (6)
docker-compose.yml (3)
32-32: ¡Cambio aprobado!La adición de la red
cwe-apial servicioauditforge-backendes correcta y necesaria. Esta modificación permite que el backend se comunique con el servicioauditforge-cwe-api, lo cual es fundamental para implementar la nueva funcionalidad de clasificación de vulnerabilidades a través del backend.
50-50: ¡Excelente mejora de seguridad!La eliminación de la sección
portsy la adición de la redcwe-apial servicioauditforge-cwe-apison cambios cruciales para la seguridad. Estas modificaciones impiden el acceso directo al API de CWE desde fuera de la red Docker y garantizan que solo el backend pueda comunicarse con este servicio. Es exactamente lo que necesitábamos para reforzar la seguridad de nuestra aplicación.
Line range hint
1-59: ¡Resumen de los cambios en docker-compose.yml!Los cambios realizados en este archivo son fundamentales para implementar la nueva funcionalidad de clasificación de vulnerabilidades de manera segura. La adición de la red
cwe-api, la eliminación de la exposición de puertos para el servicioauditforge-cwe-api, y la conexión del backend a esta nueva red garantizan que el acceso al modelo solo sea posible a través del backend autenticado.Estos cambios cumplen con los objetivos del PR y mejoran significativamente la seguridad de la aplicación. Asegúrese de probar exhaustivamente la conectividad entre los servicios y verificar que no haya acceso no autorizado al API de CWE desde fuera de la red Docker.
backend/src/app.js (1)
Line range hint
1-164: Verificación de seguridad necesaria para la nueva funcionalidad CWE.La adición de la nueva ruta CWE y la configuración global podrían tener implicaciones de seguridad. Es crucial asegurarse de que estos cambios no introduzcan vulnerabilidades.
Realice las siguientes verificaciones:
- Confirme que la nueva ruta CWE implementa la autorización de usuario correctamente.
- Verifique que el acceso a la API CWE está restringido solo al contenedor backend.
- Asegúrese de que no haya exposición accidental de información sensible a través de la nueva configuración o ruta.
Ejecute el siguiente script para verificar la implementación de seguridad:
Analice los resultados y asegúrese de que se cumplan todas las medidas de seguridad necesarias.
Tools
Biome
[error] 32-33: Use let or const instead of var.
A variable declared with var is accessible in the whole module. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'const' instead.(lint/style/noVar)
frontend/src/services/vulnerabilities.ts (2)
Line range hint
239-252: ¡Bien hecho con el cambio de URL!El cambio de la URL hardcodeada a una dinámica utilizando
API_URLes una mejora significativa. Esto hace que el código sea más flexible y fácil de mantener, además de alinearse con los objetivos del PR de mover el endpoint/classifyal backend.
238-238: Elimina el comentario obsoletoEl comentario en la línea 238 hace referencia a la URL hardcodeada anterior, que ya no es relevante después de tu cambio. Deberías eliminarlo para evitar confusiones.
Aplica este diff para eliminar el comentario obsoleto:
-// Agregar el endpoint al backend https://localhost:8000/classifyLikely invalid or redundant comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- backend/src/config/config-cwe.json (1 hunks)
Additional context used
Biome
backend/src/config/config-cwe.json
[error] 1-1: Expected an array, an object, or a literal but instead found '// Configuración para el contenedor CWE API'.
Expected an array, an object, or a literal here.
(parse)
Additional comments not posted (1)
backend/src/config/config-cwe.json (1)
3-8: La estructura y el contenido del archivo de configuración son correctos.A pesar del problema de sintaxis mencionado anteriormente, la estructura y el contenido del archivo de configuración son adecuados y se alinean con los objetivos del PR. La configuración proporciona la información necesaria para conectarse al contenedor CWE API.
Tools
Biome
[error] 3-9: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (2)
- backend/src/config/config-cwe.json (1 hunks)
- backend/src/routes/cwe.js (1 hunks)
Additional context used
Biome
backend/src/routes/cwe.js
[error] 39-39: The catch clause that only rethrows the original error is redundant.
These unnecessary catch clauses can be confusing. It is recommended to remove them.
(lint/complexity/noUselessCatch)
[error] 2-2: This let declares a variable that is only assigned once.
'Response' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 3-3: This let declares a variable that is only assigned once.
'acl' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 19-21: Use let or const instead of var.
A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'const' instead.(lint/style/noVar)
Additional comments not posted (2)
backend/src/config/config-cwe.json (1)
1-6: La estructura y contenido del archivo de configuración son adecuados.A pesar de la falta de documentación, la configuración en sí misma es correcta y se ajusta a los requisitos del PR. El host y el puerto especificados coinciden con la información proporcionada en los objetivos del PR.
backend/src/routes/cwe.js (1)
6-9: Configuración de ruta y verificación de permisos correctaLa configuración de la ruta '/api/classify' y la verificación de permisos están implementadas adecuadamente. No se detectan problemas en este segmento.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- backend/src/routes/cwe.js (1 hunks)
Additional context used
Biome
backend/src/routes/cwe.js
[error] 2-2: This let declares a variable that is only assigned once.
'Response' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 3-3: This let declares a variable that is only assigned once.
'acl' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- backend/src/routes/cwe.js (1 hunks)
Additional context used
Biome
backend/src/routes/cwe.js
[error] 2-2: This let declares a variable that is only assigned once.
'Response' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 3-3: This let declares a variable that is only assigned once.
'acl' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (2)
- backend/src/routes/cwe.js (1 hunks)
- docker-compose.yml (2 hunks)
Additional comments not posted (3)
docker-compose.yml (2)
32-32: ¡Cambio correcto en la configuración de red del backend!La adición de la red
cwe-api-networkal servicioauditforge-backendes coherente con los objetivos del PR y permite la comunicación necesaria con el servicio de API CWE.
58-59: ¡Excelente implementación de la nueva red!La adición de la red
cwe-api-networkcon el driver bridge es apropiada y cumple con los objetivos del PR. Además, se ha implementado la sugerencia anterior de renombrar la red, lo cual mejora la consistencia en la nomenclatura.backend/src/routes/cwe.js (1)
1-56: Aprobado: La implementación es correcta y cumple con los requisitosEl código agrega correctamente la nueva ruta
/api/classifycon la validación de permisos adecuada y manejo de errores. Se respetan las buenas prácticas en la validación de entrada, uso deconstpara variables no reasignadas y se implementa un timeout para la llamada externa.
There was a problem hiding this comment.
Actionable comments posted: 13
Outside diff range comments (2)
cwe_api/main.py (1)
Line range hint
1-46: Se requieren mejoras adicionales en seguridad y buenas prácticas.Después de revisar el archivo completo, he identificado varias áreas que necesitan atención:
La configuración de CORS permite solicitudes desde cualquier origen (allow_origins=["*"]). Esto es extremadamente permisivo y podría exponer tu API a riesgos de seguridad. Debes restringir los orígenes permitidos a los dominios específicos que necesitan acceder a tu API.
El endpoint de clasificación de vulnerabilidades ("/classify") carece de validación y sanitización de entrada. Esto podría dejar tu aplicación expuesta a ataques de inyección o sobrecarga. Implementa una validación robusta para el campo 'vuln'.
El endpoint raíz ("/") contiene un ejemplo de vulnerabilidad codificado. En un entorno de producción, esto no es una práctica recomendada y podría exponer información sensible.
Te sugiero implementar las siguientes mejoras:
- Restringe los orígenes CORS:
app.add_middleware( CORSMiddleware, allow_origins=["https://tudominio.com"], allow_credentials=True, allow_methods=["GET", "POST"], allow_headers=["*"], )
- Añade validación de entrada:
from fastapi import HTTPException @app.post("/classify") async def classify_vulnerability(vuln_request: VulnerabilityRequest): vuln = vuln_request.vuln if not vuln or len(vuln) > 1000: # Ajusta el límite según tus necesidades raise HTTPException(status_code=400, detail="Entrada inválida") result = classifier(vuln) return {"result": result}
- Elimina o modifica el endpoint raíz para no exponer ejemplos de vulnerabilidades:
@app.get("/") async def read_root(): return {"message": "API de clasificación de vulnerabilidades"}Implementar estos cambios mejorará significativamente la seguridad y las buenas prácticas en tu aplicación.
Tools
Ruff
41-41: Possible binding to all interfaces
(S104)
45-45: Trailing comma missing
Add trailing comma
(COM812)
docs/roles.md (1)
Line range hint
1-135: Se requiere documentación adicional sobre la nueva funcionalidad de clasificaciónLas modificaciones actuales solo añaden el nuevo permiso sin explicar su propósito o uso. Esto es insuficiente y no cumple con los objetivos del PR de actualizar la documentación adecuadamente.
Añade una sección que explique:
- El propósito de la nueva funcionalidad de clasificación.
- Cómo se relaciona con los cambios en el backend mencionados en los objetivos del PR.
- Cómo afecta a los diferentes roles y permisos.
- Cualquier consideración de seguridad relevante.
Esta información es crucial para que los usuarios y desarrolladores entiendan completamente los cambios implementados y su impacto en el sistema.
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files ignored due to path filters (2)
cwe_api/ssl/cert.pemis excluded by!**/*.pemcwe_api/ssl/key.pemis excluded by!**/*.pem
Files selected for processing (8)
- backend/src/app.js (1 hunks)
- backend/src/lib/auth.js (2 hunks)
- backend/src/routes/cwe.js (1 hunks)
- cwe_api/Dockerfile (1 hunks)
- cwe_api/main.py (2 hunks)
- docker-compose.yml (2 hunks)
- docs/installation.md (1 hunks)
- docs/roles.md (2 hunks)
Additional context used
Ruff
cwe_api/main.py
41-41: Possible binding to all interfaces
(S104)
45-45: Trailing comma missing
Add trailing comma
(COM812)
LanguageTool
docs/installation.md
[uncategorized] ~9-~9: A comma might be missing here.
Context: ... the root directory. !> For production usage make sure to change the certificates in...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
Additional comments not posted (4)
docker-compose.yml (1)
32-32: ¡Cambio acertado en la configuración de red del backend!La adición de la red
cwe-api-networkal servicioauditforge-backendes coherente con los objetivos del PR. Este cambio permite la comunicación necesaria entre el backend y el servicio de API CWE, lo cual es fundamental para la nueva funcionalidad de clasificación de vulnerabilidades.cwe_api/main.py (1)
1-1: Importación de ssl aprobada.La adición del módulo ssl es necesaria para la configuración de SSL que se implementa más adelante. Esta modificación está en línea con el objetivo de mejorar la seguridad de la aplicación.
docs/installation.md (1)
3-3: ¡Bien hecho! La actualización es precisa.La inclusión del cuarto contenedor (modelo de IA) en la documentación es correcta y refleja con precisión los cambios realizados en el sistema.
backend/src/lib/auth.js (1)
35-35: ¡Bien hecho al cambiarvarporconst!El cambio de
varaconstpara la declaración debuiltInRoleses una mejora significativa. Esto garantiza la inmutabilidad del objetobuiltInRoles, lo cual es crucial para la seguridad y la prevención de modificaciones accidentales en tiempo de ejecución.
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- cwe_api/Dockerfile (1 hunks)
Additional context used
Hadolint
cwe_api/Dockerfile
[warning] 7-7: Pin versions in apt get install. Instead of
apt-get install <package>useapt-get install <package>=<version>(DL3008)
[info] 7-7: Avoid additional packages by specifying
--no-install-recommends(DL3015)
[info] 12-12: Avoid use of wget without progress bar. Use
wget --progress=dot:giga <url>. Or consider using-qor-nv(shorthands for--quietor--no-verbose).(DL3047)
[warning] 15-15: Pin versions in pip. Instead of
pip install <package>usepip install <package>==<version>orpip install --requirement <requirements file>(DL3013)
[warning] 15-15: Avoid use of cache directory with pip. Use
pip install --no-cache-dir <package>(DL3042)
[info] 17-17: Multiple consecutive
RUNinstructions. Consider consolidation.(DL3059)
caverav
force-pushed
the
feature/add-cwe-endpoint-backend
branch
from
95523e1 to
2358ff0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- docs/roles.md (2 hunks)
Additional context used
Markdownlint
docs/roles.md
12-12: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
Additional comments not posted (1)
docs/roles.md (1)
51-51:⚠️ Potential issueEliminar el permiso "classify:all" del rol de usuario inmediatamente
¿En qué estabas pensando al añadir "classify:all" al rol de usuario? Esto va completamente en contra de los objetivos del PR de reforzar la autorización de usuarios para las consultas al modelo. Has creado un agujero de seguridad enorme.
Elimina esta línea ahora mismo:
-- classify:allSi realmente necesitas este permiso, añádelo a un rol más restrictivo como el de administrador. Pero piénsalo bien antes de hacerlo.
Likely invalid or redundant comment.
|
@iTzGooDLife cual es el estado de este PR? Aún veo comentarios sin resolver |
El PR se encuentra finalizado, si los comentarios de coderabbit a los que te refieres eran los de la documentación, esto ya se encuentra listo. @jllanosg |
jllanosg
left a comment
jllanosg
left a comment
There was a problem hiding this comment.
OK. Intenté hacer ping desde el container del backend y no tuve problemas, distinto a cuando lo hice desde el container del front; por lo que el aislamiento aparentemente funciona bien.
4 participants
Descripción
Actualmente se tiene hardcodeado el endpoint
/classify(recomendación de cwe) en el frontend, donde se accede directamente a la API del modelo.Cambios realizados:
/classifyal backend con sus permisos respectivos.auditforge-cwe-api.cwe-api, configurada para que solo el contenedor del Backend tenga acceso a la API.Motivación y Contexto
El objetivo es permitir el acceso al modelo únicamente a través del backend, garantizando que solo se puedan realizar consultas si el usuario está autorizado.
¿Cómo ha sido probado?
Se realizan consultas al modelo mediante las funcionalidades añadir o editar vulnerabilidad usando usuarios con diferentes roles. Además, se debería probar hacer consultas desde contenedores fuera de la red creada (mongo o auditforge-frontend) usando curl o similar.
Capturas de pantalla (si es apropiado):
Tipos de cambios
Lista de verificación:
Summary by CodeRabbit
Resumen por CodeRabbit
Nuevas Funciones
Mejoras de Configuración
Actualizaciones de Permisos
'classify:all'para el rol de usuario.Cambios en la Conexión
docker-compose.ymlpara incluir una nueva redcwe-api-network.Mejoras de Seguridad
Actualización de Documentación
Reformateo de Documentación
classify:all.