Merge branch 'backstage:master' into master

packages/backend-common/src/logging/index.ts

@@ -15,5 +15,10 @@
  */
 
 export * from './formats';
-export { createRootLogger, getRootLogger, setRootLogger } from './rootLogger';
+export {
+  createRootLogger,
+  getRootLogger,
+  setRootLogger,
+  redactWinstonLogLine,
+} from './rootLogger';
 export * from './voidLogger';

packages/backend-common/src/logging/rootLogger.ts

@@ -69,8 +69,10 @@ export function setRootLoggerRedactionList(redactionList: string[]) {
 /**
  * A winston formatting function that finds occurrences of filteredKeys
  * and replaces them with the corresponding identifier.
+ *
+ * @public
  */
-function redactLogLine(info: winston.Logform.TransformableInfo) {
+export function redactWinstonLogLine(info: winston.Logform.TransformableInfo) {
   // TODO(hhogg): The logger is created before the config is loaded, because the
   // logger is needed in the config loader. There is a risk of a secret being
   // logged out during the config loading stage.
@@ -104,7 +106,7 @@ export function createRootLogger(
       {
         level: env.LOG_LEVEL || 'info',
         format: winston.format.combine(
-          winston.format(redactLogLine)(),
+          winston.format(redactWinstonLogLine)(),
           env.NODE_ENV === 'production' ? winston.format.json() : coloredFormat,
         ),
         defaultMeta: {

packages/backend-common/src/reading/GitlabUrlReader.test.ts

@@ -106,7 +106,7 @@ describe('GitlabUrlReader', () => {
       );
 
     it.each([
-      // Project URLs
+      // Scoped routes
       {
         url: 'https://gitlab.com/groupA/teams/teamA/subgroupA/repoA/-/blob/branch/my/path/to/file.yaml',
         config: createConfig(),
@@ -135,12 +135,12 @@ describe('GitlabUrlReader', () => {
         }),
       },
 
-      // Raw URLs
+      // Unscoped route
       {
         url: 'https://gitlab.example.com/a/b/blob/master/c.yaml',
         config: createConfig(),
         response: expect.objectContaining({
-          url: 'https://gitlab.example.com/a/b/raw/master/c.yaml',
+          url: 'https://gitlab.example.com/api/v4/projects/12345/repository/files/c.yaml/raw?ref=master',
         }),
       },
     ])('should handle happy path %#', async ({ url, config, response }) => {

packages/backend-common/src/reading/tree/ZipArchiveResponse.test.ts

@@ -28,6 +28,9 @@ const archiveDataCorrupted = fs.readFileSync(
 const archiveDataWithExtraDir = fs.readFileSync(
   resolvePath(__filename, '../../__fixtures__/mock-with-extra-root-dir.zip'),
 );
+const archiveWithMaliciousEntry = fs.readFileSync(
+  resolvePath(__filename, '../../__fixtures__/mallory.zip'),
+);
 
 describe('ZipArchiveResponse', () => {
   beforeEach(() => {
@@ -35,6 +38,7 @@ describe('ZipArchiveResponse', () => {
       '/test-archive.zip': archiveData,
       '/test-archive-with-extra-root-dir.zip': archiveDataWithExtraDir,
       '/test-archive-corrupted.zip': archiveDataCorrupted,
+      '/test-archive-malicious.zip': archiveWithMaliciousEntry,
       '/tmp': mockFs.directory(),
     });
   });
@@ -167,4 +171,22 @@ describe('ZipArchiveResponse', () => {
       'invalid comment length. expected: 55. found: 0',
     );
   });
+
+  it('should throw on entries with a path outside the destination dir', async () => {
+    const stream = fs.createReadStream('/test-archive-malicious.zip');
+
+    const res = new ZipArchiveResponse(stream, '', '/tmp', 'etag');
+    await expect(res.files()).rejects.toThrow(
+      'invalid relative path: ../side.txt',
+    );
+  });
+
+  it('should throw on entries that attempt to write outside destination dir', async () => {
+    const stream = fs.createReadStream('/test-archive-malicious.zip');
+
+    const res = new ZipArchiveResponse(stream, '', '/tmp', 'etag');
+    await expect(res.dir()).rejects.toThrow(
+      'invalid relative path: ../side.txt',
+    );
+  });
 });

packages/backend-common/src/reading/tree/ZipArchiveResponse.ts

@@ -25,6 +25,7 @@ import {
   ReadTreeResponseFile,
 } from '../types';
 import { streamToBuffer } from './util';
+import { resolveSafeChildPath } from '../../paths';
 
 /**
  * Wraps a zip archive stream into a tree response reader.
@@ -187,10 +188,10 @@ export class ZipArchiveResponse implements ReadTreeResponse {
       const dirname = platformPath.dirname(entryPath);
 
       if (dirname) {
-        await fs.mkdirp(platformPath.join(dir, dirname));
+        await fs.mkdirp(resolveSafeChildPath(dir, dirname));
       }
       return new Promise(async (resolve, reject) => {
-        const file = fs.createWriteStream(platformPath.join(dir, entryPath));
+        const file = fs.createWriteStream(resolveSafeChildPath(dir, entryPath));
         file.on('finish', resolve);
 
         content.on('error', reject);