-
Notifications
You must be signed in to change notification settings - Fork 125
feat: add security middleware and rate limiting #323
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 |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| PORT=5000 | ||
| MONGO_URI=mongodb://localhost:27017/githubTracker | ||
| SESSION_SECRET=your-secret-key | ||
| # CLIENT_URL=http://localhost:5173 | ||
| CLIENT_URL=https://your-frontend-domain.com |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,35 +5,143 @@ const passport = require('passport'); | |
| const bodyParser = require('body-parser'); | ||
| require('dotenv').config(); | ||
| const cors = require('cors'); | ||
| const helmet = require('helmet'); | ||
| const rateLimit = require('express-rate-limit'); | ||
|
|
||
| // Passport configuration | ||
| require('./config/passportConfig'); | ||
|
|
||
| const app = express(); | ||
|
|
||
| // CORS configuration | ||
| app.use(cors('*')); | ||
| const isProduction = process.env.NODE_ENV === 'production'; | ||
|
|
||
| // Trust proxy for production deployments | ||
| if (isProduction) { | ||
| app.set('trust proxy', 1); | ||
| } | ||
|
|
||
| /* ========================= | ||
| Security Middleware | ||
| ========================= */ | ||
|
|
||
| // Helmet security headers | ||
| app.use(helmet()); | ||
|
|
||
| // Rate Limiter | ||
| const limiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 mins | ||
| max: 100, | ||
| message: 'Too many requests from this IP, please try again later.', | ||
| standardHeaders: true, | ||
| legacyHeaders: false, | ||
| }); | ||
|
|
||
| app.use('/api', limiter); | ||
|
|
||
| /* ========================= | ||
| CORS Configuration | ||
| ========================= */ | ||
|
|
||
| const rawAllowedOrigins = | ||
| process.env.ALLOWED_ORIGINS || | ||
| process.env.CLIENT_URL || | ||
| 'http://localhost:5173'; | ||
|
|
||
| const allowedOrigins = rawAllowedOrigins | ||
| .split(',') | ||
| .map(origin => origin.trim()); | ||
|
|
||
| app.use(cors({ | ||
| origin: (origin, callback) => { | ||
| // Allow server-to-server requests | ||
| if (!origin) { | ||
| return callback(null, true); | ||
| } | ||
|
|
||
| if (allowedOrigins.includes(origin)) { | ||
| return callback(null, true); | ||
| } | ||
|
|
||
| return callback(new Error('Not allowed by CORS')); | ||
| }, | ||
| credentials: true, | ||
| })); | ||
|
|
||
| /* ========================= | ||
| Body Parser | ||
| ========================= */ | ||
|
|
||
| // Middleware | ||
| app.use(bodyParser.json()); | ||
|
|
||
| /* ========================= | ||
| Session Configuration | ||
| ========================= */ | ||
|
|
||
| const sessionSecret = process.env.SESSION_SECRET; | ||
|
|
||
| if (isProduction && !sessionSecret) { | ||
| throw new Error('SESSION_SECRET is required in production'); | ||
| } | ||
|
|
||
| if (!isProduction && !sessionSecret) { | ||
| console.warn('⚠ SESSION_SECRET is missing in .env'); | ||
| } | ||
|
|
||
| app.use(session({ | ||
| secret: process.env.SESSION_SECRET, | ||
| secret: sessionSecret || 'dev-secret', | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| resave: false, | ||
| saveUninitialized: false, | ||
| cookie: { | ||
| httpOnly: true, | ||
| secure: isProduction, | ||
| sameSite: 'lax', | ||
| maxAge: 1000 * 60 * 60 * 24, // 1 day | ||
| }, | ||
| })); | ||
|
Comment on lines
90
to
100
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: cat -n backend/server.js | sed -n '70,100p'Repository: GitMetricsLab/github_tracker Length of output: 976 🏁 Script executed: rg -i "store.*session|session.*store|MemoryStore" backend/Repository: GitMetricsLab/github_tracker Length of output: 152 🏁 Script executed: cat backend/package.json | head -50Repository: GitMetricsLab/github_tracker Length of output: 743 🏁 Script executed: rg "session\(" backend/ -A 5 -B 2Repository: GitMetricsLab/github_tracker Length of output: 646 🏁 Script executed: rg "require.*session|import.*session" backend/Repository: GitMetricsLab/github_tracker Length of output: 134 🌐 Web query:
💡 Result: The express-session middleware uses MemoryStore as its default session storage, which is explicitly documented as not designed for production environments [1][2]. The official documentation provides the following warning: "Warning: The default server-side session storage, MemoryStore, is purposely not designed for a production environment. It will leak memory under most conditions, does not scale past a single process, and is meant for debugging and developing" [1][2]. Key reasons for this warning include: - Memory Leaks: The default MemoryStore does not have a robust mechanism to prune expired sessions, leading to memory accumulation over time [3][4][5]. - Lack of Scalability: Because it stores sessions in the memory of a single Node.js process, it cannot be shared across multiple server instances or processes [1][2][6]. - Data Persistence: All session data is lost if the Node.js process restarts [3]. To address this in production, developers are advised to use a compatible, production-ready session store [1][7]. A common community-recommended solution for those who still require an in-memory store but need to avoid memory leaks is the 'memorystore' package, which implements automatic pruning of expired sessions [3][4][5]. For distributed or highly scalable applications, external stores such as Redis or databases (e.g., MongoDB) are standard practice [3][8][6]. Citations:
Avoid the default in-process session store. Because no Use a production-ready session store such as Redis, MongoDB (via connect-mongo), or memorystore with automatic expiration pruning. 🤖 Prompt for AI Agents
Author
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. Thanks for the review and suggestion. I agree that a production-ready session store such as Redis or connect-mongo would be more suitable for scalable production deployments. However, since this PR is focused on introducing the initial security middleware improvements and production validation checks, I’d prefer to keep session store migration as a separate enhancement to avoid significantly expanding the scope of this contribution.
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.
Would you like me to open a follow-up GitHub issue to track the session store migration (e.g., connect-mongo or Redis) so it doesn't get lost? I can link it back to this PR for context. 🐇 _/_ |
||
|
|
||
| /* ========================= | ||
| Passport Middleware | ||
| ========================= */ | ||
|
|
||
| app.use(passport.initialize()); | ||
| app.use(passport.session()); | ||
|
|
||
| // Routes | ||
| /* ========================= | ||
| Routes | ||
| ========================= */ | ||
|
|
||
| const authRoutes = require('./routes/auth'); | ||
|
|
||
| app.use('/api/auth', authRoutes); | ||
|
|
||
| // Connect to MongoDB | ||
| mongoose.connect(process.env.MONGO_URI, {}).then(() => { | ||
| console.log('Connected to MongoDB'); | ||
| app.listen(process.env.PORT, () => { | ||
| console.log(`Server running on port ${process.env.PORT}`); | ||
| }); | ||
| }).catch((err) => { | ||
| console.log('MongoDB connection error:', err); | ||
| }); | ||
| /* ========================= | ||
| MongoDB Connection | ||
| ========================= */ | ||
|
|
||
| const PORT = process.env.PORT || 5000; | ||
|
|
||
| const MONGO_URI = | ||
| process.env.MONGO_URI || | ||
| (!isProduction | ||
| ? 'mongodb://127.0.0.1:27017/githubTracker' | ||
| : undefined); | ||
|
|
||
| if (isProduction && !process.env.MONGO_URI) { | ||
| throw new Error('MONGO_URI is required in production'); | ||
| } | ||
|
|
||
| if (!isProduction && !process.env.MONGO_URI) { | ||
| console.warn('⚠ MONGO_URI missing in .env'); | ||
| } | ||
|
|
||
| mongoose.connect(MONGO_URI) | ||
| .then(() => { | ||
| console.log('✅ Connected to MongoDB'); | ||
|
|
||
| app.listen(PORT, () => { | ||
| console.log(`🚀 Server running on port ${PORT}`); | ||
| }); | ||
| }) | ||
| .catch((err) => { | ||
| console.error('❌ MongoDB connection error:', err); | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.