diff --git a/erpnext/accounts/doctype/financial_report_template/financial_report_engine.py b/erpnext/accounts/doctype/financial_report_template/financial_report_engine.py index 1137b79a964..e182a0db48e 100644 --- a/erpnext/accounts/doctype/financial_report_template/financial_report_engine.py +++ b/erpnext/accounts/doctype/financial_report_template/financial_report_engine.py @@ -479,7 +479,10 @@ class DataCollector: if company: query = query.where(account.company == company) - if conditions := filter_parser.build_conditions(account_rows, account): + # filters are optional: no filter means all (enabled, non-group) accounts of the company. + # invalid filters can't reach here — build_conditions raises on them (raise_on_invalid). + conditions = filter_parser.build_conditions(account_rows, account, raise_on_invalid=True) + if conditions is not None: query = query.where(conditions) return query.run(pluck=True) @@ -791,17 +794,20 @@ class FilterExpressionParser: def __init__(self): self.validator = AccountFilterValidator() - def build_conditions(self, report_rows, table): + def build_conditions(self, report_rows, table, raise_on_invalid=False): conditions = [] for row in report_rows or []: - condition = self.build_condition(row, table) + condition = self.build_condition(row, table, raise_on_invalid=raise_on_invalid) if condition is not None: conditions.append(condition) + if not conditions: + return None + # ensure brackets in or condition return reduce(lambda a, b: (a) | (b), conditions) - def build_condition(self, report_row, table): + def build_condition(self, report_row, table, raise_on_invalid=False): """ Build SQL condition directly from filter formula. @@ -831,9 +837,11 @@ class FilterExpressionParser: if not filter_formula: return None - errors = self.validator.validate(report_row) + errors = self.validator.validate_filter(report_row) if not errors.is_valid: error_messages = [str(issue) for issue in errors.issues] + if raise_on_invalid: + frappe.throw("

".join(error_messages), title=_("Invalid Filter")) frappe.log_error(f"Filter validation errors found:\n{'

'.join(error_messages)}") return None @@ -1023,7 +1031,11 @@ class FormulaFieldUpdater: @frappe.whitelist() def get_filtered_accounts(company: str, account_rows: str | list): + if not company: + frappe.throw(_("Company is required"), title=_("Missing Company")) + frappe.has_permission("Financial Report Template", ptype="read", throw=True) + frappe.has_permission("Company", doc=company, throw=True) if isinstance(account_rows, str): account_rows = json.loads(account_rows, object_hook=frappe._dict) diff --git a/erpnext/accounts/doctype/financial_report_template/financial_report_validation.py b/erpnext/accounts/doctype/financial_report_template/financial_report_validation.py index 5f187006c7b..4ecdd126eb3 100644 --- a/erpnext/accounts/doctype/financial_report_template/financial_report_validation.py +++ b/erpnext/accounts/doctype/financial_report_template/financial_report_validation.py @@ -403,10 +403,19 @@ class AccountFilterValidator(Validator): self.account_fields = account_fields or set(self.account_meta._valid_columns) def validate(self, row) -> ValidationResult: - result = ValidationResult() - + # dispatch-path guard: only account-data rows are validated here if row.data_source != "Account Data": - return result + return ValidationResult() + + return self.validate_filter(row) + + def validate_filter(self, row) -> ValidationResult: + """Validate calculation_formula as an Account filter, regardless of data_source. + + The caller has already decided this row is an account filter, so unlike + `validate()` this does not opt out based on `data_source`. + """ + result = ValidationResult() try: filter_config = json.loads(row.calculation_formula) @@ -420,7 +429,7 @@ class AccountFilterValidator(Validator): result.add_error( ValidationIssue( message=_("[{0}] {1}", context="Financial Report Template").format( - get_formula_field_label(row.data_source), error + get_formula_field_label("Account Data"), error ), row_idx=row.idx, ) @@ -430,7 +439,7 @@ class AccountFilterValidator(Validator): result.add_error( ValidationIssue( message=_("[{0}] {1}", context="Financial Report Template").format( - get_formula_field_label(row.data_source), + get_formula_field_label("Account Data"), _("Invalid JSON format: {0}").format(str(e)), ), row_idx=row.idx, @@ -455,10 +464,9 @@ class AccountFilterValidator(Validator): if not isinstance(field, str) or not isinstance(operator, str): return _("Field and operator must be strings") - display = (field if advanced_filtering else self.account_meta.get_label(field)) or field - if field not in account_fields: - return _("Field '{0}' is not a valid Account field").format(display) + # escape: `field` is caller-supplied and this message renders as HTML + return _("Field '{0}' is not a valid Account field").format(frappe.utils.escape_html(field)) if operator.casefold() not in OPERATOR_MAP: return _("Invalid operator '{0}'").format(operator) diff --git a/erpnext/accounts/doctype/financial_report_template/test_financial_report_template.py b/erpnext/accounts/doctype/financial_report_template/test_financial_report_template.py index e49cc4c8333..7b23398a472 100644 --- a/erpnext/accounts/doctype/financial_report_template/test_financial_report_template.py +++ b/erpnext/accounts/doctype/financial_report_template/test_financial_report_template.py @@ -5,6 +5,7 @@ import frappe from frappe.tests.utils import whitelist_for_tests from erpnext.accounts.doctype.financial_report_template.financial_report_validation import ( + AccountFilterValidator, FormulaValidator, get_valid_api_method, ) @@ -164,3 +165,86 @@ class TestCustomAPIValidation(FinancialReportTemplateTestCase): result = validator.validate(row) self.assertFalse(result.is_valid) self.assertEqual(len(frappe.local.message_log), message_count) + + +class TestAccountFilter(FinancialReportTemplateTestCase): + """Filter fields must be validated on the account-filter parser path.""" + + @staticmethod + def _row(formula, **extra): + return frappe._dict(calculation_formula=formula, idx=1, **extra) + + def test_validate_filter_enforces_allow_list_without_data_source(self): + # the parser path has no `data_source`; the field allow-list must still apply + validator = AccountFilterValidator() + self.assertFalse(validator.validate_filter(self._row('["bad_field", "=", "x"]')).is_valid) + self.assertTrue(validator.validate_filter(self._row('["root_type", "=", "Income"]')).is_valid) + + def test_validate_gate_still_opts_out_for_non_account_data(self): + # validate() is the dispatch gate: it must not validate non "Account Data" rows + validator = AccountFilterValidator() + row = self._row('["bad_field", "=", "x"]', data_source="Custom API") + self.assertTrue(validator.validate(row).is_valid) + + def test_error_message_labels_and_escapes_field(self): + validator = AccountFilterValidator() + result = validator.validate_filter(self._row('["