From 775528ce9b8cb9d319c25840bdbd57ec40d59826 Mon Sep 17 00:00:00 2001 From: jack1142 <6032823+jack1142@users.noreply.github.com> Date: Mon, 3 Aug 2020 15:17:27 +0200 Subject: [PATCH] Reject package (extension) names that can't be valid Python identifiers (#3679) * Reject package names that can't be valid Python identifiers * Add info to `[p](re)load` * Improve internal consistency of package vs cog --- redbot/cogs/downloader/repo_manager.py | 4 + redbot/core/cog_manager.py | 16 +++- redbot/core/core_commands.py | 106 ++++++++++++++++++++----- 3 files changed, 103 insertions(+), 23 deletions(-) diff --git a/redbot/cogs/downloader/repo_manager.py b/redbot/cogs/downloader/repo_manager.py index d288af026..abbe680da 100644 --- a/redbot/cogs/downloader/repo_manager.py +++ b/redbot/cogs/downloader/repo_manager.py @@ -2,6 +2,7 @@ from __future__ import annotations import asyncio import functools +import keyword import os import pkgutil import shlex @@ -495,6 +496,9 @@ class Repo(RepoJSONMixin): ) """ for file_finder, name, is_pkg in pkgutil.iter_modules(path=[str(self.folder_path)]): + if not name.isidentifier() or keyword.iskeyword(name): + # reject package names that can't be valid python identifiers + continue if is_pkg: curr_modules.append( Installable(location=self.folder_path / name, repo=self, commit=self.commit) diff --git a/redbot/core/cog_manager.py b/redbot/core/cog_manager.py index 4929ad691..ffaec6f1e 100644 --- a/redbot/core/cog_manager.py +++ b/redbot/core/cog_manager.py @@ -1,4 +1,5 @@ import contextlib +import keyword import pkgutil from importlib import import_module, invalidate_caches from importlib.machinery import ModuleSpec @@ -214,6 +215,13 @@ class CogManager: When no cog with the requested name was found. """ + if not name.isidentifier() or keyword.iskeyword(name): + # reject package names that can't be valid python identifiers + raise NoSuchCog( + f"No 3rd party module by the name of '{name}' was found in any available path.", + name=name, + ) + real_paths = list(map(str, [await self.install_path()] + await self.user_defined_paths())) for finder, module_name, _ in pkgutil.iter_modules(real_paths): @@ -223,9 +231,7 @@ class CogManager: return spec raise NoSuchCog( - "No 3rd party module by the name of '{}' was found in any available path.".format( - name - ), + f"No 3rd party module by the name of '{name}' was found in any available path.", name=name, ) @@ -291,7 +297,9 @@ class CogManager: ret = [] for finder, module_name, _ in pkgutil.iter_modules(paths): - ret.append(module_name) + # reject package names that can't be valid python identifiers + if module_name.isidentifier() and not keyword.iskeyword(module_name): + ret.append(module_name) return ret @staticmethod diff --git a/redbot/core/core_commands.py b/redbot/core/core_commands.py index bb50b8826..8763a62a9 100644 --- a/redbot/core/core_commands.py +++ b/redbot/core/core_commands.py @@ -3,6 +3,7 @@ import contextlib import datetime import importlib import itertools +import keyword import logging import io import random @@ -109,21 +110,34 @@ class CoreLogic: self.bot.register_rpc_handler(self._invite_url) async def _load( - self, cog_names: Iterable[str] - ) -> Tuple[List[str], List[str], List[str], List[str], List[Tuple[str, str]], Set[str]]: + self, pkg_names: Iterable[str] + ) -> Tuple[ + List[str], List[str], List[str], List[str], List[str], List[Tuple[str, str]], Set[str] + ]: """ - Loads cogs by name. + Loads packages by name. + Parameters ---------- - cog_names : list of str + pkg_names : `list` of `str` + List of names of packages to load. Returns ------- tuple - 4-tuple of loaded, failed, not found and already loaded cogs. + 7-tuple of: + 1. List of names of packages that loaded successfully + 2. List of names of packages that failed to load without specified reason + 3. List of names of packages that don't have a valid package name + 4. List of names of packages that weren't found in any cog path + 5. List of names of packages that are already loaded + 6. List of 2-tuples (pkg_name, reason) for packages + that failed to load with a specified reason + 7. Set of repo names that use deprecated shared libraries """ failed_packages = [] loaded_packages = [] + invalid_pkg_names = [] notfound_packages = [] alreadyloaded_packages = [] failed_with_reason_packages = [] @@ -131,24 +145,27 @@ class CoreLogic: bot = self.bot - cogspecs = [] + pkg_specs = [] - for name in cog_names: + for name in pkg_names: + if not name.isidentifier() or keyword.iskeyword(name): + invalid_pkg_names.append(name) + continue try: spec = await bot._cog_mgr.find_cog(name) if spec: - cogspecs.append((spec, name)) + pkg_specs.append((spec, name)) else: notfound_packages.append(name) except Exception as e: log.exception("Package import failed", exc_info=e) - exception_log = "Exception during import of cog\n" + exception_log = "Exception during import of package\n" exception_log += "".join(traceback.format_exception(type(e), e, e.__traceback__)) bot._last_exception = exception_log failed_packages.append(name) - async for spec, name in AsyncIter(cogspecs, steps=10): + async for spec, name in AsyncIter(pkg_specs, steps=10): try: self._cleanup_and_refresh_modules(spec.name) await bot.load_extension(spec) @@ -159,7 +176,7 @@ class CoreLogic: except Exception as e: log.exception("Package loading failed", exc_info=e) - exception_log = "Exception during loading of cog\n" + exception_log = "Exception during loading of package\n" exception_log += "".join(traceback.format_exception(type(e), e, e.__traceback__)) bot._last_exception = exception_log failed_packages.append(name) @@ -184,6 +201,7 @@ class CoreLogic: return ( loaded_packages, failed_packages, + invalid_pkg_names, notfound_packages, alreadyloaded_packages, failed_with_reason_packages, @@ -212,13 +230,14 @@ class CoreLogic: for child_name, lib in children.items(): importlib._bootstrap._exec(lib.__spec__, lib) - async def _unload(self, cog_names: Iterable[str]) -> Tuple[List[str], List[str]]: + async def _unload(self, pkg_names: Iterable[str]) -> Tuple[List[str], List[str]]: """ - Unloads cogs with the given names. + Unloads packages with the given names. Parameters ---------- - cog_names : list of str + pkg_names : `list` of `str` + List of names of packages to unload. Returns ------- @@ -230,7 +249,7 @@ class CoreLogic: bot = self.bot - for name in cog_names: + for name in pkg_names: if name in bot.extensions: bot.unload_extension(name) await bot.remove_loaded_package(name) @@ -241,22 +260,39 @@ class CoreLogic: return unloaded_packages, failed_packages async def _reload( - self, cog_names: Sequence[str] - ) -> Tuple[List[str], List[str], List[str], List[str], List[Tuple[str, str]], Set[str]]: - await self._unload(cog_names) + self, pkg_names: Sequence[str] + ) -> Tuple[ + List[str], List[str], List[str], List[str], List[str], List[Tuple[str, str]], Set[str] + ]: + """ + Reloads packages with the given names. + + Parameters + ---------- + pkg_names : `list` of `str` + List of names of packages to reload. + + Returns + ------- + tuple + Tuple as returned by `CoreLogic._load()` + """ + await self._unload(pkg_names) ( loaded, load_failed, + invalid_pkg_names, not_found, already_loaded, load_failed_with_reason, repos_with_shared_libs, - ) = await self._load(cog_names) + ) = await self._load(pkg_names) return ( loaded, load_failed, + invalid_pkg_names, not_found, already_loaded, load_failed_with_reason, @@ -1252,6 +1288,7 @@ class Core(commands.commands._RuleDropper, commands.Cog, CoreLogic): ( loaded, failed, + invalid_pkg_names, not_found, already_loaded, failed_with_reason, @@ -1289,6 +1326,21 @@ class Core(commands.commands._RuleDropper, commands.Cog, CoreLogic): ).format(packs=humanize_list([inline(package) for package in failed])) output.append(formed) + if invalid_pkg_names: + if len(invalid_pkg_names) == 1: + formed = _( + "The following name is not a valid package name: {pack}\n" + "Package names cannot start with a number" + " and can only contain ascii numbers, letters, and underscores." + ).format(pack=inline(invalid_pkg_names[0])) + else: + formed = _( + "The following names are not valid package names: {packs}\n" + "Package names cannot start with a number" + " and can only contain ascii numbers, letters, and underscores." + ).format(packs=humanize_list([inline(package) for package in invalid_pkg_names])) + output.append(formed) + if not_found: if len(not_found) == 1: formed = _("The following package was not found in any cog path: {pack}.").format( @@ -1381,6 +1433,7 @@ class Core(commands.commands._RuleDropper, commands.Cog, CoreLogic): ( loaded, failed, + invalid_pkg_names, not_found, already_loaded, failed_with_reason, @@ -1407,6 +1460,21 @@ class Core(commands.commands._RuleDropper, commands.Cog, CoreLogic): ).format(packs=humanize_list([inline(package) for package in failed])) output.append(formed) + if invalid_pkg_names: + if len(invalid_pkg_names) == 1: + formed = _( + "The following name is not a valid package name: {pack}\n" + "Package names cannot start with a number" + " and can only contain ascii numbers, letters, and underscores." + ).format(pack=inline(invalid_pkg_names[0])) + else: + formed = _( + "The following names are not valid package names: {packs}\n" + "Package names cannot start with a number" + " and can only contain ascii numbers, letters, and underscores." + ).format(packs=humanize_list([inline(package) for package in invalid_pkg_names])) + output.append(formed) + if not_found: if len(not_found) == 1: formed = _("The following package was not found in any cog path: {pack}.").format(