Skip to content

Bazel: Add lfs_archives function #15

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
anurag6569201 wants to merge 1 commit into
base: qa/agent-github-codeql/pr-15-22483/base
Choose a base branch
from
Open

Bazel: Add lfs_archives function #15

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 46 additions & 21 deletions misc/bazel/lfs.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,29 +28,42 @@ def lfs_smudge(repository_ctx, srcs, *, extract = False, stripPrefix = None, exe
res = repository_ctx.download([], src.basename, sha256 = info, allow_fail = True, executable = executable)
if not res.success:
remote.append(src)
if remote:
infos = probe(remote)
for src, info in zip(remote, infos):
sha256, _, url = info.partition(" ")
repository_ctx.report_progress("downloading remote %s" % src.basename)
repository_ctx.download(url, src.basename, sha256 = sha256, executable = executable)
if extract:
for src in srcs:
repository_ctx.report_progress("extracting %s" % src.basename)
repository_ctx.extract(src.basename, stripPrefix = stripPrefix)
repository_ctx.delete(src.basename)
if remote:

@shipwright-agent shipwright-agent Bot Sep 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

infos = probe(remote)
for src, info in zip(remote, infos):
sha256, _, url = info.partition(" ")
repository_ctx.report_progress("downloading remote %s" % src.basename)
repository_ctx.download(url, src.basename, sha256 = sha256, executable = executable)
if extract:
for src in srcs:
repository_ctx.report_progress("extracting %s" % src.basename)
repository_ctx.extract(src.basename, stripPrefix = stripPrefix)
repository_ctx.delete(src.basename)

def _download_and_extract_lfs(repository_ctx):
def _add_build_file(repository_ctx):
attr = repository_ctx.attr
src = repository_ctx.path(attr.src)
if attr.build_file_content and attr.build_file:
fail("You should specify only one among build_file_content and build_file for rule @%s" % repository_ctx.name)
lfs_smudge(repository_ctx, [src], extract = True, stripPrefix = attr.strip_prefix)
if attr.build_file_content:
repository_ctx.file("BUILD.bazel", attr.build_file_content)
elif attr.build_file:
repository_ctx.symlink(attr.build_file, "BUILD.bazel")

def _download_and_extract_lfs_archive(repository_ctx):
attr = repository_ctx.attr
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):

@shipwright-agent shipwright-agent Bot Sep 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@shipwright-agent shipwright-agent Bot Sep 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for src in repository_ctx.attr.srcs:
lfs_smudge(
repository_ctx,
[repository_ctx.path(src)],
extract = True,
stripPrefix = repository_ctx.attr.strip_prefix,
)
_add_build_file(repository_ctx)

def _download_lfs(repository_ctx):
attr = repository_ctx.attr
if int(bool(attr.srcs)) + int(bool(attr.dir)) != 1:
Expand Down Expand Up @@ -83,18 +96,30 @@ def _download_lfs(repository_ctx):
'alias(name = "file", actual = "//:%s", visibility = ["//visibility:public"])\n' % name,
)

_lfs_archive_attrs = {

@shipwright-agent shipwright-agent Bot Sep 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"build_file": attr.label(doc = "The file to use as the BUILD file for this repository. " +
"Either build_file or build_file_content can be specified, but not both."),
"build_file_content": attr.string(doc = "The content for the BUILD file for this repository. " +
"Either build_file or build_file_content can be specified, but not both."),
"strip_prefix": attr.string(default = "", doc = "A directory prefix to strip from the extracted files."),
}

lfs_archive = repository_rule(
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,

@shipwright-agent shipwright-agent Bot Sep 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@shipwright-agent shipwright-agent Bot Sep 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

attrs = {
"src": attr.label(mandatory = True, doc = "Local path to the LFS archive to extract."),
"build_file_content": attr.string(doc = "The content for the BUILD file for this repository. " +
"Either build_file or build_file_content can be specified, but not both."),
"build_file": attr.label(doc = "The file to use as the BUILD file for this repository. " +
"Either build_file or build_file_content can be specified, but not both."),
"strip_prefix": attr.string(default = "", doc = "A directory prefix to strip from the extracted files. "),
},
} | _lfs_archive_attrs,
)

lfs_archives = repository_rule(
doc = "Overlay the contents from on-demand LFS archives. The corresponding paths should be added to be ignored " +
"in `.lfsconfig`.",
implementation = _download_and_extract_lfs_archives,
attrs = {
"srcs": attr.label_list(doc = "Local paths to the LFS archives to extract in order.", mandatory = True),
} | _lfs_archive_attrs,
)

lfs_files = repository_rule(
Expand Down