[Artifacts] Name validation + zip specification creation (#1482)

* Artifact name validation + zip specification creation * Fix linting issues * Grammar fix * Update test description
2023-08-04 09:53:42 -04:00 · 2023-08-04 09:53:42 -04:00 · 7da3ac6eda
parent 2461056696
commit 7da3ac6eda
6 changed files with 620 additions and 8 deletions
--- a/packages/artifact/tests/path-and-artifact-name-validation.test.ts
+++ b/packages/artifact/tests/path-and-artifact-name-validation.test.ts
@ -0,0 +1,79 @@
+import {
+  validateArtifactName,
+  validateFilePath
+} from '../src/internal/upload/path-and-artifact-name-validation'
+
+import * as core from '@actions/core'
+
+describe('Path and artifact name validation', () => {
+  beforeAll(() => {
+    // mock all output so that there is less noise when running tests
+    jest.spyOn(console, 'log').mockImplementation(() => {})
+    jest.spyOn(core, 'debug').mockImplementation(() => {})
+    jest.spyOn(core, 'info').mockImplementation(() => {})
+    jest.spyOn(core, 'warning').mockImplementation(() => {})
+  })
+
+  it('Check Artifact Name for any invalid characters', () => {
+    const invalidNames = [
+      'my\\artifact',
+      'my/artifact',
+      'my"artifact',
+      'my:artifact',
+      'my<artifact',
+      'my>artifact',
+      'my|artifact',
+      'my*artifact',
+      'my?artifact',
+      ''
+    ]
+    for (const invalidName of invalidNames) {
+      expect(() => {
+        validateArtifactName(invalidName)
+      }).toThrow()
+    }
+
+    const validNames = [
+      'my-normal-artifact',
+      'myNormalArtifact',
+      'm¥ñðrmålÄr†ï£å¢†'
+    ]
+    for (const validName of validNames) {
+      expect(() => {
+        validateArtifactName(validName)
+      }).not.toThrow()
+    }
+  })
+
+  it('Check Artifact File Path for any invalid characters', () => {
+    const invalidNames = [
+      'some/invalid"artifact/path',
+      'some/invalid:artifact/path',
+      'some/invalid<artifact/path',
+      'some/invalid>artifact/path',
+      'some/invalid|artifact/path',
+      'some/invalid*artifact/path',
+      'some/invalid?artifact/path',
+      'some/invalid\rartifact/path',
+      'some/invalid\nartifact/path',
+      'some/invalid\r\nartifact/path',
+      ''
+    ]
+    for (const invalidName of invalidNames) {
+      expect(() => {
+        validateFilePath(invalidName)
+      }).toThrow()
+    }
+
+    const validNames = [
+      'my/perfectly-normal/artifact-path',
+      'my/perfectly\\Normal/Artifact-path',
+      'm¥/ñðrmål/Är†ï£å¢†'
+    ]
+    for (const validName of validNames) {
+      expect(() => {
+        validateFilePath(validName)
+      }).not.toThrow()
+    }
+  })
+})
--- a/packages/artifact/tests/upload-zip-specification.test.ts
+++ b/packages/artifact/tests/upload-zip-specification.test.ts
@ -0,0 +1,312 @@
+import * as io from '../../io/src/io'
+import * as path from 'path'
+import {promises as fs} from 'fs'
+import * as core from '@actions/core'
+import {
+  getUploadZipSpecification,
+  validateRootDirectory
+} from '../src/internal/upload/upload-zip-specification'
+
+const root = path.join(__dirname, '_temp', 'upload-specification')
+const goodItem1Path = path.join(
+  root,
+  'folder-a',
+  'folder-b',
+  'folder-c',
+  'good-item1.txt'
+)
+const goodItem2Path = path.join(root, 'folder-d', 'good-item2.txt')
+const goodItem3Path = path.join(root, 'folder-d', 'good-item3.txt')
+const goodItem4Path = path.join(root, 'folder-d', 'good-item4.txt')
+const goodItem5Path = path.join(root, 'good-item5.txt')
+const badItem1Path = path.join(
+  root,
+  'folder-a',
+  'folder-b',
+  'folder-c',
+  'bad-item1.txt'
+)
+const badItem2Path = path.join(root, 'folder-d', 'bad-item2.txt')
+const badItem3Path = path.join(root, 'folder-f', 'bad-item3.txt')
+const badItem4Path = path.join(root, 'folder-h', 'folder-i', 'bad-item4.txt')
+const badItem5Path = path.join(root, 'folder-h', 'folder-i', 'bad-item5.txt')
+const extraFileInFolderCPath = path.join(
+  root,
+  'folder-a',
+  'folder-b',
+  'folder-c',
+  'extra-file-in-folder-c.txt'
+)
+const amazingFileInFolderHPath = path.join(root, 'folder-h', 'amazing-item.txt')
+
+const artifactFilesToUpload = [
+  goodItem1Path,
+  goodItem2Path,
+  goodItem3Path,
+  goodItem4Path,
+  goodItem5Path,
+  extraFileInFolderCPath,
+  amazingFileInFolderHPath
+]
+
+describe('Search', () => {
+  beforeAll(async () => {
+    // mock all output so that there is less noise when running tests
+    jest.spyOn(console, 'log').mockImplementation(() => {})
+    jest.spyOn(core, 'debug').mockImplementation(() => {})
+    jest.spyOn(core, 'info').mockImplementation(() => {})
+    jest.spyOn(core, 'warning').mockImplementation(() => {})
+
+    // clear temp directory
+    await io.rmRF(root)
+    await fs.mkdir(path.join(root, 'folder-a', 'folder-b', 'folder-c'), {
+      recursive: true
+    })
+    await fs.mkdir(path.join(root, 'folder-a', 'folder-b', 'folder-e'), {
+      recursive: true
+    })
+    await fs.mkdir(path.join(root, 'folder-d'), {
+      recursive: true
+    })
+    await fs.mkdir(path.join(root, 'folder-f'), {
+      recursive: true
+    })
+    await fs.mkdir(path.join(root, 'folder-g'), {
+      recursive: true
+    })
+    await fs.mkdir(path.join(root, 'folder-h', 'folder-i'), {
+      recursive: true
+    })
+
+    await fs.writeFile(goodItem1Path, 'good item1 file')
+    await fs.writeFile(goodItem2Path, 'good item2 file')
+    await fs.writeFile(goodItem3Path, 'good item3 file')
+    await fs.writeFile(goodItem4Path, 'good item4 file')
+    await fs.writeFile(goodItem5Path, 'good item5 file')
+
+    await fs.writeFile(badItem1Path, 'bad item1 file')
+    await fs.writeFile(badItem2Path, 'bad item2 file')
+    await fs.writeFile(badItem3Path, 'bad item3 file')
+    await fs.writeFile(badItem4Path, 'bad item4 file')
+    await fs.writeFile(badItem5Path, 'bad item5 file')
+
+    await fs.writeFile(extraFileInFolderCPath, 'extra file')
+
+    await fs.writeFile(amazingFileInFolderHPath, 'amazing file')
+    /*
+      Directory structure of files that get created:
+      root/
+          folder-a/
+              folder-b/
+                  folder-c/
+                      good-item1.txt
+                      bad-item1.txt
+                      extra-file-in-folder-c.txt
+                  folder-e/
+          folder-d/
+              good-item2.txt
+              good-item3.txt
+              good-item4.txt
+              bad-item2.txt
+          folder-f/
+              bad-item3.txt
+          folder-g/
+          folder-h/
+              amazing-item.txt
+              folder-i/
+                  bad-item4.txt
+                  bad-item5.txt
+          good-item5.txt
+    */
+  })
+
+  it('Upload Specification - Fail non-existent rootDirectory', async () => {
+    const invalidRootDirectory = path.join(
+      __dirname,
+      '_temp',
+      'upload-specification-invalid'
+    )
+    expect(() => {
+      validateRootDirectory(invalidRootDirectory)
+    }).toThrow(
+      `The provided rootDirectory ${invalidRootDirectory} does not exist`
+    )
+  })
+
+  it('Upload Specification - Fail invalid rootDirectory', async () => {
+    expect(() => {
+      validateRootDirectory(goodItem1Path)
+    }).toThrow(
+      `The provided rootDirectory ${goodItem1Path} is not a valid directory`
+    )
+  })
+
+  it('Upload Specification - File does not exist', async () => {
+    const fakeFilePath = path.join(
+      'folder-a',
+      'folder-b',
+      'non-existent-file.txt'
+    )
+    expect(() => {
+      getUploadZipSpecification([fakeFilePath], root)
+    }).toThrow(`File ${fakeFilePath} does not exist`)
+  })
+
+  it('Upload Specification - Non parent directory', async () => {
+    const folderADirectory = path.join(root, 'folder-a')
+    const artifactFiles = [
+      goodItem1Path,
+      badItem1Path,
+      extraFileInFolderCPath,
+      goodItem5Path
+    ]
+    expect(() => {
+      getUploadZipSpecification(artifactFiles, folderADirectory)
+    }).toThrow(
+      `The rootDirectory: ${folderADirectory} is not a parent directory of the file: ${goodItem5Path}`
+    )
+  })
+
+  it('Upload Specification - Success', async () => {
+    const specifications = getUploadZipSpecification(
+      artifactFilesToUpload,
+      root
+    )
+    expect(specifications.length).toEqual(7)
+
+    const absolutePaths = specifications.map(item => item.sourcePath)
+    expect(absolutePaths).toContain(goodItem1Path)
+    expect(absolutePaths).toContain(goodItem2Path)
+    expect(absolutePaths).toContain(goodItem3Path)
+    expect(absolutePaths).toContain(goodItem4Path)
+    expect(absolutePaths).toContain(goodItem5Path)
+    expect(absolutePaths).toContain(extraFileInFolderCPath)
+    expect(absolutePaths).toContain(amazingFileInFolderHPath)
+
+    for (const specification of specifications) {
+      if (specification.sourcePath === goodItem1Path) {
+        expect(specification.destinationPath).toEqual(
+          path.join('/folder-a', 'folder-b', 'folder-c', 'good-item1.txt')
+        )
+      } else if (specification.sourcePath === goodItem2Path) {
+        expect(specification.destinationPath).toEqual(
+          path.join('/folder-d', 'good-item2.txt')
+        )
+      } else if (specification.sourcePath === goodItem3Path) {
+        expect(specification.destinationPath).toEqual(
+          path.join('/folder-d', 'good-item3.txt')
+        )
+      } else if (specification.sourcePath === goodItem4Path) {
+        expect(specification.destinationPath).toEqual(
+          path.join('/folder-d', 'good-item4.txt')
+        )
+      } else if (specification.sourcePath === goodItem5Path) {
+        expect(specification.destinationPath).toEqual(
+          path.join('/good-item5.txt')
+        )
+      } else if (specification.sourcePath === extraFileInFolderCPath) {
+        expect(specification.destinationPath).toEqual(
+          path.join(
+            '/folder-a',
+            'folder-b',
+            'folder-c',
+            'extra-file-in-folder-c.txt'
+          )
+        )
+      } else if (specification.sourcePath === amazingFileInFolderHPath) {
+        expect(specification.destinationPath).toEqual(
+          path.join('/folder-h', 'amazing-item.txt')
+        )
+      } else {
+        throw new Error(
+          'Invalid specification found. This should never be reached'
+        )
+      }
+    }
+  })
+
+  it('Upload Specification - Success with extra slash', async () => {
+    const rootWithSlash = `${root}/`
+    const specifications = getUploadZipSpecification(
+      artifactFilesToUpload,
+      rootWithSlash
+    )
+    expect(specifications.length).toEqual(7)
+
+    const absolutePaths = specifications.map(item => item.sourcePath)
+    expect(absolutePaths).toContain(goodItem1Path)
+    expect(absolutePaths).toContain(goodItem2Path)
+    expect(absolutePaths).toContain(goodItem3Path)
+    expect(absolutePaths).toContain(goodItem4Path)
+    expect(absolutePaths).toContain(goodItem5Path)
+    expect(absolutePaths).toContain(extraFileInFolderCPath)
+    expect(absolutePaths).toContain(amazingFileInFolderHPath)
+
+    for (const specification of specifications) {
+      if (specification.sourcePath === goodItem1Path) {
+        expect(specification.destinationPath).toEqual(
+          path.join('/folder-a', 'folder-b', 'folder-c', 'good-item1.txt')
+        )
+      } else if (specification.sourcePath === goodItem2Path) {
+        expect(specification.destinationPath).toEqual(
+          path.join('/folder-d', 'good-item2.txt')
+        )
+      } else if (specification.sourcePath === goodItem3Path) {
+        expect(specification.destinationPath).toEqual(
+          path.join('/folder-d', 'good-item3.txt')
+        )
+      } else if (specification.sourcePath === goodItem4Path) {
+        expect(specification.destinationPath).toEqual(
+          path.join('/folder-d', 'good-item4.txt')
+        )
+      } else if (specification.sourcePath === goodItem5Path) {
+        expect(specification.destinationPath).toEqual(
+          path.join('/good-item5.txt')
+        )
+      } else if (specification.sourcePath === extraFileInFolderCPath) {
+        expect(specification.destinationPath).toEqual(
+          path.join(
+            '/folder-a',
+            'folder-b',
+            'folder-c',
+            'extra-file-in-folder-c.txt'
+          )
+        )
+      } else if (specification.sourcePath === amazingFileInFolderHPath) {
+        expect(specification.destinationPath).toEqual(
+          path.join('/folder-h', 'amazing-item.txt')
+        )
+      } else {
+        throw new Error(
+          'Invalid specification found. This should never be reached'
+        )
+      }
+    }
+  })
+
+  it('Upload Specification - Empty Directories are included', async () => {
+    const folderEPath = path.join(root, 'folder-a', 'folder-b', 'folder-e')
+    const filesWithDirectory = [goodItem1Path, folderEPath]
+    const specifications = getUploadZipSpecification(filesWithDirectory, root)
+    expect(specifications.length).toEqual(2)
+    const absolutePaths = specifications.map(item => item.sourcePath)
+    expect(absolutePaths).toContain(goodItem1Path)
+    expect(absolutePaths).toContain(null)
+
+    for (const specification of specifications) {
+      if (specification.sourcePath === goodItem1Path) {
+        expect(specification.destinationPath).toEqual(
+          path.join('/folder-a', 'folder-b', 'folder-c', 'good-item1.txt')
+        )
+      } else if (specification.sourcePath === null) {
+        expect(specification.destinationPath).toEqual(
+          path.join('/folder-a', 'folder-b', 'folder-e')
+        )
+      } else {
+        throw new Error(
+          'Invalid specification found. This should never be reached'
+        )
+      }
+    }
+  })
+})
--- a/packages/artifact/src/internal/upload/path-and-artifact-name-validation.ts
+++ b/packages/artifact/src/internal/upload/path-and-artifact-name-validation.ts
@ -0,0 +1,82 @@
+import {info} from '@actions/core'
+
+/**
+ * Invalid characters that cannot be in the artifact name or an uploaded file. Will be rejected
+ * from the server if attempted to be sent over. These characters are not allowed due to limitations with certain
+ * file systems such as NTFS. To maintain platform-agnostic behavior, all characters that are not supported by an
+ * individual filesystem/platform will not be supported on all fileSystems/platforms
+ *
+ * FilePaths can include characters such as \ and / which are not permitted in the artifact name alone
+ */
+const invalidArtifactFilePathCharacters = new Map<string, string>([
+  ['"', ' Double quote "'],
+  [':', ' Colon :'],
+  ['<', ' Less than <'],
+  ['>', ' Greater than >'],
+  ['|', ' Vertical bar |'],
+  ['*', ' Asterisk *'],
+  ['?', ' Question mark ?'],
+  ['\r', ' Carriage return \\r'],
+  ['\n', ' Line feed \\n']
+])
+
+const invalidArtifactNameCharacters = new Map<string, string>([
+  ...invalidArtifactFilePathCharacters,
+  ['\\', ' Backslash \\'],
+  ['/', ' Forward slash /']
+])
+
+/**
+ * Validates the name of the artifact to check to make sure there are no illegal characters
+ */
+export function validateArtifactName(name: string): void {
+  if (!name) {
+    throw new Error(`Provided artifact name input during validation is empty`)
+  }
+
+  for (const [
+    invalidCharacterKey,
+    errorMessageForCharacter
+  ] of invalidArtifactNameCharacters) {
+    if (name.includes(invalidCharacterKey)) {
+      throw new Error(
+        `The artifact name is not valid: ${name}. Contains the following character: ${errorMessageForCharacter}
+          
+Invalid characters include: ${Array.from(
+          invalidArtifactNameCharacters.values()
+        ).toString()}
+          
+These characters are not allowed in the artifact name due to limitations with certain file systems such as NTFS. To maintain file system agnostic behavior, these characters are intentionally not allowed to prevent potential problems with downloads on different file systems.`
+      )
+    }
+  }
+
+  info(`Artifact name is valid!`)
+}
+
+/**
+ * Validates file paths to check for any illegal characters that can cause problems on different file systems
+ */
+export function validateFilePath(path: string): void {
+  if (!path) {
+    throw new Error(`Provided file path input during validation is empty`)
+  }
+
+  for (const [
+    invalidCharacterKey,
+    errorMessageForCharacter
+  ] of invalidArtifactFilePathCharacters) {
+    if (path.includes(invalidCharacterKey)) {
+      throw new Error(
+        `The path for one of the files in artifact is not valid: ${path}. Contains the following character: ${errorMessageForCharacter}
+          
+Invalid characters include: ${Array.from(
+          invalidArtifactFilePathCharacters.values()
+        ).toString()}
+          
+The following characters are not allowed in files that are uploaded due to limitations with certain file systems such as NTFS. To maintain file system agnostic behavior, these characters are intentionally not allowed to prevent potential problems with downloads on different file systems.
+          `
+      )
+    }
+  }
+}
--- a/packages/artifact/src/internal/upload/upload-artifact.ts
+++ b/packages/artifact/src/internal/upload/upload-artifact.ts
@ -1,17 +1,39 @@
+import * as core from '@actions/core'
 import {UploadOptions} from './upload-options'
 import {UploadResponse} from './upload-response'
+import {validateArtifactName} from './path-and-artifact-name-validation'
+import {
+  UploadZipSpecification,
+  getUploadZipSpecification,
+  validateRootDirectory
+} from './upload-zip-specification'

 export async function uploadArtifact(
  name: string,
-  files: string[], // eslint-disable-line @typescript-eslint/no-unused-vars
-  rootDirectory: string, // eslint-disable-line @typescript-eslint/no-unused-vars
+  files: string[],
+  rootDirectory: string,
  options?: UploadOptions | undefined // eslint-disable-line @typescript-eslint/no-unused-vars
 ): Promise<UploadResponse> {
+  validateArtifactName(name)
+  validateRootDirectory(rootDirectory)
+
+  const zipSpecification: UploadZipSpecification[] = getUploadZipSpecification(
+    files,
+    rootDirectory
+  )
+  if (zipSpecification.length === 0) {
+    core.warning(`No files were found to upload`)
+    return {
+      success: false
+    }
+  }
+
  // TODO - Implement upload functionality

  const uploadResponse: UploadResponse = {
-    artifactName: name,
-    size: 0
+    success: true,
+    size: 0,
+    id: 0
  }

  return uploadResponse
--- a/packages/artifact/src/internal/upload/upload-response.ts
+++ b/packages/artifact/src/internal/upload/upload-response.ts
@ -1,11 +1,17 @@
 export interface UploadResponse {
  /**
-   * The name of the artifact that was uploaded
+   * Denotes if an artifact was successfully uploaded
   */
-  artifactName: string
+  success: boolean

  /**
-   * Total size of the artifact that was uploaded in bytes
+   * Total size of the artifact in bytes. Not provided if no artifact was uploaded
   */
-  size: number
+  size?: number
+
+  /**
+   * The id of the artifact that was created. Not provided if no artifact was uploaded
+   * This ID can be used as input to other APIs to download, delete or get more information about an artifact: https://docs.github.com/en/rest/actions/artifacts
+   */
+  id?: number
 }
--- a/packages/artifact/src/internal/upload/upload-zip-specification.ts
+++ b/packages/artifact/src/internal/upload/upload-zip-specification.ts
@ -0,0 +1,111 @@
+import * as fs from 'fs'
+import {info} from '@actions/core'
+import {normalize, resolve} from 'path'
+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
+   */
+  sourcePath: string | null
+
+  /**
+   * The destination path in a zip for a file
+   */
+  destinationPath: string
+}
+
+/**
+ * Checks if a root directory exists and is valid
+ * @param rootDirectory an absolute root directory path common to all input files that that will be trimmed from the final zip structure
+ */
+export function validateRootDirectory(rootDirectory: string): void {
+  if (!fs.existsSync(rootDirectory)) {
+    throw new Error(
+      `The provided rootDirectory ${rootDirectory} does not exist`
+    )
+  }
+  if (!fs.statSync(rootDirectory).isDirectory()) {
+    throw new Error(
+      `The provided rootDirectory ${rootDirectory} is not a valid directory`
+    )
+  }
+  info(`Root directory input is valid!`)
+}
+
+/**
+ * Creates a specification that describes how a zip file will be created for a set of input files
+ * @param filesToZip a list of file that should be included in the zip
+ * @param rootDirectory an absolute root directory path common to all input files that that will be trimmed from the final zip structure
+ */
+export function getUploadZipSpecification(
+  filesToZip: string[],
+  rootDirectory: string
+): UploadZipSpecification[] {
+  const specification: UploadZipSpecification[] = []
+
+  // Normalize and resolve, this allows for either absolute or relative paths to be used
+  rootDirectory = normalize(rootDirectory)
+  rootDirectory = resolve(rootDirectory)
+
+  /*
+     Example
+     
+     Input:
+       rootDirectory: '/home/user/files/plz-upload'
+       artifactFiles: [
+         '/home/user/files/plz-upload/file1.txt',
+         '/home/user/files/plz-upload/file2.txt',
+         '/home/user/files/plz-upload/dir/file3.txt'
+       ]
+     
+     Output:
+       specifications: [
+         ['/home/user/files/plz-upload/file1.txt', '/file1.txt'],
+         ['/home/user/files/plz-upload/file1.txt', '/file2.txt'],
+         ['/home/user/files/plz-upload/file1.txt', '/dir/file3.txt']
+       ]
+
+      The final zip that is later uploaded will look like this:
+
+      my-artifact.zip
+        - file.txt
+        - file2.txt
+        - dir/
+          - file3.txt
+  */
+  for (let file of filesToZip) {
+    if (!fs.existsSync(file)) {
+      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)
+
+      specification.push({
+        sourcePath: file,
+        destinationPath: uploadPath
+      })
+    } else {
+      // Empty directory
+      const directoryPath = file.replace(rootDirectory, '')
+      validateFilePath(directoryPath)
+
+      specification.push({
+        sourcePath: null,
+        destinationPath: directoryPath
+      })
+    }
+  }
+  return specification
+}