diff --git a/docs/devops-deploy.md b/docs/devops-deploy.md index 84ae28c..3d988dc 100644 --- a/docs/devops-deploy.md +++ b/docs/devops-deploy.md @@ -139,7 +139,6 @@ server { proxy_set_header X-Forwarded-Proto $scheme; proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; - proxy_set_header X-Forwarded-Proto $scheme; } } ``` diff --git a/docs/sonnet-audit.md b/docs/sonnet-audit.md new file mode 100644 index 0000000..a78e96e --- /dev/null +++ b/docs/sonnet-audit.md @@ -0,0 +1,164 @@ +# Аудит ipwhitelist-app — Claude Sonnet 4.6 + +**Дата:** 2026-06-04 +**Файлы проверены:** server.js, src/auth.js, src/routes/oidc.js, src/api/routes/entries.js, ui/routes/auth.js, src/config.js, ui/routes/entries.js, ui/api-client.js, views/index.ejs, .env.example, docs/devops-deploy.md + +--- + +## Вердикт + +**С оговорками** — к деплою почти готов, но есть один critical баг (header injection) и один major баг (зависающие запросы). После исправления этих двух пунктов — production ready. + +--- + +## Проблемы + +### 🔴 CRITICAL + +**1. HTTP header injection в `/export` через параметр `filename`** + +`server.js`, строки ~87-91: +```js +const fname = req.query.filename || 'white-list.txt'; +res.set('Content-Disposition', disp + '; filename="' + fname + '"'); +``` +Если `filename` содержит `"` или `\r\n` — атакующий может внедрить произвольные HTTP-заголовки или сломать ответ. Эндпоинт публичный (без авторизации). + +**Исправление:** санитизировать `fname` до вставки: +```js +const fname = (req.query.filename || 'white-list.txt').replace(/[^\w\-_.]/g, '_'); +``` + +--- + +### 🟠 MAJOR + +**2. `ui/api-client.js`: таймаут не работает** + +```js +req.on('timeout', () => { req.destroy(); reject(new Error('API timeout')); }); +// ...но req.setTimeout() нигде не вызывается +``` +В Node.js событие `timeout` на `http.ClientRequest` срабатывает только если установлен socket timeout через `req.setTimeout(ms)`. Без него запрос может висеть бесконечно при недоступном сервере. Все UI-запросы к API могут зависнуть. + +**Исправление:** добавить `req.setTimeout(15000, ...)` после создания запроса. + +**3. Отсутствует шаг 6 в `docs/devops-deploy.md`** + +В документе многократно упоминается «шаг 6 — регистрация Keycloak OIDC клиента», но сам шаг не написан. DevOps не сможет развернуть продакшен без понимания как создать Confidential client в Keycloak (Valid Redirect URIs, Client Authentication, получение KC_CLIENT_SECRET). + +--- + +### 🟡 MINOR + +**4. Дублирование `X-Forwarded-Proto` в nginx config** + +В `docs/devops-deploy.md`, в блоке nginx server {} на 443: +```nginx +proxy_set_header X-Forwarded-Proto $scheme; +proxy_set_header X-Real-IP $remote_addr; +proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; +proxy_set_header X-Forwarded-Proto $scheme; # ← дубль +``` +Второй заголовок — лишний, не вредит но вводит в заблуждение. + +**5. `JSON.parse` без try-catch в `ui/api-client.js`** + +```js +const data = ct.includes('application/json') ? JSON.parse(raw) : raw; +``` +Если API вернёт невалидный JSON при `content-type: application/json` — необработанный SyntaxError упадёт наверх и отдаст 500 без внятного сообщения. + +**6. Нет ограничения размера ответа IAM API** + +В `fetchIamUser` и `switchProfile` (`src/auth.js`) ответ накапливается без ограничения: +```js +let data = ''; +res.on('data', c => { data += c; }); +``` +Злонамеренный или сломанный IAM-сервер может передать гигабайты данных. + +--- + +## Ответы на 5 вопросов + +### 1. Безопасность + +| Проверка | Результат | +|---|---| +| Open redirect | ✅ Защищён. `safeLocal()` и `safeReturn()` — корректная проверка `startsWith('/')` && `!startsWith('//')`. | +| XSS в шаблонах | ✅ EJS использует `<%= %>` (с экранированием). Helmet + CSP настроены. | +| CSRF | ✅ `csrf.js` middleware подключён к мутирующим роутам. | +| Утечка токенов | ✅ Токен в сессии (PostgreSQL), не в HTML, не в cookie клиента напрямую. | +| Header injection | ❌ `/export` — `filename` из query без санитизации (см. CRITICAL #1). | +| Rate limiting | ✅ `authLimiter` на POST /login. | +| DEV_MODE в production | ✅ Жёсткая защита — `process.exit(1)` при `NODE_ENV=production && DEV_MODE=true`. | + +### 2. IAM интеграция + +`fetchIamUser` и `switchProfile` — реализованы корректно: +- Timeout: 10 секунд через `req.setTimeout(10000)` — OK. +- Error handling: IAM недоступен → `console.error` + fallback на JWT claims. Правильно. +- Fallback в `/callback` (`src/routes/oidc.js`): вычисляет `isAdmin` по `activeClientId === ADMIN_CLIENT_ID`. Безопасно — токен уже верифицирован подписью. +- `activeProfileId` сохраняется в сессии — переключение компаний через IAM API работает. + +**Единственный риск:** если `IAM_API_URL` недоступен при логине — `isAdmin: false` всегда, даже для администратора. Это намеренный безопасный дефолт, но пользователи об этом не уведомляются в UI. + +### 3. Роли/компании + +| Аспект | Результат | +|---|---| +| `isAdmin` | ✅ В IAM-режиме — из `ui.isAdmin` (IAM единственный источник). В fallback — по `clientId`. | +| `allClientIds` | ✅ `resolveCompany()` в API проверяет `profiles` (IAM) или `allClientIds` (fallback). Изоляция между компаниями соблюдена. | +| Admin ?company= | ✅ `parseInt` + `isFinite` + проверка в БД — инъекции через company ID невозможны. | +| UI vs API | ✅ UI полностью делегирует бизнес-логику к API. Расхождений нет. | +| Мульти-компания | ✅ `client_id` из query проверяется против `allowedIds` перед использованием. | + +### 4. DevOps + +**.env.example:** достаточен — все обязательные переменные перечислены с пояснениями. +**devops-deploy.md:** хорошая документация по PostgreSQL, Node.js, nginx, PM2, certbot. Но: +- Шаг 6 (Keycloak OIDC client) — отсутствует (см. MAJOR #3). +- Нет инструкции по ротации SESSION_SECRET при компрометации. +- `IAM_API_URL` задан по умолчанию в `.env.example` (не закомментирован) — хорошо, но стоит добавить пометку что это реальный внешний сервис. + +### 5. Код + +| Проверка | Результат | +|---|---| +| Незакрытые соединения | ⚠️ `ui/api-client.js` — `timeout` event без `req.setTimeout()` (см. MAJOR #2). `fetchIamUser`/`switchProfile` — OK, `setTimeout` есть. | +| Race conditions | ✅ Отсутствуют. Переключение профиля — sequential await. | +| Циклические require | ✅ `ui/routes/entries.js` → `src/auth` (lazy inside handler) — нет цикла. | +| JSON.parse unsafe | ⚠️ `ui/api-client.js` — без try-catch (см. MINOR #5). | +| Body size limits | ✅ `express.urlencoded({ limit: '32kb' })`, `express.json({ limit: '32kb' })`. | + +--- + +## Оценки + +| Категория | Оценка | Обоснование | +|---|---|---| +| **DevOps** | **7 / 10** | Хорошая документация, fail-fast проверки, но нет Keycloak-шага. | +| **Код** | **7 / 10** | Чистая архитектура API+UI, правильный CSRF/auth. Баг с таймаутом в api-client и header injection снижают. | + +--- + +## Топ-3 что исправить + +1. **[CRITICAL] Санитизировать `filename` в `/export`** (`server.js` ~87): + ```js + const fname = (req.query.filename || 'white-list.txt').replace(/[^\w\-_.]/g, '_'); + ``` + +2. **[MAJOR] Добавить `req.setTimeout()` в `ui/api-client.js`** (~35): + ```js + const req = lib.request(opts, (res) => { ... }); + req.setTimeout(15000, () => { req.destroy(); reject(new Error('API timeout')); }); + ``` + Убрать дублирующий `req.on('timeout', ...)` или оставить оба. + +3. **[MAJOR] Написать шаг 6 в `docs/devops-deploy.md`** — регистрация OIDC-клиента в Keycloak: + - Тип клиента: Confidential, Authorization Code Flow + - Valid Redirect URIs: `https://ваш-домен/callback` + - Web Origins: `https://ваш-домен` + - Как скопировать KC_CLIENT_ID и KC_CLIENT_SECRET diff --git a/package.json b/package.json index 0665220..db295ff 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "ipwhitelist", - "version": "0.5.20", + "version": "0.5.21", "description": "IP WhiteList microservice for cloud provider", "main": "server.js", "scripts": { diff --git a/server.js b/server.js index d04c8a1..15750a5 100644 --- a/server.js +++ b/server.js @@ -92,7 +92,7 @@ async function start() { const aggregated = aggregateCIDRs(cidrs); const text = aggregated.join('\n') + (aggregated.length ? '\n' : ''); res.set('Content-Type', 'text/plain; charset=utf-8'); - const fname = req.query.filename || 'white-list.txt'; + const fname = (req.query.filename || 'white-list.txt').replace(/[^\w\-_. ]/g, '_'); const disp = req.query.view === '1' ? 'inline' : 'attachment'; res.set('Content-Disposition', disp + '; filename="' + fname + '"'); res.send(text); diff --git a/ui/api-client.js b/ui/api-client.js index 2101af5..9caf734 100644 --- a/ui/api-client.js +++ b/ui/api-client.js @@ -48,12 +48,16 @@ function apiRequest(method, path, token, body) { res.on('data', c => { raw += c; }); res.on('end', () => { const ct = res.headers['content-type'] || ''; - const data = ct.includes('application/json') ? JSON.parse(raw) : raw; - resolve({ status: res.statusCode, data }); + try { + const data = ct.includes('application/json') ? JSON.parse(raw) : raw; + resolve({ status: res.statusCode, data }); + } catch (e) { + reject(new Error('Invalid JSON from API: ' + e.message)); + } }); }); + req.setTimeout(15000, () => { req.destroy(); reject(new Error('API timeout')); }); req.on('error', reject); - req.on('timeout', () => { req.destroy(); reject(new Error('API timeout')); }); if (bodyStr) req.write(bodyStr); req.end(); });