From 6d61a59b131ee6c277a6aa04702ba4da480790dc Mon Sep 17 00:00:00 2001 From: mprahl Date: Mar 19 2019 12:01:23 +0000 Subject: [PATCH 1/2] Clean up the valid API parameters The MBS submission API endpoint should not accept every parameter that is also a column on the ModuleBuild table. There are two reasons for this. The first is that a user should be notified if the supplied parameter is invalid, whereas it could get silently ignored. The second reason is that a nefarious user could pass in specially crafted API parameters causing MBS to do something unexpected or undesired. --- diff --git a/module_build_service/views.py b/module_build_service/views.py index 8b02215..e027e78 100644 --- a/module_build_service/views.py +++ b/module_build_service/views.py @@ -314,6 +314,19 @@ class ImportModuleAPI(MethodView): class BaseHandler(object): + valid_params = set([ + 'branch', + 'buildrequire_overrides', + 'modulemd', + 'module_name', + 'owner', + 'rebuild_strategy', + 'require_overrides', + 'scmurl', + 'scratch', + 'srpms' + ]) + def __init__(self, request, data=None): self.username, self.groups = module_build_service.auth.get_user(request) self.data = data or _dict_from_request(request) @@ -361,18 +374,7 @@ class BaseHandler(object): raise ValidationError(invalid_override_msg) def validate_optional_params(self): - module_build_columns = set([col.key for col in models.ModuleBuild.__table__.columns]) - other_params = set([ - 'branch', - 'buildrequire_overrides', - 'modulemd', - 'module_name', - 'rebuild_strategy', - 'require_overrides', - ]) - valid_params = other_params | module_build_columns - - forbidden_params = [k for k in self.data if k not in valid_params] + forbidden_params = [k for k in self.data if k not in self.valid_params] if forbidden_params: raise ValidationError('The request contains unspecified parameters: {}' .format(", ".join(forbidden_params))) From 275e7e4509e452ae6cae28f6343329b28603d374 Mon Sep 17 00:00:00 2001 From: mprahl Date: Mar 19 2019 12:02:39 +0000 Subject: [PATCH 2/2] Remove the concept of required and optional parameters Since the required parameters vary based on if the modulemd comes from SCM or a direct submission, the concept of optional parameters doesn't really apply. --- diff --git a/module_build_service/manage.py b/module_build_service/manage.py index ec816da..0bd4584 100755 --- a/module_build_service/manage.py +++ b/module_build_service/manage.py @@ -134,14 +134,14 @@ def build_module_locally(local_build_nsvs=None, yaml_file=None, srpms=None, db.create_all() load_local_builds(local_build_nsvs) - optional_params = {} - optional_params["local_build"] = True - optional_params["default_streams"] = {} + params = {} + params["local_build"] = True + params["default_streams"] = {} for ns in default_streams: name, stream = ns.split(":") - optional_params["default_streams"][name] = stream + params["default_streams"][name] = stream if srpms: - optional_params["srpms"] = srpms + params["srpms"] = srpms username = getpass.getuser() if not yaml_file or not yaml_file.endswith(".yaml"): @@ -154,7 +154,7 @@ def build_module_locally(local_build_nsvs=None, yaml_file=None, srpms=None, handle.filename = filename try: modules_list = submit_module_build_from_yaml( - username, handle, str(stream), skiptests, optional_params) + username, handle, params, stream=str(stream), skiptests=skiptests) except StreamAmbigous as e: logging.error(str(e)) logging.error( diff --git a/module_build_service/utils/submit.py b/module_build_service/utils/submit.py index 0fd86d2..b5dedba 100644 --- a/module_build_service/utils/submit.py +++ b/module_build_service/utils/submit.py @@ -469,8 +469,7 @@ def record_component_builds(mmd, module, initial_batch=1, return batch -def submit_module_build_from_yaml(username, handle, stream=None, skiptests=False, - optional_params=None): +def submit_module_build_from_yaml(username, handle, params, stream=None, skiptests=False): yaml_file = to_text_type(handle.read()) mmd = load_mmd(yaml_file) dt = datetime.utcfromtimestamp(int(time.time())) @@ -483,14 +482,15 @@ def submit_module_build_from_yaml(username, handle, stream=None, skiptests=False buildopts = mmd.get_rpm_buildopts() buildopts["macros"] = buildopts.get("macros", "") + "\n\n%__spec_check_pre exit 0\n" mmd.set_rpm_buildopts(buildopts) - return submit_module_build(username, None, mmd, optional_params) + return submit_module_build(username, mmd, params) _url_check_re = re.compile(r"^[^:/]+:.*$") -def submit_module_build_from_scm(username, url, branch, allow_local_url=False, - optional_params=None): +def submit_module_build_from_scm(username, params, allow_local_url=False): + url = params['scmurl'] + branch = params['branch'] # Translate local paths into file:// URL if allow_local_url and not _url_check_re.match(url): log.info( @@ -499,20 +499,20 @@ def submit_module_build_from_scm(username, url, branch, allow_local_url=False, url = "file://" + url mmd, scm = _fetch_mmd(url, branch, allow_local_url) - return submit_module_build(username, url, mmd, optional_params) + return submit_module_build(username, mmd, params) -def _apply_dep_overrides(mmd, optional_params): +def _apply_dep_overrides(mmd, params): """ Apply the dependency override parameters (if specified) on the input modulemd. :param Modulemd.Module mmd: the modulemd to apply the overrides on - :param dict optional_params: the optional API parameters passed in by the user + :param dict params: the API parameters passed in by the user :raises ValidationError: if one of the overrides doesn't apply """ dep_overrides = { - 'buildrequires': optional_params.get('buildrequire_overrides', {}), - 'requires': optional_params.get('require_overrides', {}) + 'buildrequires': params.get('buildrequire_overrides', {}), + 'requires': params.get('require_overrides', {}) } unused_dep_overrides = { @@ -541,28 +541,16 @@ def _apply_dep_overrides(mmd, optional_params): 'The {} overrides for the following modules aren\'t applicable: {}' .format(dep_type[:-1], ', '.join(sorted(unused_dep_overrides[dep_type])))) - # Delete the parameters or else they'll get past to ModuleBuild.create - if 'buildrequire_overrides' in optional_params: - del optional_params['buildrequire_overrides'] - if 'require_overrides' in optional_params: - del optional_params['require_overrides'] - mmd.set_dependencies(deps) -def submit_module_build(username, url, mmd, optional_params=None): +def submit_module_build(username, mmd, params): """ Submits new module build. :param str username: Username of the build's owner. - :param str url: SCM URL of submitted build. :param Modulemd.Module mmd: Modulemd defining the build. - :param dict optional_params: Dict with optional params for a build: - - "local_build" (bool): The module is being built locally (the MBS is - not running in infra, but on local developer's machine). - - "default_streams" (dict): Dict with name:stream mapping defining the stream - to choose for given module name if multiple streams are available to choose from. - - Any optional ModuleBuild class field (str). + :param dict params: the API parameters passed in by the user :rtype: list with ModuleBuild :return: List with submitted module builds. """ @@ -574,17 +562,14 @@ def submit_module_build(username, url, mmd, optional_params=None): raise_if_stream_ambigous = False default_streams = {} - if optional_params: - # For local builds, we want the user to choose the exact stream using the default_streams - # in case there are multiple streams to choose from and raise an exception otherwise. - if "local_build" in optional_params: - raise_if_stream_ambigous = True - del optional_params["local_build"] - # Get the default_streams if set. - if "default_streams" in optional_params: - default_streams = optional_params["default_streams"] - del optional_params["default_streams"] - _apply_dep_overrides(mmd, optional_params) + # For local builds, we want the user to choose the exact stream using the default_streams + # in case there are multiple streams to choose from and raise an exception otherwise. + if "local_build" in params: + raise_if_stream_ambigous = True + # Get the default_streams if set. + if "default_streams" in params: + default_streams = params["default_streams"] + _apply_dep_overrides(mmd, params) validate_mmd(mmd) mmds = generate_expanded_mmds(db.session, mmd, raise_if_stream_ambigous, default_streams) @@ -614,12 +599,13 @@ def submit_module_build(username, url, mmd, optional_params=None): "is allowed.", nsvc) modules.append(module) continue - if optional_params: - rebuild_strategy = optional_params.get('rebuild_strategy') - if rebuild_strategy and module.rebuild_strategy != rebuild_strategy: - raise ValidationError( - 'You cannot change the module\'s "rebuild_strategy" when ' - 'resuming a module build') + + rebuild_strategy = params.get('rebuild_strategy') + if rebuild_strategy and module.rebuild_strategy != rebuild_strategy: + raise ValidationError( + 'You cannot change the module\'s "rebuild_strategy" when ' + 'resuming a module build') + log.debug('Resuming existing module build %r' % module) # Reset all component builds that didn't complete for component in module.component_builds: @@ -645,9 +631,11 @@ def submit_module_build(username, url, mmd, optional_params=None): stream=mmd.get_stream(), version=version_str, modulemd=to_text_type(mmd.dumps()), - scmurl=url, + scmurl=params.get('scmurl'), username=username, - **(optional_params or {}) + rebuild_strategy=params.get('rebuild_strategy'), + scratch=params.get('scratch'), + srpms=params.get('srpms') ) (module.ref_build_context, module.build_context, module.runtime_context, module.context) = module.contexts_from_mmd(module.modulemd) diff --git a/module_build_service/views.py b/module_build_service/views.py index e027e78..d897fad 100644 --- a/module_build_service/views.py +++ b/module_build_service/views.py @@ -348,11 +348,6 @@ class BaseHandler(object): else: self.data['srpms'] = [] - @property - def optional_params(self): - return {k: v for k, v in self.data.items() if k not in - ["owner", "scmurl", "branch", "modulemd", "module_name"]} - def _validate_dep_overrides_format(self, key): """ Validate any dependency overrides provided to the API. @@ -422,12 +417,7 @@ class SCMHandler(BaseHandler): self.validate_optional_params() def post(self): - url = self.data["scmurl"] - branch = self.data["branch"] - - return submit_module_build_from_scm(self.username, url, branch, - allow_local_url=False, - optional_params=self.optional_params) + return submit_module_build_from_scm(self.username, self.data, allow_local_url=False) class YAMLFileHandler(BaseHandler): @@ -452,8 +442,7 @@ class YAMLFileHandler(BaseHandler): handle.filename = "unnamed" else: handle = request.files["yaml"] - return submit_module_build_from_yaml(self.username, handle, - optional_params=self.optional_params) + return submit_module_build_from_yaml(self.username, handle, self.data) def _dict_from_request(request): diff --git a/tests/test_utils/test_utils.py b/tests/test_utils/test_utils.py index 0ecbcac..de7651d 100644 --- a/tests/test_utils/test_utils.py +++ b/tests/test_utils/test_utils.py @@ -281,11 +281,11 @@ class TestUtilsComponentReuse: with open(modulemd_file_path, "rb") as fd: handle = FileStorage(fd) - module_build_service.utils.submit_module_build_from_yaml(username, handle, - stream=stream, skiptests=True) + module_build_service.utils.submit_module_build_from_yaml( + username, handle, {}, stream=stream, skiptests=True) mock_submit_args = mock_submit.call_args[0] username_arg = mock_submit_args[0] - mmd_arg = mock_submit_args[2] + mmd_arg = mock_submit_args[1] assert mmd_arg.get_stream() == stream assert "\n\n%__spec_check_pre exit 0\n" in mmd_arg.get_rpm_buildopts()['macros'] assert username_arg == username @@ -789,7 +789,7 @@ class TestUtils: generate_expanded_mmds.return_value = [mmd1, mmd2] - builds = module_build_service.utils.submit_module_build("foo", "bar", mmd1) + builds = module_build_service.utils.submit_module_build("foo", mmd1, {}) ret = {b.mmd().get_context(): b.state for b in builds} assert ret == { "c1": models.BUILD_STATES['ready'],