From 04450e10866fc56890426525062fd5cdf56c4300 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Fri, 17 Jul 2026 19:30:43 +0200 Subject: Allowlist aggregate order columns and template names Page.aggregate interpolated order_by into SQL unchecked while already allowlisting order_direction; the column is now normalized and checked against the sortable columns, falling back to pages.id. Its values arrive from editor-authored aggregate shortcodes, so this was editor-gated, but the asymmetry was wrong regardless. template_name and default_template_name now validate inclusion in Page.custom_templates -- names render as filesystem paths, so only names actually present in the template directory are acceptable. Validated only on change: legacy rows whose template file has since vanished stay saveable, and valid_template's render-time fallback to standard_template continues to cover them. Two tests that wrote fabricated template names through the front door now arrange their state correctly (update_column for the stale-name fallback test, a real template for the update-persists test). --- app/models/node.rb | 5 +++++ app/models/page.rb | 15 +++++++++++++- test/controllers/content_controller_test.rb | 3 +-- test/controllers/nodes_controller_test.rb | 14 +++++++++++-- test/models/node_test.rb | 9 ++++++++ test/models/page_test.rb | 32 +++++++++++++++++++++++++++++ 6 files changed, 73 insertions(+), 5 deletions(-) diff --git a/app/models/node.rb b/app/models/node.rb index 4f585e1..70ed4da 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -22,6 +22,11 @@ class Node < ApplicationRecord validates_uniqueness_of :slug, :scope => :parent_id, :unless => -> { parent_id.nil? } validates_presence_of :parent_id, :unless => -> { Node.root.nil? } + validates :default_template_name, + :inclusion => { :in => ->(_) { Page.custom_templates } }, + :allow_nil => true, + :if => :default_template_name_changed? + # Class methods # Returns a page for a given node. If no revision is supplied, it returns diff --git a/app/models/page.rb b/app/models/page.rb index e5a8d9d..be21ff7 100644 --- a/app/models/page.rb +++ b/app/models/page.rb @@ -11,6 +11,16 @@ class Page < ApplicationRecord translates :title, :abstract, :body # Globalize2 + # Template names render as filesystem paths; only names actually + # present in the public template directory are acceptable. Validated + # only on change so legacy rows whose template file has since + # vanished stay saveable -- valid_template already falls back to + # standard_template for those at render time. + validates :template_name, + :inclusion => { :in => ->(_) { Page.custom_templates } }, + :allow_nil => true, + :if => :template_name_changed? + # Associations belongs_to :node, optional: true belongs_to :user, optional: true @@ -77,7 +87,10 @@ class Page < ApplicationRecord .paginate(:page => page, :per_page => options[:limit]) end - scope.order("#{options[:order_by]} #{direction}") + column = options[:order_by].to_s.sub(/\Apages\./, "") + column = "id" unless %w[id published_at created_at updated_at].include?(column) + + scope.order("pages.#{column} #{direction}") .paginate(:page => page, :per_page => options[:limit]) end diff --git a/test/controllers/content_controller_test.rb b/test/controllers/content_controller_test.rb index e8bc55f..2449a0c 100644 --- a/test/controllers/content_controller_test.rb +++ b/test/controllers/content_controller_test.rb @@ -73,8 +73,7 @@ class ContentControllerTest < ActionController::TestCase def test_nonexistant_custom_template_defaults_to_standard_template new_node = create_node_under_root "fnord" draft = find_or_create_draft(new_node, @user1) - draft.template_name = "huchibu" - draft.save + draft.update_column(:template_name, "huchibu") new_node.publish_draft! get :render_page, params: { :locale => 'de', :page_path => ["fnord"] } diff --git a/test/controllers/nodes_controller_test.rb b/test/controllers/nodes_controller_test.rb index 5b66bd3..b0d7416 100644 --- a/test/controllers/nodes_controller_test.rb +++ b/test/controllers/nodes_controller_test.rb @@ -140,7 +140,7 @@ class NodesControllerTest < ActionController::TestCase :page => { :title => "Hello", :body => "There", - :template_name => "Foobar" + :template_name => "title_only" } } @@ -148,7 +148,17 @@ class NodesControllerTest < ActionController::TestCase test_node.reload assert_equal "Hello", test_node.head.title assert_equal "There", test_node.head.body - assert_equal "Foobar", test_node.head.template_name + assert_equal "title_only", test_node.head.template_name + end + + def test_update_rejects_a_template_name_not_on_disk + test_node = Node.root.children.create! :slug => "test_node" + login_as :quentin + put :update, params: { :id => test_node.id, + :page => { :title => "Hello", :template_name => "Foobar" } } + + test_node.reload + assert_not_equal "Foobar", test_node.draft&.template_name end test "publish draft with staged_slug unqueal slug" do diff --git a/test/models/node_test.rb b/test/models/node_test.rb index c8e49e5..4401651 100644 --- a/test/models/node_test.rb +++ b/test/models/node_test.rb @@ -761,4 +761,13 @@ class NodeTest < ActiveSupport::TestCase assert_equal "Second", action.metadata.dig("title", "from") assert_equal "First", action.metadata.dig("title", "to") end + + test "default_template_name rejects values not present in the template directory" do + node = Node.root.children.build(:slug => "template_guard_test", + :default_template_name => "../../layouts/admin") + assert_not node.valid? + + node.default_template_name = "standard_template" + assert node.valid? + end end diff --git a/test/models/page_test.rb b/test/models/page_test.rb index edb7c37..58794d5 100644 --- a/test/models/page_test.rb +++ b/test/models/page_test.rb @@ -370,4 +370,36 @@ class PageTest < ActiveSupport::TestCase assert en_entry[:changed] refute en_entry[:exists_there] end + + test "aggregate ignores order_by values outside the allowlist" do + sql = Page.aggregate(:order_by => "pages.id; DROP TABLE pages--").to_sql + + assert_not_includes sql, "DROP" + assert_includes sql, "pages.id ASC" + end + + test "aggregate accepts allowlisted order columns, bare or prefixed" do + assert_includes Page.aggregate(:order_by => "published_at").to_sql, + "pages.published_at ASC" + assert_includes Page.aggregate(:order_by => "pages.published_at").to_sql, + "pages.published_at ASC" + end + + test "template_name rejects values not present in the template directory" do + page = Page.create!(:title => "Template guard") + + page.template_name = "../../partials/_article" + assert_not page.valid? + + page.template_name = "standard_template" + assert page.valid? + end + + test "a stale legacy template_name does not block unrelated saves" do + page = Page.create!(:title => "Stale template") + page.update_column(:template_name, "long_deleted_template") + + page.reload + assert page.update(:abstract => "still saveable") + end end -- cgit v1.3