fix: перенос ревью app-autotest из polygon-docs в DOCS/
This commit is contained in:
@@ -1,132 +0,0 @@
|
||||
# Code Review: app-autotest v1.2.39 — Соннет
|
||||
|
||||
Дата: 2026-08-03
|
||||
|
||||
---
|
||||
|
||||
## 🔴 Критические
|
||||
|
||||
**1. XSS в instances.js — `o.operation` в innerHTML без `_esc()`**
|
||||
|
||||
instances.js (функция `toggleInstance`):
|
||||
```js
|
||||
opsEl.innerHTML = ops.map(o =>
|
||||
`<button ... onclick="runOp('${o.operation}',${o.svcOperationId})">${o.operation}</button>`
|
||||
).join('');
|
||||
```
|
||||
`o.operation` вставляется **три раза без `_esc()`**: в onclick-атрибут (`'${...}'`), в текст кнопки (`>${...}<`). Если API вернёт `operation = "'; alert(1)//"` — onclick-атрибут ломается. На практике операции — это "modify"/"delete" из Nubes API, но принцип нарушен. `_esc()` определён специально для этого.
|
||||
|
||||
---
|
||||
|
||||
## 🟡 Важные
|
||||
|
||||
**2. Двойной вызов `detect_endpoint()` при каждом HTTP-запросе**
|
||||
|
||||
auth.py — `get_client()` вызывает `detect_endpoint(token)`, и `get_stand()` вызывает `detect_endpoint(token)` независимо. В api_test.py при `POST /api/test`:
|
||||
```python
|
||||
client = get_client() # detect_endpoint() #1
|
||||
...
|
||||
get_client_id(), get_stand() # detect_endpoint() #2
|
||||
```
|
||||
Итого **2 лишних HTTP-запроса к Nubes API** (dev + test) при каждом запросе в облачном режиме. Оба блока `if endpoint in STANDS` срабатывают при стандартной конфигурации.
|
||||
|
||||
**3. `mode` и `polygon_stand` не валидируются при `set_mode`**
|
||||
|
||||
main.py:
|
||||
```python
|
||||
new_mode = request.form.get("mode", "polygon")
|
||||
new_stand = request.form.get("polygon_stand") or request.cookies.get("polygon_stand", "test")
|
||||
```
|
||||
Любое значение пишется в cookie. Если `polygon_stand = "evil/../../../etc"`, то в `get_client()` формируется URL:
|
||||
```
|
||||
polygon_url.replace("/api/v1/svc", "/evil/../../../etc/api/v1/svc")
|
||||
```
|
||||
Путь нормализуется HTTP-клиентом/сервером. Допустимые значения известны: `mode` ∈ {"polygon","cloud"}, `polygon_stand` ∈ {"dev","test","prod"} — надо добавить whitelist.
|
||||
|
||||
**4. В polygon-режиме у всех пользователей одинаковый `client_id`**
|
||||
|
||||
auth.py:
|
||||
```python
|
||||
def get_token():
|
||||
if get_mode() == "polygon":
|
||||
return current_app.config["NUBES_API_TOKEN"] # env-токен
|
||||
```
|
||||
`get_client_id()` тоже парсит env-токен → у всех пользователей в эмуляции одинаковый ClientID. Вся изоляция данных в БД (`runs`, `scenario_runs`, `scenario_definitions`) — по `(client_id, stand)`. Если два пользователя переключатся в polygon-режим — они видят историю и сценарии **друг друга**. Если приложение использует только один человек — не проблема. Если несколько — критично.
|
||||
|
||||
**5. `api_history` — соединение не в `finally`**
|
||||
|
||||
api_test.py:
|
||||
```python
|
||||
try:
|
||||
conn = get_conn()
|
||||
cur = conn.cursor()
|
||||
cur.execute(...)
|
||||
...
|
||||
cur.close()
|
||||
put_conn(conn) # явный return соединения
|
||||
return jsonify(result)
|
||||
except Exception as e:
|
||||
try: cur.close() # NameError если исключение до cur = ...
|
||||
except: pass
|
||||
try: put_conn(conn) # NameError если исключение до conn = ...
|
||||
except: pass
|
||||
return jsonify({"error": str(e)}), 500
|
||||
```
|
||||
Все остальные функции (`api_scenario_run_status`, `api_scenario_status`) используют `finally: put_conn(conn)` — корректный паттерн. Здесь — нет. При ошибке до `cur.close()` соединение возвращается, но `cur` не закрывается. Не утечка (psycopg2 закроет при возврате conn в пул), но несоответствие стилю.
|
||||
|
||||
**6. `stand_name()` — подстрочный поиск**
|
||||
|
||||
http_client.py:
|
||||
```python
|
||||
for name in ("dev", "test"):
|
||||
if name in (endpoint or ""):
|
||||
return name
|
||||
```
|
||||
Если URL содержит "dev" как часть другого слова (например, "development", "devnull"), вернёт неверное значение. URL-то сейчас конкретные, но хрупко.
|
||||
|
||||
---
|
||||
|
||||
## 🟢 Рекомендации
|
||||
|
||||
**7. Двойной отступ в scenario.py**
|
||||
|
||||
scenario.py:
|
||||
```python
|
||||
for i, step in enumerate(steps):
|
||||
step_num = i + 1 # 8 пробелов вместо 4
|
||||
```
|
||||
Весь цикл имеет нестандартный отступ (8 пробелов). Работает корректно (Python), но выглядит как след удалённого `try:` или `with:` блока, который был внутри for.
|
||||
|
||||
**8. `get_real_client()` — мёртвый алиас**
|
||||
|
||||
auth.py: функция идентична `get_client()`, есть только для обратной совместимости. В main.py и других файлах есть вызовы `get_real_client()`. Стоит постепенно заменить на `get_client()` и убрать алиас.
|
||||
|
||||
**9. Нет `UNIQUE` на `runs.op_uid`**
|
||||
|
||||
init_db.py: ручной UPSERT в save_run.py (UPDATE → INSERT) предполагает уникальность `op_uid`. Индекса нет. При маловероятной гонке двух параллельных `save_run` с одним `op_uid` — оба сделают UPDATE (rowcount=0) → оба сделают INSERT → дубль. Добавить `CREATE UNIQUE INDEX IF NOT EXISTS idx_runs_op_uid ON runs (op_uid) WHERE op_uid IS NOT NULL`.
|
||||
|
||||
**10. Ротация лога: `flock(LOCK_UN)` перед flush буфера**
|
||||
|
||||
api_test.py:
|
||||
```python
|
||||
f.write(rest)
|
||||
fcntl.flock(f, fcntl.LOCK_UN) # лок снят, но буфер ещё не сброшен
|
||||
```
|
||||
Python-буфер сбрасывается при закрытии файла (`with`-блок), но лок уже снят. Другой воркер может прочитать неполные данные. Добавить `f.flush()` перед `LOCK_UN`.
|
||||
|
||||
---
|
||||
|
||||
## Итого по приоритетам
|
||||
|
||||
| # | Файл | Проблема | Серьёзность |
|
||||
|---|------|----------|-------------|
|
||||
| 1 | `static/js/instances.js` | `o.operation` в innerHTML без `_esc()` | 🔴 |
|
||||
| 2 | `api/auth.py` | Двойной `detect_endpoint()` на запрос | 🟡 |
|
||||
| 3 | `routes/main.py` | `mode`/`polygon_stand` без whitelist-валидации | 🟡 |
|
||||
| 4 | `api/auth.py` | Shared `client_id` в polygon-режиме | 🟡 |
|
||||
| 5 | `routes/api_test.py` | `api_history` без `finally` | 🟡 |
|
||||
| 6 | `api/http_client.py` | `stand_name()` substring match | 🟡 |
|
||||
| 7 | `operations/scenario.py` | Двойной отступ в for-цикле | 🟢 |
|
||||
| 8 | `api/auth.py` | `get_real_client()` мёртвый алиас | 🟢 |
|
||||
| 9 | `db/init_db.py` | Нет UNIQUE индекса на `runs.op_uid` | 🟢 |
|
||||
| 10 | `routes/api_test.py` | `flock(LOCK_UN)` перед flush в ротации | 🟢 |
|
||||
Reference in New Issue
Block a user