Merge pull request #2727 from SDA-SE/feat/absolute-placeholder-substituion

feat(catalog-backend): support placeholder replacements for absolute urls
2020-10-02 18:01:40 +02:00
parent e724e6ebfc 0b85f4c1c8
commit e3b602f961
2 changed files with 121 additions and 4 deletions
@@ -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', () => {
@@ -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 {