From c1df27d29de1976bb45ac29be73da14a480c977e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9CNaeel=E2=80=9D?= Date: Thu, 4 Jun 2026 15:48:16 +0300 Subject: [PATCH] =?UTF-8?q?fix:=20DeepSeek=20audit=20=E2=80=94=20header=20?= =?UTF-8?q?injection=20+=20IAM=20limit=20+=20bump=200.5.22?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - src/api/routes/entries.js: санитизация filename в /api/v1/entries/export - ui/routes/export.js: санитизация filename в UI /export - src/auth.js: лимит 100KB для IAM-ответов - src/api/routes/entries.js: fallback для пустого allowedIds --- docs/deepseek-audit.md | 231 ++++++++++++++++++++++++++++++++++++++ package.json | 2 +- src/api/routes/entries.js | 5 +- src/auth.js | 4 +- ui/routes/export.js | 2 +- 5 files changed, 238 insertions(+), 6 deletions(-) create mode 100644 docs/deepseek-audit.md diff --git a/docs/deepseek-audit.md b/docs/deepseek-audit.md new file mode 100644 index 0000000..640d290 --- /dev/null +++ b/docs/deepseek-audit.md @@ -0,0 +1,231 @@ +# Аудит ipwhitelist-app — DeepSeek V4 Pro + +**Дата:** 2026-06-04 +**Файлы проверены:** все 30+ файлов из src/, ui/, views/, docs/, server.js, package.json, .env.example, sql/schema.sql + +--- + +## Вердикт + +**С оговорками** — код высокого качества, архитектура продумана, но есть 2 проблемы которые нужно исправить перед production: CSRF не подключён и две /export-ручки без санитизации filename. + +--- + +## 1. Найденные проблемы + +### 🔴 CRITICAL + +**C1. Header injection в двух /export-ручках (не санитизирован filename)** + +В проекте **три** эндпоинта `/export`. Санитизация `filename` добавлена только в одном (`server.js:93` — публичный `/export`). Два других — без защиты: + +| Файл | Строка | Статус | +|---|---|---| +| `server.js:93` (публичный `/export`) | `replace(/[^\w\-_. ]/g, '_')` | ✅ исправлено | +| `src/api/routes/entries.js:104` (`/api/v1/entries/export`) | `req.query.filename \|\| 'white-list.txt'` | ❌ без санитизации | +| `ui/routes/export.js:32` (UI `/export` за авторизацией) | `req.query.filename \|\| 'white-list.txt'` | ❌ без санитизации | + +Атакующий с Bearer-токеном может внедрить `\r\n` в `Content-Disposition` через `?filename=...%0d%0a...`. + +**Рекомендация:** вынести санитизацию в хелпер `src/config.js` и использовать во всех трёх местах. + +--- + +### 🟠 MAJOR + +**M1. CSRF-защита реализована, но НЕ подключена** + +`src/middleware/csrf.js` содержит `initCsrf()` с `doubleCsrf` (csrf-csrf v4) — современная замена deprecated `csurf`. Middleware правильно настроен: cookie `csrf-token`, `getCsrfTokenFromRequest: (req) => req.body._csrf`, HMAC-подпись. + +**Но `initCsrf()` нигде не вызывается.** Ни в `server.js`, ни в `ui/index.js`, ни в роутах. + +Во всех EJS-шаблонах `csrfToken` передаётся как пустая строка: +- `views/index.ejs` → `csrfToken: ''` +- `views/admin.ejs` → `csrfToken: ''` + +Формы содержат `` — токен всегда пустой, проверка никогда не срабатывает. + +**Рекомендация:** вызвать `initCsrf()` в `server.js → start()`, передать `doubleCsrfProtection` в UI-роуты, `generateCsrfToken` — в GET-обработчики. API-слой (`/api/v1/*`) не требует CSRF (Bearer-токены). + +--- + +**M2. IAM API — нет ограничения размера ответа** + +`fetchIamUser` и `switchProfile` в `src/auth.js` накапливают ответ без лимита: + +```js +let data = ''; +res.on('data', c => { data += c; }); +``` + +Скомпрометированный или сломанный IAM-сервер может исчерпать память процесса. + +**Рекомендация:** добавить проверку `if (data.length > 100_000) { req.destroy(); reject(...) }`. + +--- + +**M3. `resolveCompany` в API entries.js — хрупкий `profiles`/`allClientIds` fallback** + +Строки 47–58: +```js +const allowedIds = req.user.profiles && req.user.profiles.length + ? req.user.profiles.map(p => p.client_id) + : (req.user.allClientIds || []); +``` + +Когда IAM недоступен, `req.user.profiles` — `undefined`. Fallback на `allClientIds` корректен, НО: если пользователь переключил компанию через UI (`activeClientId` обновлён в сессии), а API-слой этого не видит (API читает `req.user` из Bearer-токена, не из сессии), то `client_id` из query может не пройти проверку `allowedIds.includes(requestedId)` — и переключение молча проигнорируется. + +**Рекомендация:** в API-слое добавить fallback: если `allowedIds` пуст — разрешить любой `client_id` из query (доверять тому, что в токене). + +--- + +### 🟡 MINOR + +**m1. Отсутствует `proxy_read_timeout` в nginx-конфиге `docs/devops-deploy.md`** + +Nginx по умолчанию обрывает соединение через 60с. Для `/export` (агрегация тысяч CIDR) этого может не хватить. Добавить `proxy_read_timeout 120s;`. + +**m2. `server.js` — `checkConnection()` вызывается без `await`** + +Строка 127: +```js +checkConnection() + .then(() => console.log('DB connected')) + .catch(e => console.error('DB not ready:', e.message)); +``` + +Приложение стартует до проверки соединения с БД. Если БД недоступна — сервер отвечает 500 на все запросы, но не падает. Лучше: `await checkConnection()` внутри `start()` до `app.listen()`. + +**m3. `ui/routes/auth.js` —不一致的 `safeReturn`** + +POST `/login-token`: +- Ветка `token` → `res.redirect(safeReturn(returnTo) || '/')` ✅ +- Ветка `clientId` → `res.redirect(returnTo || '/')` ❌ без `safeReturn` + +Параметр `returnTo` идёт из скрытого поля формы, но теоретически может быть подменён. Низкий риск, но inconsistent. + +**m4. `src/queries.js:getExportCIDRs` — нет LIMIT** + +При вызове без `companyId` (admin) запрос возвращает ВСЕ CIDR всех компаний. Для агрегации это ожидаемо, но при 100k+ записей — memory spike. + +**m5. `src/queries.js:getAudit` — hardcoded LIMIT 500 без пагинации** + +Для production с сотнями компаний 500 строк аудита — мало. Нужна пагинация. + +**m6. `ui/routes/entries.js:42` — `require('../../src/auth')` внутри обработчика** + +Динамический `require` в рантайме (для `switchProfile`). Node.js кеширует, так что performance impact минимален, но это «code smell». Лучше прокинуть `auth` через фабрику `createRouter({ auth })`. + +--- + +## 2. Безопасность — полный разбор + +| Вектор | Статус | Комментарий | +|---|---|---| +| Open redirect `/login?returnTo=` | ✅ | `safeReturn()` + `safeLocal()` — `startsWith('/') && !startsWith('//')` | +| Open redirect `/callback` | ✅ | `safeLocal()` на `oidcReturnTo` | +| XSS (EJS) | ✅ | `<%= %>` везде (экранирование), helmet + CSP | +| CSRF | ❌ | Код написан, но не подключён (см. M1) | +| Header injection | ⚠️ | Частично исправлено (см. C1) | +| Clickjacking | ✅ | `frameAncestors: ["'none'"]` в CSP | +| Rate limiting | ✅ | `authLimiter` (10/5min), `mutationLimiter` (30/min), `exportLimiter` (20/min) | +| DEV_MODE в production | ✅ | Жёсткий `process.exit(1)` | +| Session fixation | ✅ | `express-session` + `connect-pg-simple`, sessionID меняется при логине | +| SQL injection | ✅ | Parameterized queries (`$1`, `$2`) везде | +| Secret management | ✅ | Все секреты из `process.env`, fail-fast для `SESSION_SECRET` и `IAM_API_URL` | +| JWT validation | ✅ | RS256 + JWKS (Keycloak), issuer check, audience check | + +--- + +## 3. IAM-интеграция — полный разбор + +| Аспект | Статус | Комментарий | +|---|---|---| +| `fetchIamUser` timeout | ✅ | 10 секунд через `req.setTimeout(10000)` | +| `fetchIamUser` error handling | ✅ | try/catch с понятным сообщением | +| `switchProfile` | ✅ | Корректный POST с `profile_id` в body | +| OIDC callback IAM fallback | ✅ | При недоступности IAM — fallback на JWT claims с логом | +| `login-token` IAM enrichment | ✅ | Вызывается только в OIDC-режиме (`auth.isOidc`) | +| `userFromPayload` с `iamData` | ✅ | `profiles`, `allClientIds`, `isAdmin` — всё из IAM | +| `resolveCompany` multi-profile | ⚠️ | Хрупкий fallback (см. M3) | +| IAM response size limit | ❌ | Нет ограничения (см. M2) | +| Разные стенды IAM | ✅ | `IAM_API_URL` в .env, dev/test/prod через переменную | + +--- + +## 4. Корректность кода — полный разбор + +| Аспект | Статус | Комментарий | +|---|---|---| +| Циклические require | ✅ | Чистая архитектура: `config.js` → `auth.js` → `queries.js` → `db.js`. Нет циклов | +| DB connection pool | ✅ | `pg.Pool`, max=10, idleTimeout=30s, error listener | +| Транзакции | ✅ | `BEGIN/FOR UPDATE/COMMIT/ROLLBACK` в `createEntry`, `updateEntry`, `deleteEntry` | +| Mock-режим | ✅ | Локальная RSA-пара, `/dev-login`, `DEV_MODE`/`DEV_SECRET` | +| Middleware order | ✅ | helmet → static → session → OIDC → API → UI → error handler | +| Promise error handling | ✅ | Все async-обработчики обёрнуты в try/catch | +| `checkConnection` | ⚠️ | Не `await` (см. m2) | +| `rangeToCIDRs` | ✅ | Элегантный bit-twiddling алгоритм | +| `aggregateCIDRs` | ✅ | Scan-line merge + rangeToCIDRs | +| Уникальность CIDR | ✅ | Partial unique index `WHERE deleted_at IS NULL` | +| Soft delete | ✅ | `deleted_at` + `deleted_by`, записи не удаляются физически | + +--- + +## 5. DevOps — полный разбор + +| Аспект | Статус | Комментарий | +|---|---|---| +| Пошаговая инструкция | ✅ | 6 шагов: PostgreSQL → Node.js → .env → nginx → PM2 → Keycloak | +| Все зависимости | ✅ | Node.js 20, PostgreSQL 16, nginx, certbot, PM2 | +| Переменные окружения | ✅ | `.env.example` с комментариями, команды генерации секретов | +| Схема БД | ✅ | `sql/schema.sql` с индексами и constraints | +| HTTPS | ✅ | nginx + certbot, автообновление | +| Keycloak OIDC | ✅ | Пошаговая инструкция регистрации клиента (шаг 6) | +| PM2 автозапуск | ✅ | `pm2 startup systemd` + `pm2 save` | +| Противоречия в документах | ✅ | Нет — все ссылки согласованы | +| `proxy_read_timeout` | ⚠️ | Отсутствует (см. m1) | + +--- + +## 6. Оценки + +| Категория | Оценка | Обоснование | +|---|---|---| +| **DevOps-готовность** | **9/10** | Понятная инструкция, все зависимости описаны, OIDC-клиент документирован. Не хватает `proxy_read_timeout` в nginx. | +| **Код: безопасность** | **7/10** | JWT, CSP, helmet, rate-limit — на высоте. Но CSRF не подключён и две /export-ручки без санитизации filename. | +| **Код: архитектура** | **9/10** | Чистое разделение API/UI, middleware chain прозрачен, нет циклических зависимостей. | +| **Код: IAM интеграция** | **8/10** | Fallback при недоступности IAM, timeout, switchProfile. Нет лимита на ответ IAM. | +| **Тестирование** | **9/10** | 121 API-тест, 68 UI-тестов, 47+111 compliance-тестов, 191 стресс-тест. | +| **Общая оценка** | **8/10** | Крепкий продакшен-реди код. 2 обязательных фикса перед деплоем. | + +--- + +## 7. Топ-5 что исправить перед production + +1. **Подключить CSRF** — вызвать `initCsrf()` в `server.js`, передать в UI-роуты, заполнить `csrfToken` в шаблонах (M1) +2. **Санитизировать `filename` в двух оставшихся /export-ручках** — `src/api/routes/entries.js:104` и `ui/routes/export.js:32` (C1) +3. **Добавить `Content-Length` проверку в `fetchIamUser`/`switchProfile`** — лимит 100KB (M2) +4. **Добавить `proxy_read_timeout 120s` в nginx-конфиг** в `docs/devops-deploy.md` (m1) +5. **Унифицировать `safeReturn` в `ui/routes/auth.js`** — добавить в ветку `clientId` (m3) + +--- + +## 8. Сравнение с аудитом Claude Sonnet 4.6 + +Sonnet проверил 11 файлов и нашёл 6 проблем (1 critical, 2 major, 3 minor). Все его находки **подтверждаю**. + +Что Sonnet **пропустил** (и я нашёл дополнительно): + +| Проблема | Критичность | +|---|---| +| CSRF middleware не подключён (код есть, но не wired) | 🔴 MAJOR | +| Две /export-ручки без санитизации filename (проверил только публичную) | 🔴 CRITICAL | +| IAM API без ограничения размера ответа | 🟠 MAJOR | +| `resolveCompany` — хрупкий `profiles`/`allClientIds` fallback | 🟠 MAJOR | +| `checkConnection()` без `await` | 🟡 MINOR | +| Не-conсистентный `safeReturn` в `ui/routes/auth.js` | 🟡 MINOR | +| `getExportCIDRs` без LIMIT | 🟡 MINOR | +| `getAudit` без пагинации | 🟡 MINOR | +| Динамический `require` в `ui/routes/entries.js` | 🟡 MINOR | + +**Итого:** Sonnet нашёл 6 проблем, я подтверждаю все + дополнительно 9. Основное упущение Sonnet — он не проверил, что `initCsrf()` нигде не вызывается, и не заметил что санитизация filename применена только к одной из трёх export-ручек. diff --git a/package.json b/package.json index db295ff..5ff6045 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "ipwhitelist", - "version": "0.5.21", + "version": "0.5.22", "description": "IP WhiteList microservice for cloud provider", "main": "server.js", "scripts": { diff --git a/src/api/routes/entries.js b/src/api/routes/entries.js index f394366..1e4f96e 100644 --- a/src/api/routes/entries.js +++ b/src/api/routes/entries.js @@ -54,7 +54,8 @@ function createEntriesRouter({ q }) { const allowedIds = req.user.profiles && req.user.profiles.length ? req.user.profiles.map(p => p.client_id) : (req.user.allClientIds || []); - const effectiveClientId = (requestedId && allowedIds.includes(requestedId)) + // Если список компаний пуст (IAM не ответил, token без claims) — доверяем requestedId + const effectiveClientId = (requestedId && (!allowedIds.length || allowedIds.includes(requestedId))) ? requestedId : req.user.clientId; // companyName: для переключённой компании — находим в profiles или используем clientId @@ -104,7 +105,7 @@ function createEntriesRouter({ q }) { const cidrs = await q.getExportCIDRs(companyId); const aggregated = aggregateCIDRs(cidrs); res.setHeader('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.setHeader('Content-Disposition', `${disp}; filename="${fname}"`); res.send(aggregated.join('\n') + (aggregated.length ? '\n' : '')); diff --git a/src/auth.js b/src/auth.js index afc916a..f8fc4f3 100644 --- a/src/auth.js +++ b/src/auth.js @@ -297,7 +297,7 @@ async function fetchIamUser(token) { headers: { Authorization: 'Bearer ' + token, Accept: 'application/json' }, }, (res) => { let data = ''; - res.on('data', c => { data += c; }); + res.on('data', c => { data += c; if (data.length > 100_000) { req.destroy(); reject(new Error('Response too large')); } }); res.on('end', () => { if (res.statusCode !== 200) { return reject(new Error(`IAM API returned ${res.statusCode}: ${data.slice(0, 200)}`)); @@ -351,7 +351,7 @@ async function switchProfile(token, profileId) { }, }, (res) => { let data = ''; - res.on('data', c => { data += c; }); + res.on('data', c => { data += c; if (data.length > 100_000) { req.destroy(); reject(new Error('Response too large')); } }); res.on('end', () => { if (res.statusCode !== 200) { return reject(new Error(`IAM switch-profile failed: ${res.statusCode} ${data.slice(0, 200)}`)); diff --git a/ui/routes/export.js b/ui/routes/export.js index 3ac8892..2341f43 100644 --- a/ui/routes/export.js +++ b/ui/routes/export.js @@ -25,7 +25,7 @@ function createRouter() { res.set('Content-Type', 'text/plain; charset=utf-8'); // ?view=1 → inline (просмотр), иначе → attachment (скачивание) // ?filename=X → своё имя, по умолчанию white-list.txt - 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(r.data);