fix(proxy-backend): prefix routes with / if not present in config

This commit ensures that the default `pathRewrite` configuration for a proxy element is more resilient to the combination of whether:
- `route` is prefixed with `/`
- `pathPrefix` on the plugin is suffixed with `/` (which at present it will never be - see comments).

Signed-off-by: Brian Fox <brianhfox@gmail.com>

.changeset/thick-cobras-switch.md

@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-proxy-backend': patch
+---
+
+Prefix proxy routes with `/` if not present in configuration

docs/plugins/proxying.md

@@ -36,7 +36,7 @@ Example:
 ```yaml
 # in app-config.yaml
 proxy:
-  '/simple-example': http://simple.example.com:8080
+  simple-example: http://simple.example.com:8080
   '/larger-example/v1':
     target: http://larger.example.com:8080/svc.v1
     headers:
@@ -46,10 +46,11 @@ proxy:
 ```
 
 Each key under the proxy configuration entry is a route to match, below the
-prefix that the proxy plugin is mounted on. It must start with a slash. For
-example, if the backend mounts the proxy plugin as `/proxy`, the above
-configuration will lead to the proxy acting on backend requests to
-`/api/proxy/simple-example/...` and `/api/proxy/larger-example/v1/...`.
+prefix that the proxy plugin is mounted on. If it does not start with a slash,
+one will be prefixed automatically. For example, if the backend mounts the proxy
+plugin as `/proxy`, the above configuration will lead to the proxy acting on
+backend requests to `/api/proxy/simple-example/...` and
+`/api/proxy/larger-example/v1/...`.
 
 The value inside each route is either a simple URL string, or an object on the
 format accepted by

plugins/proxy-backend/src/service/router.test.ts

@@ -59,7 +59,7 @@ describe('buildMiddleware', () => {
     mockCreateProxyMiddleware.mockClear();
   });
 
-  it('accepts strings', async () => {
+  it('accepts strings prefixed by /', async () => {
     buildMiddleware('/proxy', logger, '/test', 'http://mocked');
 
     expect(createProxyMiddleware).toHaveBeenCalledTimes(1);
@@ -79,6 +79,46 @@ describe('buildMiddleware', () => {
     expect(fullConfig.logProvider!(logger)).toBe(logger);
   });
 
+  it('accepts routes not prefixed with / when path is not suffixed with /', async () => {
+    buildMiddleware('/proxy', logger, 'test', 'http://mocked');
+
+    expect(createProxyMiddleware).toHaveBeenCalledTimes(1);
+
+    const [filter, fullConfig] = mockCreateProxyMiddleware.mock.calls[0] as [
+      (pathname: string, req: Partial<http.IncomingMessage>) => boolean,
+      ProxyMiddlewareConfig,
+    ];
+    expect(filter('', { method: 'GET', headers: {} })).toBe(true);
+    expect(filter('', { method: 'POST', headers: {} })).toBe(true);
+    expect(filter('', { method: 'PUT', headers: {} })).toBe(true);
+    expect(filter('', { method: 'PATCH', headers: {} })).toBe(true);
+    expect(filter('', { method: 'DELETE', headers: {} })).toBe(true);
+
+    expect(fullConfig.pathRewrite).toEqual({ '^/proxy/test/': '/' });
+    expect(fullConfig.changeOrigin).toBe(true);
+    expect(fullConfig.logProvider!(logger)).toBe(logger);
+  });
+
+  it('accepts routes prefixed with / when path is suffixed with /', async () => {
+    buildMiddleware('/proxy/', logger, '/test', 'http://mocked');
+
+    expect(createProxyMiddleware).toHaveBeenCalledTimes(1);
+
+    const [filter, fullConfig] = mockCreateProxyMiddleware.mock.calls[0] as [
+      (pathname: string, req: Partial<http.IncomingMessage>) => boolean,
+      ProxyMiddlewareConfig,
+    ];
+    expect(filter('', { method: 'GET', headers: {} })).toBe(true);
+    expect(filter('', { method: 'POST', headers: {} })).toBe(true);
+    expect(filter('', { method: 'PUT', headers: {} })).toBe(true);
+    expect(filter('', { method: 'PATCH', headers: {} })).toBe(true);
+    expect(filter('', { method: 'DELETE', headers: {} })).toBe(true);
+
+    expect(fullConfig.pathRewrite).toEqual({ '^/proxy/test/': '/' });
+    expect(fullConfig.changeOrigin).toBe(true);
+    expect(fullConfig.logProvider!(logger)).toBe(logger);
+  });
+
   it('limits allowedMethods', async () => {
     buildMiddleware('/proxy', logger, '/test', {
       target: 'http://mocked',

plugins/proxy-backend/src/service/router.ts

@@ -77,10 +77,24 @@ export function buildMiddleware(
       `Proxy target is not a valid URL: ${fullConfig.target ?? ''}`,
     );
   }
+
   // Default is to do a path rewrite that strips out the proxy's path prefix
   // and the rest of the route.
   if (fullConfig.pathRewrite === undefined) {
-    const routeWithSlash = route.endsWith('/') ? route : `${route}/`;
+    let routeWithSlash = route.endsWith('/') ? route : `${route}/`;
+
+    if (!pathPrefix.endsWith('/') && !routeWithSlash.startsWith('/')) {
+      // Need to insert a / between pathPrefix and routeWithSlash
+      routeWithSlash = `/${routeWithSlash}`;
+    } else if (pathPrefix.endsWith('/') && routeWithSlash.startsWith('/')) {
+      // Never expect this to happen at this point in time as
+      // pathPrefix is set using `getExternalBaseUrl` which "Returns the
+      // external HTTP base backend URL for a given plugin,
+      // **without a trailing slash.**". But in case this changes in future, we
+      // need to drop a / on either pathPrefix or routeWithSlash
+      routeWithSlash = routeWithSlash.substring(1);
+    }
+
     fullConfig.pathRewrite = {
       [`^${pathPrefix}${routeWithSlash}`]: '/',
     };