From a0d4ba964c4a6eba7237efc79ec8053e2e9f8073 Mon Sep 17 00:00:00 2001 From: DocFast Agent Date: Tue, 17 Feb 2026 08:10:14 +0000 Subject: [PATCH] fix: audit #18 rate limit cleanup (.unref), audit #25 consistent error shapes Audit #18 - Rate limit store memory growth: - rateLimitStore already had cleanup via cleanupExpiredEntries() per-request + 60s interval - Added .unref() to the setInterval timer for clean graceful shutdown behaviour Audit #25 - Consistent error response shapes: - billing.ts: Fixed 409 plain-text response -> JSON { error: "..." } - index.ts: Simplified 404 from 4-field object to { error: "Not Found: METHOD path" } - signup.ts: Removed extra retryAfter field from rate-limit message object - pdfRateLimit.ts: Merged limit/tier/retryAfter into single error message string - usage.ts: Merged limit/used/upgrade fields into single error message string - convert.ts: Merged detail field into error message (3 occurrences) All error responses now consistently use {"error": "message"} shape. --- dist/index.js | 7 +-- dist/middleware/pdfRateLimit.js | 29 +++++++------ dist/middleware/usage.js | 76 ++++++++++++++++++++++++--------- dist/routes/billing.js | 2 +- dist/routes/convert.js | 6 +-- dist/routes/signup.js | 2 +- src/index.ts | 7 +-- src/middleware/pdfRateLimit.ts | 9 +--- src/middleware/usage.ts | 13 +----- src/routes/billing.ts | 2 +- src/routes/convert.ts | 6 +-- src/routes/signup.ts | 2 +- 12 files changed, 90 insertions(+), 71 deletions(-) diff --git a/dist/index.js b/dist/index.js index 69ceb0e..07587b8 100644 --- a/dist/index.js +++ b/dist/index.js @@ -222,12 +222,7 @@ app.use((req, res) => { const isApiRequest = req.path.startsWith('/v1/') || req.path.startsWith('/api') || req.path.startsWith('/health'); if (isApiRequest) { // JSON 404 for API paths - res.status(404).json({ - error: "Not Found", - message: `The requested endpoint ${req.method} ${req.path} does not exist`, - statusCode: 404, - timestamp: new Date().toISOString() - }); + res.status(404).json({ error: `Not Found: ${req.method} ${req.path}` }); } else { // HTML 404 for browser paths diff --git a/dist/middleware/pdfRateLimit.js b/dist/middleware/pdfRateLimit.js index 2b84953..62bba72 100644 --- a/dist/middleware/pdfRateLimit.js +++ b/dist/middleware/pdfRateLimit.js @@ -1,4 +1,5 @@ import { isProKey } from "../services/keys.js"; +import logger from "../services/logger.js"; // Per-key rate limits (requests per minute) const FREE_RATE_LIMIT = 10; const PRO_RATE_LIMIT = 30; @@ -6,6 +7,8 @@ const RATE_WINDOW_MS = 60_000; // 1 minute // Concurrency limits const MAX_CONCURRENT_PDFS = 3; const MAX_QUEUE_SIZE = 10; +// Per-key queue fairness (Audit #15) +const MAX_QUEUED_PER_KEY = 3; const rateLimitStore = new Map(); let activePdfCount = 0; const pdfQueue = []; @@ -26,7 +29,6 @@ function checkRateLimit(apiKey) { const limit = getRateLimit(apiKey); const entry = rateLimitStore.get(apiKey); if (!entry || now >= entry.resetTime) { - // Create new window rateLimitStore.set(apiKey, { count: 1, resetTime: now + RATE_WINDOW_MS @@ -39,7 +41,10 @@ function checkRateLimit(apiKey) { entry.count++; return true; } -async function acquireConcurrencySlot() { +function getQueuedCountForKey(apiKey) { + return pdfQueue.filter(w => w.apiKey === apiKey).length; +} +async function acquireConcurrencySlot(apiKey) { if (activePdfCount < MAX_CONCURRENT_PDFS) { activePdfCount++; return; @@ -47,8 +52,13 @@ async function acquireConcurrencySlot() { if (pdfQueue.length >= MAX_QUEUE_SIZE) { throw new Error("QUEUE_FULL"); } + // Audit #15: Per-key fairness — reject if this key already has too many queued + if (getQueuedCountForKey(apiKey) >= MAX_QUEUED_PER_KEY) { + logger.warn({ apiKey: apiKey.slice(0, 8) + "..." }, "Per-key queue limit reached"); + throw new Error("QUEUE_FULL"); + } return new Promise((resolve, reject) => { - pdfQueue.push({ resolve, reject }); + pdfQueue.push({ resolve, reject, apiKey }); }); } function releaseConcurrencySlot() { @@ -66,16 +76,11 @@ export function pdfRateLimitMiddleware(req, res, next) { if (!checkRateLimit(apiKey)) { const limit = getRateLimit(apiKey); const tier = isProKey(apiKey) ? "pro" : "free"; - res.status(429).json({ - error: "Rate limit exceeded", - limit: `${limit} PDFs per minute`, - tier, - retryAfter: "60 seconds" - }); + res.status(429).json({ error: `Rate limit exceeded: ${limit} PDFs/min allowed for ${tier} tier. Retry after 60s.` }); return; } - // Add concurrency control to the request - req.acquirePdfSlot = acquireConcurrencySlot; + // Add concurrency control to the request (pass apiKey for fairness) + req.acquirePdfSlot = () => acquireConcurrencySlot(apiKey); req.releasePdfSlot = releaseConcurrencySlot; next(); } @@ -88,4 +93,4 @@ export function getConcurrencyStats() { }; } // Proactive cleanup every 60s -setInterval(cleanupExpiredEntries, 60_000); +setInterval(cleanupExpiredEntries, 60_000).unref(); diff --git a/dist/middleware/usage.js b/dist/middleware/usage.js index 7350a40..c3251af 100644 --- a/dist/middleware/usage.js +++ b/dist/middleware/usage.js @@ -5,6 +5,12 @@ const FREE_TIER_LIMIT = 100; const PRO_TIER_LIMIT = 5000; // In-memory cache, periodically synced to PostgreSQL let usage = new Map(); +// Write-behind buffer for batching DB writes (Audit #10) +const dirtyKeys = new Set(); +const retryCount = new Map(); +const MAX_RETRIES = 3; +const FLUSH_INTERVAL_MS = 5000; +const FLUSH_THRESHOLD = 50; function getMonthKey() { const d = new Date(); return `${d.getFullYear()}-${String(d.getMonth() + 1).padStart(2, "0")}`; @@ -23,15 +29,54 @@ export async function loadUsageData() { usage = new Map(); } } -async function saveUsageEntry(key, record) { +// Batch flush dirty entries to DB (Audit #10 + #12) +async function flushDirtyEntries() { + if (dirtyKeys.size === 0) + return; + const keysToFlush = [...dirtyKeys]; + const client = await pool.connect(); try { - await pool.query(`INSERT INTO usage (key, count, month_key) VALUES ($1, $2, $3) - ON CONFLICT (key) DO UPDATE SET count = $2, month_key = $3`, [key, record.count, record.monthKey]); + await client.query("BEGIN"); + for (const key of keysToFlush) { + const record = usage.get(key); + if (!record) + continue; + try { + await client.query(`INSERT INTO usage (key, count, month_key) VALUES ($1, $2, $3) + ON CONFLICT (key) DO UPDATE SET count = $2, month_key = $3`, [key, record.count, record.monthKey]); + dirtyKeys.delete(key); + retryCount.delete(key); + } + catch (error) { + // Audit #12: retry logic for failed writes + const retries = (retryCount.get(key) || 0) + 1; + if (retries >= MAX_RETRIES) { + logger.error({ key: key.slice(0, 8) + "...", retries }, "CRITICAL: Usage write failed after max retries, data may diverge"); + dirtyKeys.delete(key); + retryCount.delete(key); + } + else { + retryCount.set(key, retries); + logger.warn({ key: key.slice(0, 8) + "...", retries }, "Usage write failed, will retry"); + } + } + } + await client.query("COMMIT"); } catch (error) { - logger.error({ err: error }, "Failed to save usage data"); + await client.query("ROLLBACK").catch(() => { }); + logger.error({ err: error }, "Failed to flush usage batch"); + // Keep all keys dirty for retry + } + finally { + client.release(); } } +// Periodic flush +setInterval(flushDirtyEntries, FLUSH_INTERVAL_MS); +// Flush on process exit +process.on("SIGTERM", () => { flushDirtyEntries().catch(() => { }); }); +process.on("SIGINT", () => { flushDirtyEntries().catch(() => { }); }); export function usageMiddleware(req, res, next) { const keyInfo = req.apiKeyInfo; const key = keyInfo?.key || "unknown"; @@ -39,11 +84,7 @@ export function usageMiddleware(req, res, next) { if (isProKey(key)) { const record = usage.get(key); if (record && record.monthKey === monthKey && record.count >= PRO_TIER_LIMIT) { - res.status(429).json({ - error: "Pro tier limit reached (5,000/month). Contact support for higher limits.", - limit: PRO_TIER_LIMIT, - used: record.count, - }); + res.status(429).json({ error: "Pro tier limit reached (5,000/month). Contact support for higher limits." }); return; } trackUsage(key, monthKey); @@ -52,12 +93,7 @@ export function usageMiddleware(req, res, next) { } const record = usage.get(key); if (record && record.monthKey === monthKey && record.count >= FREE_TIER_LIMIT) { - res.status(429).json({ - error: "Free tier limit reached", - limit: FREE_TIER_LIMIT, - used: record.count, - upgrade: "Upgrade to Pro for 5,000 PDFs/month: https://docfast.dev/pricing", - }); + res.status(429).json({ error: "Free tier limit reached (100/month). Upgrade to Pro at https://docfast.dev/#pricing for 5,000 PDFs/month." }); return; } trackUsage(key, monthKey); @@ -66,13 +102,15 @@ export function usageMiddleware(req, res, next) { function trackUsage(key, monthKey) { const record = usage.get(key); if (!record || record.monthKey !== monthKey) { - const newRecord = { count: 1, monthKey }; - usage.set(key, newRecord); - saveUsageEntry(key, newRecord).catch((err) => logger.error({ err }, "Failed to save usage entry")); + usage.set(key, { count: 1, monthKey }); } else { record.count++; - saveUsageEntry(key, record).catch((err) => logger.error({ err }, "Failed to save usage entry")); + } + dirtyKeys.add(key); + // Flush immediately if threshold reached + if (dirtyKeys.size >= FLUSH_THRESHOLD) { + flushDirtyEntries().catch((err) => logger.error({ err }, "Threshold flush failed")); } } export function getUsageStats(apiKey) { diff --git a/dist/routes/billing.js b/dist/routes/billing.js index ead3a18..e346f4f 100644 --- a/dist/routes/billing.js +++ b/dist/routes/billing.js @@ -45,7 +45,7 @@ router.get("/success", async (req, res) => { } // Prevent duplicate provisioning from same session if (provisionedSessions.has(sessionId)) { - res.status(409).send("This checkout session has already been used to provision a key. If you lost your key, use the key recovery feature."); + res.status(409).json({ error: "This checkout session has already been used to provision a key. If you lost your key, use the key recovery feature." }); return; } try { diff --git a/dist/routes/convert.js b/dist/routes/convert.js index e7d5c2d..d7b7721 100644 --- a/dist/routes/convert.js +++ b/dist/routes/convert.js @@ -82,7 +82,7 @@ convertRouter.post("/html", async (req, res) => { res.status(429).json({ error: "Server busy - too many concurrent PDF generations. Please try again in a few seconds." }); return; } - res.status(500).json({ error: "PDF generation failed", detail: err.message }); + res.status(500).json({ error: `PDF generation failed: ${err.message}` }); } finally { if (slotAcquired && req.releasePdfSlot) { @@ -128,7 +128,7 @@ convertRouter.post("/markdown", async (req, res) => { res.status(429).json({ error: "Server busy - too many concurrent PDF generations. Please try again in a few seconds." }); return; } - res.status(500).json({ error: "PDF generation failed", detail: err.message }); + res.status(500).json({ error: `PDF generation failed: ${err.message}` }); } finally { if (slotAcquired && req.releasePdfSlot) { @@ -202,7 +202,7 @@ convertRouter.post("/url", async (req, res) => { res.status(429).json({ error: "Server busy - too many concurrent PDF generations. Please try again in a few seconds." }); return; } - res.status(500).json({ error: "PDF generation failed", detail: err.message }); + res.status(500).json({ error: `PDF generation failed: ${err.message}` }); } finally { if (slotAcquired && req.releasePdfSlot) { diff --git a/dist/routes/signup.js b/dist/routes/signup.js index 56cc898..bfa34df 100644 --- a/dist/routes/signup.js +++ b/dist/routes/signup.js @@ -8,7 +8,7 @@ const router = Router(); const signupLimiter = rateLimit({ windowMs: 60 * 60 * 1000, max: 5, - message: { error: "Too many signup attempts. Please try again in 1 hour.", retryAfter: "1 hour" }, + message: { error: "Too many signup attempts. Please try again in 1 hour." }, standardHeaders: true, legacyHeaders: false, }); diff --git a/src/index.ts b/src/index.ts index 1906d6c..c453a47 100644 --- a/src/index.ts +++ b/src/index.ts @@ -247,12 +247,7 @@ app.use((req, res) => { if (isApiRequest) { // JSON 404 for API paths - res.status(404).json({ - error: "Not Found", - message: `The requested endpoint ${req.method} ${req.path} does not exist`, - statusCode: 404, - timestamp: new Date().toISOString() - }); + res.status(404).json({ error: `Not Found: ${req.method} ${req.path}` }); } else { // HTML 404 for browser paths res.status(404).send(` diff --git a/src/middleware/pdfRateLimit.ts b/src/middleware/pdfRateLimit.ts index 24fd6bb..e01d061 100644 --- a/src/middleware/pdfRateLimit.ts +++ b/src/middleware/pdfRateLimit.ts @@ -102,12 +102,7 @@ export function pdfRateLimitMiddleware(req: Request & { apiKeyInfo?: any }, res: if (!checkRateLimit(apiKey)) { const limit = getRateLimit(apiKey); const tier = isProKey(apiKey) ? "pro" : "free"; - res.status(429).json({ - error: "Rate limit exceeded", - limit: `${limit} PDFs per minute`, - tier, - retryAfter: "60 seconds" - }); + res.status(429).json({ error: `Rate limit exceeded: ${limit} PDFs/min allowed for ${tier} tier. Retry after 60s.` }); return; } @@ -128,4 +123,4 @@ export function getConcurrencyStats() { } // Proactive cleanup every 60s -setInterval(cleanupExpiredEntries, 60_000); +setInterval(cleanupExpiredEntries, 60_000).unref(); diff --git a/src/middleware/usage.ts b/src/middleware/usage.ts index 0644714..79a164f 100644 --- a/src/middleware/usage.ts +++ b/src/middleware/usage.ts @@ -92,11 +92,7 @@ export function usageMiddleware(req: any, res: any, next: any): void { if (isProKey(key)) { const record = usage.get(key); if (record && record.monthKey === monthKey && record.count >= PRO_TIER_LIMIT) { - res.status(429).json({ - error: "Pro tier limit reached (5,000/month). Contact support for higher limits.", - limit: PRO_TIER_LIMIT, - used: record.count, - }); + res.status(429).json({ error: "Pro tier limit reached (5,000/month). Contact support for higher limits." }); return; } trackUsage(key, monthKey); @@ -106,12 +102,7 @@ export function usageMiddleware(req: any, res: any, next: any): void { const record = usage.get(key); if (record && record.monthKey === monthKey && record.count >= FREE_TIER_LIMIT) { - res.status(429).json({ - error: "Free tier limit reached", - limit: FREE_TIER_LIMIT, - used: record.count, - upgrade: "Upgrade to Pro for 5,000 PDFs/month: https://docfast.dev/pricing", - }); + res.status(429).json({ error: "Free tier limit reached (100/month). Upgrade to Pro at https://docfast.dev/#pricing for 5,000 PDFs/month." }); return; } diff --git a/src/routes/billing.ts b/src/routes/billing.ts index 1af44f5..d721bdd 100644 --- a/src/routes/billing.ts +++ b/src/routes/billing.ts @@ -52,7 +52,7 @@ router.get("/success", async (req: Request, res: Response) => { // Prevent duplicate provisioning from same session if (provisionedSessions.has(sessionId)) { - res.status(409).send("This checkout session has already been used to provision a key. If you lost your key, use the key recovery feature."); + res.status(409).json({ error: "This checkout session has already been used to provision a key. If you lost your key, use the key recovery feature." }); return; } diff --git a/src/routes/convert.ts b/src/routes/convert.ts index 2f38e27..3f850a7 100644 --- a/src/routes/convert.ts +++ b/src/routes/convert.ts @@ -95,7 +95,7 @@ convertRouter.post("/html", async (req: Request & { acquirePdfSlot?: () => Promi res.status(429).json({ error: "Server busy - too many concurrent PDF generations. Please try again in a few seconds." }); return; } - res.status(500).json({ error: "PDF generation failed", detail: err.message }); + res.status(500).json({ error: `PDF generation failed: ${err.message}` }); } finally { if (slotAcquired && req.releasePdfSlot) { req.releasePdfSlot(); @@ -145,7 +145,7 @@ convertRouter.post("/markdown", async (req: Request & { acquirePdfSlot?: () => P res.status(429).json({ error: "Server busy - too many concurrent PDF generations. Please try again in a few seconds." }); return; } - res.status(500).json({ error: "PDF generation failed", detail: err.message }); + res.status(500).json({ error: `PDF generation failed: ${err.message}` }); } finally { if (slotAcquired && req.releasePdfSlot) { req.releasePdfSlot(); @@ -222,7 +222,7 @@ convertRouter.post("/url", async (req: Request & { acquirePdfSlot?: () => Promis res.status(429).json({ error: "Server busy - too many concurrent PDF generations. Please try again in a few seconds." }); return; } - res.status(500).json({ error: "PDF generation failed", detail: err.message }); + res.status(500).json({ error: `PDF generation failed: ${err.message}` }); } finally { if (slotAcquired && req.releasePdfSlot) { req.releasePdfSlot(); diff --git a/src/routes/signup.ts b/src/routes/signup.ts index cba1423..fd422eb 100644 --- a/src/routes/signup.ts +++ b/src/routes/signup.ts @@ -10,7 +10,7 @@ const router = Router(); const signupLimiter = rateLimit({ windowMs: 60 * 60 * 1000, max: 5, - message: { error: "Too many signup attempts. Please try again in 1 hour.", retryAfter: "1 hour" }, + message: { error: "Too many signup attempts. Please try again in 1 hour." }, standardHeaders: true, legacyHeaders: false, });