fix: DeepSeek audit — header injection + IAM limit + bump 0.5.22
- src/api/routes/entries.js: санитизация filename в /api/v1/entries/export - ui/routes/export.js: санитизация filename в UI /export - src/auth.js: лимит 100KB для IAM-ответов - src/api/routes/entries.js: fallback для пустого allowedIds
This commit is contained in:
@@ -0,0 +1,231 @@
|
||||
# Аудит ipwhitelist-app — DeepSeek V4 Pro
|
||||
|
||||
**Дата:** 2026-06-04
|
||||
**Файлы проверены:** все 30+ файлов из src/, ui/, views/, docs/, server.js, package.json, .env.example, sql/schema.sql
|
||||
|
||||
---
|
||||
|
||||
## Вердикт
|
||||
|
||||
**С оговорками** — код высокого качества, архитектура продумана, но есть 2 проблемы которые нужно исправить перед production: CSRF не подключён и две /export-ручки без санитизации filename.
|
||||
|
||||
---
|
||||
|
||||
## 1. Найденные проблемы
|
||||
|
||||
### 🔴 CRITICAL
|
||||
|
||||
**C1. Header injection в двух /export-ручках (не санитизирован filename)**
|
||||
|
||||
В проекте **три** эндпоинта `/export`. Санитизация `filename` добавлена только в одном (`server.js:93` — публичный `/export`). Два других — без защиты:
|
||||
|
||||
| Файл | Строка | Статус |
|
||||
|---|---|---|
|
||||
| `server.js:93` (публичный `/export`) | `replace(/[^\w\-_. ]/g, '_')` | ✅ исправлено |
|
||||
| `src/api/routes/entries.js:104` (`/api/v1/entries/export`) | `req.query.filename \|\| 'white-list.txt'` | ❌ без санитизации |
|
||||
| `ui/routes/export.js:32` (UI `/export` за авторизацией) | `req.query.filename \|\| 'white-list.txt'` | ❌ без санитизации |
|
||||
|
||||
Атакующий с Bearer-токеном может внедрить `\r\n` в `Content-Disposition` через `?filename=...%0d%0a...`.
|
||||
|
||||
**Рекомендация:** вынести санитизацию в хелпер `src/config.js` и использовать во всех трёх местах.
|
||||
|
||||
---
|
||||
|
||||
### 🟠 MAJOR
|
||||
|
||||
**M1. CSRF-защита реализована, но НЕ подключена**
|
||||
|
||||
`src/middleware/csrf.js` содержит `initCsrf()` с `doubleCsrf` (csrf-csrf v4) — современная замена deprecated `csurf`. Middleware правильно настроен: cookie `csrf-token`, `getCsrfTokenFromRequest: (req) => req.body._csrf`, HMAC-подпись.
|
||||
|
||||
**Но `initCsrf()` нигде не вызывается.** Ни в `server.js`, ни в `ui/index.js`, ни в роутах.
|
||||
|
||||
Во всех EJS-шаблонах `csrfToken` передаётся как пустая строка:
|
||||
- `views/index.ejs` → `csrfToken: ''`
|
||||
- `views/admin.ejs` → `csrfToken: ''`
|
||||
|
||||
Формы содержат `<input type="hidden" name="_csrf" value="<%= csrfToken %>">` — токен всегда пустой, проверка никогда не срабатывает.
|
||||
|
||||
**Рекомендация:** вызвать `initCsrf()` в `server.js → start()`, передать `doubleCsrfProtection` в UI-роуты, `generateCsrfToken` — в GET-обработчики. API-слой (`/api/v1/*`) не требует CSRF (Bearer-токены).
|
||||
|
||||
---
|
||||
|
||||
**M2. IAM API — нет ограничения размера ответа**
|
||||
|
||||
`fetchIamUser` и `switchProfile` в `src/auth.js` накапливают ответ без лимита:
|
||||
|
||||
```js
|
||||
let data = '';
|
||||
res.on('data', c => { data += c; });
|
||||
```
|
||||
|
||||
Скомпрометированный или сломанный IAM-сервер может исчерпать память процесса.
|
||||
|
||||
**Рекомендация:** добавить проверку `if (data.length > 100_000) { req.destroy(); reject(...) }`.
|
||||
|
||||
---
|
||||
|
||||
**M3. `resolveCompany` в API entries.js — хрупкий `profiles`/`allClientIds` fallback**
|
||||
|
||||
Строки 47–58:
|
||||
```js
|
||||
const allowedIds = req.user.profiles && req.user.profiles.length
|
||||
? req.user.profiles.map(p => p.client_id)
|
||||
: (req.user.allClientIds || []);
|
||||
```
|
||||
|
||||
Когда IAM недоступен, `req.user.profiles` — `undefined`. Fallback на `allClientIds` корректен, НО: если пользователь переключил компанию через UI (`activeClientId` обновлён в сессии), а API-слой этого не видит (API читает `req.user` из Bearer-токена, не из сессии), то `client_id` из query может не пройти проверку `allowedIds.includes(requestedId)` — и переключение молча проигнорируется.
|
||||
|
||||
**Рекомендация:** в API-слое добавить fallback: если `allowedIds` пуст — разрешить любой `client_id` из query (доверять тому, что в токене).
|
||||
|
||||
---
|
||||
|
||||
### 🟡 MINOR
|
||||
|
||||
**m1. Отсутствует `proxy_read_timeout` в nginx-конфиге `docs/devops-deploy.md`**
|
||||
|
||||
Nginx по умолчанию обрывает соединение через 60с. Для `/export` (агрегация тысяч CIDR) этого может не хватить. Добавить `proxy_read_timeout 120s;`.
|
||||
|
||||
**m2. `server.js` — `checkConnection()` вызывается без `await`**
|
||||
|
||||
Строка 127:
|
||||
```js
|
||||
checkConnection()
|
||||
.then(() => console.log('DB connected'))
|
||||
.catch(e => console.error('DB not ready:', e.message));
|
||||
```
|
||||
|
||||
Приложение стартует до проверки соединения с БД. Если БД недоступна — сервер отвечает 500 на все запросы, но не падает. Лучше: `await checkConnection()` внутри `start()` до `app.listen()`.
|
||||
|
||||
**m3. `ui/routes/auth.js` —不一致的 `safeReturn`**
|
||||
|
||||
POST `/login-token`:
|
||||
- Ветка `token` → `res.redirect(safeReturn(returnTo) || '/')` ✅
|
||||
- Ветка `clientId` → `res.redirect(returnTo || '/')` ❌ без `safeReturn`
|
||||
|
||||
Параметр `returnTo` идёт из скрытого поля формы, но теоретически может быть подменён. Низкий риск, но inconsistent.
|
||||
|
||||
**m4. `src/queries.js:getExportCIDRs` — нет LIMIT**
|
||||
|
||||
При вызове без `companyId` (admin) запрос возвращает ВСЕ CIDR всех компаний. Для агрегации это ожидаемо, но при 100k+ записей — memory spike.
|
||||
|
||||
**m5. `src/queries.js:getAudit` — hardcoded LIMIT 500 без пагинации**
|
||||
|
||||
Для production с сотнями компаний 500 строк аудита — мало. Нужна пагинация.
|
||||
|
||||
**m6. `ui/routes/entries.js:42` — `require('../../src/auth')` внутри обработчика**
|
||||
|
||||
Динамический `require` в рантайме (для `switchProfile`). Node.js кеширует, так что performance impact минимален, но это «code smell». Лучше прокинуть `auth` через фабрику `createRouter({ auth })`.
|
||||
|
||||
---
|
||||
|
||||
## 2. Безопасность — полный разбор
|
||||
|
||||
| Вектор | Статус | Комментарий |
|
||||
|---|---|---|
|
||||
| Open redirect `/login?returnTo=` | ✅ | `safeReturn()` + `safeLocal()` — `startsWith('/') && !startsWith('//')` |
|
||||
| Open redirect `/callback` | ✅ | `safeLocal()` на `oidcReturnTo` |
|
||||
| XSS (EJS) | ✅ | `<%= %>` везде (экранирование), helmet + CSP |
|
||||
| CSRF | ❌ | Код написан, но не подключён (см. M1) |
|
||||
| Header injection | ⚠️ | Частично исправлено (см. C1) |
|
||||
| Clickjacking | ✅ | `frameAncestors: ["'none'"]` в CSP |
|
||||
| Rate limiting | ✅ | `authLimiter` (10/5min), `mutationLimiter` (30/min), `exportLimiter` (20/min) |
|
||||
| DEV_MODE в production | ✅ | Жёсткий `process.exit(1)` |
|
||||
| Session fixation | ✅ | `express-session` + `connect-pg-simple`, sessionID меняется при логине |
|
||||
| SQL injection | ✅ | Parameterized queries (`$1`, `$2`) везде |
|
||||
| Secret management | ✅ | Все секреты из `process.env`, fail-fast для `SESSION_SECRET` и `IAM_API_URL` |
|
||||
| JWT validation | ✅ | RS256 + JWKS (Keycloak), issuer check, audience check |
|
||||
|
||||
---
|
||||
|
||||
## 3. IAM-интеграция — полный разбор
|
||||
|
||||
| Аспект | Статус | Комментарий |
|
||||
|---|---|---|
|
||||
| `fetchIamUser` timeout | ✅ | 10 секунд через `req.setTimeout(10000)` |
|
||||
| `fetchIamUser` error handling | ✅ | try/catch с понятным сообщением |
|
||||
| `switchProfile` | ✅ | Корректный POST с `profile_id` в body |
|
||||
| OIDC callback IAM fallback | ✅ | При недоступности IAM — fallback на JWT claims с логом |
|
||||
| `login-token` IAM enrichment | ✅ | Вызывается только в OIDC-режиме (`auth.isOidc`) |
|
||||
| `userFromPayload` с `iamData` | ✅ | `profiles`, `allClientIds`, `isAdmin` — всё из IAM |
|
||||
| `resolveCompany` multi-profile | ⚠️ | Хрупкий fallback (см. M3) |
|
||||
| IAM response size limit | ❌ | Нет ограничения (см. M2) |
|
||||
| Разные стенды IAM | ✅ | `IAM_API_URL` в .env, dev/test/prod через переменную |
|
||||
|
||||
---
|
||||
|
||||
## 4. Корректность кода — полный разбор
|
||||
|
||||
| Аспект | Статус | Комментарий |
|
||||
|---|---|---|
|
||||
| Циклические require | ✅ | Чистая архитектура: `config.js` → `auth.js` → `queries.js` → `db.js`. Нет циклов |
|
||||
| DB connection pool | ✅ | `pg.Pool`, max=10, idleTimeout=30s, error listener |
|
||||
| Транзакции | ✅ | `BEGIN/FOR UPDATE/COMMIT/ROLLBACK` в `createEntry`, `updateEntry`, `deleteEntry` |
|
||||
| Mock-режим | ✅ | Локальная RSA-пара, `/dev-login`, `DEV_MODE`/`DEV_SECRET` |
|
||||
| Middleware order | ✅ | helmet → static → session → OIDC → API → UI → error handler |
|
||||
| Promise error handling | ✅ | Все async-обработчики обёрнуты в try/catch |
|
||||
| `checkConnection` | ⚠️ | Не `await` (см. m2) |
|
||||
| `rangeToCIDRs` | ✅ | Элегантный bit-twiddling алгоритм |
|
||||
| `aggregateCIDRs` | ✅ | Scan-line merge + rangeToCIDRs |
|
||||
| Уникальность CIDR | ✅ | Partial unique index `WHERE deleted_at IS NULL` |
|
||||
| Soft delete | ✅ | `deleted_at` + `deleted_by`, записи не удаляются физически |
|
||||
|
||||
---
|
||||
|
||||
## 5. DevOps — полный разбор
|
||||
|
||||
| Аспект | Статус | Комментарий |
|
||||
|---|---|---|
|
||||
| Пошаговая инструкция | ✅ | 6 шагов: PostgreSQL → Node.js → .env → nginx → PM2 → Keycloak |
|
||||
| Все зависимости | ✅ | Node.js 20, PostgreSQL 16, nginx, certbot, PM2 |
|
||||
| Переменные окружения | ✅ | `.env.example` с комментариями, команды генерации секретов |
|
||||
| Схема БД | ✅ | `sql/schema.sql` с индексами и constraints |
|
||||
| HTTPS | ✅ | nginx + certbot, автообновление |
|
||||
| Keycloak OIDC | ✅ | Пошаговая инструкция регистрации клиента (шаг 6) |
|
||||
| PM2 автозапуск | ✅ | `pm2 startup systemd` + `pm2 save` |
|
||||
| Противоречия в документах | ✅ | Нет — все ссылки согласованы |
|
||||
| `proxy_read_timeout` | ⚠️ | Отсутствует (см. m1) |
|
||||
|
||||
---
|
||||
|
||||
## 6. Оценки
|
||||
|
||||
| Категория | Оценка | Обоснование |
|
||||
|---|---|---|
|
||||
| **DevOps-готовность** | **9/10** | Понятная инструкция, все зависимости описаны, OIDC-клиент документирован. Не хватает `proxy_read_timeout` в nginx. |
|
||||
| **Код: безопасность** | **7/10** | JWT, CSP, helmet, rate-limit — на высоте. Но CSRF не подключён и две /export-ручки без санитизации filename. |
|
||||
| **Код: архитектура** | **9/10** | Чистое разделение API/UI, middleware chain прозрачен, нет циклических зависимостей. |
|
||||
| **Код: IAM интеграция** | **8/10** | Fallback при недоступности IAM, timeout, switchProfile. Нет лимита на ответ IAM. |
|
||||
| **Тестирование** | **9/10** | 121 API-тест, 68 UI-тестов, 47+111 compliance-тестов, 191 стресс-тест. |
|
||||
| **Общая оценка** | **8/10** | Крепкий продакшен-реди код. 2 обязательных фикса перед деплоем. |
|
||||
|
||||
---
|
||||
|
||||
## 7. Топ-5 что исправить перед production
|
||||
|
||||
1. **Подключить CSRF** — вызвать `initCsrf()` в `server.js`, передать в UI-роуты, заполнить `csrfToken` в шаблонах (M1)
|
||||
2. **Санитизировать `filename` в двух оставшихся /export-ручках** — `src/api/routes/entries.js:104` и `ui/routes/export.js:32` (C1)
|
||||
3. **Добавить `Content-Length` проверку в `fetchIamUser`/`switchProfile`** — лимит 100KB (M2)
|
||||
4. **Добавить `proxy_read_timeout 120s` в nginx-конфиг** в `docs/devops-deploy.md` (m1)
|
||||
5. **Унифицировать `safeReturn` в `ui/routes/auth.js`** — добавить в ветку `clientId` (m3)
|
||||
|
||||
---
|
||||
|
||||
## 8. Сравнение с аудитом Claude Sonnet 4.6
|
||||
|
||||
Sonnet проверил 11 файлов и нашёл 6 проблем (1 critical, 2 major, 3 minor). Все его находки **подтверждаю**.
|
||||
|
||||
Что Sonnet **пропустил** (и я нашёл дополнительно):
|
||||
|
||||
| Проблема | Критичность |
|
||||
|---|---|
|
||||
| CSRF middleware не подключён (код есть, но не wired) | 🔴 MAJOR |
|
||||
| Две /export-ручки без санитизации filename (проверил только публичную) | 🔴 CRITICAL |
|
||||
| IAM API без ограничения размера ответа | 🟠 MAJOR |
|
||||
| `resolveCompany` — хрупкий `profiles`/`allClientIds` fallback | 🟠 MAJOR |
|
||||
| `checkConnection()` без `await` | 🟡 MINOR |
|
||||
| Не-conсистентный `safeReturn` в `ui/routes/auth.js` | 🟡 MINOR |
|
||||
| `getExportCIDRs` без LIMIT | 🟡 MINOR |
|
||||
| `getAudit` без пагинации | 🟡 MINOR |
|
||||
| Динамический `require` в `ui/routes/entries.js` | 🟡 MINOR |
|
||||
|
||||
**Итого:** Sonnet нашёл 6 проблем, я подтверждаю все + дополнительно 9. Основное упущение Sonnet — он не проверил, что `initCsrf()` нигде не вызывается, и не заметил что санитизация filename применена только к одной из трёх export-ручек.
|
||||
Reference in New Issue
Block a user