Repository: paularmstrong/build-tracker
Pull request #88 (2019-09-02 04:38:34)

[api] add BT_AUTH_API_TOKEN env var to protect POST endpoint
<!--
Thank you so much for contributing to open source and the Build Tracker project!
-->

# Problem

The `/api/build` POST endpoint is totally open and anyone can write to it.

# Solution

<!--
When trying to solve more solutions with Build Tracker, please keep in mind some of the following goals of the project:
* Be lightweight: small package sizes (single-digit KiBs, gzipped)
* Be easy: too many options in an API can become confusing
* Be clear: the intended purpose of every method should be as obvious as possible
* Is it easy to do this in "userland"? Would it be better off done there?
-->

If an env variable, `BT_API_AUTH_TOKEN` is set, it will be used to protect all POST requests to the API.

# TODO

- [x] 🤓 Add & update tests (always try to _increase_ test coverage)
- [x] 🔬 Ensure CI is passing (`yarn lint:ci`, `yarn test`, `yarn tsc:ci`)
- [x] 📖 Update relevant documentation



Patch:
diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml
index b09120e..4f87a9c 100644
--- a/.github/workflows/push.yml
+++ b/.github/workflows/push.yml
@@ -37,6 +37,8 @@ jobs:
         run: yarn build
       - name: Upload build
         run: yarn ts-node src/cli/src/bin.ts upload-build -c demo/build-tracker-cli.config.js -b next
+        env:
+          BT_API_AUTH_TOKEN: ${{ secrets.BT_API_AUTH_TOKEN }}
 
   deploy:
     name: Deploy docs
diff --git a/docs/docs/app.md b/docs/docs/app.md
index 606240e..b3bc16a 100644
--- a/docs/docs/app.md
+++ b/docs/docs/app.md
@@ -200,13 +200,33 @@ interface Queries {
 }
 ```
 
+### Securing your API
+
+It's easy to see that the API POST endpoint `/api/builds` is left completely open. To secure this and ensure that bad actors don't have an easy way to write random data to your database, you can do the following:
+
+When running your server, ensure you have an environment variable set, `BT_API_AUTH_TOKEN=my-super-secret-token`.
+
+```
+BT_API_AUTH_TOKEN=my-secret-token bt-server run
+```
+
+When your CI system uploads new builds using the [`bt-cli upload-build`](./cli#upload-build) command, ensure the token is set in that environment as well.
+
+```
+BT_API_AUTH_TOKEN=my-secret-token bt-cli upload-build
+```
+
 ## Commands
 
 The `bt-server` command has a few commands available. For the most part, you should only ever need `run`.
 
 ### `run`
 
-Run the server application.
+Run the server application. Remember to ensure that an API token environment variable is set to add a layer of security to the API.
+
+```
+BT_API_AUTH_TOKEN=my-secret-token bt-server run
+```
 
 ### `setup`
 
diff --git a/docs/docs/cli.md b/docs/docs/cli.md
index 1659b58..7e7ab6c 100644
--- a/docs/docs/cli.md
+++ b/docs/docs/cli.md
@@ -151,6 +151,12 @@ module.exports = {
 
 This command will read your configuration file, and upload the current build meta and artifact stats to your server. In most scenarios, this should be all you need.
 
+Beside the arguments below, if you're running your server with a [`BT_API_AUTH_TOKEN` environment variable](./app#securing-your-api), ensure you run this command with that variable available as well.
+
+```
+BT_API_AUTH_TOKEN=my-secret-token bt-cli upload-build
+```
+
 | option, alias        | description                                             | default                     |
 | -------------------- | ------------------------------------------------------- | --------------------------- |
 | `--branch`, `-b`     | Set the branch name and do not attempt to read from git | Current git working branch  |
diff --git a/src/api-errors/src/index.ts b/src/api-errors/src/index.ts
index e7f57a9..dd05c4b 100644
--- a/src/api-errors/src/index.ts
+++ b/src/api-errors/src/index.ts
@@ -1,6 +1,16 @@
 /**
  * Copyright (c) 2019 Paul Armstrong
  */
+
+export class AuthError extends Error {
+  public readonly status = 401;
+
+  public constructor(message?: string) {
+    super(`Unauthorized access${message ? `: ${message}` : ''}`);
+    Object.setPrototypeOf(this, AuthError.prototype);
+  }
+}
+
 export class NotFoundError extends Error {
   public readonly status = 404;
 
diff --git a/src/cli/src/commands/upload-build.ts b/src/cli/src/commands/upload-build.ts
index 875e9df..e84395d 100644
--- a/src/cli/src/commands/upload-build.ts
+++ b/src/cli/src/commands/upload-build.ts
@@ -61,6 +61,10 @@ export const handler = async (args: Args): Promise<void> => {
     }
   };
 
+  if (process.env.BT_API_AUTH_TOKEN) {
+    requestOptions.headers['x-bt-auth'] = process.env.BT_API_AUTH_TOKEN;
+  }
+
   return new Promise((resolve, reject) => {
     const req = httpProtocol.request(requestOptions, (res: http.IncomingMessage) => {
       const output = [];
diff --git a/src/server/src/api/index.ts b/src/server/src/api/index.ts
index 66d2179..2db0a82 100644
--- a/src/server/src/api/index.ts
+++ b/src/server/src/api/index.ts
@@ -4,6 +4,7 @@
 import express from 'express';
 import { Handlers } from '../types';
 import { insertBuild } from './insert';
+import protectedMiddleware from './protected';
 import { ServerConfig } from '../server';
 import { queryByRecent, queryByRevision, queryByRevisionRange, queryByRevisions, queryByTimeRange } from './read';
 
@@ -11,6 +12,7 @@ const defaultBuildInsert = (): Promise<void> => Promise.resolve();
 
 const middleware = (router: express.Router, config: ServerConfig, handlers?: Handlers): express.Router => {
   const { onBuildInsert = defaultBuildInsert } = handlers || {};
+  router.use(protectedMiddleware);
   router.post('/api/builds', insertBuild(config.queries.build, config, onBuildInsert));
   router.get('/api/builds/:limit?', queryByRecent(config.queries.builds, config));
   router.get('/api/builds/range/:startRevision..:endRevision', queryByRevisionRange(config.queries.builds));
diff --git a/src/server/src/api/protected.ts b/src/server/src/api/protected.ts
new file mode 100644
index 0000000..2db3862
--- /dev/null
+++ b/src/server/src/api/protected.ts
@@ -0,0 +1,27 @@
+/**
+ * Copyright (c) 2019 Paul Armstrong
+ */
+import { AuthError } from '@build-tracker/api-errors';
+import { NextFunction, Request, Response } from 'express';
+
+export default function(req: Request, res: Response, next: NextFunction): void {
+  const authHeaderValue = req.headers['x-bt-auth'];
+  const authToken = process.env.BT_API_AUTH_TOKEN;
+
+  if (req.method === 'GET' || !authToken) {
+    next();
+    return;
+  }
+
+  if (!authHeaderValue) {
+    res.status(401).send({ error: new AuthError(`${req.path} request x-bt-auth header`) });
+    return;
+  }
+
+  if (authHeaderValue !== authToken) {
+    res.status(401).send({ error: new AuthError('invalid auth token') });
+    return;
+  }
+
+  next();
+}


Test patch:
diff --git a/src/cli/src/commands/__tests__/upload-build.test.ts b/src/cli/src/commands/__tests__/upload-build.test.ts
index 950f796..e3640dc 100644
--- a/src/cli/src/commands/__tests__/upload-build.test.ts
+++ b/src/cli/src/commands/__tests__/upload-build.test.ts
@@ -37,49 +37,67 @@ describe('upload-build', () => {
   });
 
   describe('handler', () => {
-    test('uploads the current build', () => {
+    test('uploads the current build', async () => {
       jest.spyOn(CreateBuild, 'handler').mockImplementation(() => Promise.resolve(postData));
       const writeSpy = jest.spyOn(process.stdout, 'write').mockImplementationOnce(() => true);
       const configValues = require(config);
 
-      nock(`${configValues.applicationUrl}/`, { allowUnmocked: true })
+      nock(`${configValues.applicationUrl}/`)
         .post('/api/builds')
         .reply(200, { success: 'yep' });
 
-      return Command.handler({ config, out: true, 'skip-dirty-check': true }).then(() => {
-        expect(writeSpy).toHaveBeenCalledWith('{"success":"yep"}');
-      });
+      await expect(Command.handler({ config, out: true, 'skip-dirty-check': true })).resolves.toBeUndefined();
+      expect(writeSpy).toHaveBeenCalledWith('{"success":"yep"}');
     });
 
-    test('uploads the current build over http', () => {
+    test('uploads the current build over http', async () => {
       jest.spyOn(CreateBuild, 'handler').mockImplementation(() => Promise.resolve(postData));
       const writeSpy = jest.spyOn(process.stdout, 'write').mockImplementationOnce(() => true);
       const configValues = require(httpConfig);
 
-      nock(`${configValues.applicationUrl}/`, { allowUnmocked: true })
+      nock(`${configValues.applicationUrl}/`)
         .post('/api/builds')
         .reply(200, { success: 'yep' });
 
-      return Command.handler({ config: httpConfig, out: true, 'skip-dirty-check': true }).then(() => {
-        expect(writeSpy).toHaveBeenCalledWith('{"success":"yep"}');
-      });
+      await expect(
+        Command.handler({ config: httpConfig, out: true, 'skip-dirty-check': true })
+      ).resolves.toBeUndefined();
+      expect(writeSpy).toHaveBeenCalledWith('{"success":"yep"}');
     });
 
-    test('rejects on error response', () => {
+    test('sends the BT_API_AUTH_TOKEN', async () => {
+      const prevToken = process.env.BT_API_AUTH_TOKEN;
+      process.env.BT_API_AUTH_TOKEN = 'test-token';
+
+      jest.spyOn(CreateBuild, 'handler').mockImplementation(() => Promise.resolve(postData));
+      const writeSpy = jest.spyOn(process.stdout, 'write').mockImplementationOnce(() => true);
+      const configValues = require(httpConfig);
+
+      nock(`${configValues.applicationUrl}/`)
+        .matchHeader('x-bt-auth', 'test-token')
+        .post('/api/builds')
+        .reply(200, { success: 'yep' });
+
+      await expect(
+        Command.handler({ config: httpConfig, out: true, 'skip-dirty-check': true })
+      ).resolves.toBeUndefined();
+      expect(writeSpy).toHaveBeenCalledWith('{"success":"yep"}');
+      process.env.BT_API_AUTH_TOKEN = prevToken;
+    });
+
+    test('rejects on error response', async () => {
       jest.spyOn(CreateBuild, 'handler').mockImplementation(() => Promise.resolve(postData));
       const writeSpy = jest.spyOn(process.stderr, 'write').mockImplementationOnce(() => true);
       const configValues = require(config);
 
       const responseError = new Error('some error');
 
-      nock(`${configValues.applicationUrl}/`, { allowUnmocked: true })
+      nock(`${configValues.applicationUrl}/`)
         .post('/api/builds')
         .replyWithError(responseError);
 
-      return Command.handler({ config, out: true, 'skip-dirty-check': true }).catch(error => {
-        expect(writeSpy).toHaveBeenCalledWith(responseError.toString());
-        expect(error).toBe(responseError);
-      });
+      await expect(Command.handler({ config, out: true, 'skip-dirty-check': true })).rejects.toBe(responseError);
+      expect(writeSpy).toHaveBeenCalledWith(responseError.toString());
     });
   });
 });
diff --git a/src/server/src/api/__tests__/protected.test.ts b/src/server/src/api/__tests__/protected.test.ts
new file mode 100644
index 0000000..62f3e36
--- /dev/null
+++ b/src/server/src/api/__tests__/protected.test.ts
@@ -0,0 +1,77 @@
+/**
+ * Copyright (c) 2019 Paul Armstrong
+ */
+import protectedMiddleware from '../protected';
+import request from 'supertest';
+import express, { Request, Response } from 'express';
+
+const respondPath = (req: Request, res: Response): void => {
+  res.send(req.path);
+};
+
+describe('protectedMiddleware', () => {
+  let app;
+  beforeEach(() => {
+    app = express();
+    app.use(protectedMiddleware);
+  });
+
+  test('if not GET, HEAD or OPTIONS, goes to next', async () => {
+    app.get('/foo', respondPath);
+    app.options('/foo', respondPath);
+    await expect(request(app).get('/foo')).resolves.toMatchObject({ status: 200, text: '/foo' });
+    await expect(request(app).options('/foo')).resolves.toMatchObject({ status: 200, text: '/foo' });
+  });
+
+  describe('without env var set', () => {
+    test('if POST, goes to nextt', async () => {
+      app.post('/foo', respondPath);
+      await expect(request(app).post('/foo')).resolves.toMatchObject({ status: 200, text: '/foo' });
+    });
+
+    test('if PUT, goes to nextt', async () => {
+      app.put('/foo', respondPath);
+      await expect(request(app).put('/foo')).resolves.toMatchObject({ status: 200, text: '/foo' });
+    });
+
+    test('if DELETE, goes to nextt', async () => {
+      app.delete('/foo', respondPath);
+      await expect(request(app).delete('/foo')).resolves.toMatchObject({ status: 200, text: '/foo' });
+    });
+  });
+
+  describe('with env var set', () => {
+    let prevToken;
+    beforeEach(() => {
+      prevToken = process.env.BT_API_AUTH_TOKEN;
+      process.env.BT_API_AUTH_TOKEN = 'test-token';
+    });
+
+    afterEach(() => {
+      process.env.BT_API_AUTH_TOKEN = prevToken;
+    });
+
+    test('fails if auth header is not set', async () => {
+      app.post('/foo', respondPath);
+      await expect(request(app).post('/foo')).resolves.toMatchObject({ status: 401 });
+    });
+
+    test('fails if auth header not equal to the token', async () => {
+      app.post('/foo', respondPath);
+      await expect(
+        request(app)
+          .post('/foo')
+          .set('x-bt-auth', 'abcd')
+      ).resolves.toMatchObject({ status: 401 });
+    });
+
+    test('succeeds if auth header not equal to the token', async () => {
+      app.post('/foo', respondPath);
+      await expect(
+        request(app)
+          .post('/foo')
+          .set('x-bt-auth', 'test-token')
+      ).resolves.toMatchObject({ status: 200, text: '/foo' });
+    });
+  });
+});