From 41aae013e0768c13a2c533db5840ab9d1a0400be Mon Sep 17 00:00:00 2001 From: David Gil - Klaus Date: Sun, 19 Apr 2026 12:51:23 +0200 Subject: [PATCH 01/11] fix(security): sanitize markdown output to prevent stored XSS (FIX-05, HIGH-04) Redcarpet by default allows raw HTML in markdown input. Combined with .html_safe on the rendered output, any user-supplied markdown (e.g. AI chat responses, notes) could inject arbitrary HTML/JS resulting in stored XSS. - Add filter_html: true to the Redcarpet renderer so raw HTML tags embedded in markdown are escaped (FIX-05). - Replace .html_safe with Rails' sanitize() helper using an allowlist of safe tags and attributes as defense-in-depth (HIGH-04). --- app/helpers/application_helper.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index e8a7f008b..fbfbbe5cb 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 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] + ) end # Generate the callback URL for Enable Banking OAuth (used in views and controller). From 6ed5e47746051f59d71996ea1d2a9760017fcfe4 Mon Sep 17 00:00:00 2001 From: David Gil - Klaus Date: Sun, 19 Apr 2026 12:51:37 +0200 Subject: [PATCH 02/11] fix(security): sanitize GitHub release notes in changelog (FIX-06) The changelog page renders GitHub release notes via .html_safe, which trusts whatever the upstream GitHub API returns. A compromised release author or malicious release body could inject arbitrary HTML/JS, resulting in stored XSS for every user viewing /changelog. Replace .html_safe with Rails' sanitize() helper constrained to an allowlist of tags and attributes appropriate for release notes. --- app/views/pages/changelog.html.erb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/views/pages/changelog.html.erb b/app/views/pages/changelog.html.erb index d401f0d87..4634d301a 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]) %>
From 8668f78fdd3ae1333f0ef8f1d9299418771f474e Mon Sep 17 00:00:00 2001 From: David Gil - Klaus Date: Sun, 19 Apr 2026 12:51:50 +0200 Subject: [PATCH 03/11] fix(security): allowlist class names in import mappings controller (FIX-07) mappable_class and mapping_class called .constantize on user-supplied params (:mappable_type, :type), which permits attackers to instantiate any constant loaded in the Rails app. This is a remote code execution primitive via gadget classes and can trigger autoloading of unintended constants. Restrict both methods to an explicit allowlist of safe class names (Category/Tag/Account and the four Import::*Mapping types) before calling constantize. --- app/controllers/import/mappings_controller.rb | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/app/controllers/import/mappings_controller.rb b/app/controllers/import/mappings_controller.rb index 098c40101..b61930a6d 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 From c84911e42dae3214c517b4cea1122bb764322f6c Mon Sep 17 00:00:00 2001 From: David Gil - Klaus Date: Sun, 19 Apr 2026 12:52:03 +0200 Subject: [PATCH 04/11] fix(security): sanitize return_to to prevent open redirect (FIX-08) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit store_return_to copied the params[:return_to] value straight into the session, where later redirect_back_or_to would follow it — including fully-qualified attacker-controlled URLs or protocol-relative //evil paths. This enabled phishing via legitimate-looking links. Only persist values that start with a single "/" (internal relative paths) and reject anything else. --- app/controllers/concerns/store_location.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/controllers/concerns/store_location.rb b/app/controllers/concerns/store_location.rb index e2e8d3181..fa06d7d4a 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 From 129c86f290bc67b761a50aa0577bc576faad1b8f Mon Sep 17 00:00:00 2001 From: David Gil - Klaus Date: Sun, 19 Apr 2026 12:52:23 +0200 Subject: [PATCH 05/11] fix(security): allowlist accountable types in AccountImport (F-07) AccountImport#import! called .constantize on the user-supplied mapping value from a CSV. Because CSV import mappings are user-editable this allowed attackers with a valid family session to instantiate arbitrary constants from the Rails environment. Restrict to the nine legitimate Accountable subclasses and raise ArgumentError otherwise. --- app/models/account_import.rb | 12 +++++++++++- test/models/account_import_test_security.rb | 18 ++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 test/models/account_import_test_security.rb diff --git a/app/models/account_import.rb b/app/models/account_import.rb index 02f19c05b..7ce4bafe2 100644 --- a/app/models/account_import.rb +++ b/app/models/account_import.rb @@ -1,11 +1,21 @@ class AccountImport < Import OpeningBalanceError = Class.new(StandardError) + ALLOWED_ACCOUNTABLE_TYPES = %w[ + Depository Investment Crypto + Property Vehicle OtherAsset + CreditCard Loan OtherLiability + ].freeze + def import! transaction do rows.each do |row| mapping = mappings.account_types.find_by(key: row.entity_type) - accountable_class = mapping.value.constantize + type = mapping&.value + unless type.present? && ALLOWED_ACCOUNTABLE_TYPES.include?(type) + raise ArgumentError, "Invalid accountable type: #{type.inspect}" + end + accountable_class = type.constantize account = family.accounts.build( name: row.name, diff --git a/test/models/account_import_test_security.rb b/test/models/account_import_test_security.rb new file mode 100644 index 000000000..62056a449 --- /dev/null +++ b/test/models/account_import_test_security.rb @@ -0,0 +1,18 @@ +require "test_helper" + +class AccountImportSecurityTest < ActiveSupport::TestCase + test "ALLOWED_ACCOUNTABLE_TYPES covers all expected types" do + expected = %w[ + Depository Investment Crypto + Property Vehicle OtherAsset + CreditCard Loan OtherLiability + ] + assert_equal expected.sort, AccountImport::ALLOWED_ACCOUNTABLE_TYPES.sort + end + + test "all ALLOWED_ACCOUNTABLE_TYPES are real constants" do + AccountImport::ALLOWED_ACCOUNTABLE_TYPES.each do |type| + assert_nothing_raised { type.constantize } + end + end +end From 9e4639324732d7070671f78e453b2dfeb5e7698b Mon Sep 17 00:00:00 2001 From: David Gil - Klaus Date: Sun, 19 Apr 2026 12:53:23 +0200 Subject: [PATCH 06/11] fix(security): escape CSV formula injection in family data export (F-09) CSV/NDJSON exports echoed user-supplied strings (account names, tag names, notes, rule names, etc.) verbatim. A value starting with =, +, -, @, or certain control characters is interpreted as a formula by Excel/Google Sheets and could execute commands, exfiltrate data, or phish the recipient on open (CWE-1236). Add a sanitize_csv helper that prefixes any such value with a single quote, and apply it to every user-controlled string across the CSV and NDJSON export paths. --- app/models/family/data_exporter.rb | 35 ++++++++++++++---------- test/models/family/data_exporter_test.rb | 20 ++++++++++++++ 2 files changed, 41 insertions(+), 14 deletions(-) diff --git a/app/models/family/data_exporter.rb b/app/models/family/data_exporter.rb index dc0ffae13..bdf5e1dab 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, @@ -189,8 +189,8 @@ def generate_ndjson date: transaction.entry.date, amount: transaction.entry.amount, currency: transaction.entry.currency, - name: transaction.entry.name, - notes: transaction.entry.notes, + name: sanitize_csv(transaction.entry.name), + notes: sanitize_csv(transaction.entry.notes), excluded: transaction.entry.excluded, category_id: transaction.category_id, merchant_id: transaction.merchant_id, @@ -234,7 +234,7 @@ def generate_ndjson date: entry.date, amount: entry.amount, currency: entry.currency, - name: entry.name, + name: sanitize_csv(entry.name), created_at: entry.created_at, updated_at: entry.updated_at } @@ -271,7 +271,7 @@ def generate_ndjson def serialize_rule_for_export(rule) { - name: rule.name, + name: sanitize_csv(rule.name), resource_type: rule.resource_type, active: rule.active, effective_date: rule.effective_date&.iso8601, @@ -350,4 +350,11 @@ 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 + def sanitize_csv(value) + return value unless value.is_a?(String) + value.match?(/\A[=+\-@\t\r\n]/) ? "'" + value : value + end end diff --git a/test/models/family/data_exporter_test.rb b/test/models/family/data_exporter_test.rb index e25ed0e5d..b4dc2565e 100644 --- a/test/models/family/data_exporter_test.rb +++ b/test/models/family/data_exporter_test.rb @@ -341,4 +341,24 @@ 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 end From 340458b887f073248fbcbfc38c5c4a3daeaa8444 Mon Sep 17 00:00:00 2001 From: David Gil - Klaus Date: Sun, 19 Apr 2026 12:53:43 +0200 Subject: [PATCH 07/11] fix(security): sanitize account return_to redirect (F-10) AccountableResource#create redirected to account_params[:return_to] without validation, allowing attackers to craft account-creation links that bounce through our domain to a phishing site. Introduce safe_return_to_path which rejects absolute URLs, any value with a scheme, and protocol-relative paths, falling back to the newly created account on invalid input. --- .../concerns/accountable_resource.rb | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/app/controllers/concerns/accountable_resource.rb b/app/controllers/concerns/accountable_resource.rb index 23fd76107..fefada9e7 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,23 @@ def set_manageable_account require_account_permission!(@account) end + # Sanitize return_to parameter to prevent XSS/open-redirect attacks. + # Only allow internal relative paths (starting with "/"), and reject any scheme/host. + def safe_return_to_path + return nil if params[:return_to].blank? + + return_to = params[:return_to].to_s + + begin + uri = URI.parse(return_to) + return nil if uri.scheme.present? + return nil unless return_to.start_with?("/") + return_to + rescue URI::InvalidURIError + nil + end + end + def account_params params.require(:account).permit( :name, :balance, :subtype, :currency, :accountable_type, :return_to, From 86475aa22c020aacd0a5b4c82aeab61e6cad6e2b Mon Sep 17 00:00:00 2001 From: David Gil - Klaus Date: Sun, 19 Apr 2026 12:53:57 +0200 Subject: [PATCH 08/11] fix(security): eliminate SQL interpolation in reports ORDER BY (CRIT-02) build_sorted_transactions concatenated the sort_direction string into an ORDER BY fragment. While an allowlist check gates the value, any future refactor could regress into SQL injection. Replace the raw SQL string with Rails' hash-based order syntax, which delegates quoting to the adapter and removes the injection primitive entirely. --- app/controllers/reports_controller.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index 3ec1e8dad..cebe78ad4 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -663,13 +663,14 @@ def build_transactions_breakdown_for_export # Whitelist sort_direction to prevent SQL injection sort_direction = %w[asc desc].include?(params[:sort_direction]&.downcase) ? params[:sort_direction].upcase : "DESC" + direction = sort_direction.downcase.to_sym 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 From a08552451aa2ec956f8c98c77d61a885c0bfd186 Mon Sep 17 00:00:00 2001 From: David Gil - Klaus Date: Sun, 19 Apr 2026 12:54:08 +0200 Subject: [PATCH 09/11] fix(security): escape LIKE wildcards in API transactions search (HIGH-03) Api::V1::TransactionsController#apply_search interpolated the raw search parameter into an ILIKE pattern. Values containing % or _ bypassed the intended literal substring match and let a client enumerate or DoS the index with arbitrary pattern expansion. Pipe the search term through ActiveRecord::Base.sanitize_sql_like so % and _ are escaped before being wrapped in the wildcard template. --- app/controllers/api/v1/transactions_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/api/v1/transactions_controller.rb b/app/controllers/api/v1/transactions_controller.rb index b5942b7fc..1e59b5283 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) From b2b7b512cf0a48ce7807918a3a45ddbc20bb2a40 Mon Sep 17 00:00:00 2001 From: David Gil - Klaus Date: Sun, 19 Apr 2026 12:54:19 +0200 Subject: [PATCH 10/11] fix(security): filter raw HTML in guides markdown renderer (HIGH-05) Settings::GuidesController rendered docs/onboarding/guide.md with a default Redcarpet HTML renderer. Anyone with write access to that path (including a future migration that makes guides user-editable) could inject arbitrary HTML/JS and trigger XSS on the settings page. Instantiate the renderer with filter_html: true and standard safe link attributes as defense-in-depth. --- app/controllers/settings/guides_controller.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/controllers/settings/guides_controller.rb b/app/controllers/settings/guides_controller.rb index a21840a91..f0ee4ef22 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, From 268abe96e7b3ebbc399dc31b92eaa65e2d5eb49e Mon Sep 17 00:00:00 2001 From: David Gil - Klaus Date: Sun, 19 Apr 2026 16:41:47 +0200 Subject: [PATCH 11/11] fix(security): address CodeRabbit review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Major: - data_exporter: remove sanitize_csv from NDJSON paths. NDJSON is JSON — leading =+-@ have no formula meaning there, and prepending "'" silently mutates user-entered transaction names/notes, valuation names, and rule names on every export→import round-trip. - safe_return_to_path: reject protocol-relative URLs ("//evil.example.com"), also read return_to from account_params, and explicitly reject any URI with host or scheme. Refactors / refinements: - account_import: delegate ALLOWED_ACCOUNTABLE_TYPES to Accountable::TYPES and use Accountable.from_type instead of a duplicated constantize allowlist (prevents drift when new accountable subtypes are added). - application_helper: add `img` tag and `src`/`alt`/`title` attrs to the markdown sanitize allowlist so images render (URL scheme filtering is already handled by Rails sanitizer). - changelog view: add `dir` to sanitize attributes so GitHub's dir="auto" (bidi) is preserved on release notes. - reports_controller: collapse the two-step sort_direction normalization (uppercase then downcase-to-sym) into a single symbol. - data_exporter#sanitize_csv: comment why \t\r\n are in the class and note the CSV-only intent. Tests: - NDJSON round-trip test asserting formula-prefixed names/notes are preserved verbatim (locks in the main fix). - account_import test now asserts parity with Accountable::TYPES instead of duplicating the list. --- .../concerns/accountable_resource.rb | 22 ++++++++----- app/controllers/reports_controller.rb | 6 ++-- app/helpers/application_helper.rb | 4 +-- app/models/account_import.rb | 16 ++++------ app/models/family/data_exporter.rb | 18 +++++++---- app/views/pages/changelog.html.erb | 2 +- test/models/account_import_test_security.rb | 20 ++++++------ test/models/family/data_exporter_test.rb | 31 +++++++++++++++++++ 8 files changed, 80 insertions(+), 39 deletions(-) diff --git a/app/controllers/concerns/accountable_resource.rb b/app/controllers/concerns/accountable_resource.rb index fefada9e7..22979fa05 100644 --- a/app/controllers/concerns/accountable_resource.rb +++ b/app/controllers/concerns/accountable_resource.rb @@ -99,20 +99,28 @@ def set_manageable_account end # Sanitize return_to parameter to prevent XSS/open-redirect attacks. - # Only allow internal relative paths (starting with "/"), and reject any scheme/host. + # 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 - return nil if params[:return_to].blank? + raw = params[:return_to].presence || params.dig(:account, :return_to).presence + return nil if raw.blank? - return_to = params[:return_to].to_s + 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) - return nil if uri.scheme.present? - return nil unless return_to.start_with?("/") - return_to rescue URI::InvalidURIError - nil + return nil end + + return nil if uri.scheme.present? || uri.host.present? + + return_to end def account_params diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index cebe78ad4..5948b3191 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -660,10 +660,8 @@ 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" - - direction = sort_direction.downcase.to_sym + # 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" => direction) diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index fbfbbe5cb..e6ba2eaec 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -155,8 +155,8 @@ def markdown(text) sanitize( markdown.render(text), - tags: %w[p br strong em a 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] + 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 diff --git a/app/models/account_import.rb b/app/models/account_import.rb index 7ce4bafe2..3383071ad 100644 --- a/app/models/account_import.rb +++ b/app/models/account_import.rb @@ -1,21 +1,17 @@ class AccountImport < Import OpeningBalanceError = Class.new(StandardError) - ALLOWED_ACCOUNTABLE_TYPES = %w[ - Depository Investment Crypto - Property Vehicle OtherAsset - CreditCard Loan OtherLiability - ].freeze + # 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) - type = mapping&.value - unless type.present? && ALLOWED_ACCOUNTABLE_TYPES.include?(type) - raise ArgumentError, "Invalid accountable type: #{type.inspect}" - end - accountable_class = type.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 bdf5e1dab..f1baaba89 100644 --- a/app/models/family/data_exporter.rb +++ b/app/models/family/data_exporter.rb @@ -189,8 +189,8 @@ def generate_ndjson date: transaction.entry.date, amount: transaction.entry.amount, currency: transaction.entry.currency, - name: sanitize_csv(transaction.entry.name), - notes: sanitize_csv(transaction.entry.notes), + name: transaction.entry.name, + notes: transaction.entry.notes, excluded: transaction.entry.excluded, category_id: transaction.category_id, merchant_id: transaction.merchant_id, @@ -234,7 +234,7 @@ def generate_ndjson date: entry.date, amount: entry.amount, currency: entry.currency, - name: sanitize_csv(entry.name), + name: entry.name, created_at: entry.created_at, updated_at: entry.updated_at } @@ -271,7 +271,7 @@ def generate_ndjson def serialize_rule_for_export(rule) { - name: sanitize_csv(rule.name), + name: rule.name, resource_type: rule.resource_type, active: rule.active, effective_date: rule.effective_date&.iso8601, @@ -351,8 +351,14 @@ 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 + # 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 diff --git a/app/views/pages/changelog.html.erb b/app/views/pages/changelog.html.erb index 4634d301a..62067f664 100644 --- a/app/views/pages/changelog.html.erb +++ b/app/views/pages/changelog.html.erb @@ -23,7 +23,7 @@

<%= @release_notes[:name] %>

<%= 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]) %> + 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 index 62056a449..1cc8a0706 100644 --- a/test/models/account_import_test_security.rb +++ b/test/models/account_import_test_security.rb @@ -1,18 +1,20 @@ require "test_helper" class AccountImportSecurityTest < ActiveSupport::TestCase - test "ALLOWED_ACCOUNTABLE_TYPES covers all expected types" do - expected = %w[ - Depository Investment Crypto - Property Vehicle OtherAsset - CreditCard Loan OtherLiability - ] - assert_equal expected.sort, AccountImport::ALLOWED_ACCOUNTABLE_TYPES.sort + 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 are real constants" do + test "all ALLOWED_ACCOUNTABLE_TYPES resolve via Accountable.from_type" do AccountImport::ALLOWED_ACCOUNTABLE_TYPES.each do |type| - assert_nothing_raised { type.constantize } + 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 b4dc2565e..d95d3cb88 100644 --- a/test/models/family/data_exporter_test.rb +++ b/test/models/family/data_exporter_test.rb @@ -361,4 +361,35 @@ class Family::DataExporterTest < ActiveSupport::TestCase 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