cleanup: аудиты → audits/, версии актуальны, gitignore
- audits/: все аудиты + ревью вынесены из docs/ - .gitignore: audits/, удалены старые паттерны - README.md: v0.5.22 - .github/AGENT-GUIDE.md: v0.5.22 - Удалены из трекинга: TZ-COMPLIANCE, аудиты
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
@@ -30,6 +30,7 @@ docs/questions-devops.md
|
||||
|
||||
# Результаты код-ревью агентов
|
||||
research/
|
||||
audits/
|
||||
docs/agent-opinions.md
|
||||
|
||||
# Логи тестов
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
# IP WhiteList v0.5.18
|
||||
# IP WhiteList v0.5.22
|
||||
|
||||
Self-service портал для управления доверенными IPv4-адресами клиентов облачного провайдера.
|
||||
Записи исключаются из блокировки системами фильтрации во время DDoS-атак.
|
||||
|
||||
@@ -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=<id>` |
|
||||
| Колонки: значение, комментарий, автор, дата создания, дата изменения | ✅ | |
|
||||
| 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-проверку масок и запрещённых диапазонов
|
||||
@@ -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 нужен более глубокий анализ.
|
||||
@@ -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: ''`
|
||||
|
||||
Формы содержат `<input type="hidden" name="_csrf" value="<%= 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-ручек.
|
||||
@@ -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-путь к предсказуемой модели.
|
||||
@@ -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
|
||||
Reference in New Issue
Block a user