-
Notifications
You must be signed in to change notification settings - Fork 164
fix(security): replace wildcard CORS with allowlist and harden session cookies #467
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| # URL of the backend API (no trailing slash). | ||
| # Must match the origin the backend server listens on. | ||
| VITE_BACKEND_URL=http://localhost:5000 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| # --------------------------------------------------------------- | ||
| # Server | ||
| # --------------------------------------------------------------- | ||
| PORT=5000 | ||
| NODE_ENV=development | ||
|
|
||
| # --------------------------------------------------------------- | ||
| # MongoDB | ||
| # --------------------------------------------------------------- | ||
| MONGO_URI=mongodb://127.0.0.1:27017/github_tracker | ||
|
|
||
| # --------------------------------------------------------------- | ||
| # Session | ||
| # Generate a long random string for production, e.g.: | ||
| # node -e "console.log(require('crypto').randomBytes(64).toString('hex'))" | ||
| # --------------------------------------------------------------- | ||
| SESSION_SECRET=replace-with-a-long-random-string | ||
|
|
||
| # --------------------------------------------------------------- | ||
| # CORS — Frontend origin allowlist | ||
| # | ||
| # Set this to the exact URL of your frontend (no trailing slash). | ||
| # REQUIRED in production: the server will refuse to start without it. | ||
| # In development, the server defaults to http://localhost:5173 when | ||
| # this variable is not set. | ||
| # | ||
| # Examples: | ||
| # Development : FRONTEND_ORIGIN=http://localhost:5173 | ||
| # Production : FRONTEND_ORIGIN=https://app.example.com | ||
| # --------------------------------------------------------------- | ||
| FRONTEND_ORIGIN=http://localhost:5173 | ||
|
|
||
| # --------------------------------------------------------------- | ||
| # Logging | ||
| # Accepted values: error | warn | info | debug | ||
| # --------------------------------------------------------------- | ||
| LOG_LEVEL=debug |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| /** | ||
| * Validates required environment variables before the server starts. | ||
| * Throws so callers can decide whether to log + exit or handle otherwise, | ||
| * which keeps the logic unit-testable without spawning child processes. | ||
| */ | ||
| function validateEnv() { | ||
| if (process.env.NODE_ENV === 'production' && !process.env.FRONTEND_ORIGIN) { | ||
| throw new Error( | ||
| 'FRONTEND_ORIGIN environment variable is required in production. ' + | ||
| 'Set it to the URL of your frontend (e.g., https://app.example.com).' | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| module.exports = { validateEnv }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,22 +6,62 @@ const bodyParser = require('body-parser'); | |
| require('dotenv').config(); | ||
| const cors = require('cors'); | ||
|
|
||
| const { validateEnv } = require('./config/validateEnv'); | ||
| const logger = require('./logger'); | ||
|
|
||
| // Fail fast in production when required env vars are absent. | ||
| try { | ||
| validateEnv(); | ||
| } catch (err) { | ||
| logger.error(`[FATAL] ${err.message}`); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| // Passport configuration | ||
| require('./config/passportConfig'); | ||
|
|
||
| const logger = require('./logger'); | ||
|
|
||
| const app = express(); | ||
|
|
||
| // CORS configuration | ||
| app.use(cors('*')); | ||
| // In development, fall back to localhost:5173 if FRONTEND_ORIGIN is not set so | ||
| // that contributors can run the stack without a full .env file. | ||
| const corsOrigin = process.env.FRONTEND_ORIGIN || 'http://localhost:5173'; | ||
|
|
||
| if (!process.env.FRONTEND_ORIGIN) { | ||
| logger.warn( | ||
| 'FRONTEND_ORIGIN is not set; defaulting to http://localhost:5173. ' + | ||
| 'Set this variable in production.' | ||
| ); | ||
| } | ||
|
|
||
| // CORS — explicit allowlist with credentials support. | ||
| // A function-based origin is required so that the header is only set (and | ||
| // reflected) for allowed origins; a static string would send the header on | ||
| // every response regardless of the requesting origin. | ||
| app.use(cors({ | ||
| origin: (requestOrigin, callback) => { | ||
| // Allow same-origin requests (no Origin header) and the configured origin. | ||
| if (!requestOrigin || requestOrigin === corsOrigin) { | ||
| return callback(null, true); | ||
| } | ||
| callback(null, false); | ||
| }, | ||
| credentials: true, | ||
| methods: ['GET', 'POST'], | ||
| allowedHeaders: ['Content-Type'], | ||
| })); | ||
|
|
||
| // Middleware | ||
| app.use(bodyParser.json()); | ||
| app.use(session({ | ||
| secret: process.env.SESSION_SECRET, | ||
| resave: false, | ||
| saveUninitialized: false, | ||
| secret: process.env.SESSION_SECRET, | ||
| resave: false, | ||
| saveUninitialized: false, | ||
| cookie: { | ||
| httpOnly: true, | ||
| // Only transmit the cookie over HTTPS in production. | ||
| secure: process.env.NODE_ENV === 'production', | ||
| sameSite: 'strict', | ||
|
Comment on lines
+59
to
+63
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify whether secure cookies are enabled without trust proxy in backend/server.js
rg -n -C2 "secure:\s*process\.env\.NODE_ENV === 'production'|app\.set\(\s*['\"]trust proxy['\"]" backend/server.jsRepository: GitMetricsLab/github_tracker Length of output: 245 Configure Express
🔧 Suggested patch const app = express();
+
+if (process.env.NODE_ENV === 'production') {
+ app.set('trust proxy', 1);
+}🤖 Prompt for AI Agents |
||
| }, | ||
| })); | ||
| app.use(passport.initialize()); | ||
| app.use(passport.session()); | ||
|
|
@@ -32,10 +72,10 @@ app.use('/api/auth', authRoutes); | |
|
|
||
| // Connect to MongoDB | ||
| mongoose.connect(process.env.MONGO_URI, {}).then(() => { | ||
| logger.info('Connected to MongoDB'); | ||
| app.listen(process.env.PORT, () => { | ||
| logger.info(`Server running on port ${process.env.PORT}`); | ||
| }); | ||
| logger.info('Connected to MongoDB'); | ||
| app.listen(process.env.PORT, () => { | ||
| logger.info(`Server running on port ${process.env.PORT}`); | ||
| }); | ||
| }).catch((err) => { | ||
| logger.error('MongoDB connection error', err); | ||
| logger.error('MongoDB connection error', err); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Require
SESSION_SECRETvalidation in production.validateEnv()currently checks onlyFRONTEND_ORIGIN, so production can still start with an empty or placeholder session secret, which weakens session integrity.🔧 Suggested patch
function validateEnv() { - if (process.env.NODE_ENV === 'production' && !process.env.FRONTEND_ORIGIN) { + const isProduction = process.env.NODE_ENV === 'production'; + const frontendOrigin = process.env.FRONTEND_ORIGIN?.trim(); + const sessionSecret = process.env.SESSION_SECRET?.trim(); + + if (isProduction && !frontendOrigin) { throw new Error( 'FRONTEND_ORIGIN environment variable is required in production. ' + 'Set it to the URL of your frontend (e.g., https://app.example.com).' ); } + + if (isProduction && (!sessionSecret || sessionSecret === 'replace-with-a-long-random-string')) { + throw new Error( + 'SESSION_SECRET must be set to a strong random value in production.' + ); + } }🤖 Prompt for AI Agents