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)

nforro · 2026-03-04T11:56:27Z

packit_service/worker/jobs.py

+            # tests expect invalid syntax comments be ignored
+            logger.debug(
+                f"Comment {comment} uses unexpected syntax or contains unsupported commands. "
+                "It will be ignored.",
+            )
+            return ParsedComment()


Wouldn't some feedback be nice? Implementing a response comment should be simple, we already provide feedback for accepted retrigger tasks:

packit-service/packit_service/worker/jobs.py

Line 1339 in 1d3ed53

self.event.pull_request_object.comment(message)

We could either forward the error from the SystemExit exception or use parser.parse_known_args() and report unknown arguments.

Yes, it would definitely be ideal to respond in a comment when this kind of error occurs. Should I open a new PR regarding this?

Yes please 🙏

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

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.

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"]

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

packit_service/worker/helpers/testing_farm.py

+    @implements_fedora_ci_test(
+        "installability",
+        skipif=lambda _, project, __: DownstreamTestingFarmJobHelper.is_project_in_tests_namespace(
+            project
+        ),
+    )


This is a nice idea, I like it. Just thinking if maybe we could provide aliases for these, something like:

in_tests_namespace = ( lambda _, project, __: DownstreamTestingFarmJobHelper.is_project_in_tests_namespace(project) )

Then one could use skipif=in_tests_namespace. Similarly for is_fmf_configured. It would also make combinations easier to read (skipif=in_tests_namespace and not is_fmf_configured).

(I hope I'm not proposing an invalid syntax here 😅).

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

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 added 2 commits March 23, 2026 10:50

Remove hardcoded list of supported Fedora CI tests

a094129

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

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

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!

nforro Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

betulependule Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

nforro Mar 4, 2026

Choose a reason for hiding this comment

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!

nforro Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants