Reject invalid NVR before calling Koji API

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)

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"
+        )

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()
 

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 = {

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,19 +91,25 @@ 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():
         subject = create_subject("koji_build", nvr)
         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")