chore: replace git dependency for http - #2490
Open
pindo696 wants to merge 3 commits into
Open
Conversation
Reviewer's GuideThe PR removes Git-based repository access from production workflows by introducing authenticated HTTP archive downloads and extraction, updates FedRAMP image construction and deployment configuration to use archive URLs, retains GitPython only for tests, and adds focused coverage for the new import and error-handling paths. Sequence diagram for HTTP archive repository synchronizationsequenceDiagram
participant Job as rules_git_sync
participant Config
participant Downloader as ArchiveDownloader
participant GitHost as GitArchiveHost
participant Filesystem
participant Database
Job->>Config: read content_archive_url and playbooks_archive_url
Job->>Downloader: download_and_extract(extract_to)
Downloader->>GitHost: GET archive URL with Authorization
GitHost-->>Downloader: tar.gz or zip archive
Downloader->>Downloader: extract_commit_sha_from_archive(archive_path)
Downloader->>Filesystem: extract archive and locate directory
Downloader-->>Job: extracted_dir_path and commit_sha
Job->>Filesystem: move extracted repository to target_dir
Job->>Database: store_versions(commit_sha, archive_url)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
pindo696
force-pushed
the
replace-git-dependency
branch
from
September 2, 2026 12:10
8f6598b to
233fa36
Compare
Contributor
Author
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues
Fixed security issues:
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="taskomatic/jobs/archive_downloader.py" line_range="71-72" />
<code_context>
+
+ try:
+ if archive_type == "zip":
+ with zipfile.ZipFile(archive_path, "r") as z:
+ if sha := z.comment.decode("utf-8").strip():
+ LOGGER.debug("Extracted commit SHA from ZIP comment: %s", sha[:8])
</code_context>
<issue_to_address>
**🚨 issue (security):** ZIP archives are extracted without validating member paths, so a repository archive containing `../` entries writes files outside `extract_to` and can overwrite arbitrary files writable by the taskomatic process.
**Triggers:** When a downloaded ZIP archive contains path-traversal entries.
**Suggested fix:** Validate every ZIP member resolves beneath `extract_to` before extraction, or use a safe extraction routine equivalent to the tar `filter="data"` protection.
```suggestion
with zipfile.ZipFile(archive_path, "r") as z:
extract_root = os.path.realpath(extract_to)
for member in z.infolist():
member_path = os.path.realpath(os.path.join(extract_root, member.filename))
if os.path.commonpath((extract_root, member_path)) != extract_root:
raise ValueError(f"Unsafe ZIP member path: {member.filename}")
z.extractall(extract_root)
```
</issue_to_address>
### Comment 2
<location path="taskomatic/jobs/archive_downloader.py" line_range="78" />
<code_context>
- python312 libpq shadow-utils git-core postgresql && \
+ python312 libpq shadow-utils postgresql && \
microdnf clean all
# Copy pg_repack 1.5.2 from builder stage
</code_context>
<issue_to_address>
**issue (bug_risk):** Unsupported archive extensions are logged but do not raise an error, so `download_and_extract` treats the empty extraction directory as a successful repository, returns a fallback hash, and `download_repos` can report success before `sync` runs against missing content.
**Triggers:** When an archive URL does not end in `.tar.gz` or `.zip`, including URLs whose download endpoint has no archive suffix.
**Suggested fix:** Raise an exception for unsupported archive types and verify that extraction produced the expected repository directory before returning success.
</issue_to_address>
### Comment 3
<location path="Dockerfile" line_range="78" />
<code_context>
+ # VMaaS assets
+ curl -L "${VMAAS_ARCHIVE_URL}" -o /tmp/vmaas.tar.gz && \
+ tar -xzf /tmp/vmaas.tar.gz -C /tmp && \
+ mv /tmp/vmaas-assets-master /engine/vmaas_assets_git && \
+ rm /tmp/vmaas.tar.gz && \
+ \
</code_context>
<issue_to_address>
**issue (bug_risk):** The static-assets build assumes every archive expands to a fixed `*-master` directory, so configuring an archive URL for another branch, tag, commit, or provider-generated root name makes `mv` fail and prevents the image from building.
**Triggers:** When any `*_ARCHIVE_URL` points to an archive generated from a ref other than the default `master` layout.
**Suggested fix:** Discover the single extracted top-level directory instead of hardcoding the archive root name, or derive the expected root from the configured URL/ref.
```suggestion
mv "/tmp/$(tar -tzf /tmp/vmaas.tar.gz | sed 's#/.*##' | head -n 1)" /engine/vmaas_assets_git && \
```
</issue_to_address>
### Comment 4
<location path="Dockerfile" line_range="76-77" />
<code_context>
+ \
+ # VMaaS assets
+ curl -L "${VMAAS_ARCHIVE_URL}" -o /tmp/vmaas.tar.gz && \
+ tar -xzf /tmp/vmaas.tar.gz -C /tmp && \
+ mv /tmp/vmaas-assets-master /engine/vmaas_assets_git && \
+ rm /tmp/vmaas.tar.gz && \
</code_context>
<issue_to_address>
**issue (bug_risk):** The static-assets image build always invokes `tar -xzf`, so a configured ZIP archive—which `ArchiveDownloader` explicitly supports—fails extraction and aborts the build.
**Triggers:** When a static asset archive URL is configured with a `.zip` archive.
**Suggested fix:** Select the extraction command from the archive extension, or restrict and validate the build arguments to tar.gz URLs.
```suggestion
case "${VMAAS_ARCHIVE_URL}" in \
*.tar.gz) : ;; \
*) echo "VMAAS_ARCHIVE_URL must reference a .tar.gz archive" >&2; exit 1 ;; \
esac && \
curl -L "${VMAAS_ARCHIVE_URL}" -o /tmp/vmaas.tar.gz && \
tar -xzf /tmp/vmaas.tar.gz -C /tmp && \
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+71
to
+72
| with zipfile.ZipFile(archive_path, "r") as z: | ||
| z.extractall(extract_to) |
There was a problem hiding this comment.
🚨 issue (security): ZIP archives are extracted without validating member paths, so a repository archive containing ../ entries writes files outside extract_to and can overwrite arbitrary files writable by the taskomatic process.
Triggers: When a downloaded ZIP archive contains path-traversal entries.
Suggested fix: Validate every ZIP member resolves beneath extract_to before extraction, or use a safe extraction routine equivalent to the tar filter="data" protection.
Suggested change
| with zipfile.ZipFile(archive_path, "r") as z: | |
| z.extractall(extract_to) | |
| with zipfile.ZipFile(archive_path, "r") as z: | |
| extract_root = os.path.realpath(extract_to) | |
| for member in z.infolist(): | |
| member_path = os.path.realpath(os.path.join(extract_root, member.filename)) | |
| if os.path.commonpath((extract_root, member_path)) != extract_root: | |
| raise ValueError(f"Unsafe ZIP member path: {member.filename}") | |
| z.extractall(extract_root) |
Comment on lines
+76
to
+77
| curl -L "${VMAAS_ARCHIVE_URL}" -o /tmp/vmaas.tar.gz && \ | ||
| tar -xzf /tmp/vmaas.tar.gz -C /tmp && \ |
There was a problem hiding this comment.
issue (bug_risk): The static-assets image build always invokes tar -xzf, so a configured ZIP archive—which ArchiveDownloader explicitly supports—fails extraction and aborts the build.
Triggers: When a static asset archive URL is configured with a .zip archive.
Suggested fix: Select the extraction command from the archive extension, or restrict and validate the build arguments to tar.gz URLs.
Suggested change
| curl -L "${VMAAS_ARCHIVE_URL}" -o /tmp/vmaas.tar.gz && \ | |
| tar -xzf /tmp/vmaas.tar.gz -C /tmp && \ | |
| case "${VMAAS_ARCHIVE_URL}" in \ | |
| *.tar.gz) : ;; \ | |
| *) echo "VMAAS_ARCHIVE_URL must reference a .tar.gz archive" >&2; exit 1 ;; \ | |
| esac && \ | |
| curl -L "${VMAAS_ARCHIVE_URL}" -o /tmp/vmaas.tar.gz && \ | |
| tar -xzf /tmp/vmaas.tar.gz -C /tmp && \ |
pindo696
force-pushed
the
replace-git-dependency
branch
from
September 2, 2026 15:25
233fa36 to
d4e36f7
Compare
pindo696
force-pushed
the
replace-git-dependency
branch
from
September 10, 2026 07:39
d4e36f7 to
88b8d26
Compare
pindo696
marked this pull request as ready for review
September 10, 2026 08:59
There was a problem hiding this comment.
Hey - I've found 3 issues
Fixed security issues:
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="taskomatic/jobs/archive_downloader.py" line_range="55-57" />
<code_context>
+ LOGGER.warning("Could not extract commit SHA: %s", err)
+
+ # Fallback: SHA256 hash of archive
+ LOGGER.warning("No metadata SHA, using SHA256 hash of archive")
+ with open(archive_path, "rb") as f:
+ return hashlib.sha256(f.read()).hexdigest()[:40]
+
+
</code_context>
<issue_to_address>
**issue:** When an archive has no PAX or ZIP comment, `extract_commit_sha_from_archive` returns the first 40 characters of the archive's SHA256 digest rather than the repository commit SHA promised by the function and used as the imported content version.
**Triggers:** When GitHub or another archive provider does not include commit metadata in the generated archive.
**Suggested fix:** Derive the commit from the archive URL or provider metadata, or explicitly store and document an archive digest instead of treating it as a commit SHA.
</issue_to_address>
### Comment 2
<location path="Dockerfile" line_range="69" />
<code_context>
+ \
+ # VMaaS assets
+ curl -L -H "Authorization: Bearer ${GIT_TOKEN}" "${VMAAS_ARCHIVE_URL}" -o /tmp/vmaas${ARCHIVE_EXT} && \
+ tar -xzf /tmp/vmaas${ARCHIVE_EXT} -C /tmp && \
+ mv /tmp/vmaas-assets-* /engine/vmaas_assets_git && \
+ rm /tmp/vmaas${ARCHIVE_EXT} && \
</code_context>
<issue_to_address>
**issue (bug_risk):** The image build declares `ARCHIVE_EXT` and the downloader supports both `.tar.gz` and `.zip`, but the static-asset build always invokes `tar -xzf`; setting `ARCHIVE_EXT` to `.zip` downloads a ZIP file and then fails extraction.
**Triggers:** When a deployment overrides `ARCHIVE_EXT` to use the advertised ZIP format.
**Suggested fix:** Select the extraction command based on `ARCHIVE_EXT`, or remove ZIP from the supported formats/configuration for the image build.
```suggestion
ENV ARCHIVE_EXT=".tar.gz"
```
</issue_to_address>
### Comment 3
<location path="taskomatic/jobs/archive_downloader.py" line_range="22-23" />
<code_context>
+SUPPORTED_ARCHIVE_FORMATS = {".tar.gz": "tarball", ".zip": "zip"}
+
+
+def _get_archive_type(archive_path: str) -> str:
+ """Detect archive type from path. Returns 'tarball', 'zip', or None"""
+ for ext, archive_type in SUPPORTED_ARCHIVE_FORMATS.items():
+ if archive_path.endswith(ext):
+ return archive_type
+ return None
+
+
</code_context>
<issue_to_address>
**nitpick (bug_risk):** `_get_archive_type` is annotated as returning `str` but returns `None` for every unsupported extension, so its declared return contract is false and strict type checking reports an incompatible return value.
**Triggers:** When static type checking is enabled or a caller relies on the declared non-null return type.
**Suggested fix:** Change the annotation to `str | None` and update the docstring to match.
```suggestion
def _get_archive_type(archive_path: str) -> str | None:
"""Detect archive type from path. Returns 'tarball', 'zip', or None."""
```
</issue_to_address>
Replaced git dependency for playboooks and content import with http. This includes change from git clone to downloading an archive. Updated requirements, keeping git for dev only as tests requires them. RHINENG-30176 and RHINENG-30177
pindo696
force-pushed
the
replace-git-dependency
branch
from
September 10, 2026 15:33
b5c567d to
2a7aea7
Compare
jdobes
requested changes
Sep 11, 2026
| return commit_sha | ||
| except Exception as err: | ||
| LOGGER.exception("Error downloading repo: %s", err) | ||
| from common.utils import EXCEPTION_COUNT |
| return None | ||
|
|
||
| try: | ||
| # Use tmp dir for download and extract |
Member
There was a problem hiding this comment.
This is already in tmp dir, no need to create another one
| # Try all supported archive formats | ||
| for ext in SUPPORTED_ARCHIVE_FORMATS.keys(): | ||
| archive_path = os.path.join(STATIC_GITS_PATH, f"{repo_name}{ext}") | ||
| if os.path.exists(archive_path): |
Member
There was a problem hiding this comment.
This will never succeed, in static repo dir is unpacked repository archive (see static assets in dockerfile)
| # Fallback: SHA256 hash of archive | ||
| LOGGER.warning("No metadata SHA, using SHA256 hash of archive") | ||
| with open(archive_path, "rb") as f: | ||
| return hashlib.sha256(f.read()).hexdigest()[:40] |
Member
There was a problem hiding this comment.
Why truncating remaining 24 chars?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaced git dependency for playboooks and content import with http. This includes change from git clone to downloading an archive. Updated requirements, keeping git for dev only as tests requires them. RHINENG-30176 and RHINENG-30177
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Replace Git-based repository access with HTTP archive downloads for application content, playbooks, and static assets.
New Features:
Enhancements:
Build:
Deployment:
Tests:
Chores: