[AGILE-176] Harden backlog filter param parsing

Tampered query strings such as `?bucket_ids[0]=5` arrive as a nested
`ActionController::Parameters` and crashed `from_params` with an
`undefined method 'to_i'` error. Parsing now tokenizes only strings
and arrays and ignores nested structures, so malformed input is
dropped instead of raising a 500.

Serialization joins id lists into the compact comma form, so the
generated URLs stay bookmarkable (`?bucket_ids=1,2` rather than the
bracketed array form) and passthrough filters render as a single
hidden field instead of duplicated `name[]` inputs.

https://community.openproject.org/wp/AGILE-176

Author: myabc

modules/backlogs/app/components/backlogs/backlog_filter_select_panel_component.rb

@@ -47,10 +47,7 @@ def initialize(project:, field_name:)
     def filter_fields_for
       backlog_filter_params
         .except(filter_field)
-        .flat_map do |name, value|
-          field_name = value.is_a?(Array) ? "#{name}[]" : name
-          Array(value).map { |v| [field_name, v, { id: nil }] }
-        end
+        .map { |name, value| [name, value, { id: nil }] }
     end
 
     def items

modules/backlogs/app/controllers/backlogs/backlog_filters.rb

@@ -32,22 +32,41 @@ module Backlogs
   BacklogFilters = Data.define(:bucket_ids, :sprint_ids, :show_all) do
     def self.from_params(params)
       new(
-        bucket_ids: Array(params[:bucket_ids]).filter_map do |id|
-                      id == "inbox" ? "inbox" : id.to_i.nonzero?
-                    end.presence,
-        sprint_ids: Array(params[:sprint_ids]).filter_map { |id| id.to_i.nonzero? }.presence,
+        bucket_ids: parse_ids(params[:bucket_ids]) { |id| id == "inbox" ? "inbox" : id.to_i.nonzero? },
+        sprint_ids: parse_ids(params[:sprint_ids]) { |id| id.to_i.nonzero? },
         show_all: ActiveRecord::Type::Boolean.new.cast(params[:all]) || false
       )
     end
 
+    # Coerces a raw filter param into a deduplicated list of ids, or nil when
+    # empty. Accepts both the compact comma-delimited form ("1,2,inbox") and a
+    # plain array (["1", "2"]). Tampered or nested structures (e.g. the URL
+    # +?bucket_ids[0]=5+, which arrives as a Hash/ActionController::Parameters)
+    # are ignored rather than raising.
+    def self.parse_ids(raw, &)
+      tokenize(raw).filter_map(&).uniq.presence
+    end
+
+    def self.tokenize(raw)
+      case raw
+      when String then raw.split(",")
+      when Array then raw
+      else []
+      end.filter_map { |value| value.to_s.strip.presence }
+    end
+    private_class_method :tokenize
+
     def show_inbox?
       bucket_ids.nil? || bucket_ids.include?("inbox")
     end
 
+    # Serializes back to query params, joining id lists into the compact
+    # comma-delimited form so generated URLs stay bookmarkable
+    # (+?bucket_ids=1,2+ rather than +?bucket_ids[]=1&bucket_ids[]=2+).
     def to_h
       result = show_all? ? { all: true } : {}
-      result[:bucket_ids] = bucket_ids if bucket_ids
-      result[:sprint_ids] = sprint_ids if sprint_ids
+      result[:bucket_ids] = bucket_ids.join(",") if bucket_ids
+      result[:sprint_ids] = sprint_ids.join(",") if sprint_ids
       result
     end
 

modules/backlogs/spec/components/backlogs/backlog_filter_select_panel_component_spec.rb

@@ -88,26 +88,23 @@ def render_component(field_name:, **params)
   describe "hidden filter fields" do
     it "passes through sprint_ids when rendering the bucket panel" do
       render_component(field_name: :bucket_ids, sprint_ids: ["1"])
-      expect(page).to have_field("sprint_ids[]", type: :hidden, with: "1", visible: :all)
+      expect(page).to have_field("sprint_ids", type: :hidden, with: "1", visible: :all)
     end
 
     it "passes through bucket_ids when rendering the sprint panel" do
       render_component(field_name: :sprint_ids, bucket_ids: ["2"])
-      expect(page).to have_field("bucket_ids[]", type: :hidden, with: "2", visible: :all)
+      expect(page).to have_field("bucket_ids", type: :hidden, with: "2", visible: :all)
     end
 
-    it "expands array values into multiple hidden inputs" do
+    it "joins array values into a single comma-delimited hidden input" do
       render_component(field_name: :sprint_ids, bucket_ids: [1, 2])
-      expect(page).to have_field("bucket_ids[]", type: :hidden, with: "1", visible: :all)
-      expect(page).to have_field("bucket_ids[]", type: :hidden, with: "2", visible: :all)
+      expect(page).to have_field("bucket_ids", type: :hidden, with: "1,2", visible: :all)
     end
 
-    it "passes through scalar params as a single hidden input without brackets" do
+    it "renders each passthrough param once per form (clear + filter)" do
       render_component(field_name: :sprint_ids, all: true)
-      # The clear form has 1 `all`, the filter form has 1 `all` and 1 `sprint_ids[]` parameter
-      expect(page).to have_field(type: :hidden, count: 3, visible: :all)
+      # The clear form carries 1 `all`, the filter form carries another.
       expect(page).to have_field("all", type: :hidden, with: "true", count: 2, visible: :all)
-      expect(page).to have_field("sprint_ids[]", type: :hidden, count: 1, visible: :all)
     end
   end
 end

modules/backlogs/spec/controllers/backlogs/backlog_filters_spec.rb

@@ -55,6 +55,38 @@
         expect(filters.bucket_ids).to eq([1, 2])
       end
     end
+
+    context "when bucket_ids are a comma-delimited string" do
+      let(:params) { { bucket_ids: "1, 2 ,inbox" } }
+
+      it "splits, trims and preserves the inbox sentinel" do
+        expect(filters.bucket_ids).to eq([1, 2, "inbox"])
+      end
+    end
+
+    context "when bucket_ids repeat" do
+      let(:params) { { bucket_ids: "1,1,2" } }
+
+      it "deduplicates" do
+        expect(filters.bucket_ids).to eq([1, 2])
+      end
+    end
+
+    context "when bucket_ids are a tampered nested structure (?bucket_ids[0]=5)" do
+      let(:params) { ActionController::Parameters.new(bucket_ids: { "0" => "5" }) }
+
+      it "ignores them without raising" do
+        expect(filters.bucket_ids).to be_nil
+      end
+    end
+
+    context "when bucket_ids are a tampered array of structures (?bucket_ids[][x]=5)" do
+      let(:params) { ActionController::Parameters.new(bucket_ids: [{ "x" => "5" }]) }
+
+      it "ignores them without raising" do
+        expect(filters.bucket_ids).to be_nil
+      end
+    end
   end
 
   describe "#sprint_ids" do
@@ -119,16 +151,24 @@
     context "with bucket_ids and sprint_ids" do
       let(:params) { { bucket_ids: %w[1 2], sprint_ids: %w[3] } }
 
-      it "includes both" do
-        expect(filters.to_h).to eq({ bucket_ids: [1, 2], sprint_ids: [3] })
+      it "joins both into compact comma-delimited strings" do
+        expect(filters.to_h).to eq({ bucket_ids: "1,2", sprint_ids: "3" })
       end
     end
 
     context "with all params combined" do
       let(:params) { { all: "1", bucket_ids: %w[1], sprint_ids: %w[2] } }
 
       it "includes everything" do
-        expect(filters.to_h).to eq({ all: true, bucket_ids: [1], sprint_ids: [2] })
+        expect(filters.to_h).to eq({ all: true, bucket_ids: "1", sprint_ids: "2" })
+      end
+    end
+
+    context "with the inbox sentinel among bucket_ids" do
+      let(:params) { { bucket_ids: ["5", "inbox"] } }
+
+      it "keeps the sentinel in the serialized string" do
+        expect(filters.to_h).to eq({ bucket_ids: "5,inbox" })
       end
     end
   end

modules/backlogs/spec/helpers/backlogs/common_helper_spec.rb

@@ -80,7 +80,7 @@
       let(:params) { { bucket_ids: %w[1 2], sprint_ids: %w[3] } }
 
       it "includes both in to_h" do
-        expect(helper.backlog_filters.to_h).to eq({ bucket_ids: [1, 2], sprint_ids: [3] })
+        expect(helper.backlog_filters.to_h).to eq({ bucket_ids: "1,2", sprint_ids: "3" })
       end
     end
   end