Files
ipwhitelist-app/docs/sonnet-full-architecture-analysis.md
T
naeel 1350f0dbba security: fix OIDC state check bypass + open redirect in login-token
- oidc.js: remove || true — state check now works in all environments
- auth.js: wrap returnTo in safeReturn (was unprotected on line 166)
2026-06-11 20:02:49 +04:00

18 KiB
Raw Blame History

Полный архитектурный анализ ipwhitelist-app

Дата: 2026-06-11. Версия кода: 0.5.60. НЕ является инструкцией к правке — только диагностика.


1. Auth-поток: дублирование

Два парсера JWT вместо одного

userFromPayload() (src/auth.js) и resolveUser middleware (ui/index.js) делают одно и то же — извлекают allClientIds, activeClientId, email из payload — но по-разному:

Поле userFromPayload resolveUser
ClientID payload.ClientID || payload.client_id payload.ClientID || payload.clientId || payload.sub
KC claims обрабатывает payload.claims (массив wz-строк) не обрабатывает
email 4 fallback-а: email, login, sub, preferred_username req.session?.user?.email || payload.email || payload.login
activeClientId первый из allClientIds из сессии (если есть) или первый из токена
isAdmin activeClientId === ADMIN_CLIENT_ID iamAdmin || isNail

Что не так: если Keycloak-токен выдаёт clientIds в payload.claims[], resolveUser их не видит — пользователь получает activeClientId = '' или rawClientId. userFromPayload видит, но он применяется только в bearerMiddleware (API-слой без сессии).

Где req.session.user расходится с payload:

  • session.user.isAdmin — из IAM (реальный флаг). JWT-payload этого поля не содержит.
  • session.user.activeClientId — может быть изменён переключателем компании; JWT этого не знает.
  • session.user.profiles[] — приходит только из IAM, в JWT отсутствует.
  • session.user.isImpersonated — из IAM, в JWT нет.

Тройное дублирование конструктора session.user

Объект session.user строится в трёх местах по одной схеме, но с разными полями:

  1. src/routes/oidc.js/callback (IAM-ветка и fallback)
  2. ui/routes/auth.jsPOST /login-token (IAM-ветка и fallback) и POST /login (mock)
  3. ui/index.jsresolveUser (перестраивает req.user из токена + сессии)

Если добавить новое поле (например, fio), нужно обновить все три места. Это уже произошло: fio есть в /callback, но нет в resolveUser.

Как исправить: извлечь buildSessionUser(iamData) как общую функцию. Один источник истины — одна правка.


2. adminMode / canAdminMode / isAdmin / isNail — четыре флага

Что каждый означает сейчас

Флаг Тип Источник Смысл
isAdmin boolean IAM userInfo.isAdmin или isNail «IAM считает юзера глобальным админом»
isNail boolean хардкод email === 'ntazetdinov@nubes.ru' «это я, временный костыль»
canAdminMode boolean (isAdmin && clientId===WZ01112) || isNail «может ли юзер включить режим админа»
adminMode boolean session.adminMode (toggle) «режим сейчас включён»

Проблемы

1. canAdminMode добавляет лишнее условие clientId === WZ01112. Если IAM говорит isAdmin=true — почему ограничивать активным clientId? Логично было бы: canAdminMode = isAdmin. Либо, если это сознательное ограничение (только сотрудники Nubes могут быть админами) — тогда нужна проверка на стороне IAM, а не хардкод clientId в коде.

2. isNail присутствует в трёх местах:

  • ui/index.jsresolveUser (вычисление isAdmin, canAdminMode)
  • ui/routes/auth.js/toggle-admin (проверка права на toggle)
  • Логика toggle проверяет clientId === ADMIN_CLIENT_ID а не canAdminMode. Если условие canAdminMode изменится — toggle-проверка не обновится автоматически.

3. toggle-admin в ui/routes/auth.js не использует canAdminMode:

const canAdmin = req.session.user && (
  req.session.user.isAdmin ||
  req.session.user.clientId === ADMIN_CLIENT_ID   // ← другая формула!
);

Это третья, независимая реализация того же условия.

Как упростить: два флага вместо четырёх.

  • isAdmin — из IAM, без isNail. isNail убирается после того как IAM-роль для ntazetdinov настроена.
  • adminMode — из сессии. Право переключиться: isAdmin === true.
  • canAdminMode убирается: он равен isAdmin.

3. Компании: getOrCreateCompany, switchProfile

switchProfile при недоступном IAM

В ui/routes/entries.js:

try {
  await switchProfile(token, targetProfile.id);
  // обновляем сессию...
} catch (e) {
  // Локальный fallback без IAM
  req.session.user.activeClientId = switchTo;
  req.user.activeClientId = switchTo;
}

Проблема: при IAM-недоступности локальный fallback обновляет сессию, но IAM не обновлён. При следующем входе IAM вернёт старый активный профиль. Кроме того, ошибка проглатывается без сообщения пользователю. Пользователь думает, что переключился, а фактически переключение непостоянно — до следующего логина.

Как исправить: показывать предупреждение «Переключение не сохранено в IAM» при fallback.

getOrCreateCompany — потенциальный мусор

getOrCreateCompany вызывается в resolveCompany на каждый API-запрос. Если пользователь только экспортирует данные через /api/v1/entries/export (не-admin ветка), в БД создаётся строка компании с owner_email. Это корректно по ТЗ, но:

  • owner_email обновляется только при INSERT. Если тот же client_id войдёт с другим email, owner не поменяется. В текущей схеме это не критично, но стоит знать.
  • Нет способа понять «мусорные» компании (которые создались из-за fallback или тестов), кроме как смотреть на количество записей.

companyQuery — XSS в URL

В ui/routes/entries.js:

function companyQuery(req) {
  const id = req.query.company || (req.body && req.body.company_id);
  if (id) return '?company=' + id;
  ...
}

id не проверяется на тип. Если req.query.company содержит спецсимволы или строку, они уйдут в API-запрос как есть. В resolveCompany есть parseInt(..., 10) с проверкой isFinite — так что API-сторона защищена. Но URL-строка в UI может быть неожиданной.


4. Безопасность

4.1 КРИТИЧНО: client_id подмена при недоступном IAM

В src/api/routes/entries.js, resolveCompany:

const allowedIds = req.user.profiles && req.user.profiles.length
  ? req.user.profiles.map(p => p.client_id)
  : (req.user.allClientIds || []);

const effectiveClientId = (requestedId && (!allowedIds.length || allowedIds.includes(requestedId)))
  ? requestedId
  : req.user.clientId;

Условие !allowedIds.length означает: если IAM не ответил и JWT не содержит claims — requestedId принимается без проверки. Злоумышленник, получив Bearer-токен (даже от другой компании с пустым allClientIds), может передать ?client_id=WZXXXXX и получить данные чужой компании.

Как исправить: если allowedIds пуст — разрешать только req.user.clientId (из самого токена), отклонять requestedId.

4.2 КРИТИЧНО: OIDC state проверка отключена навсегда

В src/routes/oidc.js:

if (process.env.NODE_ENV === 'production' || true) {  // ← || true делает всегда true
  if (req.session) delete req.session.oidcState;
} else {
  // state check — никогда не выполняется
}

Проверка state параметра в /callback отключена во всех окружениях. Это убирает защиту от CSRF-атак на OAuth-поток. Злоумышленник может подменить code в callback.

Как исправить: убрать || true. Если для тестирования нужно пропускать — завести SKIP_OIDC_STATE=true env.

4.3 Open Redirect в /login-token (clientId ветка)

В ui/routes/auth.js, ветка входа по clientId:

res.redirect(returnTo || '/');

В отличие от token-ветки выше (safeReturn(returnTo)), здесь returnTo из тела POST не валидируется. Если returnTo = 'https://evil.com' — пользователь улетит туда.

Как исправить: везде использовать safeReturn(returnTo) из src/auth.js.

4.4 CSRF: применяется ли ко всем POST-формам?

В ui/routes/entries.js все POST-обработчики (/add, /edit/:id, /delete/:id) не содержат явного doubleCsrfProtection. При этом в шаблоне в GET / передаётся csrfToken: '' (пустая строка). Это означает CSRF-защита либо навешена глобально в server.js (надо проверить), либо отсутствует на этих маршрутах.

4.5 /export — кто имеет доступ

В src/api/routes/entries.js /export не требует isAdmin. Любой аутентифицированный пользователь получает список CIDR своей компании. Это, скорее всего, правильно по ТЗ (пользователь экспортирует свои же данные). Но стоит уточнить: или /export должен быть только в adminMode — тогда нужна проверка в API-слое.


5. Аудит

5.1 user_email в audit_log — всегда ли правильный?

В API-слое req.user строится в bearerMiddlewareuserFromPayload(payload) без iamData. Email берётся из JWT claims. Если пользователь сменил email в IAM после выдачи токена, в аудит попадёт старый email. Это редкий случай (токен живёт 8 часов), но надо знать.

5.2 impersonated_by — всегда NULL в API-слое

bearerMiddleware вызывает userFromPayload(payload) без имперсонационных заголовков. userFromPayload возвращает объект без originalUserEmail. Поэтому в createEntry, updateEntry, deleteEntry:

req.user.originalUserEmail  // → undefined → logAudit(..., undefined) → NULL

impersonated_by никогда не заполняется через API, только если зашить в сессию и передавать через X-заголовки (известный разрыв).

5.3 GET /api/v1/audit — есть или нет?

В src/queries.js есть getAudit(companyId). В views/audit.ejs есть шаблон. Но наличие API-эндпоинта не проверено по видимым файлам. Если /api/v1/audit не существует, аудит доступен только через UI — что делает его недоступным для API-клиентов.


6. Общая архитектура

6.1 Циклическая зависимость: ui → src

ui/routes/entries.js:

const { switchProfile } = require('../../src/auth');

UI-слой напрямую импортирует из src. Это нарушает разделение слоёв: UI должен ходить только через api-client.js. switchProfile должен быть обёрнут в /api/v1/profile/switch (или аналогичный эндпоинт) и вызываться через api-client.

6.2 Дублированный код (4 места одновременно)

allClientIds extraction — логика парсинга ClientID из payload повторяется в:

  1. src/auth.js::userFromPayload (handles payload.claims)
  2. ui/index.js::resolveUser
  3. ui/routes/auth.js::POST /login-token
  4. src/routes/oidc.js::fallback при IAM-недоступности

session.user construction — структура объекта дублируется в:

  1. src/routes/oidc.js (IAM-ветка + fallback)
  2. ui/routes/auth.js (IAM-ветка + mock + clientId-debug)

Общий паттерн нарушения: каждый раз когда IAM недоступен, используется вариация "JWT fallback" — и каждый fallback написан по-разному (разные поля, разные условия).

6.3 Отсутствие обработки ошибок

  • ui/routes/entries.js::GET / — если в adminMode api.get('/api/v1/companies') бросает, ошибка рендерится в error.ejs. OK. Но если после этого api.get('/api/v1/entries?company=...') бросает — пользователь видит форму с пустым списком и без явного сообщения (поймал catchres.render('index', { error: 'Ошибка загрузки...' })).
  • switchProfile при ошибке не показывает предупреждение.
  • resolveUser при невалидном токене делает req.session.destroy() и редирект — но не логирует причину на сервере.

6.4 DEV_MODE в production

verifyAnyToken в DEV_MODE декодирует токен без проверки подписи:

if (devMode) {
  const decoded = jwt.decode(token);
  ...
  return decoded;
}

Это нормально для dev. Но DEV_MODE проверяется в initAuth() и передаётся как замкнутое значение в createBearerMiddleware. Если process.env.DEV_MODE изменится после запуска — функция всё равно будет работать со старым значением. Жёсткая защита в initAuth() (NODE_ENV=production && devMode → throw) правильная, но её можно обойти в staging если NODE_ENV не выставлен.

6.5 Таймаут IAM API — одинаковый в 2 местах

fetchIamUser и switchProfile оба ставят req.setTimeout(10000, ...). Если IAM лагает, весь запрос пользователя ждёт 10 секунд. В идеале иметь константу IAM_TIMEOUT_MS в config.js.

6.6 getExportCIDRs без лимита строк

async function getExportCIDRs(companyId = null) {
  // без LIMIT
  return (await pool.query(sql, params)).rows.map(r => r.value_cidr);
}

Admin может вызвать экспорт без ?company — вернёт все CIDR всех компаний. При большом количестве данных это может быть медленным запросом без пагинации.


Сводная таблица приоритетов

# Проблема Критичность Файл
1 OIDC state проверка отключена ` true`
2 client_id подмена при пустом allowedIds 🔴 КРИТ src/api/routes/entries.js
3 Open redirect в /login-token (clientId ветка) 🟠 ВЫСОК ui/routes/auth.js
4 impersonated_by всегда NULL в API-слое 🟠 ВЫСОК src/auth.js, ui/api-client.js
5 isNail хардкод в 3 местах 🟡 СРЕДН ui/index.js, ui/routes/auth.js
6 toggle-admin использует другую формулу чем canAdminMode 🟡 СРЕДН ui/routes/auth.js
7 ui → src циклическая зависимость (switchProfile) 🟡 СРЕДН ui/routes/entries.js
8 buildSessionUser дублируется в 3 местах 🟡 СРЕДН oidc.js, auth.js, ui/index.js
9 switchProfile fallback без уведомления пользователя 🟢 НИЗК ui/routes/entries.js
10 getExportCIDRs без лимита строк 🟢 НИЗК src/queries.js
11 IAM_TIMEOUT_MS не вынесен в config 🟢 НИЗК src/auth.js