diff --git a/.changeset/fair-pens-wonder.md b/.changeset/fair-pens-wonder.md new file mode 100644 index 0000000000..0b395d9b9d --- /dev/null +++ b/.changeset/fair-pens-wonder.md @@ -0,0 +1,7 @@ +--- +'@backstage/plugin-scaffolder-backend': patch +'@backstage/backend-defaults': patch +'@backstage/plugin-scaffolder-node': patch +--- + +Wrap some of the action logic with `resolveSafeChildPath` and improve symlink handling when fetching remote and local files diff --git a/packages/backend-defaults/src/entrypoints/urlReader/lib/tree/ReadableArrayResponse.test.ts b/packages/backend-defaults/src/entrypoints/urlReader/lib/tree/ReadableArrayResponse.test.ts index 7420f14d70..8f0d260f6d 100644 --- a/packages/backend-defaults/src/entrypoints/urlReader/lib/tree/ReadableArrayResponse.test.ts +++ b/packages/backend-defaults/src/entrypoints/urlReader/lib/tree/ReadableArrayResponse.test.ts @@ -108,4 +108,16 @@ describe('ReadableArrayResponse', () => { }, }); }); + + it('should validate relative paths', async () => { + const arr: FromReadableArrayOptions = [ + { data: createReadStream(path1), path: '../other/file.yaml' }, + ]; + + const res = new ReadableArrayResponse(arr, targetDir.path, 'etag'); + + await expect(res.dir()).rejects.toThrow( + 'Relative path is not allowed to refer to a directory outside its parent', + ); + }); }); diff --git a/packages/backend-defaults/src/entrypoints/urlReader/lib/tree/ReadableArrayResponse.ts b/packages/backend-defaults/src/entrypoints/urlReader/lib/tree/ReadableArrayResponse.ts index 095b58edb1..6740771ad7 100644 --- a/packages/backend-defaults/src/entrypoints/urlReader/lib/tree/ReadableArrayResponse.ts +++ b/packages/backend-defaults/src/entrypoints/urlReader/lib/tree/ReadableArrayResponse.ts @@ -15,6 +15,7 @@ */ import { + resolveSafeChildPath, UrlReaderServiceReadTreeResponse, UrlReaderServiceReadTreeResponseDirOptions, UrlReaderServiceReadTreeResponseFile, @@ -98,7 +99,7 @@ export class ReadableArrayResponse implements UrlReaderServiceReadTreeResponse { for (let i = 0; i < this.stream.length; i++) { if (!this.stream[i].path.endsWith('/')) { - const filePath = platformPath.join(dir, this.stream[i].path); + const filePath = resolveSafeChildPath(dir, this.stream[i].path); await fs.mkdir(dirname(filePath), { recursive: true }); await pipeline(this.stream[i].data, fs.createWriteStream(filePath)); } diff --git a/packages/backend-defaults/src/entrypoints/urlReader/lib/tree/TarArchiveResponse.test.ts b/packages/backend-defaults/src/entrypoints/urlReader/lib/tree/TarArchiveResponse.test.ts index f69fb2e0e1..36b3d129e9 100644 --- a/packages/backend-defaults/src/entrypoints/urlReader/lib/tree/TarArchiveResponse.test.ts +++ b/packages/backend-defaults/src/entrypoints/urlReader/lib/tree/TarArchiveResponse.test.ts @@ -15,9 +15,10 @@ */ import fs from 'fs-extra'; -import { resolve as resolvePath } from 'path'; +import { resolve as resolvePath, join as joinPath } from 'path'; import { TarArchiveResponse } from './TarArchiveResponse'; import { createMockDirectory } from '@backstage/backend-test-utils'; +import tar from 'tar'; const archiveData = fs.readFileSync( resolvePath(__filename, '../../__fixtures__/mock-main.tar.gz'), @@ -223,4 +224,84 @@ describe('TarArchiveResponse', () => { await expect(res.dir({ targetDir: sub })).rejects.toThrow('NOPE'); await expect(fs.pathExists(sub)).resolves.toBe(true); }); + + describe('symlink handling', () => { + const tempDir = createMockDirectory(); + + it('should not extract symlinks with absolute targets', async () => { + const archiveSourceDir = tempDir.resolve('source'); + await fs.ensureDir(joinPath(archiveSourceDir, 'repo')); + await fs.writeFile( + joinPath(archiveSourceDir, 'repo', 'file.txt'), + 'file content', + ); + + // Create a symlink with an absolute path target + const absoluteTarget = tempDir.resolve('other.txt'); + await fs.writeFile(absoluteTarget, 'other content'); + await fs.symlink( + absoluteTarget, + joinPath(archiveSourceDir, 'repo', 'abs-link'), + ); + + const tarballPath = tempDir.resolve('archive.tar.gz'); + await tar.create( + { gzip: true, file: tarballPath, cwd: archiveSourceDir }, + ['repo'], + ); + + const stream = fs.createReadStream(tarballPath); + const res = new TarArchiveResponse(stream, '', targetDir.path, 'etag'); + + targetDir.addContent({ out: {} }); + const dir = await res.dir({ targetDir: targetDir.resolve('out') }); + + // Regular file should be extracted + await expect( + fs.readFile(joinPath(dir, 'file.txt'), 'utf8'), + ).resolves.toBe('file content'); + + // Symlink with absolute target should not be extracted + await expect(fs.lstat(joinPath(dir, 'abs-link'))).rejects.toThrow( + 'ENOENT', + ); + }); + + it('should extract symlinks with relative targets within archive', async () => { + const archiveSourceDir = tempDir.resolve('source2'); + await fs.ensureDir(joinPath(archiveSourceDir, 'repo', 'subdir')); + await fs.writeFile( + joinPath(archiveSourceDir, 'repo', 'target.txt'), + 'target content', + ); + + // Create a relative symlink pointing to a file within the archive + await fs.symlink( + '../target.txt', + joinPath(archiveSourceDir, 'repo', 'subdir', 'rel-link'), + ); + + const tarballPath = tempDir.resolve('archive2.tar.gz'); + await tar.create( + { gzip: true, file: tarballPath, cwd: archiveSourceDir }, + ['repo'], + ); + + const stream = fs.createReadStream(tarballPath); + const res = new TarArchiveResponse(stream, '', targetDir.path, 'etag'); + + targetDir.addContent({ out2: {} }); + const dir = await res.dir({ targetDir: targetDir.resolve('out2') }); + + // The symlink should be extracted and point to the correct file + const linkPath = joinPath(dir, 'subdir', 'rel-link'); + const stats = await fs.lstat(linkPath); + expect(stats.isSymbolicLink()).toBe(true); + + // Following the symlink should give us the target content + await expect(fs.readFile(linkPath, 'utf8')).resolves.toBe( + 'target content', + ); + }); + }); }); diff --git a/packages/backend-defaults/src/entrypoints/urlReader/lib/tree/TarArchiveResponse.ts b/packages/backend-defaults/src/entrypoints/urlReader/lib/tree/TarArchiveResponse.ts index 3102137f64..ab3a593c18 100644 --- a/packages/backend-defaults/src/entrypoints/urlReader/lib/tree/TarArchiveResponse.ts +++ b/packages/backend-defaults/src/entrypoints/urlReader/lib/tree/TarArchiveResponse.ts @@ -15,6 +15,7 @@ */ import { + isChildPath, UrlReaderServiceReadTreeResponse, UrlReaderServiceReadTreeResponseDirOptions, UrlReaderServiceReadTreeResponseFile, @@ -23,7 +24,7 @@ import concatStream from 'concat-stream'; import fs from 'fs-extra'; import platformPath from 'path'; import { pipeline as pipelineCb, Readable } from 'stream'; -import tar, { Parse, ParseStream, ReadEntry } from 'tar'; +import tar, { FileStat, Parse, ParseStream, ReadEntry } from 'tar'; import { promisify } from 'util'; import { stripFirstDirectoryFromPath } from './util'; @@ -182,6 +183,22 @@ export class TarArchiveResponse implements UrlReaderServiceReadTreeResponse { return false; } + // Block symlinks/hardlinks that escape the extraction directory + const entry = stat as FileStat & { type?: string; linkpath?: string }; + if ( + (entry.type === 'SymbolicLink' || entry.type === 'Link') && + entry.linkpath + ) { + const strippedPath = path.split('/').slice(strip).join('/'); + const linkDir = platformPath.dirname( + platformPath.join(dir, strippedPath), + ); + const targetPath = platformPath.resolve(linkDir, entry.linkpath); + if (!isChildPath(dir, targetPath)) { + return false; + } + } + // File path relative to the root extracted directory. Will remove the // top level dir name from the path since its name is hard to predetermine. const relativePath = this.stripFirstDirectory diff --git a/packages/backend-defaults/src/entrypoints/urlReader/lib/tree/ZipArchiveResponse.test.ts b/packages/backend-defaults/src/entrypoints/urlReader/lib/tree/ZipArchiveResponse.test.ts index 444cbf60b1..dcd9bcbd7e 100644 --- a/packages/backend-defaults/src/entrypoints/urlReader/lib/tree/ZipArchiveResponse.test.ts +++ b/packages/backend-defaults/src/entrypoints/urlReader/lib/tree/ZipArchiveResponse.test.ts @@ -271,4 +271,47 @@ describe('ZipArchiveResponse', () => { 'invalid relative path: ../side.txt', ); }); + + describe('symlink handling', () => { + it('should extract symlink entries as regular files', async () => { + const externalPath = targetDir.resolve('external.txt'); + await fs.writeFile(externalPath, 'external content'); + + const zipPath = targetDir.resolve('test.zip'); + await new Promise((resolve, reject) => { + const output = fs.createWriteStream(zipPath); + const archive = createArchive('zip'); + + output.on('close', () => resolve()); + archive.on('error', reject); + archive.pipe(output); + + archive.append('file content', { name: 'file.txt' }); + archive.symlink('link', externalPath); + + archive.finalize(); + }); + + const stream = createReadStream(zipPath); + const res = new ZipArchiveResponse(stream, '', targetDir.path, 'etag'); + + targetDir.addContent({ out: {} }); + const outDir = targetDir.resolve('out'); + const dir = await res.dir({ targetDir: outDir }); + + await expect( + fs.readFile(resolvePath(dir, 'file.txt'), 'utf8'), + ).resolves.toBe('file content'); + + const entryPath = resolvePath(dir, 'link'); + const stats = await fs.lstat(entryPath); + + // yauzl extracts symlink entries as regular files + expect(stats.isSymbolicLink()).toBe(false); + expect(stats.isFile()).toBe(true); + + const content = await fs.readFile(entryPath, 'utf8'); + expect(content).toBe(externalPath); + }); + }); }); diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/debug/log.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/debug/log.test.ts index 4606a01e0c..f9a45f0699 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/debug/log.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/debug/log.test.ts @@ -20,6 +20,7 @@ import { createDebugLogAction } from './log'; import { join } from 'path'; import yaml from 'yaml'; import { createMockDirectory } from '@backstage/backend-test-utils'; +import fs from 'fs-extra'; describe('debug:log', () => { const logger = { @@ -139,4 +140,30 @@ describe('debug:log', () => { expect.stringContaining('Hello Backstage!'), ); }); + + it('should handle symlinks to external paths', async () => { + const externalContent = 'external-file-content'; + const externalPath = mockDir.resolve('external.txt'); + await fs.writeFile(externalPath, externalContent); + + const linkPath = join(mockContext.workspacePath, 'link'); + await fs.symlink(externalPath, linkPath); + + const context = { + ...mockContext, + input: { + listWorkspace: 'with-contents' as const, + }, + }; + + await action.handler(context); + + expect(logger.info).toHaveBeenCalledWith(expect.stringContaining('link')); + expect(logger.info).not.toHaveBeenCalledWith( + expect.stringContaining(externalContent), + ); + expect(logger.info).toHaveBeenCalledWith( + expect.stringContaining('[skipped]'), + ); + }); }); diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/debug/log.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/debug/log.ts index 6eae1794f8..84e30bf543 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/debug/log.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/debug/log.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { resolveSafeChildPath } from '@backstage/backend-plugin-api'; import { readdir, stat } from 'fs-extra'; import { join, relative } from 'path'; import { createTemplateAction } from '@backstage/plugin-scaffolder-node'; @@ -66,8 +67,16 @@ export function createDebugLogAction() { .map(f => { const relativePath = relative(ctx.workspacePath, f); if (ctx.input?.listWorkspace === 'with-contents') { - const content = fs.readFileSync(f, 'utf-8'); - return ` - ${relativePath}:\n\n ${content}`; + try { + const safePath = resolveSafeChildPath( + ctx.workspacePath, + relativePath, + ); + const content = fs.readFileSync(safePath, 'utf-8'); + return ` - ${relativePath}:\n\n ${content}`; + } catch { + return ` - ${relativePath}: [skipped]`; + } } return ` - ${relativePath}`; }) diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/filesystem/delete.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/filesystem/delete.test.ts index efc2fe3143..83a85cf93b 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/filesystem/delete.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/filesystem/delete.test.ts @@ -229,4 +229,26 @@ describe('fs:delete', () => { expect(fileExists).toBe(false); }); }); + + it('should not delete files outside workspace via symlinks', async () => { + // Create an external file that should not be deleted + const externalDir = resolvePath(mockDir.path, 'external'); + const externalFile = resolvePath(externalDir, 'config.yaml'); + await fs.ensureDir(externalDir); + await fs.writeFile(externalFile, 'external content'); + + // Create a symlink inside workspace pointing to external directory + const linkPath = resolvePath(workspacePath, 'link'); + await fs.symlink(externalDir, linkPath); + + // Try to delete files through the symlink + await expect(() => + action.handler({ + ...mockContext, + input: { files: ['link/**'] }, + }), + ).rejects.toThrow( + /Relative path is not allowed to refer to a directory outside its parent/, + ); + }); }); diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/filesystem/delete.ts b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/filesystem/delete.ts index 0290414323..5ed5560023 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/builtin/filesystem/delete.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/builtin/filesystem/delete.ts @@ -16,7 +16,10 @@ import { createTemplateAction } from '@backstage/plugin-scaffolder-node'; import { InputError } from '@backstage/errors'; -import { resolveSafeChildPath } from '@backstage/backend-plugin-api'; +import { + isChildPath, + resolveSafeChildPath, +} from '@backstage/backend-plugin-api'; import fs from 'fs-extra'; import globby from 'globby'; import { examples } from './delete.examples'; @@ -58,10 +61,11 @@ export const createFilesystemDeleteAction = () => { for (const filepath of resolvedPaths) { try { - await fs.remove(filepath); - ctx.logger.info(`File ${filepath} deleted successfully`); + const safePath = resolveSafeChildPath(ctx.workspacePath, filepath); + await fs.remove(safePath); + ctx.logger.info(`File ${safePath} deleted successfully`); } catch (err) { - ctx.logger.error(`Failed to delete file ${filepath}:`, err); + ctx.logger.error(`Failed to delete file`, err); throw err; } } diff --git a/plugins/scaffolder-node/src/actions/fetch.test.ts b/plugins/scaffolder-node/src/actions/fetch.test.ts index f831d20f35..bbf0830b07 100644 --- a/plugins/scaffolder-node/src/actions/fetch.test.ts +++ b/plugins/scaffolder-node/src/actions/fetch.test.ts @@ -86,6 +86,7 @@ describe('fetchContents helper', () => { expect(fs.copy).toHaveBeenCalledWith( resolvePath('/some/foo'), 'somepath', + expect.objectContaining({ filter: expect.any(Function) }), ); }); diff --git a/plugins/scaffolder-node/src/actions/fetch.ts b/plugins/scaffolder-node/src/actions/fetch.ts index a0e8dd9f85..d9f872b3e3 100644 --- a/plugins/scaffolder-node/src/actions/fetch.ts +++ b/plugins/scaffolder-node/src/actions/fetch.ts @@ -15,7 +15,10 @@ */ import { UrlReaderService } from '@backstage/backend-plugin-api'; -import { resolveSafeChildPath } from '@backstage/backend-plugin-api'; +import { + isChildPath, + resolveSafeChildPath, +} from '@backstage/backend-plugin-api'; import { InputError } from '@backstage/errors'; import { ScmIntegrations } from '@backstage/integration'; import fs from 'fs-extra'; @@ -50,7 +53,7 @@ export async function fetchContents(options: { if (!fetchUrlIsAbsolute && baseUrl?.startsWith('file://')) { const basePath = baseUrl.slice('file://'.length); const srcDir = resolveSafeChildPath(path.dirname(basePath), fetchUrl); - await fs.copy(srcDir, outputPath); + await fs.copy(srcDir, outputPath, { filter: src => isChildPath(srcDir, src) }); } else { const readUrl = getReadUrl(fetchUrl, baseUrl, integrations);