Remove hardcoded list of supported Fedora CI tests#3039
Remove hardcoded list of supported Fedora CI tests#3039betulependule wants to merge 8 commits intopackit:mainfrom
Conversation
betulependule
requested review from
a team,
lbarcziova,
mfocko and
nforro
as code owners
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 50s |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
| """ | ||
| if isinstance(self.event, abstract.comment.CommentEvent): | ||
| arguments = parse_comment( | ||
| arguments = self.parse_comment( |
There was a problem hiding this 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.
| 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) |
There was a problem hiding this comment.
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)
|
Nice! It would be great if this also considered the filters (in the packit-service/packit_service/worker/handlers/testing_farm.py Lines 558 to 560 in 1d3ed53 However I'm not sure how to elegantly implement this. It would probably require implementing the filtering in a different way. |
I will take a look and see if I can think of a good way to implement this. Thanks. 🙏 |
|
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. |
betulependule
force-pushed
the
hardcoded-values-comment-parser
branch
2 times, most recently
from
3026371 to
82c49af
Compare
|
❌ pre-commit FAILURE in 1m 53s |
|
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 |
betulependule
force-pushed
the
hardcoded-values-comment-parser
branch
from
82c49af to
5c79adc
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 53s |
betulependule
force-pushed
the
hardcoded-values-comment-parser
branch
from
5c79adc to
20e3a5e
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 52s |
|
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. |
betulependule
force-pushed
the
hardcoded-values-comment-parser
branch
from
08cd220 to
c103735
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 50s |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 52s |
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.
betulependule
force-pushed
the
hardcoded-values-comment-parser
branch
from
187c4d8 to
39beb5d
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 51s |
The two functions `is_project_in_tests_namespace` and `is_fmf_configured` have been removed and replaced with two new checkers `IsFMFConfigMissing` and `IsProjectInTestsNamespace` in order to improve consistency of the code. Additionally, the `@implements_fedora_ci_test` decorator no longer expects a single optional callable `skipif`. Instead, it is possible to specify a list of Checkers, making it possible to specify multiple "skipif" conditions. Changes have been made to the `mixin.py` in order to resolve the circular dependency created because of the aforementioned changes.
betulependule
force-pushed
the
hardcoded-values-comment-parser
branch
from
39beb5d to
4eaefe7
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 52s |
3 participants
Fixes #3038