Allow beakerlib libraries with clashing repo and name

[LecrisUT]

The original code should already have allowed for libraries with different name part but same repo, but as shown in practice that did not happen. This is because the string construction in of str(Beakerlib) only took the last part of the name, disallowing /foo/common, /bar/common. But beakerlib does not seem to actually have that restriction.

Summary of changes:

• Reworked how the library's repo are cloned allowing for a big degree of collision
• Allow for whole libraries to collide and take the first definition
• Adjust the order of library fetching to account for the relaxed condition above
• Moved the non-existing-url handling to the git_clone utility
• Adjusted the display of the beakerlib library to display the full (repo)/(name) as it would be used in rlImport

Pull Request Checklist

• implement the feature
• extend the test coverage
  (Better repository for testing needed: Add a beakerlib test case for tmt#4440 tests#23)
• include a release note

Depends on #4499
Closes #4440

[psss]

Seems there are some failing tests:

Library 'openssl/certgen (https://github.com/beakerlib/openssl)' conflicts with previously fetched 'openssl/certgen (https://github.com/beakerlib/openssl#master)'.
:: [ 13:55:37 ] :: [   FAIL   ] :: Command 'tmt run -arvvvddd plan --name 'rpm|fmf|nick|duplicate'' (Expected 0, got 2)

[LecrisUT]

Seems there are some failing tests:

Library 'openssl/certgen (https://github.com/beakerlib/openssl)' conflicts with previously fetched 'openssl/certgen (https://github.com/beakerlib/openssl#master)'.
:: [ 13:55:37 ] :: [   FAIL   ] :: Command 'tmt run -arvvvddd plan --name 'rpm|fmf|nick|duplicate'' (Expected 0, got 2)

Let's postpone this one. The test failures are related to the feature that I was planning to address later one, but it seems I cannot fix the tests without that feature.

• Allow for whole libraries to collide and take the first definition Postponed

• Adjust the order of library fetching to account for the relaxed condition above Postponed

[psss]

Let's postpone this one. The test failures are related to the feature that I was planning to address later one, but it seems I cannot fix the tests without that feature.

Ack, makes sense.

[skycastlelily]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the beakerlib library fetching logic, addressing clashing library names and centralizing non-existent URL handling. However, it introduces path traversal vulnerabilities where untrusted input from library identifiers (repo, path, name) is used to construct filesystem paths without proper validation, potentially allowing arbitrary file reads or writes. There is also a potential Python compatibility issue and a bug in the new caching logic that needs to be addressed.

[gemini-code-assist]

This code block is susceptible to a path traversal vulnerability. The self.repo and self.fmf_node_path attributes, derived from untrusted library identifiers, can be manipulated by an attacker using absolute paths or directory traversal sequences (..). This could cause local_repo_path or local_library_path to point to arbitrary locations on the system. Furthermore, the calculation of local_library_path as the cache key is inconsistent with the destination path calculation. For BeakerLibFromPath, self.fmf_node_path can be an absolute path, leading to a cache key outside the intended workdir. The cache key should consistently represent the destination path within the workdir to mitigate this security risk and ensure correct caching.

        if isinstance(self, BeakerLibFromPath):
            # BeakerLibFromPath copies to a directory named after the library name
            relative_library_path = Path(self.name.strip('/'))
        else:
            # BeakerLibFromUrl uses the fmf_node_path
            relative_library_path = self.fmf_node_path.unrooted()

        local_library_path = local_repo_path / relative_library_path

[LecrisUT]

/packit retest-failed

[happz]

The filename probably can be renamed now.

[happz]

It'd be nice to share why they aren't...

[LecrisUT]

This is partly due to the covariant/invariant type-definitions. We could probably detangle it, but would be more complicated patch

[happz]

Yeah, I guessed it might be related. Would you mind mentioning the reason in the comment? I doubt I would be able to guess it in six months.

[happz]

Doesn't really show anything, IIUIC. Maybe ref_formatted or something similar? Plus, it could be a property`, seems to take no input.