-
Notifications
You must be signed in to change notification settings - Fork 8
Harden backend security, add global rate limiting and deployment runbook #55
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
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,108 @@ | ||
| # Deployment guide | ||
|
|
||
| This project can be deployed with the following stack: | ||
|
|
||
| - **Backend**: Render, Railway, or AWS EC2 | ||
| - **Database (PostgreSQL)**: Supabase or Neon | ||
| - **Redis**: Upstash | ||
| - **File storage**: AWS S3 or Cloudinary | ||
|
|
||
| > Current repo runtime still uses a local JSON DB for persistence. `DATABASE_URL`, `REDIS_URL`, and storage variables are wired into env validation so you can safely provide production credentials while evolving integrations. | ||
|
|
||
| ## 1) Required environment variables | ||
|
|
||
| Use these common variables in all platforms: | ||
|
|
||
| ```bash | ||
| PORT=4000 | ||
| AUTH_TOKEN_SECRET=replace-with-long-secret | ||
| AUTH_TOKEN_TTL_SECONDS=43200 | ||
| CORS_ALLOW_ORIGIN=https://your-frontend-domain.com | ||
|
|
||
| # Security/rate-limit tuning | ||
| SECURITY_HEADERS_CSP=default-src 'self' | ||
| GLOBAL_RATE_LIMIT_MAX_REQUESTS=300 | ||
| GLOBAL_RATE_LIMIT_WINDOW_MS=900000 | ||
| LOGIN_RATE_LIMIT_MAX_ATTEMPTS=5 | ||
| LOGIN_RATE_LIMIT_WINDOW_MS=900000 | ||
| LOGIN_RATE_LIMIT_BLOCK_MS=900000 | ||
|
|
||
| # PostgreSQL (Supabase/Neon) | ||
| DATABASE_URL=postgresql://... | ||
|
|
||
| # Redis (Upstash) | ||
| REDIS_URL=redis://... | ||
| UPSTASH_REDIS_REST_URL=https://... | ||
| UPSTASH_REDIS_REST_TOKEN=... | ||
|
|
||
| # Storage (choose one) | ||
| STORAGE_DRIVER=s3 | ||
| AWS_REGION=ap-south-1 | ||
| AWS_S3_BUCKET=... | ||
| AWS_ACCESS_KEY_ID=... | ||
| AWS_SECRET_ACCESS_KEY=... | ||
|
|
||
| # OR | ||
| STORAGE_DRIVER=cloudinary | ||
| CLOUDINARY_CLOUD_NAME=... | ||
| CLOUDINARY_API_KEY=... | ||
| CLOUDINARY_API_SECRET=... | ||
| ``` | ||
|
|
||
| ## 2) Render | ||
|
|
||
| 1. Create a **Web Service** from this repo. | ||
| 2. Build command: `npm install && npm run build` | ||
| 3. Start command: `npm run backend` | ||
| 4. Add the env vars above in Render dashboard. | ||
| 5. Add PostgreSQL (Supabase/Neon external) and Upstash connection URLs. | ||
|
|
||
| ## 3) Railway | ||
|
|
||
| 1. Create a new Railway project linked to this repo. | ||
| 2. Set start command to `npm run backend`. | ||
| 3. Add all env vars in the Variables tab. | ||
| 4. Set custom domain and update `CORS_ALLOW_ORIGIN`. | ||
|
|
||
| ## 4) AWS EC2 | ||
|
|
||
| 1. Provision Ubuntu instance and install Node.js LTS. | ||
| 2. Clone repo and run `npm install`. | ||
| 3. Configure env vars in systemd service file. | ||
| 4. Run service with `npm run backend` via systemd. | ||
| 5. Put Nginx in front with HTTPS (Let's Encrypt). | ||
|
|
||
| Example service snippet: | ||
|
|
||
| ```ini | ||
| [Service] | ||
| WorkingDirectory=/srv/Brocode-Party-Update-App | ||
| ExecStart=/usr/bin/npm run backend | ||
| Environment=PORT=4000 | ||
| Environment=AUTH_TOKEN_SECRET=replace-me | ||
| Restart=always | ||
| ``` | ||
|
|
||
| ## 5) PostgreSQL provider choice | ||
|
|
||
| - **Supabase**: Copy pooled connection string from Project Settings → Database. | ||
| - **Neon**: Copy connection string from Neon project dashboard. | ||
| - Set as `DATABASE_URL`. | ||
|
|
||
| ## 6) Redis (Upstash) | ||
|
|
||
| - Create Redis database in Upstash. | ||
| - Use TCP URL as `REDIS_URL` (if your runtime supports it). | ||
| - For REST-based access, set both `UPSTASH_REDIS_REST_URL` and `UPSTASH_REDIS_REST_TOKEN`. | ||
|
|
||
| ## 7) File storage | ||
|
|
||
| - **AWS S3**: set `STORAGE_DRIVER=s3` + AWS credentials and bucket vars. | ||
| - **Cloudinary**: set `STORAGE_DRIVER=cloudinary` + Cloudinary keys. | ||
|
|
||
| ## 8) Post-deploy checks | ||
|
|
||
| - `GET /api/health` returns status 200. | ||
| - Login endpoint returns token and includes security headers. | ||
| - CORS allows only your front-end domain. | ||
| - Rate limiting returns 429 after repeated requests. |
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,19 +16,67 @@ const AUTH_TOKEN_SECRET = process.env.AUTH_TOKEN_SECRET || 'brocode-dev-secret-c | |||||||||||
| const AUTH_TOKEN_TTL_SECONDS = Number(process.env.AUTH_TOKEN_TTL_SECONDS || 60 * 60 * 12); | ||||||||||||
| const EVENT_STATE_DEFAULT_TTL_SECONDS = Number(process.env.EVENT_STATE_DEFAULT_TTL_SECONDS || 120); | ||||||||||||
| const CORS_ALLOW_ORIGIN = process.env.CORS_ALLOW_ORIGIN || '*'; | ||||||||||||
| const GLOBAL_RATE_LIMIT_MAX_REQUESTS = Number(process.env.GLOBAL_RATE_LIMIT_MAX_REQUESTS || 300); | ||||||||||||
| const GLOBAL_RATE_LIMIT_WINDOW_MS = Number(process.env.GLOBAL_RATE_LIMIT_WINDOW_MS || 15 * 60 * 1000); | ||||||||||||
| const SECURITY_HEADERS_CSP = process.env.SECURITY_HEADERS_CSP || "default-src 'self'"; | ||||||||||||
| const loginAttempts = new Map(); | ||||||||||||
| const globalRequests = new Map(); | ||||||||||||
| const SWAGGER_HTML = buildSwaggerHtml(); | ||||||||||||
| const jobSystem = await createJobSystem(); | ||||||||||||
|
|
||||||||||||
| const getLoginKey = (req, username) => { | ||||||||||||
| const getLoginKey = (req, username) => `${getRequestIp(req)}:${username}`; | ||||||||||||
|
|
||||||||||||
| const getRateLimitState = (key) => { | ||||||||||||
| const now = Date.now(); | ||||||||||||
| const existing = loginAttempts.get(key); | ||||||||||||
|
|
||||||||||||
| if (!existing) { | ||||||||||||
| const state = { count: 0, windowStart: now, blockedUntil: 0 }; | ||||||||||||
| loginAttempts.set(key, state); | ||||||||||||
| return state; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| if (existing.blockedUntil > 0 && existing.blockedUntil <= now) { | ||||||||||||
| existing.count = 0; | ||||||||||||
| existing.windowStart = now; | ||||||||||||
| existing.blockedUntil = 0; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| if (now - existing.windowStart > LOGIN_RATE_LIMIT_WINDOW_MS) { | ||||||||||||
| existing.count = 0; | ||||||||||||
| existing.windowStart = now; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| return existing; | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| const getGlobalRateLimitState = (key) => { | ||||||||||||
| const now = Date.now(); | ||||||||||||
| const existing = globalRequests.get(key); | ||||||||||||
|
|
||||||||||||
| if (!existing || now - existing.windowStart > GLOBAL_RATE_LIMIT_WINDOW_MS) { | ||||||||||||
| const state = { count: 0, windowStart: now }; | ||||||||||||
| globalRequests.set(key, state); | ||||||||||||
| return state; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| return existing; | ||||||||||||
| }; | ||||||||||||
|
Comment on lines
+54
to
+65
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. 🔴 The Root Cause and ImpactIn const getGlobalRateLimitState = (key) => {
const now = Date.now();
const existing = globalRequests.get(key);
if (!existing || now - existing.windowStart > GLOBAL_RATE_LIMIT_WINDOW_MS) {
const state = { count: 0, windowStart: now };
globalRequests.set(key, state); // entry persists forever
return state;
}
return existing;
};Similarly, In a production environment exposed to the internet, unique IPs from bots, scanners, and legitimate users will accumulate indefinitely, causing gradual memory growth that could eventually exhaust available memory. Impact: Slow memory leak in production that could lead to OOM crashes over extended uptime periods, especially under diverse traffic patterns. Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback. |
||||||||||||
|
|
||||||||||||
| const getRequestIp = (req) => { | ||||||||||||
| const forwardedFor = req.headers['x-forwarded-for']; | ||||||||||||
| const firstForwardedIp = Array.isArray(forwardedFor) | ||||||||||||
| ? forwardedFor[0] | ||||||||||||
| : typeof forwardedFor === 'string' | ||||||||||||
| ? forwardedFor.split(',')[0] | ||||||||||||
| : ''; | ||||||||||||
| const remoteIp = firstForwardedIp?.trim() || req.socket?.remoteAddress || 'unknown-ip'; | ||||||||||||
| return `${remoteIp}:${username}`; | ||||||||||||
|
|
||||||||||||
| return firstForwardedIp?.trim() || req.socket?.remoteAddress || 'unknown-ip'; | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| const clearRateLimitState = (key) => { | ||||||||||||
| loginAttempts.delete(key); | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| const parseBearerToken = (authHeader) => { | ||||||||||||
|
|
@@ -109,13 +157,42 @@ const getUserFromAuthHeader = async (authHeader) => { | |||||||||||
| return database.getUserById(verifiedPayload.sub); | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| const recordFailedLoginAttempt = (key) => { | ||||||||||||
| const now = Date.now(); | ||||||||||||
| const state = getRateLimitState(key); | ||||||||||||
| state.count += 1; | ||||||||||||
|
|
||||||||||||
| if (state.count >= LOGIN_RATE_LIMIT_MAX_ATTEMPTS) { | ||||||||||||
| state.blockedUntil = now + LOGIN_RATE_LIMIT_BLOCK_MS; | ||||||||||||
| } | ||||||||||||
| }; | ||||||||||||
|
Comment on lines
+160
to
+168
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. 🔴 New in-memory login rate-limit state is checked but never populated — brute-force protection is ineffective The PR adds Root Cause and ImpactThe new in-memory rate-limit check at const rateLimitState = getRateLimitState(loginKey);
if (rateLimitState.blockedUntil > now) { ... }But on failed login, the code records the failure in the old cache-based system instead: await rateLimiter.recordFailure(loginKey, { ... }); // line 315And on success, it clears the old system: await rateLimiter.clear(loginKey); // line 324The new Meanwhile, the old cache-based Impact: The new in-memory brute-force rate limiting is completely non-functional. Attackers can make unlimited login attempts without being blocked by the in-memory limiter. Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback. |
||||||||||||
|
|
||||||||||||
| const sendJson = (res, statusCode, body, extraHeaders = {}) => { | ||||||||||||
| const sendJson = (res, statusCode, body) => { | ||||||||||||
|
Comment on lines
+170
to
171
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. 🔴 Duplicate At lines 170-171, there are two consecutive Root Cause and ImpactThe merge produced: const sendJson = (res, statusCode, body, extraHeaders = {}) => {
const sendJson = (res, statusCode, body) => {The function body uses Note: This is also a syntax error that prevents the file from running, but the logical consequence is that even if one declaration were removed, the wrong one being kept would break the Impact: Rate-limit
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. |
||||||||||||
| res.writeHead(statusCode, { | ||||||||||||
| 'Content-Type': 'application/json', | ||||||||||||
| 'Access-Control-Allow-Origin': CORS_ALLOW_ORIGIN, | ||||||||||||
| 'Access-Control-Allow-Headers': 'Content-Type, Authorization', | ||||||||||||
| 'Access-Control-Allow-Methods': 'GET,POST,DELETE,OPTIONS', | ||||||||||||
| 'Cross-Origin-Opener-Policy': 'same-origin', | ||||||||||||
| 'Cross-Origin-Resource-Policy': 'same-origin', | ||||||||||||
| 'Origin-Agent-Cluster': '?1', | ||||||||||||
| 'Referrer-Policy': 'no-referrer', | ||||||||||||
| 'Strict-Transport-Security': 'max-age=31536000; includeSubDomains', | ||||||||||||
| 'X-Content-Type-Options': 'nosniff', | ||||||||||||
| 'X-DNS-Prefetch-Control': 'off', | ||||||||||||
| 'X-Download-Options': 'noopen', | ||||||||||||
| 'X-Frame-Options': 'SAMEORIGIN', | ||||||||||||
| 'X-Permitted-Cross-Domain-Policies': 'none', | ||||||||||||
| 'X-XSS-Protection': '0', | ||||||||||||
| 'Content-Security-Policy': SECURITY_HEADERS_CSP, | ||||||||||||
| ...extraHeaders, | ||||||||||||
| }); | ||||||||||||
| if (statusCode === 204) { | ||||||||||||
| res.end(); | ||||||||||||
| return; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| res.end(JSON.stringify(body)); | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
|
|
@@ -156,6 +233,23 @@ const server = createServer(async (req, res) => { | |||||||||||
| const parsedUrl = new URL(req.url || '/', `http://localhost:${port}`); | ||||||||||||
| const path = parsedUrl.pathname; | ||||||||||||
|
|
||||||||||||
| const globalRateLimitKey = getRequestIp(req); | ||||||||||||
| const globalRateLimitState = getGlobalRateLimitState(globalRateLimitKey); | ||||||||||||
| globalRateLimitState.count += 1; | ||||||||||||
|
|
||||||||||||
| if (globalRateLimitState.count > GLOBAL_RATE_LIMIT_MAX_REQUESTS) { | ||||||||||||
| const retryAfterSeconds = Math.ceil( | ||||||||||||
| (GLOBAL_RATE_LIMIT_WINDOW_MS - (Date.now() - globalRateLimitState.windowStart)) / 1000 | ||||||||||||
| ); | ||||||||||||
| sendJson( | ||||||||||||
| res, | ||||||||||||
| 429, | ||||||||||||
| { error: 'Too many requests. Please try again later.', retryAfterSeconds }, | ||||||||||||
| { 'Retry-After': String(Math.max(retryAfterSeconds, 1)) } | ||||||||||||
| ); | ||||||||||||
| return; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| if (method === 'OPTIONS') { | ||||||||||||
| sendJson(res, 204, {}); | ||||||||||||
| return; | ||||||||||||
|
|
@@ -193,6 +287,19 @@ const server = createServer(async (req, res) => { | |||||||||||
| } | ||||||||||||
|
|
||||||||||||
| const loginKey = getLoginKey(req, username); | ||||||||||||
| const rateLimitState = getRateLimitState(loginKey); | ||||||||||||
| const now = Date.now(); | ||||||||||||
| if (rateLimitState.blockedUntil > now) { | ||||||||||||
| const retryAfterSeconds = Math.ceil((rateLimitState.blockedUntil - now) / 1000); | ||||||||||||
| sendJson( | ||||||||||||
| res, | ||||||||||||
| 429, | ||||||||||||
| { | ||||||||||||
| error: 'Too many failed login attempts. Please try again later.', | ||||||||||||
| retryAfterSeconds, | ||||||||||||
| }, | ||||||||||||
| { 'Retry-After': String(Math.max(retryAfterSeconds, 1)) } | ||||||||||||
| ); | ||||||||||||
|
Comment on lines
+301
to
+302
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. 🔴 Missing When a login request is blocked by the new in-memory rate limiter, Root Cause and ImpactAt if (rateLimitState.blockedUntil > now) {
const retryAfterSeconds = Math.ceil((rateLimitState.blockedUntil - now) / 1000);
sendJson(res, 429, { ... }, { 'Retry-After': ... });
// ← missing return here!
}After sending the 429, the handler falls through to Impact: The login brute-force protection is completely bypassed — blocked users can still authenticate.
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. |
||||||||||||
| const retryAfterSeconds = await rateLimiter.getBlockedSeconds(loginKey); | ||||||||||||
| if (retryAfterSeconds > 0) { | ||||||||||||
| sendJson(res, 429, { | ||||||||||||
|
|
||||||||||||
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.
🟡 Duplicate
REDIS_URLkey in Zod schema — stricter.url()validation overrides the lenient oneThe env schema defines
REDIS_URLtwice: once at line 62 asz.string().optional()(new, lenient) and again at line 73 asz.string().url().optional()(old, strict). In a JavaScript object literal, the second property with the same key wins.Root Cause and Impact
In
backend/env.js, the Zod schema object has:The second definition overrides the first. The new code at line 62 intentionally relaxed the validation to
z.string().optional()(allowing non-URL strings like Redis connection strings that may not be valid URLs), but the old stricterz.string().url().optional()at line 73 takes precedence. This meansREDIS_URLvalues likeredis://user:pass@host:6379that aren't strictly valid URLs per Zod's.url()check could fail validation unexpectedly.Impact: Environment validation may reject valid Redis connection strings that don't pass strict URL validation.
Prompt for agents
Was this helpful? React with 👍 or 👎 to provide feedback.