From a3cf9812df2235757d14d53cd1de1a3201044bbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9CNaeel=E2=80=9D?= Date: Thu, 4 Jun 2026 15:56:38 +0300 Subject: [PATCH] =?UTF-8?q?cleanup:=20=D0=B0=D1=83=D0=B4=D0=B8=D1=82=D1=8B?= =?UTF-8?q?=20=E2=86=92=20audits/,=20=D0=B2=D0=B5=D1=80=D1=81=D0=B8=D0=B8?= =?UTF-8?q?=20=D0=B0=D0=BA=D1=82=D1=83=D0=B0=D0=BB=D1=8C=D0=BD=D1=8B,=20gi?= =?UTF-8?q?tignore?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - audits/: все аудиты + ревью вынесены из docs/ - .gitignore: audits/, удалены старые паттерны - README.md: v0.5.22 - .github/AGENT-GUIDE.md: v0.5.22 - Удалены из трекинга: TZ-COMPLIANCE, аудиты --- .github/AGENT-GUIDE.md | 2 +- .gitignore | 1 + README.md | 2 +- docs/TZ-COMPLIANCE-REPORT.md | 141 --------------------- docs/ai-review-comparison.md | 67 ---------- docs/deepseek-audit.md | 231 ----------------------------------- docs/production-audit.md | 64 ---------- docs/sonnet-audit.md | 164 ------------------------- 8 files changed, 3 insertions(+), 669 deletions(-) delete mode 100644 docs/TZ-COMPLIANCE-REPORT.md delete mode 100644 docs/ai-review-comparison.md delete mode 100644 docs/deepseek-audit.md delete mode 100644 docs/production-audit.md delete mode 100644 docs/sonnet-audit.md diff --git a/.github/AGENT-GUIDE.md b/.github/AGENT-GUIDE.md index b03725a..f105c10 100644 --- a/.github/AGENT-GUIDE.md +++ b/.github/AGENT-GUIDE.md @@ -1,7 +1,7 @@ # IP Whitelist App — AI Agent Guide > For AI coding agents (Copilot, etc.) to understand and continue development. -> Last updated: 2026-06-03 | Version: 0.5.14 +> Last updated: 2026-06-04 | Version: 0.5.22 ## 1. Quick Overview diff --git a/.gitignore b/.gitignore index 68b7a8e..e26278a 100644 --- a/.gitignore +++ b/.gitignore @@ -30,6 +30,7 @@ docs/questions-devops.md # Результаты код-ревью агентов research/ +audits/ docs/agent-opinions.md # Логи тестов diff --git a/README.md b/README.md index ba93155..424d76a 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,4 @@ -# IP WhiteList v0.5.18 +# IP WhiteList v0.5.22 Self-service портал для управления доверенными IPv4-адресами клиентов облачного провайдера. Записи исключаются из блокировки системами фильтрации во время DDoS-атак. diff --git a/docs/TZ-COMPLIANCE-REPORT.md b/docs/TZ-COMPLIANCE-REPORT.md deleted file mode 100644 index c5d566a..0000000 --- a/docs/TZ-COMPLIANCE-REPORT.md +++ /dev/null @@ -1,141 +0,0 @@ -# ТЗ-Compliance Report — 2026-05-31 (v0.5.0) - -Независимая проверка соответствия кода Техническому Заданию (`docs/ТЗ.md`). -Аутентификация "по-настоящему" (OIDC) и сценарий "несколько компаний на пользователя" — ИСКЛЮЧЕНЫ из проверки (уточняется). - ---- - -## ИТОГ: 45/45 тестов пройдено ✅ - -Все функциональные требования ТЗ **реализованы**. Расхождения — только в объёме/глубине реализации. - ---- - -## Детальный разбор по пунктам ТЗ - -### 1. Назначение и цели — ✅ - -| Требование | Статус | Где | -|---|---|---| -| Web-интерфейс для управления списком | ✅ | `views/index.ejs`, `ui/routes/entries.js` | -| Единая точка для инженеров | ✅ | `/admin`, `/audit` | -| Машиночитаемая выдача агрегированного списка | ✅ | `/exp` (временный), `/export` | - -### 2. Объем работ — ✅ - -| Требование | Статус | Где | -|---|---|---| -| Web-страница | ✅ | SSR через EJS | -| Авторизация OIDC | ⏸️ | mock (DEV_MODE=true), OIDC-код написан но не подключён | -| Валидация клиент + сервер | ⚠️ | Сервер: ✅ полная. Клиент: только HTML5 `pattern` | -| Внешний endpoint txt | ✅ | `/exp` (без авторизации) | -| Хранение + аудит | ✅ | PostgreSQL 3 таблицы | -| Административное управление лимитами | ✅ | `/admin`, `PATCH /api/v1/companies/:id/limit` | - -### 3. Роли и права — ✅ - -| Требование | Статус | -|---|---| -| Client: видит только свои записи | ✅ | -| Client: CRUD своих записей в пределах лимита | ✅ | -| Admin (WZ01112): видит все компании | ✅ | -| Admin: CRUD всех записей | ✅ | -| Admin: изменение лимитов | ✅ | -| Несколько компаний на пользователя | ⏸️ (не реализовано — ждём devops) | - -### 4.1. Просмотр списка — ✅ - -| Требование | Статус | Примечание | -|---|---|---| -| Таблица записей активной компании | ✅ | | -| Admin — записи всех компаний с фильтром | ✅ | `?company=` | -| Колонки: значение, комментарий, автор, дата создания, дата изменения | ✅ | | -| Soft-deleted скрыты по умолчанию | ✅ | | -| Фильтр soft-deleted для admin | ❌ | `listEntries(includeDeleted)` есть, но API не принимает параметр | -| «использовано X из N» | ✅ | `used` / `limit` в ответе API | - -### 4.2. Создание записи — ✅ - -| Требование | Статус | -|---|---| -| Поля: значение, комментарий (до 255) | ✅ | -| Валидация на клиенте и сервере | ✅ (см. примечание по клиентской) | -| Проверка лимита, пересечений, дубликатов, запрещённых диапазонов | ✅ | -| Аудит при создании | ✅ | - -### 4.3. Редактирование — ✅ - -| Требование | Статус | -|---|---| -| Редактирование значения и комментария | ✅ | -| Повторная полная проверка при изменении значения | ✅ | -| Аудит с прежним и новым значением | ✅ | - -### 4.4. Soft delete — ✅ - -| Требование | Статус | -|---|---| -| Логическое удаление (deleted_at, deleted_by) | ✅ | -| Освобождение места в лимите | ✅ | -| Исключение из выдачи | ✅ | -| Аудит | ✅ | - -### 4.5. Лимиты — ✅ - -| Требование | Статус | -|---|---| -| Глобальный лимит по умолчанию: 15 | ✅ | -| Настройка через конфигурацию (env) | ✅ `DEFAULT_LIMIT` | -| Per-company лимит (выше и ниже глобального) | ✅ `custom_limit` | -| Блокировка с понятным сообщением | ✅ 409 с «Лимит исчерпан» | -| Снижение лимита не удаляет существующие записи | ✅ | - -### 4.6. Журнал аудита — ✅ - -| Требование | Статус | -|---|---| -| Все изменяющие операции фиксируются | ✅ CREATE, UPDATE, DELETE | -| Поля: кто, когда, компания, действие, прежнее/новое состояние | ✅ | -| Доступен только администратору | ✅ 403 для user | - -### 4.7. Внешняя выдача — ⚠️ - -| Требование | Статус | -|---|---| -| Суммаризация CIDR по всем компаниям | ✅ `aggregateCIDRs()` | -| HTTP GET endpoint, txt | ⚠️ `/exp` (временный), `/export` требует авторизации | -| Без авторизации (сетевое ограничение) | ⚠️ `/exp` — да, `/export` — нет | - -### 5. Валидация — ✅ - -| Требование | Статус | -|---|---| -| /22–/32 маски | ✅ | -| Только IPv4 | ✅ | -| Нормализация host bits + уведомление | ✅ `wasNormalized` передаётся в UI | -| Запрет 14 диапазонов (Приложение А) | ✅ все 14 | -| Отсутствие дубликатов в компании | ✅ | -| Отсутствие пересечений в компании | ✅ | -| Комментарий ≤ 255 | ✅ | -| Клиентская валидация | ⚠️ только HTML5 `pattern` — нет JS-проверки диапазонов | - ---- - -## НЕРЕАЛИЗОВАННЫЕ / НЕПОЛНЫЕ пункты - -| # | Пункт ТЗ | Описание расхождения | Приоритет | -|---|---|---|---| -| 1 | 4.1 | Фильтр soft-deleted записей для admin: `listEntries(includeDeleted=true)` есть, но API-роут `GET /api/v1/entries` не принимает `?includeDeleted=true` | P2 | -| 2 | 4.7 | `/export` требует авторизацию (Bearer). По ТЗ — без авторизации (сетевое ограничение). Обход: `/exp` | P1 — требуется решение | -| 3 | 5 | Клиентская валидация — только HTML5 `pattern`, нет JS-проверки масок /22–/32 и запрещённых диапазонов на стороне браузера | P3 | -| 4 | 3.1 | Несколько компаний на пользователя — не реализовано (ждём уточнения от devops) | P1 | -| 5 | 2 | OIDC-авторизация — код написан (`src/auth.js` OIDC-ветка, `src/routes/auth.js` с `/callback`), но не подключён в `server.js` | P0 для прода | - ---- - -## РЕКОМЕНДАЦИИ - -1. **Перед продом:** подключить OIDC-роутер, убрать DEV_MODE -2. **Экспорт:** решить — делать `/export` публичным или оставить `/exp` как временное решение -3. **Soft-delete фильтр:** добавить `?includeDeleted=true` в API и UI для admin -4. **Клиентская валидация:** добавить JS-проверку масок и запрещённых диапазонов diff --git a/docs/ai-review-comparison.md b/docs/ai-review-comparison.md deleted file mode 100644 index 4225541..0000000 --- a/docs/ai-review-comparison.md +++ /dev/null @@ -1,67 +0,0 @@ -# AI‑анализ проекта ipwhitelist‑app — сравнение - -> Дата: 2026-06-04 -> Простая модель (GPT) vs DeepSeek V4 Pro - ---- - -## 1. Анализ простой модели (prompt‑review.txt) - -### Общее заключение -Нет. Нельзя деплоить в production в текущем состоянии. - -### Найденные проблемы - -| # | Серьёзность | Описание | -|---|---|---| -| 1 | **Critical** | `ui/routes/auth.js` — `await` без `async` в `POST /login‑token` → SyntaxError | -| 2 | **Critical** | `server.js` — `/export` открыт публично без авторизации | -| 3 | **Major** | Источники профилей не унифицированы (JWT vs IAM), хардкод `WZ01112` для admin | -| 4 | **Major** | `jwt.decode()` без верификации в UI | -| 5 | **Minor** | `'unsafe-inline'` в CSP | -| 6 | **Minor** | Разнобой таймаутов HTTP (5s vs 10s) | -| 7 | **Minor** | Логирование IAM слабое | - -### Оценки - -| Критерий | Балл | -|---|---| -| Понятность DevOps | 8/10 | -| Чистота кода | 6/10 | - ---- - -## 2. Оценка DeepSeek V4 Pro - -### Что модель нашла верно -- `await` без `async` — реальный критический баг, приложение упадёт при запуске. -- `/export` без авторизации — реальная дыра безопасности. -- Дублирование IAM/JWT‑логики — действительно размазано по `oidc.js`, `auth.js`, `ui/routes/auth.js`. -- `'unsafe-inline'` в CSP — надо убирать (nonce/hash). -- Разнобой таймаутов — мелочь, но стоит унифицировать. - -### Что модель пропустила - -| # | Серьёзность | Описание | -|---|---|---| -| 1 | **Critical** | Хардкоженный IAM‑токен `tazetdinovn@gmail.com` (prod) лежит в `tests/api-crud.sh` в открытом виде и уже в гите. | -| 2 | **Major** | `DEV_MODE=true` на итало — но код пытается вызывать `fetchIamUser` даже в mock‑режиме. IAM недоступен локально → каждый логин будет падать с таймаутом или ошибкой. | -| 3 | **Major** | Порядок middleware: `session` → `oidc` → `ui` — в OIDC‑роутере `src/routes/oidc.js` свой `req.session.user`, а `ui/index.js` делает свой `jwt.decode()` — возможен конфликт/перезапись. | -| 4 | **Major** | Нет механизма миграций БД — только `schema.sql` с CREATE, нет ALTER/версионирования. | -| 5 | **Minor** | `csrfToken` в `views/index.ejs` передаётся как пустая строка (`csrfToken: ''`) — CSRF фактически отключён для UI‑слоя. | -| 6 | **Minor** | `package.json` → `"version": "0.5.17"`, а `README.md` → `v0.5.14` — расхождение версий. | -| 7 | **Minor** | `require('../auth')` внутри `src/routes/oidc.js` создаёт циклическую зависимость: `auth.js` → `routes/oidc.js` → `auth.js`. Работает только из‑за кеша Node.js, но хрупко. | - ---- - -## 3. Итоговое сравнение - -| Параметр | Простая модель | DeepSeek | -|---|---|---| -| Критические баги найдены | 2 из 3 | +1 (токен в гите) | -| Архитектурные проблемы | Поверхностно | Глубже (middleware, циклические зависимости, миграции) | -| Рантайм‑поведение | Не анализировала | Учёл DEV_MODE и реальные сценарии | -| Точность попаданий | 6/7 верных | Все найденные подтверждены + новые | -| Ложные срабатывания | 0 | 0 | - -**Вывод:** простая модель дала добротный первый проход — нашла два критических бага и дала разумные рекомендации. Но не копнула глубже: пропустила утекший токен, не проверила рантайм‑поведение в DEV_MODE и не заметила архитектурные завязки. Для production‑review нужен более глубокий анализ. diff --git a/docs/deepseek-audit.md b/docs/deepseek-audit.md deleted file mode 100644 index 640d290..0000000 --- a/docs/deepseek-audit.md +++ /dev/null @@ -1,231 +0,0 @@ -# Аудит 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/docs/production-audit.md b/docs/production-audit.md deleted file mode 100644 index 2d8ee10..0000000 --- a/docs/production-audit.md +++ /dev/null @@ -1,64 +0,0 @@ -# Production Audit: ipwhitelist-app - -Дата ревью: 2026-06-04 -Основа: `prompt-review.txt` - -## Общее заключение - -В production отдавать нельзя без доработок. Код в целом собран аккуратно, но есть критичные риски по доступу к `/export`, redirect-логике и IAM fallback-пути. Документация частично актуальна, но для DevOps сейчас недостаточно согласована и содержит противоречия между старым и новым описанием аутентификации. - -## Найденные проблемы - -### Critical - -1. Публичный `/export` в `server.js` открыт без авторизации и без сетевого allowlist. Это противоречит безопасной production-модели и делает выдачу whitelist доступной любому, кто достучится до приложения. -2. Логика `returnTo`/redirect допускает open redirect. Значение берётся из запроса и дальше уходит в `res.redirect()` без нормализации. Это нужно закрывать через `safeReturn()` или эквивалентный allowlist локальных путей. - -### Major - -1. IAM fallback работает слишком мягко: при ошибке `fetchIamUser()` приложение silently переходит на JWT claims. Это может привести к неверному определению компании, роли администратора или активного профиля. -2. Документация не сведена в один актуальный источник. `README.md`, `docs/devops-deploy.md`, `docs/ТЗ-реализация.md`, `docs/keycloak-auth-reference.md` и `docs/iam-integration.md` частично дублируют и частично противоречат друг другу. -3. Описание `/export` в документации и в коде расходится. В одном месте маршрут показан как публичный, в другом — как Bearer-protected. Для DevOps это опасно: можно ошибиться на этапе деплоя. - -### Minor - -1. CSP сейчас рабочий, но ослаблен `unsafe-inline` для скриптов и стилей. Для текущего inline-heavy UI это ожидаемо, но защита от XSS ограниченная. -2. Есть устаревшие или лишние пояснения вокруг mock-режима и IAM, из-за чего сложно понять, какой путь является production и какой — fallback для тестов. - -## Понятность DevOps - -Оценка: 4/10 - -Почему так низко: - -- есть хороший стартовый deploy guide, но он смешан с историческими и legacy-документами; -- не все переменные окружения и режимы описаны в одном месте; -- есть расхождения по IAM, роли администратора, multi-company и `/export`. - -## Чистота кода - -Оценка: 6/10 - -Плюсы: - -- логика разделена на слои: auth, API, UI, queries, validators; -- есть нормализация CIDR, защита от дубликатов и пересечений, soft delete, аудит; -- middleware и роутизация читаются достаточно последовательно. - -Минусы: - -- fallback-пути слишком терпимые и могут скрыть проблемы интеграции; -- redirect-логика требует жёсткой нормализации; -- часть безопасности опирается на inline-механики и не доведена до строгого production-профиля. - -## Что исправить перед продом - -1. Закрыть `/export` по умолчанию и включать его только через явную production-политику доступа. -2. Нормализовать все `returnTo`/redirect-цели через `safeReturn()` или аналогичный allowlist. -3. Перевести IAM fallback на fail-closed поведение либо явно документировать, при каких условиях он допустим. -4. Свести документацию в один актуальный production runbook и пометить старые документы как legacy. -5. Добавить тесты на open redirect, IAM-fallback и доступ к `/export`. - -## Краткий итог - -Код готов к дальнейшей доводке, но до production не дотягивает из-за вопросов безопасности и несогласованной документации. Если нужен рабочий контур для DevOps, сначала надо закрыть доступ к `/export`, убрать open redirect и привести IAM-путь к предсказуемой модели. \ No newline at end of file diff --git a/docs/sonnet-audit.md b/docs/sonnet-audit.md deleted file mode 100644 index a78e96e..0000000 --- a/docs/sonnet-audit.md +++ /dev/null @@ -1,164 +0,0 @@ -# Аудит 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