[6/6] feat(template): execute COPY/ADD steps, WORKDIR dirs, e2b docs - #74
[6/6] feat(template): execute COPY/ADD steps, WORKDIR dirs, e2b docs#74JoyboyBrian wants to merge 6 commits into
Conversation
|
🔍 OpenCodeReview found 19 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Verification does not consume the grant: consumption happens only after | ||
| // the archive has been durably published, so an upload that fails while | ||
| // streaming, staging, or storing the body stays retryable with this URL. | ||
| let now_unix = chrono::Utc::now().timestamp(); | ||
| let authorized = match store | ||
| .verify_upload_grant(&query.token, &template_id, &hash, query.expires, now_unix) | ||
| .await |
There was a problem hiding this comment.
[security · high]
The verify-then-claim flow lets every concurrent request using the same bearer token pass verification and stage up to files_max_upload_mib. A single leaked or reused URL can therefore create an unbounded number of large temporary files and storage imports before one request finally claims the grant. Single-use enforcement after publication protects the response semantics but not disk, bandwidth, or worker resources. Use an atomic grant state transition (for example, available -> uploading with a bounded lease), release it on pre-publication failure, and consume it after successful publication so only one upload can stream per token.
| if let Err(error) = file.flush().await { | ||
| warn!(error = %error, "failed to flush staged build archive"); | ||
| return Err(error_response( | ||
| StatusCode::INTERNAL_SERVER_ERROR, | ||
| "failed to stage build archive", | ||
| )); | ||
| } | ||
| Ok(total) |
There was a problem hiding this comment.
[bug · medium]
A zero-length body is treated as a successful archive and then permanently published under a first-write-wins cache key. An accidental empty PUT can consume the grant and poison this hash until retention removes it; subsequent correct uploads cannot replace it. Reject total == 0 before import (and ideally validate that the completed file is a structurally valid supported tar archive before publishing it).
Suggestion:
| if let Err(error) = file.flush().await { | |
| warn!(error = %error, "failed to flush staged build archive"); | |
| return Err(error_response( | |
| StatusCode::INTERNAL_SERVER_ERROR, | |
| "failed to stage build archive", | |
| )); | |
| } | |
| Ok(total) | |
| if total == 0 { | |
| return Err(error_response( | |
| StatusCode::BAD_REQUEST, | |
| "build archive body must not be empty", | |
| )); | |
| } | |
| if let Err(error) = file.flush().await { | |
| warn!(error = %error, "failed to flush staged build archive"); | |
| return Err(error_response( | |
| StatusCode::INTERNAL_SERVER_ERROR, | |
| "failed to stage build archive", | |
| )); | |
| } | |
| Ok(total) |
| let total = match tokio::time::timeout(upload_timeout, consume_body).await { | ||
| Ok(Ok(total)) => total, | ||
| Ok(Err(response)) => return response, | ||
| Err(_) => { | ||
| debug!(template_id, hash, "build archive upload timed out"); |
There was a problem hiding this comment.
[bug · medium]
The configured request timeout only covers reading the client body. store.import below can perform another full file copy or OSS upload with no deadline, so a request can run far beyond files_upload_timeout_secs and hang indefinitely on backend I/O despite the setting being documented as the maximum duration of the upload request. Apply one deadline to the complete staging/import/claim operation (using remaining time after body receipt), while preserving cleanup and retry semantics on timeout.
| self.client | ||
| .put_file(&key, staged) | ||
| .await | ||
| .map_err(|error| RepositoryError::backend("upload build archive", error)) |
There was a problem hiding this comment.
[security · high]
The preceding exists check is not atomic with this unconditional PUT. Two imports can both observe a miss, after which the later PUT replaces the earlier archive, violating the trait's first-write-wins guarantee and potentially changing the input of an in-flight build. Because the key is global and the caller-supplied hash is not verified against staged, a caller can also seed or race arbitrary bytes under a hash later reused by another template. Verify a cryptographic digest of the staged archive against a strict digest-shaped hash before publishing (so racing writes are content-identical), or use storage that provides an atomic create-if-absent operation; otherwise namespace the cache by an authorization boundary and do not claim immutability.
| self.client | ||
| .delete(&key) | ||
| .await | ||
| .map_err(|error| RepositoryError::backend("consume upload grant", error))?; | ||
| Ok(true) |
There was a problem hiding this comment.
[security · high]
This read-authorize-delete sequence does not make the grant single-use. Concurrent requests can both read and authorize the object before either deletion is visible, and an idempotent object-store delete lets both calls return true. The upload handler consequently accepts replayed bearer URLs despite its single-winner assumption, permitting duplicate resource consumption and, together with the non-atomic archive PUT, conflicting content. Use an atomic claim primitive in a transactional/coordination store; if OSS cannot provide one, this backend should not advertise single-use grants (or the API/trait must explicitly model and mitigate replay rather than returning success to every racer).
| fn normalize_src(src: &str) -> String { | ||
| let mut src = src.trim(); | ||
| while let Some(stripped) = src.strip_prefix("./") { | ||
| src = stripped; | ||
| } | ||
| src.trim_end_matches('/').to_string() | ||
| } |
There was a problem hiding this comment.
[bug · medium]
This does not implement Docker's source-path normalization. Docker strips a leading / and removes parent navigation from context-relative sources (for example, COPY /foo /dst and COPY ../foo /dst both select foo), whereas this leaves those prefixes intact. Since archive entries are context-relative, map_entries will reject valid SDK archives with “does not belong”/“does not match”. Normalize source components by discarding root/current/parent prefixes before matching, while preserving glob characters within normal components; add coverage for /foo, ../foo, and dir/../foo.
| let mapped: Vec<String> = if single_file_src { | ||
| vec![if dest_is_dir_hint { |
There was a problem hiding this comment.
[bug · medium]
There is no validation for a glob resolving to multiple source items with a non-directory destination spelling. Docker requires the destination to end in / when a wildcard expands to multiple sources; this implementation instead falls into the glob branch, marks the destination as a directory, creates it, and succeeds (for example, COPY *.txt /renamed with two matches). Reject multi-source expansions unless dest_is_dir_hint is true, rather than silently changing an invalid Docker instruction into a directory copy.
| vec![if dest_is_dir_hint { | ||
| // The base name has to come from the resolved entry: the source | ||
| // may be a pattern, which is never a valid path component. | ||
| join_abs(&dest, base_name(&index[0].path)) | ||
| } else { | ||
| dest.clone() | ||
| }] |
There was a problem hiding this comment.
[bug · high]
A single-file destination is also treated as a directory when that path already exists as a directory in the image, even if the instruction has no trailing slash (for example, COPY tool /usr/local/bin must create /usr/local/bin/tool when /usr/local/bin exists). This host-only decision always rewrites the entry as usr/local/bin, so extraction will fail against the existing directory instead of applying Docker semantics. Preserve enough information for the guest to choose dest/basename when dest is an existing directory, or inspect the guest filesystem before finalizing this mapping; add tests for both existing-directory and absent-path cases.
| let plan = plan_copy_archive( | ||
| &CopyRequest { | ||
| source_tar: archive, |
There was a problem hiding this comment.
[performance · medium]
plan_copy_archive synchronously reads/decompresses the source twice and writes the rewritten tar from this async method. Build contexts can be hundreds of MiB, and the runner uses a current-thread Tokio runtime, so this blocks all sandbox I/O and timers on that runtime for the duration. Move archive planning (including tempfile creation if convenient) into tokio::task::spawn_blocking, passing owned paths/request fields into the closure and preserving the step context when joining it.
| let plan = plan_copy_archive( | ||
| &CopyRequest { | ||
| source_tar: archive, | ||
| src, | ||
| dest, | ||
| workdir: &context.workdir, |
There was a problem hiding this comment.
[bug · high]
A single-file COPY cannot be mapped correctly without checking the guest filesystem. Docker treats an existing directory destination as a directory even when the destination has no trailing slash (for example, COPY a /tmp creates /tmp/a). plan_copy_archive currently relies only on the destination string, so it maps this case to the archive member tmp; extraction then tries to replace the existing /tmp directory with a regular file and fails. Probe the resolved destination in the sandbox before planning (without following unsafe paths), and pass whether it is an existing directory into the planner so the source basename is appended.
1 participant
What
Wire E2B
COPY/ADDexecution end to end: parse build steps, materialize referenced archives, rewrite them through the layer-5 planner, upload them to the build sandbox, extract once withtar -xpf, resolve--chown, enforce aggregate budgets, create WORKDIR directories, and document template builds.e2b-build/01-error-envelopee2b-build/02-alias-rebuilde2b-build/03-build-file-storee2b-build/04-upload-apie2b-build/05-copy-plane2b-build/06-copy-execWhy
The maintainer asked that the full change in #28 be split into smaller reviewable PRs. This is layer 6 of that six-PR stack and completes the user-visible E2B template-build workflow on top of layers 3–5.
Related issue
Related: #28
#28 remains the full reference PR containing the design discussion, prior review history, and two-node E2E evidence.
Scope and non-goals
This layer includes the coupled COPY/ADD build-spec, API mapping, archive materialization, guest upload/extraction, WORKDIR fixes, sandbox trait plumbing, and the complete E2B template-build documentation section. It removes layer 5's temporary
dead_codeallowance. The Copy enum variant couples these files; splitting them further would leave intermediate branches uncompilable.Until the preceding PRs merge, the Files view also shows their commits (this branch stacks on them); this layer itself = the last commit, or JoyboyBrian/AgentENV@e2b-build/05-copy-plan...e2b-build/06-copy-exec.
Design and behavior changes
The builder collects unique referenced file hashes, enforces archive-count and aggregate-size limits, and materializes them through the repository store. The executor resolves optional user/group names inside the build VM, rewrites each archive on the host, creates a missing destination root when needed, uploads through envd, and extracts with ownership/mode preservation and
--no-overwrite-dir. WORKDIR now creates its directory and resolves relative paths from the current workdir.Compatibility and operations
[template_build]budgets introduced in layer 4; no additional settings.tar; no new host dependency or port.Validation
make fmtmake clippymake test-unitcargo clippy --workspace --all-targets --all-features -- -D warningscargo test -p agentenv --libmake -C services test(required whenservices/changes)maketargetCommands and results:
All format, clippy, and library-test commands pass in the Linux verification environment. Focused unit tests cover COPY/ADD request validation, hash collection, archive budgets/materialization, chown resolution, extraction commands, WORKDIR creation/resolution, and executor error paths. The top-tree diff against branch 00 is empty and its +4852/−129 stat matches the rebased-source baseline after the three-line semantic rebase fix. #28 retains the successful two-node E2E template-build evidence.
Skipped checks and reasons: KVM/network-namespace integration suites and benchmarks were not rerun during redistribution; #28 retains the already-completed two-node E2E evidence. No services code changed.
Risks and reviewer notes
The main review surfaces are build-archive resource bounds, guest-path safety delegated to the reviewed planner,
--chownidentity resolution, and extraction semantics. Review this layer through the last commit or compare link. Merge only after layers 1–5.Checklist