135 lines
8.1 KiB
Markdown
135 lines
8.1 KiB
Markdown
# Сводка код-ревью 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 | Остальное (см. таблицу 🟡) | Качество |
|