From 0b85f4c1c8fec6e587240df8a5b6fa5311b257c0 Mon Sep 17 00:00:00 2001
From: Oliver Sand <oliver.sand@sda-se.com>
Date: Fri, 2 Oct 2020 16:56:17 +0200
Subject: [PATCH] feat(catalog-backend): support placeholder replacements for
 absolute urls

This adds support, even if the entity itself comes from a relative file location.
---
 .../processors/PlaceholderProcessor.test.ts   | 108 ++++++++++++++++++
 .../processors/PlaceholderProcessor.ts        |  17 ++-
 2 files changed, 121 insertions(+), 4 deletions(-)

diff --git a/plugins/catalog-backend/src/ingestion/processors/PlaceholderProcessor.test.ts b/plugins/catalog-backend/src/ingestion/processors/PlaceholderProcessor.test.ts
index d00e4b7b6f..6893cae0e7 100644
--- a/plugins/catalog-backend/src/ingestion/processors/PlaceholderProcessor.test.ts
+++ b/plugins/catalog-backend/src/ingestion/processors/PlaceholderProcessor.test.ts
@@ -230,6 +230,114 @@ describe('PlaceholderProcessor', () => {
       target: 'https://github.com/spotify/backstage/a/file.yaml',
     });
   });
+
+  it('resolves absolute path for absolute location', async () => {
+    read.mockResolvedValue(Buffer.from('TEXT', 'utf-8'));
+    const processor = PlaceholderProcessor.default();
+
+    await expect(
+      processor.processEntity(
+        {
+          apiVersion: 'a',
+          kind: 'k',
+          metadata: { name: 'n' },
+          spec: {
+            data: {
+              $text: 'https://github.com/spotify/backstage/catalog-info.yaml',
+            },
+          },
+        },
+        {
+          type: 'github',
+          target: 'https://github.com/spotify/backstage/a/b/catalog-info.yaml',
+        },
+        emit,
+        read,
+      ),
+    ).resolves.toEqual({
+      apiVersion: 'a',
+      kind: 'k',
+      metadata: { name: 'n' },
+      spec: { data: 'TEXT' },
+    });
+
+    expect(emit).not.toBeCalled();
+    expect(read).toBeCalledWith({
+      type: 'github',
+      target: 'https://github.com/spotify/backstage/catalog-info.yaml',
+    });
+  });
+
+  it('resolves absolute path for relative file location', async () => {
+    read.mockResolvedValue(Buffer.from('TEXT', 'utf-8'));
+    const processor = PlaceholderProcessor.default();
+
+    await expect(
+      processor.processEntity(
+        {
+          apiVersion: 'a',
+          kind: 'k',
+          metadata: { name: 'n' },
+          spec: {
+            data: {
+              $text: 'https://github.com/spotify/backstage/catalog-info.yaml',
+            },
+          },
+        },
+        {
+          type: 'github',
+          target: './a/b/catalog-info.yaml',
+        },
+        emit,
+        read,
+      ),
+    ).resolves.toEqual({
+      apiVersion: 'a',
+      kind: 'k',
+      metadata: { name: 'n' },
+      spec: { data: 'TEXT' },
+    });
+
+    expect(emit).not.toBeCalled();
+    expect(read).toBeCalledWith({
+      type: 'github',
+      target: 'https://github.com/spotify/backstage/catalog-info.yaml',
+    });
+  });
+
+  it('not resolves relative file path for relative file location', async () => {
+    // We explicitly don't support this case, as it would allow for file system
+    // traversel attacks. If we want to implement this, we need to have additional
+    // security measures in place!
+    read.mockResolvedValue(Buffer.from('TEXT', 'utf-8'));
+    const processor = PlaceholderProcessor.default();
+
+    await expect(
+      processor.processEntity(
+        {
+          apiVersion: 'a',
+          kind: 'k',
+          metadata: { name: 'n' },
+          spec: {
+            data: {
+              $text: '../c/catalog-info.yaml',
+            },
+          },
+        },
+        {
+          type: 'github',
+          target: './a/b/catalog-info.yaml',
+        },
+        emit,
+        read,
+      ),
+    ).rejects.toThrow(
+      'Placeholder $text could not form an URL out of ./a/b/catalog-info.yaml and ../c/catalog-info.yaml',
+    );
+
+    expect(emit).not.toBeCalled();
+    expect(read).not.toBeCalled();
+  });
 });
 
 describe('yamlPlaceholderResolver', () => {
diff --git a/plugins/catalog-backend/src/ingestion/processors/PlaceholderProcessor.ts b/plugins/catalog-backend/src/ingestion/processors/PlaceholderProcessor.ts
index 65433b1849..b811d217d5 100644
--- a/plugins/catalog-backend/src/ingestion/processors/PlaceholderProcessor.ts
+++ b/plugins/catalog-backend/src/ingestion/processors/PlaceholderProcessor.ts
@@ -198,10 +198,19 @@ function relativeLocation({
   try {
     // The two-value form of the URL constructor handles relative paths for us
     url = new URL(value, location.target);
-  } catch (e) {
-    throw new Error(
-      `Placeholder \$${key} could not form an URL out of ${location.target} and ${value}`,
-    );
+  } catch {
+    try {
+      // Check whether value is a valid absolute URL on it's own, if not fail.
+      url = new URL(value);
+    } catch {
+      // The only remaining case that isn't support is a relative file path that should be
+      // resolved using a relative file location. Accessing local file paths can lead to
+      // path traversal attacks and access to any file on the host system. Implementing this
+      // would require additional security measures.
+      throw new Error(
+        `Placeholder \$${key} could not form an URL out of ${location.target} and ${value}`,
+      );
+    }
   }
 
   return {