From cf207e602a7b8c6b9efeca740e8c5bf657569a8c Mon Sep 17 00:00:00 2001 From: Johan Nyman Date: Thu, 5 Sep 2024 10:35:04 +0200 Subject: [PATCH 1/2] fix: reject absolute file paths --- .../generic/src/storage/fileStorage.ts | 4 +- shared/packages/api/src/__tests__/lib.spec.ts | 46 ++++++++++- shared/packages/api/src/lib.ts | 49 +++++++++++ .../__tests__/fileShare.spec.ts | 82 +++++++++++++++++++ .../worker/accessorHandlers/__tests__/lib.ts | 65 +++++++++++++++ .../__tests__/localFolder.spec.ts | 79 ++++++++++++++++++ .../src/worker/accessorHandlers/atem.ts | 65 +++++++-------- .../accessorHandlers/corePackageInfo.ts | 19 +++-- .../src/worker/accessorHandlers/fileShare.ts | 59 +++++++++---- .../worker/accessorHandlers/genericHandle.ts | 7 ++ .../src/worker/accessorHandlers/http.ts | 56 +++++++------ .../src/worker/accessorHandlers/httpProxy.ts | 65 +++++++-------- .../accessorHandlers/lib/FileHandler.ts | 4 +- .../worker/accessorHandlers/localFolder.ts | 65 ++++++++++----- .../src/worker/accessorHandlers/quantel.ts | 41 +++++----- .../genericWorker/expectationHandlers/lib.ts | 12 +++ .../src/__mocks__/child_process.ts | 2 +- 17 files changed, 560 insertions(+), 160 deletions(-) create mode 100644 shared/packages/worker/src/worker/accessorHandlers/__tests__/fileShare.spec.ts create mode 100644 shared/packages/worker/src/worker/accessorHandlers/__tests__/lib.ts create mode 100644 shared/packages/worker/src/worker/accessorHandlers/__tests__/localFolder.spec.ts diff --git a/apps/http-server/packages/generic/src/storage/fileStorage.ts b/apps/http-server/packages/generic/src/storage/fileStorage.ts index d3b46e90..05e521a8 100644 --- a/apps/http-server/packages/generic/src/storage/fileStorage.ts +++ b/apps/http-server/packages/generic/src/storage/fileStorage.ts @@ -4,7 +4,7 @@ import { promisify } from 'util' import mime from 'mime-types' import prettyBytes from 'pretty-bytes' import { asyncPipe, CTXPost } from '../lib' -import { HTTPServerConfig, LoggerInstance } from '@sofie-package-manager/api' +import { betterPathResolve, HTTPServerConfig, LoggerInstance } from '@sofie-package-manager/api' import { BadResponse, PackageInfo, ResponseMeta, Storage } from './storage' import { Readable } from 'stream' @@ -31,7 +31,7 @@ export class FileStorage extends Storage { private logger: LoggerInstance constructor(logger: LoggerInstance, private config: HTTPServerConfig) { super() - this._basePath = path.resolve(this.config.httpServer.basePath) + this._basePath = betterPathResolve(this.config.httpServer.basePath) this.logger = logger.category('FileStorage') // Run this on startup, so that if there are any critical errors we'll see them right away: diff --git a/shared/packages/api/src/__tests__/lib.spec.ts b/shared/packages/api/src/__tests__/lib.spec.ts index edcad342..64ec53a5 100644 --- a/shared/packages/api/src/__tests__/lib.spec.ts +++ b/shared/packages/api/src/__tests__/lib.spec.ts @@ -1,4 +1,15 @@ -import { deferGets, diff, promiseTimeout, stringMaxLength, stringifyError, waitTime } from '../lib' +import * as path from 'path' +import { + betterPathIsAbsolute, + betterPathJoin, + betterPathResolve, + deferGets, + diff, + promiseTimeout, + stringMaxLength, + stringifyError, + waitTime, +} from '../lib' describe('lib', () => { test('diff', () => { @@ -226,5 +237,38 @@ describe('lib', () => { expect(stringMaxLength('0123456789abcdefg', 9)).toBe('012...efg') expect(stringMaxLength('0123456789abcdefg', 8)).toBe('01...efg') }) + test('betterPathJoin', () => { + const fixSep = (str: string) => str.replace(/\//g, path.sep) + + expect(betterPathJoin('\\\\a\\b/c')).toBe(fixSep('//a/b/c')) + expect(betterPathJoin('a/b', 'c')).toBe(fixSep('a/b/c')) + expect(betterPathJoin('a/b', '../c')).toBe(fixSep('a/c')) + expect(betterPathJoin('a/b', '/c')).toBe(fixSep('a/b/c')) + expect(betterPathJoin('C:\\a\\b', 'c\\d')).toBe(fixSep('C:/a/b/c/d')) + expect(betterPathJoin('C:\\a\\b', '../c')).toBe(fixSep('C:/a/c')) + + expect(betterPathJoin('\\\\a\\b', '../c')).toBe(fixSep('//a/c')) // This is where path.join fails + expect(betterPathJoin('//a/b', '../c')).toBe(fixSep('//a/c')) // This is where path.join fails + }) + test('betterPathResolve', () => { + const fixSep = (str: string) => str.replace(/\//g, path.sep) + + expect(betterPathResolve('a/b/c')).toBe(path.resolve(fixSep('a/b/c'))) + expect(betterPathResolve('a/b/c')).toBe(path.resolve(fixSep('a/b/c'))) + + expect(betterPathResolve('\\\\a\\b')).toBe(fixSep('//a/b')) + expect(betterPathResolve('\\\\a\\b\\c')).toBe(fixSep('//a/b/c')) + }) + test('betterPathIsAbsolute', () => { + expect(betterPathIsAbsolute('a/b/c')).toBe(false) + expect(betterPathIsAbsolute('./a/b/c')).toBe(false) + expect(betterPathIsAbsolute('../a/b/c')).toBe(false) + + expect(betterPathIsAbsolute('/a')).toBe(true) + expect(betterPathIsAbsolute('C:\\a\\b\\c')).toBe(true) + expect(betterPathIsAbsolute('\\\\a\\b\\c')).toBe(true) + expect(betterPathIsAbsolute('\\a')).toBe(true) + expect(betterPathIsAbsolute('//a')).toBe(true) + }) }) export {} diff --git a/shared/packages/api/src/lib.ts b/shared/packages/api/src/lib.ts index 246594e0..817e5786 100644 --- a/shared/packages/api/src/lib.ts +++ b/shared/packages/api/src/lib.ts @@ -1,4 +1,5 @@ import crypto from 'crypto' +import path from 'path' import { compact } from 'underscore' import { AnyProtectedString } from './ProtectedString' @@ -382,6 +383,54 @@ export function mapToObject(map: Map): { [key: string]: T } { }) return o } +/** + * Like path.join(), + * but this fixes an issue in path.join where it doesn't handle paths double slashes together with relative paths correctly + * And it also returns predictable results on both Windows and Linux + */ +export function betterPathJoin(...paths: string[]): string { + // Replace slashes with the correct path separator, because "C:\\a\\b" is not interpreted correctly on Linux (but C:/a/b is) + paths = paths.map((p) => p.replace(/[\\/]/g, path.sep)) + + let firstPath = paths[0] + const restPaths = paths.slice(1) + + let prefix = '' + if (firstPath.startsWith('//') || firstPath.startsWith('\\\\')) { + // Preserve the prefix, as path.join will remove it: + prefix = path.sep + path.sep + firstPath = firstPath.slice(2) + } + + return prefix + path.join(firstPath, ...restPaths) +} +/** + * Like path.resolve(), + * but it returns predictable results on both Windows and Linux + */ +export function betterPathResolve(p: string): string { + p = p.replace(/[\\/]/g, path.sep) + + // let prefix = '' + if (p.startsWith('//') || p.startsWith('\\\\')) { + return path.sep + path.sep + path.normalize(p.slice(2)) + } else { + return path.resolve(p) + } +} +/** + * Like path.isAbsolute(), + * but it returns same results on both Windows and Linux + */ +export function betterPathIsAbsolute(p: string): boolean { + return ( + path.isAbsolute(p) || + Boolean(p.match(/^\w:/)) || // C:\, path.isAbsolute() doesn't handle this on Linux + p.startsWith('\\\\') || // \\server\path, path.isAbsolute() doesn't handle this on Linux + p.startsWith('\\') // \server\path, path.isAbsolute() doesn't handle this on Linux + ) +} + /** Returns true if we're running tests (in Jest) */ export function isRunningInTest(): boolean { // Note: JEST_WORKER_ID is set when running in unit tests diff --git a/shared/packages/worker/src/worker/accessorHandlers/__tests__/fileShare.spec.ts b/shared/packages/worker/src/worker/accessorHandlers/__tests__/fileShare.spec.ts new file mode 100644 index 00000000..1321bbd4 --- /dev/null +++ b/shared/packages/worker/src/worker/accessorHandlers/__tests__/fileShare.spec.ts @@ -0,0 +1,82 @@ +import { + AccessorOnPackage, + protectString, + setupLogger, + initializeLogger, + ProcessConfig, + Expectation, + Accessor, +} from '@sofie-package-manager/api' +import { Content, FileShareAccessorHandle } from '../fileShare' +import { PassiveTestWorker } from './lib' + +const processConfig: ProcessConfig = { + logPath: undefined, + logLevel: undefined, + unsafeSSL: false, + certificates: [], +} +initializeLogger({ process: processConfig }) +test('checkHandleBasic', () => { + const logger = setupLogger( + { + process: processConfig, + }, + '' + ) + const worker = new PassiveTestWorker(logger) + + function getFileShareAccessor( + accessor: AccessorOnPackage.FileShare, + content: Content, + workOptions: Expectation.WorkOptions.Base & + Expectation.WorkOptions.RemoveDelay & + Expectation.WorkOptions.UseTemporaryFilePath = {} + ) { + accessor.type = Accessor.AccessType.FILE_SHARE + return new FileShareAccessorHandle(worker, protectString('share0'), accessor, content, workOptions) + } + + expect(() => getFileShareAccessor({}, {}).checkHandleBasic()).toThrowError('Bad input data') + + // missing accessor.folderPath: + expect(getFileShareAccessor({}, { filePath: 'amb.amp4' }).checkHandleBasic()).toMatchObject({ + success: false, + reason: { tech: 'Folder path not set' }, + }) + + // All OK: + expect( + getFileShareAccessor({ folderPath: '\\\\nas01\\media' }, { filePath: 'amb.amp4' }).checkHandleBasic() + ).toMatchObject({ + success: true, + }) + + // Absolute file path: + expect( + getFileShareAccessor({ folderPath: '\\\\nas01\\media' }, { filePath: '//secret/amb.amp4' }).checkHandleBasic() + ).toMatchObject({ + success: false, + reason: { tech: expect.stringMatching(/File path.*absolute path/) }, + }) + expect( + getFileShareAccessor( + { folderPath: '\\\\nas01\\media' }, + { filePath: 'C:\\secret\\amb.amp4' } + ).checkHandleBasic() + ).toMatchObject({ + success: false, + reason: { tech: expect.stringMatching(/File path.*absolute path/) }, + }) + + // File path outside of folder path: + expect( + getFileShareAccessor({ folderPath: '//nas01/media' }, { filePath: '../secret/amb.amp4' }).checkHandleBasic() + ).toMatchObject({ + success: false, + reason: { + user: `File path is outside of folder path`, + tech: expect.stringMatching(/Full path.*does not start with/), + }, + }) +}) diff --git a/shared/packages/worker/src/worker/accessorHandlers/__tests__/lib.ts b/shared/packages/worker/src/worker/accessorHandlers/__tests__/lib.ts new file mode 100644 index 00000000..1c32d539 --- /dev/null +++ b/shared/packages/worker/src/worker/accessorHandlers/__tests__/lib.ts @@ -0,0 +1,65 @@ +import { protectString, LoggerInstance } from '@sofie-package-manager/api' +import { BaseWorker } from '../../worker' + +export class PassiveTestWorker extends BaseWorker { + constructor(logger: LoggerInstance) { + super( + logger, + { + config: { + workerId: protectString('test'), + sourcePackageStabilityThreshold: 0, + windowsDriveLetters: ['X', 'Y', 'Z'], + }, + location: { + // localComputerId?: string + localNetworkIds: [], + }, + workerStorageWrite: () => { + throw new Error('Method not implemented.') + }, + workerStorageRead: () => { + throw new Error('Method not implemented.') + }, + }, + async () => { + throw new Error('Method not implemented.') + }, + 'passive-test-worker' + ) + } + + async init() { + throw new Error('Method not implemented.') + } + terminate() { + throw new Error('Method not implemented.') + } + async doYouSupportExpectation(): Promise { + throw new Error('Method not implemented.') + } + async getCostFortExpectation(): Promise { + throw new Error('Method not implemented.') + } + async isExpectationReadyToStartWorkingOn(): Promise { + throw new Error('Method not implemented.') + } + async isExpectationFulfilled(): Promise { + throw new Error('Method not implemented.') + } + async workOnExpectation(): Promise { + throw new Error('Method not implemented.') + } + async removeExpectation(): Promise { + throw new Error('Method not implemented.') + } + async doYouSupportPackageContainer(): Promise { + throw new Error('Method not implemented.') + } + async runPackageContainerCronJob(): Promise { + throw new Error('Method not implemented.') + } + async setupPackageContainerMonitors(): Promise { + throw new Error('Method not implemented.') + } +} diff --git a/shared/packages/worker/src/worker/accessorHandlers/__tests__/localFolder.spec.ts b/shared/packages/worker/src/worker/accessorHandlers/__tests__/localFolder.spec.ts new file mode 100644 index 00000000..8e760065 --- /dev/null +++ b/shared/packages/worker/src/worker/accessorHandlers/__tests__/localFolder.spec.ts @@ -0,0 +1,79 @@ +import { + AccessorOnPackage, + protectString, + setupLogger, + initializeLogger, + ProcessConfig, + Expectation, + Accessor, +} from '@sofie-package-manager/api' +import { Content, LocalFolderAccessorHandle } from '../localFolder' +import { PassiveTestWorker } from './lib' + +const processConfig: ProcessConfig = { + logPath: undefined, + logLevel: undefined, + unsafeSSL: false, + certificates: [], +} +initializeLogger({ process: processConfig }) +test('checkHandleBasic', () => { + const logger = setupLogger( + { + process: processConfig, + }, + '' + ) + const worker = new PassiveTestWorker(logger) + + function getLocalFolderAccessor( + accessor: AccessorOnPackage.LocalFolder, + content: Content, + workOptions: Expectation.WorkOptions.Base & + Expectation.WorkOptions.RemoveDelay & + Expectation.WorkOptions.UseTemporaryFilePath = {} + ) { + accessor.type = Accessor.AccessType.LOCAL_FOLDER + return new LocalFolderAccessorHandle(worker, protectString('local0'), accessor, content, workOptions) + } + + expect(() => getLocalFolderAccessor({}, {}).checkHandleBasic()).toThrowError('Bad input data') + + // missing accessor.folderPath: + expect(getLocalFolderAccessor({}, { filePath: 'amb.amp4' }).checkHandleBasic()).toMatchObject({ + success: false, + reason: { tech: 'Folder path not set' }, + }) + + // All OK: + expect(getLocalFolderAccessor({ folderPath: '/a/b/c' }, { filePath: 'amb.amp4' }).checkHandleBasic()).toMatchObject( + { + success: true, + } + ) + + // Absolute file path: + expect( + getLocalFolderAccessor({ folderPath: '/base/media' }, { filePath: '//secret/amb.amp4' }).checkHandleBasic() + ).toMatchObject({ + success: false, + reason: { tech: expect.stringMatching(/File path.*absolute path/) }, + }) + expect( + getLocalFolderAccessor({ folderPath: 'D:\\media' }, { filePath: 'C:\\secret\\amb.amp4' }).checkHandleBasic() + ).toMatchObject({ + success: false, + reason: { tech: expect.stringMatching(/File path.*absolute path/) }, + }) + + // File path outside of folder path: + expect( + getLocalFolderAccessor({ folderPath: '/base/media' }, { filePath: '../secret/amb.amp4' }).checkHandleBasic() + ).toMatchObject({ + success: false, + reason: { + user: `File path is outside of folder path`, + tech: expect.stringMatching(/Full path.*does not start with/), + }, + }) +}) diff --git a/shared/packages/worker/src/worker/accessorHandlers/atem.ts b/shared/packages/worker/src/worker/accessorHandlers/atem.ts index 5aa5614a..0e97079e 100644 --- a/shared/packages/worker/src/worker/accessorHandlers/atem.ts +++ b/shared/packages/worker/src/worker/accessorHandlers/atem.ts @@ -11,6 +11,7 @@ import { AccessorHandlerTryPackageReadResult, AccessorHandlerRunCronJobResult, PackageOperation, + AccessorHandlerCheckHandleBasicResult, } from './genericHandle' import { Expectation, Accessor, AccessorOnPackage, AccessorId, escapeFilePath } from '@sofie-package-manager/api' import { BaseWorker } from '../worker' @@ -72,15 +73,45 @@ export class ATEMAccessorHandle extends GenericAccessorHandle { // Check if the package exists: @@ -423,36 +454,6 @@ export class ATEMAccessorHandle extends GenericAccessorHandle extends GenericAccessorHand get packageName(): string { return 'PackageInfo' // Not really supported for this type of accessor } - checkHandleRead(): AccessorHandlerCheckHandleReadResult { - // Note: We assume that we always have write access here, no need to check this.accessor.allowRead - return this.checkAccessor() - } - checkHandleWrite(): AccessorHandlerCheckHandleWriteResult { - // Note: We assume that we always have write access here, no need to check this.accessor.allowWrite - return this.checkAccessor() - } - private checkAccessor(): AccessorHandlerCheckHandleWriteResult { + checkHandleBasic(): AccessorHandlerCheckHandleBasicResult { if (this.accessor.type !== Accessor.AccessType.CORE_PACKAGE_INFO) { return { success: false, @@ -77,6 +70,14 @@ export class CorePackageInfoAccessorHandle extends GenericAccessorHand } return { success: true } } + checkHandleRead(): AccessorHandlerCheckHandleReadResult { + // Note: We assume that we always have write access here, no need to check this.accessor.allowRead + return { success: true } + } + checkHandleWrite(): AccessorHandlerCheckHandleWriteResult { + // Note: We assume that we always have write access here, no need to check this.accessor.allowWrite + return { success: true } + } async checkPackageReadAccess(): Promise { // todo: add a check here? return { success: true } diff --git a/shared/packages/worker/src/worker/accessorHandlers/fileShare.ts b/shared/packages/worker/src/worker/accessorHandlers/fileShare.ts index 288cde66..5901a767 100644 --- a/shared/packages/worker/src/worker/accessorHandlers/fileShare.ts +++ b/shared/packages/worker/src/worker/accessorHandlers/fileShare.ts @@ -1,6 +1,7 @@ import { promisify } from 'util' import fs from 'fs' import { + AccessorHandlerCheckHandleBasicResult, AccessorHandlerCheckHandleReadResult, AccessorHandlerCheckHandleWriteResult, AccessorHandlerCheckPackageContainerWriteAccessResult, @@ -27,6 +28,8 @@ import { DataId, AccessorId, MonitorId, + betterPathResolve, + betterPathIsAbsolute, } from '@sofie-package-manager/api' import { BaseWorker } from '../worker' import { GenericWorker } from '../workers/genericWorker/genericWorker' @@ -51,6 +54,12 @@ const pExec = promisify(exec) const PREPARE_FILE_ACCESS_TIMEOUT = INNER_ACTION_TIMEOUT * 0.5 const PREPARE_FILE_ACCESS_TIMEOUT_INNER = PREPARE_FILE_ACCESS_TIMEOUT * 0.8 +export interface Content { + /** This is set when the class-instance is only going to be used for PackageContainer access.*/ + onlyContainerAccess?: boolean + filePath?: string +} + /** Accessor handle for accessing files on a network share */ export class FileShareAccessorHandle extends GenericFileAccessorHandle { static readonly type = FileShareAccessorHandleType @@ -59,11 +68,7 @@ export class FileShareAccessorHandle extends GenericFileAccessorHandle public disableDriveMapping = false - private content: { - /** This is set when the class-instance is only going to be used for PackageContainer access.*/ - onlyContainerAccess?: boolean - filePath?: string - } + private content: Content private workOptions: Expectation.WorkOptions.RemoveDelay & Expectation.WorkOptions.UseTemporaryFilePath constructor( @@ -114,17 +119,7 @@ export class FileShareAccessorHandle extends GenericFileAccessorHandle get packageName(): string { return this.fullPath } - checkHandleRead(): AccessorHandlerCheckHandleReadResult { - const defaultResult = defaultCheckHandleRead(this.accessor) - if (defaultResult) return defaultResult - return this.checkAccessor() - } - checkHandleWrite(): AccessorHandlerCheckHandleWriteResult { - const defaultResult = defaultCheckHandleWrite(this.accessor) - if (defaultResult) return defaultResult - return this.checkAccessor() - } - private checkAccessor(): AccessorHandlerCheckHandleWriteResult { + checkHandleBasic(): AccessorHandlerCheckHandleBasicResult { if (this.accessor.type !== Accessor.AccessType.FILE_SHARE) { return { success: false, @@ -139,9 +134,41 @@ export class FileShareAccessorHandle extends GenericFileAccessorHandle if (!this.content.onlyContainerAccess) { if (!this.filePath) return { success: false, reason: { user: `File path not set`, tech: `File path not set` } } + + // Don't allow absolute file paths: + if (betterPathIsAbsolute(this.filePath)) + return { + success: false, + reason: { + user: `File path is an absolute path`, + tech: `File path "${this.filePath}" is an absolute path`, + }, + } + + // Ensure that the file path is not outside of the folder path: + const fullPath = betterPathResolve(this.fullPath) + const folderPath = betterPathResolve(this.folderPath) + if (!fullPath.startsWith(folderPath)) + return { + success: false, + reason: { + user: `File path is outside of folder path`, + tech: `Full path "${fullPath}" does not start with "${folderPath}"`, + }, + } } return { success: true } } + checkHandleRead(): AccessorHandlerCheckHandleReadResult { + const defaultResult = defaultCheckHandleRead(this.accessor) + if (defaultResult) return defaultResult + return { success: true } + } + checkHandleWrite(): AccessorHandlerCheckHandleWriteResult { + const defaultResult = defaultCheckHandleWrite(this.accessor) + if (defaultResult) return defaultResult + return { success: true } + } async checkPackageReadAccess(): Promise { const readIssue = await this._checkPackageReadAccess() if (!readIssue.success) { diff --git a/shared/packages/worker/src/worker/accessorHandlers/genericHandle.ts b/shared/packages/worker/src/worker/accessorHandlers/genericHandle.ts index fc3225cc..81ca9fbb 100644 --- a/shared/packages/worker/src/worker/accessorHandlers/genericHandle.ts +++ b/shared/packages/worker/src/worker/accessorHandlers/genericHandle.ts @@ -77,6 +77,12 @@ export abstract class GenericAccessorHandle { /** @returns true if the accessor is (statically) able to support access the PackageContainer */ // abstract static doYouSupportAccess(worker: GenericWorker, accessor: AccessorOnPackage.Any): boolean + /** + * Checks if there are any issues with the properties in the accessor or content for being able to be used at all. + * @returns undefined if all is OK / string with error message + */ + abstract checkHandleBasic(): AccessorHandlerCheckHandleBasicResult + /** * Checks if there are any issues with the properties in the accessor or content for being able to read * @returns undefined if all is OK / string with error message @@ -215,6 +221,7 @@ type AccessorHandlerResultBad = { /** Default result returned from most accessorHandler-methods */ export type AccessorHandlerResultGeneric = AccessorHandlerResultSuccess | AccessorHandlerResultBad +export type AccessorHandlerCheckHandleBasicResult = AccessorHandlerResultGeneric export type AccessorHandlerCheckHandleReadResult = AccessorHandlerResultGeneric export type AccessorHandlerCheckHandleWriteResult = AccessorHandlerCheckHandleReadResult export type AccessorHandlerCheckPackageReadAccessResult = AccessorHandlerResultGeneric diff --git a/shared/packages/worker/src/worker/accessorHandlers/http.ts b/shared/packages/worker/src/worker/accessorHandlers/http.ts index 212fadbc..8bda82a2 100644 --- a/shared/packages/worker/src/worker/accessorHandlers/http.ts +++ b/shared/packages/worker/src/worker/accessorHandlers/http.ts @@ -11,6 +11,7 @@ import { AccessorHandlerCheckPackageReadAccessResult, AccessorHandlerTryPackageReadResult, PackageOperation, + AccessorHandlerCheckHandleBasicResult, } from './genericHandle' import { Accessor, @@ -64,15 +65,41 @@ export class HTTPAccessorHandle extends GenericAccessorHandle { const header = await this.fetchHeader() @@ -205,31 +232,6 @@ export class HTTPAccessorHandle extends GenericAccessorHandle extends GenericAccessorHandle { const header = await this.fetchHeader() @@ -232,36 +263,6 @@ export class HTTPProxyAccessorHandle extends GenericAccessorHandle extends GenericAccesso getFullPath(filePath: string): string { filePath = removeBasePath(this.orgFolderPath, filePath) - return path.join(this.folderPath, filePath) + return betterPathJoin(this.folderPath, filePath) } + getMetadataPath(filePath: string): string { return this.getFullPath(filePath) + '_metadata.json' } diff --git a/shared/packages/worker/src/worker/accessorHandlers/localFolder.ts b/shared/packages/worker/src/worker/accessorHandlers/localFolder.ts index 16913b78..5a695007 100644 --- a/shared/packages/worker/src/worker/accessorHandlers/localFolder.ts +++ b/shared/packages/worker/src/worker/accessorHandlers/localFolder.ts @@ -13,6 +13,7 @@ import { AccessorHandlerTryPackageReadResult, GenericAccessorHandle, PackageOperation, + AccessorHandlerCheckHandleBasicResult, } from './genericHandle' import { Accessor, @@ -25,6 +26,8 @@ import { AccessorId, MonitorId, protectString, + betterPathResolve, + betterPathIsAbsolute, } from '@sofie-package-manager/api' import { BaseWorker } from '../worker' import { GenericFileAccessorHandle, LocalFolderAccessorHandleType } from './lib/FileHandler' @@ -41,16 +44,18 @@ const fsWriteFile = promisify(fs.writeFile) const fsRename = promisify(fs.rename) const fsMkDir = promisify(fs.mkdir) +export interface Content { + /** This is set when the class-instance is only going to be used for PackageContainer access.*/ + onlyContainerAccess?: boolean + filePath?: string + path?: string +} + /** Accessor handle for accessing files in a local folder */ export class LocalFolderAccessorHandle extends GenericFileAccessorHandle { static readonly type = LocalFolderAccessorHandleType - private content: { - /** This is set when the class-instance is only going to be used for PackageContainer access.*/ - onlyContainerAccess?: boolean - filePath?: string - path?: string - } + private content: Content private workOptions: Expectation.WorkOptions.RemoveDelay & Expectation.WorkOptions.UseTemporaryFilePath constructor( @@ -84,20 +89,9 @@ export class LocalFolderAccessorHandle extends GenericFileAccessorHand } /** Full path to the package */ get fullPath(): string { - return path.join(this.folderPath, this.filePath) - } - - checkHandleRead(): AccessorHandlerCheckHandleReadResult { - const defaultResult = defaultCheckHandleRead(this.accessor) - if (defaultResult) return defaultResult - return this.checkAccessor() + return this.getFullPath(this.filePath) } - checkHandleWrite(): AccessorHandlerCheckHandleWriteResult { - const defaultResult = defaultCheckHandleWrite(this.accessor) - if (defaultResult) return defaultResult - return this.checkAccessor() - } - private checkAccessor(): AccessorHandlerCheckHandleWriteResult { + checkHandleBasic(): AccessorHandlerCheckHandleBasicResult { if (this.accessor.type !== Accessor.AccessType.LOCAL_FOLDER) { return { success: false, @@ -112,7 +106,40 @@ export class LocalFolderAccessorHandle extends GenericFileAccessorHand if (!this.content.onlyContainerAccess) { if (!this.filePath) return { success: false, reason: { user: `File path not set`, tech: `File path not set` } } + + // Don't allow absolute file paths: + if (betterPathIsAbsolute(this.filePath)) + return { + success: false, + reason: { + user: `File path is an absolute path`, + tech: `File path "${this.filePath}" is an absolute path`, + }, + } + + // Ensure that the file path is not outside of the folder path: + const fullPath = betterPathResolve(this.fullPath) + const folderPath = betterPathResolve(this.folderPath) + if (!fullPath.startsWith(folderPath)) + return { + success: false, + reason: { + user: `File path is outside of folder path`, + tech: `Full path "${fullPath}" does not start with "${folderPath}"`, + }, + } } + + return { success: true } + } + checkHandleRead(): AccessorHandlerCheckHandleReadResult { + const defaultResult = defaultCheckHandleRead(this.accessor) + if (defaultResult) return defaultResult + return { success: true } + } + checkHandleWrite(): AccessorHandlerCheckHandleWriteResult { + const defaultResult = defaultCheckHandleWrite(this.accessor) + if (defaultResult) return defaultResult return { success: true } } async checkPackageReadAccess(): Promise { diff --git a/shared/packages/worker/src/worker/accessorHandlers/quantel.ts b/shared/packages/worker/src/worker/accessorHandlers/quantel.ts index 319a1dca..4e6cb664 100644 --- a/shared/packages/worker/src/worker/accessorHandlers/quantel.ts +++ b/shared/packages/worker/src/worker/accessorHandlers/quantel.ts @@ -14,6 +14,7 @@ import { AccessorHandlerCheckHandleWriteResult, AccessorHandlerRunCronJobResult, PackageOperation, + AccessorHandlerCheckHandleBasicResult, } from './genericHandle' import { Accessor, @@ -74,26 +75,7 @@ export class QuantelAccessorHandle extends GenericAccessorHandle extends GenericAccessorHandle { const quantel = await this.getQuantelGateway() diff --git a/shared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/lib.ts b/shared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/lib.ts index ffc3f29b..e8cbd36d 100644 --- a/shared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/lib.ts +++ b/shared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/lib.ts @@ -161,6 +161,18 @@ export async function lookupAccessorHandles( expectationWorkOptions ) + // Check that the accessor is valid at the most basic level: + const basicResult = handle.checkHandleBasic() + if (!basicResult.success) { + errorReason = { + user: `There is an issue with the Package: ${basicResult.reason.user})`, + tech: `${packageContainer.label}: Accessor "${accessor.label || accessorId}": ${ + basicResult.reason.tech + }`, + } + continue // Maybe next accessor works? + } + if (checks.read) { // Check that the accessor-handle supports reading: const readResult = handle.checkHandleRead() diff --git a/tests/internal-tests/src/__mocks__/child_process.ts b/tests/internal-tests/src/__mocks__/child_process.ts index 8ce2ba75..f49fe0db 100644 --- a/tests/internal-tests/src/__mocks__/child_process.ts +++ b/tests/internal-tests/src/__mocks__/child_process.ts @@ -187,7 +187,7 @@ async function robocopy(spawned: SpawnedProcess, args: string[]) { spawned.emit('exit', 1) // OK spawned.emit('close', 1) // OK } catch (err) { - // console.log(err) + console.error('ROBOCOPY MOCK ERROR: ' + err) spawned.emit('exit', 16) // Serious error. Robocopy did not copy any files. spawned.emit('close', 16) // Serious error. Robocopy did not copy any files. } From fb1e55d97a8be1bf7788cd7b366e8eae30952c14 Mon Sep 17 00:00:00 2001 From: Johan Nyman Date: Thu, 5 Sep 2024 10:56:31 +0200 Subject: [PATCH 2/2] chore: refactor: session property --- .../src/evaluationRunner/evaluateExpectationState.ts | 1 - .../evaluationRunner/evaluateExpectationStates/aborted.ts | 1 - .../evaluationRunner/evaluateExpectationStates/fulfilled.ts | 1 - .../src/evaluationRunner/evaluateExpectationStates/ready.ts | 1 - .../evaluationRunner/evaluateExpectationStates/removed.ts | 1 - .../evaluationRunner/evaluateExpectationStates/restarted.ts | 1 - .../evaluationRunner/evaluateExpectationStates/waiting.ts | 1 - .../src/evaluationRunner/evaluationRunner.ts | 4 ++-- .../src/expectationTracker/lib/trackedExpectationAPI.ts | 1 - .../expectationManager/src/lib/trackedExpectation.ts | 6 +++--- 10 files changed, 5 insertions(+), 13 deletions(-) diff --git a/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationState.ts b/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationState.ts index 417a9df2..b525c0df 100644 --- a/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationState.ts +++ b/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationState.ts @@ -24,7 +24,6 @@ export async function evaluateExpectationState( const tracker: ExpectationTracker = runner.tracker const timeSinceLastEvaluation = Date.now() - trackedExp.lastEvaluationTime - if (!trackedExp.session) trackedExp.session = {} if (trackedExp.session.hadError) return // There was an error during the session. if (trackedExp.session.expectationCanBeRemoved) return // The expectation has been removed diff --git a/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/aborted.ts b/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/aborted.ts index bc50c532..db383140 100644 --- a/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/aborted.ts +++ b/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/aborted.ts @@ -14,7 +14,6 @@ export async function evaluateExpectationStateAborted({ }: EvaluateContext): Promise { assertState(trackedExp, ExpectedPackageStatusAPI.WorkStatusState.ABORTED) - if (!trackedExp.session) trackedExp.session = {} await manager.workerAgents.assignWorkerToSession(trackedExp) if (trackedExp.session.assignedWorker) { // Start by removing the expectation diff --git a/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/fulfilled.ts b/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/fulfilled.ts index 2008f750..44ab3126 100644 --- a/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/fulfilled.ts +++ b/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/fulfilled.ts @@ -20,7 +20,6 @@ export async function evaluateExpectationStateFulfilled({ // TODO: Some monitor that is able to invalidate if it isn't fulfilled anymore? if (timeSinceLastEvaluation > tracker.getFulfilledWaitTime()) { - if (!trackedExp.session) trackedExp.session = {} await manager.workerAgents.assignWorkerToSession(trackedExp) if (trackedExp.session.assignedWorker) { try { diff --git a/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/ready.ts b/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/ready.ts index cd0487a4..8f84fe70 100644 --- a/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/ready.ts +++ b/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/ready.ts @@ -17,7 +17,6 @@ export async function evaluateExpectationStateReady({ }: EvaluateContext): Promise { assertState(trackedExp, ExpectedPackageStatusAPI.WorkStatusState.READY) // Start working on it: - if (!trackedExp.session) trackedExp.session = {} await manager.workerAgents.assignWorkerToSession(trackedExp) if ( diff --git a/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/removed.ts b/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/removed.ts index 843c611e..0b6d681e 100644 --- a/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/removed.ts +++ b/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/removed.ts @@ -17,7 +17,6 @@ export async function evaluateExpectationStateRemoved({ /** When true, the expectation can be removed */ let removeTheExpectation = false - if (!trackedExp.session) trackedExp.session = {} await manager.workerAgents.assignWorkerToSession(trackedExp) if (trackedExp.session.assignedWorker) { const removed = await trackedExp.session.assignedWorker.worker.removeExpectation(trackedExp.exp) diff --git a/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/restarted.ts b/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/restarted.ts index 12eee9e3..3366047a 100644 --- a/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/restarted.ts +++ b/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/restarted.ts @@ -13,7 +13,6 @@ export async function evaluateExpectationStateRestarted({ }: EvaluateContext): Promise { assertState(trackedExp, ExpectedPackageStatusAPI.WorkStatusState.RESTARTED) - if (!trackedExp.session) trackedExp.session = {} await manager.workerAgents.assignWorkerToSession(trackedExp) if (trackedExp.session.assignedWorker) { // Start by removing the expectation diff --git a/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/waiting.ts b/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/waiting.ts index ade07e31..20b0bda5 100644 --- a/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/waiting.ts +++ b/shared/packages/expectationManager/src/evaluationRunner/evaluateExpectationStates/waiting.ts @@ -19,7 +19,6 @@ export async function evaluateExpectationStateWaiting({ assertState(trackedExp, ExpectedPackageStatusAPI.WorkStatusState.WAITING) // Check if the expectation is ready to start: - if (!trackedExp.session) trackedExp.session = {} await manager.workerAgents.assignWorkerToSession(trackedExp) if (trackedExp.session.assignedWorker) { diff --git a/shared/packages/expectationManager/src/evaluationRunner/evaluationRunner.ts b/shared/packages/expectationManager/src/evaluationRunner/evaluationRunner.ts index 7d84d5d2..94cfec0c 100644 --- a/shared/packages/expectationManager/src/evaluationRunner/evaluationRunner.ts +++ b/shared/packages/expectationManager/src/evaluationRunner/evaluationRunner.ts @@ -278,7 +278,7 @@ export class EvaluationRunner { // Step 0: Reset the session: for (const trackedExp of tracked) { - trackedExp.session = null + trackedExp.session = {} } const postProcessSession = (trackedExp: TrackedExpectation) => { @@ -343,7 +343,7 @@ export class EvaluationRunner { // Step 1.5: Reset the session: // Because during the next iteration, the worker-assignment need to be done in series for (const trackedExp of tracked) { - trackedExp.session = null + trackedExp.session = {} } this.logger.debug(`Handle other states..`) diff --git a/shared/packages/expectationManager/src/expectationTracker/lib/trackedExpectationAPI.ts b/shared/packages/expectationManager/src/expectationTracker/lib/trackedExpectationAPI.ts index ce02c0eb..da0cb1a4 100644 --- a/shared/packages/expectationManager/src/expectationTracker/lib/trackedExpectationAPI.ts +++ b/shared/packages/expectationManager/src/expectationTracker/lib/trackedExpectationAPI.ts @@ -170,7 +170,6 @@ export class TrackedExpectationAPI { * Handle an Expectation that had no worker assigned */ public noWorkerAssigned(trackedExp: TrackedExpectation): void { - if (!trackedExp.session) throw new Error('Internal error: noWorkerAssigned: session not set') if (trackedExp.session.assignedWorker) throw new Error('Internal error: noWorkerAssigned can only be called when assignedWorker is falsy') diff --git a/shared/packages/expectationManager/src/lib/trackedExpectation.ts b/shared/packages/expectationManager/src/lib/trackedExpectation.ts index 8ee2ce44..92b77a0c 100644 --- a/shared/packages/expectationManager/src/lib/trackedExpectation.ts +++ b/shared/packages/expectationManager/src/lib/trackedExpectation.ts @@ -54,8 +54,8 @@ export interface TrackedExpectation { targetCanBeUsedWhileTransferring?: boolean sourceIsPlaceholder?: boolean } - /** A storage which is persistant only for a short while, during an evaluation of the Expectation. */ - session: ExpectationStateHandlerSession | null + /** A storage which is persistent only for a short while, during an evaluation of the Expectation. */ + session: ExpectationStateHandlerSession } export function expLabel(exp: TrackedExpectation): string { @@ -120,6 +120,6 @@ export function getDefaultTrackedExpectation( }, prevStatusReasons: existingtrackedExp?.prevStatusReasons || {}, status: {}, - session: null, + session: {}, } }