-
Notifications
You must be signed in to change notification settings - Fork 748
[Bug] - Incorrectly handled absolute paths #1057
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 17 commits
7d231e6
87e401c
5acc917
be1dfd1
47b68ea
42fd855
358f44e
6bc2d0e
740df7d
130fa0e
789c1a7
8270c49
98df64a
c1df732
77abafa
a34000c
d7d5308
e2096cf
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 |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import crypto from 'node:crypto' | ||
| import fs from 'node:fs' | ||
| import fs, { PathLike } from 'node:fs' | ||
| import path from 'node:path' | ||
| import { dynamicImport, dynamicRequire } from '../utils' | ||
| import { BASE_STEP_NAME, FINALIZE_STEP_NAME } from './consts' | ||
|
|
@@ -54,7 +54,9 @@ export async function getAllFilesInPath( | |
| ignore: ignorePatterns, | ||
| withFileTypes: true, | ||
| // this is required so that the ignore pattern is relative to the file path | ||
| cwd: contextPath, | ||
| // if src is absolute, we don't need to set the cwd; otherwise use contextPath | ||
| // to anchor relative patterns to the template's context directory | ||
| cwd: path.isAbsolute(src) ? undefined : contextPath, | ||
| }) | ||
|
|
||
| for (const file of globFiles) { | ||
|
|
@@ -72,6 +74,7 @@ export async function getAllFilesInPath( | |
| const dirFiles = await glob(dirPattern, { | ||
| ignore: ignorePatterns, | ||
| withFileTypes: true, | ||
| // dirPattern is always relative (constructed from file.relative()), so use contextPath | ||
|
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. Nested directory glob uses wrong cwd for absolute pathsWhen Additional Locations (1) |
||
| cwd: contextPath, | ||
| }) | ||
| dirFiles.forEach((f) => files.set(f.fullpath(), f)) | ||
|
|
@@ -106,7 +109,7 @@ export async function calculateFilesHash( | |
| resolveSymlinks: boolean, | ||
| stackTrace: string | undefined | ||
| ): Promise<string> { | ||
| const srcPath = path.join(contextPath, src) | ||
| const srcPath = path.isAbsolute(src) ? src : path.join(contextPath, src) | ||
| const hash = crypto.createHash('sha256') | ||
| const content = `COPY ${src} ${dest}` | ||
|
|
||
|
|
@@ -255,38 +258,66 @@ export function padOctal(mode: number): string { | |
| /** | ||
| * Create a compressed tar stream of files matching a pattern. | ||
| * | ||
| * @param fileName Glob pattern for files to include | ||
| * @param filePath Glob pattern for files to include | ||
| * @param fileName Name of the file in the tar archive | ||
| * @param fileContextPath Base directory for resolving file paths | ||
| * @param ignorePatterns Ignore patterns to exclude from the archive | ||
| * @param resolveSymlinks Whether to follow symbolic links | ||
| * @returns A readable stream of the gzipped tar archive | ||
| */ | ||
| export async function tarFileStream( | ||
| fileName: string, | ||
| filePath: string, | ||
| fileContextPath: string, | ||
| ignorePatterns: string[], | ||
| resolveSymlinks: boolean | ||
| ) { | ||
| const { create } = await dynamicImport<typeof import('tar')>('tar') | ||
| const { packTar } = | ||
| await dynamicImport<typeof import('modern-tar/fs')>('modern-tar/fs') | ||
| const { createGzip } = | ||
| await dynamicImport<typeof import('node:zlib')>('node:zlib') | ||
|
|
||
| const allFiles = await getAllFilesInPath( | ||
| fileName, | ||
| filePath, | ||
| fileContextPath, | ||
| ignorePatterns, | ||
| true | ||
| ) | ||
|
|
||
| const filePaths = allFiles.map((file) => file.relativePosix()) | ||
| const sources: Array<{ | ||
| type: 'file' | 'directory' | ||
| source: string | ||
| target: string | ||
| }> = [] | ||
|
|
||
| return create( | ||
| { | ||
| gzip: true, | ||
| cwd: fileContextPath, | ||
| follow: resolveSymlinks, | ||
| noDirRecurse: true, | ||
| }, | ||
| filePaths | ||
| ) | ||
| for (const file of allFiles) { | ||
| const sourcePath = file.fullpathPosix() | ||
| const targetPath = relativizePath(sourcePath, fileContextPath) | ||
|
|
||
| if (file.isDirectory()) { | ||
| sources.push({ | ||
| type: 'directory', | ||
| source: sourcePath, | ||
| target: targetPath, | ||
| }) | ||
| } else { | ||
| sources.push({ | ||
| type: 'file', | ||
| source: sourcePath, | ||
| target: targetPath, | ||
| }) | ||
| } | ||
mishushakov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| // Create tar stream with gzip compression | ||
| const tarStream = packTar(sources, { | ||
| dereference: resolveSymlinks, // Follow symlinks when resolveSymlinks is true | ||
| }) | ||
|
|
||
| // Pipe through gzip compression | ||
| const gzipStream = createGzip() | ||
| tarStream.pipe(gzipStream) | ||
|
|
||
| return gzipStream | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -298,14 +329,14 @@ export async function tarFileStream( | |
| * @returns Object containing the content length and upload stream | ||
| */ | ||
| export async function tarFileStreamUpload( | ||
| fileName: string, | ||
| filePath: string, | ||
| fileContextPath: string, | ||
| ignorePatterns: string[], | ||
| resolveSymlinks: boolean | ||
| ) { | ||
| // First pass: calculate the compressed size | ||
| const sizeCalculationStream = await tarFileStream( | ||
| fileName, | ||
| filePath, | ||
| fileContextPath, | ||
| ignorePatterns, | ||
| resolveSymlinks | ||
|
|
@@ -318,7 +349,7 @@ export async function tarFileStreamUpload( | |
| return { | ||
| contentLength, | ||
| uploadStream: await tarFileStream( | ||
| fileName, | ||
| filePath, | ||
| fileContextPath, | ||
| ignorePatterns, | ||
| resolveSymlinks | ||
|
|
@@ -369,3 +400,32 @@ export function readGCPServiceAccountJSON( | |
| } | ||
| return JSON.stringify(pathOrContent) | ||
| } | ||
|
|
||
| /** | ||
| * Convert absolute paths to relativized paths. | ||
| * In addition to converting absolute paths to relative paths, | ||
| * it strips up directories (../ or ..\ on Windows). | ||
| * | ||
| * @param src Absolute path to convert | ||
| * @param fileContextPath Base directory for resolving relative paths | ||
| * @returns Relative path with forward slashes (for tar/cross-platform compatibility) | ||
| */ | ||
| export function relativizePath( | ||
| src: PathLike, | ||
| fileContextPath: PathLike | ||
| ): string { | ||
| let rewrittenPath = src.toString() | ||
|
|
||
| // Convert absolute paths to relative paths | ||
| if (path.isAbsolute(rewrittenPath)) { | ||
| const contextPath = path.resolve(fileContextPath.toString()) | ||
| const relativePath = path.relative(contextPath, rewrittenPath) | ||
| rewrittenPath = relativePath | ||
| } | ||
|
|
||
| // Strip up directories (../ or ..\ on Windows) | ||
| rewrittenPath = rewrittenPath.replace(/\.\.(\/|\\)/g, '') | ||
|
|
||
| // Normalize to forward slashes for cross-platform compatibility (tar archives require forward slashes) | ||
| return normalizePath(rewrittenPath) | ||
| } | ||
mishushakov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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.
JS sends unfiltered
filePathto backend, Python doesn'tThe JS
serializemethod passes the entirestepsarray directly to the API, including the newfilePathfield which contains the original absolute path and is of typePathLike(could be Buffer/URL). In contrast, Python's_serializeexplicitly constructs new step objects with onlytype,args,force,filesHash, andforceUploadfields, filtering outfilePathandresolveSymlinks. This inconsistency means the JS SDK sends potentially sensitive path information and non-stringPathLikeobjects to the backend that the Python SDK properly excludes.