From f238ef40ecdc846ddc5fe30980ca091bcd637e94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gina=20H=C3=A4u=C3=9Fge?= Date: Mon, 12 Jan 2015 15:07:14 +0100 Subject: [PATCH] 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 | 1 + src/octoprint/printer/profile.py | 117 +++++++++++++++--- src/octoprint/server/api/printer_profiles.py | 89 ++++--------- .../js/app/viewmodels/printerprofiles.js | 16 +-- src/octoprint/util/__init__.py | 14 +++ 5 files changed, 148 insertions(+), 89 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 99c4717f..719207f8 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/octoprint/printer/profile.py b/src/octoprint/printer/profile.py index 319a08dc..bbb1105d 100644 --- a/src/octoprint/printer/profile.py +++ b/src/octoprint/printer/profile.py @@ -9,17 +9,28 @@ __copyright__ = "Copyright (C) 2014 The OctoPrint Project - Released under terms 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 @@ class PrinterProfileManager(object): 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,7 +101,7 @@ class PrinterProfileManager(object): 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 @@ -95,6 +109,9 @@ class PrinterProfileManager(object): 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 @@ class PrinterProfileManager(object): 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 @@ class PrinterProfileManager(object): 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 @@ class PrinterProfileManager(object): 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 @@ class PrinterProfileManager(object): 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 + diff --git a/src/octoprint/server/api/printer_profiles.py b/src/octoprint/server/api/printer_profiles.py index 8cd8d0ac..b1d05004 100644 --- a/src/octoprint/server/api/printer_profiles.py +++ b/src/octoprint/server/api/printer_profiles.py @@ -15,6 +15,7 @@ from octoprint.server.util.flask import restricted_access 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/", 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/", 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))) diff --git a/src/octoprint/static/js/app/viewmodels/printerprofiles.js b/src/octoprint/static/js/app/viewmodels/printerprofiles.js index aaeae981..cc98881d 100644 --- a/src/octoprint/static/js/app/viewmodels/printerprofiles.js +++ b/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() } } diff --git a/src/octoprint/util/__init__.py b/src/octoprint/util/__init__.py index 2c7b72bd..b07fa6e0 100644 --- a/src/octoprint/util/__init__.py +++ b/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