security: input validation & injection fixes (XSS, constantize, open redirects, CSV, SQL)#1517
security: input validation & injection fixes (XSS, constantize, open redirects, CSV, SQL)#1517dgilperez wants to merge 11 commits intowe-promise:mainfrom
Conversation
…, 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).
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.
…IX-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.
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.
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.
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.
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.
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.
…-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.
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.
brin-security-scanner
Bot
added
contributor:verified
📝 WalkthroughWalkthroughSecurity hardening across controllers, models, helpers, views, and tests: input escaping for SQL LIKE, open-redirect/path validation, dynamic-constant allowlisting, CSV formula sanitization, HTML/Markdown sanitization, safer SQL ordering, and accompanying unit tests for these behaviors. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2b7b512cf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/models/family/data_exporter.rb (1)
192-274:⚠️ Potential issue | 🟠 MajorDo not apply CSV formula sanitization to NDJSON fields — it corrupts exported data.
sanitize_csvexists to neutralize spreadsheet formula interpretation when a value is written into a CSV cell. NDJSON is consumed as JSON, where a leading=,+,-, or@has no special meaning, so prepending'here is not a security win — it permanently mutates user-entered values (transactionname/notes, valuationname, rulename) insideall.ndjson.Concrete impact:
- A transaction named
-1.5x leverageis exported as'-1.5x leveragein NDJSON.- Any round-trip (export → re-import via NDJSON) now silently drifts names/notes with every cycle, and the
'becomes part of the stored value.serialize_rule_for_exportis also reused fromgenerate_ndjson(line 265), so rule names suffer the same corruption.Keep
sanitize_csvonly in the CSV generators (which is where formula injection actually applies) and pass raw strings through for NDJSON.🛠️ Suggested fix
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,currency: entry.currency, - name: sanitize_csv(entry.name), + name: entry.name, created_at: entry.created_at,def serialize_rule_for_export(rule) { - name: sanitize_csv(rule.name), + name: rule.name, resource_type: rule.resource_type,Note:
serialize_rule_for_exportis only called fromgenerate_ndjson, so CSV rule-name sanitization ingenerate_rules_csv(line 131) is unaffected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/family/data_exporter.rb` around lines 192 - 274, The NDJSON exporter is incorrectly applying CSV formula sanitization (sanitize_csv) to JSON output which mutates user data; remove sanitize_csv calls from the NDJSON generation paths — specifically stop calling sanitize_csv on transaction.entry.name and transaction.entry.notes in the "Transaction" block, on entry.name in the "Valuation" block, and from serialize_rule_for_export (used for "Rule" exports); leave sanitize_csv in CSV-generating methods (e.g., generate_rules_csv) untouched so CSV exports still neutralize formulas.
🧹 Nitpick comments (5)
test/models/family/data_exporter_test.rb (1)
347-363: LGTM — focused coverage forsanitize_csv.Good use of
sendto exercise the private helper directly with tight, readable cases (formula prefixes, safe strings, non-string pass-through). Fixture usage and Minitest conventions align with the project's testing guidelines.One nit: once the NDJSON misuse flagged in the production file is fixed, consider adding an assertion that NDJSON
name/notesfor a transaction whose name starts with=/-is preserved verbatim inall.ndjson— that locks in the intended separation between CSV and NDJSON handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/family/data_exporter_test.rb` around lines 347 - 363, Add a test that verifies NDJSON output preserves verbatim values for fields that would be CSV-sanitized: after fixing the NDJSON misuse in production, create or extend a test that writes a transaction with name and notes starting with "=", "+" or "-" and then asserts that the corresponding entries in the generated all.ndjson contain the original name and notes strings untouched; reference the sanitize_csv helper only to distinguish CSV behavior from NDJSON behavior and assert on the NDJSON file's "name" and "notes" properties to lock in the intended separation between CSV and NDJSON handling.app/models/family/data_exporter.rb (1)
354-359: Minor: regex character class is correct, but consider documenting intent of\t\r\n.The class
[=+\-@\t\r\n]correctly escapes-and matches OWASP's recommended leading characters. One subtle gap worth noting: some guidance also treats a literal leading space before a formula character (e.g.," =1+1") as evasion. Not a blocker — current behavior matches the typical OWASP shortlist — but worth a brief comment on why tab/CR/LF are included (they prevent leading-whitespace bypasses in some parsers) so a future maintainer doesn't trim them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/family/data_exporter.rb` around lines 354 - 359, The regex in sanitize_csv currently matches leading formula characters including tab/CR/LF but lacks an explanatory comment; update the sanitize_csv method to add a short inline note explaining that the character class [=+\-@\t\r\n] intentionally includes \t, \r, and \n to catch formula injection attempts that use leading control whitespace as evasion, and mention that a literal leading space before a formula (e.g., " =1+1") was deliberately not included (or document if you choose to trim spaces) so future maintainers understand the tradeoff.app/controllers/reports_controller.rb (1)
664-674: Redundant direction normalization.
sort_directionis whitelisted and uppercased on line 664, then immediately downcased and symbolized on line 666. Since the result flows into hash-basedorder(...)which is already safe from SQL injection, you can simplify to a single step:♻️ Proposed simplification
- 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 + sort_by = params[:sort_by] || "date" + # Whitelist sort_direction (hash-based order() 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) when "amount" transactions.order("entries.amount" => direction) else transactions.order("entries.date" => :desc) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/reports_controller.rb` around lines 664 - 674, The code redundantly uppercases then immediately downcases/symbolizes the sort direction; simplify by producing a single normalized symbol used in the orders. Replace the two-step normalization (sort_direction then direction) with one expression that validates params[:sort_direction] against %w[asc desc] and yields a symbol (:asc or :desc) to feed into transactions.order in the case branches (referencing sort_by and transactions.order("entries.date"/"entries.amount")). Remove the intermediate uppercase step and the extra variable to keep the input validation and SQL-safety while reducing redundancy.app/models/account_import.rb (1)
4-18: ReuseAccountable::TYPES/Accountable.from_typeinstead of duplicating the allowlist.
app/models/concerns/accountable.rbalready defines the canonical allowlist and safe resolver:TYPES = %w[Depository Investment Crypto Property Vehicle OtherAsset CreditCard Loan OtherLiability].freeze def self.from_type(type) return nil unless TYPES.include?(type) type.constantize end
ALLOWED_ACCOUNTABLE_TYPEShere is an exact copy. When a new accountable subtype is eventually added toAccountable::TYPES, imports will silently fail withInvalid accountable type: …until someone remembers this second list. Delegating toAccountablekeeps the security guarantee while eliminating the drift risk.♻️ Suggested refactor
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) 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(type) + raise ArgumentError, "Invalid accountable type: #{type.inspect}" unless accountable_classIf you prefer to keep
AccountImport::ALLOWED_ACCOUNTABLE_TYPESas a public API (the security test references it), define it asALLOWED_ACCOUNTABLE_TYPES = Accountable::TYPESso it stays in lockstep.As per coding guidelines ("Place business logic in
app/models/folder … use Rails concerns and POROs for organization"), routing through theAccountableconcern keeps the type-resolution logic in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/account_import.rb` around lines 4 - 18, Replace the duplicated allowlist and unsafe constantize with the canonical resolver in Accountable: remove or redefine AccountImport::ALLOWED_ACCOUNTABLE_TYPES to use Accountable::TYPES (e.g. ALLOWED_ACCOUNTABLE_TYPES = Accountable::TYPES) and in import! stop doing type = mapping&.value; accountable_class = type.constantize; instead call Accountable.from_type(mapping&.value) (or assign to a variable like accountable_class = Accountable.from_type(mapping&.value)) and raise the same ArgumentError if that returns nil so invalid types are still rejected; this keeps the allowlist and resolution centralized in Accountable.test/models/account_import_test_security.rb (1)
4-11: Test duplicates the constant instead of guarding against drift.Asserting
ALLOWED_ACCOUNTABLE_TYPESequals a hardcoded copy of the same list is tautological — any legitimate edit to the constant requires mirroring the change here, and the test still won't catch the real risk:AccountImport::ALLOWED_ACCOUNTABLE_TYPESsilently drifting fromAccountable::TYPES(the canonical accountable subtypes list inapp/models/concerns/accountable.rb). If a new accountable subtype is added toAccountable::TYPES, imports for that type will start raisingArgumentError, "Invalid accountable type: …"until someone remembers to update this second list.Prefer asserting parity with the canonical source:
♻️ Suggested change
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 + # Guard against drift from the canonical Accountable::TYPES list. + assert_equal Accountable::TYPES.sort, + AccountImport::ALLOWED_ACCOUNTABLE_TYPES.sort endThis becomes unnecessary entirely if you consolidate on
Accountable::TYPESinapp/models/account_import.rb(see the related comment on that file).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/account_import_test_security.rb` around lines 4 - 11, The test currently duplicates AccountImport::ALLOWED_ACCOUNTABLE_TYPES; change it to assert parity with the canonical source by comparing AccountImport::ALLOWED_ACCOUNTABLE_TYPES to Accountable::TYPES (e.g. sort and assert equality or set equality) so the test fails if the constant drifts from the canonical list; locate references to AccountImport::ALLOWED_ACCOUNTABLE_TYPES and replace the hardcoded expected array with a comparison to Accountable::TYPES in the test "ALLOWED_ACCOUNTABLE_TYPES covers all expected types".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/concerns/accountable_resource.rb`:
- Around line 103-112: safe_return_to_path currently allows protocol-relative
open redirects and only checks params[:return_to]; update it to read the
return_to value from both params[:return_to] and account_params[:return_to] (use
account_params if present), then reject any value that starts with '//'
(protocol-relative) or where URI.parse(return_to).host.present? or
uri.scheme.present?; only return the path when it begins with '/' (single slash)
and passes those host/scheme/leading-slash checks. Reference:
safe_return_to_path and account_params.
In `@app/helpers/application_helper.rb`:
- Around line 156-160: The sanitize allowlist currently omits img so markdown
images are stripped; update the sanitize call in application_helper.rb to
include "img" in the tags list and add safe image attributes (e.g. include src,
alt, title in the attributes array) so images render but only with constrained
attributes (leaving URL protocol filtering to Rails sanitizer), e.g. add "img"
to tags and add "src", "alt", "title" to attributes; also remove the
now-redundant .html_safe call in the provider form view where markdown output is
rendered.
In `@app/views/pages/changelog.html.erb`:
- Around line 24-26: The sanitize call rendering `@release_notes`[:body] in
app/views/pages/changelog.html.erb currently allows only attributes %w[href src
alt class id title], which strips GitHub's dir="auto" on block elements; update
the attributes list used in the sanitize(...) invocation (the attributes param
passed to sanitize) to include "dir" so bidi auto-detection (dir="auto") from
GitHub-rendered HTML is preserved while keeping other allowed attributes
unchanged.
---
Outside diff comments:
In `@app/models/family/data_exporter.rb`:
- Around line 192-274: The NDJSON exporter is incorrectly applying CSV formula
sanitization (sanitize_csv) to JSON output which mutates user data; remove
sanitize_csv calls from the NDJSON generation paths — specifically stop calling
sanitize_csv on transaction.entry.name and transaction.entry.notes in the
"Transaction" block, on entry.name in the "Valuation" block, and from
serialize_rule_for_export (used for "Rule" exports); leave sanitize_csv in
CSV-generating methods (e.g., generate_rules_csv) untouched so CSV exports still
neutralize formulas.
---
Nitpick comments:
In `@app/controllers/reports_controller.rb`:
- Around line 664-674: The code redundantly uppercases then immediately
downcases/symbolizes the sort direction; simplify by producing a single
normalized symbol used in the orders. Replace the two-step normalization
(sort_direction then direction) with one expression that validates
params[:sort_direction] against %w[asc desc] and yields a symbol (:asc or :desc)
to feed into transactions.order in the case branches (referencing sort_by and
transactions.order("entries.date"/"entries.amount")). Remove the intermediate
uppercase step and the extra variable to keep the input validation and
SQL-safety while reducing redundancy.
In `@app/models/account_import.rb`:
- Around line 4-18: Replace the duplicated allowlist and unsafe constantize with
the canonical resolver in Accountable: remove or redefine
AccountImport::ALLOWED_ACCOUNTABLE_TYPES to use Accountable::TYPES (e.g.
ALLOWED_ACCOUNTABLE_TYPES = Accountable::TYPES) and in import! stop doing type =
mapping&.value; accountable_class = type.constantize; instead call
Accountable.from_type(mapping&.value) (or assign to a variable like
accountable_class = Accountable.from_type(mapping&.value)) and raise the same
ArgumentError if that returns nil so invalid types are still rejected; this
keeps the allowlist and resolution centralized in Accountable.
In `@app/models/family/data_exporter.rb`:
- Around line 354-359: The regex in sanitize_csv currently matches leading
formula characters including tab/CR/LF but lacks an explanatory comment; update
the sanitize_csv method to add a short inline note explaining that the character
class [=+\-@\t\r\n] intentionally includes \t, \r, and \n to catch formula
injection attempts that use leading control whitespace as evasion, and mention
that a literal leading space before a formula (e.g., " =1+1") was deliberately
not included (or document if you choose to trim spaces) so future maintainers
understand the tradeoff.
In `@test/models/account_import_test_security.rb`:
- Around line 4-11: The test currently duplicates
AccountImport::ALLOWED_ACCOUNTABLE_TYPES; change it to assert parity with the
canonical source by comparing AccountImport::ALLOWED_ACCOUNTABLE_TYPES to
Accountable::TYPES (e.g. sort and assert equality or set equality) so the test
fails if the constant drifts from the canonical list; locate references to
AccountImport::ALLOWED_ACCOUNTABLE_TYPES and replace the hardcoded expected
array with a comparison to Accountable::TYPES in the test
"ALLOWED_ACCOUNTABLE_TYPES covers all expected types".
In `@test/models/family/data_exporter_test.rb`:
- Around line 347-363: Add a test that verifies NDJSON output preserves verbatim
values for fields that would be CSV-sanitized: after fixing the NDJSON misuse in
production, create or extend a test that writes a transaction with name and
notes starting with "=", "+" or "-" and then asserts that the corresponding
entries in the generated all.ndjson contain the original name and notes strings
untouched; reference the sanitize_csv helper only to distinguish CSV behavior
from NDJSON behavior and assert on the NDJSON file's "name" and "notes"
properties to lock in the intended separation between CSV and NDJSON handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a88a1a7c-cba9-4b2c-ab55-0be0feb9594d
📒 Files selected for processing (12)
app/controllers/api/v1/transactions_controller.rbapp/controllers/concerns/accountable_resource.rbapp/controllers/concerns/store_location.rbapp/controllers/import/mappings_controller.rbapp/controllers/reports_controller.rbapp/controllers/settings/guides_controller.rbapp/helpers/application_helper.rbapp/models/account_import.rbapp/models/family/data_exporter.rbapp/views/pages/changelog.html.erbtest/models/account_import_test_security.rbtest/models/family/data_exporter_test.rb
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.
|
Thanks — addressed all eight points. Major —
markdown img tag stripped ( changelog
Test duplicates the list ( All changes in commit — wolfgang 🤖 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/controllers/concerns/accountable_resource.rb (1)
104-124: Consider extracting to a shared concern to eliminate duplication.Near-identical
return_tovalidators exist inapp/controllers/sophtron_items_controller.rb(sanitize_return_to, lines 733–751) andapp/controllers/mercury_items_controller.rb(lines 756–774), andapp/controllers/concerns/store_location.rb#store_return_toimplements a weaker variant that lacks the scheme/host checks enforced here. Extractingsafe_return_to_pathinto a shared concern (e.g.,SafeRedirect) and reusing it in those controllers — includingStoreLocation— would remove duplication and close the inconsistency wheresession[:return_to]can be populated with values this helper would reject. Out of scope for this PR if preferred, but worth a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/concerns/accountable_resource.rb` around lines 104 - 124, Extract the duplicate return-to validation logic into a shared concern (suggested name SafeRedirect) and replace the per-controller methods with calls to it: move the implementation in safe_return_to_path into the concern and expose a reusable method (e.g., safe_return_to_path or sanitize_return_to_path); update app/controllers/concerns/store_location.rb to use this new method instead of its weaker store_return_to, and replace sanitize_return_to in sophtron_items_controller.rb and mercury_items_controller.rb to delegate to the concern; ensure the concern preserves the same checks (reject protocol-relative URLs, require leading "/", parse with URI and disallow scheme/host) so session[:return_to] and all controllers use the consistent validator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/controllers/concerns/accountable_resource.rb`:
- Around line 104-124: Extract the duplicate return-to validation logic into a
shared concern (suggested name SafeRedirect) and replace the per-controller
methods with calls to it: move the implementation in safe_return_to_path into
the concern and expose a reusable method (e.g., safe_return_to_path or
sanitize_return_to_path); update app/controllers/concerns/store_location.rb to
use this new method instead of its weaker store_return_to, and replace
sanitize_return_to in sophtron_items_controller.rb and
mercury_items_controller.rb to delegate to the concern; ensure the concern
preserves the same checks (reject protocol-relative URLs, require leading "/",
parse with URI and disallow scheme/host) so session[:return_to] and all
controllers use the consistent validator.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17d65a22-426b-4bf9-98e9-8a70f5258d03
📒 Files selected for processing (8)
app/controllers/concerns/accountable_resource.rbapp/controllers/reports_controller.rbapp/helpers/application_helper.rbapp/models/account_import.rbapp/models/family/data_exporter.rbapp/views/pages/changelog.html.erbtest/models/account_import_test_security.rbtest/models/family/data_exporter_test.rb
🚧 Files skipped from review as they are similar to previous changes (6)
- app/views/pages/changelog.html.erb
- test/models/family/data_exporter_test.rb
- app/controllers/reports_controller.rb
- app/models/family/data_exporter.rb
- app/models/account_import.rb
- test/models/account_import_test_security.rb
github-project-automation
Bot
moved this to Todo
in Stable release train (v0.7.0) - April 2026
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
github-project-automation
Bot
moved this to Todo
in Latest release train (v0.7.1) - May 2026
3 participants
Summary
Addresses all input-validation / output-escaping findings from the 2026-03 pentest: XSS in markdown renderers, unsafe
constantizeon user input, open-redirect vectors, CSV formula injection, and SQL interpolation in reports.Part of the security hardening originally bundled in #1104 (closed), now split by functional area per @jjmata's feedback. Companion PRs: #1516 #1518 #1519 #1520 #1521.
Findings addressed (11)
filter_html: true+sanitize()allowlist inapplication_helper.rbhtml_safeinpages/changelog.html.erbconstantizeinimport/mappings_controller.rb— allowliststore_return_to— path sanitization inconcerns/store_location.rbconstantizeinaccount_import.rb—ALLOWED_ACCOUNTABLE_TYPESallowlistfamily/data_exporter.rb—sanitize_csvescapes leading=+-@\t\r\nconcerns/accountable_resource.rb—safe_return_to_pathhelpersanitize_sql_likefilter_html: trueTests
test/models/account_import_test_security.rb(ALLOWED_ACCOUNTABLE_TYPES),test/models/family/data_exporter_test.rb(sanitize_csv cases).bin/rubocopclean,bin/brakeman0 warnings.Out of scope
Related
Summary by CodeRabbit
Bug Fixes
Tests