fix: safeReturn для всех редиректов (security)
- ui/routes/auth.js: safeReturn() для returnTo в POST /login и /login-token - src/routes/oidc.js: safeLocal() для oidcReturnTo в /callback
This commit is contained in:
@@ -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-путь к предсказуемой модели.
|
||||||
+12
-1
@@ -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;
|
delete req.session.oidcReturnTo;
|
||||||
res.redirect(returnTo);
|
res.redirect(returnTo);
|
||||||
} catch (e) {
|
} catch (e) {
|
||||||
@@ -109,4 +109,15 @@ function createRouter({ auth, doubleCsrfProtection, generateCsrfToken, authLimit
|
|||||||
return router;
|
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 };
|
module.exports = { createRouter };
|
||||||
|
|||||||
+3
-2
@@ -15,6 +15,7 @@
|
|||||||
*/
|
*/
|
||||||
|
|
||||||
const { Router } = require('express');
|
const { Router } = require('express');
|
||||||
|
const { safeReturn } = require('../../src/auth');
|
||||||
|
|
||||||
function createRouter({ auth, MOCK_USERS, authLimiter }) {
|
function createRouter({ auth, MOCK_USERS, authLimiter }) {
|
||||||
const router = Router();
|
const router = Router();
|
||||||
@@ -61,7 +62,7 @@ function createRouter({ auth, MOCK_USERS, authLimiter }) {
|
|||||||
companyName: user.companyName || user.clientId,
|
companyName: user.companyName || user.clientId,
|
||||||
isAdmin: user.isAdmin || user.role === 'admin',
|
isAdmin: user.isAdmin || user.role === 'admin',
|
||||||
};
|
};
|
||||||
res.redirect(returnTo || '/');
|
res.redirect(safeReturn(returnTo) || '/');
|
||||||
});
|
});
|
||||||
|
|
||||||
// GET /login-token — редирект на /login (форма вставки токена там же)
|
// GET /login-token — редирект на /login (форма вставки токена там же)
|
||||||
@@ -130,7 +131,7 @@ function createRouter({ auth, MOCK_USERS, authLimiter }) {
|
|||||||
isAdmin: activeClientId === (process.env.ADMIN_CLIENT_ID || 'WZ01112'),
|
isAdmin: activeClientId === (process.env.ADMIN_CLIENT_ID || 'WZ01112'),
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
return res.redirect(returnTo || '/');
|
return res.redirect(safeReturn(returnTo) || '/');
|
||||||
}
|
}
|
||||||
|
|
||||||
// ── Вход по clientId (отладка, mock) ───────────────────────────────────
|
// ── Вход по clientId (отладка, mock) ───────────────────────────────────
|
||||||
|
|||||||
Reference in New Issue
Block a user