diff --git a/.changeset/friendly-seals-peel.md b/.changeset/friendly-seals-peel.md new file mode 100644 index 0000000000..a340d46f2b --- /dev/null +++ b/.changeset/friendly-seals-peel.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-common': patch +--- + +Ensure temporary directories are cleaned up if an error is thrown in the `filter` callback of the `UrlReader.readTree` options. diff --git a/packages/backend-common/src/reading/tree/TarArchiveResponse.test.ts b/packages/backend-common/src/reading/tree/TarArchiveResponse.test.ts index aa904c5522..f0cbc6a612 100644 --- a/packages/backend-common/src/reading/tree/TarArchiveResponse.test.ts +++ b/packages/backend-common/src/reading/tree/TarArchiveResponse.test.ts @@ -147,4 +147,40 @@ describe('TarArchiveResponse', () => { fs.pathExists(resolvePath(dir, 'docs/index.md')), ).resolves.toBe(false); }); + + it('should leave temporary directories in place in the case of an error', async () => { + const stream = fs.createReadStream('/test-archive.tar.gz'); + + const res = new TarArchiveResponse(stream, '', '/tmp', 'etag', () => { + throw new Error('NOPE'); + }); + + const tmpDir = await fs.mkdtemp('/tmp/test'); + // selects the wrong overload by default + const mkdtemp = jest.spyOn(fs, 'mkdtemp') as unknown as jest.SpyInstance< + Promise, + [] + >; + mkdtemp.mockResolvedValue(tmpDir); + + await expect(fs.pathExists(tmpDir)).resolves.toBe(true); + await expect(res.dir()).rejects.toThrow('NOPE'); + await expect(fs.pathExists(tmpDir)).resolves.toBe(false); + + mkdtemp.mockRestore(); + }); + + it('should leave directory in place if provided in the case of an error', async () => { + const stream = fs.createReadStream('/test-archive.tar.gz'); + + const res = new TarArchiveResponse(stream, '', '/tmp', 'etag', () => { + throw new Error('NOPE'); + }); + + const tmpDir = await fs.mkdtemp('/tmp/test'); + + await expect(fs.pathExists(tmpDir)).resolves.toBe(true); + await expect(res.dir({ targetDir: tmpDir })).rejects.toThrow('NOPE'); + await expect(fs.pathExists(tmpDir)).resolves.toBe(true); + }); }); diff --git a/packages/backend-common/src/reading/tree/TarArchiveResponse.ts b/packages/backend-common/src/reading/tree/TarArchiveResponse.ts index f7ab7f5349..493e283c5f 100644 --- a/packages/backend-common/src/reading/tree/TarArchiveResponse.ts +++ b/packages/backend-common/src/reading/tree/TarArchiveResponse.ts @@ -150,12 +150,19 @@ export class TarArchiveResponse implements ReadTreeResponse { // When no subPath is given, remove just 1 top level directory const strip = this.subPath ? this.subPath.split('/').length : 1; + let filterError: Error | undefined = undefined; + await pipeline( this.stream, tar.extract({ strip, cwd: dir, filter: (path, stat) => { + // Filter errors will short-circuit the rest of the filtering and then throw + if (filterError) { + 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 = stripFirstDirectoryFromPath(path); @@ -164,13 +171,27 @@ export class TarArchiveResponse implements ReadTreeResponse { } if (this.filter) { const innerPath = path.split('/').slice(strip).join('/'); - return this.filter(innerPath, { size: stat.size }); + try { + return this.filter(innerPath, { size: stat.size }); + } catch (error) { + filterError = error; + return false; + } } return true; }, }), ); + if (filterError) { + // If the dir was provided we don't want to remove it, but if it wasn't it means + // we created a temporary directory and we should remove it. + if (!options?.targetDir) { + await fs.remove(dir).catch(() => {}); + } + throw filterError; + } + return dir; } }