fix: header injection, api timeout, nginx duplicate + bump 0.5.21
- server.js: санитизация filename в /export (header injection) - ui/api-client.js: req.setTimeout(15000) + try/catch JSON.parse - docs/devops-deploy.md: убран дубль X-Forwarded-Proto
This commit is contained in:
@@ -139,7 +139,6 @@ server {
|
|||||||
proxy_set_header X-Forwarded-Proto $scheme;
|
proxy_set_header X-Forwarded-Proto $scheme;
|
||||||
proxy_set_header X-Real-IP $remote_addr;
|
proxy_set_header X-Real-IP $remote_addr;
|
||||||
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
|
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
|
||||||
proxy_set_header X-Forwarded-Proto $scheme;
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
```
|
```
|
||||||
|
|||||||
@@ -0,0 +1,164 @@
|
|||||||
|
# Аудит ipwhitelist-app — Claude Sonnet 4.6
|
||||||
|
|
||||||
|
**Дата:** 2026-06-04
|
||||||
|
**Файлы проверены:** server.js, src/auth.js, src/routes/oidc.js, src/api/routes/entries.js, ui/routes/auth.js, src/config.js, ui/routes/entries.js, ui/api-client.js, views/index.ejs, .env.example, docs/devops-deploy.md
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Вердикт
|
||||||
|
|
||||||
|
**С оговорками** — к деплою почти готов, но есть один critical баг (header injection) и один major баг (зависающие запросы). После исправления этих двух пунктов — production ready.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Проблемы
|
||||||
|
|
||||||
|
### 🔴 CRITICAL
|
||||||
|
|
||||||
|
**1. HTTP header injection в `/export` через параметр `filename`**
|
||||||
|
|
||||||
|
`server.js`, строки ~87-91:
|
||||||
|
```js
|
||||||
|
const fname = req.query.filename || 'white-list.txt';
|
||||||
|
res.set('Content-Disposition', disp + '; filename="' + fname + '"');
|
||||||
|
```
|
||||||
|
Если `filename` содержит `"` или `\r\n` — атакующий может внедрить произвольные HTTP-заголовки или сломать ответ. Эндпоинт публичный (без авторизации).
|
||||||
|
|
||||||
|
**Исправление:** санитизировать `fname` до вставки:
|
||||||
|
```js
|
||||||
|
const fname = (req.query.filename || 'white-list.txt').replace(/[^\w\-_.]/g, '_');
|
||||||
|
```
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### 🟠 MAJOR
|
||||||
|
|
||||||
|
**2. `ui/api-client.js`: таймаут не работает**
|
||||||
|
|
||||||
|
```js
|
||||||
|
req.on('timeout', () => { req.destroy(); reject(new Error('API timeout')); });
|
||||||
|
// ...но req.setTimeout() нигде не вызывается
|
||||||
|
```
|
||||||
|
В Node.js событие `timeout` на `http.ClientRequest` срабатывает только если установлен socket timeout через `req.setTimeout(ms)`. Без него запрос может висеть бесконечно при недоступном сервере. Все UI-запросы к API могут зависнуть.
|
||||||
|
|
||||||
|
**Исправление:** добавить `req.setTimeout(15000, ...)` после создания запроса.
|
||||||
|
|
||||||
|
**3. Отсутствует шаг 6 в `docs/devops-deploy.md`**
|
||||||
|
|
||||||
|
В документе многократно упоминается «шаг 6 — регистрация Keycloak OIDC клиента», но сам шаг не написан. DevOps не сможет развернуть продакшен без понимания как создать Confidential client в Keycloak (Valid Redirect URIs, Client Authentication, получение KC_CLIENT_SECRET).
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### 🟡 MINOR
|
||||||
|
|
||||||
|
**4. Дублирование `X-Forwarded-Proto` в nginx config**
|
||||||
|
|
||||||
|
В `docs/devops-deploy.md`, в блоке nginx server {} на 443:
|
||||||
|
```nginx
|
||||||
|
proxy_set_header X-Forwarded-Proto $scheme;
|
||||||
|
proxy_set_header X-Real-IP $remote_addr;
|
||||||
|
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
|
||||||
|
proxy_set_header X-Forwarded-Proto $scheme; # ← дубль
|
||||||
|
```
|
||||||
|
Второй заголовок — лишний, не вредит но вводит в заблуждение.
|
||||||
|
|
||||||
|
**5. `JSON.parse` без try-catch в `ui/api-client.js`**
|
||||||
|
|
||||||
|
```js
|
||||||
|
const data = ct.includes('application/json') ? JSON.parse(raw) : raw;
|
||||||
|
```
|
||||||
|
Если API вернёт невалидный JSON при `content-type: application/json` — необработанный SyntaxError упадёт наверх и отдаст 500 без внятного сообщения.
|
||||||
|
|
||||||
|
**6. Нет ограничения размера ответа IAM API**
|
||||||
|
|
||||||
|
В `fetchIamUser` и `switchProfile` (`src/auth.js`) ответ накапливается без ограничения:
|
||||||
|
```js
|
||||||
|
let data = '';
|
||||||
|
res.on('data', c => { data += c; });
|
||||||
|
```
|
||||||
|
Злонамеренный или сломанный IAM-сервер может передать гигабайты данных.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Ответы на 5 вопросов
|
||||||
|
|
||||||
|
### 1. Безопасность
|
||||||
|
|
||||||
|
| Проверка | Результат |
|
||||||
|
|---|---|
|
||||||
|
| Open redirect | ✅ Защищён. `safeLocal()` и `safeReturn()` — корректная проверка `startsWith('/')` && `!startsWith('//')`. |
|
||||||
|
| XSS в шаблонах | ✅ EJS использует `<%= %>` (с экранированием). Helmet + CSP настроены. |
|
||||||
|
| CSRF | ✅ `csrf.js` middleware подключён к мутирующим роутам. |
|
||||||
|
| Утечка токенов | ✅ Токен в сессии (PostgreSQL), не в HTML, не в cookie клиента напрямую. |
|
||||||
|
| Header injection | ❌ `/export` — `filename` из query без санитизации (см. CRITICAL #1). |
|
||||||
|
| Rate limiting | ✅ `authLimiter` на POST /login. |
|
||||||
|
| DEV_MODE в production | ✅ Жёсткая защита — `process.exit(1)` при `NODE_ENV=production && DEV_MODE=true`. |
|
||||||
|
|
||||||
|
### 2. IAM интеграция
|
||||||
|
|
||||||
|
`fetchIamUser` и `switchProfile` — реализованы корректно:
|
||||||
|
- Timeout: 10 секунд через `req.setTimeout(10000)` — OK.
|
||||||
|
- Error handling: IAM недоступен → `console.error` + fallback на JWT claims. Правильно.
|
||||||
|
- Fallback в `/callback` (`src/routes/oidc.js`): вычисляет `isAdmin` по `activeClientId === ADMIN_CLIENT_ID`. Безопасно — токен уже верифицирован подписью.
|
||||||
|
- `activeProfileId` сохраняется в сессии — переключение компаний через IAM API работает.
|
||||||
|
|
||||||
|
**Единственный риск:** если `IAM_API_URL` недоступен при логине — `isAdmin: false` всегда, даже для администратора. Это намеренный безопасный дефолт, но пользователи об этом не уведомляются в UI.
|
||||||
|
|
||||||
|
### 3. Роли/компании
|
||||||
|
|
||||||
|
| Аспект | Результат |
|
||||||
|
|---|---|
|
||||||
|
| `isAdmin` | ✅ В IAM-режиме — из `ui.isAdmin` (IAM единственный источник). В fallback — по `clientId`. |
|
||||||
|
| `allClientIds` | ✅ `resolveCompany()` в API проверяет `profiles` (IAM) или `allClientIds` (fallback). Изоляция между компаниями соблюдена. |
|
||||||
|
| Admin ?company= | ✅ `parseInt` + `isFinite` + проверка в БД — инъекции через company ID невозможны. |
|
||||||
|
| UI vs API | ✅ UI полностью делегирует бизнес-логику к API. Расхождений нет. |
|
||||||
|
| Мульти-компания | ✅ `client_id` из query проверяется против `allowedIds` перед использованием. |
|
||||||
|
|
||||||
|
### 4. DevOps
|
||||||
|
|
||||||
|
**.env.example:** достаточен — все обязательные переменные перечислены с пояснениями.
|
||||||
|
**devops-deploy.md:** хорошая документация по PostgreSQL, Node.js, nginx, PM2, certbot. Но:
|
||||||
|
- Шаг 6 (Keycloak OIDC client) — отсутствует (см. MAJOR #3).
|
||||||
|
- Нет инструкции по ротации SESSION_SECRET при компрометации.
|
||||||
|
- `IAM_API_URL` задан по умолчанию в `.env.example` (не закомментирован) — хорошо, но стоит добавить пометку что это реальный внешний сервис.
|
||||||
|
|
||||||
|
### 5. Код
|
||||||
|
|
||||||
|
| Проверка | Результат |
|
||||||
|
|---|---|
|
||||||
|
| Незакрытые соединения | ⚠️ `ui/api-client.js` — `timeout` event без `req.setTimeout()` (см. MAJOR #2). `fetchIamUser`/`switchProfile` — OK, `setTimeout` есть. |
|
||||||
|
| Race conditions | ✅ Отсутствуют. Переключение профиля — sequential await. |
|
||||||
|
| Циклические require | ✅ `ui/routes/entries.js` → `src/auth` (lazy inside handler) — нет цикла. |
|
||||||
|
| JSON.parse unsafe | ⚠️ `ui/api-client.js` — без try-catch (см. MINOR #5). |
|
||||||
|
| Body size limits | ✅ `express.urlencoded({ limit: '32kb' })`, `express.json({ limit: '32kb' })`. |
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Оценки
|
||||||
|
|
||||||
|
| Категория | Оценка | Обоснование |
|
||||||
|
|---|---|---|
|
||||||
|
| **DevOps** | **7 / 10** | Хорошая документация, fail-fast проверки, но нет Keycloak-шага. |
|
||||||
|
| **Код** | **7 / 10** | Чистая архитектура API+UI, правильный CSRF/auth. Баг с таймаутом в api-client и header injection снижают. |
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Топ-3 что исправить
|
||||||
|
|
||||||
|
1. **[CRITICAL] Санитизировать `filename` в `/export`** (`server.js` ~87):
|
||||||
|
```js
|
||||||
|
const fname = (req.query.filename || 'white-list.txt').replace(/[^\w\-_.]/g, '_');
|
||||||
|
```
|
||||||
|
|
||||||
|
2. **[MAJOR] Добавить `req.setTimeout()` в `ui/api-client.js`** (~35):
|
||||||
|
```js
|
||||||
|
const req = lib.request(opts, (res) => { ... });
|
||||||
|
req.setTimeout(15000, () => { req.destroy(); reject(new Error('API timeout')); });
|
||||||
|
```
|
||||||
|
Убрать дублирующий `req.on('timeout', ...)` или оставить оба.
|
||||||
|
|
||||||
|
3. **[MAJOR] Написать шаг 6 в `docs/devops-deploy.md`** — регистрация OIDC-клиента в Keycloak:
|
||||||
|
- Тип клиента: Confidential, Authorization Code Flow
|
||||||
|
- Valid Redirect URIs: `https://ваш-домен/callback`
|
||||||
|
- Web Origins: `https://ваш-домен`
|
||||||
|
- Как скопировать KC_CLIENT_ID и KC_CLIENT_SECRET
|
||||||
+1
-1
@@ -1,6 +1,6 @@
|
|||||||
{
|
{
|
||||||
"name": "ipwhitelist",
|
"name": "ipwhitelist",
|
||||||
"version": "0.5.20",
|
"version": "0.5.21",
|
||||||
"description": "IP WhiteList microservice for cloud provider",
|
"description": "IP WhiteList microservice for cloud provider",
|
||||||
"main": "server.js",
|
"main": "server.js",
|
||||||
"scripts": {
|
"scripts": {
|
||||||
|
|||||||
@@ -92,7 +92,7 @@ async function start() {
|
|||||||
const aggregated = aggregateCIDRs(cidrs);
|
const aggregated = aggregateCIDRs(cidrs);
|
||||||
const text = aggregated.join('\n') + (aggregated.length ? '\n' : '');
|
const text = aggregated.join('\n') + (aggregated.length ? '\n' : '');
|
||||||
res.set('Content-Type', 'text/plain; charset=utf-8');
|
res.set('Content-Type', 'text/plain; charset=utf-8');
|
||||||
const fname = req.query.filename || 'white-list.txt';
|
const fname = (req.query.filename || 'white-list.txt').replace(/[^\w\-_. ]/g, '_');
|
||||||
const disp = req.query.view === '1' ? 'inline' : 'attachment';
|
const disp = req.query.view === '1' ? 'inline' : 'attachment';
|
||||||
res.set('Content-Disposition', disp + '; filename="' + fname + '"');
|
res.set('Content-Disposition', disp + '; filename="' + fname + '"');
|
||||||
res.send(text);
|
res.send(text);
|
||||||
|
|||||||
+7
-3
@@ -48,12 +48,16 @@ function apiRequest(method, path, token, body) {
|
|||||||
res.on('data', c => { raw += c; });
|
res.on('data', c => { raw += c; });
|
||||||
res.on('end', () => {
|
res.on('end', () => {
|
||||||
const ct = res.headers['content-type'] || '';
|
const ct = res.headers['content-type'] || '';
|
||||||
const data = ct.includes('application/json') ? JSON.parse(raw) : raw;
|
try {
|
||||||
resolve({ status: res.statusCode, data });
|
const data = ct.includes('application/json') ? JSON.parse(raw) : raw;
|
||||||
|
resolve({ status: res.statusCode, data });
|
||||||
|
} catch (e) {
|
||||||
|
reject(new Error('Invalid JSON from API: ' + e.message));
|
||||||
|
}
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
req.setTimeout(15000, () => { req.destroy(); reject(new Error('API timeout')); });
|
||||||
req.on('error', reject);
|
req.on('error', reject);
|
||||||
req.on('timeout', () => { req.destroy(); reject(new Error('API timeout')); });
|
|
||||||
if (bodyStr) req.write(bodyStr);
|
if (bodyStr) req.write(bodyStr);
|
||||||
req.end();
|
req.end();
|
||||||
});
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user