Feature: Add code quality audit tools and safe helpers
- Created CODE_QUALITY_CHECKLIST.md with comprehensive guide - Added audit_code_patterns.sh for automated code scanning - Created common/request_utils.py with safe_int(), safe_bool(), etc. - Fixed unsafe request.data access in notifications/api/views.py - Updated ldap_password_mixin.py docstring with safe example Audit found and documented: - 10 unsafe request.data['key'] accesses - 8+ unsafe int() conversions without try/except - Multiple .strip() calls without None checks
This commit is contained in:
lizerk
parent
2943299c08
commit
2ef43e7177
@ -25,7 +25,10 @@ class LdapPasswordMixin:
|
||||
class ChangePasswordAPIView(LdapPasswordMixin, APIView):
|
||||
def post(self, request):
|
||||
user = request.user
|
||||
new_password = request.data['new_password']
|
||||
# Безопасный доступ:
|
||||
new_password = request.data.get('new_password')
|
||||
if not new_password:
|
||||
return Response({'error': 'Password required'}, 400)
|
||||
|
||||
# Меняем пароль в БД
|
||||
user.set_password(new_password)
|
||||
|
||||
115
backend/common/request_utils.py
Normal file
115
backend/common/request_utils.py
Normal file
@ -0,0 +1,115 @@
|
||||
"""Утилиты для безопасной обработки данных запросов."""
|
||||
from typing import Any, Optional, Union
|
||||
|
||||
|
||||
def safe_int(value: Any, default: int = 0) -> int:
|
||||
"""Безопасно конвертирует значение в int.
|
||||
|
||||
Args:
|
||||
value: Значение для конверсии (может быть str, int, None, ...)
|
||||
default: Значение по умолчанию при ошибке конверсии
|
||||
|
||||
Returns:
|
||||
int: Сконвертированное значение или default
|
||||
|
||||
Examples:
|
||||
>>> safe_int('123')
|
||||
123
|
||||
>>> safe_int('abc', default=1)
|
||||
1
|
||||
>>> safe_int(None, default=10)
|
||||
10
|
||||
>>> safe_int('', default=5)
|
||||
5
|
||||
"""
|
||||
if value is None or value == '':
|
||||
return default
|
||||
|
||||
try:
|
||||
return int(value)
|
||||
except (ValueError, TypeError):
|
||||
return default
|
||||
|
||||
|
||||
def safe_bool(value: Any, default: bool = False) -> bool:
|
||||
"""Безопасно конвертирует значение в bool.
|
||||
|
||||
Поддерживает строки: 'true', 'false', '1', '0', 'yes', 'no'
|
||||
|
||||
Args:
|
||||
value: Значение для конверсии
|
||||
default: Значение по умолчанию при ошибке
|
||||
|
||||
Returns:
|
||||
bool: Сконвертированное значение или default
|
||||
"""
|
||||
if value is None:
|
||||
return default
|
||||
|
||||
if isinstance(value, bool):
|
||||
return value
|
||||
|
||||
if isinstance(value, (int, float)):
|
||||
return bool(value)
|
||||
|
||||
if isinstance(value, str):
|
||||
value_lower = value.lower().strip()
|
||||
if value_lower in ('true', '1', 'yes', 'on'):
|
||||
return True
|
||||
if value_lower in ('false', '0', 'no', 'off', ''):
|
||||
return False
|
||||
|
||||
return default
|
||||
|
||||
|
||||
def safe_float(value: Any, default: float = 0.0) -> float:
|
||||
"""Безопасно конвертирует значение в float.
|
||||
|
||||
Args:
|
||||
value: Значение для конверсии
|
||||
default: Значение по умолчанию при ошибке
|
||||
|
||||
Returns:
|
||||
float: Сконвертированное значение или default
|
||||
"""
|
||||
if value is None or value == '':
|
||||
return default
|
||||
|
||||
try:
|
||||
return float(value)
|
||||
except (ValueError, TypeError):
|
||||
return default
|
||||
|
||||
|
||||
def safe_get_data(
|
||||
data: Union[dict, 'QueryDict'],
|
||||
key: str,
|
||||
default: Any = None,
|
||||
required: bool = False,
|
||||
) -> Any:
|
||||
"""Безопасно извлекает значение из request.data.
|
||||
|
||||
Args:
|
||||
data: request.data (может быть dict или QueryDict)
|
||||
key: Ключ для извлечения
|
||||
default: Значение по умолчанию
|
||||
required: Если True, вызовет KeyError при отсутствии ключа
|
||||
|
||||
Returns:
|
||||
Значение или default
|
||||
|
||||
Raises:
|
||||
KeyError: Если required=True и ключа нет
|
||||
|
||||
Examples:
|
||||
>>> safe_get_data({'name': 'John'}, 'name')
|
||||
'John'
|
||||
>>> safe_get_data({}, 'age', default=18)
|
||||
18
|
||||
>>> safe_get_data({}, 'email', required=True)
|
||||
KeyError: 'email'
|
||||
"""
|
||||
if required and key not in data:
|
||||
raise KeyError(f"Required field '{key}' is missing")
|
||||
|
||||
return data.get(key, default)
|
||||
235
backend/docs/guides/CODE_QUALITY_CHECKLIST.md
Normal file
235
backend/docs/guides/CODE_QUALITY_CHECKLIST.md
Normal file
@ -0,0 +1,235 @@
|
||||
# Контрольный список качества кода (Code Quality Checklist)
|
||||
|
||||
## Обнаруженные проблемы и решения
|
||||
|
||||
### 1. 🔴 Критично: Небезопасный доступ к `request.data`
|
||||
|
||||
**Проблема:**
|
||||
```python
|
||||
value = request.data['key'] # ❌ KeyError если ключа нет
|
||||
```
|
||||
|
||||
**Решение:**
|
||||
```python
|
||||
value = request.data.get('key') # ✅ Вернёт None
|
||||
value = request.data.get('key', default) # ✅ С дефолтом
|
||||
```
|
||||
|
||||
**Найдено в:**
|
||||
- `notifications/api/views.py` (8 мест)
|
||||
- `common/ldap_password_mixin.py` (1 место)
|
||||
|
||||
---
|
||||
|
||||
### 2. 🟡 Важно: QueryDict vs dict несовместимость
|
||||
|
||||
**Проблема:**
|
||||
```python
|
||||
# DRF request.data может быть dict (JSON) или QueryDict (form data)
|
||||
data = request.data.dict() # ❌ dict не имеет метода .dict()
|
||||
```
|
||||
|
||||
**Решение:**
|
||||
```python
|
||||
data = request.data
|
||||
result = data.dict() if hasattr(data, 'dict') else dict(data) # ✅
|
||||
```
|
||||
|
||||
**Статус:** ✅ Исправлено в `api/v1/employees/views/employees.py`
|
||||
|
||||
---
|
||||
|
||||
### 3. 🟡 Важно: Небезопасный `int()` без обработки
|
||||
|
||||
**Проблема:**
|
||||
```python
|
||||
page = int(request.GET.get('page', 1)) # ❌ ValueError если 'abc'
|
||||
```
|
||||
|
||||
**Решение:**
|
||||
```python
|
||||
# Вариант 1: try/except
|
||||
try:
|
||||
page = int(request.GET.get('page', 1))
|
||||
except (ValueError, TypeError):
|
||||
page = 1
|
||||
|
||||
# Вариант 2: Helper функция
|
||||
def safe_int(value, default=0):
|
||||
try:
|
||||
return int(value)
|
||||
except (ValueError, TypeError):
|
||||
return default
|
||||
|
||||
page = safe_int(request.GET.get('page'), 1)
|
||||
```
|
||||
|
||||
**Найдено в:**
|
||||
- `communications/api/viewsets.py` (4 места)
|
||||
- `notifications/api/views.py` (2 места)
|
||||
- `api/v1/search/views.py` (1 место)
|
||||
- `api/v1/procurement/views.py` (1 место)
|
||||
|
||||
---
|
||||
|
||||
### 4. 🟢 Средне: `.strip()` без проверки на None
|
||||
|
||||
**Проблема:**
|
||||
```python
|
||||
text = some_value.strip() # ❌ AttributeError если None
|
||||
```
|
||||
|
||||
**Решение:**
|
||||
```python
|
||||
text = (some_value or '').strip() # ✅
|
||||
text = str(some_value).strip() if some_value else '' # ✅
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Автоматизация проверки
|
||||
|
||||
### Скрипт для аудита
|
||||
|
||||
Запуск:
|
||||
```bash
|
||||
./scripts/audit_code_patterns.sh
|
||||
```
|
||||
|
||||
### Pre-commit hooks
|
||||
|
||||
Добавить в `.pre-commit-config.yaml`:
|
||||
```yaml
|
||||
repos:
|
||||
- repo: local
|
||||
hooks:
|
||||
- id: check-unsafe-data-access
|
||||
name: Check unsafe request.data access
|
||||
entry: bash -c 'grep -rn "request\.data\[" --include="*.py" . | grep -v "# noqa" && exit 1 || exit 0'
|
||||
language: system
|
||||
pass_filenames: false
|
||||
```
|
||||
|
||||
### Линтеры
|
||||
|
||||
**Flake8** с плагинами:
|
||||
```bash
|
||||
pip install flake8 flake8-bugbear flake8-comprehensions
|
||||
flake8 --max-line-length=100 --extend-ignore=E203,W503
|
||||
```
|
||||
|
||||
**Pylint**:
|
||||
```bash
|
||||
pip install pylint
|
||||
pylint --disable=C0111,C0103 employees/
|
||||
```
|
||||
|
||||
**MyPy** (статическая типизация):
|
||||
```bash
|
||||
pip install mypy django-stubs djangorestframework-stubs
|
||||
mypy --config-file=mypy.ini employees/
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Тесты для обнаружения проблем
|
||||
|
||||
### Unit-тесты для API
|
||||
|
||||
```python
|
||||
from rest_framework.test import APITestCase
|
||||
|
||||
class EmployeeAPITestCase(APITestCase):
|
||||
def test_patch_with_json(self):
|
||||
"""Тест PATCH с JSON payload"""
|
||||
response = self.client.patch(
|
||||
'/api/v1/employees/1/',
|
||||
{'first_name': 'John'},
|
||||
format='json'
|
||||
)
|
||||
self.assertEqual(response.status_code, 200)
|
||||
|
||||
def test_patch_with_form_data(self):
|
||||
"""Тест PATCH с form data"""
|
||||
response = self.client.patch(
|
||||
'/api/v1/employees/1/',
|
||||
{'first_name': 'John'},
|
||||
format='multipart'
|
||||
)
|
||||
self.assertEqual(response.status_code, 200)
|
||||
|
||||
def test_missing_required_field(self):
|
||||
"""Тест обработки отсутствующего поля"""
|
||||
response = self.client.patch(
|
||||
'/api/v1/employees/1/',
|
||||
{},
|
||||
format='json'
|
||||
)
|
||||
self.assertEqual(response.status_code, 400)
|
||||
|
||||
def test_invalid_int_parameter(self):
|
||||
"""Тест невалидного целочисленного параметра"""
|
||||
response = self.client.get('/api/v1/employees/?page=abc')
|
||||
# Должен вернуть 400 или использовать дефолт
|
||||
self.assertIn(response.status_code, [200, 400])
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Рекомендации по code review
|
||||
|
||||
### Чек-лист для PR
|
||||
|
||||
- [ ] Нет прямого доступа `request.data['key']` — только `.get()`
|
||||
- [ ] Все `int()` обёрнуты в try/except или используют safe_int()
|
||||
- [ ] Проверка типа для QueryDict/dict перед `.dict()`
|
||||
- [ ] `.strip()` используется с проверкой на None
|
||||
- [ ] Есть тесты для новых API endpoints
|
||||
- [ ] Обработаны все edge cases (empty, None, invalid)
|
||||
|
||||
### GitHub Actions / CI
|
||||
|
||||
Добавить в `.github/workflows/quality.yml`:
|
||||
```yaml
|
||||
name: Code Quality
|
||||
|
||||
on: [push, pull_request]
|
||||
|
||||
jobs:
|
||||
audit:
|
||||
runs-on: ubuntu-latest
|
||||
steps:
|
||||
- uses: actions/checkout@v3
|
||||
- name: Run code audit
|
||||
run: |
|
||||
cd backend
|
||||
bash scripts/audit_code_patterns.sh
|
||||
# Fail если найдены критичные проблемы
|
||||
! grep -rn "request\.data\[" --include="*.py" . | grep -v "# noqa"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Приоритеты исправления
|
||||
|
||||
1. **Срочно (в течение недели):**
|
||||
- Исправить все `request.data['key']` → `.get('key')`
|
||||
- Добавить обработку ValueError для int()
|
||||
|
||||
2. **Важно (в течение месяца):**
|
||||
- Настроить pre-commit hooks
|
||||
- Добавить mypy для статической типизации
|
||||
- Написать тесты для критичных API endpoints
|
||||
|
||||
3. **Желательно:**
|
||||
- Настроить CI/CD с автопроверкой
|
||||
- Провести code review всех найденных мест
|
||||
- Документировать паттерны в wiki
|
||||
|
||||
---
|
||||
|
||||
## Полезные ссылки
|
||||
|
||||
- [DRF Best Practices](https://www.django-rest-framework.org/api-guide/requests/)
|
||||
- [Django Security Checklist](https://docs.djangoproject.com/en/5.0/howto/deployment/checklist/)
|
||||
- [Python Type Hints](https://docs.python.org/3/library/typing.html)
|
||||
@ -341,36 +341,36 @@ def channel_preferences(request):
|
||||
elif request.method == 'PUT':
|
||||
# Обновляем настройки
|
||||
if 'web_enabled' in request.data:
|
||||
prefs.web_enabled = request.data['web_enabled']
|
||||
prefs.web_enabled = request.data.get('web_enabled')
|
||||
|
||||
if 'email_enabled' in request.data:
|
||||
prefs.email_enabled = request.data['email_enabled']
|
||||
prefs.email_enabled = request.data.get('email_enabled')
|
||||
|
||||
if 'email_frequency' in request.data:
|
||||
frequency = request.data['email_frequency']
|
||||
frequency = request.data.get('email_frequency')
|
||||
if frequency in ['instant', 'daily', 'weekly', 'disabled']:
|
||||
prefs.email_frequency = frequency
|
||||
|
||||
if 'push_enabled' in request.data:
|
||||
prefs.push_enabled = request.data['push_enabled']
|
||||
prefs.push_enabled = request.data.get('push_enabled')
|
||||
|
||||
if 'dnd_enabled' in request.data:
|
||||
prefs.dnd_enabled = request.data['dnd_enabled']
|
||||
prefs.dnd_enabled = request.data.get('dnd_enabled')
|
||||
|
||||
if 'dnd_start_time' in request.data:
|
||||
from datetime import datetime
|
||||
time_str = request.data['dnd_start_time']
|
||||
time_str = request.data.get('dnd_start_time')
|
||||
if time_str:
|
||||
prefs.dnd_start_time = datetime.strptime(time_str, '%H:%M').time()
|
||||
|
||||
if 'dnd_end_time' in request.data:
|
||||
from datetime import datetime
|
||||
time_str = request.data['dnd_end_time']
|
||||
time_str = request.data.get('dnd_end_time')
|
||||
if time_str:
|
||||
prefs.dnd_end_time = datetime.strptime(time_str, '%H:%M').time()
|
||||
|
||||
if 'disabled_verbs' in request.data:
|
||||
prefs.disabled_verbs = request.data['disabled_verbs']
|
||||
prefs.disabled_verbs = request.data.get('disabled_verbs')
|
||||
|
||||
prefs.save()
|
||||
|
||||
|
||||
27
backend/scripts/audit_code_patterns.sh
Executable file
27
backend/scripts/audit_code_patterns.sh
Executable file
@ -0,0 +1,27 @@
|
||||
#!/bin/bash
|
||||
# Аудит потенциально проблемных паттернов в коде
|
||||
|
||||
echo "=== Поиск потенциальных проблем ==="
|
||||
echo ""
|
||||
|
||||
echo "1. Небезопасный доступ к request.data:"
|
||||
grep -rn "request\.data\[" --include="*.py" . | grep -v "# noqa" || echo "✓ Не найдено"
|
||||
echo ""
|
||||
|
||||
echo "2. Использование .dict() без проверки типа:"
|
||||
grep -rn "\.data\.dict()" --include="*.py" . | grep -v "hasattr" || echo "✓ Не найдено"
|
||||
echo ""
|
||||
|
||||
echo "3. Прямое обращение к QueryDict без .get():"
|
||||
grep -rn "request\.POST\[" --include="*.py" . | grep -v "# noqa" || echo "✓ Не найдено"
|
||||
echo ""
|
||||
|
||||
echo "4. Небезопасный int() без try/except:"
|
||||
grep -rn "int(.*request\." --include="*.py" . | head -20
|
||||
echo ""
|
||||
|
||||
echo "5. Отсутствие проверки на None перед .strip():"
|
||||
grep -rn "\.strip()" --include="*.py" . | grep -v "if.*:" | head -10
|
||||
echo ""
|
||||
|
||||
echo "=== Аудит завершен ==="
|
||||
Loading…
x
Reference in New Issue
Block a user