Repository navigation
fix(sdk): resolve directory symlinks in template copies - #2137
Elon-Jiang wants to merge 5 commits into
Conversation
Co-authored-by: OpenAI Codex <noreply@openai.com>
🦋 Changeset detectedLatest commit: efcc2b9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
Note
Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.
Devin Review found 2 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| for (const file of files.values()) { | ||
| if (!file.isSymbolicLink()) continue |
There was a problem hiding this comment.
🔴 Reincluded files under ignored links disappear
When an ignore pattern excludes a directory link but re-includes a child, getAllFilesInPath omits the child. The expansion loop never sees the excluded link, so the build uploads no re-included content.
Learn more
File discovery uses glob's ignore matcher before expanding symlink directories. An ignored symlink is absent from files, even when childrenIgnored allows traversal for a later negated pattern. Glob does not descend into symlink directories by default, so the expansion loop cannot discover that link or its re-included children. Both calculateFilesHash and spoolTarArchive consume the incomplete list.
Example: Let real/link point to a directory containing keep.txt. Copy real with resolveSymlinks: true and ignore patterns ['real/link', '!real/link/keep.txt']. The expected archive includes real/link/keep.txt; the JS traversal omits it while the Python walker descends through the ignored link.
Recommended fix: Track directory links separately even when their own entries are ignored but mayMatchUnder is true, then traverse those links using the logical path and apply ignore patterns to their descendants. Do not add ignored link entries to the archive.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const linkedFiles = await glob( | ||
| normalizePath(path.join(file.relative(), '**/*')), | ||
| { ignore, withFileTypes: true, dot: true, cwd: contextPath } |
There was a problem hiding this comment.
🟡 Special-character links lose their contents
When a matched directory link has brackets in its name, getAllFilesInPath treats that name as a new glob pattern. Its target files disappear from the archive and cache hash, or files from a different directory appear instead.
Learn more
The initial glob can match a symlink with a filename containing glob metacharacters, especially when the source is a wildcard. Its relative() value is a filesystem path, not a glob-escaped prefix. Passing that value to glob again changes the meaning of characters such as [ and ]. The same construction already exists for ordinary matched directories, but this change newly uses it for matched symlink directories.
Example: With alias[1] -> real and real/app.py, copying * matches alias[1]. The generated pattern alias[1]/**/* looks under alias1 instead of alias[1], leaving the link's uploaded directory empty. An alias1 directory can contribute unrelated files.
Recommended fix: Escape glob metacharacters in the link's literal relative path before appending /**/*, or enumerate children starting at the literal directory path while preserving logical paths and applying ignore patterns.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
TASTE.md compliance review (e2b-dev/sdk-harness). I checked the changed code against parity (T-1, T-2), API shape (T-3, T-3a), the template builder (T-37–T-42), errors (T-57, T-62, T-63) and docs (T-69–T-71).
2 violations, posted inline. They are the same issue in each SDK: the new cyclic-symlink error.
- JS
utils.tsand Pythonutils.pyraiseTemplateError/TemplateExceptioninstead ofBuildError/BuildException. The error also loses the user'scopy()stack trace (T-42, T-57), and the message doesn't say how to fix it (T-62, T-63).
Not tied to a changed line:
- T-62 / T-69: the PR adds a new failure mode to the public
copy(): it can now throw on a cyclic directory link whenresolveSymlinks/resolve_symlinksis enabled. The public docs for that option don't mention it. In JS,copy()intemplate/index.tsandCopyItem.resolveSymlinksintypes.tssay nothing; in Python,:param resolve_symlinks: Whether to resolve symlinksintemplate/main.pyis all there is. Document that directory links are traversed and when the error is thrown (and which error), in both SDKs. - Minor, internal only (not counted):
getAllFilesInPath/get_all_files_in_pathnow take two trailing optional booleans by position, so call sites read..., true, resolveSymlinks). These helpers aren't public, so T-3 / T-3a don't strictly apply. Even so, a keyword-onlyresolve_symlinks(after a*) and an options object in JS would match the house style. The JS JSDoc also lacks@param resolveSymlinks, which the Python docstring has.
Parity is otherwise good. Both SDKs keep logical link paths for archive names and ignore matching, both detect cycles, and sync/async Python share one helper (T-1, T-2). The changeset covers both packages.
| throw new TemplateError( | ||
| `Cannot resolve cyclic directory symlink: ${file.relativePosix()}` | ||
| ) |
There was a problem hiding this comment.
T-42 / T-57 — a builder configuration failure (the copy() source contains a cyclic directory link) should be a BuildError, not a TemplateError (which subclasses SandboxError; no sandbox is involved here). T-42 also requires the stack trace to point at the user's .copy(...) call. As written, this throw skips the stackTrace that calculateFilesHash already gets, so the trace lands inside utils.ts. Pass stackTrace through (e.g. as new BuildError(msg, stackTrace)), or catch and rethrow in calculateFilesHash the same way the No files found error sets error.stack.
T-62 / T-63 — the message says what failed but not what to do. Name the fix in the option the user typed:
| throw new TemplateError( | |
| `Cannot resolve cyclic directory symlink: ${file.relativePosix()}` | |
| ) | |
| throw new BuildError( | |
| `Directory symlink '${file.relativePosix()}' points to one of its own ancestors, so it can't be resolved. Remove the link, exclude it via .dockerignore, or set \`resolveSymlinks: false\` to copy the link itself.` | |
| ) |
(BuildError must be imported from ../errors.)
| raise TemplateException( | ||
| f"Cannot resolve cyclic directory symlink: {dir_relative_path}" | ||
| ) |
There was a problem hiding this comment.
T-42 / T-57: a builder configuration failure should raise BuildException, not TemplateException (a SandboxException subclass). It should also carry the user's builder frame. calculate_files_hash already receives stack_trace and applies it to No files found via .with_traceback(stack_trace), but this raise bypasses it, so the traceback points into utils.py. Catch and re-raise with .with_traceback(stack_trace) in calculate_files_hash, or thread the trace down.
T-62 / T-63: make the message actionable and use the Python option name. This also keeps it in sync with the JS message (T-1):
| raise TemplateException( | |
| f"Cannot resolve cyclic directory symlink: {dir_relative_path}" | |
| ) | |
| raise BuildException( | |
| f"Directory symlink '{dir_relative_path}' points to one of its own ancestors, so it can't be resolved. " | |
| "Remove the link, exclude it via .dockerignore, or set `resolve_symlinks=False` to copy the link itself." | |
| ) |
(BuildException must be imported from e2b.exceptions.)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17e52146aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const file of files.values()) { | ||
| if (!file.isSymbolicLink()) continue |
There was a problem hiding this comment.
Traverse excluded symlink directories for re-included files
When resolveSymlinks is true and .dockerignore excludes a directory link but later re-includes a descendant (for example, linked then !linked/keep.txt), the initial ignore-aware glob does not follow that symlink, so it never enters files and this pass cannot expand it. JS then omits the re-included file from both the upload and cache hash, while Python's walk() follows excluded directories when may_match_under is true, leaving the SDKs with different COPY contents for the same inputs.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
| def walk( | ||
| dir_path: str, dir_relative_path: str, ancestors: frozenset[str] = frozenset() | ||
| ) -> None: | ||
| real_path = os.path.realpath(dir_path) |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: MEDIUM
Description: With copy(resolve_symlinks=True) / copy({resolveSymlinks:true}), a directory symlink inside the template file context is walked and its target bytes are archived under the link's logical name. realpath is used only to reject cycles. A link to any readable directory that is not a walk ancestor — a sibling checkout, ~/.aws, ~/.ssh, or another absolute directory — is listed, hashed, and uploaded. validate_relative_path only checks the COPY source string, so the context-root boundary is not applied to the resolved target.
Impact: The template context root is the documented COPY boundary that keeps path traversal out of the upload. This change lets in-context content (a repo, PR, or project tree) steer the local build into reading files the process can already open outside that root and sending them to the remote template file store, where they are baked into the image under an in-context archive path. The builder is the confused deputy: the flag defaults off, but turning it on for legitimate in-context links does not authorize that escape, and a template definition that sets the flag can ship next to the symlink. Host credentials and other projects become part of the uploaded image.
Remediation: After realpath, skip or reject a directory symlink whose resolved target is outside the resolved context root — commonpath/relative must not escape — before listing, hashing, or archiving its children. Keep the cycle check, but do not treat it as the containment gate. Apply the same root check on both the Python walk and the JS glob expansion.
Merge current main without rewriting published history and address public-build regression and review findings. Co-authored-by: OpenAI Codex <noreply@openai.com>
|
Addressed the visible review findings in
On current main |
Co-authored-by: OpenAI Codex <noreply@openai.com>
Co-authored-by: OpenAI Codex <noreply@openai.com>
|
Traversal candidates now come from actual source matches or descendants of selected directories, while archive inclusion still applies the ignore matcher. Matched directory prefixes are literal paths when resolving. This preserves the symlinked-context test that copies only Current validation: 157 Python passed / 1 skipped, 108 JS passed / 3 skipped, including 36 Python and 18 JS public-build cases. Root format, lint, types and SDK builds pass. The PR body includes the current scope, native loopback-fixture boundaries and all regression results. |
Co-authored-by: OpenAI Codex <noreply@openai.com>
|
The public API documentation follow-up is now in Root format, lint and type/build checks pass. The same affected suites remain green: Python 157 passed / 1 skipped; JS 108 passed / 3 skipped. Runtime code is unchanged by this documentation follow-up. |
Problem and fix
Fixes #2136.
copy('linked', '/app', resolve_symlinks=True)/resolveSymlinks: truecurrently uploads an empty directory whenlinkedpoints to a directory. The cache hash also ignores the target's files, so editing one does not invalidate the COPY layer.Pass the resolution setting through the shared file-discovery path in both SDKs, so hashing and tar uploads see the same resolved descendants. Keep logical link paths for archive names and ignore matching. Preserve the default symlink-copy behavior and report cyclic or out-of-context resolved paths as build errors at the user copy callsite. Python sync and async builders use the same helper.
Usage
With
linked -> realandreal/app.py, the archive now containslinked/app.py; changing its contents changes the upload hash. Existing public options are unchanged; this restores their documented resolution behavior. Resolved paths must remain in the configured file context, including file links encountered within resolved directories. Move intended targets inside that context (or choose a context that contains them), exclude the link, or preserve links with resolution disabled. A patch changeset covers both SDKs.Validation
The two JS discovery review findings were reproduced as two failures with six controls before the follow-up fix. The re-inclusion failure is triggered by copying the context root (
.); the ordinary directory example already passed. Separate synthetic outside-target probes failed the new containment assertions before their fix in both SDKs. A second follow-up reproduces ignored self-link failures in both Python builders (two failures) and unselected-file expansion under a symlinked JS context (one failure with a resolution-disabled control); those now pass. A recursive**/*source with ignored matched directories and re-included linked children was also reproduced as one JS and two Python failures. Source matches used for traversal are now separated from archive inclusion; a literal-directory-name regression covers the added traversal path.Original behavior was reproduced on main
3e72be3f08ec1a258ec0253c153df7ceed739f10. Final validation uses main6dc8d5a10aa5db8dc8cf9c4d95fc769d886197fd(SDK 2.64.0), merged without rewriting published history:uv run pytest tests/shared/template tests/sync/template_sync/test_upload_file.py -q— 157 passed, 1 skipped.pnpm --dir packages/js-sdk test --project template tests/template/directorySymlinks.test.ts tests/template/utils tests/template/uploadFile.test.ts— 108 passed, 3 skipped.pnpm run format,pnpm run lint, andpnpm run typecheck(including SDK builds) pass. Lint emits four warnings in unchanged files.The public-build regressions run real filesystem discovery, hashing, archives and HTTP uploads against a loopback server replacing only E2B's remote build/upload endpoints. They cover nested and repeated targets, target-content cache invalidation, ignored-file stability, default symlink preservation, cycle detection (including ignored ancestors), links with literal glob characters, root-copy re-inclusion and cache invalidation, out-of-context directory/nested-file targets, actionable errors preserving the original copy frame, excluded self-referential links, and exact source selection when the context itself is a symlink. A separate native HTTP reproduction confirms the before/after archive members and hashes. No cloud image build or sandbox execution was tested. New symlink tests were run on macOS and skip on Windows where creating links requires privileges; other runtimes were not exercised.
AI assistance: OpenAI Codex helped investigate, implement, and validate this change.
The public
copy()/CopyItemAPI documentation in both SDKs now also describes file/directory link traversal, default link preservation, and the build-timeBuildError/BuildExceptionfor detected directory cycles or out-of-context targets. Repository format/lint/type checks and the same 157 Python / 108 JS passing tests were rerun after this documentation follow-up (skips unchanged: 1 / 3).