From 5ad90ff070d73d11c4dabeb8a6a009138a388a67 Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Wed, 6 Nov 2024 10:07:05 -0600 Subject: [PATCH] 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: --- src/errors.js | 53 ++++++++++++++++++++++++++++++++++ src/routes.js | 53 +++++++++++++++++++++++----------- src/schemas.js | 7 +++++ test/add-alerts-endpoint.js | 18 +++++++----- test/add-project-endpoint.js | 14 +++++++-- test/allowed-hosts.js | 1 + test/list-projects-endpoint.js | 3 +- test/observations-endpoint.js | 10 ++++--- test/sync-endpoint.js | 4 +-- 9 files changed, 130 insertions(+), 33 deletions(-) create mode 100644 src/errors.js diff --git a/src/errors.js b/src/errors.js new file mode 100644 index 0000000..eb52984 --- /dev/null +++ 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} 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 index 0705518..2f12788 100644 --- 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 index e503623..11a7917 100644 --- 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 index 8e79037..66bf45b 100644 --- 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 index beb574f..35a8ac1 100644 --- 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 index 81ab64c..6520b36 100644 --- 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 index c8cc16d..9869762 100644 --- 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 () => { diff --git a/test/observations-endpoint.js b/test/observations-endpoint.js index 350eacf..e8561d8 100644 --- a/test/observations-endpoint.js +++ b/test/observations-endpoint.js @@ -26,17 +26,18 @@ const FIXTURE_PREVIEW_PATH = new URL('preview.jpg', FIXTURES_ROOT).pathname const FIXTURE_THUMBNAIL_PATH = new URL('thumbnail.jpg', FIXTURES_ROOT).pathname const FIXTURE_AUDIO_PATH = new URL('audio.mp3', FIXTURES_ROOT).pathname -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({ method: 'GET', url: `/projects/${randomProjectPublicId()}/observations`, }) - 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 response = await server.inject({ @@ -44,7 +45,8 @@ test('returns a 403 if incorrect auth is provided', async (t) => { url: `/projects/${randomProjectPublicId()}/observations`, headers: { Authorization: 'Bearer bad' }, }) - assert.equal(response.statusCode, 403) + assert.equal(response.statusCode, 401) + assert.equal(response.json().error.code, 'UNAUTHORIZED') }) test('returning no observations', async (t) => { diff --git a/test/sync-endpoint.js b/test/sync-endpoint.js index 73f4e66..c7cbd5f 100644 --- a/test/sync-endpoint.js +++ b/test/sync-endpoint.js @@ -22,7 +22,7 @@ test('sync endpoint is available after adding a project', async (t) => { }, }) assert.equal(response.statusCode, 404) - assert.equal(response.json().error, 'Not Found') + assert.equal(response.json().error.code, 'PROJECT_NOT_FOUND') }) await server.inject({ @@ -51,5 +51,5 @@ test('sync endpoint returns error with an invalid project public ID', async (t) }) assert.equal(response.statusCode, 400) - assert.equal(response.json().code, 'FST_ERR_VALIDATION') + assert.equal(response.json().error.code, 'BAD_REQUEST') })