From 7b3b1ae89dd3e60dff6e6359298ff81dd9102d71 Mon Sep 17 00:00:00 2001
From: jeroenson <jeroen@denkgroot.com>
Date: Mon, 6 Jul 2026 09:43:08 +0200
Subject: [PATCH] Prevent redirect loops and chains when renaming pages

---
 app/models/spina/page.rb               |  2 +-
 app/models/spina/rewrite_rule.rb       | 10 +++++++
 test/models/spina/page_test.rb         | 10 +++++++
 test/models/spina/rewrite_rule_test.rb | 40 ++++++++++++++++++++++++++
 4 files changed, 61 insertions(+), 1 deletion(-)
 create mode 100644 test/models/spina/rewrite_rule_test.rb

diff --git a/app/models/spina/page.rb b/app/models/spina/page.rb
index 0c484748a..c2c1499c0 100644
--- a/app/models/spina/page.rb
+++ b/app/models/spina/page.rb
@@ -115,7 +115,7 @@ def touch_navigations
     end
 
     def rewrite_rule
-      RewriteRule.where(old_path: old_path).first_or_create.update(new_path: materialized_path) if old_path != materialized_path
+      RewriteRule.record(old_path, materialized_path)
     end
 
     def localized_materialized_path
diff --git a/app/models/spina/rewrite_rule.rb b/app/models/spina/rewrite_rule.rb
index c9d9082f1..b74ded129 100644
--- a/app/models/spina/rewrite_rule.rb
+++ b/app/models/spina/rewrite_rule.rb
@@ -1,5 +1,15 @@
 module Spina
   class RewriteRule < ApplicationRecord
     validates :old_path, uniqueness: true
+
+    # Records a redirect from old_path to new_path without leaving
+    # loops (A -> B and B -> A) or chains (A -> B -> C) behind.
+    def self.record(old_path, new_path)
+      return if old_path.blank? || old_path == new_path
+
+      where(old_path: new_path).delete_all
+      where(new_path: old_path).update_all(new_path: new_path)
+      where(old_path: old_path).first_or_create.update(new_path: new_path)
+    end
   end
 end
diff --git a/test/models/spina/page_test.rb b/test/models/spina/page_test.rb
index 6d6f29841..ad25499aa 100644
--- a/test/models/spina/page_test.rb
+++ b/test/models/spina/page_test.rb
@@ -40,6 +40,16 @@ def setup
       assert_equal "/custom-slug", @demo.materialized_path
     end
 
+    test "renaming a page back and forth does not leave redirect loops or chains" do
+      page = FactoryBot.create(:page, title: "Looping page")
+      page.update(url_title: "somewhere-else")
+      page.update(url_title: "looping-page")
+
+      rules = RewriteRule.pluck(:old_path, :new_path)
+      assert_not rules.any? { |old_path, new_path| rules.include?([new_path, old_path]) }
+      assert_equal [page.materialized_path], RewriteRule.distinct.pluck(:new_path)
+    end
+
     test "build slug from ancestors" do
       about = FactoryBot.create :about_page
       page = FactoryBot.create :services_page
diff --git a/test/models/spina/rewrite_rule_test.rb b/test/models/spina/rewrite_rule_test.rb
new file mode 100644
index 000000000..5cf466c3a
--- /dev/null
+++ b/test/models/spina/rewrite_rule_test.rb
@@ -0,0 +1,40 @@
+require "test_helper"
+
+module Spina
+  class RewriteRuleTest < ActiveSupport::TestCase
+    test "record creates a rewrite rule" do
+      RewriteRule.record("/a", "/b")
+
+      assert_equal [["/a", "/b"]], RewriteRule.pluck(:old_path, :new_path)
+    end
+
+    test "record skips blank or unchanged paths" do
+      RewriteRule.record(nil, "/a")
+      RewriteRule.record("", "/a")
+      RewriteRule.record("/a", "/a")
+
+      assert_empty RewriteRule.all
+    end
+
+    test "record updates an existing rule for the same old_path" do
+      RewriteRule.record("/a", "/b")
+      RewriteRule.record("/a", "/c")
+
+      assert_equal [["/a", "/c"]], RewriteRule.pluck(:old_path, :new_path)
+    end
+
+    test "renaming back does not leave a redirect loop" do
+      RewriteRule.record("/a", "/b")
+      RewriteRule.record("/b", "/a")
+
+      assert_equal [["/b", "/a"]], RewriteRule.pluck(:old_path, :new_path)
+    end
+
+    test "renaming again does not leave a redirect chain" do
+      RewriteRule.record("/a", "/b")
+      RewriteRule.record("/b", "/c")
+
+      assert_equal [["/a", "/c"], ["/b", "/c"]], RewriteRule.pluck(:old_path, :new_path).sort
+    end
+  end
+end