From 73118498c91d89b9ea6bcb800e088cb555951a91 Mon Sep 17 00:00:00 2001 From: Chris Date: Mon, 20 Apr 2026 09:44:48 +0200 Subject: [PATCH] docs: add security hardening implementation plan --- .../plans/2026-04-20-security-hardening.md | 564 ++++++++++++++++++ 1 file changed, 564 insertions(+) create mode 100644 docs/superpowers/plans/2026-04-20-security-hardening.md diff --git a/docs/superpowers/plans/2026-04-20-security-hardening.md b/docs/superpowers/plans/2026-04-20-security-hardening.md new file mode 100644 index 0000000..d9a4c56 --- /dev/null +++ b/docs/superpowers/plans/2026-04-20-security-hardening.md @@ -0,0 +1,564 @@ +# Security Hardening & Bug Fixes Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Harden http2pic with optional API key auth, opt-in SSRF protection, viewport cap, and fix two correctness bugs (60ms timeout, viewport set after page load). + +**Architecture:** All PHP changes stay in `web/index.php` and `src/helpers.php`. Config vars flow from `docker-compose.yml` env → `docker/start.sh` → `src/config.inc.php` defines. No new dependencies. + +**Tech Stack:** PHP 8.4, php-webdriver, Caddy, Docker + +--- + +## File Map + +| File | What changes | +|------|-------------| +| `tests/test_helpers.php` | New — unit tests for `isPrivateIP()` and `getUserIP()` | +| `src/helpers.php` | Add `isPrivateIP()`, fix `getUserIP()`, fix `addToLog()` log injection | +| `docker/start.sh` | Write `API_KEY` and `BLOCK_PRIVATE_IPS` defines into config | +| `docker-compose.yml` | Add commented env var examples | +| `web/index.php` | API key check, SSRF check, viewport cap, viewport-before-get, timeout fix, generic errors | + +--- + +## Task 1: Write failing tests for helpers + +**Files:** +- Create: `tests/test_helpers.php` + +- [ ] **Step 1: Create test file** + +```php + 0 ? 1 : 0); +``` + +- [ ] **Step 2: Run tests — expect failures** + +```bash +php tests/test_helpers.php +``` + +Expected: several `FAIL:` lines (functions don't exist yet or have wrong logic). PHP fatal error on `isPrivateIP` is fine. + +--- + +## Task 2: Implement helpers changes + +**Files:** +- Modify: `src/helpers.php` + +- [ ] **Step 1: Replace `getUserIP()` and fix `addToLog()`, add `isPrivateIP()`** + +Replace the entire contents of `src/helpers.php` with: + +```php + 3840 || $vpParts[1] > 2160) { + header('HTTP/1.0 400 Bad Request'); + echo 'Viewport exceeds maximum (3840x2160)'; + exit; + } + + $js = $_REQUEST['js'] == 'false' ? false : true; + + $serverUrl = 'http://localhost:4444'; + $options = new \Facebook\WebDriver\Chrome\ChromeOptions(); + $options->addArguments(['--headless', '--disable-gpu', '--no-sandbox', '--disable-dev-shm-usage']); + + $capabilities = DesiredCapabilities::chrome(); + $capabilities->setCapability(\Facebook\WebDriver\Chrome\ChromeOptions::CAPABILITY, $options); + + if (!$js) + $capabilities->setCapability('javascriptEnabled', false); + + $driver = null; + $error = null; + try { + $driver = RemoteWebDriver::create($serverUrl, $capabilities, 30000, 60000); + $driver->manage()->window()->setSize(new \Facebook\WebDriver\WebDriverDimension($vpParts[0], $vpParts[1])); + $driver->get($target); + $driver->executeScript('document.body.style.overflow = "hidden";'); + + addToLog("$ip\tRequested $target with viewport $viewport and js " . ($js ? 'enabled' : 'disabled')); + + $screenshot = $driver->takeScreenshot(); + header('Content-Type: image/png'); + header('Content-Length: ' . strlen($screenshot)); + echo $screenshot; + } catch (Exception $e) { + $error = $e->getMessage(); + addToLog("$ip\tRequested $target but resulted in error:\t" . $error); + } finally { + if ($driver instanceof RemoteWebDriver) { + try { $driver->quit(); } catch (Exception $q) {} + } + } + if ($error !== null) { + header('HTTP/1.0 500 Internal Server Error'); + echo 'Screenshot failed'; + } + + break; +``` + +- [ ] **Step 2: Test viewport cap with curl** + +```bash +curl -s -o /dev/null -w "%{http_code}" "http://localhost:8080/api?url=https://example.com&viewport=99999x99999" +``` + +Expected: `400` + +```bash +curl -s -o /dev/null -w "%{http_code}" "http://localhost:8080/api?url=https://example.com&viewport=1024x768" +``` + +Expected: `200` + +- [ ] **Step 3: Commit** + +```bash +git add web/index.php +git commit -m "fix: viewport before page load, 60ms→60s timeout, viewport cap, generic errors" +``` + +--- + +## Task 5: Add API key authentication + +**Files:** +- Modify: `web/index.php` + +- [ ] **Step 1: Add API key check at top of `case 'api':` block** + +Insert immediately after `case 'api':` (before the `$target` line): + +```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; + } + } +``` + +- [ ] **Step 2: Test without key (no API_KEY configured)** + +```bash +curl -s -o /dev/null -w "%{http_code}" "http://localhost:8080/api?url=https://example.com" +``` + +Expected: `200` (API_KEY not set in local config → open access) + +- [ ] **Step 3: Test with key via header** + +Temporarily set `API_KEY` in `src/config.inc.php` to `'testkey'`, then: + +```bash +# missing key → 401 +curl -s -o /dev/null -w "%{http_code}" "http://localhost:8080/api?url=https://example.com" +# header → 200 +curl -s -o /dev/null -w "%{http_code}" -H "X-API-Key: testkey" "http://localhost:8080/api?url=https://example.com" +# query param → 200 +curl -s -o /dev/null -w "%{http_code}" "http://localhost:8080/api?key=testkey&url=https://example.com" +# wrong key → 401 +curl -s -o /dev/null -w "%{http_code}" -H "X-API-Key: wrongkey" "http://localhost:8080/api?url=https://example.com" +``` + +Expected: `401`, `200`, `200`, `401` + +Revert the manual edit to `src/config.inc.php` after testing (it gets regenerated by `start.sh` in Docker). + +- [ ] **Step 4: Document API key usage in CLAUDE.md** + +In `CLAUDE.md`, add a new section under the existing content: + +```markdown +## API Key + +Set `API_KEY` env var in docker-compose to require authentication on all `/api` requests. +Leave unset (default) for open access. + +```bash +# via header (preferred — not logged in access logs) +curl -H "X-API-Key: your-secret-key" "http://host/api?url=https://example.com" + +# via query param +curl "http://host/api?key=your-secret-key&url=https://example.com" +``` + +## SSRF Protection + +Set `BLOCK_PRIVATE_IPS=true` to reject requests to LAN, loopback, and cloud metadata IPs. +Recommended when hosting publicly. Default is off (allows local/LAN addresses). +``` + +- [ ] **Step 5: Commit** + +```bash +git add web/index.php CLAUDE.md +git commit -m "feat: optional API key auth via X-API-Key header or ?key= param" +``` + +--- + +## Task 6: Add SSRF protection + +**Files:** +- Modify: `web/index.php` + +- [ ] **Step 1: Add SSRF check after viewport validation** + +Insert after the `$js = ...` line (before `$serverUrl = ...`): + +```php + if (defined('BLOCK_PRIVATE_IPS') && BLOCK_PRIVATE_IPS) { + $host = parse_url($target, PHP_URL_HOST); + if (filter_var($host, FILTER_VALIDATE_IP)) { + $resolvedIp = $host; + } else { + $resolvedIp = gethostbyname($host); + if ($resolvedIp === $host) { + header('HTTP/1.0 403 Forbidden'); + echo 'URL not allowed'; + exit; + } + } + if (isPrivateIP($resolvedIp)) { + header('HTTP/1.0 403 Forbidden'); + echo 'URL not allowed'; + exit; + } + } +``` + +- [ ] **Step 2: Test SSRF protection** + +Temporarily set `BLOCK_PRIVATE_IPS` to `true` in `src/config.inc.php`, then: + +```bash +# private IP → 403 +curl -s -o /dev/null -w "%{http_code}" "http://localhost:8080/api?url=http://192.168.1.1/" +# loopback → 403 +curl -s -o /dev/null -w "%{http_code}" "http://localhost:8080/api?url=http://127.0.0.1/" +# metadata → 403 +curl -s -o /dev/null -w "%{http_code}" "http://localhost:8080/api?url=http://169.254.169.254/" +# public URL → 200 +curl -s -o /dev/null -w "%{http_code}" "http://localhost:8080/api?url=https://example.com" +``` + +Expected: `403`, `403`, `403`, `200` + +Revert the manual edit to `src/config.inc.php` after testing. + +- [ ] **Step 3: Commit** + +```bash +git add web/index.php +git commit -m "feat: opt-in SSRF protection via BLOCK_PRIVATE_IPS env var" +``` + +--- + +## Final state of `web/index.php` `case 'api':` block + +For reference, the complete block after all tasks: + +```php + case 'api': + 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; + } + } + + $target = substr($_SERVER['REQUEST_URI'], 5); + if (!$target || !filter_var($target, FILTER_VALIDATE_URL)) + $target = $_REQUEST['url']; + if (!filter_var($target, FILTER_VALIDATE_URL)) { + header('HTTP/1.0 400 Bad Request'); + echo 'Invalid URL'; + exit; + } + $ip = getUserIP(); + + $viewport = $_REQUEST['viewport'] ?: '1024x768'; + if (!preg_match('/^\d+x\d+$/', $viewport)) { + header('HTTP/1.0 400 Bad Request'); + echo 'Invalid viewport format. Use WIDTHxHEIGHT (e.g., 1024x768)'; + exit; + } + $vpParts = array_map('intval', explode('x', $viewport)); + if ($vpParts[0] > 3840 || $vpParts[1] > 2160) { + header('HTTP/1.0 400 Bad Request'); + echo 'Viewport exceeds maximum (3840x2160)'; + exit; + } + + $js = $_REQUEST['js'] == 'false' ? false : true; + + if (defined('BLOCK_PRIVATE_IPS') && BLOCK_PRIVATE_IPS) { + $host = parse_url($target, PHP_URL_HOST); + if (filter_var($host, FILTER_VALIDATE_IP)) { + $resolvedIp = $host; + } else { + $resolvedIp = gethostbyname($host); + if ($resolvedIp === $host) { + header('HTTP/1.0 403 Forbidden'); + echo 'URL not allowed'; + exit; + } + } + if (isPrivateIP($resolvedIp)) { + header('HTTP/1.0 403 Forbidden'); + echo 'URL not allowed'; + exit; + } + } + + $serverUrl = 'http://localhost:4444'; + $options = new \Facebook\WebDriver\Chrome\ChromeOptions(); + $options->addArguments(['--headless', '--disable-gpu', '--no-sandbox', '--disable-dev-shm-usage']); + + $capabilities = DesiredCapabilities::chrome(); + $capabilities->setCapability(\Facebook\WebDriver\Chrome\ChromeOptions::CAPABILITY, $options); + + if (!$js) + $capabilities->setCapability('javascriptEnabled', false); + + $driver = null; + $error = null; + try { + $driver = RemoteWebDriver::create($serverUrl, $capabilities, 30000, 60000); + $driver->manage()->window()->setSize(new \Facebook\WebDriver\WebDriverDimension($vpParts[0], $vpParts[1])); + $driver->get($target); + $driver->executeScript('document.body.style.overflow = "hidden";'); + + addToLog("$ip\tRequested $target with viewport $viewport and js " . ($js ? 'enabled' : 'disabled')); + + $screenshot = $driver->takeScreenshot(); + header('Content-Type: image/png'); + header('Content-Length: ' . strlen($screenshot)); + echo $screenshot; + } catch (Exception $e) { + $error = $e->getMessage(); + addToLog("$ip\tRequested $target but resulted in error:\t" . $error); + } finally { + if ($driver instanceof RemoteWebDriver) { + try { $driver->quit(); } catch (Exception $q) {} + } + } + if ($error !== null) { + header('HTTP/1.0 500 Internal Server Error'); + echo 'Screenshot failed'; + } + + break; +```