diff --git a/config/custom-environment-variables.json b/config/custom-environment-variables.json index 1ba7871..67aa92f 100644 --- a/config/custom-environment-variables.json +++ b/config/custom-environment-variables.json @@ -114,8 +114,6 @@ }, "strategies": { "tilesDeletion": { - "s3Bucket": "TILES_DELETION_S3_BUCKET", - "fsSubPath": "TILES_DELETION_FS_SUB_PATH", "batchSize": { "__name": "TILES_DELETION_BATCH_SIZE", "__format": "number" diff --git a/config/default.json b/config/default.json index 4d97d6d..fe636a9 100644 --- a/config/default.json +++ b/config/default.json @@ -105,8 +105,6 @@ }, "strategies": { "tilesDeletion": { - "s3Bucket": "", - "fsSubPath": "tiles", "batchSize": 1000, "concurrency": 10 } diff --git a/helm/templates/configmap.yaml b/helm/templates/configmap.yaml index 27a5aff..95bc4c1 100644 --- a/helm/templates/configmap.yaml +++ b/helm/templates/configmap.yaml @@ -5,7 +5,6 @@ {{- $fs := ($storage.fs) | default dict -}} {{- $internalPvc := (($fs).internalPvc) | default dict -}} {{- $fsBasePath := clean (printf "/%s" $internalPvc.mountPath) -}} -{{- $fsTilesDeletionSubPath := clean (printf "%s" ($internalPvc.tilesSubPath | default "tiles")) -}} {{- if .Values.enabled -}} apiVersion: v1 kind: ConfigMap @@ -74,9 +73,7 @@ data: {{- end -}} {{- end }} FS_SUB_PATHS: {{ $subPaths | toJson | quote }} - TILES_DELETION_FS_SUB_PATH: {{ $fsTilesDeletionSubPath | quote }} {{- end }} - TILES_DELETION_S3_BUCKET: {{ $s3.tilesBucket | default "" | quote }} {{- with .Values.env.strategies.tilesDeletion }} TILES_DELETION_BATCH_SIZE: {{ .batchSize | default 1000 | quote }} TILES_DELETION_CONCURRENCY: {{ .concurrency | default 10 | quote }} diff --git a/helm/values.yaml b/helm/values.yaml index fe496fb..376c021 100644 --- a/helm/values.yaml +++ b/helm/values.yaml @@ -13,9 +13,7 @@ serviceUrls: jobTracker: "" storage: - cleanupStorageProviders: - - FS - - S3 + cleanupStorageProviders: {} s3: delete: batchSize: 1000 @@ -24,7 +22,6 @@ storage: secretName: "" sslEnabled: false region: "" - tilesBucket: "" fs: delete: batchSize: 1000 diff --git a/package-lock.json b/package-lock.json index 63640a8..d8ded2d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -16,7 +16,7 @@ "@map-colonies/js-logger": "^5.0.0", "@map-colonies/mc-priority-queue": "^9.1.0", "@map-colonies/mc-utils": "^5.1.0", - "@map-colonies/raster-shared": "^8.3.0", + "@map-colonies/raster-shared": "^9.0.0-alpha.0", "@map-colonies/read-pkg": "^1.0.0", "@map-colonies/schemas": "^1.20.0", "@map-colonies/telemetry": "^10.0.1", @@ -2543,9 +2543,9 @@ "license": "ISC" }, "node_modules/@map-colonies/raster-shared": { - "version": "8.3.0", - "resolved": "https://registry.npmjs.org/@map-colonies/raster-shared/-/raster-shared-8.3.0.tgz", - "integrity": "sha512-5faR0iBC9Dpxm7Z8fY+3d+5oolHqVk/urXeOq3nHuIEMOvwRT2TrTr9fmt6TK8DaCjyQXBeEU1nijvuPLHmrEw==", + "version": "9.0.0-alpha.0", + "resolved": "https://registry.npmjs.org/@map-colonies/raster-shared/-/raster-shared-9.0.0-alpha.0.tgz", + "integrity": "sha512-7NUVOMnlGaeSDYZu84wEXI25SyL/DatD/LHVBiGVP+CYWZ/8KjEe9WMH84EIPZS4VAlNpRbNZ770582AJNRjxQ==", "license": "ISC", "dependencies": { "@map-colonies/mc-priority-queue": "^9.1.0", diff --git a/package.json b/package.json index 187753d..ac6cbfd 100644 --- a/package.json +++ b/package.json @@ -37,7 +37,7 @@ "@map-colonies/js-logger": "^5.0.0", "@map-colonies/mc-priority-queue": "^9.1.0", "@map-colonies/mc-utils": "^5.1.0", - "@map-colonies/raster-shared": "^8.3.0", + "@map-colonies/raster-shared": "^9.0.0-alpha.0", "@map-colonies/read-pkg": "^1.0.0", "@map-colonies/schemas": "^1.20.0", "@map-colonies/telemetry": "^10.0.1", diff --git a/src/cleaner/storageProviders/fsStorageProvider.ts b/src/cleaner/storageProviders/fsStorageProvider.ts index 0a71bb4..89e3a57 100644 --- a/src/cleaner/storageProviders/fsStorageProvider.ts +++ b/src/cleaner/storageProviders/fsStorageProvider.ts @@ -4,7 +4,7 @@ import type { Logger } from '@map-colonies/js-logger'; import type { DeleteStoredResourcesParams } from '@map-colonies/raster-shared'; import { inject, injectable } from 'tsyringe'; import { mergeFailures, type DeleteFailure, type DeleteResult, type IStorageProvider, type StorageProvider } from '@src/cleaner/storageProviders'; -import { getChunk, normalizeFolderPath, resolveAbsolutePath } from '@src/cleaner/utils'; +import { getChunk, isPathWithinAllowedSubPaths, resolveAbsolutePath } from '@src/cleaner/utils'; import { SERVICES } from '@common/constants'; import { describeError, UnrecoverableError } from '../errors'; import type { FsStorageConfig } from './storageConfig'; @@ -20,10 +20,17 @@ export class FsStorageProvider implements IStorageProvider<'FS'> { this.logger.debug({ msg: 'Loaded FS storage provider', basePath: this.fsConfig.basePath }); } - public async targetExists(basePath: string, relativePath: string): Promise { - this.logger.debug({ msg: 'Checking if target resource exists', basePath, path: relativePath }); + /** + * @param subPath - Sub path of the configured base path, as supplied by the task + * @param relativePath - Path below `subPath` to check for + */ + public async targetExists(subPath: string, relativePath: string): Promise { + this.assertPathsValid([join(subPath, relativePath)]); + + const targetPath = join(this.fsConfig.basePath, subPath, relativePath); + this.logger.debug({ msg: 'Checking if target resource exists', subPath, path: relativePath, targetPath }); try { - await stat(join(basePath, relativePath)); + await stat(targetPath); return true; } catch (err) { if ((err as NodeJS.ErrnoException).code === 'ENOENT') return false; @@ -31,13 +38,18 @@ export class FsStorageProvider implements IStorageProvider<'FS'> { } } - public async delete(paths: string[], basePath: string): Promise { - this.logger.debug({ msg: 'Deleting files from filesystem', basePath, pathsCount: paths.length }); + /** + * @param subPath - Sub path of the configured base path, as supplied by the task + * @param paths - Paths relative to `subPath` + */ + public async delete(subPath: string, paths: string[]): Promise { + this.logger.debug({ msg: 'Deleting files from filesystem', subPath, pathsCount: paths.length }); + const targetPath = join(this.fsConfig.basePath, subPath); let failures: DeleteFailure = new Map(); const results = await Promise.allSettled( paths.map(async (relativePath) => { - await unlink(join(basePath, relativePath)); + await unlink(join(targetPath, relativePath)); }) ); @@ -47,14 +59,14 @@ export class FsStorageProvider implements IStorageProvider<'FS'> { const relativePath = paths[idx]!; const error: unknown = result.reason; const reason = describeError(error); - this.logger.debug({ msg: 'Failed to delete file', path: join(basePath, relativePath), reason, error }); + this.logger.debug({ msg: 'Failed to delete file', path: join(targetPath, relativePath), reason, error }); const chunkFailure = chunkFailures.get(reason); chunkFailures.set(reason, { count: (chunkFailure?.count ?? 0) + 1, sample: chunkFailure?.sample ?? relativePath }); } } failures = mergeFailures({ source: chunkFailures, target: failures }); - await this.cleanupEmptyDirs(paths, basePath); + await this.cleanupEmptyDirs(paths, targetPath); return { failures }; } @@ -68,11 +80,7 @@ export class FsStorageProvider implements IStorageProvider<'FS'> { totalFailedPathsCount = 0; const relativePaths = paths.map((path) => join(subPath, path)); - - if (!this.arePathsValid(relativePaths)) - throw new UnrecoverableError( - 'Cannot delete files/folders outside base path or subpath as well as base path or subpath itself. paths must also match a valid configured path.' - ); + this.assertPathsValid(relativePaths); let failures: DeleteFailure = new Map(); @@ -115,22 +123,26 @@ export class FsStorageProvider implements IStorageProvider<'FS'> { } /** - * Preforms several checks on input `paths`. - * Includes a check for path traversal (i.e. accessing folders above root folder) - * @param paths - * @returns boolean whether `paths` are valid and pass all checks + * Gate for every path this provider is asked to touch. + * @param relativePaths - Paths relative to the configured base path, sub path included + * @throws {UnrecoverableError} if any path fails the check; invalid paths are a producer + * bug and will not become valid on retry */ - private arePathsValid(paths: string[]): boolean { + private assertPathsValid(paths: string[]): void { this.logger.debug({ msg: 'Checking paths validity', paths }); - const badPaths = paths.filter((path) => { - const startsWithAllowedSubPath = this.fsConfig.subPaths.some((subPath) => path.startsWith(normalizeFolderPath(subPath))); - const absolutePath = resolveAbsolutePath(join(this.fsConfig.basePath, path)); - const startsWithBasePath = absolutePath.startsWith(normalizeFolderPath(this.fsConfig.basePath)); - return !(startsWithAllowedSubPath && startsWithBasePath); - }); - const areValid = badPaths.length === 0; - this.logger.debug({ msg: `Paths validity check ${areValid ? 'succeeded' : 'failed'}`, ...(!areValid && { badPaths }) }); - return areValid; + const badPaths = paths.filter( + (path) => !isPathWithinAllowedSubPaths({ relativePath: path, basePath: this.fsConfig.basePath, allowedSubPaths: this.fsConfig.subPaths }) + ); + + if (badPaths.length > 0) { + const { basePath, subPaths } = this.fsConfig; + this.logger.error({ msg: 'Paths validity check failed', badPaths, basePath, allowedSubPaths: subPaths }); + throw new UnrecoverableError( + `Cannot delete paths outside the configured sub paths (${subPaths.join(', ')}) of base path '${basePath}', or the sub paths themselves: ${badPaths.join(', ')}` + ); + } + + this.logger.debug({ msg: 'Paths validity check succeeded' }); } // Attempts to remove any directories that became empty after deletion. `relativePaths` diff --git a/src/cleaner/storageProviders/iStorageProvider.ts b/src/cleaner/storageProviders/iStorageProvider.ts index 481a529..2afd85c 100644 --- a/src/cleaner/storageProviders/iStorageProvider.ts +++ b/src/cleaner/storageProviders/iStorageProvider.ts @@ -15,12 +15,13 @@ export interface IStorageProvider { /** * Deletes a batch of relative file paths within the given storage target. * - S3: storageTarget = bucket name; paths are object keys - * - FS: storageTarget = base directory; full path = join(storageTarget, path) + * - FS: storageTarget = sub path of the configured base path; the provider joins its own + * base path and rejects anything falling outside the configured deletion sub paths * * Returns an object including delete failures aggregation with one entry per failed reason. * "Not found" is reported as a failure with additional metadata on failure - count and sample */ - delete: (paths: string[], storageTarget: string) => Promise; + delete: (storageTarget: string, paths: string[]) => Promise; /** * Deletes ALL objects/files under the given paths. @@ -32,7 +33,8 @@ export interface IStorageProvider { /** * Returns true if relativePath exists within storageTarget and contains data. * - S3: storageTarget = bucket, relativePath = key prefix — lists objects (KeyCount > 0) - * - FS: storageTarget = base directory, relativePath = subdirectory — checks fs.access + * - FS: storageTarget = sub path of the configured base path, relativePath = subdirectory + * below it — checks fs.stat, subject to the same sub path validation as `delete` */ targetExists: (storageTarget: string, relativePath: string) => Promise; } @@ -40,3 +42,6 @@ export interface IStorageProvider { export type StorageProviders = { [T in StorageProvider]?: IStorageProvider; }; + +/** A provider actually registered in the map. */ +export type ResolvedStorageProvider = NonNullable; diff --git a/src/cleaner/storageProviders/s3StorageProvider.ts b/src/cleaner/storageProviders/s3StorageProvider.ts index 57be934..b1aa1fe 100644 --- a/src/cleaner/storageProviders/s3StorageProvider.ts +++ b/src/cleaner/storageProviders/s3StorageProvider.ts @@ -45,7 +45,7 @@ export class S3StorageProvider implements IStorageProvider { + public async delete(bucket: string, paths: string[]): Promise { this.logger.debug({ msg: 'Deleting objects from S3', bucket, pathsCount: paths.length }); let failures: DeleteFailure = new Map(); diff --git a/src/cleaner/strategies/deleteStoredResourcesStrategy.ts b/src/cleaner/strategies/deleteStoredResourcesStrategy.ts index 157b371..7b9b934 100644 --- a/src/cleaner/strategies/deleteStoredResourcesStrategy.ts +++ b/src/cleaner/strategies/deleteStoredResourcesStrategy.ts @@ -1,9 +1,9 @@ import type { Logger } from '@map-colonies/js-logger'; -import { deleteStoredResourcesParamsSchema, type DeleteStoredResourcesParams } from '@map-colonies/raster-shared'; +import { deleteStoredResourcesParamsSchema, DeleteStoredResourcesParams, StorageProvider } from '@map-colonies/raster-shared'; import { inject, injectable } from 'tsyringe'; import type { ConfigType } from '@common/config'; import { SERVICES } from '@common/constants'; -import { summarizeDeleteFailures, type IStorageProvider, type StorageProvider, type StorageProviders } from '@src/cleaner/storageProviders'; +import { summarizeDeleteFailures, type IStorageProvider, type StorageProviders } from '@src/cleaner/storageProviders'; import { RecoverableError, UnrecoverableError } from '../errors'; import { validateSchema } from '../utils'; import type { ITaskStrategy } from './taskStrategy'; @@ -22,16 +22,17 @@ export class DeleteStoredResourcesStrategy implements ITaskStrategy { - const { paths } = params; - const provider = this.resolveStorageProvider(params); + const paths = params.storageProvider === StorageProvider.REDIS ? [] : params.paths; + const provider = this.resolveStorageProvider(params.storageProvider); this.logger.info({ msg: 'Starting deletion', count: paths.length, paths, provider: params.storageProvider, - ...(params.storageProvider === 'S3' && { bucket: params.bucket }), - ...(params.storageProvider === 'FS' && { subPath: params.subPath }), + ...(params.storageProvider === StorageProvider.S3 && { bucket: params.bucket }), + ...(params.storageProvider === StorageProvider.FS && { subPath: params.subPath }), + ...(params.storageProvider === StorageProvider.REDIS && { prefix: params.prefix }), }); const { failures } = await provider.deleteResources(params); @@ -56,14 +57,11 @@ export class DeleteStoredResourcesStrategy implements ITaskStrategy( - params: Extract - ): IStorageProvider { - if (!(params.storageProvider in this.storageProviders)) throw new UnrecoverableError(`Unsupported storage provider ${params.storageProvider}`); - // eslint-disable-next-line @typescript-eslint/naming-convention - const storageProvider = this.storageProviders[params.storageProvider]; - if (storageProvider === undefined) throw new UnrecoverableError(`Unsupported storage provider ${params.storageProvider}`); - this.logger.debug({ msg: `Using ${params.storageProvider} provider` }); - return storageProvider; + private resolveStorageProvider(storageProvider: K): IStorageProvider { + this.logger.debug({ msg: `Resolving storage provider`, provider: storageProvider, providers: Object.keys(this.storageProviders) }); + const provider = this.storageProviders[storageProvider]; + if (provider === undefined) throw new UnrecoverableError(`Unsupported storage provider ${storageProvider}`); + this.logger.debug({ msg: `Using ${storageProvider} provider` }); + return provider; } } diff --git a/src/cleaner/strategies/tilesDeletionStrategy.ts b/src/cleaner/strategies/tilesDeletionStrategy.ts index 792b884..5d839d8 100644 --- a/src/cleaner/strategies/tilesDeletionStrategy.ts +++ b/src/cleaner/strategies/tilesDeletionStrategy.ts @@ -1,34 +1,30 @@ -import { join } from 'node:path'; import { NoSuchKey } from '@aws-sdk/client-s3'; import type { Logger } from '@map-colonies/js-logger'; import type { TaskHandler as QueueClient } from '@map-colonies/mc-priority-queue'; -import { SourceType, TileRange, TilesDeletionParams, tilesDeletionParamsSchema } from '@map-colonies/raster-shared'; +import { StorageProvider, TileRange, TilesDeletionParams, tilesDeletionParamsSchema } from '@map-colonies/raster-shared'; import { inject, injectable } from 'tsyringe'; import type { ConfigType } from '@common/config'; import { PERCENTAGE_COMPLETE, SERVICES } from '@common/constants'; -import { - mergeFailures, - summarizeDeleteFailures, - type DeleteFailure, - type FsConfig, - type IStorageProvider, - type StorageProvider, - type StorageProviders, -} from '@src/cleaner/storageProviders'; +import { mergeFailures, summarizeDeleteFailures, type DeleteFailure, type StorageProviders } from '@src/cleaner/storageProviders'; import { RecoverableError, UnrecoverableError, describeError } from '../errors'; +import { ResolvedStorageProvider } from '../storageProviders/iStorageProvider'; import { validateSchema } from '../utils'; import type { TaskContext } from './strategyFactory'; import type { ITaskStrategy } from './taskStrategy'; const NOT_FOUND_REASONS = new Set([NoSuchKey.name, 'ENOENT']); +/** + * The params shapes this strategy can act on today. Redis tiles deletion is not implemented + * yet (MAPCO-11261): its params carry a key prefix instead of a tiles path, so there are no + * tile paths to generate. + */ +type SupportedTilesDeletionParams = Exclude; + @injectable() export class TilesDeletionStrategy implements ITaskStrategy { private readonly batchSize: number; private readonly concurrency: number; - private readonly s3Bucket: string; - private readonly fsBasePath: string; - private readonly fsTilesDeletionSubPath: string; public constructor( @inject(SERVICES.LOGGER) private readonly logger: Logger, @@ -39,9 +35,6 @@ export class TilesDeletionStrategy implements ITaskStrategy ) { this.batchSize = config.get('strategies.tilesDeletion.batchSize') as unknown as number; this.concurrency = config.get('strategies.tilesDeletion.concurrency') as unknown as number; - this.s3Bucket = config.get('strategies.tilesDeletion.s3Bucket') as unknown as string; - this.fsBasePath = config.get('storage.fs.basePath') as unknown as FsConfig['basePath']; - this.fsTilesDeletionSubPath = config.get('strategies.tilesDeletion.fsSubPath') as unknown as string; } public validate(params: unknown): TilesDeletionParams { @@ -50,17 +43,21 @@ export class TilesDeletionStrategy implements ITaskStrategy } public async execute(params: TilesDeletionParams): Promise { + if (params.storageProvider === StorageProvider.REDIS) { + throw new UnrecoverableError(`Tiles deletion is not implemented for ${StorageProvider.REDIS} storage`); + } + const { provider, storageTarget } = this.resolveStorageProvider(params); - if (!(await provider.targetExists(storageTarget, params.tilesPath))) { - throw new UnrecoverableError(`${params.sourceProvider} storage target does not exist: ${storageTarget}/${params.tilesPath}`); + if (!(await provider.targetExists(storageTarget, params.tilesRelativePath))) { + throw new UnrecoverableError(`${params.storageProvider} storage target does not exist: ${storageTarget}/${params.tilesRelativePath}`); } const totalTiles = this.countTiles(params); this.logger.info({ msg: 'Starting tiles deletion', - provider: params.sourceProvider, + provider: params.storageProvider, storageTarget, rangeCount: params.ranges.length, totalTiles, @@ -119,22 +116,19 @@ export class TilesDeletionStrategy implements ITaskStrategy this.logger.info({ msg: 'Tiles deletion completed successfully', deletedCount: totalTiles }); } - private resolveStorageProvider( - params: Extract - ): { provider: IStorageProvider; storageTarget: string } { - if (!(params.sourceProvider in this.storageProviders)) throw new UnrecoverableError(`Unsupported storage provider ${params.sourceProvider}`); + private resolveStorageProvider(params: SupportedTilesDeletionParams): { provider: ResolvedStorageProvider; storageTarget: string } { // eslint-disable-next-line @typescript-eslint/naming-convention - const storageProvider = this.storageProviders[params.sourceProvider]; - if (storageProvider === undefined) throw new UnrecoverableError(`Unsupported storage provider ${params.sourceProvider}`); - const storageTarget = params.sourceProvider === SourceType.S3 ? this.s3Bucket : join(this.fsBasePath, this.fsTilesDeletionSubPath); - this.logger.debug({ msg: `Using ${params.sourceProvider} provider` }); + const storageProvider = this.storageProviders[params.storageProvider]; + if (storageProvider === undefined) throw new UnrecoverableError(`Unsupported storage provider ${params.storageProvider}`); + const storageTarget = params.storageProvider === StorageProvider.S3 ? params.bucket : params.subPath; + this.logger.debug({ msg: `Using ${params.storageProvider} provider`, storageTarget }); return { provider: storageProvider, storageTarget }; } private async deleteAllTiles( - provider: IStorageProvider, + provider: ResolvedStorageProvider, storageTarget: string, - params: TilesDeletionParams, + params: SupportedTilesDeletionParams, totalTiles: number ): Promise { const { jobId, taskId } = this.taskContext; @@ -183,14 +177,14 @@ export class TilesDeletionStrategy implements ITaskStrategy * @returns Total tile paths attempted (not necessarily deleted). */ private async flushBatches( - provider: IStorageProvider, + provider: ResolvedStorageProvider, storageTarget: string, pendingBatches: string[][] ): Promise<{ batchFailures: DeleteFailure; processedTilesCount: number }> { let failures: DeleteFailure = new Map(); const processedTilesCount = pendingBatches.reduce((sum, b) => sum + b.length, 0); - const results = await Promise.allSettled(pendingBatches.map(async (batch) => provider.delete(batch, storageTarget))); + const results = await Promise.allSettled(pendingBatches.map(async (batch) => provider.delete(storageTarget, batch))); for (const [index, result] of results.entries()) { if (result.status === 'fulfilled') { failures = mergeFailures({ source: result.value.failures, target: failures }); @@ -212,20 +206,20 @@ export class TilesDeletionStrategy implements ITaskStrategy * For each range, the tile count is the product of the width (maxX - minX + 1) * and height (maxY - minY + 1) of the range grid. */ - private countTiles(params: TilesDeletionParams): number { + private countTiles(params: SupportedTilesDeletionParams): number { return params.ranges.reduce((sum, r) => sum + (r.maxX - r.minX + 1) * (r.maxY - r.minY + 1), 0); } - private *generateTilePaths(params: TilesDeletionParams): Generator { + private *generateTilePaths(params: SupportedTilesDeletionParams): Generator { for (const range of params.ranges) { - yield* this.generateRangePaths(range, params.tilesPath, params.fileExtension); + yield* this.generateRangePaths(range, params.tilesRelativePath, params.fileExtension); } } - private *generateRangePaths(range: TileRange, tilesPath: string, fileExtension: string): Generator { + private *generateRangePaths(range: TileRange, tilesRelativePath: string, fileExtension: string): Generator { for (let x = range.minX; x <= range.maxX; x++) { for (let y = range.minY; y <= range.maxY; y++) { - yield `${tilesPath}/${range.zoom}/${x}/${y}.${fileExtension}`; + yield `${tilesRelativePath}/${range.zoom}/${x}/${y}.${fileExtension}`; } } } diff --git a/src/cleaner/utils/index.ts b/src/cleaner/utils/index.ts index 49d0a4f..a7eead6 100644 --- a/src/cleaner/utils/index.ts +++ b/src/cleaner/utils/index.ts @@ -1,5 +1,5 @@ export { getChunk } from './chunk'; export { assertCanDeleteFromFolder } from './fs'; export { buildPollingPairs } from './pairBuilder'; -export { normalizeFolderPath, resolveAbsolutePath } from './path'; +export { isPathWithinAllowedSubPaths, normalizeFolderPath, resolveAbsolutePath } from './path'; export { validateSchema } from './validationHelper'; diff --git a/src/cleaner/utils/path.ts b/src/cleaner/utils/path.ts index c3049ee..b59df75 100644 --- a/src/cleaner/utils/path.ts +++ b/src/cleaner/utils/path.ts @@ -1,4 +1,4 @@ -import { resolve, sep } from 'node:path/posix'; +import { join, resolve, sep } from 'node:path/posix'; export const normalizeFolderPath = (path: string): string => { return path.endsWith(sep) ? path : `${path}${sep}`; @@ -14,3 +14,27 @@ export const normalizeFolderPath = (path: string): string => { export const resolveAbsolutePath = (path: string): string => { return resolve(`${path.startsWith(sep) ? '' : sep}${path}`); }; + +/** + * Guards a caller-supplied relative path before anything is deleted from the filesystem. + * A path is allowed only when it lives strictly *under* one of the configured sub paths. + * + * @param relativePath - Path relative to `basePath`, including the sub path segment + * @param basePath - Absolute mounted base directory + * @param allowedSubPaths - Sub paths deletion is permitted under, relative to `basePath` + * @returns boolean whether `relativePath` passes both checks + */ +export const isPathWithinAllowedSubPaths = ({ + relativePath, + basePath, + allowedSubPaths, +}: { + relativePath: string; + basePath: string; + allowedSubPaths: string[]; +}): boolean => { + const startsWithAllowedSubPath = allowedSubPaths.some((subPath) => relativePath.startsWith(normalizeFolderPath(subPath))); + const absolutePath = resolveAbsolutePath(join(basePath, relativePath)); + const startsWithBasePath = absolutePath.startsWith(normalizeFolderPath(basePath)); + return startsWithAllowedSubPath && startsWithBasePath; +}; diff --git a/tests/helpers/mocks.ts b/tests/helpers/mocks.ts index 949b565..9f14b46 100644 --- a/tests/helpers/mocks.ts +++ b/tests/helpers/mocks.ts @@ -80,18 +80,12 @@ export function createMockStorageProvider = {}): ConfigType { const values: Record = { 'strategies.tilesDeletion.batchSize': TILES_DELETION_CONFIG_DEFAULTS.batchSize, 'strategies.tilesDeletion.concurrency': TILES_DELETION_CONFIG_DEFAULTS.concurrency, - 'strategies.tilesDeletion.s3Bucket': TILES_DELETION_CONFIG_DEFAULTS.s3Bucket, - 'strategies.tilesDeletion.fsSubPath': TILES_DELETION_CONFIG_DEFAULTS.fsSubPath, - 'storage.fs.basePath': FS_STORAGE_CONFIG_DEFAULTS.basePath, ...overrides, }; return { get: vi.fn().mockImplementation((key: string) => values[key]) } as unknown as ConfigType; diff --git a/tests/storageProviders/fsStorageProvider.spec.ts b/tests/storageProviders/fsStorageProvider.spec.ts index 99d1f61..ab4db25 100644 --- a/tests/storageProviders/fsStorageProvider.spec.ts +++ b/tests/storageProviders/fsStorageProvider.spec.ts @@ -15,6 +15,7 @@ vi.mock('node:fs/promises', () => ({ })); const BASE_PATH = FS_VALIDATED_CONFIG_DEFAULTS.basePath; +const SUB_PATH = FS_VALIDATED_CONFIG_DEFAULTS.subPaths[0]!; describe('FsStorageProvider', () => { let provider: FsStorageProvider; @@ -41,18 +42,17 @@ describe('FsStorageProvider', () => { describe('#targetExists', () => { const RELATIVE_PATH = 'layer/v1'; - it('should call stat with full target path', async () => { - const targetPath = join(BASE_PATH, RELATIVE_PATH); - const result = await provider.targetExists(BASE_PATH, RELATIVE_PATH); + it('should call stat with the base path, sub path and relative path joined', async () => { + const result = await provider.targetExists(SUB_PATH, RELATIVE_PATH); expect(result).toBe(true); - expect(stat).toHaveBeenCalledWith(targetPath); + expect(stat).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, RELATIVE_PATH)); }); it('should return false when path does not exist (ENOENT)', async () => { vi.mocked(stat).mockRejectedValue(Object.assign(new Error('ENOENT'), { code: 'ENOENT' })); - const result = await provider.targetExists(BASE_PATH, RELATIVE_PATH); + const result = await provider.targetExists(SUB_PATH, RELATIVE_PATH); expect(result).toBe(false); }); @@ -60,36 +60,64 @@ describe('FsStorageProvider', () => { it('should throw an errors that are not ENOENT', async () => { vi.mocked(stat).mockRejectedValue(Object.assign(new Error('EACCES'), { code: 'EACCES' })); - await expect(provider.targetExists(BASE_PATH, RELATIVE_PATH)).rejects.toThrow('EACCES'); + await expect(provider.targetExists(SUB_PATH, RELATIVE_PATH)).rejects.toThrow('EACCES'); + }); + + it('should throw UnrecoverableError for a sub path outside the configured sub paths', async () => { + await expect(provider.targetExists('somewhere-else', RELATIVE_PATH)).rejects.toThrow(UnrecoverableError); + expect(stat).not.toHaveBeenCalled(); + }); + + it('should throw UnrecoverableError for a sub path that traverses out of the base path', async () => { + await expect(provider.targetExists(`${SUB_PATH}/../../..`, RELATIVE_PATH)).rejects.toThrow(UnrecoverableError); + expect(stat).not.toHaveBeenCalled(); + }); + + it('should throw UnrecoverableError when the relative path traverses out of the sub path', async () => { + await expect(provider.targetExists(SUB_PATH, '../../..')).rejects.toThrow(UnrecoverableError); + expect(stat).not.toHaveBeenCalled(); + }); + + it('should accept a sub path nested deeper inside a configured sub path', async () => { + const result = await provider.targetExists(`${SUB_PATH}/nested`, RELATIVE_PATH); + + expect(result).toBe(true); + expect(stat).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, 'nested', RELATIVE_PATH)); }); }); describe('#delete', () => { it('should return empty failures map for empty input', async () => { - const result = await provider.delete([], BASE_PATH); + const result = await provider.delete(SUB_PATH, []); expect(result).toEqual({ failures: new Map() }); expect(unlink).not.toHaveBeenCalled(); }); it('should call unlink with joined base path and relative path', async () => { - await provider.delete(['layer/v1/10/0/0.png'], BASE_PATH); + await provider.delete(SUB_PATH, ['layer/v1/10/0/0.png']); + + expect(unlink).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, 'layer/v1/10/0/0.png')); + }); + + it('should join a nested sub path onto the base path', async () => { + await provider.delete(`${SUB_PATH}/nested`, ['tile/10/0/0.png']); - expect(unlink).toHaveBeenCalledWith(join(BASE_PATH, 'layer/v1/10/0/0.png')); + expect(unlink).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, 'nested', 'tile/10/0/0.png')); }); it('should call unlink for every path', async () => { const paths = ['tile/10/0/0.png', 'tile/10/0/1.png', 'tile/10/1/0.png']; - await provider.delete(paths, BASE_PATH); + await provider.delete(SUB_PATH, paths); expect(unlink).toHaveBeenCalledTimes(3); for (const p of paths) { - expect(unlink).toHaveBeenCalledWith(join(BASE_PATH, p)); + expect(unlink).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, p)); } }); it('should return empty failures map when all unlinks succeed', async () => { - const result = await provider.delete(['tile/10/0/0.png', 'tile/10/0/1.png'], BASE_PATH); + const result = await provider.delete(SUB_PATH, ['tile/10/0/0.png', 'tile/10/0/1.png']); expect(result).toEqual({ failures: new Map() }); }); @@ -97,7 +125,7 @@ describe('FsStorageProvider', () => { const enoent = Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); vi.mocked(unlink).mockRejectedValue(enoent); - const result = await provider.delete(['tile/10/0/0.png'], BASE_PATH); + const result = await provider.delete(SUB_PATH, ['tile/10/0/0.png']); expect(result).toEqual({ failures: new Map([['ENOENT', { count: 1, sample: 'tile/10/0/0.png' }]]) }); }); @@ -106,7 +134,7 @@ describe('FsStorageProvider', () => { const permError = Object.assign(new Error('EACCES'), { code: 'EACCES' }); vi.mocked(unlink).mockRejectedValue(permError); - const result = await provider.delete(['tile/10/0/0.png'], BASE_PATH); + const result = await provider.delete(SUB_PATH, ['tile/10/0/0.png']); expect(result).toEqual({ failures: new Map([['EACCES', { count: 1, sample: 'tile/10/0/0.png' }]]) }); }); @@ -114,7 +142,7 @@ describe('FsStorageProvider', () => { it('should fall back to error message when error has no errno code', async () => { vi.mocked(unlink).mockRejectedValue(new Error('disk on fire')); - const result = await provider.delete(['tile/10/0/0.png'], BASE_PATH); + const result = await provider.delete(SUB_PATH, ['tile/10/0/0.png']); expect(result).toEqual({ failures: new Map([['disk on fire', { count: 1, sample: 'tile/10/0/0.png' }]]) }); }); @@ -122,7 +150,7 @@ describe('FsStorageProvider', () => { it('should fall back to "Unknown" when error has neither errno code nor message', async () => { vi.mocked(unlink).mockRejectedValue(new Error('')); - const result = await provider.delete(['tile/10/0/0.png'], BASE_PATH); + const result = await provider.delete(SUB_PATH, ['tile/10/0/0.png']); expect(result).toEqual({ failures: new Map([['Unknown', { count: 1, sample: 'tile/10/0/0.png' }]]) }); }); @@ -130,7 +158,7 @@ describe('FsStorageProvider', () => { it('should tag failures with the stringified value when a non-Error is thrown', async () => { vi.mocked(unlink).mockRejectedValue('raw string failure'); - const result = await provider.delete(['tile/10/0/0.png'], BASE_PATH); + const result = await provider.delete(SUB_PATH, ['tile/10/0/0.png']); expect(result).toEqual({ failures: new Map([['raw string failure', { count: 1, sample: 'tile/10/0/0.png' }]]) }); }); @@ -138,7 +166,7 @@ describe('FsStorageProvider', () => { it('should fall back to "Unknown" when a non-Error empty value is thrown', async () => { vi.mocked(unlink).mockRejectedValue(''); - const result = await provider.delete(['tile/10/0/0.png'], BASE_PATH); + const result = await provider.delete(SUB_PATH, ['tile/10/0/0.png']); expect(result).toEqual({ failures: new Map([['Unknown', { count: 1, sample: 'tile/10/0/0.png' }]]) }); }); @@ -150,7 +178,7 @@ describe('FsStorageProvider', () => { }, }); - const result = await provider.delete(['tile/10/0/0.png'], BASE_PATH); + const result = await provider.delete(SUB_PATH, ['tile/10/0/0.png']); expect(result).toEqual({ failures: new Map([['non-serializable thrown value', { count: 1, sample: 'tile/10/0/0.png' }]]) }); }); @@ -158,11 +186,11 @@ describe('FsStorageProvider', () => { it('should unlink every path of a large input', async () => { const paths = Array.from({ length: 7 }, (_, i) => `tile/10/0/${i}.png`); - await provider.delete(paths, BASE_PATH); + await provider.delete(SUB_PATH, paths); expect(unlink).toHaveBeenCalledTimes(7); for (const path of paths) { - expect(unlink).toHaveBeenCalledWith(join(BASE_PATH, path)); + expect(unlink).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, path)); } }); @@ -171,7 +199,7 @@ describe('FsStorageProvider', () => { vi.mocked(unlink).mockRejectedValue(permError); const paths = Array.from({ length: 7 }, (_, i) => `tile/10/0/${i}.png`); - const result = await provider.delete(paths, BASE_PATH); + const result = await provider.delete(SUB_PATH, paths); expect(result).toEqual({ failures: new Map([['EACCES', { count: 7, sample: 'tile/10/0/0.png' }]]) }); }); @@ -186,7 +214,7 @@ describe('FsStorageProvider', () => { .mockRejectedValueOnce(permError); // real error → failure const paths = ['tile/10/0/0.png', 'tile/10/0/1.png', 'tile/10/0/2.png']; - const result = await provider.delete(paths, BASE_PATH); + const result = await provider.delete(SUB_PATH, paths); expect(result).toEqual({ failures: new Map([ @@ -201,7 +229,7 @@ describe('FsStorageProvider', () => { vi.mocked(unlink).mockRejectedValue(permError); const relativePath = 'layer/v1/10/5/3.png'; - const result = await provider.delete([relativePath], BASE_PATH); + const result = await provider.delete(SUB_PATH, [relativePath]); expect(result).toEqual({ failures: new Map([['EACCES', { count: 1, sample: relativePath }]]) }); expect(Array.from(result.failures.values())[0]?.sample).not.toMatch(`^${BASE_PATH}*`); @@ -209,27 +237,27 @@ describe('FsStorageProvider', () => { describe('cleanupEmptyDirs', () => { it('should attempt to rmdir the parent directory after deletion', async () => { - await provider.delete(['layer/v1/10/0/0.png'], BASE_PATH); + await provider.delete(SUB_PATH, ['layer/v1/10/0/0.png']); - expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, 'layer/v1/10/0')); + expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, 'layer/v1/10/0')); }); it('should attempt to rmdir all ancestor directories bottom-up', async () => { - await provider.delete(['layer/v1/10/0/0.png'], BASE_PATH); + await provider.delete(SUB_PATH, ['layer/v1/10/0/0.png']); // x dir → zoom dir → version dir → layer dir (deepest first) - expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, 'layer/v1/10/0')); - expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, 'layer/v1/10')); - expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, 'layer/v1')); - expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, 'layer')); + expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, 'layer/v1/10/0')); + expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, 'layer/v1/10')); + expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, 'layer/v1')); + expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, 'layer')); }); it('should deduplicate rmdir calls for shared parent directories', async () => { // Both tiles share the same x-dir and zoom dir - await provider.delete(['tile/10/0/0.png', 'tile/10/0/1.png'], BASE_PATH); + await provider.delete(SUB_PATH, ['tile/10/0/0.png', 'tile/10/0/1.png']); const rmdirCalls = vi.mocked(rmdir).mock.calls.map(([p]) => p); - const xDirCalls = rmdirCalls.filter((p) => p === join(BASE_PATH, 'tile/10/0')); + const xDirCalls = rmdirCalls.filter((p) => p === join(BASE_PATH, SUB_PATH, 'tile/10/0')); expect(xDirCalls).toHaveLength(1); }); @@ -237,21 +265,21 @@ describe('FsStorageProvider', () => { const enotempty = Object.assign(new Error('ENOTEMPTY'), { code: 'ENOTEMPTY' }); vi.mocked(rmdir).mockRejectedValue(enotempty); - const result = await provider.delete(['tile/10/0/0.png'], BASE_PATH); + const result = await provider.delete(SUB_PATH, ['tile/10/0/0.png']); // Should not throw and should return correct failed paths expect(result).toEqual({ failures: new Map() }); }); it('should not call rmdir when input is empty', async () => { - await provider.delete([], BASE_PATH); + await provider.delete(SUB_PATH, []); expect(rmdir).not.toHaveBeenCalled(); }); it('should not call rmdir for a path that has no directory segments', async () => { - await provider.delete(['0.png'], BASE_PATH); + await provider.delete(SUB_PATH, ['0.png']); - expect(unlink).toHaveBeenCalledWith(join(BASE_PATH, '0.png')); + expect(unlink).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, '0.png')); expect(rmdir).not.toHaveBeenCalled(); }); @@ -259,24 +287,24 @@ describe('FsStorageProvider', () => { // batchSize is 3 → cleanup runs once for all paths, after the last batch const paths = Array.from({ length: 4 }, (_, i) => `tile/10/${i}/0.png`); - await provider.delete(paths, BASE_PATH); + await provider.delete(SUB_PATH, paths); for (let i = 0; i < 4; i++) { - expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, `tile/10/${i}`)); + expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, `tile/10/${i}`)); } - expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, 'tile/10')); - expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, 'tile')); + expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, 'tile/10')); + expect(rmdir).toHaveBeenCalledWith(join(BASE_PATH, SUB_PATH, 'tile')); }); it('should attempt to rmdir deeper directories before their ancestors', async () => { - await provider.delete(['layer/v1/10/0/0.png'], BASE_PATH); + await provider.delete(SUB_PATH, ['layer/v1/10/0/0.png']); const order = vi.mocked(rmdir).mock.calls.map(([path]) => path); expect(order).toEqual([ - join(BASE_PATH, 'layer/v1/10/0'), - join(BASE_PATH, 'layer/v1/10'), - join(BASE_PATH, 'layer/v1'), - join(BASE_PATH, 'layer'), + join(BASE_PATH, SUB_PATH, 'layer/v1/10/0'), + join(BASE_PATH, SUB_PATH, 'layer/v1/10'), + join(BASE_PATH, SUB_PATH, 'layer/v1'), + join(BASE_PATH, SUB_PATH, 'layer'), ]); }); }); diff --git a/tests/storageProviders/s3StorageProvider.spec.ts b/tests/storageProviders/s3StorageProvider.spec.ts index 50c9cd7..f0a168d 100644 --- a/tests/storageProviders/s3StorageProvider.spec.ts +++ b/tests/storageProviders/s3StorageProvider.spec.ts @@ -77,7 +77,7 @@ describe('S3StorageProvider', () => { }); it('should return empty failures map for empty input', async () => { - const result = await provider.delete([], BUCKET); + const result = await provider.delete(BUCKET, []); expect(result).toEqual({ failures: new Map() }); expect(mockSend).not.toHaveBeenCalled(); }); @@ -85,7 +85,7 @@ describe('S3StorageProvider', () => { it('should send DeleteObjectsCommand with correct keys', async () => { const paths = ['folder/a.txt', 'folder/b.txt']; - await provider.delete(paths, BUCKET); + await provider.delete(BUCKET, paths); expect(DeleteObjectsCommand).toHaveBeenCalledWith({ Bucket: BUCKET, @@ -97,7 +97,7 @@ describe('S3StorageProvider', () => { it('should return empty failures map when all deletes succeed', async () => { const paths = ['a.txt', 'b.txt']; - const result = await provider.delete(paths, BUCKET); + const result = await provider.delete(BUCKET, paths); expect(result).toEqual({ failures: new Map() }); }); @@ -108,7 +108,7 @@ describe('S3StorageProvider', () => { }); const paths = ['a.txt', 'b.txt']; - const result = await provider.delete(paths, BUCKET); + const result = await provider.delete(BUCKET, paths); expect(result).toEqual({ failures: new Map([['AccessDenied', { count: 1, sample: 'a.txt' }]]) }); }); @@ -119,7 +119,7 @@ describe('S3StorageProvider', () => { }); const paths = ['missing.txt']; - const result = await provider.delete(paths, BUCKET); + const result = await provider.delete(BUCKET, paths); expect(result).toEqual({ failures: new Map([['NoSuchKey', { count: 1, sample: 'missing.txt' }]]) }); }); @@ -129,7 +129,7 @@ describe('S3StorageProvider', () => { Errors: [{ Key: 'a.txt', Message: 'Something bad' }], }); - const result = await provider.delete(['a.txt'], BUCKET); + const result = await provider.delete(BUCKET, ['a.txt']); expect(result).toEqual({ failures: new Map([['Something bad', { count: 1, sample: 'a.txt' }]]) }); }); @@ -139,7 +139,7 @@ describe('S3StorageProvider', () => { Errors: [{ Key: 'a.txt' }], }); - const result = await provider.delete(['a.txt'], BUCKET); + const result = await provider.delete(BUCKET, ['a.txt']); expect(result).toEqual({ failures: new Map([['Unknown', { count: 1, sample: 'a.txt' }]]) }); }); @@ -154,7 +154,7 @@ describe('S3StorageProvider', () => { ], }); - const result = await provider.delete(['a.txt', 'b.txt', 'c.txt', 'd.txt'], BUCKET); + const result = await provider.delete(BUCKET, ['a.txt', 'b.txt', 'c.txt', 'd.txt']); expect(result).toEqual({ failures: new Map([ @@ -169,7 +169,7 @@ describe('S3StorageProvider', () => { const paths = Array.from({ length: 1500 }, (_, i) => `object-${i}.txt`); provider = new S3StorageProvider(createS3StorageConfig({ batchSize: 1000 }), mockLogger); - await provider.delete(paths, BUCKET); + await provider.delete(BUCKET, paths); expect(mockSend).toHaveBeenCalledTimes(2); const firstCallInput = vi.mocked(DeleteObjectsCommand).mock.calls[0]![0] as { @@ -186,7 +186,7 @@ describe('S3StorageProvider', () => { const paths = Array.from({ length: 2500 }, (_, i) => `object-${i}.txt`); provider = new S3StorageProvider(createS3StorageConfig({ batchSize: 1000 }), mockLogger); - await provider.delete(paths, BUCKET); + await provider.delete(BUCKET, paths); expect(mockSend).toHaveBeenCalledTimes(3); const callInputs = vi.mocked(DeleteObjectsCommand).mock.calls.map((call) => call[0] as { Delete: { Objects: { Key: string }[] } }); @@ -200,7 +200,7 @@ describe('S3StorageProvider', () => { .mockResolvedValueOnce({ Errors: [{ Key: 'object-0.txt', Code: 'AccessDenied' }] }) .mockResolvedValueOnce({ Errors: [{ Key: 'object-1000.txt', Code: 'AccessDenied' }] }); - const result = await provider.delete(paths, BUCKET); + const result = await provider.delete(BUCKET, paths); expect(result).toEqual({ failures: new Map([['AccessDenied', { count: 2, sample: 'object-0.txt' }]]) }); }); @@ -210,7 +210,7 @@ describe('S3StorageProvider', () => { Errors: [{ Code: 'InternalError' }, { Key: 'b.txt', Code: 'AccessDenied' }], }); - const result = await provider.delete(['a.txt', 'b.txt'], BUCKET); + const result = await provider.delete(BUCKET, ['a.txt', 'b.txt']); expect(result).toEqual({ failures: new Map([['AccessDenied', { count: 1, sample: 'b.txt' }]]) }); }); @@ -218,7 +218,7 @@ describe('S3StorageProvider', () => { it('should return an empty failures map when every returned error carries no Key', async () => { mockSend.mockResolvedValue({ Errors: [{ Code: 'InternalError' }] }); - const result = await provider.delete(['a.txt'], BUCKET); + const result = await provider.delete(BUCKET, ['a.txt']); expect(result).toEqual({ failures: new Map() }); }); @@ -226,7 +226,7 @@ describe('S3StorageProvider', () => { it('should tag the chunk with the stringified value when send rejects with a non-Error', async () => { mockSend.mockRejectedValue('connection reset'); - const result = await provider.delete(['a.txt', 'b.txt'], BUCKET); + const result = await provider.delete(BUCKET, ['a.txt', 'b.txt']); expect(result).toEqual({ failures: new Map([['connection reset', { count: 2, sample: 'a.txt' }]]) }); }); @@ -235,7 +235,7 @@ describe('S3StorageProvider', () => { mockSend.mockRejectedValue(new Error('Network error')); const paths = ['a.txt', 'b.txt']; - const result = await provider.delete(paths, BUCKET); + const result = await provider.delete(BUCKET, paths); expect(result).toEqual({ failures: new Map([['Network error', { count: 2, sample: 'a.txt' }]]) }); }); diff --git a/tests/strategies/deleteStoredResourcesStrategy.spec.ts b/tests/strategies/deleteStoredResourcesStrategy.spec.ts index 1ff0a63..b9841aa 100644 --- a/tests/strategies/deleteStoredResourcesStrategy.spec.ts +++ b/tests/strategies/deleteStoredResourcesStrategy.spec.ts @@ -1,5 +1,5 @@ import type { Logger } from '@map-colonies/js-logger'; -import { SourceType, type DeleteStoredResourcesParams } from '@map-colonies/raster-shared'; +import { SourceType, type FsDeleteStoredResourcesParams, type S3DeleteStoredResourcesParams } from '@map-colonies/raster-shared'; import { beforeEach, describe, expect, it, type vi } from 'vitest'; import { RecoverableError, UnrecoverableError, ValidationError } from '@src/cleaner/errors'; import type { IStorageProvider, StorageProviders } from '@src/cleaner/storageProviders'; @@ -10,8 +10,8 @@ import { createMockStoredResourcesDeletionStrategyConfig, createMockLogger, crea const S3_BUCKET = 'test-bucket'; const FS_SUB_PATH = 'test/artifacts/tiles'; -const s3Params: DeleteStoredResourcesParams = { storageProvider: SourceType.S3, paths: ['layer1'], bucket: S3_BUCKET }; -const fsParams: DeleteStoredResourcesParams = { storageProvider: SourceType.FS, paths: ['layer2'], subPath: FS_SUB_PATH }; +const s3Params: S3DeleteStoredResourcesParams = { storageProvider: SourceType.S3, paths: ['layer1'], bucket: S3_BUCKET }; +const fsParams: FsDeleteStoredResourcesParams = { storageProvider: SourceType.FS, paths: ['layer2'], subPath: FS_SUB_PATH }; describe('DeleteStoredResourcesStrategy', () => { let strategy: DeleteStoredResourcesStrategy; @@ -57,12 +57,26 @@ describe('DeleteStoredResourcesStrategy', () => { expect(() => strategy.validate({ storageProvider: 'GCS', catalogId: 'layer1' })).toThrow(ValidationError); }); - it('should throw ValidationError when tilesPath is empty string', () => { - expect(() => strategy.validate({ storageProvider: SourceType.S3, catalogId: '' })).toThrow(ValidationError); + it('should validate and return REDIS params, whose prefix is the locator', () => { + const redisParams = { storageProvider: 'REDIS', prefix: 'layer-redis_WorldCRS84' }; + + expect(strategy.validate(redisParams)).toEqual(redisParams); + }); + + it('should throw ValidationError when paths is an empty array', () => { + expect(() => strategy.validate({ storageProvider: SourceType.S3, bucket: S3_BUCKET, paths: [] })).toThrow(ValidationError); + }); + + it('should throw ValidationError when paths is missing', () => { + expect(() => strategy.validate({ storageProvider: SourceType.S3, bucket: S3_BUCKET })).toThrow(ValidationError); + }); + + it('should throw ValidationError when the S3 bucket is missing', () => { + expect(() => strategy.validate({ storageProvider: SourceType.S3, paths: ['layer1'] })).toThrow(ValidationError); }); - it('should throw ValidationError when tilesPath is missing', () => { - expect(() => strategy.validate({ storageProvider: SourceType.S3 })).toThrow(ValidationError); + it('should throw ValidationError when the FS subPath is missing', () => { + expect(() => strategy.validate({ storageProvider: SourceType.FS, paths: ['layer1'] })).toThrow(ValidationError); }); it('should throw ValidationError for null params', () => { @@ -94,7 +108,7 @@ describe('DeleteStoredResourcesStrategy', () => { }); it('should throw UnrecoverableError for unknown provider', async () => { - const unknownParams = { ...s3Params, storageProvider: 'UNKNOWN' } as unknown as DeleteStoredResourcesParams; + const unknownParams = { ...s3Params, storageProvider: 'UNKNOWN' } as unknown as S3DeleteStoredResourcesParams; const result = strategy.execute(unknownParams); @@ -140,7 +154,7 @@ describe('DeleteStoredResourcesStrategy', () => { }); it('should pass every path through to the provider', async () => { - const params: DeleteStoredResourcesParams = { ...s3Params, paths: ['layer1', 'layer2', 'layer3'] }; + const params: S3DeleteStoredResourcesParams = { ...s3Params, paths: ['layer1', 'layer2', 'layer3'] }; await strategy.execute(params); diff --git a/tests/strategies/tilesDeletionStrategy.spec.ts b/tests/strategies/tilesDeletionStrategy.spec.ts index d6dfc10..3121265 100644 --- a/tests/strategies/tilesDeletionStrategy.spec.ts +++ b/tests/strategies/tilesDeletionStrategy.spec.ts @@ -1,31 +1,38 @@ -import { join } from 'node:path'; import { faker } from '@faker-js/faker'; import type { TaskHandler as QueueClient } from '@map-colonies/mc-priority-queue'; -import { type TilesDeletionParams, SourceType } from '@map-colonies/raster-shared'; +import { type FsTilesDeletionParams, type S3TilesDeletionParams, SourceType } from '@map-colonies/raster-shared'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { RecoverableError, UnrecoverableError, ValidationError } from '@src/cleaner/errors'; import type { IStorageProvider, StorageProviders } from '@src/cleaner/storageProviders'; import type { TaskContext } from '@src/cleaner/strategies/strategyFactory'; import { TilesDeletionStrategy } from '@src/cleaner/strategies/tilesDeletionStrategy'; -import { createMockLogger, createMockStorageProvider, createMockStrategyConfig, TILES_DELETION_CONFIG_DEFAULTS } from '../helpers/mocks'; +import { createMockLogger, createMockStorageProvider, createMockStrategyConfig } from '../helpers/mocks'; -const { s3Bucket: S3_BUCKET, fsBasePath: FS_BASE_PATH, fsSubPath: FS_SUB_PATH } = TILES_DELETION_CONFIG_DEFAULTS; +const S3_BUCKET = 'test-bucket'; +const FS_SUB_PATH = 'artifacts/tiles'; const JOB_ID = faker.string.uuid(); const TASK_ID = faker.string.uuid(); const TASK_CONTEXT: TaskContext = { jobId: JOB_ID, taskId: TASK_ID, jobType: 'Ingestion_Update', taskType: 'tiles-deletion' }; -const s3Params: TilesDeletionParams = { - sourceProvider: 'S3', - tilesPath: 'layer/v1', +const s3Params: S3TilesDeletionParams = { + storageProvider: 'S3', + bucket: S3_BUCKET, + tilesRelativePath: 'layer/v1', fileExtension: 'png', ranges: [{ zoom: 10, minX: 0, maxX: 1, minY: 0, maxY: 1 }], }; -const fsParams: TilesDeletionParams = { ...s3Params, sourceProvider: 'FS' }; +const fsParams: FsTilesDeletionParams = { + storageProvider: 'FS', + subPath: FS_SUB_PATH, + tilesRelativePath: s3Params.tilesRelativePath, + fileExtension: s3Params.fileExtension, + ranges: s3Params.ranges, +}; -// Builds an expected tile path under the standard s3Params tilesPath/fileExtension. -const tilePath = (z: number, x: number, y: number): string => `${s3Params.tilesPath}/${z}/${x}/${y}.${s3Params.fileExtension}`; +// Builds an expected tile path under the standard tilesRelativePath/fileExtension. +const tilePath = (z: number, x: number, y: number): string => `${s3Params.tilesRelativePath}/${z}/${x}/${y}.${s3Params.fileExtension}`; describe('TilesDeletionStrategy', () => { let strategy: TilesDeletionStrategy; @@ -69,20 +76,28 @@ describe('TilesDeletionStrategy', () => { expect(strategy.validate({ ...s3Params, ranges })).toEqual({ ...s3Params, ranges }); }); - it('should throw ValidationError when sourceProvider is missing', () => { - expect(() => strategy.validate({ ...s3Params, sourceProvider: undefined })).toThrow(ValidationError); + it('should throw ValidationError when storageProvider is missing', () => { + expect(() => strategy.validate({ ...s3Params, storageProvider: undefined })).toThrow(ValidationError); }); - it('should throw ValidationError for unsupported sourceProvider value', () => { - expect(() => strategy.validate({ ...s3Params, sourceProvider: 'GCS' })).toThrow(ValidationError); + it('should throw ValidationError for unsupported storageProvider value', () => { + expect(() => strategy.validate({ ...s3Params, storageProvider: 'GCS' })).toThrow(ValidationError); }); it('should throw ValidationError for empty ranges array', () => { expect(() => strategy.validate({ ...s3Params, ranges: [] })).toThrow(ValidationError); }); - it('should throw ValidationError for empty tilesPath', () => { - expect(() => strategy.validate({ ...s3Params, tilesPath: '' })).toThrow(ValidationError); + it('should throw ValidationError for empty tilesRelativePath', () => { + expect(() => strategy.validate({ ...s3Params, tilesRelativePath: '' })).toThrow(ValidationError); + }); + + it('should throw ValidationError when the S3 bucket is missing', () => { + expect(() => strategy.validate({ ...s3Params, bucket: undefined })).toThrow(ValidationError); + }); + + it('should throw ValidationError when the FS subPath is missing', () => { + expect(() => strategy.validate({ ...fsParams, subPath: undefined })).toThrow(ValidationError); }); it('should throw ValidationError when params is null', () => { @@ -106,16 +121,16 @@ describe('TilesDeletionStrategy', () => { expect(MockFsProvider.delete).not.toHaveBeenCalled(); }); - it('should check targetExists with S3 bucket and tilesPath as relativePath', async () => { + it('should check targetExists with S3 bucket and tilesRelativePath as relativePath', async () => { await strategy.execute(s3Params); - expect(MockS3Provider.targetExists).toHaveBeenCalledWith(S3_BUCKET, s3Params.tilesPath); + expect(MockS3Provider.targetExists).toHaveBeenCalledWith(S3_BUCKET, s3Params.tilesRelativePath); }); - it('should check targetExists with FS base path and tilesPath as relativePath', async () => { + it("should check targetExists with the task's own subPath and tilesRelativePath", async () => { await strategy.execute(fsParams); - expect(MockFsProvider.targetExists).toHaveBeenCalledWith(join(FS_BASE_PATH, FS_SUB_PATH), fsParams.tilesPath); + expect(MockFsProvider.targetExists).toHaveBeenCalledWith(FS_SUB_PATH, fsParams.tilesRelativePath); }); it('should propagate an error thrown by the target existence check', async () => { @@ -128,31 +143,49 @@ describe('TilesDeletionStrategy', () => { }); describe('provider routing', () => { - it('should call S3 provider with s3Bucket as storage target', async () => { - await strategy.execute(s3Params); + it("should call S3 provider with the task's own bucket as storage target", async () => { + const params: S3TilesDeletionParams = { ...s3Params, bucket: 'per-task-bucket' }; + + await strategy.execute(params); - expect(MockS3Provider.delete).toHaveBeenCalledWith(expect.any(Array), S3_BUCKET); + expect(MockS3Provider.delete).toHaveBeenCalledWith('per-task-bucket', expect.any(Array)); expect(MockFsProvider.delete).not.toHaveBeenCalled(); }); - it('should call FS provider with fsBasePath as storage target', async () => { + it("should call FS provider with the task's own subPath as storage target", async () => { await strategy.execute(fsParams); - expect(MockFsProvider.delete).toHaveBeenCalledWith(expect.any(Array), join(FS_BASE_PATH, FS_SUB_PATH)); + expect(MockFsProvider.delete).toHaveBeenCalledWith(FS_SUB_PATH, expect.any(Array)); expect(MockS3Provider.delete).not.toHaveBeenCalled(); }); + it('should pass the subPath through untouched rather than resolving it', async () => { + const subPath = 'some/other/mount/point'; + + await strategy.execute({ ...fsParams, subPath }); + + expect(MockFsProvider.delete).toHaveBeenCalledWith(subPath, expect.any(Array)); + }); + + it('should throw UnrecoverableError for REDIS params, whose tiles are not path addressed', async () => { + const redisParams = { storageProvider: 'REDIS', prefix: 'layer-redis_WorldCRS84', ranges: s3Params.ranges }; + + await expect(strategy.execute(strategy.validate(redisParams))).rejects.toThrow(UnrecoverableError); + expect(MockS3Provider.delete).not.toHaveBeenCalled(); + expect(MockFsProvider.delete).not.toHaveBeenCalled(); + }); + it('should throw UnrecoverableError for unknown provider', async () => { - const unknownParams = { ...s3Params, sourceProvider: 'UNKNOWN' } as unknown as TilesDeletionParams; + const unknownParams = { ...s3Params, storageProvider: 'UNKNOWN' } as unknown as S3TilesDeletionParams; await expect(strategy.execute(unknownParams)).rejects.toThrow(UnrecoverableError); }); it('should throw UnrecoverableError when the provider is registered but resolves to undefined', async () => { - const storageProviders = { + const storageProviders: StorageProviders = { [SourceType.FS]: MockFsProvider, [SourceType.S3]: undefined, - } satisfies StorageProviders; + }; strategy = new TilesDeletionStrategy( createMockLogger(), createMockStrategyConfig(), @@ -172,23 +205,25 @@ describe('TilesDeletionStrategy', () => { await strategy.execute(s3Params); // range: minX=0,maxX=1 minY=0,maxY=1 → 4 tiles, x iterates outer - expect(MockS3Provider.delete).toHaveBeenCalledWith( - [tilePath(10, 0, 0), tilePath(10, 0, 1), tilePath(10, 1, 0), tilePath(10, 1, 1)], - S3_BUCKET - ); + expect(MockS3Provider.delete).toHaveBeenCalledWith(S3_BUCKET, [ + tilePath(10, 0, 0), + tilePath(10, 0, 1), + tilePath(10, 1, 0), + tilePath(10, 1, 1), + ]); }); it('should use the specified file extension', async () => { - const params: TilesDeletionParams = { ...s3Params, fileExtension: 'jpeg' }; + const params: S3TilesDeletionParams = { ...s3Params, fileExtension: 'jpeg' }; await strategy.execute(params); - const [paths] = vi.mocked(MockS3Provider.delete).mock.calls[0]!; + const [, paths] = vi.mocked(MockS3Provider.delete).mock.calls[0]!; expect(paths.every((p) => p.endsWith('.jpeg'))).toBe(true); }); it('should concatenate tiles from multiple ranges', async () => { - const params: TilesDeletionParams = { + const params: S3TilesDeletionParams = { ...s3Params, ranges: [ { zoom: 5, minX: 0, maxX: 0, minY: 0, maxY: 0 }, @@ -198,18 +233,18 @@ describe('TilesDeletionStrategy', () => { await strategy.execute(params); - expect(MockS3Provider.delete).toHaveBeenCalledWith([tilePath(5, 0, 0), tilePath(6, 0, 0)], S3_BUCKET); + expect(MockS3Provider.delete).toHaveBeenCalledWith(S3_BUCKET, [tilePath(5, 0, 0), tilePath(6, 0, 0)]); }); it('should offset x/y correctly when range does not start at 0', async () => { - const params: TilesDeletionParams = { + const params: S3TilesDeletionParams = { ...s3Params, ranges: [{ zoom: 7, minX: 3, maxX: 4, minY: 8, maxY: 9 }], }; await strategy.execute(params); - expect(MockS3Provider.delete).toHaveBeenCalledWith([tilePath(7, 3, 8), tilePath(7, 3, 9), tilePath(7, 4, 8), tilePath(7, 4, 9)], S3_BUCKET); + expect(MockS3Provider.delete).toHaveBeenCalledWith(S3_BUCKET, [tilePath(7, 3, 8), tilePath(7, 3, 9), tilePath(7, 4, 8), tilePath(7, 4, 9)]); }); }); @@ -217,7 +252,7 @@ describe('TilesDeletionStrategy', () => { it('should call updateProgress mid-stream for large tile sets without ever setting 100', async () => { // batchSize=100, concurrency=2 → flush after 200 tiles, then final flush for remainder // 14 * 15 = 210 tiles - const params: TilesDeletionParams = { + const params: S3TilesDeletionParams = { ...s3Params, ranges: [{ zoom: 5, minX: 0, maxX: 13, minY: 0, maxY: 14 }], }; @@ -230,7 +265,7 @@ describe('TilesDeletionStrategy', () => { it('should report the percentage of tiles processed so far', async () => { // batchSize=100, concurrency=2 → flush + progress report after the first 200 of 210 tiles - const params: TilesDeletionParams = { + const params: S3TilesDeletionParams = { ...s3Params, ranges: [{ zoom: 5, minX: 0, maxX: 13, minY: 0, maxY: 14 }], }; @@ -242,7 +277,7 @@ describe('TilesDeletionStrategy', () => { it('should report progress once per completed concurrency window', async () => { // 420 tiles → two full windows of 200, then a trailing batch of 20 - const params: TilesDeletionParams = { + const params: S3TilesDeletionParams = { ...s3Params, ranges: [{ zoom: 5, minX: 0, maxX: 20, minY: 0, maxY: 19 }], }; @@ -262,7 +297,7 @@ describe('TilesDeletionStrategy', () => { it('should not flush an empty trailing batch when the tile count divides evenly', async () => { // 200 tiles = exactly batchSize (100) × concurrency (2) → one window, no remainder - const params: TilesDeletionParams = { + const params: S3TilesDeletionParams = { ...s3Params, ranges: [{ zoom: 5, minX: 0, maxX: 9, minY: 0, maxY: 19 }], }; @@ -275,7 +310,7 @@ describe('TilesDeletionStrategy', () => { }); it('should pass the correct jobId and taskId on mid-stream updates', async () => { - const params: TilesDeletionParams = { + const params: S3TilesDeletionParams = { ...s3Params, ranges: [{ zoom: 5, minX: 0, maxX: 13, minY: 0, maxY: 14 }], }; @@ -383,7 +418,7 @@ describe('TilesDeletionStrategy', () => { it('should aggregate hard failures of the same reason across concurrent batches', async () => { // 200 tiles → two batches of 100, both rejecting with the same error - const params: TilesDeletionParams = { + const params: S3TilesDeletionParams = { ...s3Params, ranges: [{ zoom: 5, minX: 0, maxX: 9, minY: 0, maxY: 19 }], }; @@ -396,7 +431,7 @@ describe('TilesDeletionStrategy', () => { }); it('should aggregate hard failures of different reasons across concurrent batches', async () => { - const params: TilesDeletionParams = { + const params: S3TilesDeletionParams = { ...s3Params, ranges: [{ zoom: 5, minX: 0, maxX: 9, minY: 0, maxY: 19 }], }; @@ -406,7 +441,7 @@ describe('TilesDeletionStrategy', () => { }); it('should surface both soft failures and hard rejections from the same flush', async () => { - const params: TilesDeletionParams = { + const params: S3TilesDeletionParams = { ...s3Params, ranges: [{ zoom: 5, minX: 0, maxX: 9, minY: 0, maxY: 19 }], }; diff --git a/tests/strategyFactory.spec.ts b/tests/strategyFactory.spec.ts index 027a1f1..16c0b3b 100644 --- a/tests/strategyFactory.spec.ts +++ b/tests/strategyFactory.spec.ts @@ -7,7 +7,7 @@ import type { IStorageProvider, StorageProviders } from '@src/cleaner/storagePro import { StrategyNotFoundError } from '../src/cleaner/errors'; import { StrategyFactory, TilesDeletionStrategy, type ITaskStrategy, type TaskContext } from '../src/cleaner/strategies'; import { SERVICES } from '../src/common/constants'; -import { createMockConfig, createMockLogger, createMockQueueClient, createMockStorageProvider } from './helpers/mocks'; +import { createMockLogger, createMockQueueClient, createMockStorageProvider, createMockStrategyConfig } from './helpers/mocks'; class MockStrategy implements ITaskStrategy { public validate(params: unknown): Record { @@ -39,7 +39,7 @@ describe('StrategyFactory', () => { }; container.register(SERVICES.LOGGER, { useValue: mockLogger }); - container.register(SERVICES.CONFIG, { useValue: createMockConfig() }); + container.register(SERVICES.CONFIG, { useValue: createMockStrategyConfig() }); container.register(SERVICES.STORAGE_PROVIDERS, { useValue: storageProviders }); container.register(SERVICES.QUEUE_CLIENT, { useValue: createMockQueueClient() }); diff --git a/tests/utils/path.spec.ts b/tests/utils/path.spec.ts new file mode 100644 index 0000000..d745914 --- /dev/null +++ b/tests/utils/path.spec.ts @@ -0,0 +1,68 @@ +import { describe, expect, it } from 'vitest'; +import { isPathWithinAllowedSubPaths, normalizeFolderPath, resolveAbsolutePath } from '@src/cleaner/utils/path'; + +const BASE_PATH = '/data'; +const ALLOWED_SUB_PATHS = ['artifacts/tiles', 'artifacts/gpkgs']; + +const isAllowed = (relativePath: string): boolean => + isPathWithinAllowedSubPaths({ relativePath, basePath: BASE_PATH, allowedSubPaths: ALLOWED_SUB_PATHS }); + +describe('path', () => { + describe('#normalizeFolderPath', () => { + it('should append a trailing separator when missing', () => { + expect(normalizeFolderPath('a/b')).toBe('a/b/'); + }); + + it('should leave an already terminated path unchanged', () => { + expect(normalizeFolderPath('a/b/')).toBe('a/b/'); + }); + }); + + describe('#resolveAbsolutePath', () => { + it('should prefix a relative path with the root separator', () => { + expect(resolveAbsolutePath('data/tiles')).toBe('/data/tiles'); + }); + + it('should collapse traversal segments', () => { + expect(resolveAbsolutePath('/data/tiles/../gpkgs')).toBe('/data/gpkgs'); + }); + }); + + describe('#isPathWithinAllowedSubPaths', () => { + it('should allow a path nested under an allowed sub path', () => { + expect(isAllowed('artifacts/tiles/layer/v1')).toBe(true); + }); + + it('should allow a path under any of the allowed sub paths', () => { + expect(isAllowed('artifacts/gpkgs/layer.gpkg')).toBe(true); + }); + + it('should reject an allowed sub path root itself', () => { + expect(isAllowed('artifacts/tiles')).toBe(false); + }); + + it('should reject a path under no allowed sub path', () => { + expect(isAllowed('somewhere-else/layer')).toBe(false); + }); + + it('should reject a sibling that merely shares the sub path prefix', () => { + expect(isAllowed('artifacts/tiles-old/layer')).toBe(false); + }); + + it('should reject traversal that escapes the base path', () => { + expect(isAllowed('artifacts/tiles/../../../etc/passwd')).toBe(false); + }); + + it('should reject traversal that lands back on the base path', () => { + expect(isAllowed('artifacts/tiles/../..')).toBe(false); + }); + + it('should allow traversal that stays under an allowed sub path', () => { + expect(isAllowed('artifacts/tiles/layer/../v2')).toBe(true); + }); + + it('should reject every path when no sub paths are allowed', () => { + expect(isPathWithinAllowedSubPaths({ relativePath: 'artifacts/tiles/layer', basePath: BASE_PATH, allowedSubPaths: [] })).toBe(false); + }); + }); +});