код ревью от Опус 4.8
This commit is contained in:
@@ -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-токенов. После их устранения и добавления
|
||||
интеграционных тестов проект можно выводить в эксплуатацию.
|
||||
Reference in New Issue
Block a user