14 KiB
DocFast Code Audit — Session 43
Date: 2026-02-16
Auditor: Senior Backend Developer (automated)
Scope: All TypeScript files in /opt/docfast/src/
Summary
| Severity | Count |
|---|---|
| CRITICAL | 3 |
| HIGH | 8 |
| MEDIUM | 10 |
| LOW | 7 |
CRITICAL
1. XSS in Billing Success Page via API Key Display
File: src/routes/billing.ts ~line 70
Issue: The escapeHtml() function is applied to keyInfo.key in the HTML template, but the same key is inserted unescaped inside a JS string in the onclick handler:
onclick="navigator.clipboard.writeText('${escapeHtml(keyInfo.key)}')..."
If an API key ever contains a single quote (unlikely with hex encoding, but the pattern is dangerous), this is XSS. More importantly, escapeHtml does NOT escape single quotes for JS context — ' inside a JS string literal won't help. The key is also inserted raw into the HTML body ${escapeHtml(keyInfo.key)} which IS properly escaped, but the JS onclick is the vector.
Suggested fix: Use JSON.stringify() for any value injected into JS context, or move the key into a data- attribute and read it from JS.
2. SSRF Bypass via DNS Rebinding in URL Convert
File: src/routes/convert.ts ~line 105-115
Issue: The DNS lookup happens BEFORE page.goto(). An attacker can use DNS rebinding: first lookup resolves to a public IP (passes check), then by the time Puppeteer fetches, DNS resolves to 127.0.0.1 or internal IPs. This is a classic TOCTOU (time-of-check-time-of-use) SSRF vulnerability.
Suggested fix: Configure Puppeteer to use a custom DNS resolver that blocks private IPs, or use --host-resolver-rules Chrome flag. Alternatively, resolve the IP yourself and pass the IP directly to Puppeteer with a Host header.
3. JavaScript Disabled But URL Convert Uses networkidle0
File: src/services/browser.ts ~line 218
Issue: renderUrlPdf calls page.setJavaScriptEnabled(false) then uses waitUntil: "networkidle0". With JS disabled, many sites won't render properly (SPAs, dynamic content). But more critically, if the intent is security (preventing JS execution on rendered pages), the waitUntil setting suggests an expectation of dynamic loading that can't happen without JS. This is a logic contradiction that either breaks functionality or gives false security confidence.
Suggested fix: Either enable JS for URL rendering (with proper sandboxing/timeouts, which you already have) or change waitUntil to "domcontentloaded" and document that only static pages are supported.
HIGH
4. Duplicate 404 Handlers
File: src/index.ts ~lines 120-170 and 172-195
Issue: There are TWO app.use() 404 catch-all handlers registered sequentially. The second one is dead code — Express will never reach it because the first handler always sends a response.
Suggested fix: Remove the second 404 handler entirely.
5. Webhook Signature Verification Can Be Skipped
File: src/routes/billing.ts ~line 85-95
Issue: When STRIPE_WEBHOOK_SECRET is not set, the webhook accepts any payload without signature verification, with only a console.warn. An attacker could forge webhook events to provision Pro keys or revoke existing ones. The warning uses console.warn instead of the structured logger.
Suggested fix: In production, refuse to process webhooks if STRIPE_WEBHOOK_SECRET is not configured. Return 500 with "Webhook not configured". At minimum, check NODE_ENV.
6. No Input Validation on Template Render Data
File: src/routes/templates.ts ~line 20
Issue: req.body.data || req.body is passed directly to renderTemplate() with zero validation. Template fields have required: true definitions but they're never enforced. Missing required fields silently produce broken HTML.
Suggested fix: Add validation that checks required fields from the template definition before rendering. Return 400 with specific missing fields.
7. Content-Type Not Checked on Markdown/URL Routes
File: src/routes/convert.ts
Issue: The HTML route checks Content-Type (~line 30) but markdown (~line 68) and URL (~line 95) routes do NOT. This inconsistency means malformed requests may parse incorrectly.
Suggested fix: Apply the same Content-Type check to all three routes, or extract it into a shared middleware.
8. filename Parameter Not Sanitized — Header Injection
File: src/routes/convert.ts ~lines 56, 84, 122
Issue: body.filename is inserted directly into Content-Disposition header:
res.setHeader("Content-Disposition", `inline; filename="${filename}"`);
A filename containing " or newlines could inject headers or break the response. While Express has some protections, this is defense-in-depth.
Suggested fix: Sanitize filename: strip/replace ", \r, \n, and non-printable characters.
9. Verification Code Timing Attack
File: src/services/verification.ts ~line 120
Issue: pending.code !== code uses direct string comparison, which is vulnerable to timing attacks. An attacker could statistically determine correct digits.
Suggested fix: Use crypto.timingSafeEqual() with Buffer conversion for code comparison.
10. Usage Data Written to DB on Every Single Request
File: src/middleware/usage.ts ~line 58-65
Issue: Every PDF request triggers an immediate INSERT ... ON CONFLICT UPDATE to PostgreSQL. Under load this creates significant DB write pressure. The code even says "In-memory cache, periodically synced to PostgreSQL" but actually writes synchronously on every request.
Suggested fix: Batch writes — update in-memory immediately, flush to DB every 30-60 seconds or on shutdown.
11. Unprotected Admin Endpoints
File: src/index.ts ~lines 97-103
Issue: /v1/usage and /v1/concurrency are protected by authMiddleware but ANY valid API key (including free tier) can access them. These expose operational data that should be admin-only.
Suggested fix: Add an admin check (e.g., specific admin key or role-based access).
MEDIUM
12. In-Memory Caches Can Diverge from DB
Files: src/services/keys.ts, src/services/verification.ts, src/middleware/usage.ts
Issue: Multiple in-memory caches (keysCache, verificationsCache, usage Map) are loaded on startup and updated on writes, but if a DB write fails, the in-memory state diverges. There's no reconciliation mechanism. In keys.ts, createFreeKey pushes to cache before confirming DB write succeeds (actually it awaits, so this is mostly ok, but saveUsageEntry in usage.ts has fire-and-forget .catch()).
Suggested fix: For critical data (keys), ensure cache is only updated after confirmed DB write. Add periodic cache refresh (e.g., every 5 min).
13. verifyToken Is Synchronous But Uses Cache
File: src/services/verification.ts ~line 47
Issue: verifyToken() is sync (used in GET /verify route) and relies on an in-memory cache. The DB update for verified_at is fire-and-forget. If the server restarts between verification and DB write, the verification is lost.
Suggested fix: Make the /verify route async and use a proper DB query.
14. No Request Body Size Validation Per Endpoint
File: src/index.ts ~line 73
Issue: Global body limit is 2MB for JSON. For HTML conversion, 2MB of complex HTML could cause Puppeteer to consume excessive memory/CPU. There's no per-route limit.
Suggested fix: Add tighter limits for conversion routes (e.g., 500KB) and larger for templates if needed.
15. Browser Pool Queue Has No Per-Key Fairness
File: src/services/browser.ts
Issue: The waitingQueue is FIFO with no per-key isolation. A single user could fill the queue and starve others. The rate limiter in pdfRateLimit.ts helps but a burst of requests from one key can still monopolize the queue.
Suggested fix: Add per-key queue limits (e.g., max 3 queued requests per key).
16. console.warn Used Instead of Logger
Files: src/routes/billing.ts ~line 88, ~line 138
Issue: Two places use console.warn instead of the structured logger. These won't appear in structured log output.
Suggested fix: Replace with logger.warn(...).
17. No CSRF Protection on Billing Success
File: src/routes/billing.ts ~line 44
Issue: The /billing/success endpoint provisions a Pro API key based only on session_id query parameter. If someone obtains a valid session_id (e.g., from browser history), they can re-provision or view the key. Stripe sessions are one-time-use for payment but retrievable.
Suggested fix: Track which session IDs have been provisioned and reject duplicates.
18. Rate Limit Store Memory Leak
File: src/middleware/pdfRateLimit.ts
Issue: rateLimitStore is a Map that grows with unique API keys. cleanupExpiredEntries() runs every 60s, but between cleanups, a burst of requests with many keys could grow the map. The cleanup interval (setInterval) is never cleared on shutdown.
Suggested fix: Low risk in practice since keys are finite, but add a max size check or use an LRU cache.
19. CORS Allows * for Conversion Routes
File: src/index.ts ~line 60
Issue: Access-Control-Allow-Origin: * for API routes is intentional (public API), but combined with Authorization header, browsers will still enforce preflight. This is fine architecturally but should be documented as intentional.
Suggested fix: Add a code comment confirming this is intentional for the public API use case.
20. trust proxy Set to 1 Without Validation
File: src/index.ts ~line 78
Issue: app.set("trust proxy", 1) trusts the first proxy hop. If the app is ever deployed without nginx in front, rate limiting by IP becomes bypassable via X-Forwarded-For spoofing.
Suggested fix: Document the deployment requirement. Consider using trust proxy: "loopback" for more specificity.
21. Verificaton Cache Loads ALL Verifications
File: src/services/verification.ts ~line 62
Issue: loadVerifications() loads every row from the verifications table into memory. Over time this grows unboundedly. The 15-min cleanup only removes unverified expired ones, but verified entries persist forever in memory.
Suggested fix: Only load recent/unverified entries. Verified entries can be queried from DB on demand.
LOW
22. Unused Import
File: src/routes/convert.ts ~line 3
Issue: getPoolStats is imported but never used in this file.
Suggested fix: Remove the unused import.
23. Unused Import in Test
File: src/__tests__/api.test.ts ~line 2
Issue: express is imported but never used.
Suggested fix: Remove.
24. rejectDuplicateEmail Uses Function Type
File: src/routes/signup.ts ~line 27
Issue: next: Function should be next: NextFunction from Express types.
Suggested fix: Import and use NextFunction.
25. Inconsistent Error Response Shapes
Files: Various routes
Issue: Error responses vary: some use { error: "..." }, others { error: "...", detail: "..." }, others { error: "...", limit: ..., used: ... }. No standard error envelope.
Suggested fix: Define a standard error response type: { error: string, code?: string, details?: object }.
26. isPrivateIP Missing IPv6 Unique Local (fc00::/7)
File: src/routes/convert.ts ~line 6-18
Issue: IPv6 unique local addresses (fc00::/7, i.e., fc00:: to fdff::) are not blocked. Only link-local (fe80::/10) is checked. An attacker could use a fd00:: address to bypass SSRF protection.
Suggested fix: Add check for fc and fd prefixes.
27. No Graceful Handling of DB Connection Failure at Startup
File: src/index.ts ~line start()
Issue: If PostgreSQL is unavailable at startup, initDatabase() throws and the process exits (which is correct), but there's no retry logic. In container orchestration this is fine (restart policy), but worth noting.
Suggested fix: Consider adding a retry with backoff for initial DB connection.
28. loadKeys Upserts Seed Keys on Every Restart
File: src/services/keys.ts ~line 28
Issue: Every restart re-upserts env API_KEYS into the DB with ON CONFLICT DO NOTHING. Harmless but unnecessary DB writes. Also uses new Date().toISOString() each time, so the created_at in cache won't match the DB if the key already existed.
Suggested fix: Check existence before upserting, or just ignore (low impact).
Test Coverage Gaps
The existing test file (src/__tests__/api.test.ts) covers:
- ✅ Auth (missing key, invalid key)
- ✅ Health endpoint
- ✅ HTML → PDF (success + missing field)
- ✅ Markdown → PDF
- ✅ Template listing + rendering + 404
NOT tested (critical paths missing):
- ❌ Signup flow (free key creation, email verification, duplicate email rejection)
- ❌ Recovery flow (code generation, verification, key retrieval)
- ❌ Email change flow
- ❌ Billing/Stripe (checkout, webhook events, subscription cancellation)
- ❌ URL → PDF (SSRF protection, DNS rebinding, private IP blocking)
- ❌ Usage limits (free tier limit, pro tier limit, monthly reset)
- ❌ Rate limiting (per-key rate limits, concurrency limits, queue full)
- ❌ Browser pool (restart after N PDFs, restart after timeout, page recycling)
- ❌ Graceful shutdown
- ❌ Content-Type enforcement
- ❌ Edge cases: malformed JSON, oversized bodies, concurrent requests
Top 5 Recommended Actions
- Fix DNS rebinding SSRF (Critical #2) — resolve DNS and pass IP to Puppeteer
- Require webhook secret in production (High #5) — prevent forged Stripe events
- Remove duplicate 404 handler (High #4) — dead code causing confusion
- Add input validation to templates (High #6) — enforce required fields
- Batch usage DB writes (High #10) — reduce write pressure under load