# 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:
```js
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:
```js
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

1. **Fix DNS rebinding SSRF** (Critical #2) — resolve DNS and pass IP to Puppeteer
2. **Require webhook secret in production** (High #5) — prevent forged Stripe events
3. **Remove duplicate 404 handler** (High #4) — dead code causing confusion
4. **Add input validation to templates** (High #6) — enforce required fields
5. **Batch usage DB writes** (High #10) — reduce write pressure under load