From e9bfe6dae6832c32badc4cff9ff3fe7c81844eb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9CNaeel=E2=80=9D?= Date: Sat, 30 May 2026 15:06:51 +0300 Subject: [PATCH] =?UTF-8?q?=D0=BA=D0=BE=D0=B4=20=D1=80=D0=B5=D0=B2=D1=8C?= =?UTF-8?q?=D1=8E=20=D0=BE=D1=82=20=D0=9E=D0=BF=D1=83=D1=81=204.8?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- research/opus-full-review-2026-05-30.md | 357 ++++++++++++++++++++++++ 1 file changed, 357 insertions(+) create mode 100644 research/opus-full-review-2026-05-30.md diff --git a/research/opus-full-review-2026-05-30.md b/research/opus-full-review-2026-05-30.md new file mode 100644 index 0000000..4168da4 --- /dev/null +++ b/research/opus-full-review-2026-05-30.md @@ -0,0 +1,357 @@ +# 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-токенов. После их устранения и добавления +интеграционных тестов проект можно выводить в эксплуатацию.