diff --git a/docs/superpowers/plans/2026-07-05-check-status-and-diff-layout.md b/docs/superpowers/plans/2026-07-05-check-status-and-diff-layout.md new file mode 100644 index 0000000..cf26c69 --- /dev/null +++ b/docs/superpowers/plans/2026-07-05-check-status-and-diff-layout.md @@ -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`, значения в `` без переноса. Длинное значение (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 — значения переносятся, горизонтального скролла страницы нет. diff --git a/internal/api/api.go b/internal/api/api.go index db6c2c9..c621e20 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -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. diff --git a/internal/api/api_test.go b/internal/api/api_test.go index 3c55cd1..75f5e9b 100644 --- a/internal/api/api_test.go +++ b/internal/api/api_test.go @@ -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 diff --git a/internal/api/handlers.go b/internal/api/handlers.go index fd85cc4..adcc435 100644 --- a/internal/api/handlers.go +++ b/internal/api/handlers.go @@ -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)) } diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go index bcf3385..bb7d5d4 100644 --- a/internal/api/middleware_test.go +++ b/internal/api/middleware_test.go @@ -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() diff --git a/internal/api/tenant_test.go b/internal/api/tenant_test.go index 9c845a0..4f613b0 100644 --- a/internal/api/tenant_test.go +++ b/internal/api/tenant_test.go @@ -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 { diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 47b443b..ed23f25 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -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) } diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index d6a6c75..6d297ec 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -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 diff --git a/internal/service/status.go b/internal/service/status.go new file mode 100644 index 0000000..e83106b --- /dev/null +++ b/internal/service/status.go @@ -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 +} diff --git a/internal/service/status_test.go b/internal/service/status_test.go new file mode 100644 index 0000000..170ca72 --- /dev/null +++ b/internal/service/status_test.go @@ -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) + } +} diff --git a/internal/store/db/domains.sql.go b/internal/store/db/domains.sql.go index 4ac5c7a..25e2a07 100644 --- a/internal/store/db/domains.sql.go +++ b/internal/store/db/domains.sql.go @@ -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 } diff --git a/internal/store/queries/domains.sql b/internal/store/queries/domains.sql index 2387e19..ba0829d 100644 --- a/internal/store/queries/domains.sql +++ b/internal/store/queries/domains.sql @@ -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'; diff --git a/internal/store/schedule_test.go b/internal/store/schedule_test.go index 1ef49aa..097b86e 100644 --- a/internal/store/schedule_test.go +++ b/internal/store/schedule_test.go @@ -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) + } +} diff --git a/internal/store/tenant.go b/internal/store/tenant.go index d5dec6f..e2084e1 100644 --- a/internal/store/tenant.go +++ b/internal/store/tenant.go @@ -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 diff --git a/web/src/components/DiffView.test.tsx b/web/src/components/DiffView.test.tsx index 6d58953..5bfce43 100644 --- a/web/src/components/DiffView.test.tsx +++ b/web/src/components/DiffView.test.tsx @@ -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() + 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. diff --git a/web/src/components/DiffView.tsx b/web/src/components/DiffView.tsx index 813170f..185344a 100644 --- a/web/src/components/DiffView.tsx +++ b/web/src/components/DiffView.tsx @@ -47,43 +47,56 @@ function RecordRow({ record, tone }: { record: RecordView; tone: Tone }) { return (
- - {record.type} - + {/* Top line: type badge, name, read-only flag — always single-line, + never affected by how long the record values are. */} +
+ + {record.type} + - - {record.name} - + + {record.name} + - + {record.readOnly && ( + + + read-only + + )} +
+ + {/* 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). */} +
{showArrow && ( <> - + {" "} + {" "} )} - - - {record.readOnly && ( - - - read-only - - )} +
) } diff --git a/web/src/pages/DomainDiffPage.tsx b/web/src/pages/DomainDiffPage.tsx index 9bc49ff..aaccd1b 100644 --- a/web/src/pages/DomainDiffPage.tsx +++ b/web/src/pages/DomainDiffPage.tsx @@ -167,7 +167,9 @@ export function DomainDiffPage() { {r.type} {r.name} {r.ttl} - {r.values.join("\n")} + + {r.values.join("\n")} + ))}