From 88062ec4731967e1d4d835dee48b663379430edc Mon Sep 17 00:00:00 2001 From: Konrad Pabjan Date: Wed, 1 Dec 2021 16:31:37 -0500 Subject: [PATCH] Check for newlines and carriage return in artifact paths and name (#951) * Check for newlines and carriage return in artifact paths and name * Fix linting issue * Update comments * Add comment about spacing * Remove extra space --- .../path-and-artifact-name-validation.test.ts | 78 ++++++++++++++++++ packages/artifact/__tests__/util.test.ts | 60 -------------- .../artifact/src/internal/artifact-client.ts | 2 +- .../path-and-artifact-name-validation.ts | 82 +++++++++++++++++++ .../src/internal/upload-specification.ts | 2 +- packages/artifact/src/internal/utils.ts | 58 ------------- 6 files changed, 162 insertions(+), 120 deletions(-) create mode 100644 packages/artifact/__tests__/path-and-artifact-name-validation.test.ts create mode 100644 packages/artifact/src/internal/path-and-artifact-name-validation.ts diff --git a/packages/artifact/__tests__/path-and-artifact-name-validation.test.ts b/packages/artifact/__tests__/path-and-artifact-name-validation.test.ts new file mode 100644 index 00000000..58622c97 --- /dev/null +++ b/packages/artifact/__tests__/path-and-artifact-name-validation.test.ts @@ -0,0 +1,78 @@ +import { + checkArtifactName, + checkArtifactFilePath +} from '../src/internal/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', + 'myartifact', + 'my|artifact', + 'my*artifact', + 'my?artifact', + '' + ] + for (const invalidName of invalidNames) { + expect(() => { + checkArtifactName(invalidName) + }).toThrow() + } + + const validNames = [ + 'my-normal-artifact', + 'myNormalArtifact', + 'm¥ñðrmålÄr†ï£å¢†' + ] + for (const validName of validNames) { + expect(() => { + checkArtifactName(validName) + }).not.toThrow() + } + }) + + it('Check Artifact File Path for any invalid characters', () => { + const invalidNames = [ + 'some/invalid"artifact/path', + 'some/invalid:artifact/path', + 'some/invalidartifact/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(() => { + checkArtifactFilePath(invalidName) + }).toThrow() + } + + const validNames = [ + 'my/perfectly-normal/artifact-path', + 'my/perfectly\\Normal/Artifact-path', + 'm¥/ñðrmål/Är†ï£å¢†' + ] + for (const validName of validNames) { + expect(() => { + checkArtifactFilePath(validName) + }).not.toThrow() + } + }) +}) diff --git a/packages/artifact/__tests__/util.test.ts b/packages/artifact/__tests__/util.test.ts index 1513566f..c353cdc6 100644 --- a/packages/artifact/__tests__/util.test.ts +++ b/packages/artifact/__tests__/util.test.ts @@ -46,66 +46,6 @@ describe('Utils', () => { } }) - it('Check Artifact Name for any invalid characters', () => { - const invalidNames = [ - 'my\\artifact', - 'my/artifact', - 'my"artifact', - 'my:artifact', - 'myartifact', - 'my|artifact', - 'my*artifact', - 'my?artifact', - '' - ] - for (const invalidName of invalidNames) { - expect(() => { - utils.checkArtifactName(invalidName) - }).toThrow() - } - - const validNames = [ - 'my-normal-artifact', - 'myNormalArtifact', - 'm¥ñðrmålÄr†ï£å¢†' - ] - for (const validName of validNames) { - expect(() => { - utils.checkArtifactName(validName) - }).not.toThrow() - } - }) - - it('Check Artifact File Path for any invalid characters', () => { - const invalidNames = [ - 'some/invalid"artifact/path', - 'some/invalid:artifact/path', - 'some/invalidartifact/path', - 'some/invalid|artifact/path', - 'some/invalid*artifact/path', - 'some/invalid?artifact/path', - '' - ] - for (const invalidName of invalidNames) { - expect(() => { - utils.checkArtifactFilePath(invalidName) - }).toThrow() - } - - const validNames = [ - 'my/perfectly-normal/artifact-path', - 'my/perfectly\\Normal/Artifact-path', - 'm¥/ñðrmål/Är†ï£å¢†' - ] - for (const validName of validNames) { - expect(() => { - utils.checkArtifactFilePath(validName) - }).not.toThrow() - } - }) - it('Test negative artifact retention throws', () => { expect(() => { utils.getProperRetention(-1, undefined) diff --git a/packages/artifact/src/internal/artifact-client.ts b/packages/artifact/src/internal/artifact-client.ts index 730e19d5..49f8b601 100644 --- a/packages/artifact/src/internal/artifact-client.ts +++ b/packages/artifact/src/internal/artifact-client.ts @@ -9,10 +9,10 @@ import {UploadOptions} from './upload-options' import {DownloadOptions} from './download-options' import {DownloadResponse} from './download-response' import { - checkArtifactName, createDirectoriesForArtifact, createEmptyFilesForArtifact } from './utils' +import {checkArtifactName} from './path-and-artifact-name-validation' import {DownloadHttpClient} from './download-http-client' import {getDownloadSpecification} from './download-specification' import {getWorkSpaceDirectory} from './config-variables' diff --git a/packages/artifact/src/internal/path-and-artifact-name-validation.ts b/packages/artifact/src/internal/path-and-artifact-name-validation.ts new file mode 100644 index 00000000..65d4ab53 --- /dev/null +++ b/packages/artifact/src/internal/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([ + ['"', ' Double quote "'], + [':', ' Colon :'], + ['<', ' Less than <'], + ['>', ' Greater than >'], + ['|', ' Vertical bar |'], + ['*', ' Asterisk *'], + ['?', ' Question mark ?'], + ['\r', ' Carriage return \\r'], + ['\n', ' Line feed \\n'] +]) + +const invalidArtifactNameCharacters = new Map([ + ...invalidArtifactFilePathCharacters, + ['\\', ' Backslash \\'], + ['/', ' Forward slash /'] +]) + +/** + * Scans the name of the artifact to make sure there are no illegal characters + */ +export function checkArtifactName(name: string): void { + if (!name) { + throw new Error(`Artifact name: ${name}, is incorrectly provided`) + } + + for (const [ + invalidCharacterKey, + errorMessageForCharacter + ] of invalidArtifactNameCharacters) { + if (name.includes(invalidCharacterKey)) { + throw new Error( + `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!`) +} + +/** + * Scans the name of the filePath used to make sure there are no illegal characters + */ +export function checkArtifactFilePath(path: string): void { + if (!path) { + throw new Error(`Artifact path: ${path}, is incorrectly provided`) + } + + for (const [ + invalidCharacterKey, + errorMessageForCharacter + ] of invalidArtifactFilePathCharacters) { + if (path.includes(invalidCharacterKey)) { + throw new Error( + `Artifact path 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. + ` + ) + } + } +} diff --git a/packages/artifact/src/internal/upload-specification.ts b/packages/artifact/src/internal/upload-specification.ts index 884a5967..68468c4d 100644 --- a/packages/artifact/src/internal/upload-specification.ts +++ b/packages/artifact/src/internal/upload-specification.ts @@ -1,7 +1,7 @@ import * as fs from 'fs' import {debug} from '@actions/core' import {join, normalize, resolve} from 'path' -import {checkArtifactFilePath} from './utils' +import {checkArtifactFilePath} from './path-and-artifact-name-validation' export interface UploadSpecification { absoluteFilePath: string diff --git a/packages/artifact/src/internal/utils.ts b/packages/artifact/src/internal/utils.ts index 8d91444f..f2b99f33 100644 --- a/packages/artifact/src/internal/utils.ts +++ b/packages/artifact/src/internal/utils.ts @@ -237,64 +237,6 @@ Header Information: ${JSON.stringify(response.message.headers, undefined, 2)} ) } -/** - * 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 = ['"', ':', '<', '>', '|', '*', '?'] -const invalidArtifactNameCharacters = [ - ...invalidArtifactFilePathCharacters, - '\\', - '/' -] - -/** - * Scans the name of the artifact to make sure there are no illegal characters - */ -export function checkArtifactName(name: string): void { - if (!name) { - throw new Error(`Artifact name: ${name}, is incorrectly provided`) - } - - for (const invalidChar of invalidArtifactNameCharacters) { - if (name.includes(invalidChar)) { - throw new Error( - `Artifact name is not valid: ${name}. Contains the following character: "${invalidChar}". - -Invalid characters include: ${invalidArtifactNameCharacters.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!`) -} - -/** - * Scans the name of the filePath used to make sure there are no illegal characters - */ -export function checkArtifactFilePath(path: string): void { - if (!path) { - throw new Error(`Artifact path: ${path}, is incorrectly provided`) - } - - for (const invalidChar of invalidArtifactFilePathCharacters) { - if (path.includes(invalidChar)) { - throw new Error( - `Artifact path is not valid: ${path}. Contains the following character: "${invalidChar}". Invalid characters include: ${invalidArtifactFilePathCharacters.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. - ` - ) - } - } -} - export async function createDirectoriesForArtifact( directories: string[] ): Promise {