From c2ba33ee94fa015894cf1eda8f876ac970734600 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Tue, 27 Oct 2026 15:32:29 +0100 Subject: [PATCH] fix: the runtime cycle log is keyed by rules and bounded (WR-13) redirectHopsLogged was a never-evicted map keyed by the anonymous request's URL with its query, so requests to a looping address with fresh query strings grew it without limit and logged each one. The log is now keyed by the cycle's rule ids, repeats at most once an hour and starts over at 1024 keys. --- README.md | 2 +- classes/redirect_graph_test.go | 31 +++++++++++++++++++++ classes/redirects.go | 50 ++++++++++++++++++++++++++++++---- 3 files changed, 77 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index f5de104..dee9c85 100644 --- a/README.md +++ b/README.md @@ -109,7 +109,7 @@ The pipeline runs normalisation, then redirects on the normalised path (followed {"action": "redirect", "status": 301, "location": "https://example.com/pages/b", "url": "https://example.com/pages/a", "locale": "", "staging": false, "seo": null} ``` -`status` is 302 when a temporary redirect takes part. A 410 rule answers `"action": "gone"`, `"status": 410`. A redirect cycle met at runtime (stored past the save check) stops after 10 hops with `not_found` and is logged once per URL. A path that no published record owns and no URL rule covers answers `"action": "not_found"`, `"status": 404` and `"seo": null` (the home page `/` is always known). A `url` that is neither a path nor an http(s) URL is a `422`. Absolute URLs are built from `golem15.seo.site_url`, or from the request URL when it names the site's host or its `www` twin (for the scheme and host rules), never from the request's Host header. +`status` is 302 when a temporary redirect takes part. A 410 rule answers `"action": "gone"`, `"status": 410`. A redirect cycle met at runtime (stored past the save check) stops after 10 hops with `not_found` and is logged at most once an hour per cycle (keyed by its rules, not by the requested URL). A path that no published record owns and no URL rule covers answers `"action": "not_found"`, `"status": 404` and `"seo": null` (the home page `/` is always known). A `url` that is neither a path nor an http(s) URL is a `422`. Absolute URLs are built from `golem15.seo.site_url`, or from the request URL when it names the site's host or its `www` twin (for the scheme and host rules), never from the request's Host header. ### The 404 report endpoint diff --git a/classes/redirect_graph_test.go b/classes/redirect_graph_test.go index 8117417..d87f643 100644 --- a/classes/redirect_graph_test.go +++ b/classes/redirect_graph_test.go @@ -203,3 +203,34 @@ func TestRedirectGraph(t *testing.T) { } }) } + +// TestCycleLogBounded: the runtime cycle log is keyed by the cycle's rules, +// not the request URL, and never holds more than maxCycleLog keys (WR-13). +func TestCycleLogBounded(t *testing.T) { + set := NewRedirectSet([]models.Redirect{rd(1, "/la", "/lb"), rd(2, "/lb", "/la")}) + ch, err := set.Follow(rdSite, "/la", "", MaxRedirectHops) + if err != nil || !ch.Loop { + t.Fatalf("chain = %+v, %v; want a loop", ch, err) + } + cycleLog.Lock() + cycleLog.seen = nil + cycleLog.Unlock() + for i := 0; i < 5000; i++ { + logRedirectCycle(rdSite+"/la?x="+strconv.Itoa(i), ch) + } + cycleLog.Lock() + n := len(cycleLog.seen) + cycleLog.Unlock() + if n != 1 { + t.Fatalf("cycle log keys after one cycle from 5000 URLs = %d, want 1", n) + } + for i := 0; i < maxCycleLog+10; i++ { + logRedirectCycle("/x", Chain{Hops: []RedirectHit{{Rule: rd(uint(100+i), "/a", "/b")}}}) + } + cycleLog.Lock() + n = len(cycleLog.seen) + cycleLog.Unlock() + if n > maxCycleLog { + t.Fatalf("cycle log keys = %d, want at most %d", n, maxCycleLog) + } +} diff --git a/classes/redirects.go b/classes/redirects.go index 5056759..f2cb3ff 100644 --- a/classes/redirects.go +++ b/classes/redirects.go @@ -7,6 +7,7 @@ import ( "strconv" "strings" "sync" + "time" "git.golem15.com/golem15/sm-seo-plugin/models" "git.golem15.com/golem15/summercms/modules/diary" @@ -529,15 +530,54 @@ func internalTarget(site, target string) (string, string, bool) { return p, u.RawQuery, true } -// redirectHopsLogged keeps the URLs whose runtime redirect cycle was -// logged, so each is logged once. -var redirectHopsLogged sync.Map +// cycleLog keeps when each runtime redirect cycle was last logged, keyed +// by the rules it runs through (not by the request URL, which an anonymous +// client controls), and holds at most maxCycleLog keys (WR-13). +var cycleLog struct { + sync.Mutex + seen map[string]time.Time +} -// logRedirectCycle logs a redirect cycle met at runtime, once per URL. +// maxCycleLog bounds cycleLog; when it is full it starts over. +const maxCycleLog = 1024 + +// cycleLogEvery is how often one cycle is logged again. +const cycleLogEvery = time.Hour + +// cycleKey identifies a cycle by the sorted ids of the rules it fired. +func cycleKey(ch Chain) string { + ids := make([]int, 0, len(ch.Hops)) + for _, h := range ch.Hops { + ids = append(ids, int(h.Rule.ID)) + } + sort.Ints(ids) + var b strings.Builder + for i, id := range ids { + if i > 0 && ids[i-1] == id { + continue + } + b.WriteString(strconv.Itoa(id)) + b.WriteByte(',') + } + return b.String() +} + +// logRedirectCycle logs a redirect cycle met at runtime, at most once an +// hour per set of rules. func logRedirectCycle(url string, ch Chain) { - if _, seen := redirectHopsLogged.LoadOrStore(url, true); seen { + key := cycleKey(ch) + now := time.Now() + cycleLog.Lock() + if cycleLog.seen == nil || len(cycleLog.seen) >= maxCycleLog { + cycleLog.seen = map[string]time.Time{} + } + at, seen := cycleLog.seen[key] + if seen && now.Sub(at) < cycleLogEvery { + cycleLog.Unlock() return } + cycleLog.seen[key] = now + cycleLog.Unlock() log.Warn("golem15.seo: redirect cycle, answering not_found", "url", url, "path", strings.Join(ch.Visited, " → ")) }