From 02ba265abdced576fed68fa6dd6223dc33caa8a4 Mon Sep 17 00:00:00 2001 From: Mickael Date: Wed, 27 Feb 2019 13:47:26 +0100 Subject: [PATCH 01/10] add check service in install file match with manifest (new services) --- package_linter.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/package_linter.py b/package_linter.py index 2883784..571a642 100755 --- a/package_linter.py +++ b/package_linter.py @@ -315,15 +315,16 @@ class App(): print_error( "[YEP-2.1] \"multi_instance\" field must be boolean type values 'true' or 'false' and not string type") - if "services" in manifest: - services = ("nginx", "mysql", "uwsgi", "metronome", - "php5-fpm", "php7.0-fpm", "php-fpm", - "postfix", "dovecot", "rspamd") + 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 services: - # FIXME : wtf is it supposed to mean ... - print_warning("[YEP-2.1] " + service + " service may not exist") + if service not in known_services: + if 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"]: @@ -379,6 +380,7 @@ class Script(): lines = '\n'.join(lines).replace("\\\n", "").split("\n") for line in lines: + try: line = shlex.split(line, True) yield line @@ -392,7 +394,7 @@ class Script(): For instance, "app setting" is contained in "yunohost app setting $app ..." """ return any(command in line - for line in [ ' '.join(line) for line in self.lines]) + for line in [' '.join(line) for line in self.lines]) def analyze(self): From 9f32e06dcc102ffce8c2b5ba8a7d31e3417d92c3 Mon Sep 17 00:00:00 2001 From: Mickael Date: Mon, 25 Feb 2019 23:24:28 +0100 Subject: [PATCH 02/10] add test of common file --- package_linter.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/package_linter.py b/package_linter.py index 2883784..88ad7bc 100755 --- a/package_linter.py +++ b/package_linter.py @@ -402,6 +402,7 @@ class Script(): self.check_set_usage() self.check_helper_usage_dependencies() self.check_deprecated_practices() + self.check_source_common() def check_verifications_done_before_modifying_system(self): """ @@ -518,6 +519,26 @@ class Script(): ) + def check_source_common(self): + #test for standard execution + if script["name"] in ["install","upgrade","remove"]: + count_common=len(re.findall("^source _common.sh",script["raw"],flags=re.MULTILINE)) + if count_common == 0: + print_warning("Calling _common.sh seams not present in this script, please add \"source _common.sh\"") + elif count_common > 1: + print_warning("Duplicates calls to _common.sh, please clean your code") + + #test for "save" files + else: + count_common=len(re.findall("^source _common.sh",script["raw"],flags=re.MULTILINE)) + count_common_save=len(re.findall("^source ../settings/scripts/_common.sh",script["raw"],flags=re.MULTILINE)) + if count_common > 0 and count_common_save == 0: + print_error("You must call _common.sh with \"source ../settings/scripts/_common.sh\" in this script to respect context execution") + elif count_common == 0 and count_common_save == 0: + print_warning("Calling _common.sh seams not present in this script, please add \"source ../settings/scripts/_common.sh\" in this script to respect context execution") + elif count_common_save > 1: + print_warning("Duplicates calls to _common.sh, please clean your code") + def main(): if len(sys.argv) != 2: print("Give one app package path.") @@ -526,6 +547,7 @@ def main(): app_path = sys.argv[1] header(app_path) App(app_path).analyze() + sys.exit(return_code) From 46e6439233aa21a77bef02a3eeb5e2175eb1e239 Mon Sep 17 00:00:00 2001 From: Mickael Date: Mon, 25 Feb 2019 23:53:46 +0100 Subject: [PATCH 03/10] add check ./common.sh cf kay0u ;-) --- package_linter.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/package_linter.py b/package_linter.py index 88ad7bc..cc57581 100755 --- a/package_linter.py +++ b/package_linter.py @@ -518,11 +518,10 @@ class Script(): "You can use 'ynh_print_info' or 'ynh_script_progression' for this." ) - def check_source_common(self): #test for standard execution if script["name"] in ["install","upgrade","remove"]: - count_common=len(re.findall("^source _common.sh",script["raw"],flags=re.MULTILINE)) + count_common=len(re.findall("^source _common.sh|^source ./_common.sh",script["raw"],flags=re.MULTILINE)) if count_common == 0: print_warning("Calling _common.sh seams not present in this script, please add \"source _common.sh\"") elif count_common > 1: @@ -530,7 +529,7 @@ class Script(): #test for "save" files else: - count_common=len(re.findall("^source _common.sh",script["raw"],flags=re.MULTILINE)) + count_common=len(re.findall("^source _common.sh|^source ./_common.sh",script["raw"],flags=re.MULTILINE)) count_common_save=len(re.findall("^source ../settings/scripts/_common.sh",script["raw"],flags=re.MULTILINE)) if count_common > 0 and count_common_save == 0: print_error("You must call _common.sh with \"source ../settings/scripts/_common.sh\" in this script to respect context execution") @@ -539,6 +538,7 @@ class Script(): elif count_common_save > 1: print_warning("Duplicates calls to _common.sh, please clean your code") + def main(): if len(sys.argv) != 2: print("Give one app package path.") From ecb7483694a5002fac53277d8bbe6ca16472929d Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Sat, 9 Mar 2019 17:39:00 +0100 Subject: [PATCH 04/10] Rework check_source_common --- package_linter.py | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/package_linter.py b/package_linter.py index cc57581..09f0444 100755 --- a/package_linter.py +++ b/package_linter.py @@ -519,24 +519,10 @@ class Script(): ) def check_source_common(self): - #test for standard execution - if script["name"] in ["install","upgrade","remove"]: - count_common=len(re.findall("^source _common.sh|^source ./_common.sh",script["raw"],flags=re.MULTILINE)) - if count_common == 0: - print_warning("Calling _common.sh seams not present in this script, please add \"source _common.sh\"") - elif count_common > 1: - print_warning("Duplicates calls to _common.sh, please clean your code") - #test for "save" files - else: - count_common=len(re.findall("^source _common.sh|^source ./_common.sh",script["raw"],flags=re.MULTILINE)) - count_common_save=len(re.findall("^source ../settings/scripts/_common.sh",script["raw"],flags=re.MULTILINE)) - if count_common > 0 and count_common_save == 0: - print_error("You must call _common.sh with \"source ../settings/scripts/_common.sh\" in this script to respect context execution") - elif count_common == 0 and count_common_save == 0: - print_warning("Calling _common.sh seams not present in this script, please add \"source ../settings/scripts/_common.sh\" in this script to respect context execution") - elif count_common_save > 1: - print_warning("Duplicates calls to _common.sh, please clean your code") + if self.name in ["backup", "restore"]: + if self.contains("source _common.sh") or self.contains("source ./_common.sh"): + print_error("In the context of backup and restore script, you should load _common.sh with \"source ../settings/scripts/_common.sh\"") def main(): From 652d6bacb33254ef5ed18156ed71ff2d8b4194c1 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Sat, 9 Mar 2019 17:52:08 +0100 Subject: [PATCH 05/10] Encourage packagers to detail what is_public means --- package_linter.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/package_linter.py b/package_linter.py index 2883784..90d5b17 100755 --- a/package_linter.py +++ b/package_linter.py @@ -350,6 +350,16 @@ class App(): "No need to specify the choices list yourself." % argument["name"] ) + if argument["name"] == "is_public" and "help" not in argument.keys(): + print_warning( + "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' + ' }') + + if "url" in manifest and manifest["url"].endswith("_ynh"): print_warning( "'url' is not meant to be the url of the yunohost package, " From f6e98894cfe841aedaa7efd590937f0255193913 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Sat, 9 Mar 2019 18:38:37 +0100 Subject: [PATCH 06/10] Check for path traversal issue --- package_linter.py | 46 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/package_linter.py b/package_linter.py index 620dcd5..2ad01cb 100755 --- a/package_linter.py +++ b/package_linter.py @@ -137,12 +137,19 @@ class App(): ) # - # Deprecated usage of 'add_header' in nginx conf + # 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 not os.path.isfile(self.path + "/conf/" + filename): + # Ignore subdirs or filename not containing nginx in the name + if not os.path.isfile(self.path + "/conf/" + filename) or "nginx" not in filename: continue + + # + # 'add_header' usage + # content = open(self.path + "/conf/" + filename).read() if "location" in content and "add_header" in content: print_warning( @@ -151,6 +158,41 @@ class App(): "and https://github.com/openresty/headers-more-nginx-module#more_set_headers )" ) + # + # Path traversal issues + # + lines = open(self.path + "/conf/" + filename).readlines() + lines = [line.strip() for line in lines if not line.strip().startswith("#")] + # Let's find the first location line + location_line = None + path_traversal_vulnerable = False + lines_iter = lines.__iter__() + for line in lines_iter: + if line.startswith("location"): + location_line = line + break + # Look at the next lines for an 'alias' directive + if location_line is not None: + for line in lines_iter: + if line.startswith("location"): + # Entering a new location block ... abort here + # and assume there's no alias block later... + break + if line.startswith("alias"): + # We should definitely check for path traversal issue + # Does the location target ends with / ? + target = location_line.split()[-2] + if not target.endswith("/"): + path_traversal_vulnerable = True + break + if path_traversal_vulnerable: + print_warning( + "The nginx configuration appears vulnerable to path traversal as explained in " + "https://www.acunetix.com/vulnerabilities/web/path-traversal-via-misconfigured-nginx-alias/\n" + "To fix it, look at the first lines of the nginx conf of the example app : " + "https://github.com/YunoHost/example_ynh/blob/master/conf/nginx.conf" + ) + def check_helper_consistency(self): """ check if ynh_install_app_dependencies is present in install/upgrade/restore From 82b715f06801d356e0714f497ef25636a8a8bdec Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Sat, 9 Mar 2019 19:22:43 +0100 Subject: [PATCH 07/10] Handle case were { is on a newline --- package_linter.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/package_linter.py b/package_linter.py index 2ad01cb..c5edcb9 100755 --- a/package_linter.py +++ b/package_linter.py @@ -169,7 +169,7 @@ class App(): lines_iter = lines.__iter__() for line in lines_iter: if line.startswith("location"): - location_line = line + location_line = line.split() break # Look at the next lines for an 'alias' directive if location_line is not None: @@ -181,7 +181,7 @@ class App(): if line.startswith("alias"): # We should definitely check for path traversal issue # Does the location target ends with / ? - target = location_line.split()[-2] + target = location_line[-2] if location_line[-1] == "{" else location_line[-1] if not target.endswith("/"): path_traversal_vulnerable = True break From f99f2ff5a1efe2c5a7158316afef9de5202366de Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Sat, 9 Mar 2019 19:40:46 +0100 Subject: [PATCH 08/10] Additional check to avoid trying to read binary file in some edge case --- package_linter.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package_linter.py b/package_linter.py index ec190ca..bb6e732 100755 --- a/package_linter.py +++ b/package_linter.py @@ -144,7 +144,7 @@ class App(): # for filename in os.listdir(self.path + "/conf"): - if not os.path.isfile(self.path + "/conf/" + filename): + if not os.path.isfile(self.path + "/conf/" + filename) or "nginx" not in filename: continue content = open(self.path + "/conf/" + filename).read() if "location" in content and "add_header" in content: From 9867d11ff85efb087c1d796f245e0c75645a8222 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Sat, 9 Mar 2019 19:54:55 +0100 Subject: [PATCH 09/10] Check for duplicated key in json + empty description --- package_linter.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/package_linter.py b/package_linter.py index bb6e732..f3c9c1f 100755 --- a/package_linter.py +++ b/package_linter.py @@ -17,6 +17,16 @@ return_code = 0 # 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' @@ -199,7 +209,7 @@ class App(): try: with open(manifest, encoding='utf-8') as data_file: - manifest = json.loads(data_file.read()) + 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.") @@ -294,7 +304,7 @@ class App(): if isinstance(descr, dict): descr = descr.get("en", None) - if descr is None or descr == manifest.get("name", None): + if descr is None or descr == "" or descr == manifest.get("name", None): print_warning( "[YEP-1.9] You should write a good description of the app, " "at least in english (1 line is enough)." From dc2194dfcfafe3009ccffa0888d961958763dd5a Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Sat, 9 Mar 2019 20:26:30 +0100 Subject: [PATCH 10/10] Add a warning if required YunoHost version is still 2.x --- package_linter.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/package_linter.py b/package_linter.py index bb6e732..0e1d309 100755 --- a/package_linter.py +++ b/package_linter.py @@ -359,6 +359,18 @@ class App(): "but rather the website or repo of the upstream app itself..." ) + yunohost_version_req = manifest.get("requirements", {}).get("yunohost", None) + if yunohost_version_req: + major_version = yunohost_version_req.split()[-1] + if major_version.startswith("2"): + print_warning( + "YunoHost version requirement is still 2.x ... Good job if " + "it does still work on Jessie !... But are you really sure " + "about that ;) ? be careful that many new helpers you might " + "already be playing with are only available on 3.x..." + ) + + class Script():