Merge pull request from GHSA-4jqc-jvh2-pxg9

Path traversal for local publishers in TechDocs backend
2022-06-13 14:35:57 +02:00
parent 6659e90873 5296b00d4a
commit 429c9f9daa
2 changed files with 167 additions and 81 deletions
@@ -54,62 +54,107 @@ const resolvedDir = resolvePackagePath(
 );

 describe('local publisher', () => {
-  it('should publish generated documentation dir', async () => {
-    mockFs({
-      [tmpDir]: {
-        'index.html': '',
-      },
+  describe('publish', () => {
+    beforeEach(() => {
+      mockFs({
+        [tmpDir]: {
+          'index.html': '',
+        },
+      });
    });

-    const mockConfig = new ConfigReader({});
-
-    const publisher = LocalPublish.fromConfig(
-      mockConfig,
-      logger,
-      testDiscovery,
-    );
-    const mockEntity = createMockEntity();
-    const lowerMockEntity = createMockEntity(undefined, true);
-
-    await publisher.publish({ entity: mockEntity, directory: tmpDir });
-
-    expect(await publisher.hasDocsBeenGenerated(mockEntity)).toBe(true);
-
-    // Lower/upper should be treated the same.
-    expect(await publisher.hasDocsBeenGenerated(lowerMockEntity)).toBe(true);
-
-    mockFs.restore();
-  });
-
-  it('should respect legacy casing', async () => {
-    mockFs({
-      [tmpDir]: {
-        'index.html': '',
-      },
+    afterEach(() => {
+      mockFs.restore();
    });

-    const mockConfig = new ConfigReader({
-      techdocs: {
-        legacyUseCaseSensitiveTripletPaths: true,
-      },
+    it('should publish generated documentation dir', async () => {
+      const mockConfig = new ConfigReader({});
+
+      const publisher = LocalPublish.fromConfig(
+        mockConfig,
+        logger,
+        testDiscovery,
+      );
+      const mockEntity = createMockEntity();
+      const lowerMockEntity = createMockEntity(undefined, true);
+
+      await publisher.publish({ entity: mockEntity, directory: tmpDir });
+
+      expect(await publisher.hasDocsBeenGenerated(mockEntity)).toBe(true);
+
+      // Lower/upper should be treated the same.
+      expect(await publisher.hasDocsBeenGenerated(lowerMockEntity)).toBe(true);
+
+      mockFs.restore();
    });

-    const publisher = LocalPublish.fromConfig(
-      mockConfig,
-      logger,
-      testDiscovery,
-    );
-    const mockEntity = createMockEntity();
-    const lowerMockEntity = createMockEntity(undefined, true);
+    it('should respect legacy casing', async () => {
+      const mockConfig = new ConfigReader({
+        techdocs: {
+          legacyUseCaseSensitiveTripletPaths: true,
+        },
+      });

-    await publisher.publish({ entity: mockEntity, directory: tmpDir });
+      const publisher = LocalPublish.fromConfig(
+        mockConfig,
+        logger,
+        testDiscovery,
+      );
+      const mockEntity = createMockEntity();
+      const lowerMockEntity = createMockEntity(undefined, true);

-    expect(await publisher.hasDocsBeenGenerated(mockEntity)).toBe(true);
+      await publisher.publish({ entity: mockEntity, directory: tmpDir });

-    // Lower/upper should be treated differently.
-    expect(await publisher.hasDocsBeenGenerated(lowerMockEntity)).toBe(false);
+      expect(await publisher.hasDocsBeenGenerated(mockEntity)).toBe(true);

-    mockFs.restore();
+      // Lower/upper should be treated differently.
+      expect(await publisher.hasDocsBeenGenerated(lowerMockEntity)).toBe(false);
+
+      mockFs.restore();
+    });
+
+    it('should throw with unsafe triplet', async () => {
+      const mockConfig = new ConfigReader({});
+      const publisher = LocalPublish.fromConfig(
+        mockConfig,
+        logger,
+        testDiscovery,
+      );
+      const mockEntity = {
+        ...createMockEntity(),
+        ...{
+          kind: '..',
+          metadata: { name: '..', namespace: '..' },
+        },
+      };
+
+      await expect(() =>
+        publisher.publish({ entity: mockEntity, directory: tmpDir }),
+      ).rejects.toThrowError('Unable to publish TechDocs site');
+    });
+
+    it('should throw with unsafe name', async () => {
+      const mockConfig = new ConfigReader({});
+      const publisher = LocalPublish.fromConfig(
+        mockConfig,
+        logger,
+        testDiscovery,
+      );
+      const mockEntity = {
+        ...createMockEntity(),
+        ...{
+          kind: 'component',
+          metadata: {
+            name: '../component/other-component',
+            namespace: 'default',
+          },
+        },
+      };
+
+      await expect(() =>
+        publisher.publish({ entity: mockEntity, directory: tmpDir }),
+      ).rejects.toThrowError('Unable to publish TechDocs site');
+    });
  });

  describe('docsRouter', () => {
@@ -16,8 +16,13 @@
 import {
  PluginEndpointDiscovery,
  resolvePackagePath,
+  resolveSafeChildPath,
 } from '@backstage/backend-common';
-import { Entity, CompoundEntityRef } from '@backstage/catalog-model';
+import {
+  Entity,
+  CompoundEntityRef,
+  stringifyEntityRef,
+} from '@backstage/catalog-model';
 import { Config } from '@backstage/config';
 import express from 'express';
 import fs from 'fs-extra';
@@ -37,7 +42,7 @@ import {
  getHeadersForFileExtension,
  lowerCaseEntityTripletInStoragePath,
 } from './helpers';
-import { assertError } from '@backstage/errors';
+import { ForwardedError } from '@backstage/errors';

 // TODO: Use a more persistent storage than node_modules or /tmp directory.
 // Make it configurable with techdocs.publisher.local.publishDirectory
@@ -103,12 +108,22 @@ export class LocalPublish implements PublisherBase {
    directory,
  }: PublishRequest): Promise<PublishResponse> {
    const entityNamespace = entity.metadata.namespace ?? 'default';
+    let publishDir: string;

-    const publishDir = this.staticEntityPathJoin(
-      entityNamespace,
-      entity.kind,
-      entity.metadata.name,
-    );
+    try {
+      publishDir = this.staticEntityPathJoin(
+        entityNamespace,
+        entity.kind,
+        entity.metadata.name,
+      );
+    } catch (error) {
+      throw new ForwardedError(
+        `Unable to publish TechDocs site for entity: ${stringifyEntityRef(
+          entity,
+        )}`,
+        error,
+      );
+    }

    if (!fs.existsSync(publishDir)) {
      this.logger.info(`Could not find ${publishDir}, creating the directory.`);
@@ -144,21 +159,31 @@ export class LocalPublish implements PublisherBase {
  async fetchTechDocsMetadata(
    entityName: CompoundEntityRef,
  ): Promise<TechDocsMetadata> {
-    const metadataPath = this.staticEntityPathJoin(
-      entityName.namespace,
-      entityName.kind,
-      entityName.name,
-      'techdocs_metadata.json',
-    );
+    let metadataPath: string;
+
+    try {
+      metadataPath = this.staticEntityPathJoin(
+        entityName.namespace,
+        entityName.kind,
+        entityName.name,
+        'techdocs_metadata.json',
+      );
+    } catch (err) {
+      throw new ForwardedError(
+        `Unexpected entity when fetching metadata: ${stringifyEntityRef(
+          entityName,
+        )}`,
+        err,
+      );
+    }

    try {
      return await fs.readJson(metadataPath);
    } catch (err) {
-      assertError(err);
-      this.logger.error(
+      throw new ForwardedError(
        `Unable to read techdocs_metadata.json at ${metadataPath}. Error: ${err}`,
+        err,
      );
-      throw new Error(err.message);
    }
  }

@@ -217,18 +242,26 @@ export class LocalPublish implements PublisherBase {
  async hasDocsBeenGenerated(entity: Entity): Promise<boolean> {
    const namespace = entity.metadata.namespace ?? 'default';

-    const indexHtmlPath = this.staticEntityPathJoin(
-      namespace,
-      entity.kind,
-      entity.metadata.name,
-      'index.html',
-    );
-
    // Check if the file exists
    try {
+      const indexHtmlPath = this.staticEntityPathJoin(
+        namespace,
+        entity.kind,
+        entity.metadata.name,
+        'index.html',
+      );
+
      await fs.access(indexHtmlPath, fs.constants.F_OK);
+
      return true;
    } catch (err) {
+      if (err.name === 'NotAllowedError') {
+        this.logger.error(
+          `Unexpected entity when checking if generated: ${stringifyEntityRef(
+            entity,
+          )}`,
+        );
+      }
      return false;
    }
  }
@@ -278,17 +311,25 @@ export class LocalPublish implements PublisherBase {
   * Utility wrapper around path.join(), used to control legacy case logic.
   */
  protected staticEntityPathJoin(...allParts: string[]): string {
-    if (this.legacyPathCasing) {
-      const [namespace, kind, name, ...parts] = allParts;
-      return path.join(staticDocsDir, namespace, kind, name, ...parts);
-    }
-    const [namespace, kind, name, ...parts] = allParts;
-    return path.join(
-      staticDocsDir,
-      namespace.toLowerCase(),
-      kind.toLowerCase(),
-      name.toLowerCase(),
-      ...parts,
-    );
+    let staticEntityPath = staticDocsDir;
+
+    allParts
+      .map(part => part.split(path.sep))
+      .flat()
+      .forEach((part, index) => {
+        // Respect legacy path casing when operating on namespace, kind, or name.
+        if (index < 3) {
+          staticEntityPath = resolveSafeChildPath(
+            staticEntityPath,
+            this.legacyPathCasing ? part : part.toLowerCase(),
+          );
+          return;
+        }
+
+        // Otherwise, respect the provided case.
+        staticEntityPath = resolveSafeChildPath(staticEntityPath, part);
+      });
+
+    return staticEntityPath;
  }
 }