From cb614cb1fbd284c027bb7c82a850e616cd8ede81 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Tue, 27 Oct 2026 15:27:21 +0100 Subject: [PATCH] fix: address changes run the loop simulation and validate their addresses (WR-09) AddressChanged wrote its 301 and re-pointed rules without Problems or the save-time simulation, and removed only an exact rule whose source equals the new address literally. It now removes every exact rule that matches the new address with its options, simulates the 301 and the re-pointing with the stored rules and writes nothing when that loops, stores an absolute old address on the site as its path, and skips an address that is not a path on the site. --- README.md | 2 +- classes/addresses.go | 195 ++++++++++++++++++++++++++++--------------- redirects_test.go | 54 ++++++++++++ 3 files changed, 184 insertions(+), 67 deletions(-) diff --git a/README.md b/README.md index a8cd7d1..b40cbc1 100644 --- a/README.md +++ b/README.md @@ -24,7 +24,7 @@ The frontend (a Nuxt layer or app) asks `GET /_seo/api/v1/resolve?url=...` for t - **Resolver.** Canonical URLs are self-referencing and absolute from the configured site URL, keep content parameters and drop filter and tracking parameters; overrides may be relative or absolute. Robots resolve per directive and are also answered as the `X-Robots-Tag` value. Open Graph and Twitter values inherit from the effective title, description and image. Verification meta tags come with the home page. - **Redirects** (`golem15_seo_redirects`): 301, 302 and 410 rules with exact, pattern (`/old/*` → `/new/*`) and regex (`^/p/(\d+)$` → `/post/$1`) sources, evaluated exact, then patterns (longest literal prefix first), then regex. Options: ignore letter case, match with and without a trailing slash, ignore or pass the query string; a source with `?` matches its query exactly and wins over the same path without a query. Regex sources use the standard library (RE2, linear time) and are capped at 512 characters. A capture never turns a path target into another host, and an absolute target keeps its own host. - **Loops and chains.** Every redirect write runs a simulation with the rule in place (a probe URL for patterns and regex, at most 10 hops): a loop is refused, from the admin with `redirects.loop` on the target and nothing saved; a chain is saved with `redirects.chain_notice` naming the final target and is marked in the list. -- **Address changes.** When a published record's address changes, a 301 from the old to the new address is created, earlier redirects to the old address are pointed straight at the new one, and a redirect whose source is the new address is removed (a slug changed back and forth ends with one redirect per old address). In the admin this happens inside the save transaction and the editor gets `redirects.auto_created` on the slug field; writes outside the admin (Journal's API, MCP, imports) are handled after commit through `pact.RecordSaved`. Deleting or unpublishing a record creates nothing. +- **Address changes.** When a published record's address changes, a 301 from the old to the new address is created, earlier redirects to the old address are pointed straight at the new one, and an exact redirect that matches the new address (with its case and trailing-slash options) is removed (a slug changed back and forth ends with one redirect per old address). The change is simulated first: when it would loop through a pattern or regex rule, or an address is not a path on the site, no redirect is written and a warning is logged. In the admin this happens inside the save transaction and the editor gets `redirects.auto_created` on the slug field; writes outside the admin (Journal's API, MCP, imports) are handled after commit through `pact.RecordSaved`. Deleting or unpublishing a record creates nothing. - **404 log** (`golem15_seo_notfound`): the frontend reports each 404 to `POST /_seo/api/v1/notfound`; the log keeps the address, hits, first seen, last hit and last referer. An entry is resolved with one click: **Create redirect…** (a prompted 301), **Mark as gone** (410), or a bulk 410. - **URL normalisation** (settings tab Addresses): https, `www` or bare host, trailing slash added or removed, optional lowercasing of ASCII letters, double slashes and `/index.html` collapsed. The defaults change nothing but scheme, slashes and index files. - **Strict URLs.** A path that names a record without being its address (a wrong category, a locale prefix with another locale's slug) 301s to the record's address; a wrong URL never answers `ok`. diff --git a/classes/addresses.go b/classes/addresses.go index b03175c..f038d01 100644 --- a/classes/addresses.go +++ b/classes/addresses.go @@ -36,99 +36,162 @@ type AddressResult struct { // AddressChanged applies a record's address change inside tx, the // caller's transaction (D-18, P-20): -// 1. exact redirects whose source is the new address are deleted, so the -// record is never shadowed (this also undoes an earlier change back); +// 1. exact redirects that match the new address (with their case and +// trailing-slash options) are deleted, so the record is never shadowed +// (this also undoes an earlier change back); // 2. old → new is created as a 301 (origin slug), or the existing exact // rule of the old address is pointed at the new one; // 3. redirects whose target is the old address are pointed at the new // one, so no chain forms (A→B, then B→C makes A→C); // 4. redirects that now point to themselves are deleted; -// 5. the entry's URL becomes the new address. +// 5. the entry's URL becomes the new address (as the descriptor wrote it). +// +// Steps 2 to 4 are simulated first with the stored rules (P-19): when the +// result would loop (a pattern or regex rule that sends the new address +// back), or when either address is not a path on this site, no redirect is +// written, a warning is logged and only the entry's URL moves; the result's +// Redirect is then nil. // // Every redirect write is recorded in history with via auto. Nothing // happens when either address is empty or they are equal. func (s *Service) AddressChanged(ctx context.Context, tx *gorm.DB, change AddressChange) (AddressResult, error) { - res := AddressResult{Locale: change.Locale, Old: strings.TrimSpace(change.Old), New: strings.TrimSpace(change.New)} + res := AddressResult{Locale: change.Locale, Old: s.sitePath(change.Old), New: s.sitePath(change.New)} if res.Old == "" || res.New == "" || res.Old == res.New { return res, nil } ctx = WithVia(ctx, models.ViaAuto) now := time.Now().UTC() - var shadowing []models.Redirect - if err := tx.Where("match = ? AND source = ?", models.RedirectExact, res.New).Find(&shadowing).Error; err != nil { + if err := s.applyAddressRedirects(ctx, tx, &res, now); err != nil { return res, err } - for _, r := range shadowing { - if err := s.deleteRedirect(ctx, tx, r); err != nil { - return res, err - } - } - - var existing []models.Redirect - if err := tx.Where("match = ? AND source = ?", models.RedirectExact, res.Old).Limit(1).Find(&existing).Error; err != nil { - return res, err - } - var redirect models.Redirect - if len(existing) > 0 { - redirect = existing[0] - before := RedirectValues(redirect) - redirect.Target, redirect.Status, redirect.UpdatedAt = res.New, models.Status301, &now - if err := tx.Save(&redirect).Error; err != nil { - return res, err - } - if err := s.recordRedirect(ctx, tx, redirect, before, RedirectValues(redirect)); err != nil { - return res, err - } - } else { - redirect = models.NewRedirect() - redirect.Source, redirect.Target, redirect.Origin = res.Old, res.New, models.OriginSlug - redirect.CreatedAt, redirect.UpdatedAt = &now, &now - if err := tx.Create(&redirect).Error; err != nil { - return res, err - } - if err := s.recordRedirect(ctx, tx, redirect, nil, RedirectValues(redirect)); err != nil { - return res, err - } - } - res.Redirect = &redirect - - var pointing []models.Redirect - targets := []string{res.Old} - if site := s.SiteURL(); site != "" { - targets = append(targets, site+res.Old) - } - if err := tx.Where("target IN ? AND id <> ?", targets, redirect.ID).Find(&pointing).Error; err != nil { - return res, err - } - for _, r := range pointing { - before := RedirectValues(r) - r.Target, r.UpdatedAt = res.New, &now - if r.Match == models.RedirectExact && s.samePath(r.Target, r.Source) { - if err := s.deleteRedirect(ctx, tx, r); err != nil { - return res, err - } - continue - } - if err := tx.Save(&r).Error; err != nil { - return res, err - } - if err := s.recordRedirect(ctx, tx, r, before, RedirectValues(r)); err != nil { - return res, err - } - res.Repointed++ - } if change.ModelType != "" && change.RecordID != "" { if err := tx.Model(&models.Entry{}). Where("model_type = ? AND record_id = ? AND locale = ?", change.ModelType, change.RecordID, change.Locale). - Updates(map[string]any{"url": res.New, "updated_at": now}).Error; err != nil { + Updates(map[string]any{"url": strings.TrimSpace(change.New), "updated_at": now}).Error; err != nil { return res, err } } return res, nil } +// sitePath trims an address and turns an absolute URL on the site URL into +// its path, so the redirect source is the form the resolver matches. +func (s *Service) sitePath(u string) string { + u = strings.TrimSpace(u) + if site := s.SiteURL(); site != "" && strings.HasPrefix(u, site+"/") { + return strings.TrimPrefix(u, site) + } + return u +} + +// applyAddressRedirects runs steps 1 to 4 of AddressChanged. +func (s *Service) applyAddressRedirects(ctx context.Context, tx *gorm.DB, res *AddressResult, now time.Time) error { + candidate := models.NewRedirect() + candidate.Source, candidate.Target, candidate.Origin = res.Old, res.New, models.OriginSlug + newPath, newQuery, _ := strings.Cut(res.New, "?") + if problems := candidate.Problems(nil); len(problems) > 0 || !strings.HasPrefix(res.New, "/") { + log.Warn("golem15.seo: address change is not a path on the site, no redirect created", "old", res.Old, "new", res.New) + return nil + } + + rules, err := LoadRedirects(tx) + if err != nil { + return err + } + kept := rules[:0:0] + for _, r := range rules { + if r.Match == models.RedirectExact && exactMatches(r, newPath, newQuery) { + if err := s.deleteRedirect(ctx, tx, r); err != nil { + return err + } + continue + } + kept = append(kept, r) + } + + // Simulate steps 2 to 4 on a copy of the remaining rules. + redirect := candidate + for _, r := range kept { + if r.Match == models.RedirectExact && r.Source == res.Old { + redirect = r + redirect.Target, redirect.Status = res.New, models.Status301 + break + } + } + targets := map[string]bool{res.Old: true} + if site := s.SiteURL(); site != "" { + targets[site+res.Old] = true + } + simulated := make([]models.Redirect, 0, len(kept)) + for _, r := range kept { + if r.ID != redirect.ID && targets[r.Target] { + r.Target = res.New + if r.Match == models.RedirectExact && s.samePath(r.Target, r.Source) { + continue + } + } + simulated = append(simulated, r) + } + check, err := checkRedirectRules(s.SiteURL(), simulated, redirect) + if err != nil { + return err + } + if check.Loop { + log.Warn("golem15.seo: address change would create a redirect loop, no redirect created", "old", res.Old, "new", res.New, "path", check.PathText()) + return nil + } + + if redirect.ID != 0 { + var stored models.Redirect + for _, r := range kept { + if r.ID == redirect.ID { + stored = r + } + } + before := RedirectValues(stored) + redirect.UpdatedAt = &now + if err := tx.Save(&redirect).Error; err != nil { + return err + } + if err := s.recordRedirect(ctx, tx, redirect, before, RedirectValues(redirect)); err != nil { + return err + } + } else { + redirect.CreatedAt, redirect.UpdatedAt = &now, &now + if err := tx.Create(&redirect).Error; err != nil { + return err + } + if err := s.recordRedirect(ctx, tx, redirect, nil, RedirectValues(redirect)); err != nil { + return err + } + } + res.Redirect = &redirect + + for _, r := range kept { + if r.ID == redirect.ID || !targets[r.Target] { + continue + } + before := RedirectValues(r) + r.Target, r.UpdatedAt = res.New, &now + if r.Match == models.RedirectExact && s.samePath(r.Target, r.Source) { + if err := s.deleteRedirect(ctx, tx, r); err != nil { + return err + } + continue + } + if err := tx.Save(&r).Error; err != nil { + return err + } + if err := s.recordRedirect(ctx, tx, r, before, RedirectValues(r)); err != nil { + return err + } + res.Repointed++ + } + return nil +} + // deleteRedirect deletes r inside tx and records it in history. func (s *Service) deleteRedirect(ctx context.Context, tx *gorm.DB, r models.Redirect) error { if err := tx.Delete(&models.Redirect{}, r.ID).Error; err != nil { diff --git a/redirects_test.go b/redirects_test.go index 540b371..81d73cf 100644 --- a/redirects_test.go +++ b/redirects_test.go @@ -374,6 +374,60 @@ func TestExactRedirectKeys(t *testing.T) { } } +// TestAddressChangeGuards checks that an address change never stores a +// redirect loop or an unusable source (WR-09): a pattern rule that sends +// the new address back to the old one stops the 301, an exact rule that +// matches the new address with its case or trailing-slash options is +// removed, an absolute old address on the site is stored as its path, and +// a new address that is not a path on the site creates nothing. +func TestAddressChangeGuards(t *testing.T) { + env := bootEnv(t) + svc := env.seoPlugin(t).Service() + change := func(old, new string) classes.AddressResult { + t.Helper() + var res classes.AddressResult + if err := env.db.Transaction(func(tx *gorm.DB) error { + var err error + res, err = svc.AddressChanged(t.Context(), tx, classes.AddressChange{Old: old, New: new}) + return err + }); err != nil { + t.Fatal(err) + } + return res + } + + env.mustRedirect(t, "/a/*", "pattern", "/b/*", 301) + if res := change("/b/x", "/a/x"); res.Redirect != nil { + t.Fatalf("/b/x -> /a/x under /a/* -> /b/*: redirect = %+v, want none (a loop)", res.Redirect) + } + if got := redirectTo(env.redirects(t), "/b/x"); got != "" { + t.Fatalf("/b/x has a redirect to %q, want none", got) + } + + env.mustRedirect(t, "/Moved/", "exact", "/elsewhere", 301, func(r *models.Redirect) { r.IgnoreCase = true }) + if res := change("/before", "/moved"); res.Redirect == nil { + t.Fatal("/before -> /moved: no redirect") + } + rows := env.redirects(t) + if got := redirectTo(rows, "/Moved/"); got != "" { + t.Fatalf("/Moved/ still redirects to %q; it shadows the record's new address", got) + } + env.expectRedirect(t, "/before", 301, siteURL+"/moved") + + if res := change(siteURL+"/abs-old", "/abs-new"); res.Redirect == nil || res.Redirect.Source != "/abs-old" { + t.Fatalf("absolute old address: result = %+v, want a redirect from /abs-old", res) + } + env.expectRedirect(t, "/abs-old", 301, siteURL+"/abs-new") + + before := env.redirectCount(t) + if res := change("/plain", "//evil.example/x"); res.Redirect != nil { + t.Fatalf("protocol-relative new address: redirect = %+v, want none", res.Redirect) + } + if got := env.redirectCount(t); got != before { + t.Fatalf("redirects = %d after a refused change, want %d", got, before) + } +} + // redirectCount is the number of stored redirects. func (e *testEnv) redirectCount(t *testing.T) int64 { t.Helper()