[5/6] feat(template): host-side COPY plan (archive rewrite to guest paths) - #73
[5/6] feat(template): host-side COPY plan (archive rewrite to guest paths)#73JoyboyBrian wants to merge 5 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}
| let validation = tokio::task::spawn_blocking(move || { | ||
| templates_template_id_files_hash_get_validation(path_params) | ||
| }) | ||
| .await | ||
| .unwrap(); |
There was a problem hiding this comment.
[bug · medium]
This unwrap() can panic the request handler if the blocking task fails with a JoinError (for example, if it panics or is cancelled), instead of returning an HTTP error. The same .await.unwrap() pattern is repeated for JSON serialization in every response branch below. Propagate/map the JoinError to StatusCode::INTERNAL_SERVER_ERROR and log it; ideally fix the server-code generator so generated handlers consistently avoid these production-path panics.
Suggestion:
| let validation = tokio::task::spawn_blocking(move || { | |
| templates_template_id_files_hash_get_validation(path_params) | |
| }) | |
| .await | |
| .unwrap(); | |
| let validation = tokio::task::spawn_blocking(move || { | |
| templates_template_id_files_hash_get_validation(path_params) | |
| }) | |
| .await | |
| .map_err(|e| { | |
| error!(error = ?e, "validation task failed"); | |
| StatusCode::INTERNAL_SERVER_ERROR | |
| })?; |
| #[config(default = 4096u64)] | ||
| pub files_max_context_mib: u64, | ||
| /// Cap on the combined on-disk size of all build-context archives one | ||
| /// build spec may reference, in MiB. | ||
| #[config(default = 4096u64)] | ||
| pub files_max_build_context_mib: u64, |
There was a problem hiding this comment.
[security · medium]
These two advertised resource limits are not consumed anywhere in src/ (only their declarations, validation, and tests reference them). Consequently, compressed archives are accepted without the configured expansion bound being applied, and builds have no aggregate context-size cap. Wire these values into archive planning/build execution before exposing the upload feature; otherwise operators may rely on controls that currently provide no protection.
| #[config(env = "AENV_TEMPLATE_BUILD_PUBLIC_BASE_URL", parse_env = parse_trimmed_string)] | ||
| pub public_base_url: Option<String>, |
There was a problem hiding this comment.
[security · high]
Leaving this optional makes the security-sensitive default depend on the request Host: the upload-link implementation builds http://{host}/...?...token=..., and the SDK then follows that URL with the build archive. If an untrusted client or intermediary can influence Host, it can redirect both the bearer token and source archive to an attacker-controlled origin; the fixed http scheme also exposes them on TLS-fronted deployments. Require an explicit external base URL whenever this endpoint is enabled, or derive the origin only from trusted/allowlisted proxy configuration rather than the request header.
| if !matches!(parsed.scheme(), "http" | "https") { | ||
| bail!( | ||
| "invalid template_build.public_base_url {base_url:?}: scheme must be http or \ | ||
| https" | ||
| ); | ||
| } | ||
| if parsed.query().is_some() { |
There was a problem hiding this comment.
[security · medium]
The value is documented as an external origin, but this validation accepts URL userinfo such as https://user:password@example.com. The downstream builder returns the base verbatim in every upload URL, exposing those configured credentials to API clients and logs. Reject non-empty usernames/passwords (and consider explicitly defining whether path prefixes are supported) before accepting the base URL.
Suggestion:
| if !matches!(parsed.scheme(), "http" | "https") { | |
| bail!( | |
| "invalid template_build.public_base_url {base_url:?}: scheme must be http or \ | |
| https" | |
| ); | |
| } | |
| if parsed.query().is_some() { | |
| if !matches!(parsed.scheme(), "http" | "https") { | |
| bail!( | |
| "invalid template_build.public_base_url {base_url:?}: scheme must be http or \ | |
| https" | |
| ); | |
| } | |
| if !parsed.username().is_empty() || parsed.password().is_some() { | |
| bail!( | |
| "invalid template_build.public_base_url {base_url:?}: must have no credentials" | |
| ); | |
| } | |
| if parsed.query().is_some() { |
| let record = self | ||
| .write_committed_record( | ||
| metadata.id.clone(), | ||
| metadata.alias.clone(), | ||
| metadata.resources, | ||
| committed, | ||
| metadata.source.clone(), | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
[other · high]
There is a durable inconsistency window after this await: cancellation or process termination before bind_alias leaves a committed record claiming the alias while the alias still resolves to the previous snapshot. list() reads record aliases directly, and no recovery path reconciles this state, so misleading duplicate alias claims can persist indefinitely. Please make publication recoverable/transactional (for example, persist an explicit pending-alias state and reconcile it on startup/read), rather than relying only on rollback for returned errors.
| /// archive, so an in-flight build can never observe its build context | ||
| /// change underneath it. | ||
| async fn import(&self, hash: &str, staged: &Path) -> RepositoryResult<()>; |
There was a problem hiding this comment.
[security · high]
This contract permits a globally shared, first-write-wins cache entry to be published without checking that the archive actually represents hash. The upload route passes authenticated caller-controlled bytes directly here, and both backends reuse the entry across templates, so a malformed or malicious upload can permanently poison a known cache key and later builds will materialize the wrong context. Validate the archive against the SDK's canonical content-hash algorithm before publication (and reject mismatches); also constrain accepted keys to the actual lowercase SHA-256 format rather than 16–128 hex characters. If the SDK hash is not a digest of the tar bytes, verification needs to reproduce its canonical context hashing rather than hashing the raw tar stream.
Suggestion:
| /// archive, so an in-flight build can never observe its build context | |
| /// change underneath it. | |
| async fn import(&self, hash: &str, staged: &Path) -> RepositoryResult<()>; | |
| /// archive, so an in-flight build can never observe its build context | |
| /// change underneath it. Before publishing, implementations must verify | |
| /// that `staged` represents the canonical build context identified by | |
| /// `hash` and reject a mismatch. | |
| async fn import(&self, hash: &str, staged: &Path) -> RepositoryResult<()>; |
| async fn create_upload_grant( | ||
| &self, | ||
| template_id: &str, | ||
| hash: &str, | ||
| expires_unix: i64, | ||
| ) -> RepositoryResult<String>; |
There was a problem hiding this comment.
[security · medium]
The grant lifecycle has no operation or explicit contract for deleting grants that expire without a successful claim. This occurs routinely when clients request a link but skip the PUT (and the current link endpoint creates a grant even when the archive is already present). The POSIX backend happens to prune opportunistically, but the OSS backend only delegates retention to an externally configured bucket lifecycle; without that optional policy, bearer-grant objects accumulate indefinitely and allow storage/metadata exhaustion. Add an implementation-independent expiry cleanup contract/API, or make backend initialization install/require a lifecycle rule for the upload-grant prefix.
| } else { | ||
| // Glob source: every matched top-level item lands inside dest. Matched | ||
| // files keep their base name; matched directories contribute their | ||
| // contents (Docker treats each matched directory like a directory | ||
| // source). | ||
| let mut mapped = Vec::with_capacity(index.len()); |
There was a problem hiding this comment.
[bug · medium]
A glob that resolves to multiple source items is accepted even when dest_raw does not end in /. Docker requires a multi-source COPY destination to be an explicitly directory-form path (ending in /); for example, COPY *.txt /data should fail rather than silently creating /data and placing all matches beneath it. Track the distinct matched roots and reject multiple roots when dest_is_dir_hint is false.
| mapped.push(if rel.is_empty() && !entry.is_dir { | ||
| join_abs(&dest, base_name(&root)) | ||
| } else { | ||
| join_abs(&dest, rel) | ||
| }); |
There was a problem hiding this comment.
[performance · medium]
Each target owns another copy of the full destination prefix. Because dest is caller-controlled and its length is not charged against max_total_bytes, a long destination combined with many empty entries can amplify a small allowed archive into entry_count * dest.len() memory during planning (and long-name records can similarly make the rewritten archive exceed the documented budget). Store only per-entry relative mappings and join while streaming, or explicitly bound/charge generated target-path bytes and rewritten tar framing.
| let Some(target) = mapped.targets.get(seen) else { | ||
| bail!("build context archive changed while it was being rewritten"); | ||
| }; | ||
| seen += 1; | ||
| let Some(target) = target else { | ||
| continue; | ||
| }; |
There was a problem hiding this comment.
[bug · medium]
The two-pass consistency check verifies only the entry count. Keeping one file handle pins the inode but does not make its contents immutable: another writable handle can replace/reorder entries or change their types between passes, after which second-pass entries receive mappings computed for different first-pass entries. Compare each second-pass normalized path/type with its EntryIndex before using the positional target, or snapshot/lock the input before pass one.
1 participant
What
Add the pure host-side COPY planner that rewrites an uploaded build-context tar into absolute guest paths using Docker-style source/destination semantics, segment-aware globs, two-pass streaming, and entry/byte/escape safeguards.
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 5 of that six-PR stack and isolates the substantial path/archive logic from the executor wiring that lands in layer 6.
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 adds only
src/template/copy_plan.rsand its private module declaration. It has no runtime caller until layer 6, so it carries one temporary module-leveldead_codeallowance that layer 6 removes. Storage, HTTP upload, guest transfer, extraction, and build-spec wiring are outside this layer.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/04-upload-api...e2b-build/05-copy-plan.
Design and behavior changes
The planner opens the source archive once and performs two bounded passes: the first normalizes/indexes paths and computes Docker-compatible mappings; the second streams entries into a rewritten tar with final absolute paths and requested ownership/mode metadata. It rejects traversal, unsupported entry types, invalid paths, excessive entries, and decompressed-byte budget violations.
Compatibility and operations
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 listed commands pass in the Linux verification environment. The tmpfs
TMPDIRavoids a pre-existingETXTBSYfrom devmachine's shared/tmpin an unrelated credential-helper test. An isolated clippy probe against this module without its temporary allowance reports 25 expecteddead_codeerrors, confirming the allowance is required until layer 6. Unit tests cover path resolution, globs, file/directory destinations, metadata, gzip, traversal and symlink safety, unsupported types, and entry/byte budgets. #28 retains the end-to-end COPY evidence.Skipped checks and reasons: integration tests, service tests, code generation, docs checks, and benchmarks are outside this pure-logic layer.
Risks and reviewer notes
Archive path rewriting and decompression bounds are the security-sensitive surfaces. This layer intentionally has no executor behavior; review the last commit or compare link, with the temporary allowance removed by the next layer.
Checklist