Nail/IPWhiteList
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

doc: сводка ревью, STATE.md, тесты, HAR, WhiteIPlist

Browse Source
This commit is contained in:
“Naeel”
2026-05-30 08:47:19 +03:00
parent b238124d2d
commit 3b46ad1a08
9 changed files with 31981 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
STATE.md
+109
View File
@@ -0,0 +1,109 @@
# Состояние проекта на 2026-05-30 (коммит `2fb4548`)
## Репозитории
| Репо | 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=<auth-api JWKS URL>
```
Всё остальное работает без изменений.
WhiteIPlist.txt → docs/WhiteIPlist.txt
View File
nubes_login.har
Executable
+31257
View File
File diff suppressed because one or more lines are too long
research/REVIEW-SUMMARY.md
+134
View File
@@ -0,0 +1,134 @@
# Сводка код-ревью 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` + `<input name="_csrf">` в формах.
### 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 | Остальное (см. таблицу 🟡) | Качество |
research/opus-review-ejs.md
+97
View File
@@ -0,0 +1,97 @@
# Код-ревью index.ejs — XSS, CSRF, clickjacking, client-валидация
> Дата: 2026-05-30
## 🟢 XSS — экранирование корректно
Весь динамический вывод идёт через `<%= %>`, который EJS экранирует (`&<>"'`). `<%- %>` не используется нигде. Векторы проверены:
- `<%= message %>`, `<%= error %>` — экранируются. Даже если в `error` попадёт сырая ошибка БД с `<script>`, она будет обезврежена.
- `<%= e.value_cidr %>`, `<%= e.comment %>`, `<%= e.created_by %>` (данные из БД) — экранируются.
- `<%= user.clientId %>`, `<%= user.email %>` — экранируются.
Сохранённого XSS через комментарий/email нет. Это сильная сторона шаблона.
⚠️ Единственный нюанс: `action="/delete/<%= e.id %>"` — `e.id` идёт в атрибут URL. Так как это integer из БД (SERIAL), инъекция невозможна. Но если тип когда-нибудь станет строковым — атрибутный контекст потребует особой осторожности. Сейчас безопасно.
## 🔴 CSRF — формы без токена (критично)
```html
<form method="POST" action="/add">
<form method="POST" action="/delete/<%= e.id %>" ...>
```
Ни одна форма не содержит CSRF-токена. Обе меняют состояние. Сторонний сайт может авто-сабмитить POST на `/add`/`/delete/:id`. Зеркалит находку из ревью server.js. Фикс — пробросить токен из middleware (`csurf`) и в каждой форме:
```html
<form method="POST" action="/add">
<input type="hidden" name="_csrf" value="<%= csrfToken %>">
...
</form>
<form method="POST" action="/delete/<%= e.id %>" style="display:inline" onsubmit="return confirm('Удалить запись?')">
<input type="hidden" name="_csrf" value="<%= csrfToken %>">
<button class="btn btn-danger">Удалить</button>
</form>
```
(Требует прокидывания `csrfToken` в `res.render` во всех роутах server.js.)
## 🔴 Clickjacking — нет защиты фрейминга
Шаблон с кнопками «Удалить» можно встроить в `<iframe>` на фишинговом сайте и подложить под клик (UI redress). В самом EJS защиты нет — нужны заголовки на стороне server.js (`helmet` → `X-Frame-Options: DENY` / CSP `frame-ancestors 'none'`). Дублирует находку из ревью server.js. На уровне шаблона можно добавить CSP через meta (слабее заголовка, но лучше чем ничего):
```html
<meta http-equiv="Content-Security-Policy" content="frame-ancestors 'none'; default-src 'self'; style-src 'self' 'unsafe-inline'">
```
⚠️ `style-src 'unsafe-inline'` потребуется из-за инлайн-`<style>` и inline-атрибутов `style="..."` в header/кнопках — это ослабляет CSP. По-хорошему вынести стили в отдельный `.css` файл и убрать `unsafe-inline`.
## 🟡 disabled-поля обходятся через DevTools (это и есть главная дыра валидации)
```html
<input name="value" ... <%= used >= limit ? 'disabled' : '' %>>
<button ... <%= used >= limit ? 'disabled' : '' %>>
```
`disabled` — только UX. Атакующий через DevTools снимает атрибут и шлёт POST `/add` сверх лимита. ЭТО НЕ УЯЗВИМОСТЬ ШАБЛОНА, пока сервер проверяет лимит — а он проверяет (`createEntry`). Вывод: клиентский `disabled` не является защитой и не должен ею считаться; настоящая защита — серверная проверка лимита (она есть, но уязвима к гонке — см. ревью queries.js). Шаблон тут корректен ровно при условии серверной проверки.
## 🟡 Нет client-side валидации (несоответствие ТЗ)
ТЗ требует клиентскую валидацию формата IPv4/CIDR. Сейчас только `required` и серверная проверка. Пользователь узнаёт об ошибке только после round-trip. Добавить `pattern` для базовой проверки + JS для маски /22–/32:
```html
<input name="value"
pattern="^(\d{1,3}\.){3}\d{1,3}(/\d{1,2})?$"
title="IPv4 или CIDR, например 203.0.113.0/24"
placeholder="Например: 203.0.113.10 или 203.0.113.0/24"
required <%= used >= limit ? 'disabled' : '' %>>
```
`pattern` — только формат; диапазон маски (/22–/32) и host-биты всё равно валидирует сервер (`validators.js`). Это UX-улучшение, не замена серверной проверки.
## 🟡 maxlength только на comment, не на value
`comment` имеет `maxlength="255"` (совпадает со схемой VARCHAR(255) — хорошо). У `value` нет `maxlength` — стоит добавить `maxlength="18"` под `VARCHAR(18)`, чтобы не слать заведомо длинное и для согласованности.
## 🟢 Утечка чужих данных — нет
В шаблоне выводятся только `user.clientId`/`user.email` (свои) и `entries` (своей компании, отфильтрованы по company_id в `listEntries`). Данных других компаний нет. Изоляция на уровне шаблона соблюдена (зависит от корректной фильтрации в queries.js — там она есть).
## 🟡 onsubmit confirm — не защита, но ок
`onsubmit="return confirm(...)"` легко обходится, но это UX-подтверждение, не security-контроль. Приемлемо.
## 🟡 favicon/иконка — внешних ресурсов нет
Все ресурсы локальные (`/favicon.png`, инлайн SVG, инлайн CSS). Нет внешних CDN → меньше поверхность для supply-chain. Хорошо. Обратная сторона — инлайн-стили мешают строгой CSP (см. выше).
---
**Итог:**
1. 🟢 XSS нет — всё через `<%= %>`, `<%- %>` не используется. Главная сильная сторона.
2. 🔴 CSRF-токенов в формах нет — добавить `_csrf` в `/add` и `/delete` (+ middleware в server.js).
3. 🔴 Clickjacking — защита только заголовками (helmet в server.js); опционально CSP-meta.
4. 🟡 `disabled` по лимиту обходится через DevTools — не баг шаблона при условии серверной проверки (она есть).
5. 🟡 Нет client-валидации формата (ТЗ требует) — добавить `pattern` + `maxlength` на `value`.
6. 🟡 Инлайн-стили вынудят `unsafe-inline` в CSP — вынести в отдельный .css для строгой политики.
Шаблон по XSS написан правильно; основные пробелы — CSRF и clickjacking (закрываются в server.js) и отсутствие клиентской валидации из ТЗ.
research/opus-review-queries.md
+133
View File
@@ -0,0 +1,133 @@
# Код-ревью queries.js — гонки, транзакции, безопасность
> Дата: 2026-05-30
## 🔴 Race condition 1 — обход лимита (TOCTOU, критично)
`createEntry`: между `SELECT COUNT(*)` (проверка лимита) и `INSERT` нет транзакции и блокировки. Два параллельных запроса от одной компании оба прочитают `cnt = 14`, оба пройдут проверку `cnt >= 15`, оба вставят запись → 16 записей при лимите 15. То же самое позволяет вставить две пересекающиеся/дублирующие записи одновременно (проверка `existing` тоже вне транзакции).
Фикс — обернуть всю операцию в транзакцию с блокировкой строки компании (`SELECT ... FOR UPDATE` сериализует параллельные вставки в рамках одной компании):
```js
async function createEntry(companyId, rawValue, comment, userEmail) {
const { cidr, wasNormalized } = validate(rawValue);
const client = await pool.connect();
try {
await client.query('BEGIN');
// блокируем строку компании — параллельные createEntry этой компании встают в очередь
const company = (await client.query(
'SELECT * FROM companies WHERE id = $1 FOR UPDATE', [companyId]
)).rows[0];
if (!company) throw new Error('Компания не найдена');
const limit = await getLimit(company);
const cnt = (await client.query(
'SELECT COUNT(*)::int AS c FROM whitelist_entries WHERE company_id = $1 AND deleted_at IS NULL',
[companyId]
)).rows[0].c;
if (cnt >= limit) throw new Error(`Лимит исчерпан: ${cnt} из ${limit}`);
const existing = (await client.query(
'SELECT value_cidr FROM whitelist_entries WHERE company_id = $1 AND deleted_at IS NULL',
[companyId]
)).rows;
for (const row of existing) {
if (row.value_cidr === cidr) throw new Error('Такой адрес уже существует');
if (overlaps(cidr, row.value_cidr))
throw new Error(`Пересечение с существующей записью ${row.value_cidr}`);
}
const res = await client.query(
`INSERT INTO whitelist_entries (company_id, value_cidr, comment, created_by)
VALUES ($1, $2, $3, $4) RETURNING *`,
[companyId, cidr, comment || null, userEmail]
);
await logAudit(userEmail, companyId, 'CREATE', null, cidr, res.rows[0].id, client);
await client.query('COMMIT');
return { entry: res.rows[0], wasNormalized };
} catch (e) {
await client.query('ROLLBACK');
throw e;
} finally {
client.release();
}
}
```
## 🔴 Race condition 2 — то же в updateEntry
`updateEntry` имеет идентичную проблему: проверка пересечений (`existing`) и `UPDATE` не в транзакции. Параллельное обновление двух записей в пересекающиеся CIDR пройдёт обе проверки. Обернуть так же: `BEGIN` → `SELECT ... FOR UPDATE` строки компании → проверки → `UPDATE` → `logAudit(...,client)` → `COMMIT`/`ROLLBACK`.
## 🔴 Race condition 3 — getOrCreateCompany (дубли компаний)
`getOrCreateCompany`: между `SELECT` и `INSERT` нет защиты. Два первых запроса новой компании оба не найдут строку и оба сделают `INSERT`. Спасает только `UNIQUE` на `client_id` в схеме (второй упадёт), но ошибка вылетит наружу некрасиво. Фикс — атомарный upsert:
```js
async function getOrCreateCompany(clientId, companyName) {
const res = await pool.query(
`INSERT INTO companies (client_id, name) VALUES ($1, $2)
ON CONFLICT (client_id) DO UPDATE SET name = COALESCE(companies.name, EXCLUDED.name)
RETURNING *`,
[clientId, companyName || clientId]
);
return res.rows[0];
}
```
## 🟡 audit_log пишется вне транзакции
`logAudit` использует глобальный `pool`, а не клиента транзакции. Если INSERT записи прошёл, а logAudit упал — запись есть, аудита нет (или наоборот при будущих изменениях). Аудит обязателен по ТЗ. Передавать клиента транзакции:
```js
async function logAudit(userEmail, companyId, action, oldValue, newValue, entryId, db = pool) {
await db.query(
`INSERT INTO audit_log (user_email, company_id, action, old_value, new_value, entry_id)
VALUES ($1, $2, $3, $4, $5, $6)`,
[userEmail, companyId, action, oldValue, newValue, entryId || null]
);
}
```
## 🟡 deleteEntry — UPDATE без company_id в WHERE
```js
await pool.query(
'UPDATE whitelist_entries SET deleted_by = $1, deleted_at = NOW() WHERE id = $2',
[userEmail, entryId]
);
```
`old` уже проверен по `company_id`, поэтому изоляция сейчас не нарушается. Но WHERE по одному `id` хрупкий — при рефакторинге легко потерять привязку. Дублировать company_id в WHERE для глубины защиты:
```js
await pool.query(
'UPDATE whitelist_entries SET deleted_by = $1, deleted_at = NOW() WHERE id = $2 AND company_id = $3',
[userEmail, entryId, companyId]
);
```
Также deleteEntry не в транзакции с logAudit — обернуть аналогично create/update.
## 🟡 getLimit — custom_limit = 0 игнорируется
```js
return company.custom_limit || defaultLimit;
```
Если админ задал `custom_limit = 0` (запретить компании добавлять), `0 || 15` вернёт 15. ТЗ разрешает снижать лимит. Фикс:
```js
return company.custom_limit != null ? company.custom_limit : defaultLimit;
```
## 🟢 SQL-инъекций нет
Все запросы параметризованы ($1, $2...). Конкатенации с пользовательским вводом нет. `listEntries`/`getAudit` строят SQL из булевых флагов, не из ввода — безопасно.
## 🟢 Изоляция по company_id
`updateEntry` и `deleteEntry` проверяют `company_id` при выборке `old` — пользователь компании А не затронет записи компании Б. Корректно (но см. замечание по deleteEntry WHERE).
---
**Итог:** SQL-инъекций и утечек между компаниями нет. Главная проблема — отсутствие транзакций: 3 эксплуатируемые гонки (обход лимита, дубли/пересечения, дубли компаний) + риск рассинхрона аудита. Все чинятся обёрткой в транзакцию с `FOR UPDATE` и передачей клиента в logAudit. Плюс мелкий баг с `custom_limit = 0`.
research/opus-review-schema.md
+141
View File
@@ -0,0 +1,141 @@
# Код-ревью schema.sql — индексы, constraint'ы, FK, типы
> Дата: 2026-05-30
## 🔴 Нет уникального constraint на активный CIDR компании
Дубликаты предотвращаются только в коде (`createEntry`), а это уязвимо к гонке (см. ревью queries.js — TOCTOU). БД должна гарантировать уникальность активного адреса в рамках компании независимо от кода:
```sql
CREATE UNIQUE INDEX IF NOT EXISTS uq_entries_active_cidr
ON whitelist_entries(company_id, value_cidr) WHERE deleted_at IS NULL;
```
Это превращает существующий `idx_entries_active` в уникальный (можно заменить им) — параллельные INSERT одинакового CIDR упадут на втором, гонка закрывается на уровне БД. Пересечения (overlaps) так не закрыть — для них нужен `inet`/GiST (см. ниже) или транзакция.
## 🔴 value_cidr хранится как VARCHAR — нет валидации и пересечений на уровне БД
```sql
value_cidr VARCHAR(18) NOT NULL,
```
`VARCHAR(18)` хранит произвольную строку — БД не проверяет, что это валидный CIDR, и не умеет искать пересечения. Production-вариант — нативный тип `cidr`:
```sql
value_cidr CIDR NOT NULL,
```
Преимущества: БД отвергает мусор; операторы `&&` (overlaps), `<<=` (subnet); можно сделать exclusion constraint на пересечения внутри компании:
```sql
CREATE EXTENSION IF NOT EXISTS btree_gist;
ALTER TABLE whitelist_entries
ADD CONSTRAINT excl_entries_overlap
EXCLUDE USING gist (company_id WITH =, value_cidr inet_ops WITH &&)
WHERE (deleted_at IS NULL);
```
Это закрывает гонку пересечений (RC №2 из ревью queries.js) на уровне БД. Если тип менять не хотите — оставить VARCHAR, но тогда уникальность/пересечения держатся только на транзакциях в коде. Минимум — CHECK на формат:
```sql
ALTER TABLE whitelist_entries
ADD CONSTRAINT chk_cidr_format CHECK (value_cidr ~ '^(\d{1,3}\.){3}\d{1,3}/\d{1,2}$');
```
## 🔴 FK без ON DELETE / нет каскада
```sql
company_id INTEGER NOT NULL REFERENCES companies(id),
```
Поведение по умолчанию — `NO ACTION`: удалить компанию нельзя, пока есть записи. Для сервиса с soft-delete это, скорее, правильно (компании не удаляются физически). Но это надо сделать осознанно: явно указать `ON DELETE RESTRICT` (документирует намерение) либо `ON DELETE CASCADE`, если компании реально удаляются. Сейчас умолчание неявное.
## 🟡 audit_log.company_id без FK и без типизации действий
```sql
company_id INTEGER NOT NULL,
action VARCHAR(32) NOT NULL,
```
`company_id` в audit_log не ссылается на `companies` — допустимо (аудит должен переживать удаление компании), но тогда стоит это зафиксировать комментарием. `action` — свободный VARCHAR, можно записать что угодно. Ограничить:
```sql
ALTER TABLE audit_log
ADD CONSTRAINT chk_action CHECK (action IN ('CREATE','UPDATE','DELETE'));
```
`entry_id` тоже без FK — ок (запись может быть hard-удалена в будущем, аудит сохраняется).
## 🟡 Индекс аудита недостаточен для типичных запросов
```sql
CREATE INDEX idx_audit_company ON audit_log(company_id);
```
Аудит почти всегда смотрят «по компании, свежие сверху». Нужен составной с временем:
```sql
CREATE INDEX IF NOT EXISTS idx_audit_company_time
ON audit_log(company_id, created_at DESC);
```
## 🟡 Нет автообновления updated_at
`updated_at` в companies имеет DEFAULT NOW(), но при UPDATE не меняется автоматически — код должен сам выставлять. Для надёжности — триггер:
```sql
CREATE OR REPLACE FUNCTION set_updated_at() RETURNS trigger AS $$
BEGIN NEW.updated_at = NOW(); RETURN NEW; END $$ LANGUAGE plpgsql;
CREATE TRIGGER trg_companies_updated
BEFORE UPDATE ON companies
FOR EACH ROW EXECUTE FUNCTION set_updated_at();
```
## 🟡 SERIAL вместо IDENTITY
`SERIAL` — легаси-приём. Для нового кода предпочтительнее:
```sql
id INTEGER GENERATED ALWAYS AS IDENTITY PRIMARY KEY,
```
Не критично, но это современный стандарт PG 10+ (чище права на sequence, нельзя случайно вставить id вручную).
## 🟡 custom_limit без CHECK на неотрицательность
```sql
custom_limit INTEGER DEFAULT NULL,
```
Можно записать отрицательный лимит. Добавить:
```sql
ALTER TABLE companies
ADD CONSTRAINT chk_custom_limit CHECK (custom_limit IS NULL OR custom_limit >= 0);
```
(Связано с багом `custom_limit = 0` из ревью queries.js — на уровне БД 0 разрешён, в коде игнорируется.)
## 🟡 comment/created_by — длины
`created_by VARCHAR(255)` под email — ок. `comment VARCHAR(255)` — приемлемо, но если ТЗ не ограничивает комментарий — рассмотреть TEXT. Не критично.
## 🟢 Изоляция через БД
Структурно обойти изоляцию нельзя: записи привязаны к `company_id`, утечка возможна только через код (запрос без фильтра company_id — см. `/export` в ревью server.js), не через схему.
## 🟢 Партиal-индекс idx_entries_active
Правильный приём — индекс только по активным записям, soft-deleted не раздувают индекс. Хорошо.
---
**Итог:**
1. 🔴 Добавить UNIQUE на активный (company_id, value_cidr) — закрывает гонку дублей на уровне БД.
2. 🔴 Рассмотреть тип `CIDR` + exclusion constraint (btree_gist) — закрывает гонку пересечений в БД; иначе минимум CHECK на формат.
3. 🔴 Явно задать ON DELETE для FK company_id.
4. 🟡 CHECK на action, на custom_limit ≥ 0; составной индекс аудита (company_id, created_at DESC); FK-политику аудита задокументировать.
5. 🟡 Триггер updated_at; перейти на IDENTITY вместо SERIAL.
Главное: текущая схема перекладывает уникальность и проверку пересечений целиком на код, который к ним уязвим в гонках. Перенос этих гарантий в БД (UNIQUE + exclusion/CHECK) — основной production-апгрейд.
research/opus-review-server.md
+97
View File
@@ -0,0 +1,97 @@
# Код-ревью server.js — auth, CSRF, XSS, заголовки
> Дата: 2026-05-30
## 🔴 Auth bypass 1 — отсутствие проверки подписи JWT (критично)
```js
const payload = JSON.parse(Buffer.from(auth.replace('Bearer ', '').split('.')[1], 'base64').toString());
```
Это не аутентификация — это просто декодирование base64 payload без проверки подписи. Любой может прислать самодельный токен `Bearer xxx.<base64 любого JSON>.yyy` и стать любой компанией. `ClientID`, `company_id` полностью подконтрольны атакующему → полный обход изоляции компаний, доступ к чужим whitelist, экспорт. Это та самая дыра, ради которой нужен JWKS/проверка подписи (см. отдельный prompt-opus-auth). До внедрения проверки подписи продакшен поднимать нельзя.
Минимум: `jose.jwtVerify(token, JWKS, { issuer: 'auth-api' })` с кэшированием ключей, и брать payload только из верифицированного результата. Также `split('.')[1]` упадёт на токене без точек → попадёт в `catch` → `req.user = {}`.
## 🔴 Auth bypass 2 — пустой req.user не блокирует доступ
```js
} catch { req.user = {}; }
next();
```
При невалидном токене `req.user = {}` и запрос идёт дальше. В `/` тогда `clientId = undefined` → `getOrCreateCompany(undefined, undefined)` создаст/найдёт компанию с `client_id = NULL`. Все анонимы делят одну «нулевую» компанию, видят и редактируют её whitelist. Нет ни одного `return res.status(401)`. Фикс — после catch и для не-DEV пути: если нет `req.user.clientId` → `return res.status(401).send('Unauthorized')`.
## 🔴 DEV_MODE — риск включения в проде
```js
const DEV = process.env.DEV_MODE === 'true';
...
if (DEV) { req.user = { ... clientId: 'WZ01325' ... }; return next(); }
```
Если `DEV_MODE=true` случайно попадёт в прод-конфиг — полный обход аутентификации, все становятся `WZ01325`. Нет защиты «DEV только не в production». Добавить страховку: `const DEV = process.env.DEV_MODE === 'true' && process.env.NODE_ENV !== 'production';` и логировать предупреждение при старте, если DEV активен.
## 🔴 CSRF — все POST-формы без токенов (критично)
`/add`, `/delete/:id` — обычные form-POST, меняют состояние, без CSRF-токена. В проде аутентификация по cookie/сессии (а не по заголовку Authorization вручную) ⇒ сторонний сайт может отправить `<form action="https://white.../delete/123" method=POST>` и удалить чужие записи. Если же токен реально приходит только в заголовке Authorization (не в cookie), CSRF слабее — но форма в браузере не может сама проставить Authorization, значит модель аутентификации в браузере вообще не работает с текущим кодом. Это надо прояснить (см. questions.md). В любом случае при cookie-сессии нужен CSRF-токен (`csurf` или double-submit) на все POST.
## 🟡 XSS через ?error= в редиректе
```js
res.redirect('/?error=' + encodeURIComponent(e.message));
```
Сам редирект экранирует. Уязвимость — в шаблоне: если `index.ejs` выводит `error` через `<%- %>` (не экранируя) — рефлексивный XSS. По ревью ejs вывод идёт через `<%= %>`, так что сейчас безопасно. Но `e.message` может содержать сырой текст ошибки БД — нежелательно показывать пользователю (info leak). Маппить на дружелюбные сообщения, не отдавать `e.message` БД наружу.
Кроме того `/?error=` читается из query, но в роуте `/` параметр `req.query.error` вообще не прокидывается в render (`error: null`) — то есть редирект с `?error=` ничего не покажет. Несоответствие: либо читать `req.query.error`, либо убрать. Если читать — обязательно только экранированный вывод.
## 🟡 Обработка ошибок — каскад в /add
```js
} catch (e) {
const company = await q.getOrCreateCompany(clientId, companyName).catch(() => null);
const entries = company ? await q.listEntries(company.id).catch(() => []) : [];
...
```
В catch-ветке снова дёргается БД (3 запроса). Если БД легла — все упадут в `.catch(() => ...)` и пользователь увидит исходную ошибку с пустым списком — приемлемо, но шумно. Главное: нет глобального error-handler middleware (`app.use((err, req, res, next) => ...)`) — необработанный промис в любом роуте уронит ответ висящим. Добавить финальный error middleware + `process.on('unhandledRejection')`.
## 🟡 Нет helmet / security-заголовков
Отсутствуют `X-Frame-Options`/CSP (clickjacking), `X-Content-Type-Options: nosniff`, `Referrer-Policy`, HSTS. Добавить `helmet()` сразу после создания app. Особенно `frame-ancestors`/`X-Frame-Options: DENY` — UI с формами удаления уязвим к clickjacking.
## 🟡 Нет rate-limit
`/add`, `/delete`, `/export` без ограничения частоты. Можно засыпать INSERT-ами/экспортом. Добавить `express-rate-limit` на мутирующие роуты и на `/export`.
## 🟡 /export — нет авторизации и изоляции (важно по ТЗ)
```js
app.get('/export', async (req, res) => {
const cidrs = await q.getExportCIDRs();
```
`getExportCIDRs()` без аргументов — отдаёт CIDR ВСЕХ компаний всем подряд, без проверки `req.user`, без фильтра по компании. Это утечка whitelist всех клиентов. По ТЗ экспорт должен быть либо служебным (ограничен по IP/токену), либо в рамках компании. Сейчас — публичный дамп всех адресов. Требует решения из questions.md (IP-ограничение/служебный токен), но в текущем виде — критичная утечка.
## 🟡 /healthz выше auth — ок, но раскрывает «OK» только
`/healthz` объявлен после middleware (значит проходит через auth). При DEV ок; в проде healthz будет требовать токен → проба готовности k8s упадёт. Вынести `/healthz` ВЫШЕ auth middleware.
## 🟡 Тело запроса без лимита размера
`express.urlencoded({ extended: true })` без `limit`. DoS большими телами. Поставить `limit: '32kb'`.
## 🟢 SSRF/path traversal
`express.static` на фиксированную папку, шаблоны фиксированы — обхода нет.
---
**Итог (по приоритету):**
1. 🔴 JWT без проверки подписи → полный обход auth и изоляции — блокер прода.
2. 🔴 Пустой `req.user = {}` не возвращает 401 → анонимы делят NULL-компанию.
3. 🔴 `/export` без авторизации отдаёт CIDR всех компаний — утечка.
4. 🔴 DEV_MODE без привязки к NODE_ENV — риск обхода auth в проде.
5. 🔴 Нет CSRF на POST-формах.
6. 🟡 Нет helmet, rate-limit, лимита тела, глобального error-handler.
7. 🟡 `/healthz` под auth (сломает k8s-пробу), `?error=` не читается в `/`, сырые ошибки БД наружу.
tests/test-results.md
+13
View File
@@ -3,6 +3,7 @@
> Дата: 2026-05-30 > Дата: 2026-05-30
> Среда: `https://white.nodejsk8s.dev.nubes.ru` > Среда: `https://white.nodejsk8s.dev.nubes.ru`
> DEV_MODE: true > DEV_MODE: true
> ⚠️ После фиксов от 2026-05-30: код запушен, миграция БД применена, ожидается передеплой Nubes
--- ---
@@ -29,6 +30,13 @@
| 11 | `абвгд` | Отклонено (мусор) | ✅ «Некорректный IPv4 адрес» | | 11 | `абвгд` | Отклонено (мусор) | ✅ «Некорректный IPv4 адрес» |
| 12 | (пусто) | Отклонено | ✅ «Пустое значение» | | 12 | (пусто) | Отклонено | ✅ «Пустое значение» |
### Баги (исправлены 2026-05-30)
| № | Ввод | Ожидание | Результат |
|---|---|---|---|
| 17 | `192.0.2.0/22` | Отклонено (суперсеть над TEST-NET) | ⬜ |
| 18 | `8.8.8.8/24abc` | Отклонено (parseInt мусор) | ⬜ |
| 19 | `10.0.0.0/24/8` | Отклонено (множественные слэши) | ⬜ |
### Бизнес-логика ### Бизнес-логика
| № | Тест | Ожидание | Результат | | № | Тест | Ожидание | Результат |
|---|---|---|---| |---|---|---|---|
@@ -81,4 +89,9 @@ curl -s -X POST -d "value=1.2.3.5/24" "$BASE/add"
# Экспорт # Экспорт
curl -s "$BASE/export" curl -s "$BASE/export"
# Баги (исправлены 2026-05-30)
curl -s -X POST -d "value=192.0.2.0/22" "$BASE/add" # суперсеть над TEST-NET
curl -s -X POST -d "value=8.8.8.8/24abc" "$BASE/add" # parseInt мусор
curl -s -X POST -d "value=10.0.0.0/24/8" "$BASE/add" # множественные слэши
``` ```