diff --git a/.gitignore b/.gitignore index 6744824..0a99fcb 100644 --- a/.gitignore +++ b/.gitignore @@ -13,5 +13,11 @@ AGENT-DIAGNOSIS.md CONTEXT.md prompt-opus-*.md +# Результаты код-ревью агентов +research/opus-review-*.md +research/opus-full-review-*.md +research/REVIEW-SUMMARY.md +docs/STATE-old.md + # Результаты тестов — генерируются при прогоне test-results/ diff --git a/docs/STATE-old.md b/docs/STATE-old.md deleted file mode 100644 index f47486f..0000000 --- a/docs/STATE-old.md +++ /dev/null @@ -1,204 +0,0 @@ -# Состояние проекта на 2026-05-30 14:43 (ветка `sonnet`) - -## Репозитории - -| Репо | URL | Ветка | Локальный путь | -|---|---|---|---| -| Код приложения | `https://gitea.services.ngcloud.ru/Nail/ipwhitelist-app.git` | **sonnet** | `/home/naeel/ipwhitelist-app` | -| Документация | `https://gitea.services.ngcloud.ru/Nail/IPWhiteList.git` | main | `/home/naeel/IPWhiteList` | - -## Стек - -Node.js + Express + EJS + PostgreSQL + pg pool + jsonwebtoken - -## Деплой - -- URL: `https://white.nodejsk8s.dev.nubes.ru` -- DEV_MODE=true (мок-аутентификация) -- ⚠️ Нужен передеплой Nubes для ветки sonnet - -## БД - -- `write.bde8229b-1381-4330-b24b-727ad73fcb44.dev.nubes.ru` -- user: `super`, db: `ipwhitelist` -- Миграция от 2026-05-30 применена (CHECK, UNIQUE, индексы) - ---- - -## Что сделано в ветке `sonnet` (4 коммита поверх master) - -### 1. `src/validators.js` — CIDR агрегация -- `aggregateCIDRs(cidrs)` — суммаризация: merge пересекающихся + смежных диапазонов → минимальный набор CIDR -- Вспомогательные: `numToIP(n)`, `rangeToCIDRs(start, end)` -- Экспортируется и используется в `/export` - -### 2. `src/auth.js` — admin-роль -- `ADMIN_CLIENT_ID` = `process.env.ADMIN_CLIENT_ID || 'WZ01112'` -- `req.user.isAdmin` — определяется по `clientId === ADMIN_CLIENT_ID` -- `DEV_ADMIN=true` в `.env` → admin-права в dev-режиме -- `requireAdmin` middleware — 403 для не-admin - -### 3. `src/queries.js` — новые функции -- `getCompanyById(id)` — компания по числовому PK -- `getAllCompanies()` — все компании + `active_count` (LEFT JOIN) -- `setLimit(companyId, newLimit)` — установить/сбросить (null) индивидуальный лимит -- `getAudit()` — обновлён: JOIN с companies (поля `company_name`, `client_id`) - -### 4. `server.js` — новые роуты -- `/export` — публичный (до auth.middleware), возвращает агрегированный список всех компаний -- `GET /` — admin: видит все компании с переключателем `?company=X` -- `POST /edit/:id` — редактирование записи (admin + user) -- `GET /audit` — журнал аудита (только admin) -- `GET /admin` — управление лимитами (только admin) -- `POST /admin/limit/:companyId` — изменить/сбросить лимит компании -- `backUrl()` — хелпер для редиректа обратно с учётом контекста admin/user - -### 5. `views/index.ejs` -- Admin-панель выбора компании (dropdown + быстрые ссылки на Аудит/Лимиты) -- Кнопки Аудит/Лимиты/Выйти в header для admin -- Кнопка "Изменить" в таблице (открывает edit modal) -- Edit modal — overlay с формой, закрывается по Escape/backdrop -- Скрытый `company_id` в формах add/delete для корректной admin-ветки - -### 6. `views/audit.ejs` — **новый** -- Таблица журнала аудита с фильтром по компании -- Цветные badges (CREATE/UPDATE/DELETE) -- old_value → new_value стрелочкой - -### 7. `views/admin.ejs` — **новый** -- Таблица всех компаний: client_id, active_count, лимит -- Прогресс-бар использования (зелёный/янтарный/красный) -- Форма изменения лимита с подтверждением; кнопка ↺ сброс на дефолт - ---- - -## Не сделано (production-hardening, не баги) - -- helmet (X-Frame-Options, CSP, HSTS) -- rate-limit на POST /add, /delete, /export -- CSRF-токены в формах -- JWT-верификация через внешний JWKS (нужен URL от девопсов) -- Multi-company (нужен формат claims от платформы — массив clientId?) - -## Мёртвый код (не критично) - -- `isSubnetOf()` в validators.js — определена, не используется, не экспортируется - -## Для прода - -Выставить в `.env`: -``` -NODE_ENV=production -JWKS_URL= -ADMIN_CLIENT_ID= -DEFAULT_LIMIT=15 -``` - - -## Репозитории - -| Репо | URL | Ветка | Локальный путь | -|---|---|---|---| -| Код приложения | `https://gitea.services.ngcloud.ru/Nail/ipwhitelist-app.git` | master | `/home/naeel/ipwhitelist-app` | -| Документация | `https://gitea.services.ngcloud.ru/Nail/IPWhiteList.git` | main | `/home/naeel/IPWhiteList` | - -## Стек - -Node.js + Express + EJS + PostgreSQL + pg pool + jsonwebtoken - -## Деплой - -- URL: `https://white.nodejsk8s.dev.nubes.ru` -- DEV_MODE=true (мок-аутентификация) -- ⚠️ Код запушен, но Nubes не передеплоил — крутится старая версия - -## БД - -- `write.bde8229b-1381-4330-b24b-727ad73fcb44.dev.nubes.ru` -- user: `super`, db: `ipwhitelist` -- Миграция от 2026-05-30 применена (CHECK, UNIQUE, индексы) - ---- - -## Что сделано (запушено) - -### 1. `src/validators.js` — исправлены 3 бага -- Запрещённые диапазоны: `isSubnetOf` → `overlaps` (обход через суперсеть `/22`) -- Маска: `parseInt('24abc')` глотал мусор → строгая проверка `/^\d{1,2}$/` -- Множественные слэши: `10.0.0.0/24/8` теперь отклоняется - -### 2. `src/queries.js` — транзакции + гонки -- `createEntry`, `updateEntry`, `deleteEntry` — внутри транзакции с `SELECT ... FOR UPDATE` -- `getOrCreateCompany` — атомарный `INSERT ... ON CONFLICT` -- `getLimit` — `!= null` вместо `||` (custom_limit=0 не игнорируется) -- `logAudit` — принимает клиента транзакции (пишется атомарно) -- `getExportCIDRs` — фильтр по `companyId` -- `deleteEntry` — `company_id` в WHERE - -### 3. `src/auth.js` — **новый.** Мок JWT-аутентификация -- Генерирует RSA-ключи при старте -- JWKS endpoint: `/.well-known/jwks.json` -- `verifyJWT(token)` — RS256, issuer: `mock-auth-api` -- `issueJWT(claims)` — выпускает токен с claims как в HAR (`ClientID`, `company_id`, `company_name`, `email`) -- Middleware: извлекает JWT из cookie (`jwt`) или `Authorization: Bearer` -- DEV_MODE: при `DEV_MODE=true && NODE_ENV!=production` — обход auth -- **Для прода:** выставить `JWKS_URL=https://auth-api.../jwks` → switches to external verification - -### 4. `views/login.ejs` — **новый.** Мок-страница входа -- Выбор из 3 пользователей (admin WZ01112, тест WZ01325, компания 2 WZ02001) -- В проде заменяется на редирект в Keycloak - -### 5. `server.js` -- `cookie-parser` для чтения JWT из cookie -- `/healthz` — выше auth (k8s probe) -- `/login` GET/POST — мок-логин -- `/logout` — чистит cookie -- `/export` — только для своей компании (с авторизацией) -- `req.query.error` читается -- `urlencoded({ limit: '32kb' })` - -### 6. `sql/schema.sql` -- UNIQUE INDEX на активный `(company_id, value_cidr)` WHERE deleted_at IS NULL -- CHECK на `value_cidr` формат -- CHECK на `audit_log.action IN ('CREATE','UPDATE','DELETE')` -- CHECK на `custom_limit IS NULL OR >= 0` -- FK: `ON DELETE RESTRICT` -- Составной индекс `(company_id, created_at DESC)` на audit_log -- Индекс `(company_id, created_at DESC)` на whitelist_entries - -### 7. `views/index.ejs` -- `pattern` + `maxlength="18"` + `title` на инпуте value - ---- - -## Ревью (`/home/naeel/IPWhiteList/research/`) - -| Файл | Что | -|---|---| -| `REVIEW-SUMMARY.md` | Сводка всех находок (11 критических, 21 средний) | -| `opus-review-validators.md` | 1 критичный + 2 средних | -| `opus-review-queries.md` | 3 гонки + audit + getLimit | -| `opus-review-server.md` | JWT без подписи, 401, CSRF, /export | -| `opus-review-schema.md` | UNIQUE, CIDR, FK, индексы | -| `opus-review-ejs.md` | CSRF, clickjacking, client-валидация | -| `auth-flow.md` | Анализ HAR: claims, цепочка auth-api | - ---- - -## Не сделано (production-hardening, не баги) - -- helmet (X-Frame-Options, CSP, HSTS) -- rate-limit на POST /add, /delete, /export -- CSRF-токены в формах -- JWT-верификация через внешний JWKS (нужен URL от девопсов) -- Admin-признак в токене (нужен пример токена админа) -- Multi-company (нужен формат claims от платформы) - -## Для прода - -Выставить в `.env`: -``` -NODE_ENV=production -JWKS_URL= -``` -Всё остальное работает без изменений. diff --git a/research/REVIEW-SUMMARY.md b/research/REVIEW-SUMMARY.md deleted file mode 100644 index 82774a5..0000000 --- a/research/REVIEW-SUMMARY.md +++ /dev/null @@ -1,134 +0,0 @@ -# Сводка код-ревью IP WhiteList (все 5 файлов) - -> Дата: 2026-05-30 - -## 🔴 Критические (блокеры прода) - -### 1. JWT без проверки подписи — server.js - -```js -const payload = JSON.parse(Buffer.from(auth.replace('Bearer ', '').split('.')[1], 'base64').toString()); -``` - -Декодирует payload без верификации подписи. Любой подделывает токен → любая компания. Нужна `jose.jwtVerify(token, JWKS, { issuer: 'auth-api' })`. - -### 2. Пустой req.user не возвращает 401 — server.js - -```js -} catch { req.user = {}; } -next(); -``` - -Невалидный токен → `req.user = {}` → `clientId = undefined` → анонимы делят NULL-компанию. Нужно `if (!req.user.clientId) return res.status(401).send(...)`. - -### 3. Race conditions (TOCTOU) — queries.js - -**Три гонки из-за отсутствия транзакций:** -- **Обход лимита:** два параллельных запроса читают `cnt=14`, оба вставляют → 16 записей -- **Дубли/пересечения:** проверка `existing` и `INSERT` не в транзакции -- **Дубли компаний:** `getOrCreateCompany` — два первых запроса новой компании оба не находят, оба INSERT - -**Фикс:** `BEGIN` → `SELECT ... FOR UPDATE` строки компании → проверки → INSERT/UPDATE → `logAudit(..., client)` → `COMMIT`. - -### 4. audit вне транзакции — queries.js - -`logAudit` использует глобальный `pool`, не клиент транзакции. При сбое: запись есть, аудита нет (или наоборот). Передавать клиент транзакции в `logAudit`. - -### 5. /export без авторизации — server.js - -`getExportCIDRs()` без аргументов отдаёт CIDR всех компаний без проверки `req.user`. Публичная утечка whitelist всех клиентов. - -### 6. DEV_MODE не привязан к NODE_ENV — server.js - -`DEV_MODE=true` в проде → все становятся `WZ01325` без auth. Добавить `&& process.env.NODE_ENV !== 'production'`. - -### 7. CSRF — server.js + index.ejs - -POST-формы `/add`, `/delete/:id` без CSRF-токенов. Нужен `csurf` + `` в формах. - -### 8. Обход запрещённых диапазонов через суперсеть — validators.js - -Блокировка использует `isSubnetOf(normalized, blocked)` — можно обойти `/22`, содержащей запрещённый `/24` (например `192.0.2.0/22` содержит TEST-NET-1). Заменить на `overlaps(normalized, blocked)`. - ---- - -## 🟠 Схема БД (structure) - -### 9. Нет UNIQUE на активный CIDR компании — schema.sql - -Дубли держатся только на коде. Добавить: - -```sql -CREATE UNIQUE INDEX uq_entries_active_cidr - ON whitelist_entries(company_id, value_cidr) WHERE deleted_at IS NULL; -``` - -### 10. value_cidr как VARCHAR — нет проверок в БД — schema.sql - -БД не валидирует формат и не ловит пересечения. Варианты: - -| Уровень | Что | -|---|---| -| Минимум | `CHECK (value_cidr ~ '^(\d{1,3}\.){3}\d{1,3}/\d{1,2}$')` | -| Production | Тип `CIDR` + exclusion constraint (btree_gist) для пересечений | - -### 11. FK без явного ON DELETE — schema.sql - -`REFERENCES companies(id)` без указания поведения. Явно задать `ON DELETE RESTRICT`. - ---- - -## 🟡 Средние (production-hardening) - -| # | Где | Что | -|---|---|---| -| 12 | server.js | Нет **helmet** — X-Frame-Options, CSP, HSTS отсутствуют | -| 13 | server.js | Нет **rate-limit** на `/add`, `/delete`, `/export` | -| 14 | server.js | `express.urlencoded` без `limit` — DoS большими телами | -| 15 | server.js | Нет глобального error-handler middleware | -| 16 | server.js | `/healthz` под auth — сломает k8s-пробу. Вынести выше | -| 17 | server.js | `?error=` в редиректе не читается в `res.render('/', ...)` — параметр молча теряется | -| 18 | server.js | Сырые `e.message` БД наружу — info leak. Маппить на дружелюбные сообщения | -| 19 | queries.js | `custom_limit = 0` игнорируется: `company.custom_limit \|\| defaultLimit` → `0 \|\| 15 = 15` | -| 20 | queries.js | `getOrCreateCompany` не атомарен (хотя UNIQUE на client_id спасает). Upsert: `INSERT ... ON CONFLICT` | -| 21 | queries.js | `deleteEntry` WHERE только по `id` — добавить `AND company_id = $3` для глубины защиты | -| 22 | schema.sql | `audit_log.company_id` без FK на companies (допустимо, но задокументировать) | -| 23 | schema.sql | `audit_log.action` — свободный VARCHAR. Добавить `CHECK (action IN ('CREATE','UPDATE','DELETE'))` | -| 24 | schema.sql | Индекс аудита только на company_id. Добавить `(company_id, created_at DESC)` | -| 25 | schema.sql | `updated_at` не обновляется автоматически. Добавить триггер | -| 26 | schema.sql | `SERIAL` → `GENERATED ALWAYS AS IDENTITY` (PG 10+) | -| 27 | schema.sql | `custom_limit` без CHECK ≥ 0 | -| 28 | validators.js | `parseInt('24abc') = 24` — глотает мусор. Проверять `^\d{1,2}$` | -| 29 | validators.js | Множественные слэши не отсекаются: `10.0.0.0/24/8` → средняя часть игнорируется | -| 30 | index.ejs | Нет client-валидации формата (ТЗ требует). Добавить `pattern` + `maxlength="18"` | -| 31 | index.ejs | `disabled` по лимиту обходится через DevTools — не баг, т.к. сервер проверяет | -| 32 | index.ejs | Инлайн-стили → `unsafe-inline` в CSP. Вынести в `.css` для строгой политики | - ---- - -## 🟢 Безопасно (проверено) - -- **SQL-инъекций нет** — все запросы параметризованы ($1, $2...) -- **XSS в EJS нет** — всё через `<%= %>`, `<%- %>` не используется -- **Изоляция компаний корректна** — `updateEntry`/`deleteEntry` проверяют `company_id`, `listEntries` фильтрует по компании -- **Сохранённого XSS через БД нет** — все поля экранируются -- **`overlaps()` формула корректна** — проверено 12 тестами, старая и новая формулы математически эквивалентны -- **Изоляция через схему БД** — записи привязаны к `company_id`, обход только через код (не схему) -- **Partial-индекс `idx_entries_active`** — правильный приём, soft-deleted не раздувают индекс - ---- - -## Приоритет исправлений - -| Порядок | Что | Блокирует | -|---|---|---| -| 1 | JWT — проверка подписи | Продакшен | -| 2 | Транзакции в createEntry/updateEntry/deleteEntry | Целостность данных | -| 3 | UNIQUE на активный CIDR в БД | Защита от гонок | -| 4 | 401 при пустом req.user | Auth | -| 5 | DEV_MODE → NODE_ENV | Безопасность прода | -| 6 | /export — авторизация | Утечка данных | -| 7 | CSRF-токены | Безопасность | -| 8 | `overlaps` вместо `isSubnetOf` в блокировке | Валидация | -| 9 | helmet + rate-limit | Production-hardening | -| 10 | Остальное (см. таблицу 🟡) | Качество | diff --git a/research/opus-full-review-2026-05-30.md b/research/opus-full-review-2026-05-30.md deleted file mode 100644 index 4168da4..0000000 --- a/research/opus-full-review-2026-05-30.md +++ /dev/null @@ -1,357 +0,0 @@ -# Full Code Review — IP WhiteList (Opus, 2026-05-30) - -> Ревью по коду из `prompt-opus-full-review-2026-05-30.md` (ветка `sonnet`). -> Легенда: ✅ хорошо · ⚠️ замечание · ❌ проблема (блокер/риск). - ---- - -## 0. Краткое резюме (TL;DR) - -Проект аккуратно структурирован: фабрики роутеров с DI, транзакции с `FOR UPDATE`, -параметризованные запросы, частичные уникальные индексы для защиты от гонок. -Базовая гигиена SQL/изоляции компаний — на хорошем уровне. - -Однако к продакшену проект **не готов**. Найдено несколько серьёзных проблем: - -| # | Проблема | Severity | -|---|----------|----------| -| 1 | `/export` смонтирован **до** auth-middleware → публичная выгрузка CIDR **всех** компаний | ❌ Критично | -| 2 | CSRF `getSessionIdentifier` читает несуществующую cookie `jwt` → токен не привязан к сессии | ❌ Критично | -| 3 | Нет `session.regenerate()` при логине → session fixation | ❌ Высокий | -| 4 | Open redirect через `returnTo` | ❌ Высокий | -| 5 | OIDC: нет проверки `issuer`/`audience`, нет matching по `kid`, JWKS не рефрешится и null в bearer-режиме | ❌ Высокий | -| 6 | `ssl: { rejectUnauthorized: false }` к БД | ⚠️/❌ | -| 7 | Дефолтные секреты (`SESSION_SECRET`, `CSRF_SECRET`) не fail-fast в проде | ⚠️ | -| 8 | CSP отключён (`contentSecurityPolicy: false`) | ⚠️ | -| 9 | Нет интеграционных тестов (auth, CSRF, IDOR, export) | ⚠️ | - ---- - -## 1. Безопасность (OWASP Top 10) - -### 1.1 ❌ Публичная выгрузка всех компаний через `/export` - -В `server.js` порядок монтирования: - -```js -app.use(require('./src/routes/export').createRouter({ q, exportLimiter, aggregateCIDRs })); -// ... -app.use(auth.middleware); // ← аутентификация ПОСЛЕ export -``` - -`/export` доступен **без аутентификации**, а внутри: - -```js -const cidrs = await q.getExportCIDRs(); // companyId = null → ВСЕ компании -const aggregated = aggregateCIDRs(cidrs); -``` - -`getExportCIDRs(null)` возвращает CIDR **всех** компаний, агрегированные вместе. -Любой анонимный пользователь получает полный список whitelisted-IP всех арендаторов. -Это нарушение изоляции данных (A01 Broken Access Control) и утечка информации (A01/A04). - -**Рекомендация:** одно из: -- перенести `/export` **после** `auth.middleware` и фильтровать по `req.user` (для админа — все/выбранная компания, для пользователя — только своя); -- либо, если выгрузка для оборудования должна быть машинной, защитить статическим bearer-токеном/mTLS и **никогда** не отдавать срез всех компаний без явной авторизации. - -### 1.2 ❌ CSRF: `getSessionIdentifier` привязан к мёртвой cookie - -```js -// src/middleware/csrf.js -getSessionIdentifier: (req) => req.cookies.jwt || '', -``` - -В коде есть честный комментарий, что после перехода на `express-session` cookie `jwt` -больше не выдаётся. Значит идентификатор сессии для **всех** пользователей = `''`. -Double-submit перестаёт быть привязан к конкретной сессии — токен валиден «глобально», -что ослабляет защиту (особенно с учётом session fixation ниже). - -**Рекомендация:** -```js -getSessionIdentifier: (req) => req.session?.id || req.sessionID || '', -``` -и убедиться, что `initCsrf()` вызывается после подключения `session` middleware (сейчас так и есть). - -### 1.3 ❌ Session fixation — нет регенерации сессии при логине - -В `routes/auth.js` (POST `/login`, `/callback`, `/dev-login`) сразу пишется -`req.session.user = ...` без `req.session.regenerate()`. Идентификатор сессии, -выданный до аутентификации, сохраняется — классический session fixation (A07). - -**Рекомендация:** перед установкой `user` вызывать: -```js -req.session.regenerate(err => { if (err) ...; req.session.user = user; req.session.save(() => res.redirect(...)); }); -``` - -### 1.4 ❌ Open redirect через `returnTo` - -```js -// POST /login -res.redirect(req.query.returnTo || '/'); -// /callback -const returnTo = req.session.returnTo || '/'; -res.redirect(returnTo); -``` - -`returnTo` приходит из запроса и не валидируется. Значение вида `//evil.com` -или `https://evil.com` приведёт к открытому редиректу (A01, фишинг). - -**Рекомендация:** разрешать только локальные пути: -```js -function safeReturn(t) { - return (typeof t === 'string' && t.startsWith('/') && !t.startsWith('//')) ? t : '/'; -} -``` - -### 1.5 ❌ OIDC verification — недостаточная проверка токена - -```js -function verifyOidcToken(token) { - const key = cachedJwks.keys.find(k => k.kty === 'RSA' && k.use === 'sig'); // ← не по kid - ... - return jwt.verify(token, pem, { algorithms: ['RS256'] }); // ← нет issuer/audience -} -``` - -Проблемы: -- **Нет проверки `issuer` и `audience`.** Любой RS256-токен, подписанный ключом из этого JWKS - (например, токен, выданный другому клиенту того же realm), пройдёт проверку → privilege/tenant confusion. -- **Выбор ключа не по `kid`** из заголовка токена, а «первый RSA sig». При ротации/нескольких - ключах возможны как ложные отказы, так и приём не того ключа. -- **`cachedJwks` загружается только в `exchangeCode`** и больше не рефрешится. В bearer-режиме - (запрос с `Authorization: Bearer` без предварительного `/callback`) `cachedJwks === null` → - `verifyOidcToken` бросит `JWKS not loaded yet`. При ротации ключей в KC — отказы до рестарта. - -**Рекомендация:** грузить JWKS при старте и кэшировать с TTL/refresh по `kid`; в `jwt.verify` -передавать `{ issuer: KC_ISSUER, audience: KC_CLIENT_ID, algorithms: ['RS256'] }`; выбирать ключ -по `kid` из декодированного заголовка. - -### 1.6 ⚠️ TLS к БД отключает проверку сертификата - -```js -ssl: process.env.DB_SSLMODE === 'require' ? { rejectUnauthorized: false } : false, -``` - -`rejectUnauthorized: false` = шифрование без аутентификации сервера → MITM возможен (A02/A05). -Имя `require` обманчиво: это поведение `sslmode=require` в libpq, но для прод-окружения -нужен `verify-full` с CA. - -**Рекомендация:** добавить режим с CA: `{ ca: fs.readFileSync(DB_CA), rejectUnauthorized: true }`. - -### 1.7 ⚠️ Дефолтные секреты не приводят к отказу в проде - -```js -secret: process.env.SESSION_SECRET || 'dev-session-secret-change-me', -getSecret: () => process.env.CSRF_SECRET || 'dev-csrf-secret-change-in-prod', -``` - -Если переменные не заданы в проде — приложение молча стартует со слабыми предсказуемыми -секретами (A02/A05). Подделка сессионных cookie/CSRF становится тривиальной. - -**Рекомендация:** при `NODE_ENV === 'production'` — fail-fast, если секреты не заданы/равны дефолту. - -### 1.8 ⚠️ CSP отключён - -```js -app.use(helmet({ contentSecurityPolicy: false })); -``` - -Отключённая CSP убирает важный слой защиты от XSS (A03). EJS-шаблоны в промпте не приведены — -**нельзя подтвердить**, что пользовательский ввод (`comment`, `companyName`, сообщения `error`/`message` -из query) экранируется через `<%= %>`, а не `<%- %>`. `error`/`message` берутся прямо из `req.query` -и рендерятся — при `<%- %>` это reflected XSS. - -**Рекомендация:** включить разумную CSP; проверить, что все вывод-точки используют экранирование `<%= %>`. - -### 1.9 ⚠️ `dev-login` — риск в проде - -`/dev-login` при `DEV_MODE=true` (или заданном `DEV_SECRET`) позволяет войти под любым -пользователем, включая `isAdmin: on`, без пароля. Если `DEV_MODE` случайно окажется `true` в проде — -полный обход аутентификации. - -**Рекомендация:** жёстко запретить `DEV_MODE` при `NODE_ENV=production` (отказ старта), -а не полагаться на конфигурацию окружения. - -### 1.10 ⚠️ Нет rate-limit на логин - -POST `/login`, `/dev-login`, `/callback` не покрыты лимитером — для mock некритично, -но при реальном OIDC `/callback` без лимита может использоваться для нагрузки на токен-эндпоинт KC. - -### 1.11 ✅ Что сделано хорошо - -- **SQL injection** — все запросы параметризованы (`$1, $2, ...`), конкатенации пользовательского - ввода в SQL нет. ✅ -- **IDOR / изоляция компаний** — обычный пользователь не может передать `company_id`; для него всегда - `getOrCreateCompany(clientId, ...)` по его собственному `clientId` из токена. Все мутации (`createEntry`, - `updateEntry`, `deleteEntry`) фильтруют по `company_id`, а `getCompanyById` доступен только в админ-ветке. ✅ -- **CSRF-обработчик** ошибок (`EBADCSRFTOKEN`) даёт понятный 403. ✅ -- Cookie-флаги `httpOnly`, `secure` (в проде), `sameSite: 'lax'`. ✅ - ---- - -## 2. Корректность бизнес-логики, транзакции, конкурентность - -### 2.1 ✅ Гонки при добавлении/лимиты - -`createEntry` берёт `SELECT ... FOR UPDATE` по строке компании, затем считает count и -проверяет пересечения внутри одной транзакции. Это сериализует параллельные вставки в рамках -одной компании. Плюс частичный уникальный индекс `uq_entries_active_cidr` страхует от дублей -на уровне БД. Хорошая многоуровневая защита. ✅ - -### 2.2 ⚠️ Проверка пересечений O(n) перебором в приложении - -`createEntry`/`updateEntry` загружают все активные CIDR и сравнивают через `overlaps` в JS. -При лимите ~15 записей это незаметно, но логика дублируется и проверка пересечений невозможна -на уровне БД (индекс ловит только точный дубль, не overlap). Для текущих лимитов — приемлемо. ⚠️ - -### 2.3 ⚠️ `updateEntry`: `comment || old.comment` - -Пустая строка комментария (`''`) трактуется как «не менять» и возвращает старый комментарий — -пользователь не сможет очистить комментарий. Edge case. ⚠️ - -### 2.4 ⚠️ `value_cidr VARCHAR(18)` и regex в CHECK - -Схема ограничивает `/\d{1,2}/` для маски, но приложение разрешает только `/22`–`/32` — -согласовано. Однако CHECK-regex в БД допускает невалидные октеты (`999.999.999.999/40`), -полагаясь полностью на валидацию приложения. Дубль-валидация на уровне БД неполная. ⚠️ - -### 2.5 ⚠️ `companyId` (UUID) из токена фактически не используется - -Для обычного пользователя доступ к данным идёт по `companies.id` (SERIAL), полученному из -`getOrCreateCompany(clientId)`. UUID `company_id` из токена в выборках не участвует. Это не баг -(изоляция по `clientId` корректна), но источник путаницы: два разных идентификатора компании. ⚠️ - -### 2.6 ✅ Аудит в той же транзакции - -`logAudit(..., client)` выполняется внутри транзакции мутации — запись аудита атомарна -с изменением. ✅ - ---- - -## 3. CIDR-валидация и агрегация (`validators.js`) - -### 3.1 ✅ `validate()` - -- IPv6 отбрасывается, проверка формата, нормализация к адресу сети, проверка против - `BLOCKED_RANGES`. Логика корректна для /22–/32. -- Битовые операции `(acc << 8) + parseInt(...)` дают знаковое 32-битное промежуточное значение, - но финальный `>>> 0` приводит к беззнаковому — для рассматриваемых масок результат верный. ✅ - -### 3.2 ⚠️ `overlaps()` — корректно, но нечитаемо - -```js -return a.start <= b.end && b.start <= a.start || - b.start <= a.end && a.start <= b.start; -``` - -Сводится к «начало одного интервала лежит внутри другого» — это **корректный** критерий -пересечения двух интервалов (проверено на граничных случаях: вложенность, смежность, непересечение). -Но запись через смешанные `&&`/`||` без скобок хрупкая и трудна для ревью. - -**Рекомендация:** заменить на каноническое `a.start <= b.end && b.start <= a.end`. - -### 3.3 ⚠️ Список `BLOCKED_RANGES` неполон - -Заблокированы RFC1918/CGNAT/loopback/link-local/multicast/reserved, но **не** `0.0.0.0/8` -(«this network»). Можно добавить, например, `0.0.0.0/22`. Маловажно, но для строгого whitelist стоит закрыть. - -### 3.4 ✅ `aggregateCIDRs()` / `rangeToCIDRs()` - -- Сортировка по `start`, слияние перекрывающихся и **смежных** диапазонов (с защитой от переполнения - `last.end < 0xFFFFFFFF`), затем разбиение объединённого диапазона на минимальный набор выровненных CIDR. -- `rangeToCIDRs` корректно выбирает наибольший выровненный блок (`trailingZeros`) и уменьшает префикс, - пока блок не помещается в диапазон; курсор всегда продвигается → бесконечного цикла нет, граница - `0xFFFFFFFF` обработана. ✅ - -Алгоритмически — самая сильная часть проекта. - ---- - -## 4. Архитектура и качество кода - -### 4.1 ✅ Сильные стороны - -- **Фабрики роутеров с DI** (`createRouter({...})`) — тестируемо, явные зависимости, без скрытых импортов состояния. -- **Разделение слоёв**: `db` / `queries` / `validators` / `routes` / `middleware`. -- **Транзакции** с корректным `BEGIN/COMMIT/ROLLBACK` и `finally { client.release() }`. -- **Auth-абстракция** поддерживает и mock-RS256, и реальный OIDC за единым интерфейсом. - -### 4.2 ⚠️ Замечания - -- **Дублирование** обработки `company_id` в трёх хендлерах `entries.js` (add/edit/delete) — почти - идентичный блок «определить компанию». Можно вынести в helper-middleware `resolveCompany`. -- **Обработка ошибок через redirect c `error` в query** удобна для UI, но смешивает 4xx-валидацию - и 5xx-сбои БД (любая ошибка `createEntry` уезжает в `?error=...`). Стоит различать пользовательские - ошибки и системные (логировать stack для последних). -- `cachedJwks` / `mockKeyPair` — модульное состояние; для горизонтального масштабирования mock-JWKS - у каждого инстанса свой ключ → токены не валидны между подами. Для mock-режима ок, но в проде - mock использоваться не должен. -- **`express-session` MemoryStore** (стор не задан) — утечки памяти и потеря сессий при рестарте/масштабировании. - Для прода нужен внешний стор (Redis/PG). ⚠️ (фактически блокер прода) -- Комментарии-TODO прямо в коде (`csrf.js`, `db.js`) — хорошо, что зафиксированы, но это незакрытый долг. - ---- - -## 5. Тесты - -Из промпта видно ~50 юнит-тестов без БД: загрузка модулей, `config`, `validators` (30+ кейсов), -`auth` (session middleware, `requireAdmin`). - -### ✅ Покрыто -- Валидаторы CIDR / агрегация / overlaps — основной риск-домен покрыт хорошо. -- Session-middleware happy path и `requireAdmin`. - -### ❌ Не покрыто (критично добавить) -1. **Публичность `/export`** — тест, что неаутентифицированный запрос **не** получает данные - (после фикса 1.1). Сейчас регрессия не отлавливается. -2. **CSRF** — отклонение запроса без/с чужим токеном; привязка токена к сессии (фикс 1.2). -3. **IDOR** — обычный пользователь пытается передать `company_id` чужой компании в add/edit/delete → - должен работать только со своей. -4. **Изоляция в `queries`** — пользователь A не видит/не меняет записи компании B. -5. **Session fixation** — id сессии меняется после логина (фикс 1.3). -6. **Open redirect** — `returnTo=//evil.com` не приводит к внешнему редиректу (фикс 1.4). -7. **Лимиты/гонки** — параллельные `createEntry` не превышают лимит (интеграционный, с БД). -8. **OIDC verify** — отклонение токена с чужим `iss`/`aud`, выбор ключа по `kid` (фикс 1.5). - -Сейчас нет интеграционных тестов с БД и HTTP-слоем — основной пробел. - ---- - -## 6. Готовность к продакшену — чек-лист блокеров - -- [ ] ❌ Закрыть `/export` аутентификацией + фильтрацией по компании. -- [ ] ❌ Починить CSRF `getSessionIdentifier` (`req.session.id`). -- [ ] ❌ `session.regenerate()` при логине (fixation). -- [ ] ❌ Валидация `returnTo` (open redirect). -- [ ] ❌ OIDC: `issuer`/`audience`/`kid` + рефреш JWKS. -- [ ] ❌ Внешний session store (Redis/PG) вместо MemoryStore. -- [ ] ⚠️ TLS к БД с проверкой CA (`verify-full`). -- [ ] ⚠️ Fail-fast при дефолтных секретах в проде. -- [ ] ⚠️ Запретить `DEV_MODE`/`/dev-login` в проде. -- [ ] ⚠️ Включить CSP; подтвердить экранирование EJS (`<%= %>`). -- [ ] ⚠️ Добавить интеграционные тесты (export/CSRF/IDOR/fixation/OIDC). - ---- - -## 7. Итоговая оценка по блокам - -| Блок | Оценка | -|------|--------| -| SQL injection / параметризация | ✅ | -| Изоляция компаний (IDOR в роутах) | ✅ (но без тестов) | -| `/export` доступ | ❌ | -| CSRF-конфигурация | ❌ | -| Session-управление (fixation, store) | ❌ | -| OIDC / token verification | ❌ | -| Open redirect | ❌ | -| TLS к БД / секреты | ⚠️ | -| CSP / XSS (не подтверждено по views) | ⚠️ | -| Бизнес-логика / транзакции / гонки | ✅ | -| CIDR-валидация и агрегация | ✅ | -| Архитектура / DI | ✅ | -| Покрытие тестами | ⚠️ | - -**Вывод:** ядро (валидация, агрегация, транзакции, изоляция в запросах) сделано грамотно. -Блокируют прод в первую очередь четыре вещи: публичный `/export`, сломанная привязка CSRF, -session fixation + MemoryStore и неполная проверка OIDC-токенов. После их устранения и добавления -интеграционных тестов проект можно выводить в эксплуатацию. diff --git a/research/opus-review-ejs.md b/research/opus-review-ejs.md deleted file mode 100644 index 4d94af2..0000000 --- a/research/opus-review-ejs.md +++ /dev/null @@ -1,97 +0,0 @@ -# Код-ревью index.ejs — XSS, CSRF, clickjacking, client-валидация - -> Дата: 2026-05-30 - -## 🟢 XSS — экранирование корректно - -Весь динамический вывод идёт через `<%= %>`, который EJS экранирует (`&<>"'`). `<%- %>` не используется нигде. Векторы проверены: -- `<%= message %>`, `<%= error %>` — экранируются. Даже если в `error` попадёт сырая ошибка БД с `