ipwhitelist-app/research/opus-full-review-2026-05-30.md

# 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-токенов. После их устранения и добавления
интеграционных тестов проект можно выводить в эксплуатацию.