fix(accounts): validate currency exchange provider requests and responses

erpnext/accounts/doctype/currency_exchange_settings/currency_exchange_settings.py

@@ -1,6 +1,8 @@
 # Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and contributors
 # For license information, please see license.txt
 
+from math import isfinite
+
 import frappe
 import requests
 from frappe import _
@@ -41,6 +43,14 @@ class CurrencyExchangeSettings(Document):
 		response, value = self.validate_parameters()
 		self.validate_result(response, value)
 
+	def on_update(self):
+		def clear_rates():
+			frappe.cache.delete_keys("currency_exchange_rate_")
+
+		clear_rates()
+		# Concurrent lookups can cache old provider rates before this save commits.
+		frappe.db.after_commit.add(clear_rates)
+
 	def set_parameters_and_result(self):
 		if self.service_provider == "exchangerate.host":
 			if not self.access_key:
@@ -88,7 +98,9 @@ class CurrencyExchangeSettings(Document):
 		api_url = self.api_endpoint.format(transaction_date=nowdate(), to_currency="INR", from_currency="USD")
 
 		try:
-			response = requests.get(api_url, params=params)
+			response = requests.get(
+				api_url, params=params, timeout=frappe.conf.get("currency_exchange_request_timeout") or 10
+			)
 		except requests.exceptions.RequestException as e:
 			frappe.throw("Error: " + str(e))
 
@@ -105,8 +117,8 @@ class CurrencyExchangeSettings(Document):
 				]
 		except Exception:
 			frappe.throw(_("Invalid result key. Response:") + " " + response.text)
-		if not isinstance(value, int | float):
-			frappe.throw(_("Returned exchange rate is neither integer not float."))
+		if isinstance(value, bool) or not isinstance(value, int | float) or not isfinite(value) or value <= 0:
+			frappe.throw(_("Returned exchange rate must be a positive finite number."))
 
 		self.url = response.url
 

erpnext/setup/utils.py

@@ -1,10 +1,15 @@
 # Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors
 # License: GNU General Public License v3. See license.txt
 
+from math import isfinite
+from urllib.parse import quote
+
 import frappe
+import requests
 from frappe import _
-from frappe.utils import add_days, flt, get_datetime_str, nowdate
-from frappe.utils.data import getdate, now_datetime
+from frappe.permissions import is_system_user
+from frappe.utils import add_days, flt, get_datetime_str, getdate, nowdate
+from frappe.utils.data import now_datetime
 from frappe.utils.nestedset import get_root_of
 
 from erpnext import get_default_company
@@ -98,11 +103,17 @@ def get_exchange_rate(from_currency, to_currency, transaction_date=None, args=No
 	if not (from_currency and to_currency):
 		# manqala 19/09/2016: Should this be an empty return or should it throw and exception?
 		return
+	for currency in (from_currency, to_currency):
+		if not isinstance(currency, str) or not frappe.db.exists("Currency", currency):
+			frappe.throw(_("Invalid currency: {0}").format(currency))
+
 	if from_currency == to_currency:
 		return 1
 
+	transaction_date = getdate(transaction_date)
 	if not transaction_date:
-		transaction_date = nowdate()
+		frappe.throw(_("Invalid transaction date"))
+	transaction_date = transaction_date.isoformat()
 
 	currency_settings = frappe.get_cached_doc("Accounts Settings")
 	allow_stale_rates = currency_settings.get("allow_stale")
@@ -143,10 +154,14 @@ def get_exchange_rate(from_currency, to_currency, transaction_date=None, args=No
 	try:
 		cache = frappe.cache()
 		key = f"currency_exchange_rate_{transaction_date}:{from_currency}:{to_currency}"
-		value = cache.get(key)
+		value = cache.get_value(key, expires=True)
 
 		if not value:
-			import requests
+			if not is_system_user():
+				frappe.throw(
+					_("Only system users can fetch exchange rates from an external provider."),
+					frappe.PermissionError,
+				)
 
 			settings = frappe.get_cached_doc("Currency Exchange Settings")
 			req_params = {
@@ -161,16 +176,29 @@ def get_exchange_rate(from_currency, to_currency, transaction_date=None, args=No
 			params = {}
 			for row in settings.req_params:
 				params[row.key] = format_ces_api(row.value, req_params)
-			response = requests.get(format_ces_api(settings.api_endpoint, req_params), params=params)
+			response = requests.get(
+				format_ces_api(
+					settings.api_endpoint, {key: quote(value, safe="") for key, value in req_params.items()}
+				),
+				params=params,
+				timeout=frappe.conf.get("currency_exchange_request_timeout") or 10,
+			)
 			# expire in 6 hours
 			response.raise_for_status()
 			value = response.json()
 			for res_key in settings.result_key:
 				value = value[format_ces_api(str(res_key.key), req_params)]
-			cache.setex(name=key, time=21600, value=flt(value))
+			if isinstance(value, bool):
+				raise ValueError("Invalid exchange rate")
+			value = float(value)
+			if not isfinite(value) or value <= 0:
+				raise ValueError("Exchange rate must be a positive finite number")
+			cache.set_value(key, value, expires_in_sec=21600)
 
 		# Support multiple pegged currencies
-		value = flt(value)
+		value = float(value)
+		if not isfinite(value) or value <= 0:
+			raise ValueError("Exchange rate must be a positive finite number")
 
 		if currency_settings.allow_pegged_currencies_exchange_rates and to_currency in pegged_currencies:
 			value *= flt(pegged_currencies[to_currency]["ratio"])
@@ -178,6 +206,8 @@ def get_exchange_rate(from_currency, to_currency, transaction_date=None, args=No
 			value /= flt(pegged_currencies[from_currency]["ratio"])
 
 		return flt(value)
+	except frappe.PermissionError:
+		raise
 	except Exception:
 		frappe.log_error("Unable to fetch exchange rate")
 		frappe.msgprint(