feat(proxy-backend): limit the forwarded http headers to a safe set

.changeset/2804.md

@@ -0,0 +1,15 @@
+---
+'@backstage/plugin-proxy-backend': minor
+---
+
+Limit the http headers that are forwarded from the request to a safe set of defaults.
+A user can configure additional headers that should be forwarded if the specific applications needs that.
+
+```yaml
+proxy:
+  '/my-api':
+    target: 'https://my-api.com/get'
+    allowedHeaders:
+      # We need to forward the Authorization header that was provided by the caller
+      - Authorization
+```

docs/plugins/proxying.md

@@ -57,6 +57,15 @@ is also possible to limit the forwarded HTTP methods with the configuration
 `allowedMethods`, for example `allowedMethods: ['GET']` to enforce read-only
 access.
 
+By default, the proxy will only forward safe HTTP request headers to the target.
+Those are based on the headers that are considered safe for CORS and includes
+headers like `content-type` or `last-modified`, as well as all headers that are
+set by the proxy. If the proxy should forward other headers like
+`authorization`, this must be enabled by the `allowedHeaders` config, for
+example `allowedHeaders: ['Authorization']`. This should help to not
+accidentally forward confidential headers (`cookie`, `X-Auth-Request-User`) to
+third-parties.
+
 If the value is a string, it is assumed to correspond to:
 
 ```yaml

plugins/proxy-backend/package.json

@@ -22,7 +22,6 @@
     "@backstage/backend-common": "^0.1.1-alpha.24",
     "@backstage/config": "^0.1.1-alpha.24",
     "@types/express": "^4.17.6",
-    "@types/http-proxy-middleware": "^0.19.3",
     "express": "^4.17.1",
     "express-promise-router": "^3.0.3",
     "http-proxy-middleware": "^0.19.1",
@@ -36,6 +35,7 @@
   },
   "devDependencies": {
     "@backstage/cli": "^0.1.1-alpha.24",
+    "@types/http-proxy-middleware": "^0.19.3",
     "@types/node-fetch": "^2.5.7",
     "@types/supertest": "^2.0.8",
     "@types/uuid": "^8.0.0",

plugins/proxy-backend/src/index.ts

@@ -14,4 +14,4 @@
  * limitations under the License.
  */
 
-export * from './service/router';
+export * from './service';

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

@@ -0,0 +1,17 @@
+/*
+ * Copyright 2020 Spotify AB
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+export { createRouter } from './router';

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

@@ -14,13 +14,30 @@
  * limitations under the License.
  */
 
-import { createRouter } from './router';
+import { buildMiddleware, createRouter } from './router';
 import * as winston from 'winston';
 import { ConfigReader } from '@backstage/config';
 import {
   loadBackendConfig,
   SingleHostDiscovery,
 } from '@backstage/backend-common';
+import createProxyMiddleware, {
+  Config as ProxyMiddlewareConfig,
+  Proxy,
+} from 'http-proxy-middleware';
+import * as http from 'http';
+
+jest.mock('http-proxy-middleware', () => {
+  return jest.fn().mockImplementation(
+    (): Proxy => {
+      return () => undefined;
+    },
+  );
+});
+
+const mockCreateProxyMiddleware = createProxyMiddleware as jest.MockedFunction<
+  typeof createProxyMiddleware
+>;
 
 describe('createRouter', () => {
   it('works', async () => {
@@ -35,3 +52,151 @@ describe('createRouter', () => {
     expect(router).toBeDefined();
   });
 });
+
+describe('buildMiddleware', () => {
+  const logger = winston.createLogger();
+
+  beforeEach(() => {
+    mockCreateProxyMiddleware.mockClear();
+  });
+
+  it('accepts strings', async () => {
+    buildMiddleware('/api/', 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' })).toBe(true);
+    expect(filter('', { method: 'POST' })).toBe(true);
+    expect(filter('', { method: 'PUT' })).toBe(true);
+    expect(filter('', { method: 'PATCH' })).toBe(true);
+    expect(filter('', { method: 'DELETE' })).toBe(true);
+
+    expect(fullConfig.pathRewrite).toEqual({ '^/api/test/': '/' });
+    expect(fullConfig.changeOrigin).toBe(true);
+    expect(fullConfig.logProvider!(logger)).toBe(logger);
+  });
+
+  it('limits allowedMethods', async () => {
+    buildMiddleware('/api/', logger, 'test', {
+      target: 'http://mocked',
+      allowedMethods: ['GET', 'DELETE'],
+    });
+
+    expect(createProxyMiddleware).toHaveBeenCalledTimes(1);
+
+    const [filter, fullConfig] = mockCreateProxyMiddleware.mock.calls[0] as [
+      (pathname: string, req: Partial<http.IncomingMessage>) => boolean,
+      ProxyMiddlewareConfig,
+    ];
+    expect(filter('', { method: 'GET' })).toBe(true);
+    expect(filter('', { method: 'POST' })).toBe(false);
+    expect(filter('', { method: 'PUT' })).toBe(false);
+    expect(filter('', { method: 'PATCH' })).toBe(false);
+    expect(filter('', { method: 'DELETE' })).toBe(true);
+
+    expect(fullConfig.pathRewrite).toEqual({ '^/api/test/': '/' });
+    expect(fullConfig.changeOrigin).toBe(true);
+    expect(fullConfig.logProvider!(logger)).toBe(logger);
+  });
+
+  it('permits default headers', async () => {
+    buildMiddleware('/api/', logger, 'test', {
+      target: 'http://mocked',
+    });
+
+    expect(createProxyMiddleware).toHaveBeenCalledTimes(1);
+
+    const config = mockCreateProxyMiddleware.mock
+      .calls[0][1] as ProxyMiddlewareConfig;
+
+    const testClientRequest = {
+      getHeaderNames: () => [
+        'cache-control',
+        'content-language',
+        'content-length',
+        'content-type',
+        'expires',
+        'last-modified',
+        'pragma',
+        'host',
+        'accept',
+        'accept-language',
+        'user-agent',
+        'cookie',
+      ],
+      removeHeader: jest.fn(),
+    } as Partial<http.ClientRequest>;
+
+    expect(config).toBeDefined();
+    expect(config.onProxyReq).toBeDefined();
+
+    config.onProxyReq!(
+      testClientRequest as http.ClientRequest,
+      {} as http.IncomingMessage,
+      {} as http.ServerResponse,
+    );
+
+    expect(testClientRequest.removeHeader).toHaveBeenCalledTimes(1);
+    expect(testClientRequest.removeHeader).toHaveBeenCalledWith('cookie');
+  });
+
+  it('permits default and configured headers', async () => {
+    buildMiddleware('/api/', logger, 'test', {
+      target: 'http://mocked',
+      headers: {
+        Authorization: 'my-token',
+      },
+    });
+
+    expect(createProxyMiddleware).toHaveBeenCalledTimes(1);
+
+    const config = mockCreateProxyMiddleware.mock
+      .calls[0][1] as ProxyMiddlewareConfig;
+
+    const testClientRequest = {
+      getHeaderNames: () => ['authorization', 'Cookie'],
+      removeHeader: jest.fn(),
+    } as Partial<http.ClientRequest>;
+
+    config.onProxyReq!(
+      testClientRequest as http.ClientRequest,
+      {} as http.IncomingMessage,
+      {} as http.ServerResponse,
+    );
+
+    expect(testClientRequest.removeHeader).toHaveBeenCalledTimes(1);
+    expect(testClientRequest.removeHeader).toHaveBeenCalledWith('Cookie');
+  });
+
+  it('permits configured headers', async () => {
+    buildMiddleware('/api/', logger, 'test', {
+      target: 'http://mocked',
+      allowedHeaders: ['authorization', 'cookie'],
+    });
+
+    expect(createProxyMiddleware).toHaveBeenCalledTimes(1);
+
+    const config = mockCreateProxyMiddleware.mock
+      .calls[0][1] as ProxyMiddlewareConfig;
+
+    const testClientRequest = {
+      getHeaderNames: () => ['authorization', 'Cookie', 'X-Auth-Request-User'],
+      removeHeader: jest.fn(),
+    } as Partial<http.ClientRequest>;
+
+    config.onProxyReq!(
+      testClientRequest as http.ClientRequest,
+      {} as http.IncomingMessage,
+      {} as http.ServerResponse,
+    );
+
+    expect(testClientRequest.removeHeader).toHaveBeenCalledTimes(1);
+    expect(testClientRequest.removeHeader).toHaveBeenCalledWith(
+      'X-Auth-Request-User',
+    );
+  });
+});

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

@@ -25,6 +25,27 @@ import { Logger } from 'winston';
 import http from 'http';
 import { PluginEndpointDiscovery } from '@backstage/backend-common';
 
+// A list of headers that are always forwarded to the proxy targets.
+const safeForwardHeaders = [
+  // https://fetch.spec.whatwg.org/#cors-safelisted-request-header
+  'cache-control',
+  'content-language',
+  'content-length',
+  'content-type',
+  'expires',
+  'last-modified',
+  'pragma',
+
+  // host is overridden by default. if changeOrigin is configured to false,
+  // we assume this is a intentional and should also be forwarded.
+  'host',
+
+  // other headers that we assume to be ok
+  'accept',
+  'accept-language',
+  'user-agent',
+];
+
 export interface RouterOptions {
   logger: Logger;
   config: Config;
@@ -33,11 +54,12 @@ export interface RouterOptions {
 
 export interface ProxyConfig extends ProxyMiddlewareConfig {
   allowedMethods?: string[];
+  allowedHeaders?: string[];
 }
 
 // Creates a proxy middleware, possibly with defaults added on top of the
 // given config.
-function buildMiddleware(
+export function buildMiddleware(
   pathPrefix: string,
   logger: Logger,
   route: string,
@@ -68,6 +90,30 @@ function buildMiddleware(
     return fullConfig?.allowedMethods?.includes(req.method!) ?? true;
   };
 
+  // Only forward the allowed HTTP headers to not forward unwanted secret headers
+  const headerAllowList = new Set<string>(
+    [
+      // allow all safe headers
+      ...safeForwardHeaders,
+
+      // allow all headers that are set by the proxy
+      ...((fullConfig.headers && Object.keys(fullConfig.headers)) || []),
+
+      // allow all configured headers
+      ...(fullConfig.allowedHeaders || []),
+    ].map(h => h.toLocaleLowerCase()),
+  );
+
+  fullConfig.onProxyReq = (proxyReq: http.ClientRequest) => {
+    const headerNames = proxyReq.getHeaderNames();
+
+    headerNames.forEach(h => {
+      if (!headerAllowList.has(h.toLocaleLowerCase())) {
+        proxyReq.removeHeader(h);
+      }
+    });
+  };
+
   return createProxyMiddleware(filter, fullConfig);
 }