Better error validation for printer profiles

Data types loaded from disk were not properly ensured to match expected types and input validation also had deficits.

Should fix #714

CHANGELOG.md

@@ -98,6 +98,7 @@
     printer profiles
   * [#683](https://github.com/foosel/OctoPrint/issues/683) - Fixed heated bed option not being properly displayed in
     printer profiles
+  * [#714](https://github.com/foosel/OctoPrint/issues/714) - Fixed type validation of printer profiles
 * Various fixes without tickets:
   * GCODE viewer now doesn't stumble over completely extrusionless GCODE files
   * Do not deliver the API key on settings API unless user has admin rights

src/octoprint/printer/profile.py

@@ -9,17 +9,28 @@
 import os
 import copy
 import re
+import logging
 
 from octoprint.settings import settings
-from octoprint.util import dict_merge, dict_clean
+from octoprint.util import dict_merge, dict_clean, dict_contains_keys
 
 class SaveError(Exception):
 	pass
 
+class CouldNotOverwriteError(SaveError):
+	pass
+
+class InvalidProfileError(Exception):
+	pass
+
 class BedTypes(object):
 	RECTANGULAR = "rectangular"
 	CIRCULAR = "circular"
 
+	@classmethod
+	def values(cls):
+		return [getattr(cls, name) for name in cls.__dict__ if not name.startswith("__")]
+
 class PrinterProfileManager(object):
 
 	default = dict(
@@ -51,8 +62,8 @@ class PrinterProfileManager(object):
 
 	def __init__(self):
 		self._current = None
-
 		self._folder = settings().getBaseFolder("printerProfiles")
+		self._logger = logging.getLogger(__name__)
 
 	def select(self, identifier):
 		if identifier is None or not self.exists(identifier):
@@ -69,11 +80,14 @@ def get_all(self):
 		return self._load_all()
 
 	def get(self, identifier):
-		if identifier == "_default":
-			return self._load_default()
-		elif self.exists(identifier):
-			return self._load_from_path(self._get_profile_path(identifier))
-		else:
+		try:
+			if identifier == "_default":
+				return self._load_default()
+			elif self.exists(identifier):
+				return self._load_from_path(self._get_profile_path(identifier))
+			else:
+				return None
+		except InvalidProfileError:
 			return None
 
 	def remove(self, identifier):
@@ -87,14 +101,17 @@ def save(self, profile, allow_overwrite=False, make_default=False):
 		elif "name" in profile:
 			identifier = profile["name"]
 		else:
-			raise ValueError("profile must contain either id or name")
+			raise InvalidProfileError("profile must contain either id or name")
 
 		identifier = self._sanitize(identifier)
 		profile["id"] = identifier
 		profile = dict_clean(profile, self.__class__.default)
 
 		if identifier == "_default":
 			default_profile = dict_merge(self._load_default(), profile)
+			if not self._ensure_valid_profile(default_profile):
+				raise InvalidProfileError()
+
 			settings().set(["printerProfiles", "defaultProfile"], default_profile, defaults=dict(printerProfiles=dict(defaultProfile=self.__class__.default)))
 			settings().save()
 		else:
@@ -144,10 +161,13 @@ def _load_all(self):
 		all_identifiers = self._load_all_identifiers()
 		results = dict()
 		for identifier, path in all_identifiers.items():
-			if identifier == "_default":
-				profile = self._load_default()
-			else:
-				profile = self._load_from_path(path)
+			try:
+				if identifier == "_default":
+					profile = self._load_default()
+				else:
+					profile = self._load_from_path(path)
+			except InvalidProfileError:
+				continue
 
 			if profile is None:
 				continue
@@ -176,9 +196,17 @@ def _load_from_path(self, path):
 		import yaml
 		with open(path) as f:
 			profile = yaml.safe_load(f)
+		profile = self._ensure_valid_profile(profile)
+		if not profile:
+			self._logger.warn("Invalid profile: %s" % path)
+			raise InvalidProfileError()
 		return profile
 
 	def _save_to_path(self, path, profile, allow_overwrite=False):
+		validated_profile = self._ensure_valid_profile(profile)
+		if not validated_profile:
+			raise InvalidProfileError()
+
 		if os.path.exists(path) and not allow_overwrite:
 			raise SaveError("Profile %s already exists and not allowed to overwrite" % profile["id"])
 
@@ -197,8 +225,12 @@ def _remove_from_path(self, path):
 			return False
 
 	def _load_default(self):
-		default_profile = settings().get(["printerProfiles", "defaultProfile"])
-		return dict_merge(copy.deepcopy(self.__class__.default), default_profile)
+		default_overrides = settings().get(["printerProfiles", "defaultProfile"])
+		profile = self._ensure_valid_profile(dict_merge(copy.deepcopy(self.__class__.default), default_overrides))
+		if not profile:
+			self._logger.warn("Invalid default profile after applying overrides")
+			raise InvalidProfileError()
+		return profile
 
 	def _get_profile_path(self, identifier):
 		return os.path.join(self._folder, "%s.profile" % identifier)
@@ -216,3 +248,60 @@ def _sanitize(self, name):
 		sanitized_name = sanitized_name.replace(" ", "_")
 		return sanitized_name
 
+	def _ensure_valid_profile(self, profile):
+		# ensure all keys are present
+		if not dict_contains_keys(self.default, profile):
+			return False
+
+		# conversion helper
+		def convert_value(profile, path, converter):
+			value = profile
+			for part in path[:-1]:
+				if not isinstance(value, dict) or not part in value:
+					raise RuntimeError("%s is not contained in profile" % ".".join(path))
+				value = value[part]
+
+			if not isinstance(value, dict) or not path[-1] in value:
+				raise RuntimeError("%s is not contained in profile" % ".".join(path))
+
+			value[path[-1]] = converter(value[path[-1]])
+
+		# convert ints
+		for path in (("extruder", "count"), ("axes", "x", "speed"), ("axes", "y", "speed"), ("axes", "z", "speed")):
+			try:
+				convert_value(profile, path, int)
+			except:
+				return False
+
+		# convert floats
+		for path in (("volume", "width"), ("volume", "depth"), ("volume", "height"), ("extruder", "nozzleDiameter")):
+			try:
+				convert_value(profile, path, float)
+			except:
+				return False
+
+		# convert booleans
+		for path in (("axes", "x", "inverted"), ("axes", "y", "inverted"), ("axes", "z", "inverted")):
+			try:
+				convert_value(profile, path, bool)
+			except:
+				return False
+
+		# validate form factor
+		if not profile["volume"]["formFactor"] in BedTypes.values():
+			return False
+
+		# validate offsets
+		offsets = []
+		for offset in profile["extruder"]["offsets"]:
+			if not len(offset) == 2:
+				return False
+			x_offset, y_offset = offset
+			try:
+				offsets.append((float(x_offset), float(y_offset)))
+			except:
+				return False
+		profile["extruder"]["offsets"] = offsets
+
+		return profile
+

src/octoprint/server/api/printer_profiles.py

@@ -15,6 +15,7 @@
 from octoprint.util import dict_merge
 
 from octoprint.server import printerProfileManager
+from octoprint.printer.profile import InvalidProfileError, CouldNotOverwriteError, SaveError
 
 
 @api.route("/printerprofiles", methods=["GET"])
@@ -42,18 +43,26 @@ def printerProfilesAdd():
 		del base_profile["id"]
 	if "name" in base_profile:
 		del base_profile["name"]
-	profile = dict_merge(base_profile, json_data["profile"])
-	if not _validate_profile(profile):
-		return None, None, make_response("Profile is invalid, missing obligatory values", 400)
 
-	return _overwrite_profile(profile)
+	profile = dict_merge(base_profile, json_data["profile"])
+	try:
+		saved_profile = printerProfileManager.save(profile, allow_overwrite=False)
+	except InvalidProfileError:
+		return make_response("Profile is invalid", 400)
+	except CouldNotOverwriteError:
+		return make_response("Profile already exists and overwriting was not allowed", 400)
+	except Exception as e:
+		return make_response("Could not save profile: %s" % e.message, 500)
+	else:
+		return jsonify(dict(profile=_convert_profile(saved_profile)))
 
 @api.route("/printerprofiles/<string:identifier>", methods=["GET"])
 def printerProfilesGet(identifier):
 	profile = printerProfileManager.get(identifier)
 	if profile is None:
-		make_response("Unknown profile: %s" % identifier, 404)
-	return jsonify(_convert_profile(profile))
+		return make_response("Unknown profile: %s" % identifier, 404)
+	else:
+		return jsonify(_convert_profile(profile))
 
 @api.route("/printerprofiles/<string:identifier>", methods=["DELETE"])
 @restricted_access
@@ -84,15 +93,17 @@ def printerProfilesUpdate(identifier):
 		del new_profile["default"]
 
 	new_profile["id"] = identifier
-	if not _validate_profile(new_profile):
-		make_response("Combined profile is invalid, missing obligatory values", 400)
 
 	try:
 		saved_profile = printerProfileManager.save(new_profile, allow_overwrite=True, make_default=make_default)
+	except InvalidProfileError:
+		return make_response("Profile is invalid", 400)
+	except CouldNotOverwriteError:
+		return make_response("Profile already exists and overwriting was not allowed", 400)
 	except Exception as e:
-		make_response("Could not save profile: %s" % e.message)
-
-	return jsonify(dict(profile=_convert_profile(saved_profile)))
+		return make_response("Could not save profile: %s" % e.message, 500)
+	else:
+		return jsonify(dict(profile=_convert_profile(saved_profile)))
 
 def _convert_profiles(profiles):
 	result = dict()
@@ -109,59 +120,3 @@ def _convert_profile(profile):
 	converted["default"] = (profile["id"] == default)
 	converted["current"] = (profile["id"] == current)
 	return converted
-
-def _validate_profile(profile):
-	if not "name" in profile \
-			and "volume" in profile \
-			and "width" in profile["volume"] \
-			and "depth" in profile["volume"] \
-			and "height" in profile["volume"] \
-			and "formFactor" in profile["volume"] \
-			and "heatedBed" in profile \
-			and "extruder" in profile \
-			and "count" in profile["extruder"] \
-			and "offsets" in profile["extruder"] \
-			and len(profile["extruder"]["offsets"]) == profile["extruder"]["count"]:
-		return False
-
-	for dimension in ("width", "depth", "height"):
-		try:
-			profile["volume"][dimension] = float(profile["volume"][dimension])
-		except:
-			return False
-
-	if not profile["volume"]["formFactor"] in ("rectangular", "circular"):
-		return False
-
-	try:
-		profile["heatedBed"] = bool(profile["heatedBed"])
-	except:
-		return False
-
-	try:
-		profile["extruder"]["count"] = int(profile["extruder"]["count"])
-	except:
-		return False
-
-	converted_offsets = []
-	for offset in profile["extruder"]["offsets"]:
-		try:
-			converted_offsets.append((float(offset[0]), float(offset[1])))
-		except:
-			return False
-	profile["extruder"]["offsets"] = converted_offsets
-
-	return True
-
-def _overwrite_profile(profile):
-	if not "id" in profile and not "name" in profile:
-		return None, None, make_response("Profile must contain either id or name")
-	elif not "name" in profile:
-		return None, None, make_response("Profile must contain a name")
-
-	try:
-		saved_profile = printerProfileManager.save(profile, allow_overwrite=False)
-	except Exception as e:
-		return None, None, make_response("Could not save profile: %s" % e.message)
-
-	return jsonify(dict(profile=_convert_profile(saved_profile)))

src/octoprint/static/js/app/viewmodels/printerprofiles.js

@@ -268,30 +268,30 @@ function PrinterProfilesViewModel() {
             color: self.editorColor(),
             model: self.editorModel(),
             volume: {
-                width: self.editorVolumeWidth(),
-                depth: self.editorVolumeDepth(),
-                height: self.editorVolumeHeight(),
+                width: parseFloat(self.editorVolumeWidth()),
+                depth: parseFloat(self.editorVolumeDepth()),
+                height: parseFloat(self.editorVolumeHeight()),
                 formFactor: self.editorVolumeFormFactor()
             },
             heatedBed: self.editorHeatedBed(),
             extruder: {
-                count: self.editorExtruders(),
+                count: parseInt(self.editorExtruders()),
                 offsets: [
                     [0.0, 0.0]
                 ],
-                nozzleDiameter: self.editorNozzleDiameter()
+                nozzleDiameter: parseFloat(self.editorNozzleDiameter())
             },
             axes: {
                 x: {
-                    speed: self.editorAxisXSpeed(),
+                    speed: parseInt(self.editorAxisXSpeed()),
                     inverted: self.editorAxisXInverted()
                 },
                 y: {
-                    speed: self.editorAxisYSpeed(),
+                    speed: parseInt(self.editorAxisYSpeed()),
                     inverted: self.editorAxisYInverted()
                 },
                 z: {
-                    speed: self.editorAxisZSpeed(),
+                    speed: parseInt(self.editorAxisZSpeed()),
                     inverted: self.editorAxisZInverted()
                 }
             }

src/octoprint/util/__init__.py

@@ -269,6 +269,20 @@ def dict_clean(a, b):
 	return result
 
 
+def dict_contains_keys(a, b):
+	if not isinstance(a, dict) or not isinstance(b, dict):
+		return False
+
+	for k, v in a.iteritems():
+		if not k in b:
+			return False
+		elif isinstance(v, dict):
+			if not dict_contains_keys(v, b[k]):
+				return False
+
+	return True
+
+
 class Object(object):
 	pass