Bazel: Add lfs_archives function - #15
anurag6569201 wants to merge 1 commit into
Conversation
Source PR: github#22483 Source head: 7814be5
⛔ Shipwright · BlockedRecommendation: do not merge PR #15 · Tier
Findings (7)
Fireworks usage: 7,391 input · 863 output · 8,254 total tokens · $0.0022 · 17s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
| lfs_smudge(repository_ctx, [repository_ctx.path(attr.src)], extract = True, stripPrefix = attr.strip_prefix) | ||
| _add_build_file(repository_ctx) | ||
|
|
||
| def _download_and_extract_lfs_archives(repository_ctx): |
There was a problem hiding this comment.
Shipwright · CRITICAL
The new '_download_and_extract_lfs_archives' function calls 'lfs_smudge' once per source archive, and each call performs 'repository_ctx.extract' followed by 'repository_ctx.delete
Impact: The new '_download_and_extract_lfs_archives' function calls 'lfs_smudge' once per source archive, and each call performs 'repository_ctx.extract' followed by 'repository_ctx.delete'. If two archives in 'srcs' contain files with the same relative path, the second extraction will silently overwrite the first, producing a non-deterministic or corrupted repository state depending on archive ordering. There is no collisi…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| lfs_smudge(repository_ctx, [repository_ctx.path(attr.src)], extract = True, stripPrefix = attr.strip_prefix) | ||
| _add_build_file(repository_ctx) | ||
|
|
||
| def _download_and_extract_lfs_archives(repository_ctx): |
There was a problem hiding this comment.
Shipwright · CRITICAL
'_download_and_extract_lfs_archives' applies the same 'strip_prefix' to every archive in 'srcs'.
Impact: '_download_and_extract_lfs_archives' applies the same 'strip_prefix' to every archive in 'srcs'. If the archives have different internal top-level directory structures, extraction will fail or place files in unexpected locations. The singular 'strip_prefix' attribute cannot correctly handle heterogeneous archives, making the rule unusable for realistic multi-archive overlays.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| doc = "Export the contents from an on-demand LFS archive. The corresponding path should be added to be ignored " + | ||
| "in `.lfsconfig`.", | ||
| implementation = _download_and_extract_lfs, | ||
| implementation = _download_and_extract_lfs_archive, |
There was a problem hiding this comment.
Shipwright · HIGH
The new rule 'lfs_archives' has no documentation explaining the ordering semantics of 'srcs' (e.g., later archives override earlier ones, or extraction order matters).
Impact: The new rule 'lfs_archives' has no documentation explaining the ordering semantics of 'srcs' (e.g., later archives override earlier ones, or extraction order matters). A future maintainer cannot know whether file collisions are intentional or a bug without reading the implementation.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| 'alias(name = "file", actual = "//:%s", visibility = ["//visibility:public"])\n' % name, | ||
| ) | ||
|
|
||
| _lfs_archive_attrs = { |
There was a problem hiding this comment.
Shipwright · HIGH
The shared '_lfs_archive_attrs' dictionary is defined at module level and merged into two different repository rules using the '|' operator.
Impact: The shared '_lfs_archive_attrs' dictionary is defined at module level and merged into two different repository rules using the '|' operator. This pattern is uncommon in Starlark and may confuse contributors who expect attributes to be declared inline. Additionally, if '_lfs_archive_attrs' is mutated elsewhere, both rules would be affected.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| repository_ctx.report_progress("extracting %s" % src.basename) | ||
| repository_ctx.extract(src.basename, stripPrefix = stripPrefix) | ||
| repository_ctx.delete(src.basename) | ||
| if remote: |
There was a problem hiding this comment.
Shipwright · HIGH
The refactor changed indentation of the 'if remote:' and 'if extract:' blocks from 8 spaces to 4 spaces.
Impact: The refactor changed indentation of the 'if remote:' and 'if extract:' blocks from 8 spaces to 4 spaces. In Starlark, indentation is syntactically significant, and inconsistent indentation can cause parse errors or alter block nesting. The diff shows the 'if remote:' block now at the same indentation level as the preceding 'for' loop body, which may be a formatting artifact but must be verified against the actual fi…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| doc = "Export the contents from an on-demand LFS archive. The corresponding path should be added to be ignored " + | ||
| "in `.lfsconfig`.", | ||
| implementation = _download_and_extract_lfs, | ||
| implementation = _download_and_extract_lfs_archive, |
There was a problem hiding this comment.
Shipwright · HIGH
The 'lfs_archives' rule accepts a list of arbitrary local paths via 'srcs' and extracts them into the repository.
Impact: The 'lfs_archives' rule accepts a list of arbitrary local paths via 'srcs' and extracts them into the repository. If an attacker can influence the 'srcs' list (e.g., via a malicious 'WORKSPACE' or 'MODULE.bazel' dependency), they could cause arbitrary file extraction into the build workspace, potentially overwriting build files or injecting malicious content. There is no validation that the archives are trusted or t…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
1 participant
This pulls out some generic stuff from the Swift static runtime into a generic location.
Internal clean-up PR will follow once this is in.
Source merge-base:
b0fa3e770386f18fd4d63225301ea4c41e7a5721Source head:
7814be5ff2eb3547a3793a9fd4b2d5646b57fa9b