Remove hardcoded list of supported Fedora CI tests by betulependule · Pull Request #3039 · packit/packit-service

betulependule · 2026-03-04T11:34:50Z

centosinfra-prod-github-app · 2026-03-04T11:39:08Z

Build succeeded.
https://gateway-cloud-softwarefactory.apps.ocp.cloud.ci.centos.org/zuul/t/packit-service/buildset/fdce0683b13640faa14b8c4097528b13

✔️ pre-commit SUCCESS in 1m 50s
✔️ packit-service-tests SUCCESS in 3m 33s

gemini-code-assist

Code Review

This pull request refactors the parsing of PR comments to dynamically fetch the list of supported Fedora CI tests, moving the parse_comment function into the SteveJobs class for better access to instance-specific data. While the refactoring improves flexibility, a critical security vulnerability exists: the modified is_packit_config_present function continues to use flawed logic that trusts issue descriptions for configuration, which could lead to malicious configuration injection and potential RCE. Additionally, a minor code quality issue was noted due to a missing docstring, violating the repository's style guide.

gemini-code-assist · 2026-03-04T11:39:25Z

packit_service/worker/jobs.py

        """
        if isinstance(self.event, abstract.comment.CommentEvent):
-            arguments = parse_comment(
+            arguments = self.parse_comment(


The is_packit_config_present method (modified in this PR) relies on search_distgit_config_in_issue to extract a dist-git URL and package configuration from the description of the issue associated with the event. Since issue descriptions are user-controlled, an attacker can craft a malicious issue description that points to a repository they control. If a maintainer or an allowlisted user is tricked into commenting on this issue, Packit will load and use the attacker's malicious packit.yaml configuration. This can lead to Remote Code Execution (RCE) on the Packit worker if the malicious configuration defines arbitrary actions (e.g., post-upstream-clone).

Remediation:
Verify that the issue was created by a trusted account (e.g., the Packit bot itself) before trusting its description. Alternatively, avoid loading configuration from issue descriptions and instead use a more secure way to store and retrieve failure context.

gemini-code-assist · 2026-03-04T11:39:25Z

packit_service/utils.py

-    prog: str, description: str, epilog: str
+    prog: str, description: str, epilog: str, supported_test_types: list[str]
 ) -> argparse.ArgumentParser:
    parser = _create_base_parser(prog, description, epilog)


According to the repository's style guide (lines 412-414), all non-trivial functions should have a docstring. Please add a docstring to get_pr_comment_parser_fedora_ci explaining its purpose, arguments, and what it returns.

"""Create a parser for the Fedora CI PR comment. Args: prog: The name of the program. description: The program description. epilog: Text to display after the argument help. supported_test_types: A list of supported test types. Returns: An argument parser for Fedora CI PR comments. """ parser = _create_base_parser(prog, description, epilog)

References

With exception of trivial cases, all code must contain accurate and sufficiently detailed docstrings, formatted with accordance with the PEP 257 standard and in Google-style. ^(link)

packit_service/worker/jobs.py

nforro · 2026-03-04T12:03:51Z

Nice! It would be great if this also considered the filters (in the tests namespace there is only the custom check available; or no check if there is no fmf metadata in the project):

packit-service/packit_service/worker/handlers/testing_farm.py

Lines 558 to 560 in 1d3ed53

    
           @classmethod 
        
           def filter_ci_tests(cls, tests: list[str]) -> list[str]: 
        
               return [t for t in tests if t == "custom"]

However I'm not sure how to elegantly implement this. It would probably require implementing the filtering in a different way.

betulependule · 2026-03-04T12:26:41Z

Nice! It would be great if this also considered the filters (in the tests namespace there is only the custom check available; or no check if there is no fmf metadata in the project):

I will take a look and see if I can think of a good way to implement this. Thanks. 🙏

centosinfra-prod-github-app · 2026-03-17T14:56:12Z

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
Warning:
Error merging github.com/packit/packit-service for 3039,dbdd0d1e8491c7a838e0f89564af33c5e8db99c3

centosinfra-prod-github-app · 2026-03-17T15:16:17Z

Build failed.
https://gateway-cloud-softwarefactory.apps.ocp.cloud.ci.centos.org/zuul/t/packit-service/buildset/9b7e3762b6b84c8187a51622fd10ff6a

❌ pre-commit FAILURE in 1m 53s
✔️ packit-service-tests SUCCESS in 3m 58s

betulependule · 2026-03-17T15:56:59Z

Not sure what's with the pre-commit fail. It sometimes just randomly occurs.

I've implemented the filtering of available CI tests. It's the best way that I could think of. There is a side effect that it changes the behavior of get_fedora_ci_tests as all three installability, rpminspect and rpmlint are filtered out in the "tests" namespace now. It doesn't break any tests, but it's still something to consider. I don't think I can simply test this on a local deployment, so perhaps I should write a regular test for this.

centosinfra-prod-github-app · 2026-03-18T08:20:15Z

Build succeeded.
https://gateway-cloud-softwarefactory.apps.ocp.cloud.ci.centos.org/zuul/t/packit-service/buildset/dab3661c0eeb4bfca0e0e6452e1ebd52

✔️ pre-commit SUCCESS in 1m 53s
✔️ packit-service-tests SUCCESS in 4m 11s

centosinfra-prod-github-app · 2026-03-23T09:59:19Z

Build succeeded.
https://gateway-cloud-softwarefactory.apps.ocp.cloud.ci.centos.org/zuul/t/packit-service/buildset/0d2cd7d72536466da03b3ca1b0952752

✔️ pre-commit SUCCESS in 1m 52s
✔️ packit-service-tests SUCCESS in 4m 05s

nforro · 2026-03-23T17:21:14Z

packit_service/worker/helpers/testing_farm.py

+    def get_fedora_ci_tests_available_in_context(
+        service_config: ServiceConfig, project: GitProject, metadata: EventData
+    ) -> list[str]:


How is this different from get_fedora_ci_tests()? Couldn't you modify get_fedora_ci_tests() instead? Also, doesn't this make filter_ci_tests() useless?

packit-service/packit_service/worker/handlers/testing_farm.py

Lines 565 to 567 in 830d855

@classmethod

def filter_ci_tests(cls, tests: list[str]) -> list[str]:

return [t for t in tests if t == "custom"]

Well, get_fedora_ci_tests() has some additinal filtering of the list of tests based on the retriggering comment.
But I could edit the function by giving it an another bool parameter that would specify whether to skip this filtering. Then I could use get_fedora_ci_tests() instead of get_fedora_ci_tests_available_in_context. It would probably be cleaner like that, so I'll edit the code that way. Thanks.

And yes, it seems that filter_ci_tests() is now redundant. I'll try to remove it and see whether it breaks any tests.

packit_service/worker/helpers/testing_farm.py

centosinfra-prod-github-app · 2026-03-25T10:42:34Z

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
Warning:
Error merging github.com/packit/packit-service for 3039,08cd220eaf1fb0e37ea02317fd3dbbf305d4c641

It is now taken into consideration that in the "tests" namespace only the `custom` check should be available. When no fmf metadata is available, no test types are supported in the "tests" namespace. The method `get_fedora_ci_tests_available_in_context` retrieves a list of supported test types based on these restrictions. The previously used `get_fedora_ci_tests` method cannot be used for this purpose as it filters out test types based on the user's comment content. In case of the `/packit help` comment, this would lead to all test types being filtered out and the help message wouldn't list any supported test types.

The method has been modified with an additional parameter, which specifies whether to retrieve all Fedora CI test types regardless of the content of user's comment.

The `filter_ci_tests()` was rendered redundant with the recently added `skipif` option used in some `@implements_fedora_ci` decorators, which implements the same filtering of CI tests.

This improves the readability of some `@implements_fedora_ci` decorators. The `is_fmf_configured` method in `TestingFarmJobHelper` now reuses the `is_fmf_configured` function to reduce code duplication.

centosinfra-prod-github-app · 2026-03-25T10:56:40Z

Build succeeded.
https://gateway-cloud-softwarefactory.apps.ocp.cloud.ci.centos.org/zuul/t/packit-service/buildset/5822bead7e174fe597d76a0b5b65d1b5

✔️ pre-commit SUCCESS in 1m 50s
✔️ packit-service-tests SUCCESS in 4m 07s

packit_service/worker/helpers/testing_farm.py

packit_service/worker/jobs.py

nforro · 2026-03-25T11:31:02Z

packit_service/worker/helpers/testing_farm.py

+def is_fmf_configured(project: GitProject, metadata: EventData) -> bool:
+    try:
+        project.get_file_content(
+            path=".fmf/version",
+            ref=metadata.commit_sha,
+        )
+    except FileNotFoundError:
+        return False
+    return True
+
+
+def is_project_in_tests_namespace(project: GitProject) -> bool:
+    return project.namespace == "tests"


I'm wondering if we couldn't/shouldn't use a concept similar to Checkers.

That would be probably better. I'll think of how to go about it.

I suppose I can implement IsFmfConfigured and IsProjectInTestsNamespace checkers. Then I can redefine @implements_fedora_ci_test to accept a Checker instead of Callable and edit get_fedora_ci_tests() to call the pre_check() method to filter out test types that should be skipped (so edit the portion of the code with skipif).

all_tests = [ name for name, (_, skipif) in FEDORA_CI_TESTS.items() if not skipif or not skipif(service_config, project, metadata) ]

I suppose I can implement IsFmfConfigured and IsProjectInTestsNamespace checkers. Then I can redefine @implements_fedora_ci_test to accept a Checker instead of Callable and edit get_fedora_ci_tests() to call the pre_check() method to filter out test types that should be skipped (so edit the portion of the code with skipif).

all_tests = [ name for name, (_, skipif) in FEDORA_CI_TESTS.items() if not skipif or not skipif(service_config, project, metadata) ]

Sounds good, thanks!

centosinfra-prod-github-app · 2026-03-25T11:48:03Z

Build succeeded.
https://gateway-cloud-softwarefactory.apps.ocp.cloud.ci.centos.org/zuul/t/packit-service/buildset/7d8f7ecce7454fdfbb4140864d1037a1

✔️ pre-commit SUCCESS in 1m 52s
✔️ packit-service-tests SUCCESS in 4m 12s

nforro · 2026-03-26T11:56:25Z

packit_service/worker/helpers/testing_farm.py

            if not skipif or not skipif(service_config, project, metadata)
        ]
-        if metadata.event_type != pagure.pr.Comment.event_type():
+        if metadata.event_type != pagure.pr.Comment.event_type() or not filter_specific_test:


This is just a nitpick, but I think it would be more understandable to write it like this:

Suggested change

if metadata.event_type != pagure.pr.Comment.event_type() or not filter_specific_test:

if not filter_specific_test:

return all_tests

if metadata.event_type != pagure.pr.Comment.event_type():

return all_tests

Or perhaps moving the comment processing into a nested function, to make it clear it's an extra filtering step. I assume in the future it will be unified and probably replaced with some function/method call.

betulependule requested review from a team, lbarcziova, mfocko and nforro as code owners March 4, 2026 11:34

usercont-release-bot added this to Packit pull requests Mar 4, 2026

github-project-automation bot moved this to New in Packit pull requests Mar 4, 2026

gemini-code-assist bot reviewed Mar 4, 2026

View reviewed changes

nforro reviewed Mar 4, 2026

View reviewed changes

packit_service/worker/jobs.py Show resolved Hide resolved

betulependule force-pushed the hardcoded-values-comment-parser branch 2 times, most recently from 3026371 to 82c49af Compare March 17, 2026 15:11

betulependule force-pushed the hardcoded-values-comment-parser branch from 82c49af to 5c79adc Compare March 18, 2026 08:15

betulependule force-pushed the hardcoded-values-comment-parser branch from 5c79adc to 20e3a5e Compare March 23, 2026 09:54

nforro reviewed Mar 23, 2026

View reviewed changes

packit_service/worker/helpers/testing_farm.py Show resolved Hide resolved

betulependule added 5 commits March 25, 2026 11:51

Remove hardcoded list of supported Fedora CI tests

6122db2

Reuse and modify get_fedora_ci_tests()

109f82e

The method has been modified with an additional parameter, which specifies whether to retrieve all Fedora CI test types regardless of the content of user's comment.

Remove the filter_ci_tests() method due to redundancy

1ae5614

The `filter_ci_tests()` was rendered redundant with the recently added `skipif` option used in some `@implements_fedora_ci` decorators, which implements the same filtering of CI tests.

Move checks outside DownstreamTestingFarmJobHelper

c103735

This improves the readability of some `@implements_fedora_ci` decorators. The `is_fmf_configured` method in `TestingFarmJobHelper` now reuses the `is_fmf_configured` function to reduce code duplication.

betulependule force-pushed the hardcoded-values-comment-parser branch from 08cd220 to c103735 Compare March 25, 2026 10:51

nforro reviewed Mar 25, 2026

View reviewed changes

packit_service/worker/helpers/testing_farm.py Outdated Show resolved Hide resolved

nforro reviewed Mar 25, 2026

View reviewed changes

packit_service/worker/jobs.py Outdated Show resolved Hide resolved

nforro reviewed Mar 25, 2026

View reviewed changes

Rename ambiguous parameter and list it in docstring

187c4d8

nforro reviewed Mar 26, 2026

View reviewed changes

	@classmethod
	def filter_ci_tests(cls, tests: list[str]) -> list[str]:
	return [t for t in tests if t == "custom"]

-        if metadata.event_type != pagure.pr.Comment.event_type() or not filter_specific_test:
+        if not filter_specific_test:
+            return all_tests
+        if metadata.event_type != pagure.pr.Comment.event_type():
+            return all_tests

Conversation

betulependule commented Mar 4, 2026

Uh oh!

centosinfra-prod-github-app bot commented Mar 4, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nforro commented Mar 4, 2026

Uh oh!

betulependule commented Mar 4, 2026

Uh oh!

centosinfra-prod-github-app bot commented Mar 17, 2026

Uh oh!

centosinfra-prod-github-app bot commented Mar 17, 2026

Uh oh!

betulependule commented Mar 17, 2026

Uh oh!

centosinfra-prod-github-app bot commented Mar 18, 2026

Uh oh!

centosinfra-prod-github-app bot commented Mar 23, 2026

Uh oh!

nforro Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

betulependule Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

centosinfra-prod-github-app bot commented Mar 25, 2026

Uh oh!

centosinfra-prod-github-app bot commented Mar 25, 2026

Uh oh!

Uh oh!

Uh oh!

nforro Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

betulependule Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

betulependule Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

nforro Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

centosinfra-prod-github-app bot commented Mar 25, 2026

Uh oh!

nforro Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants