Files

T

2026-05-30 15:06:51 +03:00

23 KiB

Raw Permalink Blame History

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 порядок монтирования:

app.use(require('./src/routes/export').createRouter({ q, exportLimiter, aggregateCIDRs }));
// ...
app.use(auth.middleware);   // ← аутентификация ПОСЛЕ export

/export доступен без аутентификации, а внутри:

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

// src/middleware/csrf.js
getSessionIdentifier: (req) => req.cookies.jwt || '',

В коде есть честный комментарий, что после перехода на express-session cookie jwt больше не выдаётся. Значит идентификатор сессии для всех пользователей = ''. Double-submit перестаёт быть привязан к конкретной сессии — токен валиден «глобально», что ослабляет защиту (особенно с учётом session fixation ниже).

Рекомендация:

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 вызывать:

req.session.regenerate(err => { if (err) ...; req.session.user = user; req.session.save(() => res.redirect(...)); });

1.4 ❌ Open redirect через `returnTo`

// POST /login
res.redirect(req.query.returnTo || '/');
// /callback
const returnTo = req.session.returnTo || '/';
res.redirect(returnTo);

returnTo приходит из запроса и не валидируется. Значение вида //evil.com или https://evil.com приведёт к открытому редиректу (A01, фишинг).

Рекомендация: разрешать только локальные пути:

function safeReturn(t) {
  return (typeof t === 'string' && t.startsWith('/') && !t.startsWith('//')) ? t : '/';
}

1.5 ❌ OIDC verification — недостаточная проверка токена

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 к БД отключает проверку сертификата

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 ⚠️ Дефолтные секреты не приводят к отказу в проде

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 отключён

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()` — корректно, но нечитаемо

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.

❌ Не покрыто (критично добавить)

Публичность /export — тест, что неаутентифицированный запрос не получает данные (после фикса 1.1). Сейчас регрессия не отлавливается.
CSRF — отклонение запроса без/с чужим токеном; привязка токена к сессии (фикс 1.2).
IDOR — обычный пользователь пытается передать company_id чужой компании в add/edit/delete → должен работать только со своей.
Изоляция в queries — пользователь A не видит/не меняет записи компании B.
Session fixation — id сессии меняется после логина (фикс 1.3).
Open redirect — returnTo=//evil.com не приводит к внешнему редиректу (фикс 1.4).
Лимиты/гонки — параллельные createEntry не превышают лимит (интеграционный, с БД).
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-токенов. После их устранения и добавления интеграционных тестов проект можно выводить в эксплуатацию.

23 KiB Raw Permalink Blame History Unescape Escape