🚑 fix: Remove unnecessary CORS header assignment in app.js#105
🚑 fix: Remove unnecessary CORS header assignment in app.js#105caverav wants to merge 5 commits intodevelopmentfrom
Conversation
caverav
requested review from
Icksir,
iTzGooDLife,
jllanosg,
massi-ponce and
sealra
📝 Walkthrough📝 WalkthroughWalkthroughSe ha introducido una nueva variable de configuración Changes
Sequence Diagram(s)(No se generan diagramas de secuencia debido a la simplicidad de los cambios y la falta de nuevas características o modificaciones en el flujo de control.) Posiblemente PRs relacionados
Revisores sugeridos
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 (
|
|
Considero que está bien el cambio y funciona, pero creo que debería ser implementado más adelante (al final del desarrollo o sprint 3), ya que al quitar todos los origenes, no se puede puede hacer uso de la aplicación con npm run dev (pq el origen seria el puerto 5173), si no que se tendrá que buildear para ir probando los cambios, haciendo más lento el proceso de desarrollo. |
Ejemplo de error de cors al usar lo mencionado anteriormente: |
Apoyo esta observación. El poder mantener el uso del |
- Added dotenv package version 16.4.5 to manage environment variables. - Updated package.json, package-lock.json, and .env.example to reflect the changes. - Configured dotenv in app.js to load environment variables from .env file. - CORS_BYPASS environment variable set to 'false' by default, allowing CORS requests only from the same origin. - Instructions added in .env.example about setting CORS_BYPASS to true for development purposes only. - Updated dependencies related to dotenv across package.json and package-lock.json files.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range comments (1)
backend/src/app.js (1)
Line range hint
1-170: Resumen de problemas y pasos a seguir.Los cambios introducidos en este archivo presentan problemas significativos:
- No cumplen con el objetivo declarado del PR de eliminar CORS.
- Introducen un riesgo de seguridad potencial.
- No abordan las preocupaciones planteadas en los comentarios sobre el flujo de desarrollo.
Pasos recomendados:
- Reconsiderar la implementación de CORS basándose en los requisitos reales del proyecto.
- Coordinar con el equipo para determinar el momento adecuado para implementar los cambios de CORS.
- Implementar una solución que mantenga la seguridad y permita un flujo de desarrollo eficiente.
- Actualizar la documentación del proyecto para reflejar los cambios en la configuración de CORS y el uso de variables de entorno.
Por favor, revise y aborde estos problemas antes de proceder con la fusión de este PR.
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files ignored due to path filters (1)
backend/package-lock.jsonis excluded by!**/package-lock.json
Files selected for processing (3)
- backend/.env.example (1 hunks)
- backend/package.json (1 hunks)
- backend/src/app.js (2 hunks)
Additional comments not posted (3)
backend/.env.example (1)
1-1: ¡Bien hecho! La adición de esta variable de entorno es una buena práctica.La introducción de
CORS_BYPASSproporciona flexibilidad para el desarrollo mientras mantiene la seguridad por defecto. Es una solución elegante para el problema planteado en los comentarios del PR.Sin embargo, es crucial recordar que esto NUNCA debe usarse en producción. Sugiero agregar una advertencia más prominente en el comentario.
Ejecutemos un script para verificar que esta variable no se está utilizando incorrectamente en el código:
backend/package.json (1)
30-30: Verificar el uso de dotenv en la aplicaciónLa adición de la dependencia
dotenves una práctica común, pero necesitamos asegurarnos de que se está utilizando correctamente en la aplicación.Ejecute el siguiente script para verificar el uso de
dotenv:Verification successful
Corrección necesaria en el script de verificación de dotenv
El script anterior contiene un error de sintaxis que impide su ejecución. A continuación, se proporciona el script corregido para verificar el uso de
dotenven la aplicación:Por favor, ejecute el script corregido para continuar con la verificación.
Corrección necesaria en el script de verificación de dotenv
El script anterior aún contiene un error de sintaxis que impide su correcta ejecución. A continuación, se proporciona el script corregido para verificar el uso de
dotenven la aplicación:Por favor, ejecute el script corregido para continuar con la verificación.
Corrección necesaria en el script de verificación de dotenv
El script anterior aún contiene un error de sintaxis que impide su correcta ejecución. A continuación, se proporciona el script corregido para verificar el uso de
dotenven la aplicación:Por favor, ejecute el script corregido para continuar con la verificación.
Uso de
dotenvverificado correctamenteSe ha confirmado que la dependencia
dotenvestá siendo utilizada adecuadamente en la aplicación. Se ha encontrado la importación y configuración dedotenv, así como el uso deprocess.enven los archivos correspondientes.
Importación y configuración de dotenv:
backend/src/app.js:require('dotenv').config();Uso de variables de entorno:
backend/tests/index.test.js:var env = process.env.NODE_ENV || 'dev';backend/src/app.js:var env = process.env.NODE_ENV || 'dev';backend/src/app.js: if (process.env.CORS_BYPASS === 'true') {backend/src/lib/auth.js:var env = process.env.NODE_ENV || 'dev';No se encontraron archivos
.enven el repositorio, lo cual es habitual ya que estos archivos generalmente están incluidos en.gitignorepara evitar exponer variables sensibles.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Descripción: Verificar el uso de dotenv en el código fuente # Prueba: Buscar la importación y uso de dotenv rg --type js 'require\(["\']dotenv["\']|import.*from.*["\']dotenv["\']' # Prueba: Buscar el uso de process.env rg --type js 'process\.env\.' # Prueba: Buscar archivos .env fd -e envLength of output: 93
Script:
#!/bin/bash # Descripción: Verificar el uso de dotenv en el código fuente # Prueba: Buscar la importación y uso de dotenv rg --type js 'require\(["\']dotenv["\']|import.*from.*["\']dotenv["\']' # Prueba: Buscar el uso de process.env rg --type js 'process\.env\.' # Prueba: Buscar archivos .env fd -e envLength of output: 93
Script:
#!/bin/bash # Descripción: Verificar el uso de dotenv en el código fuente # Prueba: Buscar la importación y uso de dotenv rg --type js 'require\(["\']dotenv["\']\)|import.*from.*["\']dotenv["\']' # Prueba: Buscar el uso de process.env rg --type js 'process\.env\.' # Prueba: Buscar archivos .env fd -e envLength of output: 93
Script:
#!/bin/bash # Descripción: Verificar el uso de dotenv en el código fuente # Prueba: Buscar la importación y uso de dotenv rg --type js "require\(['\"]dotenv['\"]\)|import.*from.*['\"]dotenv['\"]" # Prueba: Buscar el uso de process.env rg --type js 'process\.env\.' # Prueba: Buscar archivos .env fd -e envLength of output: 431
backend/src/app.js (1)
3-3: Adición correcta de 'dotenv'.La inclusión de 'dotenv' es necesaria y coherente con la introducción de la variable de entorno
CORS_BYPASS.
…PASS in backend environment file to handle frontend separate usage.
|
TLDR: ahora para bypassear las restricciones de CORS, hay que settear |
|
Me parece bien el último cambio, pero me sigue apareciendo el error de CORS al usar el servicio desde el puerto 5173 y con la variable de entorno. Verifiqué y si se cumple la condición del if, pero aún así persiste el problema.
|
iTzGooDLife
left a comment
iTzGooDLife
left a comment
There was a problem hiding this comment.
Se explica la razón en el link.
Se debería ver con los demás si les ocurre el mismo problema @massi-ponce @Icksir @sealra @jllanosg
Es verdad. Me parece que la solución va más por el implementar una versión de desarrollo de los contenedores, como hacía pwndoc. |
Confirmo.
|
También me tira el error de CORS Missing Allow Origin en la autenticación |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
docs/installation.md (1)
Line range hint
1-24: La documentación ha mejorado, pero aún puede optimizarse.Las instrucciones de instalación son más completas ahora, especialmente con la adición de información sobre los contenedores. Sin embargo, hay oportunidades para mejorar aún más:
- Falta una tabla de contenidos para facilitar la navegación en el documento.
- No se proporciona información sobre las versiones de las dependencias requeridas.
- La sección de pruebas podría beneficiarse de más detalles sobre cómo interpretar los resultados.
Sugiero las siguientes mejoras:
- Añadir una tabla de contenidos al principio del documento.
- Incluir una sección de "Requisitos previos" que liste las versiones mínimas de Docker, Node.js, etc.
- Expandir la sección de pruebas con ejemplos de salida esperada y cómo interpretar diferentes resultados.
¿Le gustaría que generara un borrador para estas mejoras sugeridas?
Also applies to: 51-103
🧰 Tools
🪛 LanguageTool
[style] ~49-~49: The word ‘gonna’ is informal.
Context: ...443/api ## Development > Note: if you gonna be using the frontend separately (other...(GONNA)
[uncategorized] ~53-~53: Possible missing article found.
Context: ...> Source code can be modified live and application will automatically reload on changes. ...(AI_HYDRA_LEO_MISSING_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
backend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
- backend/package.json (1 hunks)
- backend/src/app.js (2 hunks)
- docs/installation.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/installation.md
[style] ~49-~49: The word ‘gonna’ is informal.
Context: ...443/api ## Development > Note: if you gonna be using the frontend separately (other...(GONNA)
🔇 Additional comments (2)
backend/package.json (1)
30-30: ¡Bien hecho al agregar la dependencia dotenv!La adición de la dependencia "dotenv" es una decisión acertada para manejar variables de entorno en la aplicación. Esta biblioteca es crucial para gestionar configuraciones sensibles, como la nueva variable CORS_BYPASS, de manera segura y flexible.
Sin embargo, es imperativo que actualices la documentación del proyecto para reflejar este cambio. Los desarrolladores deben saber cómo configurar correctamente las variables de entorno para el desarrollo local y la producción.
Ejecuta el siguiente script para verificar si se ha actualizado la documentación:
backend/src/app.js (1)
3-3: ¡Bien hecho al agregar la configuración de dotenv!La adición de dotenv es una buena práctica para manejar variables de entorno. Esto mejora la seguridad y la flexibilidad de la configuración.
| > Note: if you gonna be using the frontend separately (other origin) you need to change the `CORS_BYPASS` in `backend/.env` to `true` | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
La implementación de CORS_BYPASS ha sido verificada correctamente.
El script ejecutado confirma que CORS_BYPASS está implementado en backend/src/app.js, lo que valida la precisión de la nota añadida en docs/installation.md. Sin embargo, todavía es necesario:
- Reemplazar el lenguaje informal ("gonna") por uno más formal y adecuado para la documentación técnica.
- Incluir una explicación sobre las implicaciones de seguridad al establecer
CORS_BYPASSentrue.
🔗 Analysis chain
La nota sobre CORS_BYPASS es útil, pero necesita mejoras.
La adición de esta información es valiosa, pero hay aspectos que deben abordarse:
- El lenguaje utilizado es demasiado informal para una documentación técnica. "gonna" no es apropiado aquí.
- No se explican las implicaciones de seguridad de establecer CORS_BYPASS en true.
Sugiero la siguiente reformulación:
-> Note: if you gonna be using the frontend separately (other origin) you need to change the `CORS_BYPASS` in `backend/.env` to `true`
+> Nota: Si se va a utilizar el frontend de forma separada (desde un origen diferente), es necesario cambiar `CORS_BYPASS` en `backend/.env` a `true`. Tenga en cuenta que esto puede tener implicaciones de seguridad y solo debe utilizarse en entornos de desarrollo controlados.Para asegurar que esta configuración está correctamente implementada, ejecute el siguiente script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verificar la implementación de CORS_BYPASS en el código
echo "Buscando la implementación de CORS_BYPASS en el código:"
rg "CORS_BYPASS" --type js
Length of output: 210
🧰 Tools
🪛 LanguageTool
[style] ~49-~49: The word ‘gonna’ is informal.
Context: ...443/api ## Development > Note: if you gonna be using the frontend separately (other...(GONNA)
| if (process.env.CORS_BYPASS === 'true') { | ||
| res.header('Access-Control-Allow-Origin', '*'); | ||
| res.header('Access-Control-Allow-Credentials', 'true'); | ||
| } |
There was a problem hiding this comment.
Esta implementación de CORS es inaceptable y potencialmente peligrosa.
La nueva configuración de CORS presenta varios problemas graves:
- Contradice directamente el objetivo del PR de eliminar la asignación innecesaria de encabezados CORS.
- Introduce un riesgo de seguridad significativo al permitir todos los orígenes cuando
CORS_BYPASSes 'true'. - No se alinea con los comentarios que sugieren retrasar la eliminación de CORS.
Es imperativo que se realicen las siguientes mejoras:
- Si el proxy NGINX hace innecesario CORS, elimine completamente esta configuración.
- Si se requiere mantener CORS temporalmente para desarrollo:
- Implemente una lista blanca de orígenes permitidos en lugar de permitir todos indiscriminadamente.
- Cree una configuración separada para entornos de desarrollo y producción.
Ejemplo de una implementación más segura:
if (process.env.NODE_ENV === 'development') {
const allowedOrigins = ['http://localhost:5173', 'http://127.0.0.1:5173'];
const origin = allowedOrigins.includes(req.header('Origin'))
? req.header('Origin')
: allowedOrigins[0];
res.header('Access-Control-Allow-Origin', origin);
res.header('Access-Control-Allow-Credentials', 'true');
}Esta implementación restringe los orígenes permitidos y solo se aplica en el entorno de desarrollo, lo que es mucho más seguro que la implementación actual.
Es crucial que se aborden estos problemas de inmediato para evitar vulnerabilidades de seguridad potencialmente graves.
5 participants
Descripción
Se elimina permitir CORS de todos los origenes
Motivación y Contexto
Ya se implementó el proxy pass desde NGINX, por lo que no es necesario para el desarrollo permitir todos los orígenes
¿Cómo ha sido probado?
Probando el flujo normal, haciendo consultas a la API desde el frontend
Tipos de cambios
Lista de verificación:
Summary by CodeRabbit
Resumen por CodeRabbit
Nuevas Funciones
CORS_BYPASSen el archivo.env.example, permitiendo a los desarrolladores controlar el comportamiento de CORS durante el desarrollo.dotenvpara gestionar variables de entorno en la aplicación.Correcciones de Errores
CORS_BYPASS, permitiendo solicitudes de cualquier origen cuando está activada.