diff --git a/app/controllers/api/v1/transactions_controller.rb b/app/controllers/api/v1/transactions_controller.rb index b5942b7fca4..1e59b52839e 100644 --- a/app/controllers/api/v1/transactions_controller.rb +++ b/app/controllers/api/v1/transactions_controller.rb @@ -270,7 +270,7 @@ def apply_filters(query) end def apply_search(query) - search_term = "%#{params[:search]}%" + search_term = "%#{ActiveRecord::Base.sanitize_sql_like(params[:search])}%" query.joins(:entry) .left_joins(:merchant) diff --git a/app/controllers/concerns/accountable_resource.rb b/app/controllers/concerns/accountable_resource.rb index 23fd761073a..22979fa0583 100644 --- a/app/controllers/concerns/accountable_resource.rb +++ b/app/controllers/concerns/accountable_resource.rb @@ -48,7 +48,7 @@ def create @account.lock_saved_attributes! end - redirect_to account_params[:return_to].presence || @account, notice: t("accounts.create.success", type: accountable_type.name.underscore.humanize) + redirect_to safe_return_to_path || @account, notice: t("accounts.create.success", type: accountable_type.name.underscore.humanize) end def update @@ -98,6 +98,31 @@ def set_manageable_account require_account_permission!(@account) end + # Sanitize return_to parameter to prevent XSS/open-redirect attacks. + # Only allow internal relative paths (single leading "/"), and reject any scheme/host. + # Accepts return_to from either the top-level params or nested account_params. + def safe_return_to_path + raw = params[:return_to].presence || params.dig(:account, :return_to).presence + return nil if raw.blank? + + return_to = raw.to_s + + # Reject protocol-relative URLs like "//evil.example.com/path" that browsers + # treat as cross-origin even though they pass a naive start_with?("/") check. + return nil if return_to.start_with?("//") + return nil unless return_to.start_with?("/") + + begin + uri = URI.parse(return_to) + rescue URI::InvalidURIError + return nil + end + + return nil if uri.scheme.present? || uri.host.present? + + return_to + end + def account_params params.require(:account).permit( :name, :balance, :subtype, :currency, :accountable_type, :return_to, diff --git a/app/controllers/concerns/store_location.rb b/app/controllers/concerns/store_location.rb index e2e8d318170..fa06d7d4a6c 100644 --- a/app/controllers/concerns/store_location.rb +++ b/app/controllers/concerns/store_location.rb @@ -24,8 +24,12 @@ def handle_not_found end def store_return_to - if params[:return_to].present? - session[:return_to] = params[:return_to] + return if params[:return_to].blank? + + path = params[:return_to].to_s + # Only allow relative paths to prevent open redirect attacks + if path.start_with?("/") && !path.start_with?("//") + session[:return_to] = path end end diff --git a/app/controllers/import/mappings_controller.rb b/app/controllers/import/mappings_controller.rb index 098c4010102..b61930a6de6 100644 --- a/app/controllers/import/mappings_controller.rb +++ b/app/controllers/import/mappings_controller.rb @@ -33,11 +33,21 @@ def create_when_empty mapping_params[:mappable_id] == mapping_class::CREATE_NEW_KEY end + ALLOWED_MAPPABLE_TYPES = %w[Category Tag Account].freeze + ALLOWED_MAPPING_TYPES = %w[ + Import::CategoryMapping Import::TagMapping + Import::AccountMapping Import::AccountTypeMapping + ].freeze + def mappable_class - mapping_params[:mappable_type]&.constantize + type = mapping_params[:mappable_type] + return nil unless type.present? && ALLOWED_MAPPABLE_TYPES.include?(type) + type.constantize end def mapping_class - mapping_params[:type]&.constantize + type = mapping_params[:type] + return nil unless type.present? && ALLOWED_MAPPING_TYPES.include?(type) + type.constantize end end diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index 3ec1e8dad3d..5948b3191ea 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -660,16 +660,15 @@ def build_transactions_breakdown_for_export transactions = apply_transaction_filters(transactions) sort_by = params[:sort_by] || "date" - # Whitelist sort_direction to prevent SQL injection - sort_direction = %w[asc desc].include?(params[:sort_direction]&.downcase) ? params[:sort_direction].upcase : "DESC" - + # Whitelist sort_direction (hash-based order() below also guards against SQL injection) + direction = %w[asc desc].include?(params[:sort_direction]&.downcase) ? params[:sort_direction].downcase.to_sym : :desc case sort_by when "date" - transactions.order("entries.date #{sort_direction}") + transactions.order("entries.date" => direction) when "amount" - transactions.order("entries.amount #{sort_direction}") + transactions.order("entries.amount" => direction) else - transactions.order("entries.date DESC") + transactions.order("entries.date" => :desc) end end diff --git a/app/controllers/settings/guides_controller.rb b/app/controllers/settings/guides_controller.rb index a21840a9147..f0ee4ef22ff 100644 --- a/app/controllers/settings/guides_controller.rb +++ b/app/controllers/settings/guides_controller.rb @@ -6,7 +6,11 @@ def show [ "Home", root_path ], [ "Guides", nil ] ] - markdown = Redcarpet::Markdown.new(Redcarpet::Render::HTML, + renderer = Redcarpet::Render::HTML.new( + filter_html: true, + link_attributes: { target: "_blank", rel: "noopener noreferrer" } + ) + markdown = Redcarpet::Markdown.new(renderer, autolink: true, tables: true, fenced_code_blocks: true, diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index e8a7f008bb9..e6ba2eaeca7 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -136,6 +136,7 @@ def markdown(text) renderer = Redcarpet::Render::HTML.new( hard_wrap: true, + filter_html: true, link_attributes: { target: "_blank", rel: "noopener noreferrer" } ) @@ -152,7 +153,11 @@ def markdown(text) footnotes: true ) - markdown.render(text).html_safe + sanitize( + markdown.render(text), + tags: %w[p br strong em a img ul ol li h1 h2 h3 h4 h5 h6 pre code blockquote table thead tbody tr th td span div sup del mark ins hr dt dd dl], + attributes: %w[href target rel class id src alt title] + ) end # Generate the callback URL for Enable Banking OAuth (used in views and controller). diff --git a/app/models/account_import.rb b/app/models/account_import.rb index 02f19c05b87..3383071ad94 100644 --- a/app/models/account_import.rb +++ b/app/models/account_import.rb @@ -1,11 +1,17 @@ class AccountImport < Import OpeningBalanceError = Class.new(StandardError) + # Delegate the allow-list to `Accountable::TYPES` so AccountImport and the + # Accountable concern cannot drift. Kept as a public constant because tests + # and other code may reference it. + ALLOWED_ACCOUNTABLE_TYPES = Accountable::TYPES + def import! transaction do rows.each do |row| mapping = mappings.account_types.find_by(key: row.entity_type) - accountable_class = mapping.value.constantize + accountable_class = Accountable.from_type(mapping&.value) + raise ArgumentError, "Invalid accountable type: #{mapping&.value.inspect}" unless accountable_class account = family.accounts.build( name: row.name, diff --git a/app/models/family/data_exporter.rb b/app/models/family/data_exporter.rb index dc0ffae13af..f1baaba8985 100644 --- a/app/models/family/data_exporter.rb +++ b/app/models/family/data_exporter.rb @@ -49,7 +49,7 @@ def generate_accounts_csv @family.accounts.includes(:accountable).find_each do |account| csv << [ account.id, - account.name, + sanitize_csv(account.name), account.accountable_type, account.subtype, account.balance.to_s, @@ -72,12 +72,12 @@ def generate_transactions_csv .find_each do |transaction| csv << [ transaction.entry.date.iso8601, - transaction.entry.account.name, + sanitize_csv(transaction.entry.account.name), transaction.entry.amount.to_s, - transaction.entry.name, - transaction.category&.name, - transaction.tags.pluck(:name).join(","), - transaction.entry.notes, + sanitize_csv(transaction.entry.name), + sanitize_csv(transaction.category&.name), + transaction.tags.pluck(:name).map { |t| sanitize_csv(t) }.join(","), + sanitize_csv(transaction.entry.notes), transaction.entry.currency ] end @@ -94,7 +94,7 @@ def generate_trades_csv .find_each do |trade| csv << [ trade.entry.date.iso8601, - trade.entry.account.name, + sanitize_csv(trade.entry.account.name), trade.security.ticker, trade.qty.to_s, trade.price.to_s, @@ -112,9 +112,9 @@ def generate_categories_csv # Only export categories belonging to this family @family.categories.includes(:parent).find_each do |category| csv << [ - category.name, + sanitize_csv(category.name), category.color, - category.parent&.name, + sanitize_csv(category.parent&.name), category.lucide_icon ] end @@ -128,7 +128,7 @@ def generate_rules_csv # Only export rules belonging to this family @family.rules.includes(conditions: :sub_conditions, actions: []).find_each do |rule| csv << [ - rule.name, + sanitize_csv(rule.name), rule.resource_type, rule.active, rule.effective_date&.iso8601, @@ -350,4 +350,17 @@ def serialize_conditions_for_csv(conditions) def serialize_actions_for_csv(actions) actions.map { |a| serialize_action(a) }.to_json end + + # Prevent CSV formula injection (CWE-1236). + # Values starting with =, +, -, @ can execute as formulas in Excel / Sheets. + # \t, \r, \n are included because some spreadsheet parsers trim leading + # whitespace-like characters before evaluating the cell, so "=1+1" prefixed + # with a tab/newline would still trigger a formula. Leading literal spaces + # are NOT treated as bypasses by mainstream parsers today; if that changes, + # extend the character class to cover them. + # CSV-only — do not apply to JSON/NDJSON output (it would mutate user data). + def sanitize_csv(value) + return value unless value.is_a?(String) + value.match?(/\A[=+\-@\t\r\n]/) ? "'" + value : value + end end diff --git a/app/views/pages/changelog.html.erb b/app/views/pages/changelog.html.erb index d401f0d8770..62067f66483 100644 --- a/app/views/pages/changelog.html.erb +++ b/app/views/pages/changelog.html.erb @@ -21,7 +21,9 @@

<%= @release_notes[:name] %>

- <%= (@release_notes[:body] || "No release notes available").html_safe %> + <%= sanitize(@release_notes[:body] || "No release notes available", + tags: %w[h1 h2 h3 h4 h5 h6 p a ul ol li code pre blockquote strong em br img hr div span table thead tbody tr th td dl dt dd sup sub], + attributes: %w[href src alt class id title dir]) %>
diff --git a/test/models/account_import_test_security.rb b/test/models/account_import_test_security.rb new file mode 100644 index 00000000000..1cc8a0706f7 --- /dev/null +++ b/test/models/account_import_test_security.rb @@ -0,0 +1,20 @@ +require "test_helper" + +class AccountImportSecurityTest < ActiveSupport::TestCase + test "ALLOWED_ACCOUNTABLE_TYPES mirrors Accountable::TYPES (guards against drift)" do + assert_equal Accountable::TYPES.sort, AccountImport::ALLOWED_ACCOUNTABLE_TYPES.sort + end + + test "all ALLOWED_ACCOUNTABLE_TYPES resolve via Accountable.from_type" do + AccountImport::ALLOWED_ACCOUNTABLE_TYPES.each do |type| + assert_not_nil Accountable.from_type(type), "#{type} should resolve to a class" + end + end + + test "Accountable.from_type rejects unknown input" do + assert_nil Accountable.from_type("ActiveRecord::Base") + assert_nil Accountable.from_type("NotAClass") + assert_nil Accountable.from_type(nil) + assert_nil Accountable.from_type("") + end +end diff --git a/test/models/family/data_exporter_test.rb b/test/models/family/data_exporter_test.rb index e25ed0e5d4f..d95d3cb8862 100644 --- a/test/models/family/data_exporter_test.rb +++ b/test/models/family/data_exporter_test.rb @@ -341,4 +341,55 @@ class Family::DataExporterTest < ActiveSupport::TestCase refute ndjson_content.include?(other_rule.name) end end + + # CSV injection prevention (CWE-1236) ---------------------------------------- + + test "sanitize_csv prefixes formula-starting values with single quote" do + dangerous = { "=SUM(A1)" => "'=SUM(A1)", "+cmd" => "'+cmd", "-1+1" => "'-1+1", "@user" => "'@user" } + dangerous.each do |input, expected| + assert_equal expected, @exporter.send(:sanitize_csv, input), "Failed for: #{input}" + end + end + + test "sanitize_csv leaves safe strings unchanged" do + %w[hello Normal 123 category].each do |safe| + assert_equal safe, @exporter.send(:sanitize_csv, safe) + end + end + + test "sanitize_csv passes through non-string values unchanged" do + assert_nil @exporter.send(:sanitize_csv, nil) + assert_equal 42, @exporter.send(:sanitize_csv, 42) + end + + test "NDJSON preserves formula-prefixed names/notes verbatim (no sanitize_csv leak)" do + # Transaction with name/notes starting with CSV formula chars — NDJSON must + # round-trip them unchanged, otherwise export→import silently mutates data. + dangerous_name = "=SUM(A1)" + dangerous_notes = "-1.5x leverage" + entry = @account.entries.create!( + date: Date.current, + name: dangerous_name, + amount: 10, + currency: "USD", + notes: dangerous_notes, + entryable: Transaction.new + ) + + zip_data = @exporter.generate_export + + Zip::File.open_buffer(zip_data) do |zip| + ndjson = zip.read("all.ndjson") + + transaction_line = ndjson.each_line.find do |line| + parsed = JSON.parse(line) + parsed["type"] == "Transaction" && parsed.dig("data", "entry_id") == entry.id + end + + assert transaction_line, "Should find the exported transaction in all.ndjson" + data = JSON.parse(transaction_line)["data"] + assert_equal dangerous_name, data["name"] + assert_equal dangerous_notes, data["notes"] + end + end end