diff --git a/greenwave/resources.py b/greenwave/resources.py index 992dce9f..6e051d8c 100644 --- a/greenwave/resources.py +++ b/greenwave/resources.py @@ -209,9 +209,20 @@ def retrieve_koji_build_target(task_id, koji_url: str): return None +def _is_valid_nvr(nvr: str) -> bool: + """ + Check that the NVR can be passed to the Koji API, mirroring the + validation done by koji.parse_NVR(): name, version and release must be + separated by dashes, with non-empty version and release. The name may + contain dashes (e.g. "pki-core-10.5.1-1.el8"), an epoch, or be empty. + Unlike koji.parse_NVR(), line breaks in the name are also rejected. + """ + return isinstance(nvr, str) and re.fullmatch(r".*-[^-]+-[^-]+", nvr) is not None + + @cached def _retrieve_koji_build_attributes(nvr: str, koji_url: str): - if not isinstance(nvr, str) or "-" not in nvr: + if not _is_valid_nvr(nvr): raise BadRequest(f"Invalid NVR format: {nvr!r}") log.debug("Getting Koji build %r", nvr) diff --git a/greenwave/tests/test_koji.py b/greenwave/tests/test_koji.py index 255d88a0..9843c685 100644 --- a/greenwave/tests/test_koji.py +++ b/greenwave/tests/test_koji.py @@ -1,7 +1,8 @@ # SPDX-License-Identifier: GPL-2.0+ import pytest -from werkzeug.exceptions import BadRequest +from defusedxml.xmlrpc import xmlrpc_client +from werkzeug.exceptions import BadGateway, BadRequest from greenwave.resources import retrieve_koji_build_task_id @@ -13,9 +14,48 @@ 123, "null", "bad_subject", + # A bare package name is not a valid NVR: name, version and release + # must be separated by dashes. + "pki-core", + "nethack-1.2.3", + # Empty release. + "nethack-1.2.3-", + # Empty version. + "nethack--1", + # Line break in the name. + "foo\nbar-1.0-1", ), ) -def test_koji_bad_nvr(app, subject): +def test_koji_bad_nvr(app, koji_proxy, subject): expected = "Invalid NVR format: " with pytest.raises(BadRequest, match=expected): retrieve_koji_build_task_id(subject, "https://localhost:5006/kojihub") + + +@pytest.mark.parametrize( + "nvr", + ( + "nethack-1.2.3-1.rawhide", + # Package name may contain dashes. + "pki-core-10.5.1-1.el8", + # Draft build. + "freerdp-2.1.1-5.el7_9.14,draft_4144991", + # Epoch is a part of the name for koji.parse_NVR(). + "1:foo-1.0-1", + # Name may be empty for koji.parse_NVR(). + "-1.0-1", + ), +) +def test_koji_good_nvr(app, koji_proxy, nvr): + """NVRs accepted by koji.parse_NVR() are passed to the Koji API.""" + retrieve_koji_build_task_id(nvr, "https://localhost:5006/kojihub") + koji_proxy.getBuild.assert_called_once_with(nvr) + + +def test_koji_fault(app, koji_proxy): + expected = "Failed to get Koji build" + koji_proxy.getBuild.side_effect = xmlrpc_client.Fault(1000, "invalid format") + with pytest.raises(BadGateway, match=expected): + retrieve_koji_build_task_id( + "nethack-1.2.3-1.rawhide", "https://localhost:5006/kojihub" + ) diff --git a/greenwave/tests/test_listeners.py b/greenwave/tests/test_listeners.py index 5d9bf563..93277997 100644 --- a/greenwave/tests/test_listeners.py +++ b/greenwave/tests/test_listeners.py @@ -5,6 +5,7 @@ import pytest import stomp +from defusedxml.xmlrpc import xmlrpc_client from requests.exceptions import HTTPError from werkzeug.exceptions import BadGateway, NotFound @@ -643,6 +644,32 @@ def test_kafka_listener_commits_message_on_failed_policy_matching( listener._kafka_bus.commit.assert_called_once_with(message) +def test_decision_change_for_invalid_nvr( + mock_retrieve_decision, + mock_retrieve_results, + mock_connection, + koji_proxy, +): + """ + Test that a message about a subject with an invalid NVR, e.g. a bare + package name, does not crash the listener: the invalid NVR is rejected + before it is sent to the Koji API and the failure to fetch the remote + rule file is reported in the published decision. + """ + koji_proxy.getBuild.side_effect = xmlrpc_client.Fault( + 1000, "invalid format: pki-core" + ) + + listener = resultsdb_listener(POLICIES_WITH_REMOTE_RULE) + + with listener.app.app_context(): + listener.on_message(DummyMessage(nvr="pki-core")) + + # The invalid NVR was not sent to the Koji API. + koji_proxy.getBuild.assert_not_called() + assert len(mock_connection.send.mock_calls) == 1 + + def test_guess_product_version(mock_connection, koji_proxy): listener = resultsdb_listener() diff --git a/greenwave/tests/test_policies.py b/greenwave/tests/test_policies.py index 0805c057..91a0ebb8 100644 --- a/greenwave/tests/test_policies.py +++ b/greenwave/tests/test_policies.py @@ -1765,7 +1765,7 @@ def test_remote_rule_policy_on_demand_policy(namespace): def test_on_demand_policy_match(two_rules, koji_proxy): """Proceed other rules when there's no source URL in Koji build""" - nvr = "httpd-2.4.el9000" + nvr = "httpd-2.4-1.el9000" subject = create_subject("koji_build", nvr) serverside_json = { diff --git a/greenwave/tests/test_rules.py b/greenwave/tests/test_rules.py index ccb2edc8..cdb6451a 100644 --- a/greenwave/tests/test_rules.py +++ b/greenwave/tests/test_rules.py @@ -1,9 +1,7 @@ -import re from textwrap import dedent from unittest import mock import pytest -from defusedxml.xmlrpc import xmlrpc_client from werkzeug.exceptions import BadGateway, BadRequest, NotFound from greenwave.app_factory import create_app @@ -93,7 +91,6 @@ def test_invalid_nvr_iden(koji, mock_retrieve_yaml_remote_rule): - !RemoteRule {} """) nvr = "nieco-1" - koji().getBuild.side_effect = xmlrpc_client.Fault(1000, "invalid format") app = create_app("greenwave.config.TestingConfig") with app.app_context(): @@ -101,11 +98,18 @@ def test_invalid_nvr_iden(koji, mock_retrieve_yaml_remote_rule): policies = Policy.safe_load_all(policy_yaml) policy = policies[0] rule = policy.rules[0] - expected_error = re.escape( - f'Failed to get Koji build for "{nvr}": invalid format (code: 1000)' + + # The invalid NVR is rejected before it is sent to the Koji API and + # the failure to fetch the remote rule file is reported. + sub_policies, answers = rule._get_sub_policies(policy, subject) + koji.assert_not_called() + assert sub_policies == [] + assert len(answers) == 1 + answer = answers[0].to_json() + assert answer["type"] == "failed-fetch-gating-yaml" + assert answer["error"] == ( + "Failed to get remote policies: 400 Bad Request: Invalid NVR format: 'nieco-1'" ) - with pytest.raises(BadGateway, match=expected_error): - rule._get_sub_policies(policy, subject) @mock.patch("greenwave.resources.retrieve_yaml_remote_rule")