[4/4] feat(template): execute COPY/ADD steps and document E2B builds - #74
[4/4] feat(template): execute COPY/ADD steps and document E2B builds#74JoyboyBrian wants to merge 4 commits into
Conversation
|
🔍 OpenCodeReview found 26 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.
0ac4148 to
730cc51
Compare
JoyboyBrian
changed the title
| if let Err(error) = store.import(&hash, &staged_path).await { | ||
| warn!(error = %error, hash, "failed to import build archive"); | ||
| return error_response( | ||
| StatusCode::INTERNAL_SERVER_ERROR, | ||
| "failed to store build archive; the upload can be retried with the same link", | ||
| ); | ||
| } |
There was a problem hiding this comment.
[performance · medium]
files_upload_timeout_secs bounds only consume_body; this import and the following grant operations are outside that timeout. For the OSS backend, put_file, grant reads, and deletion do not have a repository-level deadline, so a stalled object store can keep the request and temporary staging file alive indefinitely. Apply a timeout to the complete publish/claim phase (or ensure every backend operation has an equivalent bounded timeout) and return a controlled error when it expires.
| if let Err(error) = store.import(&hash, &staged_path).await { | ||
| warn!(error = %error, hash, "failed to import build archive"); | ||
| return error_response( | ||
| StatusCode::INTERNAL_SERVER_ERROR, | ||
| "failed to store build archive; the upload can be retried with the same link", | ||
| ); | ||
| } | ||
|
|
||
| // The archive is published, so the claim only enforces single-use: the | ||
| // atomic remove/delete picks a single winner among concurrent replays, and | ||
| // a replay that loses the race is rejected even though the archive it | ||
| // uploaded is stored. `now_unix` is the timestamp taken before the body was | ||
| // read, so a slow but authorized upload is not rejected for aging past the | ||
| // TTL. | ||
| let claimed = match store | ||
| .claim_upload_grant(&query.token, &template_id, &hash, query.expires, now_unix) | ||
| .await | ||
| { | ||
| Ok(claimed) => claimed, | ||
| Err(error) => { | ||
| warn!(error = %error, "failed to claim build-file upload grant"); | ||
| return error_response( | ||
| StatusCode::INTERNAL_SERVER_ERROR, | ||
| "failed to validate upload grant", | ||
| ); | ||
| } | ||
| }; | ||
| if !claimed { | ||
| return error_response( | ||
| StatusCode::UNAUTHORIZED, | ||
| "upload grant is invalid, expired, or already used; request a fresh upload link", | ||
| ); | ||
| } |
There was a problem hiding this comment.
[security · high]
The single-use credential is enforced too late. Two requests can both pass verification and race into this first-write-wins import; because the handler does not verify that the body matches hash, the request that later loses claim_upload_grant can still publish its bytes and determine the archive used by the build. The OSS backend is even less restrictive: its claim is a read followed by an unconditional delete, so concurrent requests can both return claimed = true. Publication must be coupled to an atomic grant reservation/claim (with a retryable state for failed imports), or the received content must be authenticated against the key before it can be published. The comment's claim that atomic delete makes an unclaimed replay harmless is not valid for either backend.
| let body_content = tokio::task::spawn_blocking(move || | ||
| serde_json::to_vec(&body).map_err(|e| { | ||
| error!(error = ?e); | ||
| StatusCode::INTERNAL_SERVER_ERROR | ||
| })).await.unwrap()?; |
There was a problem hiding this comment.
[other · medium]
These spawn_blocking joins are unwrapped in the new handler (both validation above and each response serialization branch). If the blocking pool cannot join because the runtime is shutting down, or if serialization panics, the request handler panics instead of returning an HTTP 5xx response. Please propagate JoinError (for example with await.map_err(...)??) and map it to StatusCode::INTERNAL_SERVER_ERROR, as the handler already does for serialization errors.
| let dest = args | ||
| .get(1) | ||
| .map(|value| value.trim()) | ||
| .filter(|value| !value.is_empty()) |
There was a problem hiding this comment.
[bug · medium]
Preserve the original destination value after checking that it is not blank. Leading/trailing whitespace can be part of a quoted Docker/E2B pathname, but this stores the trimmed value and can therefore copy to a different guest path than requested. Use filter(|value| !value.trim().is_empty()) on the original &String (and apply the same pattern to src) rather than mapping the value to trim().
| Ok(mode) | ||
| }) | ||
| .transpose()?; | ||
| spec = spec.copy(src, dest, files_hash, user, mode); |
There was a problem hiding this comment.
[bug · medium]
Reject arguments beyond the four fields consumed above. TemplateStep.args has no schema-level arity constraint, so this currently accepts malformed or newer payloads while silently dropping every value after mode; that can produce a successful build with different COPY/ADD semantics than the client requested. Validate args.len() <= 4 (and add a regression test) before constructing the step.
| match store.materialize(&hash, &scratch).await { | ||
| Ok(Some(path)) => { | ||
| let size = tokio::fs::metadata(&path) |
There was a problem hiding this comment.
[performance · medium]
The aggregate disk limit is enforced only after materialize has completed. For the OSS backend, this call downloads the entire object into scratch, so an archive that pushes the build over the limit has already consumed that disk space (and a backend/download error may also leave a partial destination). Thus files_max_build_context_mib does not actually bound node-local disk usage during materialization. Pass the remaining byte budget into materialization and enforce it while streaming (removing partial files on error), or query trusted object sizes before downloading and still cap the streamed bytes to handle races/misreported metadata.
| let mapped: Vec<String> = if single_file_src { | ||
| 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]
This treats a single-file destination as a directory only when its syntax ends in / (or /.). Docker also appends the source basename when the resolved destination already exists as a directory. For example, COPY a.txt /app against an image containing directory /app is currently rewritten as member app, and the guest extraction will fail instead of creating /app/a.txt. Determine the destination type in the sandbox before rewriting (or pass that resolved type into this planner) rather than relying only on trailing syntax.
| builder | ||
| .append_data(&mut header, relative, &mut entry) | ||
| .with_context(|| format!("write file entry '{target}'"))?; |
There was a problem hiding this comment.
[performance · medium]
max_total_bytes is enforced only against the source archive. Rewriting can exceed it because every target is prefixed with the unchecked destination/workdir, and tar::Builder emits extra long-name/PAX records for long paths. With many entries, a small accepted source can therefore produce a much larger temporary file and upload, contrary to CopyRequest's documented bound. Track the actual output bytes (for example via a counting/limiting writer) and fail once the rewritten archive exceeds the configured limit; independently limiting destination/workdir length would also reduce amplification.
| script.push_str(&format!( | ||
| "tar -xp --no-overwrite-dir -f {archive} -C /\nrc=$?\nrm -f {archive}\nif [ $rc -ne 0 ]; then exit $rc; fi\n", | ||
| archive = shell_quote(&guest_archive), | ||
| )); |
There was a problem hiding this comment.
[other · medium]
The guest archive is only removed after tar returns. Any failure in the preceding mkdir/chown/chmod block exits the shell before reaching rm, and an error or cancellation from run_command_with_opts likewise leaves the uploaded archive in /tmp. Since the archive can be as large as the configured context limit, repeated failed or cancelled COPY steps can accumulate guest disk usage. Install a shell trap for rm -f before any setup command (and keep the tar exit status), or explicitly perform best-effort cleanup on every Rust error/cancellation path.
Suggestion:
| script.push_str(&format!( | |
| "tar -xp --no-overwrite-dir -f {archive} -C /\nrc=$?\nrm -f {archive}\nif [ $rc -ne 0 ]; then exit $rc; fi\n", | |
| archive = shell_quote(&guest_archive), | |
| )); | |
| script.push_str(&format!( | |
| "trap 'rm -f -- {archive}' EXIT\\n" | |
| "tar -xp --no-overwrite-dir -f {archive} -C /\\n", | |
| archive = shell_quote(&guest_archive), | |
| )); |
| script.push_str(&format!( | ||
| "tar -xp --no-overwrite-dir -f {archive} -C /\nrc=$?\nrm -f {archive}\nif [ $rc -ne 0 ]; then exit $rc; fi\n", | ||
| archive = shell_quote(&guest_archive), | ||
| )); |
There was a problem hiding this comment.
[security · high]
The rewritten archive is extracted directly into /, while the planner permits symlink entries. A user-controlled symlink target (including an absolute target or a ..-relative target) can make later archive members resolve outside the intended COPY destination if the guest tar follows links during extraction, allowing writes to arbitrary image paths. Path-normalizing the member names is not sufficient because symlink targets are resolved by the guest. Reject symlinks (and validate any other link-like entries) when planning, or extract in a way that never follows archive-created/existing symlinks before committing files.
|
Closing: this targets the deferred server-side template-builder |
1 participant
What
Wire E2B
COPYandADDexecution end to end:--chown, preserve requested modes, and extract once withtar;This is the final layer of the active four-PR stack, replayed onto current main at
00ba6cbwithout #70.e2b-build/03-build-file-storee2b-build/04-upload-apie2b-build/05-copy-plane2b-build/06-copy-execWhy
This completes the user-visible E2B template-build workflow on top of #71–#73. The official SDK can upload a local Dockerfile context, reference it by
filesHash, and build a reusable AgentENV template containing the copied files.Related: #28. The original PR remains the full concept/reference discussion.
Scope and non-goals
This layer contains the COPY/ADD build-spec mapping, archive materialization, guest upload/extraction, sandbox plumbing, documentation, and E2E coverage. It removes #73's temporary
dead_codeallowance.It does not introduce or claim ownership of WORKDIR behavior. WORKDIR directory creation and relative resolution already exist on current main; the E2E uses that existing behavior only to verify copied files from a realistic Dockerfile.
Existing-alias rebuild remains unsupported. Use a new alias or delete the existing template before rebuilding; #70 is parked pending the alias refactor.
Exact layer diff: e2b-build/05-copy-plan...e2b-build/06-copy-exec
Design
filesHashreferences, enforces count/aggregate size limits, and materializes archives through the repository store.--no-overwrite-dir.Compatibility and operations
[template_build]limits introduced by [2/4] feat(api): E2B build-context upload endpoints and configuration #72; no additional setting is added here.tar; no new host dependency or port.Validation
Fresh-template Linux/KVM E2E on
devmachine:The E2E uses a newly generated alias and temporary local build context, so it exercises the supported path independently of the parked alias-rebuild work.
Reviewer notes
The main review surfaces are archive resource bounds, path safety at the planner boundary,
--chownidentity resolution, and extraction semantics. Merge after #71–#73; #70 is not required.