From 89a9eb3f738a5bc899dd9396d8cea58086db1632 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Thu, 8 Oct 2020 14:25:11 +0200 Subject: [PATCH 01/15] [wip] Epic refactoring --- package_linter.py | 921 ++++++++++++++++++++++++++-------------------- 1 file changed, 517 insertions(+), 404 deletions(-) diff --git a/package_linter.py b/package_linter.py index 8475762..9994515 100755 --- a/package_linter.py +++ b/package_linter.py @@ -162,17 +162,6 @@ official_helpers = { # Utilities # ############################################################################ -# Taken from https://stackoverflow.com/a/49518779 -def check_for_duplicate_keys(ordered_pairs): - dict_out = {} - for key, val in ordered_pairs: - if key in dict_out: - print_warning("Duplicated key '%s' in %s" % (key, ordered_pairs)) - else: - dict_out[key] = val - return dict_out - - class c: HEADER = '\033[94m' OKBLUE = '\033[94m' @@ -184,6 +173,23 @@ class c: BOLD = '\033[1m' UNDERLINE = '\033[4m' +class TestReport: + + def __init__(self, message): + self.message = message + + def display(self): + _print(self.style, self.message, c.END) + +class Warning(TestReport): + style = c.WARNING + "!" + +class Error(TestReport): + style = c.FAIL + "✘" + +class Info(TestReport): + style = c.OKBLUE + def header(app): _print(""" @@ -213,24 +219,10 @@ def print_header(str): _print("\n [" + c.BOLD + c.HEADER + str.title() + c.END + "]\n") -def print_warning_not_reliable(str): +def report_warning_not_reliable(str): _print(c.MAYBE_FAIL + "?", str, c.END) -warnings = [] - - -def print_warning(str): - warnings.append(str) - _print(c.WARNING + "!", str, c.END) - - -errors = [] - - -def print_error(str): - errors.append(str) - _print(c.FAIL + "✘", str, c.END) def print_happy(str): @@ -257,12 +249,53 @@ def spdx_licenses(): if os.path.exists(cachefile) and time.time() - os.path.getmtime(cachefile) < 3600: return open(cachefile).read() - link = "https://spdx.org/licenses/" - content = urlopen(link)['content'] + url = "https://spdx.org/licenses/" + content = urlopen(url)['content'] open(cachefile, "w").write(content) return content +def app_list(): + + cachefile = "./.apps.json" + if os.path.exists(cachefile) and time.time() - os.path.getmtime(cachefile) < 3600: + try: + return json.loads(open(cachefile).read()) + except: + _print("Uuuuh failed to load apps.json from cache...") + + url = "https://raw.githubusercontent.com/YunoHost/apps/master/apps.json" + content = urlopen(url)['content'] + open(cachefile, "w").write(content) + return json.loads(content) + + +tests = {} +tests_reports = [] + +def test(**kwargs): + def decorator(f): + clsname = f.__qualname__.split(".")[0] + if clsname not in tests: + tests[clsname] = [] + tests[clsname].append((f,kwargs)) + return f + return decorator + +class TestSuite(): + + def run_tests(self): + for test, options in tests[self.__class__.__name__]: + if "only" in options and self.name not in options["only"]: + continue + if "ignore" in options and self.name in options["ignore"]: + continue + reports = list(test(self)) + for report in reports: + if output == "plain": + report.display() + tests_reports.append((test.__qualname__, report)) + # ############################################################################ # Actual high-level checks # ############################################################################ @@ -270,134 +303,205 @@ def spdx_licenses(): scriptnames = ["_common.sh", "install", "remove", "upgrade", "backup", "restore"] -class App(): +class App(TestSuite): def __init__(self, path): print_header("LOADING APP") self.path = path + self.manifest_ = Manifest(self.path) + self.manifest = self.manifest_.manifest self.scripts = {f: Script(self.path, f) for f in scriptnames} + def analyze(self): - self.check_manifest() - self.misc_file_checks() - self.check_helpers_usage() + print_header("MANIFEST") + self.manifest_.run_tests() for script in [self.scripts[s] for s in scriptnames if self.scripts[s].exists]: - script.analyze() + print_header(script.name.upper() + " SCRIPT") + script.run_tests() - def check_helpers_usage(self): + print_header("MISC HELPER USAGE / CONSISTENCY") + self.run_tests() - print_header("HELPERS USAGE") + ####################################### + # _ _ _ # + # | | | | | | # + # | |__| | ___| |_ __ ___ _ __ ___ # + # | __ |/ _ \ | '_ \ / _ \ '__/ __| # + # | | | | __/ | |_) | __/ | \__ \ # + # |_| |_|\___|_| .__/ \___|_| |___/ # + # | | # + # |_| # + ####################################### - # Check for custom helpers definition that are now official... - cmd = "grep -IhEro 'ynh_\w+ *\( *\)' '%s/scripts' | tr -d '() '" % self.path + @test() + def helpers_now_official(app): + + cmd = "grep -IhEro 'ynh_\w+ *\( *\)' '%s/scripts' | tr -d '() '" % app.path custom_helpers = subprocess.check_output(cmd, shell=True).decode('utf-8').strip().split("\n") custom_helpers = [c.split("__")[0] for c in custom_helpers] for custom_helper in custom_helpers: if custom_helper in official_helpers.keys(): - print_warning("%s is now an official helper since version '%s'" % (custom_helper, official_helpers[custom_helper] or '?')) + yield Warning("%s is now an official helper since version '%s'" % (custom_helper, official_helpers[custom_helper] or '?')) - # Check for helpers usage that do not match version required in manifest... - if self.yunohost_version_req: - cmd = "grep -IhEro 'ynh_\w+' '%s/scripts'" % self.path - helpers_used = subprocess.check_output(cmd, shell=True).decode('utf-8').strip().split("\n") - helpers_used = sorted(set(helpers_used)) + @test() + def helpers_version_requirement(app): - manifest_req = [int(i) for i in self.yunohost_version_req.strip(">= ").split('.')] + [0,0,0] - def validate_version_requirement(helper_req): - if helper_req == '': - return True - helper_req = [int(i) for i in helper_req.split('.')] - for i in range(0,len(helper_req)): - if helper_req[i] == manifest_req[i]: - continue - return helper_req[i] <= manifest_req[i] + cmd = "grep -IhEro 'ynh_\w+ *\( *\)' '%s/scripts' | tr -d '() '" % app.path + custom_helpers = subprocess.check_output(cmd, shell=True).decode('utf-8').strip().split("\n") + custom_helpers = [c.split("__")[0] for c in custom_helpers] + + yunohost_version_req = app.manifest.get("requirements", {}).get("yunohost", "").strip(">= ") + + cmd = "grep -IhEro 'ynh_\w+' '%s/scripts'" % app.path + helpers_used = subprocess.check_output(cmd, shell=True).decode('utf-8').strip().split("\n") + helpers_used = sorted(set(helpers_used)) + + manifest_req = [int(i) for i in yunohost_version_req.split('.')] + [0,0,0] + def validate_version_requirement(helper_req): + if helper_req == '': return True - - for helper in [h for h in helpers_used if h in official_helpers.keys()]: - if helper in custom_helpers: + helper_req = [int(i) for i in helper_req.split('.')] + for i in range(0,len(helper_req)): + if helper_req[i] == manifest_req[i]: continue - helper_req = official_helpers[helper] - if not validate_version_requirement(helper_req): - major_diff = manifest_req[0] > int(helper_req[0]) - message = "Using official helper %s implies requiring at least version %s, but manifest only requires %s" % (helper, helper_req, self.yunohost_version_req) - if major_diff: - print_error(message) - else: - print_warning(message) + return helper_req[i] <= manifest_req[i] + return True + + for helper in [h for h in helpers_used if h in official_helpers.keys()]: + if helper in custom_helpers: + continue + helper_req = official_helpers[helper] + if not validate_version_requirement(helper_req): + major_diff = manifest_req[0] > int(helper_req[0]) + message = "Using official helper %s implies requiring at least version %s, but manifest only requires %s" % (helper, helper_req, yunohost_version_req) + yield Error(message) if major_diff else Warning(message) - def misc_file_checks(self): + @test() + def helper_consistency(app): + """ + check if ynh_install_app_dependencies is present in install/upgrade/restore + so dependencies are up to date after restoration or upgrade + """ - print_header("MISC FILE CHECKS") + install_script = app.scripts["install"] + if install_script.exists: + if install_script.contains("ynh_install_app_dependencies"): + for name in ["upgrade", "restore"]: + if app.scripts[name].exists and not app.scripts[name].contains("ynh_install_app_dependencies"): + yield Warning("ynh_install_app_dependencies should also be in %s script" % name) - # - # Check for recommended and mandatory files - # + if install_script.contains("yunohost service add"): + if app.scripts["remove"].exists and not app.scripts["remove"].contains("yunohost service remove"): + yield Error( + "You used 'yunohost service add' in the install script, " + "but not 'yunohost service remove' in the remove script." + ) +# +# if "services" in manifest and app.scripts["install"].exists: +# +# known_services = ("nginx", "mysql", "uwsgi", "metronome", +# "php5-fpm", "php7.0-fpm", "php-fpm", +# "postfix", "dovecot", "rspamd") +# +# for service in manifest["services"]: +# if service not in known_services: +# if service == 'postgresql': +# if not app.scripts["install"].contains('ynh_psql_test_if_first_run')\ +# or not app.scripts["restore"].contains('ynh_psql_test_if_first_run'): +# report_error("[YEP-2.1?] postgresql service present in the manifest, install and restore scripts must call ynh_psql_test_if_first_run") +# elif not app.scripts["install"].contains("yunohost service add %s" % service): +# report_error("[YEP-2.1?] " + service + " service not installed by the install file but present in the manifest") +# + + + + + ########################################################### + # _____ __ __ _ # + # / ____| / _| / / (_) # + # | | ___ _ __ | |_ / / _ __ ___ _ ___ ___ # + # | | / _ \| '_ \| _| / / | '_ ` _ \| / __|/ __| # + # | |___| (_) | | | | | / / | | | | | | \__ \ (__ # + # \_____\___/|_| |_|_| /_/ |_| |_| |_|_|___/\___| # + # # + ########################################################### + + @test() + def misc_files(app): filenames = ("manifest.json", "LICENSE", "README.md", "scripts/install", "scripts/remove", "scripts/upgrade", "scripts/backup", "scripts/restore") - non_mandatory = ("script/backup", "script/restore") for filename in filenames: - if file_exists(self.path + "/" + filename): - continue - elif filename in non_mandatory: - print_warning("Consider adding a file %s" % filename) - else: - print_error("Providing a %s is mandatory" % filename) + if not file_exists(app.path + "/" + filename): + yield Error("Providing %s is mandatory" % filename) - # - # Deprecated php-fpm.ini thing - # - if file_exists(self.path + "/conf/php-fpm.ini"): - print_warning( + @test() + def misc_legacy_phpini(app): + + if file_exists(app.path + "/conf/php-fpm.ini"): + yield Warning( "Using a separate php-fpm.ini file is deprecated. " "Please merge your php-fpm directives directly in the pool file. " "(c.f. https://github.com/YunoHost-Apps/nextcloud_ynh/issues/138 )" ) - # - # Source management - # - source_dir = os.path.join(self.path, "sources") + + @test() + def misc_source_management(app): + + source_dir = os.path.join(app.path, "sources") if os.path.exists(source_dir) \ and len([name for name in os.listdir(source_dir) if os.path.isfile(os.path.join(source_dir, name))]) > 5: - print_warning( - "[YEP-3.3] Upstream app sources shouldn't be stored in this 'sources' folder of this git repository as a copy/paste\n" + yield Warning( + "Upstream app sources shouldn't be stored in this 'sources' folder of this git repository as a copy/paste\n" "During installation, the package should download sources from upstream via 'ynh_setup_source'.\n" "See the helper documentation. " "Original discussion happened here : " "https://github.com/YunoHost/issues/issues/201#issuecomment-391549262" ) + + + @test() + def misc_nginx_add_header(app): + # # Analyze nginx conf # - Deprecated usage of 'add_header' in nginx conf # - Spot path traversal issue vulnerability # - for filename in os.listdir(self.path + "/conf") if os.path.exists(self.path + "/conf") else []: + for filename in os.listdir(app.path + "/conf") if os.path.exists(app.path + "/conf") else []: # Ignore subdirs or filename not containing nginx in the name - if not os.path.isfile(self.path + "/conf/" + filename) or "nginx" not in filename: + if not os.path.isfile(app.path + "/conf/" + filename) or "nginx" not in filename: continue - # - # 'add_header' usage - # - content = open(self.path + "/conf/" + filename).read() + content = open(app.path + "/conf/" + filename).read() if "location" in content and "add_header" in content: - print_warning( + yield Warning( "Do not use 'add_header' in the nginx conf. Use 'more_set_headers' instead. " "(See https://www.peterbe.com/plog/be-very-careful-with-your-add_header-in-nginx " "and https://github.com/openresty/headers-more-nginx-module#more_set_headers )" ) + + @test() + def misc_nginx_path_traversal(app): + for filename in os.listdir(app.path + "/conf") if os.path.exists(app.path + "/conf") else []: + # Ignore subdirs or filename not containing nginx in the name + if not os.path.isfile(app.path + "/conf/" + filename) or "nginx" not in filename: + continue + content = open(app.path + "/conf/" + filename).read() + # # Path traversal issues # @@ -457,13 +561,13 @@ class App(): if do_path_traversal_check: from lib.nginxparser import nginxparser try: - nginxconf = nginxparser.load(open(self.path + "/conf/" + filename)) + nginxconf = nginxparser.load(open(app.path + "/conf/" + filename)) except Exception as e: - print_warning_not_reliable("Could not parse nginx conf ... : " + str(e)) + _print("Could not parse nginx conf ... : " + str(e)) nginxconf = [] for location in find_path_traversal_issue(nginxconf): - print_warning( + yield Warning( "The nginx configuration (especially location %s) " "appears vulnerable to path traversal issues as explained in\n" " https://www.acunetix.com/vulnerabilities/web/path-traversal-via-misconfigured-nginx-alias/\n" @@ -471,247 +575,234 @@ class App(): " https://github.com/YunoHost/example_ynh/blob/master/conf/nginx.conf" % location ) - def check_helper_consistency(self): - """ - check if ynh_install_app_dependencies is present in install/upgrade/restore - so dependencies are up to date after restoration or upgrade - """ +############################################# +# __ __ _ __ _ # +# | \/ | (_)/ _| | | # +# | \ / | __ _ _ __ _| |_ ___ ___| |_ # +# | |\/| |/ _` | '_ \| | _/ _ \/ __| __| # +# | | | | (_| | | | | | || __/\__ \ |_ # +# |_| |_|\__,_|_| |_|_|_| \___||___/\__| # +# # +############################################# - install_script = self.scripts["install"] - if install_script.exists: - if install_script.contains("ynh_install_app_dependencies"): - for name in ["upgrade", "restore"]: - if self.scripts[name].exists and not self.scripts[name].contains("ynh_install_app_dependencies"): - print_warning("ynh_install_app_dependencies should also be in %s script" % name) +class Manifest(TestSuite): - if install_script.contains("yunohost service add"): - if self.scripts["remove"].exists and not self.scripts["remove"].contains("yunohost service remove"): - print_error( - "You used 'yunohost service add' in the install script, " - "but not 'yunohost service remove' in the remove script." - ) + def __init__(self, path): + self.path = path + manifest_path = os.path.join(path, 'manifest.json') + assert os.path.exists(manifest_path), "manifest.json is missing" - def check_manifest(self): - manifest = os.path.join(self.path, 'manifest.json') - if not os.path.exists(manifest): - return - print_header("MANIFEST") - """ - Check if there is no comma syntax issue - """ + # Taken from https://stackoverflow.com/a/49518779 + def check_for_duplicate_keys(ordered_pairs): + dict_out = {} + for key, val in ordered_pairs: + if key in dict_out: + raise Exception("Duplicated key '%s' in %s" % (key, ordered_pairs)) + else: + dict_out[key] = val + return dict_out + manifest_path = os.path.join(path, 'manifest.json') + raw_manifest = open(manifest_path, encoding='utf-8').read() try: - with open(manifest, encoding='utf-8') as data_file: - manifest = json.loads(data_file.read(), object_pairs_hook=check_for_duplicate_keys) - except: - print_error("[YEP-2.1] Syntax (comma) or encoding issue with manifest.json. Can't check file.") + self.manifest = json.loads(raw_manifest, object_pairs_hook=check_for_duplicate_keys) + except Exception as e: + raise Exception("Looks like there's a syntax issue in your json ? %s" % e) + + + @test() + def mandatory_fields(self): fields = ("name", "id", "packaging_format", "description", "url", "version", "license", "maintainer", "requirements", "multi_instance", "services", "arguments") + missing_fields = [field for field in fields if field not in self.manifest.keys()] - for field in fields: - if field not in manifest: - print_warning("[YEP-2.1] \"" + field + "\" field is missing") + if missing_fields: + yield Error("The following mandatory fields are missing: %s" % missing_fields) - """ - Check values in keys - """ + @test() + def yunohost_version_requirement(self): - if "packaging_format" not in manifest: - print_error("[YEP-2.1] \"packaging_format\" key is missing") - elif not isinstance(manifest["packaging_format"], int): - print_error("[YEP-2.1] \"packaging_format\": value isn't an integer type") - elif manifest["packaging_format"] != 1: - print_error("[YEP-2.1] \"packaging_format\" field: current format value is '1'") + if not self.manifest.get("requirements", {}).get("yunohost", ""): + yield Error("You should add a yunohost version requirement in the manifest") - # YEP 1.1 Name is app - if "id" in manifest: - if not re.match('^[a-z0-9]((_|-)?[a-z0-9])+$', manifest["id"]): - print_error("[YEP-1.1] 'id' field '%s' should respect this regex '^[a-z0-9]((_|-)?[a-z0-9])+$'") + @test() + def basic_fields_format(self): - if "name" in manifest: - if len(manifest["name"]) > 22: - print_warning( - "[YEP-1.1] The 'name' field shouldn't be too long to be able to be with one line in the app list. " - "The most current bigger name is actually compound of 22 characters." - ) + if self.manifest.get("packaging_format") != 1: + yield Error("packaging_format should be 1") + if not re.match('^[a-z0-9]((_|-)?[a-z0-9])+$', self.manifest.get("id")): + yield Error("The app id is not a valid app id") + if len(self.manifest["name"]) > 22: + yield Warning("The app name is too long") - # YEP 1.3 License - def license_mentionned_in_readme(path): - readme_path = os.path.join(path, 'README.md') + @test() + def license(self): + + if not "license" in self.manifest: + return + + def license_mentionned_in_readme(): + readme_path = os.path.join(app.path, 'README.md') if os.path.isfile(readme_path): return "LICENSE" in open(readme_path).read() return False - if "license" in manifest: - for license in manifest['license'].replace('&', ',').split(','): - code_license = '' + license + '' - if license == "nonfree": - print_warning("[YEP-1.3] The correct value for non free license in license field is 'non-free' and not 'nonfree'") - license = "non-free" - if license in ["free", "non-free", "dep-non-free"]: - if not license_mentionned_in_readme(self.path): - print_warning( - "[YEP-1.3] The use of '%s' in license field implies " - " to write something about the license in your README.md" % (license) - ) - if license in ["non-free", "dep-non-free"]: - print_warning( - "[YEP-1.3] 'non-free' apps can't be officialized. " - " Their integration is still being discussed, especially for apps with non-free dependencies" - ) - elif code_license not in spdx_licenses(): - print_warning( - "[YEP-1.3] The license '%s' is not registered in https://spdx.org/licenses/ . " - "It can be a typo error. If not, you should replace it by 'free' " - "or 'non-free' and give some explanations in the README.md." % (license) - ) - - # YEP 1.4 Inform if we continue to maintain the app - # YEP 1.5 Update regularly the app status - # YEP 1.6 Check regularly the evolution of the upstream - - # YEP 1.2 Put the app in a weel known repo - # YEP 1.7 - Add an app to the YunoHost-Apps organization - if "id" in manifest: - - cachefile = "./.apps.json" - app_list = None - if os.path.exists(cachefile) and time.time() - os.path.getmtime(cachefile) < 3600: - try: - app_list = open(cachefile).read() - app_list = json.loads(app_list) - except: - _print("Uuuuh failed to load apps.json from cache...") - app_list = None - - if app_list is None: - app_list_url = "https://raw.githubusercontent.com/YunoHost/apps/master/apps.json" - app_list = urlopen(app_list_url)['content'] - open(cachefile, "w").write(app_list) - app_list = json.loads(app_list) - - if manifest["id"] not in app_list: - print_warning("[YEP-1.2] This app is not registered in our applications list") - - all_urls = [infos.get("url", "").lower() for infos in app_list.values()] - - repo_org = "https://github.com/YunoHost-Apps/%s_ynh" % (manifest["id"]) - repo_brique = "https://github.com/labriqueinternet/%s_ynh" % (manifest["id"]) - - if repo_org.lower() not in all_urls and repo_brique.lower() not in all_urls: - is_not_added_to_org = urlopen(repo_org)['code'] == 404 - is_not_added_to_brique = urlopen(repo_brique)['code'] == 404 - - if is_not_added_to_org and is_not_added_to_brique: - print_warning("[YEP-1.7] You should add your app in the YunoHost-Apps organisation.") - - # YEP 1.8 Publish test request - # YEP 1.9 Document app - if "description" in manifest: - descr = manifest.get("description", "") - if isinstance(descr, dict): - descr = descr.get("en", "") - - if len(descr) < 5: - print_warning( - "[YEP-1.9] You should write a good description of the app, " - "at least in english (1 line is enough)." - ) - - if len(descr) > 150: - print_warning( - "[YEP-1.9] Please use a shorter description (or the rendering on the webadmin / app list will be messy ...). Just describe in consise terms what the app is / does." - ) - - elif "for yunohost" in descr.lower() : - print_warning( - "[YEP-1.9] The 'description' should explain what the app actually does. " - "No need to say that it is 'for YunoHost' - this is a YunoHost app " - "so of course we know it is for YunoHost ;-)." - ) - if descr.lower().startswith(manifest["id"].lower()) or descr.lower().startswith(manifest["name"].lower()): - print_warning("[YEP-1.9] Try to avoid starting the description by '$app is' ... explain what the app is / does directly !") - - # TODO test a specific template in README.md - - # YEP 1.10 Garder un historique de version propre - - if not "version" in manifest or manifest["version"][-5:-1] != "~ynh": - print_warning("Please specify a 'version' field in the manifest. It should match the format ~ynh. For example : 4.3-2~ynh3. It is composed of the upstream version number (in the example, 4.3-2) and an incremental number for each change in the package without upstream change (in the example, 3). This incremental number can be reset to 1 each time the upstream version changes.") - - # YEP 1.11 Cancelled - - # YEP 2.1 - if "multi_instance" in manifest and manifest["multi_instance"] != 1 and manifest["multi_instance"] != 0: - print_error( - "[YEP-2.1] \"multi_instance\" field must be boolean type values 'true' or 'false' and not string type") - - if "services" in manifest and self.scripts["install"].exists: - - known_services = ("nginx", "mysql", "uwsgi", "metronome", - "php5-fpm", "php7.0-fpm", "php-fpm", - "postfix", "dovecot", "rspamd") - - for service in manifest["services"]: - if service not in known_services: - if service == 'postgresql': - if not self.scripts["install"].contains('ynh_psql_test_if_first_run')\ - or not self.scripts["restore"].contains('ynh_psql_test_if_first_run'): - print_error("[YEP-2.1?] postgresql service present in the manifest, install and restore scripts must call ynh_psql_test_if_first_run") - elif not self.scripts["install"].contains("yunohost service add %s" % service): - print_error("[YEP-2.1?] " + service + " service not installed by the install file but present in the manifest") - - if "install" in manifest["arguments"]: - - recognized_types = ("domain", "path", "boolean", "app", "password", "user", "string", "display_text") - - for argument in manifest["arguments"]["install"]: - if "optional" in argument.keys(): - if not isinstance(argument["optional"], bool): - print_warning("The key 'optional' value for setting %s should be a boolean (true or false)" % argument["name"]) - if "type" not in argument.keys(): - print_warning( - "[YEP-2.1] You should specify the type of the argument '%s'. " - "You can use : %s." % (argument["name"], ', '.join(recognized_types)) - ) - elif argument["type"] not in recognized_types: - print_warning( - "[YEP-2.1] The type '%s' for argument '%s' is not recognized... " - "it probably doesn't behave as you expect ? Choose among those instead : %s" % (argument["type"], argument["name"], ', '.join(recognized_types)) - ) - - if "choices" in argument.keys(): - choices = [c.lower() for c in argument["choices"]] - if len(choices) == 2: - if ("true" in choices and "false" in choices) or ("yes" in choices and "no" in choices): - print_warning( - "Argument %s : you might want to simply use a boolean-type argument. " - "No need to specify the choices list yourself." % argument["name"] - ) - - if argument["name"] == "is_public" and "help" not in argument.keys(): - print_warning_not_reliable( - "Consider adding an 'help' key for argument 'is_public' " - "to explain to the user what it means for *this* app " - "to be public or private :\n" - ' "help": {\n' - ' "en": "Some explanation"\n' - ' }') + license = self.manifest["license"] - if "url" in manifest and manifest["url"].endswith("_ynh"): - print_warning( + if "nonfree" in license.replace("-", ""): + yield Warning("'non-free' apps can't be officialized.") + + + code_license = '' + license + '' + + if license == "free" and not license_mentionned_in_readme(): + yield Warning( + "Setting the license as 'free' implies to write something about it in " + "the README.md. Alternatively, consider using one of the codes available " + "on https://spdx.org/licenses/" + ) + elif code_license not in spdx_licenses(): + yield Warning( + "The license id '%s' is not registered in https://spdx.org/licenses/. " + "It can be a typo error. If not, you should replace it by 'free' " + "or 'non-free' and give some explanations in the README.md." % (license) + ) + + + @test() + def app_in_app_list(self): + + if self.manifest["id"] not in app_list(): + yield Error("This app is not Yunohost's application catalog") + + @test() + def app_in_app_list(self): + + all_urls = [infos.get("url", "").lower() for infos in app_list().values()] + + repo_org = "https://github.com/YunoHost-Apps/%s_ynh" % (self.manifest["id"]) + repo_brique = "https://github.com/labriqueinternet/%s_ynh" % (self.manifest["id"]) + + if repo_org.lower() in all_urls or repo_brique.lower() in all_urls: + return + + def is_in_github_org(): + return urlopen(repo_org)['code'] != 404 + def is_in_brique_org(): + return urlopen(repo_brique)['code'] != 404 + + if not is_in_github_org() and not is_in_brique_org(): + yield Error("Consider adding your app to the YunoHost-Apps organization to allow the community to contribute more easily") + + @test() + def description(self): + + descr = self.manifest.get("description", "") + id = self.manifest["id"].lower() + name = self.manifest["name"].lower() + + if isinstance(descr, dict): + descr = descr.get("en", "") + + if len(descr) < 5 or len(descr) > 150: + yield Warning("The description of your app is either missing, too short or too long... Please describe in *consise* terms what the app is / does.") + + if "for yunohost" in descr.lower(): + yield Warning( + "The 'description' should explain what the app actually does. " + "No need to say that it is 'for YunoHost' - this is a YunoHost app " + "so of course we know it is for YunoHost ;-)." + ) + if descr.lower().startswith(id) or descr.lower().startswith(name): + yield Warning( + "Try to avoid starting the description by '$app is' " + "... explain what the app is / does directly !" + ) + + @test() + def version_format(self): + + if self.manifest["version"][-5:-1] != "~ynh": + yield Warning( + "The 'version' field should match the format ~ynh. " + "For example : 4.3-2~ynh3. It is composed of the upstream version number (in the " + "example, 4.3-2) and an incremental number for each change in the package without " + "upstream change (in the example, 3). This incremental number can be reset to 1 " + "each time the upstream version changes." + ) + + @test() + def multiinstance_format(self): + + if self.manifest["multi_instance"] not in [True, False, 0, 1]: + yield Error("\"multi_instance\" field should be boolean 'true' or 'false' and not string type") + + @test() + def url(self): + if self.manifest["url"].endswith("_ynh"): + yield Warning( "'url' is not meant to be the url of the yunohost package, " "but rather the website or repo of the upstream app itself..." ) - self.yunohost_version_req = manifest.get("requirements", {}).get("yunohost", None) + @test() + def install_args(self): + + recognized_types = ("domain", "path", "boolean", "app", "password", "user", "string", "display_text") + + for argument in self.manifest["arguments"].get("install", []): + if not isinstance(argument.get("optional", False), bool): + yield Warning( + "The key 'optional' value for setting %s should be a boolean (true or false)" % argument["name"] + ) + if "type" not in argument.keys(): + yield Warning( + "You should specify the type of the argument '%s'. " + "You can use : %s." % (argument["name"], ', '.join(recognized_types)) + ) + elif argument["type"] not in recognized_types: + yield Warning( + "The type '%s' for argument '%s' is not recognized... " + "it probably doesn't behave as you expect ? Choose among those instead : %s" % (argument["type"], argument["name"], ', '.join(recognized_types)) + ) + + if "choices" in argument.keys(): + choices = [c.lower() for c in argument["choices"]] + if len(choices) == 2: + if ("true" in choices and "false" in choices) or ("yes" in choices and "no" in choices): + yield Warning( + "Argument %s : you might want to simply use a boolean-type argument. " + "No need to specify the choices list yourself." % argument["name"] + ) + + @test() + def is_public_help(self): + for argument in self.manifest["arguments"].get("install", []): + if argument["name"] == "is_public" and "help" not in argument.keys(): + yield Info( + "Consider adding an 'help' key for argument 'is_public' " + "to explain to the user what it means for *this* app " + "to be public or private :\n" + ' "help": {\n' + ' "en": "Some explanation"\n' + ' }') -class Script(): +################################## +# _____ _ _ # +# / ____| (_) | | # +# | (___ ___ _ __ _ _ __ | |_ # +# \___ \ / __| '__| | '_ \| __| # +# ____) | (__| | | | |_) | |_ # +# |_____/ \___|_| |_| .__/ \__| # +# | | # +# |_| # +################################## +class Script(TestSuite): def __init__(self, app_path, name): self.name = name @@ -744,7 +835,7 @@ class Script(): if not some_parsing_failed: _print("Some lines could not be parsed in script %s. (That's probably not really critical)" % self.name) some_parsing_failed = True - print_warning_not_reliable("%s : %s" % (e, line)) + report_warning_not_reliable("%s : %s" % (e, line)) def contains(self, command): """ @@ -764,161 +855,179 @@ class Script(): return any(re.search(regex, line) for line in [' '.join(line) for line in self.lines]) - def analyze(self): - print_header(self.name.upper() + " SCRIPT") + @test() + def error_handling(self): - self.check_set_usage() - self.check_helper_usage_dependencies() - self.check_deprecated_practices() - - def check_set_usage(self): - - if self.name == "_common.sh": - return - - present = False - - if self.name in ["backup", "remove"]: + if self.name in ["backup", "remove", "_common.sh"]: present = self.contains("ynh_abort_if_errors") or self.contains("set -eu") else: present = self.contains("ynh_abort_if_errors") - if self.name == "remove": - # Remove script shouldn't use set -eu or ynh_abort_if_errors + if self.name in ["remove", "_common.sh"]: if present: - print_error( - "[YEP-2.4] set -eu or ynh_abort_if_errors is present. " - "If there is a crash, it could put yunohost system in " - "a broken state. For details, look at " - "https://github.com/YunoHost/issues/issues/419" + yield Error( + "Do not use set -eu or ynh_abort_if_errors in the remove or _common.sh: " + "If a single instruction fails, it will stop the script and is " + "likely to leave the system in a broken state." ) elif not present: - print_error( - "[YEP-2.4] ynh_abort_if_errors is missing. For details, " - "look at https://github.com/YunoHost/issues/issues/419" + yield Error( + "You should add 'ynh_abort_if_errors' in this script, " + "c.f. https://github.com/YunoHost/issues/issues/419" ) - def check_helper_usage_dependencies(self): - """ - Detect usage of ynh_package_* & apt-get * - and suggest herlpers ynh_install_app_dependencies and ynh_remove_app_dependencies - """ + # Skip this in common.sh, sometimes custom not-yet-official helpers need this + @test(ignore=["_common.sh"]) + def raw_apt_commands(self): - # Skip this in common.sh, sometimes custom not-yet-official helpers need this - if self.name == "_common.sh": - return - - if self.contains("ynh_package_install") or self.contains("apt-get install"): - print_warning( + if self.contains("ynh_package_install") or self.contains("apt install") or self.contains("apt-get install"): + yield Warning( "You should not use `ynh_package_install` or `apt-get install`, " "use `ynh_install_app_dependencies` instead" ) - if self.contains("ynh_package_remove") or self.contains("apt-get remove"): - print_warning( + if self.contains("ynh_package_remove") or self.contains("apt remove") or self.contains("apt-get remove"): + yield Warning( "You should not use `ynh_package_remove` or `apt-get remove`, " "use `ynh_remove_app_dependencies` instead" ) - def check_deprecated_practices(self): - + @test() + def obsolete_helpers(self): if self.contains("yunohost app setting"): - print_error("Do not use 'yunohost app setting' directly. Please use 'ynh_app_setting_(set,get,delete)' instead.") + yield Error("Do not use 'yunohost app setting' directly. Please use 'ynh_app_setting_(set,get,delete)' instead.") if self.contains("yunohost app checkurl"): - print_error("'yunohost app checkurl' is obsolete!!! Please use 'ynh_webpath_register' instead.") + yield Error("'yunohost app checkurl' is obsolete!!! Please use 'ynh_webpath_register' instead.") if self.contains("yunohost app checkport"): - print_error("'yunohost app checkport' is obsolete!!! Please use 'ynh_find_port' instead.") + yield Error("'yunohost app checkport' is obsolete!!! Please use 'ynh_find_port' instead.") if self.contains("yunohost app initdb"): - print_error("'yunohost app initdb' is obsolete!!! Please use 'ynh_mysql_setup_db' instead.") + yield Error("'yunohost app initdb' is obsolete!!! Please use 'ynh_mysql_setup_db' instead.") + + @test() + def safe_rm(self): + if self.contains("rm -rf"): + yield Error("You should avoid using 'rm -rf', please use 'ynh_secure_remove' instead") + + @test() + def nginx_restart(self): + if self.contains("systemctl restart nginx") or self.contains("service nginx restart"): + yield Error( + "Restarting nginx is quite dangerous (especially for web installs) " + "and should be avoided at all cost. Use 'reload' instead." + ) + + @test(only=["install"]) + def argument_fetching(self): + + if self.containsregex(r"^\w+\=\$\{?[0-9]"): + yield Error( + "Do not fetch arguments from manifest using variable=$N (e.g." + " domain=$1 ...) Instead, use name=$YNH_APP_ARG_NAME" + ) + + + @test(only=["install"]) + def sources_list_tweaking(self): + if self.contains("/etc/apt/sources.list") \ + or (os.path.exists(self.app_path + "/scripts/_common.sh") and "/etc/apt/sources.list" in open(self.app_path+"/scripts/_common.sh").read() and "ynh_add_repo" not in open(self.app_path+"/scripts/_common.sh").read()): + yield Error( + "Manually messing with apt's sources.lists is strongly discouraged " + "and should be avoided. Please use ynh_install_extra_app_dependencies is you " + "need to install dependencies from a custom apt repo." + ) + + @test() + def exit_ynhdie(self): + if self.contains("exit"): - print_warning("'exit' command shouldn't be used. Please use 'ynh_die' instead.") + yield Warning("'exit' command shouldn't be used. Please use 'ynh_die' instead.") + @test() + def old_regenconf(self): if self.contains("yunohost service regen-conf"): - print_warning("'yunohost tools regen-conf' has been replaced by 'yunohost tools regen-conf'.") + yield Warning("'yunohost tools regen-conf' has been replaced by 'yunohost tools regen-conf'.") + @test() + def ssowatconf(self): # Dirty hack to check only the 10 last lines for ssowatconf # (the "bad" practice being using this at the very end of the script, but some apps legitimately need this in the middle of the script) oldlines = list(self.lines) self.lines = self.lines[-10:] if self.contains("yunohost app ssowatconf"): - print_warning("You probably don't need to run 'yunohost app ssowatconf' in the app script. It's supposed to be ran automatically after the script.") + yield Warning("You probably don't need to run 'yunohost app ssowatconf' in the app self. It's supposed to be ran automatically after the script.") self.lines = oldlines - if self.contains("rm -rf"): - print_error("[YEP-2.12] You should avoid using 'rm -rf', please use 'ynh_secure_remove' instead") - + @test() + def sed(self): if self.containsregex(r"sed\s+(-i|--in-place)\s+(-r\s+)?s") or self.containsregex(r"sed\s+s\S*\s+(-i|--in-place)"): - print_warning("[YEP-2.12] You should avoid using 'sed -i' for substitutions, please use 'ynh_replace_string' instead") + yield Warning("You should avoid using 'sed -i' for substitutions, please use 'ynh_replace_string' instead") + + @test() + def sudo(self): if self.containsregex(r"sudo \w"): # \w is here to not match sudo -u, legit use because ynh_exec_as not official yet... - print_warning( - "[YEP-2.12] You should not need to use 'sudo', the script is being run as root. " + yield Warning( + "You should not need to use 'sudo', the self is being run as root. " "(If you need to run a command using a specific user, use 'ynh_exec_as')" ) + @test() + def chmod(self): if self.containsregex(r"chmod .*777") or self.containsregex(r'chmod .*o\+w'): - print_warning( + yield Warning( "DO NOT use chmod 777 or chmod o+w that gives write permission to every users on the system !!! If you have permission issues, just make sure that the owner and/or group owner is right ..." ) + @test() + def random(self): if self.contains("dd if=/dev/urandom") or self.contains("openssl rand"): - print_warning( + yield Warning( "Instead of 'dd if=/dev/urandom' or 'openssl rand', " "you might want to use ynh_string_random" ) - if self.contains("systemctl restart nginx") or self.contains("service nginx restart"): - print_error( - "Restarting nginx is quite dangerous (especially for web installs) " - "and should be avoided at all cost. Use 'reload' instead." - ) - - if self.name == "install" and not self.contains("ynh_print_info") and not self.contains("ynh_script_progression"): - print_warning( + @test(only=["install"]) + def progression(self): + if not self.contains("ynh_print_info") and not self.contains("ynh_script_progression"): + yield Warning( "Please add a few messages for the user, to explain what is going on " "(in friendly, not-too-technical terms) during the installation. " "You can use 'ynh_print_info' or 'ynh_script_progression' for this." ) - if self.name == "install" and self.containsregex(r"^\w+\=\$\{?[0-9]"): - print_error( - "Do not fetch arguments from manifest using variable=$N (e.g." - " domain=$1 ...) Instead, use name=$YNH_APP_ARG_NAME" - ) + @test() + def progression_weight(self): - if self.name == "install": - if self.contains("/etc/apt/sources.list") \ - or (os.path.exists(self.app_path + "/scripts/_common.sh") and "/etc/apt/sources.list" in open(self.app_path+"/scripts/_common.sh").read() and "ynh_add_repo" not in open(self.app_path+"/scripts/_common.sh").read()): - print_error( - "[YEP-3.7] Manually messing with apt's sources.lists is strongly discouraged " - "and should be avoided. Please use ynh_install_extra_app_dependencies is you " - "need to install dependencies from a custom apt repo." - ) + # Usage of ynh_script_prorgression with --time or --weight=1 all over the place... + if self.containsregex(r"ynh_script_progression.*--time"): + yield Warning("Using ynh_script_progression --time should only be for calibrating the weight (c.f. --weight). It's not meant to be kept for production versions.") + if self.containsregex(r"ynh_script_progression.*--weight=1") \ + and not self.containsregex(r"ynh_script_progression.*--weight=([^1]|[1-9][0-9]+)"): + yield Warning("Having only '--weight=1' for ynh_script_progression is useless... Either calibrate the weights with --time once, or don't put any --weight at all.") - if self.name in ["install", "_common.sh"]: - if self.containsregex("dependencies.*php-"): - print_warning("You should avoid having dependencies like 'php-foobar'. Instead, specify the exact version you want like 'php7.0-foobar'. Otherwise, the *wrong* version of the dependency may be installed if sury is also installed. Note that for Stretch/Buster/Bullseye/... transition, Yunohost will automatically patch your file so there's no need to care about that.") - if self.name == "backup": - if self.containsregex("^ynh_systemd_action"): - print_warning("Unless you really have a good reason to do so, starting/stopping services during backup has no benefit and leads to unecessary service interruptions when creating backups... As a 'reminder': apart from possibly database dumps (which usually do not require the service to be stopped) or other super-specific action, running the backup script is only a *declaration* of what needs to be backuped. The real copy and archive creation happens *after* the backup script is ran.") + @test(only=["install", "_common.sh"]) + def php_deps(self): + if self.containsregex("dependencies.*php-"): + yield Warning("You should avoid having dependencies like 'php-foobar'. Instead, specify the exact version you want like 'php7.0-foobar'. Otherwise, the *wrong* version of the dependency may be installed if sury is also installed. Note that for Stretch/Buster/Bullseye/... transition, Yunohost will automatically patch your file so there's no need to care about that.") + @test(only=["backup"]) + def random(self): + if self.containsregex("^ynh_systemd_action"): + yield Warning("Unless you really have a good reason to do so, starting/stopping services during backup has no benefit and leads to unecessary service interruptions when creating backups... As a 'reminder': apart from possibly database dumps (which usually do not require the service to be stopped) or other super-specific action, running the backup script is only a *declaration* of what needs to be backuped. The real copy and archive creation happens *after* the backup script is ran.") + + @test() + def helpers_sourcing(self): helpers_after_official = subprocess.check_output("head -n 30 '%s' | grep -A 10 '^ *source */usr/share/yunohost/helpers' | grep '^ *source' | tail -n +2" % self.path, shell=True).decode("utf-8") helpers_after_official = helpers_after_official.replace("source", "").replace(" ", "").strip() if helpers_after_official: helpers_after_official = helpers_after_official.split("\n") - print_warning("Please avoid sourcing additional helpers after the official helpers (in this case file %s)" % ", ".join(helpers_after_official)) + yield Warning("Please avoid sourcing additional helpers after the official helpers (in this case file %s)" % ", ".join(helpers_after_official)) - if self.name in ["backup", "restore"]: - if self.contains("source _common.sh") or self.contains("source ./_common.sh"): - print_warning("In the context of backup and restore script, you should load _common.sh with \"source ../settings/scripts/_common.sh\"") - - # Usage of ynh_script_prorgression with --time or --weight=1 all over the place... - if self.containsregex(r"ynh_script_progression.*--time"): - print_warning("Using ynh_script_progression --time should only be for calibrating the weight (c.f. --weight). It's not meant to be kept for production versions.") - if self.containsregex(r"ynh_script_progression.*--weight=1") \ - and not self.containsregex(r"ynh_script_progression.*--weight=([^1]|[1-9][0-9]+)"): - print_warning("Having only '--weight=1' for ynh_script_progression is useless... Either calibrate the weights with --time once, or don't put any --weight at all.") + @test(only=["backup", "restore"]) + def helpers_sourcing(self): + if self.contains("source _common.sh") or self.contains("source ./_common.sh"): + yield Warning("In the context of backup and restore script, you should load _common.sh with \"source ../settings/scripts/_common.sh\"") def main(): @@ -935,9 +1044,13 @@ def main(): App(app_path).analyze() if output == "json": - print(json.dumps({"warnings": warnings, "errors": errors}, indent=4)) + print(json.dumps({"warnings": [test for test, report in tests_reports if isinstance(report, Warning)], + "errors": [test for test, report in tests_reports if isinstance(report, Error)]}, indent=4)) else: - if len(errors) > 0: + errors = [report for _, report in tests_reports if isinstance(report, Error)] + warnings = [report for _, report in tests_reports if isinstance(report, Warning)] + if errors: + print("Uhoh there are some errors to be fixed :(") sys.exit(1) elif len(warnings) > 3: print("Still some warnings to be fixed :s") From 961e0bce9f82c74d481b872b4cd803bee3f91b73 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Thu, 8 Oct 2020 15:01:36 +0200 Subject: [PATCH 02/15] Fix regressions --- package_linter.py | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/package_linter.py b/package_linter.py index 9994515..29a5a73 100755 --- a/package_linter.py +++ b/package_linter.py @@ -614,14 +614,20 @@ class Manifest(TestSuite): @test() def mandatory_fields(self): - fields = ("name", "id", "packaging_format", "description", "url", "version", - "license", "maintainer", "requirements", "multi_instance", + fields = ("name", "id", "packaging_format", "description", "version", + "maintainer", "requirements", "multi_instance", "services", "arguments") missing_fields = [field for field in fields if field not in self.manifest.keys()] if missing_fields: yield Error("The following mandatory fields are missing: %s" % missing_fields) + fields = ("license", "url") + missing_fields = [field for field in fields if field not in self.manifest.keys()] + + if missing_fields: + yield Warning("The following mandatory fields are missing: %s" % missing_fields) + @test() def yunohost_version_requirement(self): @@ -645,7 +651,7 @@ class Manifest(TestSuite): return def license_mentionned_in_readme(): - readme_path = os.path.join(app.path, 'README.md') + readme_path = os.path.join(self.path, 'README.md') if os.path.isfile(readme_path): return "LICENSE" in open(readme_path).read() return False @@ -674,13 +680,13 @@ class Manifest(TestSuite): @test() - def app_in_app_list(self): + def app_in_app_catalog(self): if self.manifest["id"] not in app_list(): - yield Error("This app is not Yunohost's application catalog") + yield Warning("This app is not Yunohost's application catalog") @test() - def app_in_app_list(self): + def app_in_github_org(self): all_urls = [infos.get("url", "").lower() for infos in app_list().values()] @@ -696,7 +702,7 @@ class Manifest(TestSuite): return urlopen(repo_brique)['code'] != 404 if not is_in_github_org() and not is_in_brique_org(): - yield Error("Consider adding your app to the YunoHost-Apps organization to allow the community to contribute more easily") + yield Warning("Consider adding your app to the YunoHost-Apps organization to allow the community to contribute more easily") @test() def description(self): @@ -743,7 +749,7 @@ class Manifest(TestSuite): @test() def url(self): - if self.manifest["url"].endswith("_ynh"): + if self.manifest.get("url", "").endswith("_ynh"): yield Warning( "'url' is not meant to be the url of the yunohost package, " "but rather the website or repo of the upstream app itself..." From 25e6cc19629fe2597702939923b766e020187dc5 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Thu, 8 Oct 2020 16:03:52 +0200 Subject: [PATCH 03/15] Moar consistency checks --- package_linter.py | 58 ++++++++++++++++++++++++----------------------- 1 file changed, 30 insertions(+), 28 deletions(-) diff --git a/package_linter.py b/package_linter.py index 29a5a73..fa682ec 100755 --- a/package_linter.py +++ b/package_linter.py @@ -383,44 +383,46 @@ class App(TestSuite): @test() - def helper_consistency(app): + def helper_consistency_apt_deps(app): """ check if ynh_install_app_dependencies is present in install/upgrade/restore so dependencies are up to date after restoration or upgrade """ install_script = app.scripts["install"] - if install_script.exists: - if install_script.contains("ynh_install_app_dependencies"): - for name in ["upgrade", "restore"]: - if app.scripts[name].exists and not app.scripts[name].contains("ynh_install_app_dependencies"): - yield Warning("ynh_install_app_dependencies should also be in %s script" % name) + if install_script.contains("ynh_install_app_dependencies"): + for name in ["upgrade", "restore"]: + if app.scripts[name].exists and not app.scripts[name].contains("ynh_install_app_dependencies"): + yield Warning("ynh_install_app_dependencies should also be in %s script" % name) - if install_script.contains("yunohost service add"): - if app.scripts["remove"].exists and not app.scripts["remove"].contains("yunohost service remove"): - yield Error( - "You used 'yunohost service add' in the install script, " - "but not 'yunohost service remove' in the remove script." - ) -# -# if "services" in manifest and app.scripts["install"].exists: -# -# known_services = ("nginx", "mysql", "uwsgi", "metronome", -# "php5-fpm", "php7.0-fpm", "php-fpm", -# "postfix", "dovecot", "rspamd") -# -# for service in manifest["services"]: -# if service not in known_services: -# if service == 'postgresql': -# if not app.scripts["install"].contains('ynh_psql_test_if_first_run')\ -# or not app.scripts["restore"].contains('ynh_psql_test_if_first_run'): -# report_error("[YEP-2.1?] postgresql service present in the manifest, install and restore scripts must call ynh_psql_test_if_first_run") -# elif not app.scripts["install"].contains("yunohost service add %s" % service): -# report_error("[YEP-2.1?] " + service + " service not installed by the install file but present in the manifest") -# + @test() + def helper_consistency_service_add(app): + install_script = app.scripts["install"] + if install_script.contains("yunohost service add"): + if app.scripts["remove"].exists and not app.scripts["remove"].contains("yunohost service remove"): + yield Error( + "You used 'yunohost service add' in the install script, " + "but not 'yunohost service remove' in the remove script." + ) + if app.scripts["upgrade"].exists and not app.scripts["upgrade"].contains("yunohost service add"): + yield Warning( + "You used 'yunohost service add' in the install script, " + "but not in the upgrade script" + ) + if app.scripts["restore"].exists and not app.scripts["restore"].contains("yunohost service add"): + yield Warning( + "You used 'yunohost service add' in the install script, " + "but not in the restore script" + ) + @test() + def helper_consistency_firewall(app): + install_script = app.scripts["install"] + if install_script.contains("yunohost firewall allow"): + if not install_script.contains("--needs_exposed_ports"): + yield Warning("The install script expose a port on the outside with 'yunohost firewall allow' but doesn't use 'yunohost service add' with --needs_exposed_ports ... If your are ABSOLUTELY SURE that the service needs to be exposed on THE OUTSIDE, then add --needs_exposed_ports to 'yunohost service add' with the relevant port number. Otherwise, opening the port leads to a significant security risk and you should keep the damn port closed !") ########################################################### From 79c52b28c7e01f7fc1a96a8fabff44ed1bddedc8 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Thu, 8 Oct 2020 16:04:17 +0200 Subject: [PATCH 04/15] Enforcing some stuff as errors because most level 7 apps pass these --- package_linter.py | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/package_linter.py b/package_linter.py index fa682ec..ae0b1d2 100755 --- a/package_linter.py +++ b/package_linter.py @@ -451,7 +451,7 @@ class App(TestSuite): def misc_legacy_phpini(app): if file_exists(app.path + "/conf/php-fpm.ini"): - yield Warning( + yield Error( "Using a separate php-fpm.ini file is deprecated. " "Please merge your php-fpm directives directly in the pool file. " "(c.f. https://github.com/YunoHost-Apps/nextcloud_ynh/issues/138 )" @@ -464,7 +464,7 @@ class App(TestSuite): source_dir = os.path.join(app.path, "sources") if os.path.exists(source_dir) \ and len([name for name in os.listdir(source_dir) if os.path.isfile(os.path.join(source_dir, name))]) > 5: - yield Warning( + yield Error( "Upstream app sources shouldn't be stored in this 'sources' folder of this git repository as a copy/paste\n" "During installation, the package should download sources from upstream via 'ynh_setup_source'.\n" "See the helper documentation. " @@ -489,7 +489,7 @@ class App(TestSuite): content = open(app.path + "/conf/" + filename).read() if "location" in content and "add_header" in content: - yield Warning( + yield Error( "Do not use 'add_header' in the nginx conf. Use 'more_set_headers' instead. " "(See https://www.peterbe.com/plog/be-very-careful-with-your-add_header-in-nginx " "and https://github.com/openresty/headers-more-nginx-module#more_set_headers )" @@ -569,7 +569,7 @@ class App(TestSuite): nginxconf = [] for location in find_path_traversal_issue(nginxconf): - yield Warning( + yield Error( "The nginx configuration (especially location %s) " "appears vulnerable to path traversal issues as explained in\n" " https://www.acunetix.com/vulnerabilities/web/path-traversal-via-misconfigured-nginx-alias/\n" @@ -735,7 +735,7 @@ class Manifest(TestSuite): def version_format(self): if self.manifest["version"][-5:-1] != "~ynh": - yield Warning( + yield Error( "The 'version' field should match the format ~ynh. " "For example : 4.3-2~ynh3. It is composed of the upstream version number (in the " "example, 4.3-2) and an incremental number for each change in the package without " @@ -980,7 +980,7 @@ class Script(TestSuite): ) @test() - def chmod(self): + def chmod777(self): if self.containsregex(r"chmod .*777") or self.containsregex(r'chmod .*o\+w'): yield Warning( "DO NOT use chmod 777 or chmod o+w that gives write permission to every users on the system !!! If you have permission issues, just make sure that the owner and/or group owner is right ..." @@ -989,9 +989,8 @@ class Script(TestSuite): @test() def random(self): if self.contains("dd if=/dev/urandom") or self.contains("openssl rand"): - yield Warning( - "Instead of 'dd if=/dev/urandom' or 'openssl rand', " - "you might want to use ynh_string_random" + yield Error( + "Instead of 'dd if=/dev/urandom' or 'openssl rand', you should use ynh_string_random" ) @test(only=["install"]) @@ -1020,12 +1019,12 @@ class Script(TestSuite): yield Warning("You should avoid having dependencies like 'php-foobar'. Instead, specify the exact version you want like 'php7.0-foobar'. Otherwise, the *wrong* version of the dependency may be installed if sury is also installed. Note that for Stretch/Buster/Bullseye/... transition, Yunohost will automatically patch your file so there's no need to care about that.") @test(only=["backup"]) - def random(self): + def systemd_during_backup(self): if self.containsregex("^ynh_systemd_action"): yield Warning("Unless you really have a good reason to do so, starting/stopping services during backup has no benefit and leads to unecessary service interruptions when creating backups... As a 'reminder': apart from possibly database dumps (which usually do not require the service to be stopped) or other super-specific action, running the backup script is only a *declaration* of what needs to be backuped. The real copy and archive creation happens *after* the backup script is ran.") @test() - def helpers_sourcing(self): + def helpers_sourcing_after_official(self): helpers_after_official = subprocess.check_output("head -n 30 '%s' | grep -A 10 '^ *source */usr/share/yunohost/helpers' | grep '^ *source' | tail -n +2" % self.path, shell=True).decode("utf-8") helpers_after_official = helpers_after_official.replace("source", "").replace(" ", "").strip() if helpers_after_official: @@ -1033,7 +1032,7 @@ class Script(TestSuite): yield Warning("Please avoid sourcing additional helpers after the official helpers (in this case file %s)" % ", ".join(helpers_after_official)) @test(only=["backup", "restore"]) - def helpers_sourcing(self): + def helpers_sourcing_backuprestore(self): if self.contains("source _common.sh") or self.contains("source ./_common.sh"): yield Warning("In the context of backup and restore script, you should load _common.sh with \"source ../settings/scripts/_common.sh\"") From 01adf07c5ea3254078d95b2ab4311f47a2b6c3ce Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Thu, 8 Oct 2020 16:06:19 +0200 Subject: [PATCH 05/15] Fix an edge case of path traversal detection --- package_linter.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/package_linter.py b/package_linter.py index ae0b1d2..d1ef925 100755 --- a/package_linter.py +++ b/package_linter.py @@ -526,6 +526,9 @@ class App(TestSuite): for block in nginxconf: for location, alias in find_location_with_alias(block): + # Ignore locations which are regexes..? + if location.startswith("^") and location.endswith("$"): + continue alias_path = alias[-1] # For path traversal issues to occur, both of those are needed : # - location /foo { (*without* a / after foo) From cd396af50ee481ce79b57ad6b8ab427e47298342 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Thu, 8 Oct 2020 16:40:04 +0200 Subject: [PATCH 06/15] Fix some false-negative about exit usage + report it as error --- package_linter.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/package_linter.py b/package_linter.py index d1ef925..0cbe5a7 100755 --- a/package_linter.py +++ b/package_linter.py @@ -951,8 +951,8 @@ class Script(TestSuite): @test() def exit_ynhdie(self): - if self.contains("exit"): - yield Warning("'exit' command shouldn't be used. Please use 'ynh_die' instead.") + if self.contains(r"\bexit\b"): + yield Error("'exit' command shouldn't be used. Please use 'ynh_die' instead.") @test() def old_regenconf(self): From a374b997e05af9a69a659f166d3ce12faaad3cbb Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Thu, 15 Oct 2020 21:29:17 +0200 Subject: [PATCH 07/15] Encourage people to implement a change_url script --- package_linter.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/package_linter.py b/package_linter.py index 0cbe5a7..a10ff39 100755 --- a/package_linter.py +++ b/package_linter.py @@ -326,6 +326,7 @@ class App(TestSuite): print_header("MISC HELPER USAGE / CONSISTENCY") self.run_tests() + ####################################### # _ _ _ # # | | | | | | # @@ -436,7 +437,7 @@ class App(TestSuite): ########################################################### @test() - def misc_files(app): + def mandatory_scripts(app): filenames = ("manifest.json", "LICENSE", "README.md", "scripts/install", "scripts/remove", "scripts/upgrade", @@ -446,6 +447,12 @@ class App(TestSuite): if not file_exists(app.path + "/" + filename): yield Error("Providing %s is mandatory" % filename) + @test() + def change_url_script(app): + + if not file_exists(app.path + "/scripts/change_url"): + yield Warning("Consider adding a change_url script to support changing where the app is installed") + @test() def misc_legacy_phpini(app): From 2e4b25e0086e9c47384f760d4a447a5817c20ddf Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Thu, 15 Oct 2020 21:38:14 +0200 Subject: [PATCH 08/15] Add check_process checks --- package_linter.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/package_linter.py b/package_linter.py index a10ff39..53f0222 100755 --- a/package_linter.py +++ b/package_linter.py @@ -453,6 +453,26 @@ class App(TestSuite): if not file_exists(app.path + "/scripts/change_url"): yield Warning("Consider adding a change_url script to support changing where the app is installed") + @test() + def check_process_exists(app): + + check_process_file = app.path + "/check_process" + + if not file_exists(check_process_file): + yield Warning("You should add a 'check_process' file to properly interface with the continuous integration system") + + @test() + def check_process_syntax(app): + + check_process_file = app.path + "/check_process" + if not file_exists(check_process_file): + return + + if os.system("grep -q 'Level 5=1' %s" % check_process_file) == 0: + yield Error("Do not force Level 5=1 in check_process...") + + if os.system("grep -q ' *Level [^5]=' %s" % check_process_file) == 0: + yield Warning("Setting Level x=y in check_process is obsolete / not relevant anymore") @test() def misc_legacy_phpini(app): From 43523543e03c6fcf0ae29df4a534b4df5f80a5d3 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Tue, 3 Nov 2020 17:45:17 +0100 Subject: [PATCH 09/15] Add a check that yunohost required version is not 2.x --- package_linter.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/package_linter.py b/package_linter.py index 53f0222..1bccad1 100755 --- a/package_linter.py +++ b/package_linter.py @@ -666,6 +666,13 @@ class Manifest(TestSuite): if not self.manifest.get("requirements", {}).get("yunohost", ""): yield Error("You should add a yunohost version requirement in the manifest") + @test() + def yunohost_version_requirement_superold(app): + + yunohost_version_req = app.manifest.get("requirements", {}).get("yunohost", "").strip(">= ") + if yunohost_version_req.startswith("2."): + yield Error("Your app only requires yunohost >= 2.x, which tends to indicate that your app may not be up to date with recommended packaging practices and helpers.") + @test() def basic_fields_format(self): From 7d5b87ef84b2698f1d154b0027b865db785b4892 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Tue, 3 Nov 2020 19:03:01 +0100 Subject: [PATCH 10/15] Add some consistency check for content of check_process compared to manifest --- package_linter.py | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/package_linter.py b/package_linter.py index 1bccad1..886b065 100755 --- a/package_linter.py +++ b/package_linter.py @@ -474,6 +474,35 @@ class App(TestSuite): if os.system("grep -q ' *Level [^5]=' %s" % check_process_file) == 0: yield Warning("Setting Level x=y in check_process is obsolete / not relevant anymore") + @test() + def check_process_consistency(app): + + check_process_file = app.path + "/check_process" + if not file_exists(check_process_file): + return + + has_is_public_arg = any(a["name"] == "is_public" for a in app.manifest["arguments"].get("install", [])) + if has_is_public_arg: + if os.system(r"grep -q '^\s*setup_public=1' %s" % check_process_file) != 0: + yield Warning("It looks like you forgot to enable setup_public test in check_process ?") + + if os.system(r"grep -q '^\s*setup_private=1' %s" % check_process_file) != 0: + yield Warning("It looks like you forgot to enable setup_private test in check_process ?") + + has_path_arg = any(a["name"] == "path" for a in app.manifest["arguments"].get("install", [])) + if has_path_arg: + if os.system(r"grep -q '^\s*setup_sub_dir=1' %s" % check_process_file) != 0: + yield Warning("It looks like you forgot to enable setup_sub_dir test in check_process ?") + + if app.manifest.get("multi_instance") in [True, 1, "True", "true"]: + if os.system(r"grep -q '^\s*multi_instance=1' %s" % check_process_file) != 0: + yield Warning("It looks like you forgot to enable multi_instance test in check_process ?") + + if app.scripts["backup"].exists: + if os.system(r"grep -q '^\s*backup_restore=1' %s" % check_process_file) != 0: + yield Warning("It looks like you forgot to enable backup_restore test in check_process ?") + + @test() def misc_legacy_phpini(app): From 94e81683ab68c716f2688b1cfd7015bdd146fbc5 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 4 Nov 2020 14:06:17 +0100 Subject: [PATCH 11/15] change_url script is only relevant if this is a webapp --- package_linter.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/package_linter.py b/package_linter.py index 886b065..3374845 100755 --- a/package_linter.py +++ b/package_linter.py @@ -450,7 +450,8 @@ class App(TestSuite): @test() def change_url_script(app): - if not file_exists(app.path + "/scripts/change_url"): + has_domain_arg = any(a["name"] == "is_public" for a in app.manifest["arguments"].get("install", [])) + if has_domain_arg and not file_exists(app.path + "/scripts/change_url"): yield Warning("Consider adding a change_url script to support changing where the app is installed") @test() From 06b89530334342bfad6e63a90589707ae0d8fefb Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 4 Nov 2020 15:51:24 +0100 Subject: [PATCH 12/15] Tweak checks about progress bar weights, check that the weights don't have super huge differences --- package_linter.py | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/package_linter.py b/package_linter.py index 3374845..5ecd51f 100755 --- a/package_linter.py +++ b/package_linter.py @@ -10,6 +10,7 @@ import urllib.request import codecs import subprocess import time +import statistics reader = codecs.getreader("utf-8") @@ -1070,15 +1071,33 @@ class Script(TestSuite): ) @test() - def progression_weight(self): + def progression_time(self): # Usage of ynh_script_prorgression with --time or --weight=1 all over the place... if self.containsregex(r"ynh_script_progression.*--time"): yield Warning("Using ynh_script_progression --time should only be for calibrating the weight (c.f. --weight). It's not meant to be kept for production versions.") - if self.containsregex(r"ynh_script_progression.*--weight=1") \ - and not self.containsregex(r"ynh_script_progression.*--weight=([^1]|[1-9][0-9]+)"): - yield Warning("Having only '--weight=1' for ynh_script_progression is useless... Either calibrate the weights with --time once, or don't put any --weight at all.") + @test(ignore=["_common.sh", "backup"]) + def progression_meaningful_weights(self): + + def weight(line): + match = re.search(r"ynh_script_progression.*--weight=([0-9]+)", ' '.join(line)) + if match: + try: + return int(match.groups()[0]) + except: + return -1 + else: + return 1 + + script_progress = [line for line in self.lines if "ynh_script_progression" in line] + weights = [weight(line) for line in script_progress] + + if not weights: + return + + if len(weights) > 3 and statistics.stdev(weights) > 50: + yield Warning("To have a meaningful progress bar, try to keep the weights in the same range of values, for example [1,10], or [10,100] ... otherwise, if you have super-huge weight differentes, the progress bar rendering will be completely dominated by one or two steps... If these steps are really long, just try to indicated in the message that this will take a while.") @test(only=["install", "_common.sh"]) def php_deps(self): From 603d663c8d39fbde7f84ad534e65722253252890 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Sat, 7 Nov 2020 17:40:45 +0100 Subject: [PATCH 13/15] Add more tests related to app catalog infos --- package_linter.py | 62 +++++++++++++++++++++++++++++++++++------------ 1 file changed, 47 insertions(+), 15 deletions(-) diff --git a/package_linter.py b/package_linter.py index 5ecd51f..95160dd 100755 --- a/package_linter.py +++ b/package_linter.py @@ -673,6 +673,8 @@ class Manifest(TestSuite): except Exception as e: raise Exception("Looks like there's a syntax issue in your json ? %s" % e) + self.catalog_infos = app_list().get(self.manifest.get("id"), {}) + @test() def mandatory_fields(self): @@ -730,7 +732,7 @@ class Manifest(TestSuite): if "nonfree" in license.replace("-", ""): - yield Warning("'non-free' apps can't be officialized.") + yield Warning("'non-free' apps cannot be integrated in Yunohost's app catalog.") code_license = '' + license + '' @@ -748,31 +750,61 @@ class Manifest(TestSuite): "or 'non-free' and give some explanations in the README.md." % (license) ) + @test() + def app_catalog(self): + + if not self.catalog_infos: + yield Warning("This app is not Yunohost's application catalog") @test() - def app_in_app_catalog(self): + def app_catalog_revision(self): - if self.manifest["id"] not in app_list(): - yield Warning("This app is not Yunohost's application catalog") + if self.catalog_infos and self.catalog_infos.get("revision", "HEAD") != "HEAD": + yield Error("You should make sure that the revision used in Yunohost's apps catalog is HEAD ...") + + @test() + def app_catalog_state(self): + + if self.catalog_infos and self.catalog_infos.get("state", "working") != "working": + yield Warning("The application is not flagged as working in Yunohost's apps catalog") + + @test() + def app_catalog_maintained(self): + + if self.catalog_infos and self.catalog_infos.get("maintained", True) is not True: + yield Warning("The application is flagged as not maintained in Yunohost's apps catalog") + + @test() + def app_catalog_category(self): + if self.catalog_infos and not self.catalog_infos.get("category"): + yield Warning("The application has no associated category in Yunohost's apps catalog") @test() def app_in_github_org(self): - all_urls = [infos.get("url", "").lower() for infos in app_list().values()] - repo_org = "https://github.com/YunoHost-Apps/%s_ynh" % (self.manifest["id"]) repo_brique = "https://github.com/labriqueinternet/%s_ynh" % (self.manifest["id"]) - if repo_org.lower() in all_urls or repo_brique.lower() in all_urls: - return + if self.catalog_infos: + repo_url = self.catalog_infos["url"] - def is_in_github_org(): - return urlopen(repo_org)['code'] != 404 - def is_in_brique_org(): - return urlopen(repo_brique)['code'] != 404 + all_urls = [infos.get("url", "").lower() for infos in app_list().values()] - if not is_in_github_org() and not is_in_brique_org(): - yield Warning("Consider adding your app to the YunoHost-Apps organization to allow the community to contribute more easily") + if repo_url.lower() not in [repo_org.lower(), repo_brique.lower()]: + if repo_url.lower().startswith("https://github.com/YunoHost-Apps/"): + yield Warning("The url for this app in the catalog should be ") + else: + yield Warning("Consider adding your app to the YunoHost-Apps organization to allow the community to contribute more easily") + + else: + def is_in_github_org(): + return urlopen(repo_org)['code'] != 404 + + def is_in_brique_org(): + return urlopen(repo_brique)['code'] != 404 + + if not is_in_github_org() and not is_in_brique_org(): + yield Warning("Consider adding your app to the YunoHost-Apps organization to allow the community to contribute more easily") @test() def description(self): @@ -802,7 +834,7 @@ class Manifest(TestSuite): @test() def version_format(self): - if self.manifest["version"][-5:-1] != "~ynh": + if self.manifest.get("version", "")[-5:-1] != "~ynh": yield Error( "The 'version' field should match the format ~ynh. " "For example : 4.3-2~ynh3. It is composed of the upstream version number (in the " From e15a959860dc54c252638991a7ce4f9997252663 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Sat, 7 Nov 2020 17:41:40 +0100 Subject: [PATCH 14/15] No more 'app' type --- package_linter.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package_linter.py b/package_linter.py index 95160dd..dee8926 100755 --- a/package_linter.py +++ b/package_linter.py @@ -860,7 +860,7 @@ class Manifest(TestSuite): @test() def install_args(self): - recognized_types = ("domain", "path", "boolean", "app", "password", "user", "string", "display_text") + recognized_types = ("domain", "path", "boolean", "password", "user", "string", "display_text") for argument in self.manifest["arguments"].get("install", []): if not isinstance(argument.get("optional", False), bool): From c4e823062fbd5e56de8877aed8b47ad164fdf4b1 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Sun, 8 Nov 2020 15:41:53 +0100 Subject: [PATCH 15/15] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Éric Gaspar <46165813+ericgaspar@users.noreply.github.com> --- package_linter.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/package_linter.py b/package_linter.py index dee8926..add83ce 100755 --- a/package_linter.py +++ b/package_linter.py @@ -732,7 +732,7 @@ class Manifest(TestSuite): if "nonfree" in license.replace("-", ""): - yield Warning("'non-free' apps cannot be integrated in Yunohost's app catalog.") + yield Warning("'non-free' apps cannot be integrated in YunoHost's app catalog.") code_license = '' + license + '' @@ -754,30 +754,30 @@ class Manifest(TestSuite): def app_catalog(self): if not self.catalog_infos: - yield Warning("This app is not Yunohost's application catalog") + yield Warning("This app is not in YunoHost's application catalog") @test() def app_catalog_revision(self): if self.catalog_infos and self.catalog_infos.get("revision", "HEAD") != "HEAD": - yield Error("You should make sure that the revision used in Yunohost's apps catalog is HEAD ...") + yield Error("You should make sure that the revision used in YunoHost's apps catalog is HEAD...") @test() def app_catalog_state(self): if self.catalog_infos and self.catalog_infos.get("state", "working") != "working": - yield Warning("The application is not flagged as working in Yunohost's apps catalog") + yield Warning("The application is not flagged as working in YunoHost's apps catalog") @test() def app_catalog_maintained(self): if self.catalog_infos and self.catalog_infos.get("maintained", True) is not True: - yield Warning("The application is flagged as not maintained in Yunohost's apps catalog") + yield Warning("The application is flagged as not maintained in YunoHost's apps catalog") @test() def app_catalog_category(self): if self.catalog_infos and not self.catalog_infos.get("category"): - yield Warning("The application has no associated category in Yunohost's apps catalog") + yield Warning("The application has no associated category in YunoHost's apps catalog") @test() def app_in_github_org(self):