chore: clean up error handling (#33)

This change: - Normalizes response bodies to something like this: ``` { "error": { "code": "SOMETHING_BAD", "message": "Something went wrong" } } ``` - Adds error codes for various errors - Changes some 403s to 401s for correctness See also: <digidem/comapeo-core#955>
digidem · Nov 6, 2024 · 5ad90ff · 5ad90ff
1 parent 458f706
commit 5ad90ff
Show file tree

Hide file tree

Showing 9 changed files with 130 additions and 33 deletions.
diff --git a/src/errors.js b/src/errors.js
@@ -0,0 +1,53 @@
+class HttpError extends Error {
+  /**
+   * @readonly
+   * @prop {number}
+   */
+  statusCode
+
+  /**
+   * @readonly
+   * @prop {string}
+   */
+  code
+
+  /**
+   * @param {number} statusCode
+   * @param {Uppercase<string>} code
+   * @param {string} message
+   */
+  constructor(statusCode, code, message) {
+    super(message)
+    this.statusCode = statusCode
+    this.code = code
+  }
+}
+
+export const invalidBearerToken = () =>
+  new HttpError(401, 'UNAUTHORIZED', 'Invalid bearer token')
+
+export const projectNotInAllowlist = () =>
+  new HttpError(403, 'PROJECT_NOT_IN_ALLOWLIST', 'Project not allowed')
+
+export const tooManyProjects = () =>
+  new HttpError(
+    403,
+    'TOO_MANY_PROJECTS',
+    'Server is already linked to the maximum number of projects',
+  )
+
+export const projectNotFoundError = () =>
+  new HttpError(404, 'PROJECT_NOT_FOUND', 'Project not found')
+
+/**
+ * @param {string} str
+ * @returns {string}
+ */
+export const normalizeCode = (str) => {
+  switch (str) {
+    case 'FST_ERR_VALIDATION':
+      return 'BAD_REQUEST'
+    default:
+      return str.toUpperCase().replace(/[^A-Z]/gu, '_')
+  }
+}
diff --git a/src/routes.js b/src/routes.js
@@ -5,7 +5,9 @@ import timingSafeEqual from 'string-timing-safe-equal'
 
 import assert from 'node:assert/strict'
 import * as fs from 'node:fs'
+import { STATUS_CODES } from 'node:http'
 
+import * as errors from './errors.js'
 import * as schemas from './schemas.js'
 import { wsCoreReplicator } from './ws-core-replicator.js'
 
@@ -41,10 +43,32 @@ export default async function routes(
    */
   const verifyBearerAuth = (req) => {
     if (!isBearerTokenValid(req.headers.authorization, serverBearerToken)) {
-      throw fastify.httpErrors.forbidden('Invalid bearer token')
+      throw errors.invalidBearerToken()
     }
   }
 
+  fastify.setErrorHandler((error, _req, reply) => {
+    /** @type {number} */
+    let statusCode = error.statusCode || 500
+    if (
+      !Number.isInteger(statusCode) ||
+      statusCode < 400 ||
+      statusCode >= 600
+    ) {
+      statusCode = 500
+    }
+
+    const code = errors.normalizeCode(
+      typeof error.code === 'string'
+        ? error.code
+        : STATUS_CODES[statusCode] || 'ERROR',
+    )
+
+    const { message = 'Server error' } = error
+
+    reply.status(statusCode).send({ error: { code, message } })
+  })
+
   fastify.get('/', (_req, reply) => {
     const stream = fs.createReadStream(INDEX_HTML_PATH)
     reply.header('Content-Type', 'text/html')
@@ -62,7 +86,6 @@ export default async function routes(
               name: Type.String(),
             }),
           }),
-          500: { $ref: 'HttpError' },
         },
       },
     },
@@ -86,11 +109,11 @@ export default async function routes(
             data: Type.Array(
               Type.Object({
                 projectId: Type.String(),
-                name: Type.String(),
+                name: Type.Optional(Type.String()),
               }),
             ),
           }),
-          403: { $ref: 'HttpError' },
+          '4xx': schemas.errorResponse,
         },
       },
       async preHandler(req) {
@@ -122,7 +145,7 @@ export default async function routes(
               deviceId: schemas.HEX_STRING_32_BYTES,
             }),
           }),
-          400: { $ref: 'HttpError' },
+          400: schemas.errorResponse,
         },
       },
     },
@@ -152,16 +175,14 @@ export default async function routes(
           allowedProjectsSetOrNumber instanceof Set &&
           !allowedProjectsSetOrNumber.has(projectPublicId)
         ) {
-          throw fastify.httpErrors.forbidden('Project not allowed')
+          throw errors.projectNotInAllowlist()
         }
 
         if (
           typeof allowedProjectsSetOrNumber === 'number' &&
           existingProjects.length >= allowedProjectsSetOrNumber
         ) {
-          throw fastify.httpErrors.forbidden(
-            'Server is already linked to the maximum number of projects',
-          )
+          throw errors.tooManyProjects()
         }
       }
 
@@ -222,7 +243,7 @@ export default async function routes(
           projectPublicId: BASE32_STRING_32_BYTES,
         }),
         response: {
-          404: { $ref: 'HttpError' },
+          '4xx': schemas.errorResponse,
         },
       },
       async preHandler(req) {
@@ -253,8 +274,7 @@ export default async function routes(
           200: Type.Object({
             data: Type.Array(schemas.observationResult),
           }),
-          403: { $ref: 'HttpError' },
-          404: { $ref: 'HttpError' },
+          '4xx': schemas.errorResponse,
         },
       },
       async preHandler(req) {
@@ -305,8 +325,7 @@ export default async function routes(
         body: schemas.remoteDetectionAlertToAdd,
         response: {
           201: Type.Literal(''),
-          403: { $ref: 'HttpError' },
-          404: { $ref: 'HttpError' },
+          '4xx': schemas.errorResponse,
         },
       },
       async preHandler(req) {
@@ -352,8 +371,8 @@ export default async function routes(
           ),
         }),
         response: {
-          403: { $ref: 'HttpError' },
-          404: { $ref: 'HttpError' },
+          200: {},
+          '4xx': schemas.errorResponse,
         },
       },
       async preHandler(req) {
@@ -413,7 +432,7 @@ async function ensureProjectExists(fastify, req) {
     await fastify.comapeo.getProject(req.params.projectPublicId)
   } catch (e) {
     if (e instanceof Error && e.message.startsWith('NotFound')) {
-      throw fastify.httpErrors.notFound('Project not found')
+      throw errors.projectNotFoundError()
     }
     throw e
   }

diff --git a/src/schemas.js b/src/schemas.js
@@ -7,6 +7,13 @@ const dateTimeString = Type.String({ format: 'date-time' })
 const latitude = Type.Number({ minimum: -90, maximum: 90 })
 const longitude = Type.Number({ minimum: -180, maximum: 180 })
 
+export const errorResponse = Type.Object({
+  error: Type.Object({
+    code: Type.String(),
+    message: Type.String(),
+  }),
+})
+
 export const projectToAdd = Type.Object({
   projectName: Type.String({ minLength: 1 }),
   projectKey: HEX_STRING_32_BYTES,

diff --git a/test/add-alerts-endpoint.js b/test/add-alerts-endpoint.js
@@ -21,7 +21,7 @@ import {
 /** @import { RemoteDetectionAlertValue } from '@comapeo/schema'*/
 /** @import { FastifyInstance } from 'fastify' */
 
-test('returns a 403 if no auth is provided', async (t) => {
+test('returns a 401 if no auth is provided', async (t) => {
   const server = createTestServer(t)
 
   const response = await server.inject({
@@ -30,10 +30,11 @@ test('returns a 403 if no auth is provided', async (t) => {
     headers: { 'Content-Type': 'application/json' },
     body: JSON.stringify(generateAlert()),
   })
-  assert.equal(response.statusCode, 403)
+  assert.equal(response.statusCode, 401)
+  assert.equal(response.json().error.code, 'UNAUTHORIZED')
 })
 
-test('returns a 403 if incorrect auth is provided', async (t) => {
+test('returns a 401 if incorrect auth is provided', async (t) => {
   const server = createTestServer(t)
 
   const projectPublicId = await addProject(server)
@@ -47,22 +48,24 @@ test('returns a 403 if incorrect auth is provided', async (t) => {
     },
     body: JSON.stringify(generateAlert()),
   })
-  assert.equal(response.statusCode, 403)
+  assert.equal(response.statusCode, 401)
+  assert.equal(response.json().error.code, 'UNAUTHORIZED')
 })
 
-test('returns a 403 if trying to add alerts to a non-existent project', async (t) => {
+test('returns a 404 if trying to add alerts to a non-existent project', async (t) => {
   const server = createTestServer(t)
 
   const response = await server.inject({
     method: 'POST',
     url: `/projects/${randomProjectPublicId()}/remoteDetectionAlerts`,
     headers: {
-      Authorization: 'Bearer bad',
+      Authorization: 'Bearer ' + BEARER_TOKEN,
       'Content-Type': 'application/json',
     },
     body: JSON.stringify(generateAlert()),
   })
-  assert.equal(response.statusCode, 403)
+  assert.equal(response.statusCode, 404)
+  assert.equal(response.json().error.code, 'PROJECT_NOT_FOUND')
 })
 
 test('returns a 400 if trying to add invalid alerts', async (t) => {
@@ -154,6 +157,7 @@ test('returns a 400 if trying to add invalid alerts', async (t) => {
         400,
         `${body} should be invalid and return a 400`,
       )
+      assert.equal(response.json().error.code, 'BAD_REQUEST')
     }),
   )
 })

diff --git a/test/add-project-endpoint.js b/test/add-project-endpoint.js
@@ -20,6 +20,7 @@ test('request missing project name', async (t) => {
   })
 
   assert.equal(response.statusCode, 400)
+  assert.equal(response.json().error.code, 'BAD_REQUEST')
 })
 
 test('request with empty project name', async (t) => {
@@ -32,6 +33,7 @@ test('request with empty project name', async (t) => {
   })
 
   assert.equal(response.statusCode, 400)
+  assert.equal(response.json().error.code, 'BAD_REQUEST')
 })
 
 test('request missing project key', async (t) => {
@@ -44,6 +46,7 @@ test('request missing project key', async (t) => {
   })
 
   assert.equal(response.statusCode, 400)
+  assert.equal(response.json().error.code, 'BAD_REQUEST')
 })
 
 test("request with a project key that's too short", async (t) => {
@@ -56,6 +59,7 @@ test("request with a project key that's too short", async (t) => {
   })
 
   assert.equal(response.statusCode, 400)
+  assert.equal(response.json().error.code, 'BAD_REQUEST')
 })
 
 test('request missing any encryption keys', async (t) => {
@@ -68,6 +72,7 @@ test('request missing any encryption keys', async (t) => {
   })
 
   assert.equal(response.statusCode, 400)
+  assert.equal(response.json().error.code, 'BAD_REQUEST')
 })
 
 test('request missing an encryption key', async (t) => {
@@ -84,6 +89,7 @@ test('request missing an encryption key', async (t) => {
   })
 
   assert.equal(response.statusCode, 400)
+  assert.equal(response.json().error.code, 'BAD_REQUEST')
 })
 
 test("request with an encryption key that's too short", async (t) => {
@@ -100,6 +106,7 @@ test("request with an encryption key that's too short", async (t) => {
   })
 
   assert.equal(response.statusCode, 400)
+  assert.equal(response.json().error.code, 'BAD_REQUEST')
 })
 
 test('adding a project', async (t) => {
@@ -133,7 +140,8 @@ test('adding a second project fails by default', async (t) => {
     body: randomAddProjectBody(),
   })
   assert.equal(response.statusCode, 403)
-  assert.match(response.json().message, /maximum number of projects/u)
+  assert.equal(response.json().error.code, 'TOO_MANY_PROJECTS')
+  assert.match(response.json().error.message, /maximum number of projects/u)
 })
 
 test('allowing a maximum number of projects', async (t) => {
@@ -157,7 +165,8 @@ test('allowing a maximum number of projects', async (t) => {
       body: randomAddProjectBody(),
     })
     assert.equal(response.statusCode, 403)
-    assert.match(response.json().message, /maximum number of projects/u)
+    assert.equal(response.json().error.code, 'TOO_MANY_PROJECTS')
+    assert.match(response.json().error.message, /maximum number of projects/u)
   })
 })
 
@@ -189,6 +198,7 @@ test(
         body: randomAddProjectBody(),
       })
       assert.equal(response.statusCode, 403)
+      assert.equal(response.json().error.code, 'PROJECT_NOT_IN_ALLOWLIST')
     })
   },
 )

diff --git a/test/allowed-hosts.js b/test/allowed-hosts.js
@@ -26,4 +26,5 @@ test('disallowed host', async (t) => {
   })
 
   assert.equal(response.statusCode, 403)
+  assert.equal(response.json().error.code, 'FORBIDDEN')
 })
diff --git a/test/list-projects-endpoint.js b/test/list-projects-endpoint.js
@@ -18,7 +18,8 @@ test('listing projects', async (t) => {
       url: '/projects',
       headers: { Authorization: 'Bearer bad' },
     })
-    assert.equal(response.statusCode, 403)
+    assert.equal(response.statusCode, 401)
+    assert.equal(response.json().error.code, 'UNAUTHORIZED')
   })
 
   await t.test('with no projects', async () => {
-Original file line number
+Diff line change
@@ Expand Up / @@ -26,4 +26,5 @@ test('disallowed host', async (t) => { @@
       })
       assert.equal(response.statusCode, 403)
+      assert.equal(response.json().error.code, 'FORBIDDEN')
     })