From 70f9bc6793da5ca2061159cd3e5523f02b3062a4 Mon Sep 17 00:00:00 2001 From: Vassiliy Yegorov Date: Fri, 3 Jul 2026 13:13:24 +0700 Subject: [PATCH] =?UTF-8?q?harden(selectel):=20=D0=B7=D0=B0=D1=89=D0=B8?= =?UTF-8?q?=D1=82=D0=B0=20=D0=BF=D0=B0=D0=B3=D0=B8=D0=BD=D0=B0=D1=86=D0=B8?= =?UTF-8?q?=D0=B8=20=D0=BE=D1=82=20=D0=BD=D0=B5=D0=BF=D0=BE=D0=B4=D0=B2?= =?UTF-8?q?=D0=B8=D0=B6=D0=BD=D0=BE=D0=B3=D0=BE=20offset,=20=D1=82=D0=B5?= =?UTF-8?q?=D1=81=D1=82=20New,=20=D0=B4=D0=BE=D0=BA=D1=83=D0=BC=D0=B5?= =?UTF-8?q?=D0=BD=D1=82=D0=B8=D1=80=D0=BE=D0=B2=D0=B0=D0=BD=D0=B8=D0=B5=20?= =?UTF-8?q?disabled?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- internal/provider/selectel/selectel.go | 18 ++++ internal/provider/selectel/selectel_test.go | 104 ++++++++++++++++++++ 2 files changed, 122 insertions(+) diff --git a/internal/provider/selectel/selectel.go b/internal/provider/selectel/selectel.go index 91c4949..7056039 100644 --- a/internal/provider/selectel/selectel.go +++ b/internal/provider/selectel/selectel.go @@ -106,6 +106,10 @@ func (c *Client) ListZones(ctx context.Context, creds provider.Credentials) ([]p if page.NextOffset == 0 || len(page.Result) == 0 { break } + if page.NextOffset <= offset { + // API returned a non-advancing offset; stop instead of looping forever. + break + } offset = page.NextOffset } return zones, nil @@ -136,6 +140,10 @@ func (c *Client) listRRSets(ctx context.Context, token, zoneID string) ([]apiRRS if page.NextOffset == 0 || len(page.Result) == 0 { break } + if page.NextOffset <= offset { + // API returned a non-advancing offset; stop instead of looping forever. + break + } offset = page.NextOffset } return all, nil @@ -192,6 +200,12 @@ func (c *Client) ApplyChanges(ctx context.Context, creds provider.Credentials, z return nil } +// NOTE: disabled-записи не участвуют в diff и не сохраняются при apply, т.к. PATCH +// заменяет весь набор records; поддержка отключённых записей — вне текущей фазы. +// Selectel PATCH /rrset/{id} полностью заменяет набор records: он не мержит переданные +// значения с уже существующими. toRecord отбрасывает disabled-записи при чтении, а +// toRRSet никогда их не восстанавливает при записи — поэтому round-trip (read -> diff +// -> apply) безвозвратно теряет ранее disabled-записи в rrset, если он был изменён. func toRecord(rr apiRRSet) model.Record { vals := make([]string, 0, len(rr.Records)) for _, r := range rr.Records { @@ -203,6 +217,10 @@ func toRecord(rr apiRRSet) model.Record { return model.Record{Type: model.RecordType(rr.Type), Name: rr.Name, TTL: rr.TTL, Values: vals} } +// NOTE: disabled-записи не участвуют в diff и не сохраняются при apply, т.к. PATCH +// заменяет весь набор records; поддержка отключённых записей — вне текущей фазы. +// toRRSet всегда шлёт записи без поля disabled, т.е. любой ранее disabled контент, +// отсутствующий в model.Record.Values, будет молча потерян при следующем PATCH этого rrset. func toRRSet(rec model.Record) apiRRSet { rs := apiRRSet{Name: rec.Name, Type: string(rec.Type), TTL: rec.TTL} for _, v := range rec.Values { diff --git a/internal/provider/selectel/selectel_test.go b/internal/provider/selectel/selectel_test.go index 4b08519..a3a884d 100644 --- a/internal/provider/selectel/selectel_test.go +++ b/internal/provider/selectel/selectel_test.go @@ -7,6 +7,7 @@ import ( "net/http/httptest" "strings" "testing" + "time" "github.com/vasyakrg/dns-autoresolver/internal/diff" "github.com/vasyakrg/dns-autoresolver/internal/model" @@ -344,6 +345,109 @@ func TestApplyChangesAddWithNilDesiredReturnsErrorNoPanic(t *testing.T) { } } +// Global Constraint: New() must configure the default base URL and a 30s HTTP timeout. +func TestNewDefaults(t *testing.T) { + c := New() + if c.BaseURL != DefaultBaseURL { + t.Fatalf("BaseURL = %q, want %q", c.BaseURL, DefaultBaseURL) + } + if c.HTTP == nil { + t.Fatal("HTTP client must not be nil") + } + if c.HTTP.Timeout != 30*time.Second { + t.Fatalf("HTTP.Timeout = %v, want %v", c.HTTP.Timeout, 30*time.Second) + } +} + +// Global Constraint: if the API returns a NextOffset that does not advance past the +// current offset (stuck or regressing), even with a non-empty page, ListZones must +// stop pagination instead of looping forever on the same/earlier offset. +func TestListZonesStopsOnNonAdvancingNextOffset(t *testing.T) { + var requests int + c, srv := newTestClient(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + if requests > 5 { + t.Fatalf("too many requests, possible infinite pagination loop on stuck offset") + } + offset := r.URL.Query().Get("offset") + switch offset { + case "0": + json.NewEncoder(w).Encode(map[string]any{ + "result": []map[string]any{{"id": "z1", "name": "first.example.com."}}, + "next_offset": 1000, + }) + case "1000": + // Buggy/malicious API: next_offset does not advance past current offset, + // yet the page is non-empty. + json.NewEncoder(w).Encode(map[string]any{ + "result": []map[string]any{{"id": "z2", "name": "second.example.com."}}, + "next_offset": 1000, + }) + default: + t.Fatalf("unexpected offset %q", offset) + } + })) + defer srv.Close() + + zs, err := c.ListZones(context.Background(), creds()) + if err != nil { + t.Fatal(err) + } + if requests != 2 { + t.Fatalf("expected exactly 2 requests before stopping on stuck offset, got %d", requests) + } + if len(zs) != 2 || zs[0].ID != "z1" || zs[1].ID != "z2" { + t.Fatalf("expected zones accumulated from both pages before stopping, got %+v", zs) + } +} + +// Global Constraint: same guard applies to listRRSets (exercised via GetRecords) — +// a non-advancing NextOffset on a non-empty page must stop pagination, not loop forever. +func TestGetRecordsStopsOnNonAdvancingNextOffset(t *testing.T) { + var requests int + c, srv := newTestClient(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + if requests > 5 { + t.Fatalf("too many requests, possible infinite pagination loop on stuck offset") + } + offset := r.URL.Query().Get("offset") + switch offset { + case "0": + json.NewEncoder(w).Encode(map[string]any{ + "result": []map[string]any{ + {"id": "r1", "name": "a.example.com.", "type": "A", "ttl": 300, + "records": []map[string]any{{"content": "1.1.1.1"}}}, + }, + "next_offset": 1000, + }) + case "1000": + // Buggy/malicious API: next_offset regresses below current offset, + // yet the page is non-empty. + json.NewEncoder(w).Encode(map[string]any{ + "result": []map[string]any{ + {"id": "r2", "name": "b.example.com.", "type": "A", "ttl": 300, + "records": []map[string]any{{"content": "2.2.2.2"}}}, + }, + "next_offset": 500, + }) + default: + t.Fatalf("unexpected offset %q", offset) + } + })) + defer srv.Close() + + recs, err := c.GetRecords(context.Background(), creds(), "z1") + if err != nil { + t.Fatal(err) + } + if requests != 2 { + t.Fatalf("expected exactly 2 requests before stopping on stuck/regressing offset, got %d", requests) + } + if len(recs) != 2 { + t.Fatalf("expected records accumulated from both pages before stopping, got %+v", recs) + } +} + // Global Constraint: HTTP errors (status >= 300) must surface a non-nil error whose text // includes the method/path/status (or response body) for diagnosability. func TestListZonesHTTPErrorIncludesMethodPathStatus(t *testing.T) {