From 65f913de226033b99efbc1fe7be1d53d6a4dc825 Mon Sep 17 00:00:00 2001
From: Patrik Oldsberg <poldsberg@gmail.com>
Date: Tue, 11 Feb 2025 14:23:56 +0100
Subject: [PATCH] cli/new: reject configurations with duplicate template names

Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com>
---
 .../loadPortableTemplateConfig.test.ts        | 35 +++++++++++++++++--
 .../preparation/loadPortableTemplateConfig.ts | 24 +++++++++----
 2 files changed, 50 insertions(+), 9 deletions(-)

diff --git a/packages/cli/src/lib/new/preparation/loadPortableTemplateConfig.test.ts b/packages/cli/src/lib/new/preparation/loadPortableTemplateConfig.test.ts
index d6e1370bf7..b8a7940bcf 100644
--- a/packages/cli/src/lib/new/preparation/loadPortableTemplateConfig.test.ts
+++ b/packages/cli/src/lib/new/preparation/loadPortableTemplateConfig.test.ts
@@ -19,6 +19,7 @@ import { loadPortableTemplateConfig } from './loadPortableTemplateConfig';
 import { defaultTemplates } from '../defaultTemplates';
 import { createMockDirectory } from '@backstage/backend-test-utils';
 import { TEMPLATE_FILE_NAME } from '../types';
+import { basename } from 'node:path';
 
 describe('loadPortableTemplateConfig', () => {
   const mockDir = createMockDirectory();
@@ -174,7 +175,7 @@ describe('loadPortableTemplateConfig', () => {
       node_modules: Object.fromEntries(
         defaultTemplates.map(t => [
           t,
-          { [TEMPLATE_FILE_NAME]: `name: x\nrole: web-library\n` },
+          { [TEMPLATE_FILE_NAME]: `name: ${basename(t)}\nrole: web-library\n` },
         ]),
       ),
     });
@@ -186,7 +187,7 @@ describe('loadPortableTemplateConfig', () => {
     ).resolves.toEqual({
       isUsingDefaultTemplates: true,
       templatePointers: defaultTemplates.map(t => ({
-        name: 'x',
+        name: basename(t),
         target: realpathSync(
           mockDir.resolve(`node_modules/${t}`, TEMPLATE_FILE_NAME),
         ),
@@ -199,6 +200,34 @@ describe('loadPortableTemplateConfig', () => {
     });
   });
 
+  it('should reject templates with conflicting names', async () => {
+    mockDir.setContent({
+      'package.json': JSON.stringify({
+        backstage: {
+          cli: {
+            new: {
+              templates: ['./template1', './template2'],
+            },
+          },
+        },
+      }),
+      template1: {
+        [TEMPLATE_FILE_NAME]: 'name: test\nrole: frontend-plugin\n',
+      },
+      template2: {
+        [TEMPLATE_FILE_NAME]: 'name: test\nrole: backend-plugin\n',
+      },
+    });
+
+    await expect(
+      loadPortableTemplateConfig({
+        packagePath: mockDir.resolve('package.json'),
+      }),
+    ).rejects.toThrow(
+      `Invalid template configuration, received conflicting template name 'test' from './template1' and './template2'`,
+    );
+  });
+
   it('should throw an error if package.json is invalid', async () => {
     mockDir.setContent({
       'package.json': JSON.stringify({
@@ -271,7 +300,7 @@ describe('loadPortableTemplateConfig', () => {
       node_modules: Object.fromEntries(
         defaultTemplates.map(t => [
           t,
-          { [TEMPLATE_FILE_NAME]: `name: x\nrole: web-library\n` },
+          { [TEMPLATE_FILE_NAME]: `name: ${basename(t)}\nrole: web-library\n` },
         ]),
       ),
     });
diff --git a/packages/cli/src/lib/new/preparation/loadPortableTemplateConfig.ts b/packages/cli/src/lib/new/preparation/loadPortableTemplateConfig.ts
index 292f57ca0b..d4705b0320 100644
--- a/packages/cli/src/lib/new/preparation/loadPortableTemplateConfig.ts
+++ b/packages/cli/src/lib/new/preparation/loadPortableTemplateConfig.ts
@@ -89,24 +89,36 @@ export async function loadPortableTemplateConfig(
   const config = parsed.data.backstage?.cli?.new;
 
   const basePath = dirname(pkgPath);
-  const templatePointers = await Promise.all(
-    (config?.templates ?? defaultTemplates).map(async pointer => {
+  const templatePointerEntries = await Promise.all(
+    (config?.templates ?? defaultTemplates).map(async rawPointer => {
       try {
-        const templatePath = resolveLocalTemplatePath(pointer, basePath);
+        const templatePath = resolveLocalTemplatePath(rawPointer, basePath);
 
-        return await peekLocalTemplateDefinition(templatePath);
+        const pointer = await peekLocalTemplateDefinition(templatePath);
+        return { pointer, rawPointer };
       } catch (error) {
         throw new ForwardedError(
-          `Failed to load template definition '${pointer}'`,
+          `Failed to load template definition '${rawPointer}'`,
           error,
         );
       }
     }),
   );
 
+  const templateNameConflicts = new Map<string, string>();
+  for (const { pointer, rawPointer } of templatePointerEntries) {
+    const conflict = templateNameConflicts.get(pointer.name);
+    if (conflict) {
+      throw new Error(
+        `Invalid template configuration, received conflicting template name '${pointer.name}' from '${conflict}' and '${rawPointer}'`,
+      );
+    }
+    templateNameConflicts.set(pointer.name, rawPointer);
+  }
+
   return {
     isUsingDefaultTemplates: !config?.templates,
-    templatePointers,
+    templatePointers: templatePointerEntries.map(({ pointer }) => pointer),
     license: overrides.license ?? config?.globals?.license ?? defaults.license,
     version: overrides.version ?? config?.globals?.version ?? defaults.version,
     private: overrides.private ?? config?.globals?.private ?? defaults.private,