diff --git a/docs/superpowers/specs/2026-04-20-security-hardening-design.md b/docs/superpowers/specs/2026-04-20-security-hardening-design.md new file mode 100644 index 0000000..96124cb --- /dev/null +++ b/docs/superpowers/specs/2026-04-20-security-hardening-design.md @@ -0,0 +1,169 @@ +# http2pic Security Hardening & Bug Fixes + +**Date:** 2026-04-20 +**Status:** Approved + +## Summary + +Harden http2pic against abuse and fix two correctness bugs. All changes target `web/index.php`, `src/helpers.php`, `docker/start.sh`, and `docker-compose.yml`. No new dependencies. + +--- + +## 1. Config & Environment + +Two new optional env vars. Both default to off so existing deployments are unaffected. + +| Var | Default | Effect | +|-----|---------|--------| +| `API_KEY` | `""` | If non-empty, all `/api` requests must supply it | +| `BLOCK_PRIVATE_IPS` | `false` | If `true`, rejects URLs that resolve to private/loopback/metadata IPs | + +`docker/start.sh` writes both into `src/config.inc.php` alongside the existing `URL` define: + +```php +define('API_KEY', '${API_KEY:-}'); +define('BLOCK_PRIVATE_IPS', ${BLOCK_PRIVATE_IPS:-false}); +``` + +`docker-compose.yml` gets commented-out examples: + +```yaml +environment: + - URL=http://localhost:8080 + # - API_KEY=your-secret-key # if set, all /api requests must provide it + # - BLOCK_PRIVATE_IPS=true # block LAN/loopback/metadata IPs (safe default for public hosting) +``` + +--- + +## 2. API Key Authentication + +Checked at the top of `case 'api':`, before any Chrome work. + +**Logic:** if `API_KEY` is defined and non-empty, require the key. Accept it from either: +- HTTP header: `X-API-Key: ` +- Query param: `?key=` + +Header takes priority. Returns `401 Unauthorized` if missing or wrong. + +```php +if (defined('API_KEY') && API_KEY !== '') { + $provided = $_SERVER['HTTP_X_API_KEY'] ?? $_REQUEST['key'] ?? ''; + if ($provided !== API_KEY) { + header('HTTP/1.0 401 Unauthorized'); + echo 'Invalid or missing API key'; + exit; + } +} +``` + +**Usage examples (to add to README):** + +```bash +# via header (preferred — not logged in access logs) +curl -H "X-API-Key: secret" "http://host/api?url=https://example.com" + +# via query param (simpler for browser use) +curl "http://host/api?key=secret&url=https://example.com" +``` + +--- + +## 3. SSRF Protection (opt-in) + +Active only when `BLOCK_PRIVATE_IPS=true`. Runs after URL format validation, before spawning Chrome. + +**Flow:** +1. Extract hostname from target URL via `parse_url()` +2. Resolve to IP via `gethostbyname()` +3. If resolution fails (returns same string) or IP is private → `403 Forbidden` + +**Blocked ranges:** `127.0.0.0/8`, `10.0.0.0/8`, `172.16.0.0/12`, `192.168.0.0/16`, `169.254.0.0/16` (AWS/cloud metadata). + +Implementation uses `ip2long()` + bitmask checks — no regex. + +A new `isPrivateIP(string $ip): bool` helper lives in `src/helpers.php`. + +Returns `403 Forbidden` with body `URL not allowed` (no detail about why, to avoid enumeration). + +--- + +## 4. Bug Fixes + +### 4a. RemoteWebDriver request timeout (60ms → 60s) + +```php +// before +RemoteWebDriver::create($serverUrl, $capabilities, 30000, 60); +// after +RemoteWebDriver::create($serverUrl, $capabilities, 30000, 60000); +``` + +4th param is milliseconds. `60` = 60ms, effectively zero. Fix to `60000`. + +### 4b. Viewport set before page load + +Move `window()->setSize()` to before `driver->get()` so the page loads at the correct viewport from the start. This ensures responsive layouts and media queries fire at the intended dimensions. + +--- + +## 5. Security Hardening (always applied) + +### 5a. Viewport dimension cap + +After format validation, reject dimensions exceeding 3840×2160: + +```php +$parts = explode('x', $viewport); +if ((int)$parts[0] > 3840 || (int)$parts[1] > 2160) { + header('HTTP/1.0 400 Bad Request'); + echo 'Viewport exceeds maximum (3840x2160)'; + exit; +} +``` + +Prevents memory exhaustion via `99999x99999` requests. + +### 5b. Generic error responses + +Catch block logs full exception message but returns only: + +``` +Screenshot failed +``` + +Stops ChromeDriver internals (internal hostnames, paths, stack traces) leaking to clients. + +### 5c. Log injection fix + +`addToLog()` strips `\n`, `\r`, `\t` from data before writing: + +```php +$data = str_replace(["\n", "\r", "\t"], ' ', $data); +``` + +Prevents crafted URLs/IPs from injecting fake log entries. + +### 5d. getUserIP() fix + +Drop `HTTP_CLIENT_IP` (client-controlled, untrustworthy). Take only the first IP from `X-Forwarded-For` (Caddy appends subsequent hops; first entry is the real client): + +```php +function getUserIP() { + if (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])) + return trim(explode(',', $_SERVER['HTTP_X_FORWARDED_FOR'])[0]); + return $_SERVER['REMOTE_ADDR']; +} +``` + +--- + +## Files Changed + +| File | Changes | +|------|---------| +| `web/index.php` | API key check, SSRF check, viewport cap, viewport-before-get, timeout fix, generic errors | +| `src/helpers.php` | `isPrivateIP()` helper, log injection fix, `getUserIP()` fix | +| `docker/start.sh` | Write `API_KEY` and `BLOCK_PRIVATE_IPS` into config | +| `docker-compose.yml` | Commented env var examples | +| `src/config.inc.php` | (generated) gets two new defines |