213 lines
14 KiB
Markdown
213 lines
14 KiB
Markdown
# 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
|