diff --git a/HISTORY/code-review-results.md b/HISTORY/code-review-results.md new file mode 100644 index 0000000..63f5878 --- /dev/null +++ b/HISTORY/code-review-results.md @@ -0,0 +1,134 @@ +# Code Review — Соннет (2026-08-08) + +## Файлы ревью +- `server/main.go` — HTTP, S3, роуты +- `server/gpg_key.go` — GPG-ключи +- `server/docs.go` — статика из S3 +- `Dockerfile` — сборка + +--- + +## 1. `defer` внутри `if err == nil` — ✅ OK + +```go +if err == nil { + defer shasumsObj.Close() // выполнится при выходе из ФУНКЦИИ, не из if +} +``` +Не утечка. `defer` всегда выполняется при выходе из функции. `shasumsObj` открывается один раз на вызов и закрывается при возврате — правильный паттерн. + +--- + +## 2. `readyzHandler` без проверки S3 — ⚠️ надо чинить + +```go +// Проверить доступность S3 ← комментарий, не реализовано +w.Write([]byte("ok")) +``` +K8s readiness probe всегда 200 — даже если S3 недоступен. Pod остаётся в rotation с неработающим S3. + +**Фикс:** `s3Client.BucketExists(ctx, bucketName)` + +--- + +## 3. Версии не сортируются — ⚠️ надо чинить + +`seenVersions` — map, итерация недетерминирована. Terraform может некорректно выбрать последнюю версию. + +**Фикс:** `sort.Slice` по семверу. + +--- + +## 4. `proxyHandler` без лимитов — 🔴 КРИТИЧНО + +### 4a. Размер/таймаут — некритично для внутреннего сервиса. + +### 4b. SSRF / Authorization bypass — КРИТИЧНО + +```go +bucket := r.URL.Query().Get("bucket") // ← принимает ЛЮБОЙ bucket! +key := r.URL.Query().Get("key") +``` +Кто знает URL сервера — может читать объекты из любого бакета в том же S3. + +**Фикс:** захардкодить `bucket = bucketName`, игнорировать параметр `bucket` из запроса. `key` валидировать что начинается с разрешённого префикса. + +--- + +## 5. HTML хардкод — ✅ OK + +```go +fmt.Fprintf(w, `...` + VERSION + `...`) +``` +Для крошечной страницы приемлемо. `VERSION` сейчас `"1.0.2"` — безопасно. + +--- + +## 6. Graceful Shutdown — ⚠️ желательно + +```go +log.Fatal(http.ListenAndServe(":"+port, nil)) +``` +SIGTERM убивает процесс мгновенно. In-flight download обрывается → Terraform-клиент теряет кэш. + +**Фикс:** +```go +srv := &http.Server{Addr: ":" + port} +go srv.ListenAndServe() +// signal.NotifyContext + srv.Shutdown(ctx) +``` + +--- + +## 7. Индентация в docs.go — ✅ косметика + +Смешаны табы и пробелы. Go компилирует нормально, но `gofmt -w docs.go` переформатирует полностью. Если CI проверяет `gofmt -l` — упадёт. + +--- + +## 8. GPG-ключи в коде — ✅ OK + +Это **публичные** ключи — их задача быть известными. Смена ключа = rebuild + redeploy — для редкого события приемлемо. + +--- + +## 9. `go mod init` + `go mod tidy` в Dockerfile — 🔴 КРИТИЧНО + +```dockerfile +RUN go mod init tf-registry && go mod tidy && ... +``` +Каждая сборка качает зависимости из интернета. Если proxy недоступен или версия изменилась — сборка сломается. + +**Фикс:** добавить `go.mod` + `go.sum` в репо (`server/go.mod`, `server/go.sum`). +```dockerfile +COPY server/go.mod server/go.sum ./ +RUN go mod download +COPY server/ . +RUN CGO_ENABLED=0 go build -o registry . +``` + +--- + +## 10. Общая архитектура и безопасность + +| Проблема | Серьёзность | Статус | +|---|---|---| +| `proxyHandler` принимает любой `bucket` | 🔴 Высокая | SSRF — фикс п.4b | +| Нет аутентификации | 🟡 Средняя | Внутренний сервис | +| `key` не валидируется | 🟡 Средняя | `../` MinIO отклонит, но лучше проверять | +| JSON-ошибки не логируются | 🟢 Низкая | `json.NewEncoder` без проверки ошибок | +| Нет `go.sum` | 🟡 Средняя | Нерепродуцируемые сборки | +| `readyz` всегда 200 | 🟡 Средняя | см. п.2 | + +--- + +## Приоритеты к исправлению + +| # | Что | Серьёзность | +|---|---|---| +| 1 | `proxyHandler` — убрать параметр `bucket` из запроса | 🔴 SSRF | +| 2 | `go.sum` в репо | 🔴 Воспроизводимость | +| 3 | `readyz` — реальная проверка S3 | 🟡 | +| 4 | Сортировка версий | 🟡 | +| 5 | Graceful shutdown | 🟡 | +| 6 | `gofmt` docs.go | 🟢 |