From 8cbbe52316e0877e4d8cc267ff467c3eb870d3a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9CNaeel=E2=80=9D?= Date: Thu, 4 Jun 2026 15:26:26 +0300 Subject: [PATCH] =?UTF-8?q?fix:=20safeReturn=20=D0=B4=D0=BB=D1=8F=20=D0=B2?= =?UTF-8?q?=D1=81=D0=B5=D1=85=20=D1=80=D0=B5=D0=B4=D0=B8=D1=80=D0=B5=D0=BA?= =?UTF-8?q?=D1=82=D0=BE=D0=B2=20(security)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ui/routes/auth.js: safeReturn() для returnTo в POST /login и /login-token - src/routes/oidc.js: safeLocal() для oidcReturnTo в /callback --- docs/production-audit.md | 64 ++++++++++++++++++++++++++++++++++++++++ src/routes/oidc.js | 13 +++++++- ui/routes/auth.js | 5 ++-- 3 files changed, 79 insertions(+), 3 deletions(-) create mode 100644 docs/production-audit.md diff --git a/docs/production-audit.md b/docs/production-audit.md new file mode 100644 index 0000000..2d8ee10 --- /dev/null +++ b/docs/production-audit.md @@ -0,0 +1,64 @@ +# Production Audit: ipwhitelist-app + +Дата ревью: 2026-06-04 +Основа: `prompt-review.txt` + +## Общее заключение + +В production отдавать нельзя без доработок. Код в целом собран аккуратно, но есть критичные риски по доступу к `/export`, redirect-логике и IAM fallback-пути. Документация частично актуальна, но для DevOps сейчас недостаточно согласована и содержит противоречия между старым и новым описанием аутентификации. + +## Найденные проблемы + +### Critical + +1. Публичный `/export` в `server.js` открыт без авторизации и без сетевого allowlist. Это противоречит безопасной production-модели и делает выдачу whitelist доступной любому, кто достучится до приложения. +2. Логика `returnTo`/redirect допускает open redirect. Значение берётся из запроса и дальше уходит в `res.redirect()` без нормализации. Это нужно закрывать через `safeReturn()` или эквивалентный allowlist локальных путей. + +### Major + +1. IAM fallback работает слишком мягко: при ошибке `fetchIamUser()` приложение silently переходит на JWT claims. Это может привести к неверному определению компании, роли администратора или активного профиля. +2. Документация не сведена в один актуальный источник. `README.md`, `docs/devops-deploy.md`, `docs/ТЗ-реализация.md`, `docs/keycloak-auth-reference.md` и `docs/iam-integration.md` частично дублируют и частично противоречат друг другу. +3. Описание `/export` в документации и в коде расходится. В одном месте маршрут показан как публичный, в другом — как Bearer-protected. Для DevOps это опасно: можно ошибиться на этапе деплоя. + +### Minor + +1. CSP сейчас рабочий, но ослаблен `unsafe-inline` для скриптов и стилей. Для текущего inline-heavy UI это ожидаемо, но защита от XSS ограниченная. +2. Есть устаревшие или лишние пояснения вокруг mock-режима и IAM, из-за чего сложно понять, какой путь является production и какой — fallback для тестов. + +## Понятность DevOps + +Оценка: 4/10 + +Почему так низко: + +- есть хороший стартовый deploy guide, но он смешан с историческими и legacy-документами; +- не все переменные окружения и режимы описаны в одном месте; +- есть расхождения по IAM, роли администратора, multi-company и `/export`. + +## Чистота кода + +Оценка: 6/10 + +Плюсы: + +- логика разделена на слои: auth, API, UI, queries, validators; +- есть нормализация CIDR, защита от дубликатов и пересечений, soft delete, аудит; +- middleware и роутизация читаются достаточно последовательно. + +Минусы: + +- fallback-пути слишком терпимые и могут скрыть проблемы интеграции; +- redirect-логика требует жёсткой нормализации; +- часть безопасности опирается на inline-механики и не доведена до строгого production-профиля. + +## Что исправить перед продом + +1. Закрыть `/export` по умолчанию и включать его только через явную production-политику доступа. +2. Нормализовать все `returnTo`/redirect-цели через `safeReturn()` или аналогичный allowlist. +3. Перевести IAM fallback на fail-closed поведение либо явно документировать, при каких условиях он допустим. +4. Свести документацию в один актуальный production runbook и пометить старые документы как legacy. +5. Добавить тесты на open redirect, IAM-fallback и доступ к `/export`. + +## Краткий итог + +Код готов к дальнейшей доводке, но до production не дотягивает из-за вопросов безопасности и несогласованной документации. Если нужен рабочий контур для DevOps, сначала надо закрыть доступ к `/export`, убрать open redirect и привести IAM-путь к предсказуемой модели. \ No newline at end of file diff --git a/src/routes/oidc.js b/src/routes/oidc.js index ca2691b..d62aee4 100644 --- a/src/routes/oidc.js +++ b/src/routes/oidc.js @@ -88,7 +88,7 @@ function createRouter({ auth, doubleCsrfProtection, generateCsrfToken, authLimit }; } - const returnTo = req.session.oidcReturnTo || '/'; + const returnTo = safeLocal(req.session.oidcReturnTo) || '/'; delete req.session.oidcReturnTo; res.redirect(returnTo); } catch (e) { @@ -109,4 +109,15 @@ function createRouter({ auth, doubleCsrfProtection, generateCsrfToken, authLimit return router; } +/** + * Валидирует redirect-цель: разрешает только локальные пути. + * Защита от open redirect: //evil.com и https://evil.com → '/'. + */ +function safeLocal(target) { + if (typeof target === 'string' && target.startsWith('/') && !target.startsWith('//')) { + return target; + } + return '/'; +} + module.exports = { createRouter }; diff --git a/ui/routes/auth.js b/ui/routes/auth.js index f178923..5fc32d6 100644 --- a/ui/routes/auth.js +++ b/ui/routes/auth.js @@ -15,6 +15,7 @@ */ const { Router } = require('express'); +const { safeReturn } = require('../../src/auth'); function createRouter({ auth, MOCK_USERS, authLimiter }) { const router = Router(); @@ -61,7 +62,7 @@ function createRouter({ auth, MOCK_USERS, authLimiter }) { companyName: user.companyName || user.clientId, isAdmin: user.isAdmin || user.role === 'admin', }; - res.redirect(returnTo || '/'); + res.redirect(safeReturn(returnTo) || '/'); }); // GET /login-token — редирект на /login (форма вставки токена там же) @@ -130,7 +131,7 @@ function createRouter({ auth, MOCK_USERS, authLimiter }) { isAdmin: activeClientId === (process.env.ADMIN_CLIENT_ID || 'WZ01112'), }; } - return res.redirect(returnTo || '/'); + return res.redirect(safeReturn(returnTo) || '/'); } // ── Вход по clientId (отладка, mock) ───────────────────────────────────