133 lines
7.2 KiB
Markdown
133 lines
7.2 KiB
Markdown
# 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 в ротации | 🟢 |
|