fix: formula evaluation and line reference validation for FRT (#59084)

* fix: use one formula environment in validator and engine

* test: cover shared formula environment

* fix: use distinct dummy values when test-evaluating formulas

* fix: reject line references that can't be used in a formula

* fix: drop the undefined-reference check

* fix: normalise line references before validating

* fix: normalise formulas on save instead of during validation

* fix: escape validation messages where they are rendered

* fix: strip the formula in the engine instead of relying on the validator

* fix: ignore division by zero when test-evaluating formulas

erpnext/accounts/doctype/financial_report_template/financial_report_engine.py

@@ -3,7 +3,6 @@
 
 import ast
 import json
-import math
 from abc import ABC, abstractmethod
 from dataclasses import dataclass, field
 from functools import cache, reduce
@@ -29,6 +28,7 @@ from erpnext.accounts.doctype.financial_report_template.financial_report_templat
 	FinancialReportTemplate,
 )
 from erpnext.accounts.doctype.financial_report_template.financial_report_validation import (
+	FORMULA_FUNCTIONS,
 	AccountFilterValidator,
 	CalculationFormulaValidator,
 	DependencyValidator,
@@ -1328,26 +1328,14 @@ class FormulaCalculator:
 		self.precision = get_currency_precision()
 		self.validator = CalculationFormulaValidator(set(row_data.keys()))
 
-		self.math_functions = {
-			"abs": abs,
-			"round": round,
-			"min": min,
-			"max": max,
-			"sum": sum,
-			"sqrt": math.sqrt,
-			"pow": math.pow,
-			"ceil": math.ceil,
-			"floor": math.floor,
-		}
-
 	def evaluate_formula(self, report_row: dict[str, Any]) -> list[float]:
 		validation_result = self.validator.validate(report_row)
-		formula = report_row.calculation_formula
+		formula = (report_row.calculation_formula or "").strip()
 		negation_factor = -1 if report_row.reverse_sign else 1
 
 		if validation_result.issues:
 			# TODO: Throw?
-			messages = "<br><br>".join(issue.message for issue in validation_result.issues)
+			messages = "<br><br>".join(str(issue) for issue in validation_result.issues)
 			frappe.log_error(f"Formula validation errors found:\n{messages}")
 			return [0.0] * len(self.period_list)
 
@@ -1362,7 +1350,7 @@ class FormulaCalculator:
 		# TODO: consistent error handling
 		try:
 			context = self._build_context(period_index)
-			result = frappe.safe_eval(formula, context)
+			result = frappe.safe_eval(formula, eval_globals=None, eval_locals=context)
 			return flt(result * negation_factor, self.precision)
 
 		except ZeroDivisionError:
@@ -1383,7 +1371,7 @@ class FormulaCalculator:
 				context[code] = 0.0
 
 		# math functions
-		context.update(self.math_functions)
+		context.update(FORMULA_FUNCTIONS)
 
 		return context
 

erpnext/accounts/doctype/financial_report_template/financial_report_template.py

@@ -34,6 +34,13 @@ class FinancialReportTemplate(Document):
 	def before_validate(self):
 		self.clear_hidden_fields()
 
+		for row in self.rows:
+			if row.reference_code:
+				row.reference_code = row.reference_code.strip()
+
+			if row.calculation_formula:
+				row.calculation_formula = row.calculation_formula.strip()
+
 	def clear_hidden_fields(self):
 		style_data_sources = {"Blank Line", "Column Break", "Section Break"}
 

erpnext/accounts/doctype/financial_report_template/financial_report_validation.py

@@ -2,6 +2,8 @@
 # For license information, please see license.txt
 
 import json
+import keyword
+import math
 import re
 from abc import ABC, abstractmethod
 from dataclasses import dataclass, field
@@ -10,6 +12,19 @@ from typing import Any
 import frappe
 from frappe import _, is_whitelisted
 from frappe.database.operator_map import OPERATOR_MAP
+from frappe.utils import escape_html
+
+FORMULA_FUNCTIONS = {
+	"abs": abs,
+	"round": round,
+	"min": min,
+	"max": max,
+	"sum": sum,
+	"sqrt": math.sqrt,
+	"pow": math.pow,
+	"ceil": math.ceil,
+	"floor": math.floor,
+}
 
 
 def get_valid_api_method(api_path: str):
@@ -89,8 +104,9 @@ class ValidationResult:
 		self.warnings.append(issue)
 
 	def notify_user(self) -> None:
-		warnings = "<br><br>".join(str(w) for w in self.warnings if w)
-		errors = "<br><br>".join(str(e) for e in self.issues if e)
+		# messages quote user input back, and both are rendered as HTML
+		warnings = "<br><br>".join(escape_html(str(w)) for w in self.warnings if w)
+		errors = "<br><br>".join(escape_html(str(e)) for e in self.issues if e)
 
 		if warnings:
 			frappe.msgprint(warnings, title=_("Warnings"), indicator="orange")
@@ -147,18 +163,27 @@ class TemplateStructureValidator(Validator):
 			if not row.reference_code:
 				continue
 
-			ref_code = row.reference_code.strip()
+			ref_code = row.reference_code
 
-			# Check format
-			if not re.match(r"^[A-Za-z][A-Za-z0-9_-]*$", ref_code):
+			# a line reference is used as a name in formulas, so it must be a usable one
+			if not re.match(r"^[A-Za-z][A-Za-z0-9_]*$", ref_code):
 				result.add_error(
 					ValidationIssue(
 						message=_(
-							"Invalid line reference format: '{0}'. Must start with letter and contain only letters, numbers, underscores, and hyphens"
+							"Invalid line reference format: '{0}'. Must start with a letter and contain only letters, numbers and underscores"
 						).format(ref_code),
 						row_idx=row.idx,
 					)
 				)
+			elif keyword.iskeyword(ref_code) or ref_code in FORMULA_FUNCTIONS:
+				result.add_error(
+					ValidationIssue(
+						message=_("'{0}' is a reserved name and cannot be used as a line reference").format(
+							ref_code
+						),
+						row_idx=row.idx,
+					)
+				)
 
 			# Check uniqueness
 			if ref_code in used_codes:
@@ -208,12 +233,7 @@ class DependencyValidator(Validator):
 		self.dependencies = self._build_dependency_graph()
 
 	def validate(self, context=None) -> ValidationResult:
-		result = ValidationResult()
-
-		result.merge(self._validate_circular_dependencies())
-		result.merge(self._validate_missing_dependencies())
-
-		return result
+		return self._validate_circular_dependencies()
 
 	def _build_dependency_graph(self) -> dict[str, list[str]]:
 		graph = {}
@@ -280,31 +300,6 @@ class DependencyValidator(Validator):
 
 		return result
 
-	def _validate_missing_dependencies(self) -> ValidationResult:
-		available = {row.reference_code for row in self.template.rows if row.reference_code}
-		result = ValidationResult()
-
-		for ref_code, deps in self.dependencies.items():
-			undefined = [d for d in deps if d not in available]
-			if undefined:
-				row_idx = self._get_row_idx(ref_code)
-				result.add_error(
-					ValidationIssue(
-						message=_("Line references undefined in {0}: {1}").format(
-							get_formula_field_label("Calculated Amount"), ", ".join(undefined)
-						),
-						row_idx=row_idx,
-					)
-				)
-
-		return result
-
-	def _get_row_idx(self, reference_code: str) -> int | None:
-		for row in self.template.rows:
-			if row.reference_code == reference_code:
-				return row.idx
-		return None
-
 
 class CalculationFormulaValidator(Validator):
 	"""Validates calculation formulas used in Calculated Amount rows"""
@@ -320,7 +315,6 @@ class CalculationFormulaValidator(Validator):
 			return result
 
 		formula = self._preprocess_formula(row.calculation_formula)
-		row.calculation_formula = formula
 
 		# Check parentheses
 		if not self._are_parentheses_balanced(formula):
@@ -368,25 +362,15 @@ class CalculationFormulaValidator(Validator):
 	def _test_formula_evaluation(self, formula: str, available_codes: list[str]) -> str | None:
 		try:
 			context = {code: 1.0 for code in available_codes}
-			context.update(
-				{
-					"abs": abs,
-					"round": round,
-					"min": min,
-					"max": max,
-					"sum": sum,
-					"sqrt": lambda x: x**0.5,
-					"pow": pow,
-					"ceil": lambda x: int(x) + (1 if x % 1 else 0),
-					"floor": int,
-				}
-			)
+			context.update(FORMULA_FUNCTIONS)
 
 			result = frappe.safe_eval(formula, eval_globals=None, eval_locals=context)
 
-			if not isinstance(result, (int, float)):  # noqa: UP038
+			if not isinstance(result, (int | float)):
 				return _("Formula must return a numeric value, got {0}").format(type(result).__name__)
 
+			return None
+		except ZeroDivisionError:
 			return None
 		except Exception as e:
 			return str(e)
@@ -462,13 +446,14 @@ class AccountFilterValidator(Validator):
 				return _("Field and operator must be strings")
 
 			if field not in account_fields:
-				# 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))
+				return _("Field '{0}' is not a valid Account field").format(field)
 
-			if operator.casefold() not in OPERATOR_MAP:
+			normalized_operator = operator.casefold()
+
+			if normalized_operator not in OPERATOR_MAP:
 				return _("Invalid operator '{0}'").format(operator)
 
-			if operator in ["in", "not in"] and not isinstance(value, list):
+			if normalized_operator in ["in", "not in"] and not isinstance(value, list):
 				return _("Operator '{0}' requires a list value").format(operator)
 
 		# logical condition: {"and": [condition1, condition2]}

erpnext/accounts/doctype/financial_report_template/test_financial_report_template.py

@@ -5,8 +5,11 @@ import frappe
 from frappe.tests.utils import whitelist_for_tests
 
 from erpnext.accounts.doctype.financial_report_template.financial_report_validation import (
+	FORMULA_FUNCTIONS,
 	AccountFilterValidator,
+	CalculationFormulaValidator,
 	FormulaValidator,
+	TemplateStructureValidator,
 	get_valid_api_method,
 )
 from erpnext.tests.utils import ERPNextTestSuite
@@ -189,8 +192,13 @@ class TestAccountFilter(FinancialReportTemplateTestCase):
 	def test_error_message_labels_and_escapes_field(self):
 		validator = AccountFilterValidator()
 		result = validator.validate_filter(self._row('["<script>", "=", "x"]'))
-		message = str(result.issues[0])
-		self.assertIn("[Account Filter]", message)
+		self.assertIn("[Account Filter]", str(result.issues[0]))
+
+		# escaping happens where the message is rendered, not where it is built
+		frappe.clear_messages()
+		with self.assertRaises(frappe.ValidationError):
+			result.notify_user()
+		message = frappe.get_message_log()[-1]["message"]
 		self.assertIn("&lt;script&gt;", message)
 		self.assertNotIn("<script>", message)
 
@@ -248,3 +256,149 @@ class TestAccountFilter(FinancialReportTemplateTestCase):
 			pluck="name",
 		)
 		self.assertEqual(sorted(get_filtered_accounts(company, "[]")), sorted(expected))
+
+
+class TestFormulaEnvironment(FinancialReportTemplateTestCase):
+	"""Validator and engine must evaluate a formula in the same environment."""
+
+	@staticmethod
+	def _calc(row_data):
+		from erpnext.accounts.doctype.financial_report_template.financial_report_engine import (
+			FormulaCalculator,
+		)
+
+		return FormulaCalculator(row_data, [{"key": "p1"}])
+
+	@staticmethod
+	def _row(formula):
+		return frappe._dict(
+			calculation_formula=formula,
+			idx=1,
+			reverse_sign=0,
+			data_source="Calculated Amount",
+			reference_code="X",
+		)
+
+	def test_engine_keeps_reference_codes_named_like_builtins(self):
+		# "int" and "long" are whitelisted safe_eval globals; the row values must win
+		calc = self._calc({"int": [500.0], "long": [2000.0]})
+		self.assertEqual(calc.evaluate_formula(self._row("int + long"))[0], 2500.0)
+
+	def test_validator_keeps_reference_codes_named_like_builtins(self):
+		validator = CalculationFormulaValidator({"int", "long"})
+		self.assertTrue(validator.validate(self._row("int + long")).is_valid)
+
+	def test_engine_uses_the_shared_function_list(self):
+		context = self._calc({"A": [1.0]})._build_context(0)
+		for name, function in FORMULA_FUNCTIONS.items():
+			self.assertIs(context[name], function)
+
+	def test_rounding_matches_math_module(self):
+		calc = self._calc({"A": [1.0]})
+		self.assertEqual(calc.evaluate_formula(self._row("floor(-2.5)"))[0], -3.0)
+		self.assertEqual(calc.evaluate_formula(self._row("ceil(-2.5)"))[0], -2.0)
+
+
+class TestCalculationFormula(FinancialReportTemplateTestCase):
+	"""Formulas are test-evaluated with dummy values before a template can be saved."""
+
+	@staticmethod
+	def _validate(formula, codes=("A", "B", "C")):
+		row = frappe._dict(
+			calculation_formula=formula, idx=1, data_source="Calculated Amount", reference_code="X"
+		)
+		return CalculationFormulaValidator(set(codes)).validate(row)
+
+	def test_division_by_zero_is_not_a_validation_error(self):
+		# the dummy values are all 1.0, so a denominator can only be zero by accident;
+		# the engine tolerates real division by zero at run time
+		self.assertTrue(self._validate("A / (B - C)").is_valid)
+		self.assertTrue(self._validate("(A - B) / (A - C)").is_valid)
+		self.assertTrue(self._validate("ROM / (CAS + FDE - ROM)", ("ROM", "CAS", "FDE")).is_valid)
+		self.assertTrue(self._validate("A / 0").is_valid)
+
+	def test_broken_formulas_are_rejected(self):
+		self.assertFalse(self._validate("A +").is_valid)
+		self.assertFalse(self._validate("NOPE * 2").is_valid)
+		self.assertFalse(self._validate("'text'").is_valid)
+
+
+class TestFilterOperatorCase(FinancialReportTemplateTestCase):
+	"""Operators are matched case-insensitively, so their value checks must be too."""
+
+	@staticmethod
+	def _row(formula):
+		return frappe._dict(calculation_formula=formula, idx=1)
+
+	def test_uppercase_in_requires_a_list_value(self):
+		validator = AccountFilterValidator()
+		self.assertFalse(validator.validate_filter(self._row('["root_type", "IN", "Income"]')).is_valid)
+		self.assertFalse(validator.validate_filter(self._row('["root_type", "NOT IN", "Income"]')).is_valid)
+
+	def test_uppercase_in_accepts_a_list_value(self):
+		validator = AccountFilterValidator()
+		self.assertTrue(validator.validate_filter(self._row('["root_type", "IN", ["Income"]]')).is_valid)
+
+
+class TestLineReferenceNames(FinancialReportTemplateTestCase):
+	"""A line reference becomes a name in formulas, so it must be usable as one."""
+
+	@staticmethod
+	def _validate(code):
+		template = frappe._dict(rows=[frappe._dict(reference_code=code, idx=1, data_source="Blank Line")])
+		return TemplateStructureValidator()._validate_reference_codes(template)
+
+	def test_plain_codes_are_accepted(self):
+		for code in ("REV", "CA100", "cash_flow_2"):
+			self.assertTrue(self._validate(code).is_valid, code)
+
+	def test_hyphen_is_rejected(self):
+		# "-" reads as subtraction in a formula and is not a valid Python name
+		self.assertFalse(self._validate("REV-COGS").is_valid)
+
+	def test_python_keyword_is_rejected(self):
+		for code in ("if", "None", "class"):
+			self.assertFalse(self._validate(code).is_valid, code)
+
+	def test_formula_function_name_is_rejected(self):
+		# these would be overwritten by the function of the same name
+		for code in ("sum", "round", "abs"):
+			self.assertFalse(self._validate(code).is_valid, code)
+
+	def test_surrounding_spaces_are_normalised_before_validation(self):
+		template = frappe.new_doc("Financial Report Template")
+		template.template_name = "Spaces"
+		template.append("rows", {"reference_code": "  REV  ", "data_source": "Blank Line"})
+		template.append(
+			"rows",
+			{
+				"reference_code": "X",
+				"data_source": "Calculated Amount",
+				"calculation_formula": "  REV * 2  ",
+			},
+		)
+		template.before_validate()
+		self.assertEqual(template.rows[0].reference_code, "REV")
+		self.assertEqual(template.rows[1].calculation_formula, "REV * 2")
+
+	def test_validation_does_not_modify_the_row(self):
+		row = frappe._dict(
+			calculation_formula="  REV * 2  ",
+			idx=1,
+			data_source="Calculated Amount",
+			reference_code="X",
+		)
+		CalculationFormulaValidator({"REV", "X"}).validate(row)
+		self.assertEqual(row.calculation_formula, "  REV * 2  ")
+
+	def test_invalid_reference_code_is_escaped(self):
+		# this message fires when the code fails the format check, so it can hold anything
+		template = frappe._dict(rows=[frappe._dict(reference_code="<img src=x onerror=alert(1)>", idx=1)])
+		result = TemplateStructureValidator()._validate_reference_codes(template)
+
+		frappe.clear_messages()
+		with self.assertRaises(frappe.ValidationError):
+			result.notify_user()
+		message = frappe.get_message_log()[-1]["message"]
+		self.assertIn("&lt;img", message)
+		self.assertNotIn("<img", message)