← [Раздел 05](README.md) · [Главная](../README.md)

# Security code review

## Цель

Освоить **security-focused code review**: что искать в pull request помимо стиля и логики, как использовать чеклисты и автоматизацию, и когда эскалировать к AppSec.

## Предварительно

- [bezopasnye-trebovaniya.md](bezopasnye-trebovaniya.md) — знаете, какие требования должны быть в коде.
- Базово: pull request, diff, merge.

## Время

~30 минут чтения + практика на одном PR (30–60 мин)

---

## Code review vs security code review

| Обычный review | Security review |
|----------------|-----------------|
| Читаемость, архитектура | AuthN/AuthZ, injection, secrets |
| Performance | Unsafe defaults, crypto misuse |
| Tests for happy path | Abuse cases, trust boundaries |

Security review **не заменяет** SAST/DAST — дополняет там, где tools слепы (бизнес-логика, IDOR).

## Когда делать security review

| Обязательно | Желательно |
|-------------|------------|
| Auth / session / crypto | Новые public endpoints |
| Payment, PII processing | Изменения в middleware |
| File upload / SSRF-prone code | Dependency major bumps |
| Security bug fix | Config / Dockerfile changes |

## Процесс (lite)

```
PR opened
   │
   ├── CI: SAST, SCA, secrets scan (авто)
   │
   ├── Author: self-review по security checklist
   │
   ├── Reviewer: diff + checklist + threat-linked AC
   │
   └── Critical finding? → block merge / AppSec consult
```

Подробнее о gates — [quality-gates-v-ci.md](../06-bezopasnost-koda/quality-gates-v-ci.md).

## OWASP Top 10 как lens (не как библия)

Используйте Top 10 как **вопросы к diff**, не как галочку «мы compliant».

| Категория | Вопрос к PR |
|-----------|-------------|
| Broken Access Control | Проверяется ли ownership resource? |
| Cryptographic Failures | Где secrets? Какой алгоритм? |
| Injection | User input попадает в SQL/shell/HTML? |
| Insecure Design | Есть ли rate limit для этой операции? |
| Security Misconfiguration | Debug=true? Default creds? |
| Vulnerable Components | Обновлены ли deps с CVE? |
| Auth Failures | Session fixation? Weak recovery? |
| Integrity failures | CI/CD pipeline защищён? |
| Logging failures | Логируются security events? |
| SSRF | URL из input fetch'ится сервером? |

## Security checklist для reviewer

### Authentication & sessions

- [ ] Passwords hashed (bcrypt/argon2), не MD5/SHA1 alone
- [ ] Session ID unpredictable, HttpOnly, Secure cookies
- [ ] Logout invalidates server-side session (если applicable)

### Authorization

- [ ] Каждый endpoint проверяет permission, не только authentication
- [ ] IDOR: `{id}` из URL сверяется с current user
- [ ] Admin routes отдельно защищены (role + optional IP allowlist)

### Input & output

- [ ] Parameterized queries / ORM, не raw SQL concat
- [ ] Output encoding для HTML (XSS)
- [ ] File upload: type/size limits, storage outside webroot

### Secrets & config

- [ ] Нет hardcoded tokens, API keys
- [ ] `.env` в `.gitignore`; примеры — `.env.example` без secrets
- [ ] Production config не в repo

### Errors & logging

- [ ] Generic errors клиенту; details только в server logs
- [ ] Passwords/tokens не пишутся в logs
- [ ] Audit events для sensitive actions

## Примеры комментариев в PR (шаблоны)

**Blocker:**
> 🔴 **Security:** Endpoint `GET /users/{id}` не проверяет, что `{id}` принадлежит caller. Risk: IDOR (OWASP A01). Нужна проверка `user.id == params.id` или RBAC.

**Suggestion:**
> 🟡 **Security:** Consider rate limiting on `/password-reset` — threat T1 из threat model.

**Question:**
> ❓ Логируется ли `role_changed` для audit? Requirement AC-S5.

## Что автоматизировать, что нет

| Хорошо для automation | Нужен human review |
|-----------------------|-------------------|
| Secrets in git | Business logic auth bypass |
| Known vulnerable libs | «Можно ли обойти workflow?» |
| SQLi patterns (частично) | Complex IDOR across services |
| Insecure crypto API usage | Threat-specific abuse cases |

## Роли и SLA

| Severity finding in review | Действие | SLA (пример) |
|----------------------------|----------|--------------|
| Critical | Block merge | Fix before merge |
| High | Block или exception ticket | 48h |
| Medium | Merge с follow-up issue | Sprint |
| Low / Info | Backlog | Best effort |

## Обучение команды без токсичности

| Плохо | Хорошо |
|-------|--------|
| «Ты опять сломал security» | «Здесь типичный IDOR pattern, вот doc и пример fix» |
| 200 комментариев на первый PR | Pair review + shared checklist |
| Security только gatekeeper | Office hours, шаблоны secure snippets |

## Secure snippet library

Заведите внутренний repo/wiki: **«как у нас правильно»** — login, file upload, JWT validate. Reviewer ссылается на snippet, не переписывает архитектуру в каждом PR.

## Связь с compliance

Security review notes могут быть evidence для SOC2/ISO:

- PR link + checklist tick + approver
- Exception register с risk acceptance

---

## Самопроверка

1. Назовите 3 вещи, которые SAST не поймает, но reviewer должен.
2. Чем IDOR отличается от «нет авторизации»?
3. Пройдите checklist на воображаемом PR «добавлен export CSV всех users» — что блокируете?
4. Когда эскалировать в AppSec?

## Дальше

Раздел 05 завершён. Переходите к [разделу 06 — Безопасность кода](../06-bezopasnost-koda/README.md).