Merge pull request #58704 from frappe/mergify/bp/version-16-hotfix/pr-58697

fix!: improve validation in financial report template (backport #58697)

erpnext/accounts/doctype/financial_report_template/financial_report_engine.py

@@ -31,6 +31,7 @@ from erpnext.accounts.doctype.financial_report_template.financial_report_validat
 	AccountFilterValidator,
 	CalculationFormulaValidator,
 	DependencyValidator,
+	get_valid_api_method,
 )
 from erpnext.accounts.report.financial_statements import (
 	get_columns,
@@ -1164,10 +1165,12 @@ class RowProcessor:
 
 	def _process_api_row(self, row) -> RowData:
 		api_path = row.calculation_formula
-		# TODO
+
+		method = get_valid_api_method(api_path)
 
 		try:
-			values = frappe.call(api_path, filters=self.context.filters, periods=self.period_list, row=row)
+			# nosemgrep: frappe-semgrep-rules.rules.security.frappe-codeinjection-eval
+			values = frappe.call(method, filters=self.context.filters, periods=self.period_list, row=row)
 
 			if row.reverse_sign:
 				values = [-1 * v for v in values]

erpnext/accounts/doctype/financial_report_template/financial_report_template.js

@@ -370,7 +370,7 @@ function update_formula_description(frm, data_source) {
 		description_html = `
 			<div ${container_style}>
 				<h5 ${title_style}>Custom API Setup</h5>
-				<p ${text_style}>Path to your custom method that returns financial data.</p>
+				<p ${text_style}>Path to your custom whitelisted method that returns financial data. It must permit GET requests.</p>
 
 				<h6 ${subtitle_style}>Format:</h6>
 				<ul ${list_style}>
@@ -380,7 +380,8 @@ function update_formula_description(frm, data_source) {
 
 				<h6 ${subtitle_style}>Method Signature:</h6>
 				<div ${code_style}>
-					<pre ${pre_style}>def get_custom_data(filters, periods, row): <br>&nbsp; # filters: dict — report filters (company, period, etc.) <br>&nbsp; # periods: list[dict] — period definitions <br>&nbsp; # row: dict — the current report row <br><br>&nbsp; return [1000.0, 1200.0, 1150.0]  # one value per period</pre>
+					<!-- &#10; is used for line breaks since frappe.render replaces newlines with spaces -->
+					<pre ${pre_style} class="language-python">@frappe.whitelist(methods=["GET"])&#10;def get_custom_data(filters, periods, row):&#10;    # filters: dict — report filters (company, period, etc.)&#10;    # periods: list[dict] — period definitions&#10;    # row: dict — the current report row&#10;&#10;    return [1000.0, 1200.0, 1150.0]  # one value per period</pre>
 				</div>
 
 				<h6 ${subtitle_style}>Return Format:</h6>

erpnext/accounts/doctype/financial_report_template/financial_report_validation.py

@@ -10,11 +10,26 @@ from enum import Enum
 from typing import Any, ClassVar
 
 import frappe
-from frappe import _
+from frappe import _, is_whitelisted
 from frappe.database.operator_map import OPERATOR_MAP
 from frappe.database.query import SQLFunctionParser
 
 
+def get_valid_api_method(api_path: str):
+	"""Resolve `api_path`, ensuring it is whitelisted and permits GET (i.e. read-only)."""
+	method = frappe.get_attr(api_path)
+	is_whitelisted(method)
+
+	if "GET" not in frappe.allowed_http_methods_for_whitelisted_func.get(method, ()):
+		frappe.throw(
+			_("Method {0} must permit GET requests").format(frappe.bold(api_path)),
+			frappe.PermissionError,
+			title=_("Method Not Allowed"),
+		)
+
+	return method
+
+
 def get_formula_field_label(data_source: str) -> str:
 	# Must mirror the `labels` map in financial_report_template.js (update_formula_label),
 	labels = {
@@ -532,29 +547,23 @@ class FormulaValidator(Validator):
 			)
 			return result
 
-		# Method exists?
 		try:
-			module_path, method_name = api_path.rsplit(".", 1)
-			module = frappe.get_module(module_path)
-
-			if not hasattr(module, method_name):
-				result.add_error(
-					ValidationIssue(
-						message=_(
-							"{0}: Method '{1}' not found in module '{2}' (might be environment-specific)"
-						).format(get_formula_field_label(row.data_source), method_name, module_path),
-						row_idx=row.idx,
-					)
-				)
+			get_valid_api_method(api_path)
 		except Exception as e:
-			result.add_error(
-				ValidationIssue(
-					message=_("Could not validate {0}: {1}").format(
-						get_formula_field_label(row.data_source), str(e)
-					),
-					row_idx=row.idx,
+			if isinstance(e, frappe.PermissionError | frappe.ValidationError):
+				# frappe.throw inside get_valid_api_method logs a message that would pop up in UI
+				frappe.clear_last_message()
+
+			if isinstance(e, frappe.PermissionError):
+				message = _("{0}: Method '{1}' must be whitelisted and permit GET requests").format(
+					get_formula_field_label(row.data_source), api_path
 				)
-			)
+			else:
+				message = _("Could not validate {0}: {1}").format(
+					get_formula_field_label(row.data_source), str(e)
+				)
+
+			result.add_error(ValidationIssue(message=message, row_idx=row.idx))
 
 		return result
 

erpnext/accounts/doctype/financial_report_template/test_financial_report_template.py

@@ -2,7 +2,12 @@
 # For license information, please see license.txt
 
 import frappe
+from frappe.tests.utils import whitelist_for_tests
 
+from erpnext.accounts.doctype.financial_report_template.financial_report_validation import (
+	FormulaValidator,
+	get_valid_api_method,
+)
 from erpnext.tests.utils import ERPNextTestSuite
 
 
@@ -72,3 +77,90 @@ class FinancialReportTemplateTestCase(ERPNextTestSuite):
 			{"doctype": "Financial Report Template", "template_name": template_name, "rows": rows_data}
 		)
 		return template
+
+
+def not_whitelisted_method(**kwargs):
+	return [42.0]
+
+
+@whitelist_for_tests(methods=["POST"])
+def whitelisted_post_only_method(**kwargs):
+	return [42.0]
+
+
+@whitelist_for_tests(methods=["GET"])
+def whitelisted_get_method(**kwargs):
+	return [42.0]
+
+
+class TestCustomAPIValidation(FinancialReportTemplateTestCase):
+	"""Custom API rows must point to whitelisted methods that permit GET"""
+
+	TEST_MODULE = "erpnext.accounts.doctype.financial_report_template.test_financial_report_template"
+	NOT_WHITELISTED = f"{TEST_MODULE}.not_whitelisted_method"
+	WHITELISTED_POST_ONLY = f"{TEST_MODULE}.whitelisted_post_only_method"
+	WHITELISTED_GET = f"{TEST_MODULE}.whitelisted_get_method"
+
+	def create_api_template(self, api_path):
+		template = self.create_test_template_with_rows(
+			[
+				{
+					"reference_code": "API001",
+					"display_name": "API Row",
+					"data_source": "Custom API",
+					"calculation_formula": api_path,
+				}
+			]
+		)
+		template.report_type = "Profit and Loss Statement"
+		return template
+
+	def test_get_valid_api_method(self):
+		self.assertRaises(frappe.PermissionError, get_valid_api_method, self.NOT_WHITELISTED)
+		self.assertRaises(frappe.PermissionError, get_valid_api_method, self.WHITELISTED_POST_ONLY)
+		self.assertEqual(get_valid_api_method(self.WHITELISTED_GET), frappe.get_attr(self.WHITELISTED_GET))
+
+	def test_save_rejects_invalid_api_methods(self):
+		for api_path in (self.NOT_WHITELISTED, self.WHITELISTED_POST_ONLY):
+			template = self.create_api_template(api_path)
+			self.assertRaises(frappe.ValidationError, template.insert)
+
+	def test_save_allows_get_whitelisted_method(self):
+		template = self.create_api_template(self.WHITELISTED_GET)
+		template.insert()
+		template.delete()
+
+	def test_engine_rejects_invalid_api_methods(self):
+		from erpnext.accounts.doctype.financial_report_template.financial_report_engine import (
+			ReportContext,
+			RowProcessor,
+		)
+
+		for api_path in (self.NOT_WHITELISTED, self.WHITELISTED_POST_ONLY):
+			template = self.create_api_template(api_path)
+			context = ReportContext(template=template, filters={}, period_list=[{"key": "p1"}])
+			processor = RowProcessor(context)
+			self.assertRaises(frappe.PermissionError, processor._process_api_row, template.rows[0])
+
+	def test_engine_calls_valid_api_method(self):
+		from erpnext.accounts.doctype.financial_report_template.financial_report_engine import (
+			ReportContext,
+			RowProcessor,
+		)
+
+		template = self.create_api_template(self.WHITELISTED_GET)
+		context = ReportContext(template=template, filters={}, period_list=[{"key": "p1"}])
+		processor = RowProcessor(context)
+		row_data = processor._process_api_row(template.rows[0])
+		self.assertEqual(row_data.values, [42.0])
+
+	def test_validation_keeps_message_log_clean(self):
+		validator = FormulaValidator(frappe._dict(rows=[]))
+		message_count = len(frappe.local.message_log)
+
+		# last path raises AppNotInstalledError, which also logs a message via frappe.throw
+		for api_path in (self.NOT_WHITELISTED, self.WHITELISTED_POST_ONLY, "missing_app.api.method"):
+			row = frappe._dict(data_source="Custom API", calculation_formula=api_path, idx=1)
+			result = validator.validate(row)
+			self.assertFalse(result.is_valid)
+			self.assertEqual(len(frappe.local.message_log), message_count)