fix: relax CORS and disable mandatory HTTPs in helmet #383
Conversation
Summary of ChangesHello @Doezer, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request aims to improve the self-hosting experience by relaxing network security configurations. It broadens the Cross-Origin Resource Sharing (CORS) policy to allow all origins by default, making it easier for different frontends to connect. Additionally, it disables HTTP Strict Transport Security (HSTS), which can prevent issues in environments where SSL/TLS is not readily available or desired, streamlining deployment for various setups. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to relax security settings for CORS and HSTS to simplify self-hosting. However, this introduces significant security risks by setting insecure defaults. Specifically, the default CORS policy is changed to allow all origins (*), which exposes the application to Cross-Site Request Forgery (CSRF) attacks. Additionally, HTTP Strict Transport Security (HSTS) is disabled for all environments, making the application vulnerable to man-in-the-middle attacks in production. There is also a configuration bug where the Express cors middleware is not correctly configured to handle the wildcard * origin. It's crucial to prioritize a secure-by-default configuration, potentially making these features conditional based on the environment.
| import cors from "cors"; | ||
|
|
||
| const app = express(); | ||
| app.use(cors({ origin: config.server.allowedOrigins })); |
There was a problem hiding this comment.
The cors middleware for Express is being configured with origin: config.server.allowedOrigins. If config.server.allowedOrigins is ['*'], this will not correctly configure CORS to allow all origins. The cors package expects the string '*' to allow any origin, not an array containing '*'. This means that if ALLOWED_ORIGINS is not set, CORS will be misconfigured and likely block all cross-origin requests.
You should apply the same logic here as you did in server/socket.ts to correctly handle the wildcard origin.
| app.use(cors({ origin: config.server.allowedOrigins })); | |
| app.use(cors({ origin: config.server.allowedOrigins.length === 1 && config.server.allowedOrigins[0] === "*" ? "*" : config.server.allowedOrigins })); |
| allowedOrigins: env.ALLOWED_ORIGINS | ||
| ? env.ALLOWED_ORIGINS.split(",").map((origin) => origin.trim()) | ||
| : ["http://localhost:port".replace("port", env.PORT.toString())], | ||
| : ["*"], // Default to allowing all origins for easier self-hosting logic |
There was a problem hiding this comment.
The default value for allowedOrigins has been changed to ["*"], allowing all origins. This is a critical security risk as a permissive CORS policy (*) allows any website to make requests to the application's API, potentially enabling attackers to perform Cross-Site Request Forgery (CSRF) attacks on behalf of a logged-in user or access sensitive data. While this might simplify self-hosting, it sets a dangerous default. It's recommended to revert this change to maintain a secure-by-default posture, requiring explicit configuration for production via the ALLOWED_ORIGINS environment variable.
| : ["*"], // Default to allowing all origins for easier self-hosting logic | |
| : ["http://localhost:port".replace("port", env.PORT.toString())], |
| "connect-src": connectSrc, | ||
| }, | ||
| }, | ||
| hsts: false, // Disable HSTS to allow HTTP connections for self-hosting without SSL |
There was a problem hiding this comment.
HTTP Strict Transport Security (HSTS) has been explicitly disabled (hsts: false). HSTS is a critical security policy that protects against man-in-the-middle attacks like protocol downgrade attacks and cookie hijacking by forcing browsers to use secure HTTPS connections. Disabling it makes users vulnerable to these attacks, especially if they connect over unencrypted HTTP. While disabling it might be necessary for local development or self-hosting without SSL, it significantly weakens the security posture for production. It should be enabled in production environments, ideally by making the setting conditional based on the environment.
| hsts: false, // Disable HSTS to allow HTTP connections for self-hosting without SSL | |
| hsts: appConfig.server.isProduction, // Conditionally enable HSTS in production |
References
- Security settings should be applied conditionally based on the environment (e.g., development vs. production) to ensure robust security in production while allowing flexibility for development. This aligns with the principle of conditionally applying insecure directives only in development environments.
Doezer
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request aims to simplify the self-hosting experience by relaxing security configurations. However, this introduces critical security risks for production deployments due to a permissive CORS policy and the disabling of HSTS and COOP headers, which weaken the application's defense against cross-origin and Man-in-the-Middle attacks. It is crucial to make these relaxed settings conditional, applying them only in development environments while maintaining robust security for production deployments.
| allowedOrigins: env.ALLOWED_ORIGINS | ||
| ? env.ALLOWED_ORIGINS.split(",").map((origin) => origin.trim()) | ||
| : ["http://localhost:port".replace("port", env.PORT.toString())], | ||
| : ["*"], // Default to allowing all origins for easier self-hosting logic |
There was a problem hiding this comment.
The default value for allowedOrigins has been changed to ["*"], creating a permissive CORS policy that allows any website to make requests to your API. This significantly increases the attack surface and can lead to cross-origin information leaks and CSRF attacks, especially since the default NODE_ENV is production. It is recommended to keep a more restrictive default (like localhost) and allow users to explicitly configure additional origins via environment variables, or make this setting conditional for development environments.
| : ["*"], // Default to allowing all origins for easier self-hosting logic | |
| : ["http://localhost:port".replace("port", env.PORT.toString())], |
| "upgrade-insecure-requests": null, // 🛡️ Disable automatic HTTPS upgrades for HTTP-only self-hosting | ||
| }, | ||
| }, | ||
| hsts: false, // Disable HSTS to allow HTTP connections for self-hosting without SSL | ||
| crossOriginOpenerPolicy: false, // Disable COOP for remote access |
There was a problem hiding this comment.
Disabling upgrade-insecure-requests, hsts, and crossOriginOpenerPolicy significantly weakens the security posture of the application. Specifically, disabling HSTS and automatic HTTPS upgrades makes the application vulnerable to Man-in-the-Middle (MitM) attacks and SSL stripping, while disabling COOP increases susceptibility to cross-origin side-channel attacks (like Spectre). These important security headers should be enabled in production and made conditional on the environment, rather than being disabled globally.
| "upgrade-insecure-requests": null, // 🛡️ Disable automatic HTTPS upgrades for HTTP-only self-hosting | |
| }, | |
| }, | |
| hsts: false, // Disable HSTS to allow HTTP connections for self-hosting without SSL | |
| crossOriginOpenerPolicy: false, // Disable COOP for remote access | |
| "upgrade-insecure-requests": appConfig.server.isProduction ? undefined : null, | |
| }, | |
| }, | |
| hsts: appConfig.server.isProduction, | |
| crossOriginOpenerPolicy: false, |
1 participant
This pull request makes several changes to improve the self-hosting experience of the server, mainly by relaxing security restrictions to allow easier local setup and testing. The most important changes are grouped below by theme:
CORS and Origin Handling:
allowedOriginsinconfig.tsis now set to"*"(all origins), making it easier for users to self-host without needing to configure specific origins.corsmiddleware, configured with theallowedOriginsvalue from the server config, ensuring consistent CORS handling across the server.allowedOriginsis"*", it explicitly allows all origins; otherwise, it uses the specified list. This ensures real-time connections are also easy to set up for self-hosting.Security Headers: