From aea211c0b55546f5d8651f324c4067d7ac81b0a4 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Fri, 13 Mar 2026 09:49:59 +0100 Subject: [PATCH 01/22] Prevent custom field names to contain ascii control characters --- app/models/custom_field.rb | 2 + .../remove_ascii_control_characters.rb | 10 +++ .../remove_ascii_control_characters_spec.rb | 61 +++++++++++++++++++ spec/models/custom_field_spec.rb | 10 +++ 4 files changed, 83 insertions(+) create mode 100644 lib/open_project/remove_ascii_control_characters.rb create mode 100644 spec/lib/open_project/remove_ascii_control_characters_spec.rb diff --git a/app/models/custom_field.rb b/app/models/custom_field.rb index f00fde6cbb3..95427f38fe4 100644 --- a/app/models/custom_field.rb +++ b/app/models/custom_field.rb @@ -32,6 +32,8 @@ class CustomField < ApplicationRecord include CustomField::OrderStatements include CustomField::CalculatedValue + normalizes :name, with: OpenProject::RemoveAsciiControlCharacters + has_many :custom_values, dependent: :delete_all # WARNING: the inverse_of option is also required in order # for the 'touch: true' option on the custom_field association in CustomOption diff --git a/lib/open_project/remove_ascii_control_characters.rb b/lib/open_project/remove_ascii_control_characters.rb new file mode 100644 index 00000000000..0e189ae6b1d --- /dev/null +++ b/lib/open_project/remove_ascii_control_characters.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +module OpenProject + # Strips ASCII control characters (0x00–0x1F, 0x7F) from a string. + # Designed for use with ActiveRecord's `normalizes` API: + # + # normalizes :name, with: OpenProject::RemoveAsciiControlCharacters + # + RemoveAsciiControlCharacters = ->(value) { value.is_a?(String) ? value.gsub(/[\x00-\x1F\x7F]/, "") : value } +end diff --git a/spec/lib/open_project/remove_ascii_control_characters_spec.rb b/spec/lib/open_project/remove_ascii_control_characters_spec.rb new file mode 100644 index 00000000000..d7394c18b55 --- /dev/null +++ b/spec/lib/open_project/remove_ascii_control_characters_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe OpenProject::RemoveAsciiControlCharacters do + subject(:call) { described_class.call(value) } + + context "with a clean string" do + let(:value) { "Hello World" } + + it { is_expected.to eq("Hello World") } + end + + context "with newline and tab characters" do + let(:value) { "Hello\n\tWorld\r\n" } + + it { is_expected.to eq("HelloWorld") } + end + + context "with null byte" do + let(:value) { "Hello\x00World" } + + it { is_expected.to eq("HelloWorld") } + end + + context "with escape and delete characters" do + let(:value) { "Hello\x1B\x7FWorld" } + + it { is_expected.to eq("HelloWorld") } + end + + context "with a mix of control characters" do + let(:value) { "\x01He\x02llo\x03 \x04Wo\x05rld\x06" } + + it { is_expected.to eq("Hello World") } + end + + context "with Unicode characters (preserved)" do + let(:value) { "Héllo Wörld 日本語" } + + it { is_expected.to eq("Héllo Wörld 日本語") } + end + + context "with spaces and quotes (preserved)" do + let(:value) { %{It's a "test" value} } + + it { is_expected.to eq(%{It's a "test" value}) } + end + + context "with a non-string value" do + let(:value) { 42 } + + it { is_expected.to eq(42) } + end + + context "with nil" do + let(:value) { nil } + + it { is_expected.to be_nil } + end +end diff --git a/spec/models/custom_field_spec.rb b/spec/models/custom_field_spec.rb index cc3f6f70ccd..266ea12053f 100644 --- a/spec/models/custom_field_spec.rb +++ b/spec/models/custom_field_spec.rb @@ -44,6 +44,16 @@ RSpec.describe CustomField do it { is_expected.to validate_presence_of(:name) } it { is_expected.to validate_length_of(:name).is_at_most(256) } + it "strips ASCII control characters on assignment" do + cf = build(:custom_field, name: "Test\nField\x00Name\t!") + expect(cf.name).to eq("TestFieldName!") + end + + it "preserves Unicode and normal characters" do + cf = build(:custom_field, name: "Héllo Wörld 日本語") + expect(cf.name).to eq("Héllo Wörld 日本語") + end + describe "uniqueness" do describe "WHEN value, locale and type are identical" do before do From 41f3e474753a4c8d49cebe136e8d8cd6d9d185e5 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Fri, 13 Mar 2026 09:52:27 +0100 Subject: [PATCH 02/22] Run migration to remove ascii control characters from Custom Field Names --- ...trol_characters_from_custom_field_names.rb | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 db/migrate/20260313120000_strip_control_characters_from_custom_field_names.rb diff --git a/db/migrate/20260313120000_strip_control_characters_from_custom_field_names.rb b/db/migrate/20260313120000_strip_control_characters_from_custom_field_names.rb new file mode 100644 index 00000000000..aec50ce1ec6 --- /dev/null +++ b/db/migrate/20260313120000_strip_control_characters_from_custom_field_names.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +class StripControlCharactersFromCustomFieldNames < ActiveRecord::Migration[7.1] + def up + execute <<~SQL.squish + UPDATE custom_fields + SET name = regexp_replace(name, E'[\\x01-\\x1F\\x7F]', '', 'g') + WHERE name ~ E'[\\x01-\\x1F\\x7F]' + SQL + end + + def down + # Irreversible — stripped characters cannot be restored + end +end From 5ae66849a1213ebcba551fccb1c829faf08fab9b Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Fri, 13 Mar 2026 09:58:30 +0100 Subject: [PATCH 03/22] Do not put user-controller custom field name in SQL query comment --- .../models/cost_query/custom_field_mixin.rb | 12 +++---- .../cost_query/custom_field_mixin_spec.rb | 36 +++++++++++++++++++ 2 files changed, 42 insertions(+), 6 deletions(-) create mode 100644 modules/reporting/spec/models/cost_query/custom_field_mixin_spec.rb diff --git a/modules/reporting/app/models/cost_query/custom_field_mixin.rb b/modules/reporting/app/models/cost_query/custom_field_mixin.rb index 9eedfdc55b8..d101ae75461 100644 --- a/modules/reporting/app/models/cost_query/custom_field_mixin.rb +++ b/modules/reporting/app/models/cost_query/custom_field_mixin.rb @@ -113,7 +113,7 @@ module CostQuery::CustomFieldMixin custom_options_table = CustomOption.table_name <<-SQL - -- BEGIN Custom Field Join: #{db_field} + -- BEGIN Custom Field Join: cf_#{field.id} LEFT OUTER JOIN ( SELECT co.id AS #{db_field}, @@ -129,16 +129,16 @@ module CostQuery::CustomFieldMixin AND #{db_field}.custom_field_id = #{field.id} AND #{db_field}.customized_id = entries.entity_id - -- END Custom Field Join: #{db_field} + -- END Custom Field Join: cf_#{field.id} SQL end def default_join_table(field) - <<-SQL % [CustomValue.table_name, table_name, field.id, field.name, SQL_TYPES[field.field_format]] - -- BEGIN Custom Field Join: "%4$s" + <<-SQL % [CustomValue.table_name, table_name, field.id, SQL_TYPES[field.field_format]] + -- BEGIN Custom Field Join: cf_%3$d LEFT OUTER JOIN ( \tSELECT - \t\tCAST(value AS %5$s) AS %2$s, + \t\tCAST(value AS %4$s) AS %2$s, \t\tcustomized_type, \t\tcustom_field_id, \t\tcustomized_id @@ -148,7 +148,7 @@ module CostQuery::CustomFieldMixin ON %2$s.customized_type = 'WorkPackage' AND %2$s.custom_field_id = %3$d AND %2$s.customized_id = entries.entity_id - -- END Custom Field Join: "%4$s" + -- END Custom Field Join: cf_%3$d SQL end diff --git a/modules/reporting/spec/models/cost_query/custom_field_mixin_spec.rb b/modules/reporting/spec/models/cost_query/custom_field_mixin_spec.rb new file mode 100644 index 00000000000..9023483bcbd --- /dev/null +++ b/modules/reporting/spec/models/cost_query/custom_field_mixin_spec.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +require_relative "../../spec_helper" + +RSpec.describe CostQuery::CustomFieldMixin, :reporting_query_helper do + minimal_query + + let!(:project) { create(:project_with_types) } + let!(:user) { create(:admin) } + + describe "#default_join_table" do + let!(:custom_field) do + create(:wp_custom_field, :string, name: "Robert'); DROP TABLE Students;-- Roberts") + end + + before do + CostQuery::Cache.reset! + CostQuery::Filter::CustomFieldEntries.all + end + + after do + CostQuery::Cache.reset! + CostQuery::Filter::CustomFieldEntries.reset! + end + + it "uses field.id in the SQL comment and does not include the field name" do + query.filter custom_field.attribute_name, operator: "=", value: "test" + sql = query.sql_statement.to_s + + expect(sql).to include("-- BEGIN Custom Field Join: cf_#{custom_field.id}") + expect(sql).to include("-- END Custom Field Join: cf_#{custom_field.id}") + expect(sql).not_to include("DROP TABLE students") + expect(sql).to include("CAST(value AS varchar)") + end + end +end From 33d208ed659e1532117c74c370a8dbee80f2e2ca Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Fri, 13 Mar 2026 09:59:24 +0100 Subject: [PATCH 04/22] Do not arbitrarily load all code in plugins --- config/initializers/00-load_plugins.rb | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/config/initializers/00-load_plugins.rb b/config/initializers/00-load_plugins.rb index 2ea001b64f4..e2de30f5ab6 100644 --- a/config/initializers/00-load_plugins.rb +++ b/config/initializers/00-load_plugins.rb @@ -31,7 +31,16 @@ # TODO: check if this can be postponed and if some plugins can make use of the ActiveSupport.on_load hooks # Loads the core plugins located in lib_static/plugins -Dir.glob(Rails.root.join("lib_static/plugins/*")).each do |directory| +CORE_PLUGINS = %w[ + acts_as_attachable + acts_as_customizable + acts_as_event + acts_as_journalized + acts_as_searchable + verification +].freeze + +CORE_PLUGINS.map { |name| Rails.root.join("lib_static/plugins", name).to_s }.each do |directory| if File.directory?(directory) lib = File.join(directory, "lib") From ec8f5f1265e42e8230bc3211a3c0f85bf36eb24a Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Fri, 13 Mar 2026 10:03:57 +0100 Subject: [PATCH 05/22] Prevent escaping the checkout root path for repositories --- lib/open_project/scm/adapters/git.rb | 11 +++++--- lib/open_project/scm/manageable_repository.rb | 9 ++++++- .../scm/adapters/git_adapter_spec.rb | 27 +++++++++++++++++++ spec/models/repository/git_spec.rb | 12 +++++++++ 4 files changed, 55 insertions(+), 4 deletions(-) diff --git a/lib/open_project/scm/adapters/git.rb b/lib/open_project/scm/adapters/git.rb index cca3094bd25..c10c09ba002 100644 --- a/lib/open_project/scm/adapters/git.rb +++ b/lib/open_project/scm/adapters/git.rb @@ -201,9 +201,14 @@ module OpenProject end def checkout_path - Pathname(OpenProject::Configuration.scm_local_checkout_path) - .join(@identifier) - .expand_path + root = Pathname(OpenProject::Configuration.scm_local_checkout_path).expand_path.to_s + path = Pathname(root).join(@identifier).expand_path.to_s + + unless path.start_with?("#{root}/") + raise ArgumentError, "Checkout path escapes the configured root directory" + end + + Pathname(path) end def checkout_uri diff --git a/lib/open_project/scm/manageable_repository.rb b/lib/open_project/scm/manageable_repository.rb index 4ed2ce65301..f5cc07db129 100644 --- a/lib/open_project/scm/manageable_repository.rb +++ b/lib/open_project/scm/manageable_repository.rb @@ -111,7 +111,14 @@ module OpenProject # Used only in the creation of a repository, at a later point # in time, it is referred to in the root_url def managed_repository_path - File.join(self.class.managed_root, repository_identifier) + root = File.expand_path(self.class.managed_root) + path = File.expand_path(File.join(root, repository_identifier)) + + unless path.start_with?("#{root}/") + raise ArgumentError, "Repository path escapes the configured managed root directory" + end + + path end ## diff --git a/spec/lib/open_project/scm/adapters/git_adapter_spec.rb b/spec/lib/open_project/scm/adapters/git_adapter_spec.rb index 58f3dc40147..083983b2780 100644 --- a/spec/lib/open_project/scm/adapters/git_adapter_spec.rb +++ b/spec/lib/open_project/scm/adapters/git_adapter_spec.rb @@ -550,6 +550,33 @@ RSpec.describe OpenProject::SCM::Adapters::Git do end end + describe "#checkout_path" do + let(:repos_dir) { Dir.mktmpdir } + + before do + allow(OpenProject::Configuration) + .to receive(:scm_local_checkout_path) + .and_return(repos_dir) + end + + after { FileUtils.remove_entry(repos_dir) } + + it "returns a path within the configured root for a normal identifier" do + adapter = described_class.new("file:///some/repo.git", nil, nil, nil, nil, "my-project") + expect(adapter.checkout_path.to_s).to eq(File.join(repos_dir, "my-project")) + end + + it "raises ArgumentError when the identifier contains path traversal" do + adapter = described_class.new("file:///some/repo.git", nil, nil, nil, nil, "../../etc") + expect { adapter.checkout_path }.to raise_error(ArgumentError, /escapes the configured root/) + end + + it "raises ArgumentError for an identifier that resolves to the root itself" do + adapter = described_class.new("file:///some/repo.git", nil, nil, nil, nil, ".") + expect { adapter.checkout_path }.to raise_error(ArgumentError, /escapes the configured root/) + end + end + context "with a local repository" do it_behaves_like "git adapter specs" end diff --git a/spec/models/repository/git_spec.rb b/spec/models/repository/git_spec.rb index 8f8fa0b4548..f855bb1f21b 100644 --- a/spec/models/repository/git_spec.rb +++ b/spec/models/repository/git_spec.rb @@ -125,6 +125,18 @@ RSpec.describe Repository::Git do end end + context "and project with traversal identifier" do + before do + instance.project = project + allow(instance).to receive(:repository_identifier).and_return("../../etc/evil") + end + + it "raises ArgumentError for path traversal" do + expect { instance.managed_repository_path } + .to raise_error(ArgumentError, /escapes the configured managed root/) + end + end + context "and associated project with parent" do let(:parent) { build(:project) } let(:project) { build(:project, parent:) } From afc0012a72178c105ae8db685114edfb4277b841 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Fri, 13 Mar 2026 14:15:07 +0100 Subject: [PATCH 06/22] Refactor HTML generation for repositories --- app/helpers/repositories_helper.rb | 61 +++-- spec/helpers/repositories_helper_spec.rb | 278 +++++++++++++++++++++++ 2 files changed, 308 insertions(+), 31 deletions(-) create mode 100644 spec/helpers/repositories_helper_spec.rb diff --git a/app/helpers/repositories_helper.rb b/app/helpers/repositories_helper.rb index 7b3d9802693..5e8343e159d 100644 --- a/app/helpers/repositories_helper.rb +++ b/app/helpers/repositories_helper.rb @@ -114,13 +114,13 @@ module RepositoriesHelper end def render_changes_tree(tree) - return "" if tree.nil? + return "".html_safe if tree.nil? - output = +"
    " - tree.keys.sort.each do |file| - style = +"change" + items = tree.keys.sort.flat_map do |file| + style = "change" text = File.basename(file) - if s = tree[file][:s] + + if (s = tree[file][:s]) style += " folder" path_param = without_leading_slash(to_path_param(@repository.relative_path(file))) text = link_to(h(text), @@ -129,38 +129,40 @@ module RepositoriesHelper rev: @changeset.identifier), title: I18n.t(:label_folder)) - output += "
  • #{text}
    • " - output += render_changes_tree(s) - elsif c = tree[file][:c] + folder_li = content_tag(:li, text, + class: "#{style} icon icon-folder-#{calculate_folder_action(s)}") + [folder_li, render_changes_tree(s)] + elsif (c = tree[file][:c]) style += " change-#{c.action}" path_param = without_leading_slash(to_path_param(@repository.relative_path(c.path))) - unless c.action == "D" - title_text = changes_tree_change_title c.action + text_parts = [] + unless c.action == "D" text = link_to(h(text), entry_revision_project_repository_path(project_id: @project, repo_path: path_param, rev: @changeset.identifier), - title: title_text) + title: changes_tree_change_title(c.action)) end - text << raw(" - #{h(c.revision)}") if c.revision.present? + text_parts << text + text_parts << " - " << h(c.revision) if c.revision.present? if c.action == "M" - text << raw(" (" + link_to(I18n.t(:label_diff), - diff_revision_project_repository_path(project_id: @project, - repo_path: path_param, - rev: @changeset.identifier)) + ") ") + text_parts << " (" << link_to(I18n.t(:label_diff), + diff_revision_project_repository_path(project_id: @project, + repo_path: path_param, + rev: @changeset.identifier)) << ") " end - text << raw(" " + content_tag("span", h(c.from_path), class: "copied-from")) if c.from_path.present? + text_parts << " " << content_tag(:span, c.from_path, class: "copied-from") if c.from_path.present? - output += changes_tree_li_element(c.action, text, style) + [changes_tree_li_element(c.action, safe_join(text_parts), style)] end - end - output += "
" - output.html_safe + end.compact + + content_tag(:ul, safe_join(items)) end def to_utf8_for_repositories(str) @@ -296,19 +298,16 @@ module RepositoriesHelper def changes_tree_li_element(action, text, style) icon_name = case action - when "A" - "icon-add" - when "D" - "icon-delete" - when "C" - "icon-copy" - when "R" - "icon-rename" + when "A" then "icon-add" + when "D" then "icon-delete" + when "C" then "icon-copy" + when "R" then "icon-rename" else "icon-arrow-left-right" end - "
  • #{text}
    • " + content_tag(:li, text, + class: "#{style} icon #{icon_name}", + title: changes_tree_change_title(action)) end end diff --git a/spec/helpers/repositories_helper_spec.rb b/spec/helpers/repositories_helper_spec.rb new file mode 100644 index 00000000000..9d6c9ec630b --- /dev/null +++ b/spec/helpers/repositories_helper_spec.rb @@ -0,0 +1,278 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +require "spec_helper" + +RSpec.describe RepositoriesHelper do + let(:project) { build_stubbed(:project) } + let(:repository) { build_stubbed(:repository_subversion, project:) } + let(:changeset) { build_stubbed(:changeset, repository:, revision: "42") } + + before do + assign(:project, project) + assign(:repository, repository) + assign(:changeset, changeset) + + allow(repository).to receive(:relative_path) { |path| path } + + allow(helper).to receive_messages( + show_revisions_path_project_repository_path: "/revisions", + entry_revision_project_repository_path: "/entry", + diff_revision_project_repository_path: "/diff" + ) + end + + describe "#changes_tree_li_element" do + it "escapes plain text content" do + malicious_text = '' + result = helper.changes_tree_li_element("D", malicious_text, "change change-D") + + expect(result).not_to include("') + } + } + end + + it "escapes the revision" do + result = helper.render_changes_tree(tree) + + expect(result).not_to include("