Throw when copying from paths outside the context dir#1067
Throw when copying from paths outside the context dir#1067mishushakov wants to merge 30 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: bd60304 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da296034db
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
packages/python-sdk/tests/async/template_async/test_stacktrace.py
Outdated
Show resolved
Hide resolved
packages/python-sdk/tests/async/template_async/test_stacktrace.py
Outdated
Show resolved
Hide resolved
jakubno
left a comment
There was a problem hiding this comment.
why can't we throw directly in .copy() command? Can't you resolve the path directly there? You want to work with the relative path ideally everywhere anyway?
packages/python-sdk/tests/shared/template/utils/test_is_safe_relative.py
Show resolved
Hide resolved
|
Cursor review |
|
|
||
| for (const src of srcs) { | ||
| const normalizedSrc = normalizePath(src.toString()) | ||
| const normalizedDest = normalizePath(dest.toString()) |
There was a problem hiding this comment.
You can move this outside of the for loop
| expect(isSafeRelative('../file.txt')).toBe(false) | ||
| }) | ||
|
|
||
| test('should return true for paths starting with ..\\', () => { |
There was a problem hiding this comment.
| test('should return true for paths starting with ..\\', () => { | |
| test('should return false for paths starting with ..\\', () => { |
| expect(isSafeRelative('..\\file.txt')).toBe(false) | ||
| }) | ||
|
|
||
| test('should return true for normalized paths that escape context', () => { |
There was a problem hiding this comment.
| test('should return true for normalized paths that escape context', () => { | |
| test('should return false for normalized paths that escape context', () => { |
| expect(isSafeRelative('..')).toBe(false) | ||
| }) | ||
|
|
||
| test('should return true for paths starting with ../', () => { |
There was a problem hiding this comment.
| test('should return true for paths starting with ../', () => { | |
| test('should return false for paths starting with ../', () => { |
| expect(isSafeRelative('./folder/file.txt')).toBe(true) | ||
| }) | ||
|
|
||
| test('should return false for glob patterns', () => { |
There was a problem hiding this comment.
| test('should return false for glob patterns', () => { | |
| test('should return true for glob patterns', () => { |
| # Determine if absolute | ||
| is_absolute = working_path.startswith("/") or working_path.startswith("\\") | ||
|
|
||
| # Normalize separators to '/' | ||
| normalized_path = re.sub(r"[\\/]+", "/", working_path) |
|
|
||
| for part in parts: | ||
| if part == "..": | ||
| if normalized and normalized[-1] != "..": |
| os.path.isabs(norm_path) | ||
| or norm_path == ".." | ||
| or norm_path.startswith("../") | ||
| or norm_path.startswith("..\\") |
There was a problem hiding this comment.
normpath doesn't contain \ anymore right?
| dockerfile = """FROM node:24 | ||
| COPY --chown=myuser:mygroup app.js /app/ | ||
| COPY --chown=anotheruser config.json /config/""" | ||
| COPY --chown=myuser:mygroup app.js /app |
There was a problem hiding this comment.
why did you remove the extra slashes?
| from e2b.template.utils import normalize_path | ||
|
|
||
|
|
||
| class TestBasicPathNormalization: |
Pull request was converted to draft
Note
Medium Risk
Introduces stricter validation and path normalization for
copyinputs, which can break existing templates that relied on absolute paths or..traversal; otherwise changes are localized and well-covered by tests.Overview
Prevents path traversal in template
copyoperations by throwing when the source path is outside the template context directory (absolute paths,..escapes), while preserving stack traces for better error attribution.Adds cross-platform path normalization (
normalizePath/normalize_path) and a shared safety check (isSafeRelative/is_safe_relative) in both the JS and Python SDKs, and updates CLI fixtures and Dockerfile parsing/tests to reflect normalized destinations (e.g., removing trailing slashes) plus new unit coverage for the helpers.Written by Cursor Bugbot for commit bd60304. This will update automatically on new commits. Configure here.