Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 24 additions & 10 deletions server/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,14 @@ import feedbackRoutes from "./routes/feedback.js";
import pdfChatRoutes from "./routes/pdfChat.js";
import sitemapRoutes from "./routes/sitemap.js";
import { setupSocketHandlers } from "./socket/socketHandlers.js";
import initRedis from "./utils/redis.js";
import {
generalLimiter,
authLimiter,
uploadLimiter,
discussionLimiter,
chatLimiter,
strictAuthLimiter,
} from "./middleware/rateLimiter.js";

BigInt.prototype.toJSON = function () {
return this.toString();
Expand All @@ -27,14 +34,6 @@ BigInt.prototype.toJSON = function () {
// Load environment variables
dotenv.config();

// Initialize Redis (optional - will work without it)
initRedis().catch((err) => {
console.log(
"⚠️ Redis not available, continuing without cache:",
err.message
);
});

const app = express();
const server = createServer(app);

Expand Down Expand Up @@ -78,6 +77,21 @@ app.use(express.urlencoded({ extended: true, limit: "50mb" }));
// Initialize Passport
app.use(passport.initialize());

// Rate Limiting Middleware

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ High — Rate limiting is applied via app.use("/api/", generalLimiter) before the health route (app.get("/api/health", ...)) but the general limiter skip only checks req.path === "/api/health". Depending on Express mounting, req.path for requests may not include the /api prefix (it can be /health when the middleware is mounted on /api/). If the skip doesn’t match, /api/health may get rate-limited unintentionally.

Ensure skip logic matches the effective path for the mounted middleware. For example, use req.originalUrl (req.originalUrl === "/api/health") or configure the middleware on /api and skip using req.path === "/health".

bugs

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ High — Rate limiting is applied to the broad /api/ prefix via app.use("/api/", generalLimiter), which means every API request hits express-rate-limit logic (including keyGenerator/skip checks) before reaching route handlers. Under normal traffic this adds measurable CPU overhead to the hot path.

Apply the general limiter only to specific high-traffic endpoints or route groups (e.g., app.use('/api/auth', ...), app.use('/api/upload', ...), etc.). Alternatively, configure skip to quickly return for static/low-risk endpoints and ensure keyGenerator is minimal (avoid expensive string work).

performance

app.use("/api/", generalLimiter);
app.use("/api/auth/login", authLimiter);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ High — authLimiter is mounted specifically for /api/auth/login and /api/auth/send-otp, but the limiter’s skip rule is tied to req.path.includes("signup") && req.method === "POST". For these mounted routes, that skip condition likely never triggers, meaning behavior diverges from the code comment/intent ("Authentication Rate Limit").

Document the skip rationale (which endpoints should bypass) and verify that it matches the actual signup route path. If signup is not actually under a path containing "signup", update the skip logic or add documentation clarifying the intended route.

documentation

app.use("/api/auth/send-otp", authLimiter);
app.use("/api/auth/forgot-password", strictAuthLimiter);
app.use("/api/auth/reset-password", strictAuthLimiter);
app.use("/api/upload", uploadLimiter);
app.use("/api/discussions", (req, res, next) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ High — The custom wrapper uses discussionLimiter(req, res, next) but returns directly only inside the POST branch. If discussionLimiter calls next(err) or throws internally, this wrapper does not add any logging/consistent error handling, and the debugging middleware (console.log) is not correlated with limiter events.

Use the limiter as middleware directly without re-wrapping when possible, e.g. app.post("/api/discussions", discussionLimiter) (or mount on /api/discussions and set the limiter’s own skip based on method). If wrapping is needed, ensure you always return discussionLimiter(...) and add structured logging inside limiter handler/custom keyGenerator failures.

best-practices

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Medium — For discussions, rate limiting middleware is conditionally invoked only for POST via (req, res, next) => { if (["POST"].includes(req.method)) return discussionLimiter(req, res, next); next(); }. If the actual discussion creation endpoint uses another method (e.g., PUT/PATCH) or if the route uses router.post but subpaths don’t map exactly to /api/discussions, limits may not apply as intended. This is especially risky because the other limiters are mounted directly without such a method guard.

Confirm actual HTTP method(s) used for “posting” in server/routes/discussions.js. Prefer mounting discussionLimiter directly on the specific route(s) (or expand the method set) rather than relying on a generic /api/discussions mount with a hardcoded POST check.

bugs

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Medium — The discussions limiter is wired as app.use("/api/discussions", (req,res,next)=>{ if (["POST"].includes(req.method)) return discussionLimiter(req,res,next); next(); }). This allocates ['POST'] and runs includes for every request to /api/discussions, adding avoidable per-request overhead and extra middleware invocation depth.

Replace with a method check without array allocation: if (req.method === 'POST') return discussionLimiter(req,res,next); return next(); or mount directly with app.post('/api/discussions/...', discussionLimiter, ...) if routes are known.

performance

if (["POST"].includes(req.method)) {
return discussionLimiter(req, res, next);
}
next();
});
app.use("/api/pdf-chat", chatLimiter);

// Debug middleware to log all requests
app.use((req, res, next) => {
console.log(`🔍 ${req.method} ${req.path}`);
Expand Down Expand Up @@ -107,7 +121,7 @@ app.use("/", sitemapRoutes);
// Setup Socket.IO handlers
setupSocketHandlers(io);

// Health check endpoint
// Health check endpoint (not rate limited)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Medium — Health check endpoint /api/health is explicitly excluded from the general rate limit (skip: (req) => req.path === '/api/health' in the limiter). This endpoint performs DB connection and returns environment-variable presence indicators and database: Connected/Disconnected, which can support reconnaissance without rate limiting.

Do not skip /api/health, or apply a tighter rate limit specifically to it. Also avoid returning environment-variable presence for external callers; require authentication or restrict to internal networks.

security

app.get("/api/health", async (req, res) => {
try {
// Test database connection
Expand Down
159 changes: 159 additions & 0 deletions server/middleware/rateLimiter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
import rateLimit from "express-rate-limit";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ High — New exported limiter middleware/constants are defined (generalLimiter, authLimiter, uploadLimiter, discussionLimiter, chatLimiter, strictAuthLimiter, createCustomLimiter) but none of them have JSDoc describing purpose, behavior, inputs (req.user/auth assumptions), and outputs/response shape on 429.

Add JSDoc blocks for each exported limiter (or a shared typedef) including: what route patterns they’re intended for, required authentication state (e.g., relies on req.user.id), keying strategy, and the exact 429 JSON response fields (error/retryAfter). For createCustomLimiter, document options, defaults, and return type (Express middleware/rateLimit handler).

documentation


// General API Rate Limit: 500 requests per 15 minutes per IP
export const generalLimiter = rateLimit({
windowMs: 15 * 60 * 1000,
max: 500,
message: {
error: "Too many requests from this IP, please try again later.",
retryAfter: "15 minutes",
},
standardHeaders: true,
legacyHeaders: false,
skip: (req) => req.path === "/api/health",
keyGenerator: (req) =>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ HighkeyGenerator uses x-forwarded-for first: req.headers["x-forwarded-for"]?.split(",")[0]. If your app is not strictly behind a trusted proxy (or if app.set('trust proxy', ...) is not configured), clients can spoof x-forwarded-for, causing attackers to bypass per-IP rate limits by rotating spoofed values.

Harden IP extraction by relying on Express’s req.ip with proper app.set('trust proxy', <value>), or explicitly parse XFF only when trust proxy is configured (or when req.connection indicates a trusted proxy).

bugs

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ HighkeyGenerator uses x-forwarded-for without validating that the app is actually behind a trusted proxy. This can be spoofed by clients, allowing attackers to bypass per-IP rate limits or poison rate-limit buckets.

Use Express trust proxy and validate/normalize x-forwarded-for based on req.ip (preferred) or only accept x-forwarded-for when req.app.get('trust proxy') is configured appropriately. In most cases, prefer req.ip with trust proxy enabled.

best-practices

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Medium — Rate limiter keying uses untrusted client-controlled header x-forwarded-for to determine the identifier (via req.headers['x-forwarded-for']?.split(',')[0]). If your Express app/proxy trust is not tightly configured, an attacker can spoof this header to evade rate limits (OWASP: security misconfiguration / broken access control via flawed controls).

Use req.ip (Express) with app.set('trust proxy', true) only when behind a trusted reverse proxy, or configure express-rate-limit keyGenerator to use req.ip and ignore x-forwarded-for. Ensure reverse proxy sets/overwrites X-Forwarded-For and that Express trusts only that proxy.

security

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 MediumkeyGenerator repeatedly parses x-forwarded-for using split(",")[0] for every request. split allocates arrays and strings and can become a CPU hotspot at scale (rate limiter runs before handlers).

Avoid split/array allocation. Use a small parser: take substring up to first comma (e.g., const xff=req.headers['x-forwarded-for']; const key = typeof xff==='string' ? xff.split(',',1)[0].trim() : ...), or use a library/proxy setting to trust a single client IP via Express trust proxy and then read req.ip (no manual parsing).

performance

req.headers["x-forwarded-for"]?.split(",")[0] ||

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ High — Client IP extraction via req.headers['x-forwarded-for']?.split(',')[0] is not sanitized/validated. x-forwarded-for can contain whitespace and multiple IPs, and may be spoofed unless your deployment ensures the header is trusted only from the load balancer.

Trim the selected value and consider using Express trust proxy + req.ip (or a centralized helper that implements safe parsing and trims). At minimum: ...split(',')[0]?.trim().

readability

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Medium — IP parsing picks split(",")[0] without trimming whitespace. With headers like "1.2.3.4, 5.6.7.8", the first element may contain leading spaces, producing keys like "ip= 5.6.7.8" (or similarly malformed keys).

Trim the parsed segment: req.headers["x-forwarded-for"]?.split(",")[0]?.trim().

bugs

req.socket.remoteAddress ||
"unknown",
});

// Authentication Rate Limit: 5 login attempts per 15 minutes per IP
export const authLimiter = rateLimit({
windowMs: 15 * 60 * 1000,
max: 5,
message: {
error: "Too many login attempts, please try again after 15 minutes.",
retryAfter: "15 minutes",
},
standardHeaders: true,
legacyHeaders: false,
skip: (req) => req.path.includes("signup") && req.method === "POST",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 MediumauthLimiter applies only to req.path.includes('signup') && req.method === 'POST' in skip, meaning sign-up is rate-limited inconsistently while login/OTP/reset endpoints are elsewhere. This can allow abuse of auth-adjacent endpoints that are not covered or are skipped unexpectedly due to path matching quirks (e.g., routes with different casing/prefixes).

Remove the skip rule unless there is a strong reason. Instead, explicitly rate-limit exact endpoints in server/index.js (e.g., /api/auth/signup) with dedicated limiters, and avoid path.includes(...) string matching.

security

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 MediumauthLimiter.skip uses req.path.includes("signup") && req.method === "POST". includes runs on every auth request and can also unintentionally skip rate limiting if a different path contains signup as a substring.

Use exact route matching (e.g., req.baseUrl/req.route.path) or compare with normalized full path: req.path === '/signup' && req.method==='POST' depending on your route mount points.

performance

keyGenerator: (req) =>
req.headers["x-forwarded-for"]?.split(",")[0] ||
req.socket.remoteAddress ||
"unknown",
});

// File Upload Rate Limit: 10 uploads per hour per authenticated user

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ High — uploadLimiter behavior depends on req.user.id for keyGenerator, but req.user population/auth requirement is not documented. If req.user is missing, it falls back to IP-based limits, which may be unintended.

Document that uploadLimiter expects authentication middleware to have set req.user (and what shape is expected: req.user.id). If unauthenticated uploads are possible, document the intended fallback behavior (IP-based).

documentation

export const uploadLimiter = rateLimit({
windowMs: 60 * 60 * 1000,
max: 10,
message: {
error: "Upload limit exceeded. Maximum 10 files per hour.",
retryAfter: "1 hour",
},
standardHeaders: true,
legacyHeaders: false,
keyGenerator: (req) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ High — For unauthenticated uploads, uploadLimiter falls back to upload_${x-forwarded-for_or_ip}. This still relies on the untrusted x-forwarded-for header and creates a user-impacting security hole (spoofing IP keys) plus potential inconsistent behavior behind proxies.

Use req.ip (with correct proxy trust) for the unauthenticated key. For authenticated requests, ensure req.user.id is always reliable (and consider including auth method/tenant if applicable).

best-practices

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Medium — For uploadLimiter, when unauthenticated it keys by upload_${x-forwarded-for or remoteAddress}. If x-forwarded-for is user-controlled and not normalized (e.g., via untrusted proxies), cardinality can explode (many distinct keys), reducing effectiveness and increasing memory usage in the limiter store.

Set app.set('trust proxy', true) appropriately and use req.ip for stable client IP. Also consider always requiring auth for upload rate limiting (so keys are req.user.id) and for anonymous requests use a single trusted IP source.

performance

if (req.user && req.user.id) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 Critical — In uploadLimiter.keyGenerator, the authenticated-user branch returns upload_${req.user.id} assuming req.user.id is truthy. If req.user.id exists but is 0 (or other falsy valid values), the code falls through to the unauthenticated branch. More importantly, the unauthenticated fallback always returns a string, but express-rate-limit expects keyGenerator to always return a string; this is fine. The critical issue is elsewhere: in strictAuthLimiter, email.toLowerCase() assumes email is a string; if email is provided as a non-string (e.g., array from query params like ?email=a&email=b), toLowerCase will throw.

Defensively coerce/validate email: if (typeof email === 'string' && email) return ...; and handle arrays (Array.isArray(email) ? email[0] : email). Similarly validate req.user.id type.

bugs

return `upload_${req.user.id}`;
}
return `upload_${
req.headers["x-forwarded-for"]?.split(",")[0] ||
req.socket.remoteAddress ||
"unknown"
}`;
},
handler: (req, res) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ High — uploadLimiter includes a custom handler that returns 429 with { error, retryAfter }, but other limiters rely on express-rate-limit default message handling and may return a different response shape. This inconsistency is not documented.

Standardize response shape across all limiters or document the differences. Ensure documentation states the exact JSON body for 429 responses for each limiter.

documentation

res.status(429).json({
error: "Upload limit exceeded. Maximum 10 files per hour.",
retryAfter: "1 hour",
});
},
});

// Discussion/Comment Rate Limit: 20 posts per hour per authenticated user
export const discussionLimiter = rateLimit({
windowMs: 60 * 60 * 1000,
max: 20,
message: {
error: "Discussion posting limit exceeded. Maximum 20 posts per hour.",
retryAfter: "1 hour",
},
standardHeaders: true,
legacyHeaders: false,
keyGenerator: (req) => {
if (req.user && req.user.id) {
return `discussion_${req.user.id}`;
}
return `discussion_${
req.headers["x-forwarded-for"]?.split(",")[0] ||
req.socket.remoteAddress ||
"unknown"
}`;
},
handler: (req, res) => {
res.status(429).json({
error: "Discussion posting limit exceeded. Maximum 20 posts per hour.",
retryAfter: "1 hour",
});
},
});

// AI Chat Rate Limit: 30 requests per hour per authenticated user
export const chatLimiter = rateLimit({
windowMs: 60 * 60 * 1000,
max: 30,
message: {
error: "Chat limit exceeded. Maximum 30 messages per hour.",
retryAfter: "1 hour",
},
standardHeaders: true,
legacyHeaders: false,
keyGenerator: (req) => {
if (req.user && req.user.id) {
return `chat_${req.user.id}`;
}
return `chat_${
req.headers["x-forwarded-for"]?.split(",")[0] ||
req.socket.remoteAddress ||
"unknown"
}`;
},
handler: (req, res) => {
res.status(429).json({
error: "Chat limit exceeded. Maximum 30 messages per hour.",
retryAfter: "1 hour",
});
},
});

// Password Reset Rate Limit: 3 attempts per hour per email/IP
export const strictAuthLimiter = rateLimit({
windowMs: 60 * 60 * 1000,
max: 3,
message: {
error: "Too many password reset attempts. Please try again after 1 hour.",
retryAfter: "1 hour",
},
standardHeaders: true,
legacyHeaders: false,
keyGenerator: (req) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ High — Password reset limiter uses req.body?.email || req.query?.email to generate a key. This assumes body parsing has run and that the email is present/normalized. If a request hits this limiter without a parsed body (or with unexpected content types), email.toLowerCase() may throw or produce inconsistent keys.

Guard more defensively: ensure typeof email === 'string' before calling .toLowerCase(). Consider normalizing via a helper like normalizeEmail(email) and/or fall back to IP-only when email is missing/invalid.

readability

const email = req.body?.email || req.query?.email;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 CriticalstrictAuthLimiter.keyGenerator does const email = req.body?.email || req.query?.email; and then email.toLowerCase() at [L134]. If req.query.email is an array (Express can produce arrays for repeated query params), or if body/query parsing yields a non-string, toLowerCase() will throw, potentially causing 500s for requests instead of a clean 429 response.

Guard with type checks and normalize: `const emailVal = ...; const emailStr = Array.isArray(emailVal) ? emailVal[0] : emailVal; if (typeof emailStr === 'string') ... else fallback to IP key.

bugs

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ HighstrictAuthLimiter uses req.body?.email || req.query?.email and directly interpolates it into the limiter key (passwordreset_${email.toLowerCase()}). Unvalidated/massively long or crafted email values can produce extremely large keys and increase memory/storage pressure in the rate-limit backend (DoS risk), and can create key-collision/normalization issues (e.g., unusual Unicode, whitespace).

Validate and normalize email before using it in a key (e.g., strict regex for RFC5322-lite, trim whitespace, cap length). If invalid/missing, fall back to IP-based keying rather than using attacker-controlled raw input.

security

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ HighstrictAuthLimiter.keyGenerator reads req.body?.email and req.query?.email. For POST JSON requests, express-rate-limit may run before the body is parsed depending on middleware order. If body parsing isn’t ready, email becomes undefined and the limiter falls back to IP keys, reducing effectiveness (more brute-force attempts per email will share the same IP bucket). This undermines the intended protection and can increase load from attackers.

Ensure express.json() / express.urlencoded() runs before the limiter for these endpoints (it currently is in server/index.js), but make the limiter robust: prefer req.body only when available, and otherwise derive the key from a verified identifier (e.g., email from a parsed field earlier in the chain) or use route-specific middleware after body parsing.

performance

if (email) {
return `passwordreset_${email.toLowerCase()}`;
}
return `passwordreset_${
req.headers["x-forwarded-for"]?.split(",")[0] ||
req.socket.remoteAddress ||
"unknown"
}`;
},
handler: (req, res) => {
res.status(429).json({
error: "Too many password reset attempts. Please try again after 1 hour.",
retryAfter: "1 hour",
});
},
});

// Custom rate limiter factory for fine-grained control
export const createCustomLimiter = (options = {}) => {
const defaults = {
windowMs: 15 * 60 * 1000,
max: 500,
standardHeaders: true,
legacyHeaders: false,
};
return rateLimit({ ...defaults, ...options });
};
1 change: 1 addition & 0 deletions server/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
"cors": "^2.8.5",
"dotenv": "^17.2.1",
"express": "^4.18.2",
"express-rate-limit": "^7.4.0",
"groq-sdk": "^0.30.0",
"jsonwebtoken": "^9.0.2",
"mongodb": "^6.20.0",
Expand Down