From cce93c02026888d482d1b25701fcae4921859a40 Mon Sep 17 00:00:00 2001 From: Robin Munn Date: Tue, 23 Apr 2024 15:31:06 +0700 Subject: [PATCH 1/2] Include fs stats and mode in zip archive --- .../__tests__/upload-artifact.test.ts | 45 ++++++++++------ .../upload/upload-zip-specification.ts | 54 +++++++++---------- packages/artifact/src/internal/upload/zip.ts | 12 +++-- 3 files changed, 64 insertions(+), 47 deletions(-) diff --git a/packages/artifact/__tests__/upload-artifact.test.ts b/packages/artifact/__tests__/upload-artifact.test.ts index 1761fa01dc..02ecd18b33 100644 --- a/packages/artifact/__tests__/upload-artifact.test.ts +++ b/packages/artifact/__tests__/upload-artifact.test.ts @@ -31,15 +31,18 @@ describe('upload-artifact', () => { .mockReturnValue([ { sourcePath: '/home/user/files/plz-upload/file1.txt', - destinationPath: 'file1.txt' + destinationPath: 'file1.txt', + stats: new fs.Stats() }, { sourcePath: '/home/user/files/plz-upload/file2.txt', - destinationPath: 'file2.txt' + destinationPath: 'file2.txt', + stats: new fs.Stats() }, { sourcePath: '/home/user/files/plz-upload/dir/file3.txt', - destinationPath: 'dir/file3.txt' + destinationPath: 'dir/file3.txt', + stats: new fs.Stats() } ]) @@ -139,15 +142,18 @@ describe('upload-artifact', () => { .mockReturnValue([ { sourcePath: '/home/user/files/plz-upload/file1.txt', - destinationPath: 'file1.txt' + destinationPath: 'file1.txt', + stats: new fs.Stats() }, { sourcePath: '/home/user/files/plz-upload/file2.txt', - destinationPath: 'file2.txt' + destinationPath: 'file2.txt', + stats: new fs.Stats() }, { sourcePath: '/home/user/files/plz-upload/dir/file3.txt', - destinationPath: 'dir/file3.txt' + destinationPath: 'dir/file3.txt', + stats: new fs.Stats() } ]) @@ -178,15 +184,18 @@ describe('upload-artifact', () => { .mockReturnValue([ { sourcePath: '/home/user/files/plz-upload/file1.txt', - destinationPath: 'file1.txt' + destinationPath: 'file1.txt', + stats: fs.Stats.prototype }, { sourcePath: '/home/user/files/plz-upload/file2.txt', - destinationPath: 'file2.txt' + destinationPath: 'file2.txt', + stats: fs.Stats.prototype }, { sourcePath: '/home/user/files/plz-upload/dir/file3.txt', - destinationPath: 'dir/file3.txt' + destinationPath: 'dir/file3.txt', + stats: fs.Stats.prototype } ]) @@ -233,15 +242,18 @@ describe('upload-artifact', () => { .mockReturnValue([ { sourcePath: '/home/user/files/plz-upload/file1.txt', - destinationPath: 'file1.txt' + destinationPath: 'file1.txt', + stats: new fs.Stats() }, { sourcePath: '/home/user/files/plz-upload/file2.txt', - destinationPath: 'file2.txt' + destinationPath: 'file2.txt', + stats: new fs.Stats() }, { sourcePath: '/home/user/files/plz-upload/dir/file3.txt', - destinationPath: 'dir/file3.txt' + destinationPath: 'dir/file3.txt', + stats: new fs.Stats() } ]) @@ -296,15 +308,18 @@ describe('upload-artifact', () => { .mockReturnValue([ { sourcePath: '/home/user/files/plz-upload/file1.txt', - destinationPath: 'file1.txt' + destinationPath: 'file1.txt', + stats: new fs.Stats() }, { sourcePath: '/home/user/files/plz-upload/file2.txt', - destinationPath: 'file2.txt' + destinationPath: 'file2.txt', + stats: new fs.Stats() }, { sourcePath: '/home/user/files/plz-upload/dir/file3.txt', - destinationPath: 'dir/file3.txt' + destinationPath: 'dir/file3.txt', + stats: new fs.Stats() } ]) diff --git a/packages/artifact/src/internal/upload/upload-zip-specification.ts b/packages/artifact/src/internal/upload/upload-zip-specification.ts index c6e807e646..9899bad2e0 100644 --- a/packages/artifact/src/internal/upload/upload-zip-specification.ts +++ b/packages/artifact/src/internal/upload/upload-zip-specification.ts @@ -5,14 +5,19 @@ import {validateFilePath} from './path-and-artifact-name-validation' export interface UploadZipSpecification { /** - * An absolute source path that points to a file that will be added to a zip. Null if creating a new directory + * An absolute source path that points to a file or directory that will be added to a zip */ - sourcePath: string | null + sourcePath: string /** * The destination path in a zip for a file */ destinationPath: string + + /** + * Metadata (permissions, creation time, etc) about the file + */ + stats: fs.Stats } /** @@ -75,37 +80,28 @@ export function getUploadZipSpecification( - file3.txt */ for (let file of filesToZip) { - if (!fs.existsSync(file)) { + const stats = fs.statSync(file, {throwIfNoEntry: false}); + if (!stats) { throw new Error(`File ${file} does not exist`) } - if (!fs.statSync(file).isDirectory()) { - // Normalize and resolve, this allows for either absolute or relative paths to be used - file = normalize(file) - file = resolve(file) - if (!file.startsWith(rootDirectory)) { - throw new Error( - `The rootDirectory: ${rootDirectory} is not a parent directory of the file: ${file}` - ) - } - - // Check for forbidden characters in file paths that may cause ambiguous behavior if downloaded on different file systems - const uploadPath = file.replace(rootDirectory, '') - validateFilePath(uploadPath) + // Normalize and resolve, this allows for either absolute or relative paths to be used + file = normalize(file) + file = resolve(file) + if (!file.startsWith(rootDirectory)) { + throw new Error( + `The rootDirectory: ${rootDirectory} is not a parent directory of the file: ${file}` + ) + } - specification.push({ - sourcePath: file, - destinationPath: uploadPath - }) - } else { - // Empty directory - const directoryPath = file.replace(rootDirectory, '') - validateFilePath(directoryPath) + // Check for forbidden characters in file paths that may cause ambiguous behavior if downloaded on different file systems + const destinationPath = file.replace(rootDirectory, '') + validateFilePath(destinationPath) - specification.push({ - sourcePath: null, - destinationPath: directoryPath - }) - } + specification.push({ + sourcePath: file, + destinationPath, + stats + }) } return specification } diff --git a/packages/artifact/src/internal/upload/zip.ts b/packages/artifact/src/internal/upload/zip.ts index 5a9231193c..cf1f8dd45e 100644 --- a/packages/artifact/src/internal/upload/zip.ts +++ b/packages/artifact/src/internal/upload/zip.ts @@ -42,14 +42,20 @@ export async function createZipUploadStream( zip.on('end', zipEndCallback) for (const file of uploadSpecification) { - if (file.sourcePath !== null) { + if (!file.stats.isDirectory()) { // Add a normal file to the zip zip.append(createReadStream(file.sourcePath), { - name: file.destinationPath + name: file.destinationPath, + stats: file.stats, + mode: file.stats.mode }) } else { // Add a directory to the zip - zip.append('', {name: file.destinationPath}) + zip.append('', { + name: file.destinationPath, + stats: file.stats, + mode: file.stats.mode + }) } } From 5f62f1e65ba5d59c8c7ec0bb0387358c8d905263 Mon Sep 17 00:00:00 2001 From: Robin Munn Date: Mon, 29 Apr 2024 11:22:16 +0700 Subject: [PATCH 2/2] Prevent "too many open files" in artifact upload See https://www.archiverjs.com/docs/archiver/#file --- packages/artifact/src/internal/upload/zip.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/artifact/src/internal/upload/zip.ts b/packages/artifact/src/internal/upload/zip.ts index cf1f8dd45e..622ff25fa6 100644 --- a/packages/artifact/src/internal/upload/zip.ts +++ b/packages/artifact/src/internal/upload/zip.ts @@ -44,7 +44,7 @@ export async function createZipUploadStream( for (const file of uploadSpecification) { if (!file.stats.isDirectory()) { // Add a normal file to the zip - zip.append(createReadStream(file.sourcePath), { + zip.file(file.sourcePath, { name: file.destinationPath, stats: file.stats, mode: file.stats.mode