From 2682ebe7d21a1750e8ccb3b51524a876673d58a6 Mon Sep 17 00:00:00 2001 From: Aramis Sennyey Date: Mon, 14 Nov 2022 12:12:04 -0500 Subject: [PATCH] CR updates, update CLI to be more forceful in rejecting weird relative urls Signed-off-by: Aramis Sennyey --- .changeset/metal-fishes-grow.md | 2 +- packages/app-defaults/src/defaults/apis.ts | 7 ++-- packages/cli/src/commands/index.ts | 8 ----- packages/cli/src/lib/config.test.ts | 36 +++++++++++++++----- packages/cli/src/lib/config.ts | 13 +++++-- packages/core-app-api/src/app/AppManager.tsx | 9 +++-- 6 files changed, 48 insertions(+), 27 deletions(-) diff --git a/.changeset/metal-fishes-grow.md b/.changeset/metal-fishes-grow.md index 01ac5119d4..7067a881bb 100644 --- a/.changeset/metal-fishes-grow.md +++ b/.changeset/metal-fishes-grow.md @@ -2,4 +2,4 @@ '@backstage/core-app-api': patch --- -Updated AppManager to return an absolute URL for `backend.baseUrl` when a relative URL is provided. +Apps will now detect when a relative `backend.baseUrl` is provided and update the config accordingly. diff --git a/packages/app-defaults/src/defaults/apis.ts b/packages/app-defaults/src/defaults/apis.ts index 7ff8ce16bc..3f5cfc1c58 100644 --- a/packages/app-defaults/src/defaults/apis.ts +++ b/packages/app-defaults/src/defaults/apis.ts @@ -64,11 +64,10 @@ export const apis = [ createApiFactory({ api: discoveryApiRef, deps: { configApi: configApiRef }, - factory: ({ configApi }) => { - return UrlPatternDiscovery.compile( + factory: ({ configApi }) => + UrlPatternDiscovery.compile( `${configApi.getString('backend.baseUrl')}/api/{{ pluginId }}`, - ); - }, + ), }), createApiFactory({ api: alertApiRef, diff --git a/packages/cli/src/commands/index.ts b/packages/cli/src/commands/index.ts index 90d3afd4d5..2dd2973de1 100644 --- a/packages/cli/src/commands/index.ts +++ b/packages/cli/src/commands/index.ts @@ -39,14 +39,6 @@ export function registerRepoCommand(program: Command) { '--all', 'Build all packages, including bundled app and backend packages.', ) - .option( - '--public-path ', - 'Public path for hosting the website on, can be relative.', - ) - .option( - '--backend-url ', - 'Backend url, expects just the origin or sub-route. Do not include /api. Can be relative.', - ) .option( '--since ', 'Only build packages and their dev dependents that changed since the specified ref', diff --git a/packages/cli/src/lib/config.test.ts b/packages/cli/src/lib/config.test.ts index bc6d8b6931..9db076e8e4 100644 --- a/packages/cli/src/lib/config.test.ts +++ b/packages/cli/src/lib/config.test.ts @@ -32,13 +32,13 @@ describe('readCliConfig', () => { it('should return backend.baseUrl when backendUrl present in cli options', () => { expect( readCliConfig({ - backendUrl: 'http://localhost:3000', + backendUrl: '/', }), ).toEqual([ { data: { backend: { - baseUrl: 'http://localhost:3000', + baseUrl: '/', }, }, context: 'cli', @@ -49,13 +49,13 @@ describe('readCliConfig', () => { it('should return app.baseUrl when publicPath present in cli options', () => { expect( readCliConfig({ - publicPath: 'http://localhost:3000', + publicPath: '/', }), ).toEqual([ { data: { app: { - baseUrl: 'http://localhost:3000', + baseUrl: '/', }, }, context: 'cli', @@ -66,21 +66,41 @@ describe('readCliConfig', () => { it('should return app.baseUrl and backend.baseUrl when publicPath and backendUrl present in cli options', () => { expect( readCliConfig({ - publicPath: 'http://localhost:3000', - backendUrl: 'http://localhost:3000/api', + publicPath: '/', + backendUrl: '/api', }), ).toEqual([ { data: { app: { - baseUrl: 'http://localhost:3000', + baseUrl: '/', }, backend: { - baseUrl: 'http://localhost:3000/api', + baseUrl: '/api', }, }, context: 'cli', }, ]); }); + + it('should throw for public paths that do NOT start with /', () => { + ['http://localhost:3000', './', '../..'].forEach(publicPath => + expect(() => { + readCliConfig({ + publicPath, + }); + }).toThrow('Public path must be relative'), + ); + }); + + it('should throw for backend urls that do NOT start with /', () => { + ['http://localhost:3000', './', '../..'].forEach(backendUrl => + expect(() => { + readCliConfig({ + backendUrl, + }); + }).toThrow('Backend URL must be relative'), + ); + }); }); diff --git a/packages/cli/src/lib/config.ts b/packages/cli/src/lib/config.ts index 1220586ce0..9084f72057 100644 --- a/packages/cli/src/lib/config.ts +++ b/packages/cli/src/lib/config.ts @@ -52,16 +52,23 @@ export function readCliConfig(opts?: CliConfigOptions): AppConfig[] { if (!opts || Object.keys(opts).length === 0) return []; const data: JsonObject = {}; - if (opts.publicPath) { + if (opts.publicPath?.startsWith('/')) { data.app = { baseUrl: opts.publicPath, }; + } else if (opts.publicPath) { + throw new Error( + 'Public path must be relative and start with "/" when specified through CLI options. Path traversals like "./" and assumed relative endpoints like "backstage" are not supported.', + ); } - - if (opts.backendUrl) { + if (opts.backendUrl?.startsWith('/')) { data.backend = { baseUrl: opts.backendUrl, }; + } else if (opts.backendUrl) { + throw new Error( + 'Backend URL must be relative and start with "/" when specified through CLI options. Path traversals like "./" and assumed relative endpoint like "api" are not supported.', + ); } if (Object.keys(data).length === 0) return []; diff --git a/packages/core-app-api/src/app/AppManager.tsx b/packages/core-app-api/src/app/AppManager.tsx index cd571c7799..176171e975 100644 --- a/packages/core-app-api/src/app/AppManager.tsx +++ b/packages/core-app-api/src/app/AppManager.tsx @@ -179,9 +179,12 @@ function useConfigLoader( context: 'relative-override', }; - configReader = ConfigReader.fromConfigs( - config.value ? [...config.value, relativeBackendConfig] : [], - ); + // Config reader may not have backend.baseUrl set. config.value may be undefined. + if (configReader.getOptionalString('backend.baseUrl')?.startsWith('/')) { + config.value?.push(relativeBackendConfig); + } + + configReader = ConfigReader.fromConfigs(config.value ?? []); return { api: configReader }; }