Merge fix/check-status-and-diff-layout: статус при ручной проверке + перенос длинных значений
(A) Ручной check персистит last_check_status (был вечный unknown до планировщика); SetDomainStatus скоуплен по project_id (закрыт IDOR на запись). (B) Длинные значения (DKIM/SPF) переносятся в diff, убран горизонтальный оверфлоу. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BwxdSt4reTm7Dj1oxRvpP3
This commit is contained in:
@@ -0,0 +1,176 @@
|
||||
# Ручной check пишет статус + перенос длинных значений в diff — Implementation Plan
|
||||
|
||||
> **For agentic workers:** REQUIRED SUB-SKILL: superpowers:subagent-driven-development. Steps use `- [ ]`.
|
||||
|
||||
**Goal:** (A) Ручная проверка домена (Recheck / заход на страницу diff) должна обновлять `last_check_status`, а не оставлять `unknown` до прогона планировщика. (B) Длинные значения записей (DKIM-ключ, длинный SPF) должны переноситься в diff-представлении, не ломая вёрстку в горизонтальный оверфлоу.
|
||||
|
||||
**Architecture:** Статус домена (`unknown|in_sync|drift|error`) сейчас пишет только планировщик. Выносим вычисление статуса в `internal/service` (единый источник), и ручной `check`-хендлер тоже персистит его. Планировщик переиспользует те же константы (через алиасы — без переписывания). Фронтовый `DiffView` переносит длинные значения вместо распирания строки.
|
||||
|
||||
**Tech Stack:** Go (chi), React + Vite + Tailwind.
|
||||
|
||||
## Global Constraints
|
||||
|
||||
- Значения статуса — контракт с БД (`domains.last_check_status`, default `unknown`) и фронтом (`StatusBadge`): `unknown|in_sync|drift|error`. Единый источник — `internal/service`.
|
||||
- Ручной check НЕ шлёт уведомления (notify остаётся за планировщиком) — только обновляет статус и историю.
|
||||
- Планировщик остаётся владельцем notify; его поведение и тесты не должны сломаться.
|
||||
- Комментарии в Go — на английском; в web — как в окружающих файлах. TDD. НЕ коммитить реальную сборку `internal/web/dist/*`.
|
||||
|
||||
---
|
||||
|
||||
### Task 1: Backend — ручной check персистит last_check_status
|
||||
|
||||
**Files:**
|
||||
- Create: `internal/service/status.go`, `internal/service/status_test.go`
|
||||
- Modify: `internal/scheduler/scheduler.go` (константы → алиасы service), `internal/api/handlers.go` (handleCheck пишет статус), `internal/api/api.go` (TenantStore += SetDomainStatus)
|
||||
- Test: `internal/api/tenant_test.go` / `api_test.go` (mock TenantStore += SetDomainStatus; тест что check пишет статус)
|
||||
|
||||
**Interfaces:**
|
||||
- Produces: `service.StatusUnknown/StatusInSync/StatusDrift/StatusError`; `service.DeriveStatus(cs diff.Changeset) string`.
|
||||
- Consumes: `diff.Changeset.Actionable()`; `store.SetDomainStatus`.
|
||||
|
||||
- [ ] **Step 1: Тест service.DeriveStatus**
|
||||
|
||||
`internal/service/status_test.go`:
|
||||
```go
|
||||
func TestDeriveStatus(t *testing.T) {
|
||||
// no actionable diffs → in_sync
|
||||
if got := service.DeriveStatus(diff.Changeset{}); got != service.StatusInSync {
|
||||
t.Fatalf("empty: %q", got)
|
||||
}
|
||||
// an actionable prune → drift
|
||||
cs := diff.Changeset{Diffs: []diff.RecordDiff{
|
||||
{Kind: diff.Delete, Type: model.A, Name: "x.example.com.", Actual: &model.Record{Type: model.A, Name: "x.example.com.", Values: []string{"1.1.1.1"}}},
|
||||
}}
|
||||
if got := service.DeriveStatus(cs); got != service.StatusDrift {
|
||||
t.Fatalf("prune: %q", got)
|
||||
}
|
||||
}
|
||||
```
|
||||
(Сверь, что `diff.Delete` на managed-типе попадает в `Actionable()` — по существующим diff-тестам A-delete actionable, NS/SOA нет.) Run: `go test ./internal/service/... -run DeriveStatus` — Ожидание FAIL (нет функции).
|
||||
|
||||
- [ ] **Step 2: service/status.go**
|
||||
|
||||
```go
|
||||
package service
|
||||
|
||||
import "github.com/vasyakrg/dns-autoresolver/internal/diff"
|
||||
|
||||
// Domain check statuses persisted in domains.last_check_status. "unknown" is
|
||||
// the DB default for a domain that has never been checked. Single source of
|
||||
// truth — the scheduler aliases these, the API check handler writes them.
|
||||
const (
|
||||
StatusUnknown = "unknown"
|
||||
StatusInSync = "in_sync"
|
||||
StatusDrift = "drift"
|
||||
StatusError = "error"
|
||||
)
|
||||
|
||||
// DeriveStatus maps a successful check's changeset to a domain status:
|
||||
// actionable drift (managed adds/updates/prunes) → "drift", otherwise
|
||||
// "in_sync". A failed check (provider/loader error) is "error" and handled by
|
||||
// the caller, which has the error in hand.
|
||||
func DeriveStatus(cs diff.Changeset) string {
|
||||
if len(cs.Actionable()) > 0 {
|
||||
return StatusDrift
|
||||
}
|
||||
return StatusInSync
|
||||
}
|
||||
```
|
||||
Run: `go test ./internal/service/... -run DeriveStatus` — Ожидание PASS.
|
||||
|
||||
- [ ] **Step 3: scheduler константы → алиасы (единый источник)**
|
||||
|
||||
`internal/scheduler/scheduler.go` — заменить локальный блок `const ( StatusUnknown = "unknown" ... )` на алиасы (добавить импорт `internal/service`):
|
||||
```go
|
||||
const (
|
||||
StatusUnknown = service.StatusUnknown
|
||||
StatusInSync = service.StatusInSync
|
||||
StatusDrift = service.StatusDrift
|
||||
StatusError = service.StatusError
|
||||
)
|
||||
```
|
||||
Значения не меняются, весь остальной код планировщика и его тесты (используют `StatusDrift` и т.д.) работают без правок. Проверь, что нет цикла импорта (service НЕ импортирует scheduler — цикла нет). `go build ./...` должен пройти.
|
||||
|
||||
- [ ] **Step 4: TenantStore += SetDomainStatus**
|
||||
|
||||
`internal/api/api.go` — в интерфейс `TenantStore` добавить:
|
||||
```go
|
||||
SetDomainStatus(ctx context.Context, domainID uuid.UUID, status string) error
|
||||
```
|
||||
(`*store.Store` уже реализует — метод существует.) Обнови mock `TenantStore` в тестах API (добавь метод-заглушку с записью аргументов для проверки).
|
||||
|
||||
- [ ] **Step 5: handleCheck персистит статус**
|
||||
|
||||
`internal/api/handlers.go` `handleCheck` — после получения changeset и до/после ответа записать статус (не фейлить ответ при ошибке записи статуса — она вторична):
|
||||
```go
|
||||
cs, err := a.Svc.Check(r.Context(), pid, did)
|
||||
if err != nil {
|
||||
// Persist the failure so the domain badge reflects it instead of stale
|
||||
// "unknown"; the write error (if any) is logged, never masks the 500.
|
||||
if serr := a.Store.SetDomainStatus(r.Context(), did, service.StatusError); serr != nil {
|
||||
log.Printf("api: set domain status (error) failed: %v", serr)
|
||||
}
|
||||
log.Printf("api: check failed: %v", err)
|
||||
writeErr(w, http.StatusInternalServerError, "internal error")
|
||||
return
|
||||
}
|
||||
if serr := a.Store.SetDomainStatus(r.Context(), did, service.DeriveStatus(cs)); serr != nil {
|
||||
log.Printf("api: set domain status failed: %v", serr)
|
||||
}
|
||||
writeJSON(w, http.StatusOK, toChangesetResponse(cs))
|
||||
```
|
||||
(импортируй `internal/service`.)
|
||||
|
||||
- [ ] **Step 6: Тест — check пишет статус**
|
||||
|
||||
`internal/api/…_test.go`: mock CheckApplier возвращает changeset с actionable prune → после `GET /domains/{did}/check` mock TenantStore.SetDomainStatus получил `"drift"`; changeset без actionable → `"in_sync"`; Svc.Check возвращает ошибку → SetDomainStatus получил `"error"` и ответ 500.
|
||||
|
||||
- [ ] **Step 7: Прогон и коммит**
|
||||
|
||||
Run: `go build ./... && go test ./internal/...`. Ожидание PASS.
|
||||
```bash
|
||||
git add internal/
|
||||
git commit -m "fix(api): manual check persists last_check_status (was stale unknown)"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 2: Frontend — перенос длинных значений в diff и просмотре зоны
|
||||
|
||||
**Files:**
|
||||
- Modify: `web/src/components/DiffView.tsx`, `web/src/pages/DomainDiffPage.tsx` (таблица записей зоны)
|
||||
- Test: `web/src/components/DiffView.test.tsx`
|
||||
|
||||
**Interfaces:** только вёрстка; поведение не меняется.
|
||||
|
||||
Проблема: в `DiffView` `RecordRow` — `flex items-center`, значения в `<span className="... shrink-0 ...">` без переноса. Длинное значение (DKIM ~400 символов, длинный SPF) не переносится → распирает строку → горизонтальный скролл всей страницы.
|
||||
|
||||
- [ ] **Step 1: Перенос значений в RecordRow**
|
||||
|
||||
Переструктурировать строку так, чтобы длинные значения переносились и НЕ вызывали горизонтального оверфлоу, сохранив аккуратный вид коротких записей. Рекомендуемый подход: сделать строку `items-start`, убрать `shrink-0` у контейнера значений, дать значениям `min-w-0 break-all` (или `break-words`), при необходимости вынести пару actual→desired на отдельную строку под именем на узких экранах. Ключевые требования:
|
||||
- значения оборачиваются (`break-all` для длинных беспробельных строк вроде DKIM `p=...`);
|
||||
- контейнер строки не шире родителя (нет `overflow-x` у страницы) — проверить, что `min-w-0` есть на всех flex-детях, которые могут содержать длинный контент;
|
||||
- badge типа и метка read-only остаются на месте, короткие значения читаемы.
|
||||
Реализуй по месту — это вёрстка, критерий приёмки в Step 3.
|
||||
|
||||
- [ ] **Step 2: Перенос в таблице записей зоны**
|
||||
|
||||
`web/src/pages/DomainDiffPage.tsx` — read-only таблица записей зоны (ветка без шаблона): ячейка значений `values.join("\n")` с `whitespace-pre-line`. Добавить `break-all` (или `break-words`) и убедиться, что длинный DKIM в ячейке переносится, а таблица не растягивает страницу (обернуть таблицу в `overflow-x-auto` контейнер, если нужно горизонтальной прокрутки внутри самой таблицы, но НЕ страницы).
|
||||
|
||||
- [ ] **Step 3: Тест + прогон**
|
||||
|
||||
Тест `DiffView.test.tsx`: запись с очень длинным значением (напр. DKIM `"v=DKIM1; ... p="+"A".repeat(400)`) рендерится (не падает) и значение присутствует; (визуальный критерий переноса словами проверяется вручную). Ключевая ручная проверка приёмки: на странице diff домена с DKIM/длинным SPF нет горизонтального скролла, значения переносятся.
|
||||
Run: `cd web && npm run test -- --run && npx tsc --noEmit && npm run build`.
|
||||
```bash
|
||||
cd .. && git checkout internal/web/dist/index.html
|
||||
git add web/src/
|
||||
git commit -m "fix(web): wrap long record values in diff and zone view (no horizontal overflow)"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Итоговая проверка
|
||||
|
||||
- `go build ./... && go test ./...` — PASS.
|
||||
- `cd web && npm run test -- --run && npm run build` — PASS.
|
||||
- Ручная: (A) Recheck домена с шаблоном и prune → бейдж в списке становится `drift` (не `unknown`); чистый домен → `in_sync`. (B) diff домена с DKIM/длинным SPF — значения переносятся, горизонтального скролла страницы нет.
|
||||
@@ -50,6 +50,11 @@ type TenantStore interface {
|
||||
DeleteDomain(ctx context.Context, id, projectID uuid.UUID) error
|
||||
ImportDomains(ctx context.Context, projectID, accountID uuid.UUID, zones []provider.Zone) ([]store.Domain, error)
|
||||
SetDomainTemplate(ctx context.Context, domainID, projectID uuid.UUID, templateID *uuid.UUID) (store.Domain, error)
|
||||
// SetDomainStatus persists the outcome of a manual check (handleCheck) so
|
||||
// the domain's badge reflects reality immediately, instead of staying
|
||||
// "unknown" until the scheduler's next tick. Scoped by projectID so a
|
||||
// foreign domain ID can never have its status overwritten (IDOR-on-write).
|
||||
SetDomainStatus(ctx context.Context, domainID, projectID uuid.UUID, status string) error
|
||||
}
|
||||
|
||||
// Cipher encrypts/decrypts provider account secrets. *crypto.Cipher satisfies it.
|
||||
|
||||
+132
-1
@@ -4,6 +4,7 @@ import (
|
||||
"bytes"
|
||||
"context"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"strings"
|
||||
@@ -20,9 +21,20 @@ type mockCheckApplier struct {
|
||||
lastReq service.ApplyRequest
|
||||
zoneRecords []model.Record
|
||||
zoneErr error
|
||||
|
||||
// checkCS/checkErr, when set, override Check's default fixed changeset —
|
||||
// used by handleCheck status-persistence tests (drift/in_sync/error).
|
||||
checkCS *diff.Changeset
|
||||
checkErr error
|
||||
}
|
||||
|
||||
func (m *mockCheckApplier) Check(context.Context, uuid.UUID, uuid.UUID) (diff.Changeset, error) {
|
||||
if m.checkErr != nil {
|
||||
return diff.Changeset{}, m.checkErr
|
||||
}
|
||||
if m.checkCS != nil {
|
||||
return *m.checkCS, nil
|
||||
}
|
||||
d := model.Record{Type: model.A, Name: "a.example.com.", TTL: 300, Values: []string{"1.1.1.1"}}
|
||||
return diff.Changeset{Diffs: []diff.RecordDiff{{Kind: diff.Add, Type: d.Type, Name: d.Name, Desired: &d}}}, nil
|
||||
}
|
||||
@@ -44,7 +56,10 @@ func (m *mockCheckApplier) ZoneRecords(context.Context, uuid.UUID, uuid.UUID) ([
|
||||
// middleware_test.go's own tests and the IDOR regression.
|
||||
func newTestAPI() (*API, *mockCheckApplier) {
|
||||
m := &mockCheckApplier{}
|
||||
return &API{Svc: m, Auth: alwaysOwnedAuthStore(), Sessions: alwaysValidSessions(uuid.New())}, m
|
||||
return &API{
|
||||
Svc: m, Store: &mockTenantStore{},
|
||||
Auth: alwaysOwnedAuthStore(), Sessions: alwaysValidSessions(uuid.New()),
|
||||
}, m
|
||||
}
|
||||
|
||||
func TestCheckEndpoint(t *testing.T) {
|
||||
@@ -137,6 +152,122 @@ func TestApplyBadUUID(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestCheckEndpoint_PersistsDriftStatus covers the core bug fix: a manual
|
||||
// check (GET .../check) whose changeset has an actionable prune must persist
|
||||
// "drift" via Store.SetDomainStatus — previously only the scheduler wrote
|
||||
// last_check_status, leaving a manually-checked domain stuck at "unknown".
|
||||
func TestCheckEndpoint_PersistsDriftStatus(t *testing.T) {
|
||||
a, m := newTestAPI()
|
||||
ts := a.Store.(*mockTenantStore)
|
||||
rec := model.Record{Type: model.A, Name: "x.example.com.", Values: []string{"1.1.1.1"}}
|
||||
m.checkCS = &diff.Changeset{Diffs: []diff.RecordDiff{{Kind: diff.Delete, Type: rec.Type, Name: rec.Name, Actual: &rec}}}
|
||||
router := NewRouter(a)
|
||||
|
||||
did := uuid.New()
|
||||
req := requestWithSessionCookie(http.MethodGet,
|
||||
"/api/v1/projects/00000000-0000-0000-0000-000000000002/domains/"+did.String()+"/check", nil)
|
||||
w := httptest.NewRecorder()
|
||||
router.ServeHTTP(w, req)
|
||||
|
||||
if w.Code != http.StatusOK {
|
||||
t.Fatalf("status %d body %s", w.Code, w.Body.String())
|
||||
}
|
||||
wantPID := uuid.MustParse("00000000-0000-0000-0000-000000000002")
|
||||
if len(ts.statusCalls) != 1 || ts.statusCalls[0].domainID != did || ts.statusCalls[0].projectID != wantPID || ts.statusCalls[0].status != service.StatusDrift {
|
||||
t.Fatalf("expected SetDomainStatus(%s, %s, drift), got %+v", did, wantPID, ts.statusCalls)
|
||||
}
|
||||
}
|
||||
|
||||
// TestCheckEndpoint_PersistsInSyncStatus covers the no-drift case: a
|
||||
// changeset with no actionable diffs persists "in_sync".
|
||||
func TestCheckEndpoint_PersistsInSyncStatus(t *testing.T) {
|
||||
a, m := newTestAPI()
|
||||
ts := a.Store.(*mockTenantStore)
|
||||
m.checkCS = &diff.Changeset{}
|
||||
router := NewRouter(a)
|
||||
|
||||
did := uuid.New()
|
||||
req := requestWithSessionCookie(http.MethodGet,
|
||||
"/api/v1/projects/00000000-0000-0000-0000-000000000002/domains/"+did.String()+"/check", nil)
|
||||
w := httptest.NewRecorder()
|
||||
router.ServeHTTP(w, req)
|
||||
|
||||
if w.Code != http.StatusOK {
|
||||
t.Fatalf("status %d body %s", w.Code, w.Body.String())
|
||||
}
|
||||
wantPID := uuid.MustParse("00000000-0000-0000-0000-000000000002")
|
||||
if len(ts.statusCalls) != 1 || ts.statusCalls[0].projectID != wantPID || ts.statusCalls[0].status != service.StatusInSync {
|
||||
t.Fatalf("expected SetDomainStatus(_, %s, in_sync), got %+v", wantPID, ts.statusCalls)
|
||||
}
|
||||
}
|
||||
|
||||
// TestCheckEndpoint_ErrorPersistsErrorStatus covers the failure path: when
|
||||
// Svc.Check itself fails (provider/loader error), the handler must persist
|
||||
// "error" before returning 500 — a write failure of the status itself must
|
||||
// not mask the original 500.
|
||||
func TestCheckEndpoint_ErrorPersistsErrorStatus(t *testing.T) {
|
||||
a, m := newTestAPI()
|
||||
ts := a.Store.(*mockTenantStore)
|
||||
m.checkErr = errors.New("boom: provider unreachable")
|
||||
router := NewRouter(a)
|
||||
|
||||
did := uuid.New()
|
||||
req := requestWithSessionCookie(http.MethodGet,
|
||||
"/api/v1/projects/00000000-0000-0000-0000-000000000002/domains/"+did.String()+"/check", nil)
|
||||
w := httptest.NewRecorder()
|
||||
router.ServeHTTP(w, req)
|
||||
|
||||
if w.Code != http.StatusInternalServerError {
|
||||
t.Fatalf("expected 500, got %d body %s", w.Code, w.Body.String())
|
||||
}
|
||||
wantPID := uuid.MustParse("00000000-0000-0000-0000-000000000002")
|
||||
if len(ts.statusCalls) != 1 || ts.statusCalls[0].domainID != did || ts.statusCalls[0].projectID != wantPID || ts.statusCalls[0].status != service.StatusError {
|
||||
t.Fatalf("expected SetDomainStatus(%s, %s, error), got %+v", did, wantPID, ts.statusCalls)
|
||||
}
|
||||
}
|
||||
|
||||
// TestCheckEndpoint_ErrorScopesStatusToCallerProject covers the HIGH
|
||||
// IDOR-on-write fix: handleCheck's error branch must persist the failure
|
||||
// status scoped to the caller's OWN project (pid from the URL/context), even
|
||||
// when the domain ID in the URL belongs to (or doesn't exist in) a different
|
||||
// tenant. The handler itself has no way to know whether did is foreign — the
|
||||
// scoping guarantee comes from always passing pid through to
|
||||
// Store.SetDomainStatus, which is enforced to be a no-op for a mismatched
|
||||
// project_id at the store/SQL layer (see internal/store/schedule_test.go's
|
||||
// TestSetDomainStatus_ScopedByProject_ForeignProjectIsNoOp). This test proves
|
||||
// the handler holds up its side of that contract: pid, never a zero value or
|
||||
// some other project, is what gets passed down.
|
||||
func TestCheckEndpoint_ErrorScopesStatusToCallerProject(t *testing.T) {
|
||||
a, m := newTestAPI()
|
||||
ts := a.Store.(*mockTenantStore)
|
||||
m.checkErr = errors.New("boom: provider unreachable")
|
||||
router := NewRouter(a)
|
||||
|
||||
callerPID := uuid.New()
|
||||
// foreignDID stands in for a domain ID the caller does not own — from the
|
||||
// handler's perspective it's just whatever {did} was in the URL; only the
|
||||
// store layer can (and does) enforce that it isn't actually foreignPID's.
|
||||
foreignDID := uuid.New()
|
||||
req := requestWithSessionCookie(http.MethodGet,
|
||||
"/api/v1/projects/"+callerPID.String()+"/domains/"+foreignDID.String()+"/check", nil)
|
||||
w := httptest.NewRecorder()
|
||||
router.ServeHTTP(w, req)
|
||||
|
||||
if w.Code != http.StatusInternalServerError {
|
||||
t.Fatalf("expected 500, got %d body %s", w.Code, w.Body.String())
|
||||
}
|
||||
if len(ts.statusCalls) != 1 {
|
||||
t.Fatalf("expected exactly 1 SetDomainStatus call, got %+v", ts.statusCalls)
|
||||
}
|
||||
call := ts.statusCalls[0]
|
||||
if call.projectID != callerPID {
|
||||
t.Fatalf("expected SetDomainStatus scoped to caller's own pid %s, got projectID %s (never empty/foreign)", callerPID, call.projectID)
|
||||
}
|
||||
if call.domainID != foreignDID || call.status != service.StatusError {
|
||||
t.Fatalf("expected SetDomainStatus(%s, %s, error), got %+v", foreignDID, callerPID, call)
|
||||
}
|
||||
}
|
||||
|
||||
// TestChangesetResponseEmptyMarshalsToArrays guards the белый-экран bug: an
|
||||
// empty changeset (zone matches its template exactly, e.g. right after a
|
||||
// snapshot) must marshal updates/prunes/readOnly as [] not null — a nil slice
|
||||
|
||||
@@ -37,10 +37,20 @@ func (a *API) handleCheck(w http.ResponseWriter, r *http.Request) {
|
||||
}
|
||||
cs, err := a.Svc.Check(r.Context(), pid, did)
|
||||
if err != nil {
|
||||
// Persist the failure so the domain badge reflects it instead of stale
|
||||
// "unknown"; the write error (if any) is logged, never masks the 500.
|
||||
if serr := a.Store.SetDomainStatus(r.Context(), did, pid, service.StatusError); serr != nil {
|
||||
log.Printf("api: set domain status (error) failed: %v", serr)
|
||||
}
|
||||
log.Printf("api: check failed: %v", err)
|
||||
writeErr(w, http.StatusInternalServerError, "internal error")
|
||||
return
|
||||
}
|
||||
// Manual check persists status/history only — no notification. Notify
|
||||
// remains the scheduler's responsibility (see internal/scheduler).
|
||||
if serr := a.Store.SetDomainStatus(r.Context(), did, pid, service.DeriveStatus(cs)); serr != nil {
|
||||
log.Printf("api: set domain status failed: %v", serr)
|
||||
}
|
||||
writeJSON(w, http.StatusOK, toChangesetResponse(cs))
|
||||
}
|
||||
|
||||
|
||||
@@ -239,7 +239,7 @@ func TestIDOR_CheckForeignProject_Returns404AndServiceNotCalled(t *testing.T) {
|
||||
}
|
||||
return store.Project{}, errors.New("not found")
|
||||
}}
|
||||
a := &API{Svc: svc, Auth: auth, Sessions: alwaysValidSessions(userA)}
|
||||
a := &API{Svc: svc, Store: &mockTenantStore{}, Auth: auth, Sessions: alwaysValidSessions(userA)}
|
||||
router := NewRouter(a)
|
||||
|
||||
did := uuid.New().String()
|
||||
|
||||
@@ -39,6 +39,16 @@ type mockTenantStore struct {
|
||||
importCalled bool
|
||||
|
||||
setDomainTemplateErr error
|
||||
|
||||
// statusCalls records every SetDomainStatus(domainID, projectID, status)
|
||||
// call, in order, so tests can assert what the handler persisted — and,
|
||||
// crucially, which projectID it scoped the write to (IDOR regression).
|
||||
statusCalls []struct {
|
||||
domainID uuid.UUID
|
||||
projectID uuid.UUID
|
||||
status string
|
||||
}
|
||||
setDomainStatusErr error
|
||||
}
|
||||
|
||||
func (m *mockTenantStore) CreateAccount(_ context.Context, projectID uuid.UUID, prov, secretEnc, comment string) (store.Account, error) {
|
||||
@@ -126,6 +136,18 @@ func (m *mockTenantStore) SetDomainTemplate(_ context.Context, domainID, project
|
||||
return d, nil
|
||||
}
|
||||
|
||||
// SetDomainStatus records the call for assertion instead of actually mutating
|
||||
// m.domains — handleCheck tests only need to verify what was written, and
|
||||
// which projectID it was scoped to (IDOR regression coverage).
|
||||
func (m *mockTenantStore) SetDomainStatus(_ context.Context, domainID, projectID uuid.UUID, status string) error {
|
||||
m.statusCalls = append(m.statusCalls, struct {
|
||||
domainID uuid.UUID
|
||||
projectID uuid.UUID
|
||||
status string
|
||||
}{domainID, projectID, status})
|
||||
return m.setDomainStatusErr
|
||||
}
|
||||
|
||||
func (m *mockTenantStore) ImportDomains(_ context.Context, projectID, accountID uuid.UUID, zones []provider.Zone) ([]store.Domain, error) {
|
||||
m.importCalled = true
|
||||
if m.importDomainsErr != nil {
|
||||
|
||||
@@ -14,17 +14,21 @@ import (
|
||||
"github.com/vasyakrg/dns-autoresolver/internal/diff"
|
||||
"github.com/vasyakrg/dns-autoresolver/internal/metrics"
|
||||
"github.com/vasyakrg/dns-autoresolver/internal/notify"
|
||||
"github.com/vasyakrg/dns-autoresolver/internal/service"
|
||||
"github.com/vasyakrg/dns-autoresolver/internal/store"
|
||||
)
|
||||
|
||||
// Domain check statuses persisted via SchedStore.SetDomainStatus /
|
||||
// surfaced via GetDomainStatus. "unknown" is the DB default for a domain
|
||||
// that has never been checked (see migrations/0004_schedule_notify.sql).
|
||||
// Aliased from internal/service — the single source of truth shared with the
|
||||
// manual check handler (internal/api), so both paths persist the same
|
||||
// values.
|
||||
const (
|
||||
StatusUnknown = "unknown"
|
||||
StatusInSync = "in_sync"
|
||||
StatusDrift = "drift"
|
||||
StatusError = "error"
|
||||
StatusUnknown = service.StatusUnknown
|
||||
StatusInSync = service.StatusInSync
|
||||
StatusDrift = service.StatusDrift
|
||||
StatusError = service.StatusError
|
||||
)
|
||||
|
||||
// SchedStore is the narrow store dependency the scheduler needs: due
|
||||
@@ -36,7 +40,9 @@ type SchedStore interface {
|
||||
TouchScheduleRun(ctx context.Context, projectID uuid.UUID, at time.Time) error
|
||||
ListDomains(ctx context.Context, projectID uuid.UUID) ([]store.Domain, error)
|
||||
GetDomainStatus(ctx context.Context, domainID uuid.UUID) (string, error)
|
||||
SetDomainStatus(ctx context.Context, domainID uuid.UUID, status string) error
|
||||
// SetDomainStatus is scoped by projectID so a foreign domain ID can never
|
||||
// have its status overwritten (IDOR-on-write) — see internal/store/tenant.go.
|
||||
SetDomainStatus(ctx context.Context, domainID, projectID uuid.UUID, status string) error
|
||||
CountDriftDomains(ctx context.Context) (int, error)
|
||||
}
|
||||
|
||||
@@ -140,12 +146,12 @@ func (s *Scheduler) checkDomain(ctx context.Context, projectID uuid.UUID, d stor
|
||||
cs, checkErr := s.checker.Check(ctx, projectID, d.ID)
|
||||
dur := time.Since(start)
|
||||
|
||||
newStatus := StatusInSync
|
||||
switch {
|
||||
case checkErr != nil:
|
||||
newStatus = StatusError
|
||||
case len(cs.Actionable()) > 0:
|
||||
newStatus = StatusDrift
|
||||
// Derive the status via the same helper the manual check handler uses
|
||||
// (internal/api/handlers.go) so both paths agree on what counts as
|
||||
// "drift" vs. "in sync" — a failed check is always "error" regardless.
|
||||
newStatus := StatusError
|
||||
if checkErr == nil {
|
||||
newStatus = service.DeriveStatus(cs)
|
||||
}
|
||||
s.metrics.ObserveCheck(newStatus, dur)
|
||||
|
||||
@@ -160,7 +166,7 @@ func (s *Scheduler) checkDomain(ctx context.Context, projectID uuid.UUID, d stor
|
||||
// check (drift or in_sync). Calling it again here would double-write
|
||||
// check_runs history for the same check.
|
||||
|
||||
if err := s.store.SetDomainStatus(ctx, d.ID, newStatus); err != nil {
|
||||
if err := s.store.SetDomainStatus(ctx, d.ID, projectID, newStatus); err != nil {
|
||||
log.Printf("scheduler: set domain status for %s failed: %v", d.ID, err)
|
||||
}
|
||||
|
||||
|
||||
@@ -62,7 +62,11 @@ func (m *mockStore) GetDomainStatus(ctx context.Context, domainID uuid.UUID) (st
|
||||
return StatusUnknown, nil
|
||||
}
|
||||
|
||||
func (m *mockStore) SetDomainStatus(ctx context.Context, domainID uuid.UUID, status string) error {
|
||||
// SetDomainStatus ignores projectID here — this in-memory fake is keyed by
|
||||
// domainID alone and isn't exercising the IDOR scoping itself (that's
|
||||
// covered at the store layer / API handler level); it exists only to match
|
||||
// the SchedStore interface signature.
|
||||
func (m *mockStore) SetDomainStatus(ctx context.Context, domainID, projectID uuid.UUID, status string) error {
|
||||
m.mu.Lock()
|
||||
defer m.mu.Unlock()
|
||||
m.status[domainID] = status
|
||||
|
||||
@@ -0,0 +1,24 @@
|
||||
package service
|
||||
|
||||
import "github.com/vasyakrg/dns-autoresolver/internal/diff"
|
||||
|
||||
// Domain check statuses persisted in domains.last_check_status. "unknown" is
|
||||
// the DB default for a domain that has never been checked. Single source of
|
||||
// truth — the scheduler aliases these, the API check handler writes them.
|
||||
const (
|
||||
StatusUnknown = "unknown"
|
||||
StatusInSync = "in_sync"
|
||||
StatusDrift = "drift"
|
||||
StatusError = "error"
|
||||
)
|
||||
|
||||
// DeriveStatus maps a successful check's changeset to a domain status:
|
||||
// actionable drift (managed adds/updates/prunes) → "drift", otherwise
|
||||
// "in_sync". A failed check (provider/loader error) is "error" and handled by
|
||||
// the caller, which has the error in hand.
|
||||
func DeriveStatus(cs diff.Changeset) string {
|
||||
if len(cs.Actionable()) > 0 {
|
||||
return StatusDrift
|
||||
}
|
||||
return StatusInSync
|
||||
}
|
||||
@@ -0,0 +1,23 @@
|
||||
package service_test
|
||||
|
||||
import (
|
||||
"testing"
|
||||
|
||||
"github.com/vasyakrg/dns-autoresolver/internal/diff"
|
||||
"github.com/vasyakrg/dns-autoresolver/internal/model"
|
||||
"github.com/vasyakrg/dns-autoresolver/internal/service"
|
||||
)
|
||||
|
||||
func TestDeriveStatus(t *testing.T) {
|
||||
// no actionable diffs → in_sync
|
||||
if got := service.DeriveStatus(diff.Changeset{}); got != service.StatusInSync {
|
||||
t.Fatalf("empty: %q", got)
|
||||
}
|
||||
// an actionable prune → drift
|
||||
cs := diff.Changeset{Diffs: []diff.RecordDiff{
|
||||
{Kind: diff.Delete, Type: model.A, Name: "x.example.com.", Actual: &model.Record{Type: model.A, Name: "x.example.com.", Values: []string{"1.1.1.1"}}},
|
||||
}}
|
||||
if got := service.DeriveStatus(cs); got != service.StatusDrift {
|
||||
t.Fatalf("prune: %q", got)
|
||||
}
|
||||
}
|
||||
@@ -218,16 +218,17 @@ func (q *Queries) LoadDomainFull(ctx context.Context, arg LoadDomainFullParams)
|
||||
}
|
||||
|
||||
const setDomainStatus = `-- name: SetDomainStatus :exec
|
||||
UPDATE domains SET last_check_status = $2 WHERE id = $1
|
||||
UPDATE domains SET last_check_status = $2 WHERE id = $1 AND project_id = $3
|
||||
`
|
||||
|
||||
type SetDomainStatusParams struct {
|
||||
ID uuid.UUID `json:"id"`
|
||||
LastCheckStatus string `json:"last_check_status"`
|
||||
ProjectID uuid.UUID `json:"project_id"`
|
||||
}
|
||||
|
||||
func (q *Queries) SetDomainStatus(ctx context.Context, arg SetDomainStatusParams) error {
|
||||
_, err := q.db.Exec(ctx, setDomainStatus, arg.ID, arg.LastCheckStatus)
|
||||
_, err := q.db.Exec(ctx, setDomainStatus, arg.ID, arg.LastCheckStatus, arg.ProjectID)
|
||||
return err
|
||||
}
|
||||
|
||||
|
||||
@@ -33,7 +33,7 @@ WHERE d.id = $1 AND d.project_id = $2;
|
||||
SELECT last_check_status FROM domains WHERE id = $1;
|
||||
|
||||
-- name: SetDomainStatus :exec
|
||||
UPDATE domains SET last_check_status = $2 WHERE id = $1;
|
||||
UPDATE domains SET last_check_status = $2 WHERE id = $1 AND project_id = $3;
|
||||
|
||||
-- name: CountDriftDomains :one
|
||||
SELECT count(*) FROM domains WHERE last_check_status = 'drift';
|
||||
|
||||
@@ -246,7 +246,7 @@ func TestDomainStatus_RoundTrip(t *testing.T) {
|
||||
t.Fatalf("expected default status 'unknown', got %q", status)
|
||||
}
|
||||
|
||||
if err := s.SetDomainStatus(ctx, d.ID, "ok"); err != nil {
|
||||
if err := s.SetDomainStatus(ctx, d.ID, p.ID, "ok"); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
status, err = s.GetDomainStatus(ctx, d.ID)
|
||||
@@ -265,3 +265,56 @@ func TestDomainStatus_RoundTrip(t *testing.T) {
|
||||
t.Fatalf("expected ListDomains to reflect updated status: %+v", domains)
|
||||
}
|
||||
}
|
||||
|
||||
// TestSetDomainStatus_ScopedByProject_ForeignProjectIsNoOp covers the IDOR
|
||||
// fix: SetDomainStatus is called with a valid domain ID but a projectID that
|
||||
// does NOT own it (e.g. an authenticated caller's own pid, paired with
|
||||
// another tenant's did in the URL). The WHERE id = $1 AND project_id = $3
|
||||
// clause must match zero rows — no error, but the foreign domain's status
|
||||
// must remain untouched, never "error"/"drift"/whatever was passed in.
|
||||
func TestSetDomainStatus_ScopedByProject_ForeignProjectIsNoOp(t *testing.T) {
|
||||
s, ctx := newStore(t)
|
||||
_, owner, err := s.RegisterUser(ctx, "domain-status-owner@example.com", "argon2-hash")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
_, attacker, err := s.RegisterUser(ctx, "domain-status-attacker@example.com", "argon2-hash")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
acc, err := s.CreateAccount(ctx, owner.ID, "selectel", "enc-blob", "test")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
d, err := s.CreateDomain(ctx, owner.ID, acc.ID, "example.com", "zone-1", nil)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
// attacker's own (valid) project ID, paired with owner's domain ID —
|
||||
// mirrors the exact request shape an authenticated attacker could send.
|
||||
if err := s.SetDomainStatus(ctx, d.ID, attacker.ID, "error"); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
status, err := s.GetDomainStatus(ctx, d.ID)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if status != "unknown" {
|
||||
t.Fatalf("expected foreign-project SetDomainStatus to be a no-op, but status changed to %q", status)
|
||||
}
|
||||
|
||||
// The legitimate owner can still update it — proves the no-op above was
|
||||
// due to project scoping, not some unrelated write failure.
|
||||
if err := s.SetDomainStatus(ctx, d.ID, owner.ID, "error"); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
status, err = s.GetDomainStatus(ctx, d.ID)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if status != "error" {
|
||||
t.Fatalf("expected owner's SetDomainStatus to apply, got %q", status)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -253,9 +253,13 @@ func (s *Store) GetDomainStatus(ctx context.Context, domainID uuid.UUID) (string
|
||||
}
|
||||
|
||||
// SetDomainStatus records the outcome of the most recent check/apply run for
|
||||
// a domain (e.g. "ok", "drift", "error").
|
||||
func (s *Store) SetDomainStatus(ctx context.Context, domainID uuid.UUID, status string) error {
|
||||
return s.q.SetDomainStatus(ctx, db.SetDomainStatusParams{ID: domainID, LastCheckStatus: status})
|
||||
// a domain (e.g. "ok", "drift", "error"). Scoped by projectID — a domain ID
|
||||
// belonging to another tenant's project is left untouched (matches zero
|
||||
// rows) rather than being overwritten, closing an IDOR-on-write where a
|
||||
// caller's own valid pid + a foreign did could otherwise flip a stranger's
|
||||
// domain status.
|
||||
func (s *Store) SetDomainStatus(ctx context.Context, domainID, projectID uuid.UUID, status string) error {
|
||||
return s.q.SetDomainStatus(ctx, db.SetDomainStatusParams{ID: domainID, LastCheckStatus: status, ProjectID: projectID})
|
||||
}
|
||||
|
||||
// CountDriftDomains returns the current number of domains system-wide whose
|
||||
|
||||
@@ -27,6 +27,30 @@ test("marks read-only records", () => {
|
||||
expect(screen.getByText(/NS/)).toBeInTheDocument()
|
||||
})
|
||||
|
||||
test("renders a very long unbreakable value (DKIM key) without crashing", () => {
|
||||
// Real DKIM records ship a ~400-char unbroken p= blob. This must not
|
||||
// throw and the value must land in the DOM (wrapping itself is a CSS
|
||||
// concern verified manually, not via jsdom layout).
|
||||
const longValue = "v=DKIM1; k=rsa; p=" + "A".repeat(400)
|
||||
const csWithDkim: ChangesetResponse = {
|
||||
updates: [
|
||||
{
|
||||
kind: "update",
|
||||
type: "TXT",
|
||||
name: "default._domainkey.example.com.",
|
||||
desired: [longValue],
|
||||
actual: [],
|
||||
readOnly: false,
|
||||
},
|
||||
],
|
||||
prunes: [],
|
||||
readOnly: [],
|
||||
inSyncCount: 0,
|
||||
}
|
||||
render(<DiffView changeset={csWithDkim} />)
|
||||
expect(screen.getByText(new RegExp(longValue))).toBeInTheDocument()
|
||||
})
|
||||
|
||||
test("does not crash when changeset fields are null", () => {
|
||||
// An empty changeset from an older/edge backend can arrive with null slices
|
||||
// instead of []. DiffView must normalise them, not blow up on .length/.map.
|
||||
|
||||
@@ -47,11 +47,14 @@ function RecordRow({ record, tone }: { record: RecordView; tone: Tone }) {
|
||||
return (
|
||||
<div
|
||||
className={cn(
|
||||
"group/row flex items-center gap-3 border-l-2 px-3 py-2.5 transition-colors",
|
||||
"group/row flex flex-col gap-1.5 border-l-2 px-3 py-2.5 transition-colors",
|
||||
"hover:bg-foreground/[0.025]",
|
||||
)}
|
||||
style={{ borderLeftColor: meta.dot }}
|
||||
>
|
||||
{/* Top line: type badge, name, read-only flag — always single-line,
|
||||
never affected by how long the record values are. */}
|
||||
<div className="flex items-center gap-3">
|
||||
<Badge
|
||||
variant="outline"
|
||||
className="font-dns w-11 shrink-0 justify-center border-border text-[10px] tracking-wide text-muted-foreground"
|
||||
@@ -63,28 +66,38 @@ function RecordRow({ record, tone }: { record: RecordView; tone: Tone }) {
|
||||
{record.name}
|
||||
</span>
|
||||
|
||||
<span className="font-dns hidden shrink-0 items-center gap-1.5 text-xs text-muted-foreground sm:flex">
|
||||
<Values values={record.actual} />
|
||||
{showArrow && (
|
||||
<>
|
||||
<ArrowRight className="size-3 text-muted-foreground/50" strokeWidth={1.75} />
|
||||
<span style={{ color: meta.dot }}>
|
||||
<Values values={record.desired} />
|
||||
</span>
|
||||
</>
|
||||
)}
|
||||
</span>
|
||||
|
||||
{record.readOnly && (
|
||||
<Badge
|
||||
variant="secondary"
|
||||
className="ml-1 shrink-0 gap-1 bg-muted text-[10px] text-muted-foreground"
|
||||
className="shrink-0 gap-1 bg-muted text-[10px] text-muted-foreground"
|
||||
>
|
||||
<Lock className="size-2.5" strokeWidth={2} />
|
||||
read-only
|
||||
</Badge>
|
||||
)}
|
||||
</div>
|
||||
|
||||
{/* Values line: plain block-level text (not flex) so a long
|
||||
unbreakable value like a DKIM key wraps within the row's own
|
||||
width instead of stretching it — a flex item's content can
|
||||
otherwise refuse to shrink below its intrinsic width. Indented
|
||||
to align under the name (badge width + gap). */}
|
||||
<div className="font-dns hidden pl-14 text-xs leading-relaxed break-all text-muted-foreground sm:block">
|
||||
<Values values={record.actual} />
|
||||
{showArrow && (
|
||||
<>
|
||||
{" "}
|
||||
<ArrowRight
|
||||
className="mx-1 inline-block size-3 align-[-2px] text-muted-foreground/50"
|
||||
strokeWidth={1.75}
|
||||
/>{" "}
|
||||
<span style={{ color: meta.dot }}>
|
||||
<Values values={record.desired} />
|
||||
</span>
|
||||
</>
|
||||
)}
|
||||
</div>
|
||||
</div>
|
||||
)
|
||||
}
|
||||
|
||||
|
||||
@@ -167,7 +167,9 @@ export function DomainDiffPage() {
|
||||
<TableCell className="font-dns">{r.type}</TableCell>
|
||||
<TableCell className="font-dns">{r.name}</TableCell>
|
||||
<TableCell className="font-dns">{r.ttl}</TableCell>
|
||||
<TableCell className="font-dns whitespace-pre-line">{r.values.join("\n")}</TableCell>
|
||||
<TableCell className="font-dns whitespace-pre-line break-all">
|
||||
{r.values.join("\n")}
|
||||
</TableCell>
|
||||
</TableRow>
|
||||
))}
|
||||
</TableBody>
|
||||
|
||||
Reference in New Issue
Block a user