-
Notifications
You must be signed in to change notification settings - Fork 768
Throw when copying from paths outside the context dir #1067
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
da29603
49e1d63
6c56d95
5187988
484cbbc
e02de9c
850df11
a6ea577
d1e8f00
bff918b
040aef9
3c6bf6d
2f7de17
1144910
b8ac1a6
b4961b6
21b36af
6b62a1f
51065a7
b141fd0
034dcad
9ec4481
5b11999
eada42b
356d3ec
f95ade5
52417cd
e41ffae
56b668f
bd60304
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| '@e2b/python-sdk': patch | ||
| 'e2b': patch | ||
| --- | ||
|
|
||
| throw when copying paths outside of the context dir |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -63,12 +63,50 @@ export function readDockerignore(contextPath: string): string[] { | |||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Normalize path separators to forward slashes for glob patterns (glob expects / even on Windows) | ||||||||||||||||||||||
| * Normalize paths on Windows and Unix | ||||||||||||||||||||||
| * | ||||||||||||||||||||||
| * @param path - The path to normalize | ||||||||||||||||||||||
| * @returns The normalized path | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| function normalizePath(path: string): string { | ||||||||||||||||||||||
| return path.replace(/\\/g, '/') | ||||||||||||||||||||||
| export function normalizePath(path: string): string { | ||||||||||||||||||||||
| if (!path || path === '.') { | ||||||||||||||||||||||
| return '.' | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Remove drive letter if present (e.g., 'C:') | ||||||||||||||||||||||
| let workingPath = path | ||||||||||||||||||||||
| if (/^[a-zA-Z]:/.test(path)) { | ||||||||||||||||||||||
| workingPath = path.substring(2) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Check if path is absolute | ||||||||||||||||||||||
| const isAbsolute = workingPath.startsWith('/') || workingPath.startsWith('\\') | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Normalize all separators to forward slashes and split | ||||||||||||||||||||||
| const normalizedPath = workingPath.replace(/[\\/]+/g, '/') | ||||||||||||||||||||||
|
Comment on lines
+82
to
+86
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||
| const parts = normalizedPath.split('/').filter((part) => part && part !== '.') | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const normalized: string[] = [] | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| for (const part of parts) { | ||||||||||||||||||||||
| if (part === '..') { | ||||||||||||||||||||||
| // Go up one directory if possible (but not past root for absolute paths) | ||||||||||||||||||||||
| if (normalized.length > 0 && normalized[normalized.length - 1] !== '..') { | ||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what if last part was |
||||||||||||||||||||||
| normalized.pop() | ||||||||||||||||||||||
| } else if (!isAbsolute) { | ||||||||||||||||||||||
| // For relative paths, keep the '..' if we can't go up further | ||||||||||||||||||||||
| normalized.push('..') | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } else { | ||||||||||||||||||||||
| normalized.push(part) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
mishushakov marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||
| } | ||||||||||||||||||||||
mishushakov marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Build the final path in POSIX style | ||||||||||||||||||||||
| const result = (isAbsolute ? '/' : '') + normalized.join('/') | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Return '.' for empty relative paths | ||||||||||||||||||||||
| return result || '.' | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
|
|
@@ -408,3 +446,20 @@ export function readGCPServiceAccountJSON( | |||||||||||||||||||||
| } | ||||||||||||||||||||||
| return JSON.stringify(pathOrContent) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Returns true if src is a relative path and does not contain any up-path parts. | ||||||||||||||||||||||
| * Works on both Windows and Unix. | ||||||||||||||||||||||
| * | ||||||||||||||||||||||
| * @param src Source path | ||||||||||||||||||||||
| * @returns boolean | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| export function isSafeRelative(src: string): boolean { | ||||||||||||||||||||||
| const normPath = normalizePath(src) | ||||||||||||||||||||||
| return !( | ||||||||||||||||||||||
| path.isAbsolute(normPath) || | ||||||||||||||||||||||
| normPath === '..' || | ||||||||||||||||||||||
| normPath.startsWith('../') || | ||||||||||||||||||||||
| normPath.startsWith('..\\') | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
mishushakov marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||
| } | ||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,52 @@ | ||||||
| import { expect, test, describe } from 'vitest' | ||||||
| import { isSafeRelative } from '../../../src/template/utils' | ||||||
|
|
||||||
| describe('isSafeRelative', () => { | ||||||
| describe('absolute paths', () => { | ||||||
| test('should return false for Unix absolute paths', () => { | ||||||
| expect(isSafeRelative('/absolute/path')).toBe(false) | ||||||
| }) | ||||||
| }) | ||||||
|
|
||||||
| describe('parent directory traversal', () => { | ||||||
| test('should return false for parent directory only', () => { | ||||||
| expect(isSafeRelative('..')).toBe(false) | ||||||
| }) | ||||||
|
|
||||||
| test('should return true for paths starting with ../', () => { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| expect(isSafeRelative('../file.txt')).toBe(false) | ||||||
| }) | ||||||
|
|
||||||
| test('should return true for paths starting with ..\\', () => { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| expect(isSafeRelative('..\\file.txt')).toBe(false) | ||||||
| }) | ||||||
|
|
||||||
| test('should return true for normalized paths that escape context', () => { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| expect(isSafeRelative('foo/../../bar')).toBe(false) | ||||||
| }) | ||||||
| }) | ||||||
|
|
||||||
| describe('valid relative paths', () => { | ||||||
| test('should return false for simple relative paths', () => { | ||||||
| expect(isSafeRelative('file.txt')).toBe(true) | ||||||
| expect(isSafeRelative('folder/file.txt')).toBe(true) | ||||||
| }) | ||||||
|
|
||||||
| test('should return true for current directory references', () => { | ||||||
| expect(isSafeRelative('.')).toBe(true) | ||||||
| expect(isSafeRelative('./file.txt')).toBe(true) | ||||||
| expect(isSafeRelative('./folder/file.txt')).toBe(true) | ||||||
| }) | ||||||
|
|
||||||
| test('should return false for glob patterns', () => { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| expect(isSafeRelative('*.txt')).toBe(true) | ||||||
| expect(isSafeRelative('**/*.ts')).toBe(true) | ||||||
| expect(isSafeRelative('src/**/*')).toBe(true) | ||||||
| }) | ||||||
|
|
||||||
| test('should return false for hidden files and directories', () => { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| expect(isSafeRelative('.hidden')).toBe(true) | ||||||
| expect(isSafeRelative('.config/settings')).toBe(true) | ||||||
| }) | ||||||
| }) | ||||||
| }) | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| import { expect, test, describe } from 'vitest' | ||
| import { normalizePath } from '../../../src/template/utils' | ||
|
|
||
| describe('normalizePath', () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add a test for |
||
| describe('basic path normalization', () => { | ||
| test('should resolve parent directory references', () => { | ||
| expect(normalizePath('/foo/bar/../baz')).toBe('/foo/baz') | ||
| }) | ||
|
|
||
| test('should remove current directory references', () => { | ||
| expect(normalizePath('foo/./bar')).toBe('foo/bar') | ||
| }) | ||
|
|
||
| test('should collapse multiple slashes', () => { | ||
| expect(normalizePath('foo//bar///baz')).toBe('foo/bar/baz') | ||
| }) | ||
|
|
||
| test('should handle multiple parent directory traversals in relative paths', () => { | ||
| expect(normalizePath('../foo/../../bar')).toBe('../../bar') | ||
| }) | ||
|
|
||
| test('should not traverse past root for absolute paths', () => { | ||
| expect(normalizePath('/foo/../../bar')).toBe('/bar') | ||
| }) | ||
|
|
||
| test('should return dot for empty path', () => { | ||
| expect(normalizePath('')).toBe('.') | ||
| }) | ||
|
|
||
| test('should remove leading current directory reference', () => { | ||
| expect(normalizePath('./foo/bar')).toBe('foo/bar') | ||
| }) | ||
| }) | ||
|
|
||
| describe('Windows paths converted to POSIX style', () => { | ||
| test('should normalize Windows path with drive letter and backslashes', () => { | ||
| expect(normalizePath('C:\\foo\\bar\\..\\baz')).toBe('/foo/baz') | ||
| }) | ||
|
|
||
| test('should normalize Windows path with drive letter and forward slashes', () => { | ||
| expect(normalizePath('C:/foo/bar/../baz')).toBe('/foo/baz') | ||
| }) | ||
|
|
||
| test('should normalize backslash with current directory reference', () => { | ||
| expect(normalizePath('foo\\.\\bar')).toBe('foo/bar') | ||
| }) | ||
|
|
||
| test('should handle backslash parent directory traversal', () => { | ||
| expect(normalizePath('..\\..\\foo')).toBe('../../foo') | ||
| }) | ||
|
|
||
| test('should normalize drive letter root to POSIX root', () => { | ||
| expect(normalizePath('C:\\')).toBe('/') | ||
| }) | ||
| }) | ||
|
|
||
| describe('edge cases', () => { | ||
| test('should return dot for current directory', () => { | ||
| expect(normalizePath('.')).toBe('.') | ||
| }) | ||
|
|
||
| test('should handle parent directory only', () => { | ||
| expect(normalizePath('..')).toBe('..') | ||
| }) | ||
|
|
||
| test('should handle absolute root path', () => { | ||
| expect(normalizePath('/')).toBe('/') | ||
| }) | ||
|
|
||
| test('should handle complex nested path', () => { | ||
| expect(normalizePath('a/b/c/../../d/./e/../f')).toBe('a/d/f') | ||
| }) | ||
|
|
||
| test('should preserve trailing segments after parent traversal', () => { | ||
| expect(normalizePath('a/../b/../c')).toBe('c') | ||
| }) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,8 @@ | |
| read_dockerignore, | ||
| read_gcp_service_account_json, | ||
| get_caller_frame, | ||
| is_safe_relative, | ||
| normalize_path, | ||
| ) | ||
| from types import TracebackType | ||
|
|
||
|
|
@@ -65,9 +67,27 @@ def copy( | |
| srcs = [src] if isinstance(src, (str, Path)) else src | ||
|
|
||
| for src_item in srcs: | ||
| normalized_src_item = normalize_path(str(src_item)) | ||
| normalized_dest = normalize_path(str(dest)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be done outside of the loop |
||
|
|
||
| if not is_safe_relative(str(src_item)): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't this use the normalized path? |
||
| caller_frame = get_caller_frame(STACK_TRACE_DEPTH - 1) | ||
| stack_trace = None | ||
| if caller_frame is not None: | ||
| stack_trace = TracebackType( | ||
| tb_next=None, | ||
| tb_frame=caller_frame, | ||
| tb_lasti=caller_frame.f_lasti, | ||
| tb_lineno=caller_frame.f_lineno, | ||
| ) | ||
|
|
||
| raise ValueError( | ||
| f"Source path {src_item} is outside of the context directory." | ||
| ).with_traceback(stack_trace) | ||
|
|
||
| args = [ | ||
| str(src_item), | ||
| str(dest), | ||
| normalized_src_item, | ||
| normalized_dest, | ||
| user or "", | ||
| pad_octal(mode) if mode else "", | ||
| ] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can move this outside of the for loop