From b7b242aca69ab3f654de6a494fd1e0d94c109c59 Mon Sep 17 00:00:00 2001 From: semantabhandari Date: Mon, 13 Jul 2026 17:58:04 +0545 Subject: [PATCH] fix: distinguish account not-found (404) from system errors (500) in discovery Account discovery caught every failure with one handler that always returned 404 Account not found, conflating genuine unknown-subdomain traffic with real system failures (DB outage, timeout, bug). Lowering the log to warn to quiet unknown-subdomain noise made outages silent 404 + warn: no alert, wrong status. discoverAccount now throws a typed AccountNotFoundError for genuine not-found (it still returns null for excluded routes and still propagates infra errors). The hook maps AccountNotFoundError to the existing 404 body at debug severity and rethrows everything else so the central error handler logs it at error and returns 500. The 404 status and body shape are unchanged. --- .../__test__/accountDiscoveryPlugin.test.ts | 160 +++++++++++++++++ packages/fastify/src/index.ts | 1 + .../src/lib/__test__/discoverAccount.test.ts | 161 ++++++++++++++++++ .../fastify/src/lib/accountNotFoundError.ts | 16 ++ packages/fastify/src/lib/discoverAccount.ts | 4 +- .../src/plugins/accountDiscoveryPlugin.ts | 15 +- 6 files changed, 352 insertions(+), 5 deletions(-) create mode 100644 packages/fastify/__test__/accountDiscoveryPlugin.test.ts create mode 100644 packages/fastify/src/lib/__test__/discoverAccount.test.ts create mode 100644 packages/fastify/src/lib/accountNotFoundError.ts diff --git a/packages/fastify/__test__/accountDiscoveryPlugin.test.ts b/packages/fastify/__test__/accountDiscoveryPlugin.test.ts new file mode 100644 index 0000000..bb7cd52 --- /dev/null +++ b/packages/fastify/__test__/accountDiscoveryPlugin.test.ts @@ -0,0 +1,160 @@ +/* eslint-disable unicorn/no-null */ +import type { ApiConfig } from "@prefabs.tech/fastify-config"; + +import errorHandlerPlugin from "@prefabs.tech/fastify-error-handler"; +import Fastify, { FastifyInstance } from "fastify"; +import { Writable } from "node:stream"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import type { Account } from "../src/types"; + +import AccountNotFoundError from "../src/lib/accountNotFoundError"; +import discoverAccount from "../src/lib/discoverAccount"; +import accountDiscoveryPlugin from "../src/plugins/accountDiscoveryPlugin"; + +// Mock discovery so we can drive each outcome (found / not-found / DB failure) +// without a database. A factory keeps the heavy real import graph out of the +// test (it transitively pulls in an ESM-incompatible dependency). +vi.mock("../src/lib/discoverAccount", () => ({ default: vi.fn() })); + +const mockedDiscoverAccount = vi.mocked(discoverAccount); + +const account = { + database: null, + id: "acc_1", + slug: "acme", +} as unknown as Account; + +const testConfig = { + saas: { + apps: [{ domain: "admin.example.test", name: "admin", subdomain: "admin" }], + rootDomain: "example.test", + subdomains: "required", + }, +} as unknown as ApiConfig; + +// Captured pino log lines. Levels: debug=20, info=30, warn=40, error=50. +let logLines: Array<{ level: number }> = []; + +const buildApp = async (): Promise => { + const stream = new Writable({ + write(chunk, _encoding, callback) { + logLines.push(JSON.parse(chunk.toString())); + + callback(); + }, + }); + + const app = Fastify({ logger: { level: "debug", stream } }); + + // Provide the request decorations the hook reads. `slonik` is only forwarded + // to the (mocked) discoverAccount, so its value is irrelevant here. + app.addHook("onRequest", async (request) => { + request.config = testConfig; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (request as any).slonik = {}; + }); + + await app.register(errorHandlerPlugin, {}); + await app.register(accountDiscoveryPlugin); + + app.get("/resource", async (request) => ({ + account: request.account?.id ?? null, + })); + + await app.ready(); + + return app; +}; + +const maxLevel = () => Math.max(0, ...logLines.map((line) => line.level)); + +describe("accountDiscoveryPlugin", () => { + beforeEach(() => { + vi.clearAllMocks(); + logLines = []; + }); + + it("decorates the request and responds 200 when an account is found", async () => { + mockedDiscoverAccount.mockResolvedValue(account); + + const app = await buildApp(); + + const response = await app.inject({ + headers: { host: "acme.example.test" }, + method: "GET", + url: "/resource", + }); + + expect(response.statusCode).toBe(200); + expect(response.json()).toEqual({ account: "acc_1" }); + }); + + it("genuine not-found → 404 with the legacy body and no error/warn log", async () => { + mockedDiscoverAccount.mockRejectedValue(new AccountNotFoundError()); + + const app = await buildApp(); + + const response = await app.inject({ + headers: { host: "ghost.example.test" }, + method: "GET", + url: "/resource", + }); + + expect(response.statusCode).toBe(404); + expect(response.json()).toEqual({ + error: { message: "Account not found" }, + }); + // Operational, expected — never logged loud enough to alert. + expect(maxLevel()).toBeLessThan(40); + }); + + it("unexpected DB failure → 500 and an error-level log (not a 404)", async () => { + mockedDiscoverAccount.mockRejectedValue( + new Error("connection terminated unexpectedly"), + ); + + const app = await buildApp(); + + const response = await app.inject({ + headers: { host: "acme.example.test" }, + method: "GET", + url: "/resource", + }); + + expect(response.statusCode).toBe(500); + expect(response.json()).not.toEqual({ + error: { message: "Account not found" }, + }); + // The central error handler must log system failures at error level. + expect(maxLevel()).toBe(50); + }); + + it("does not 404 when discovery returns null (excluded route)", async () => { + mockedDiscoverAccount.mockResolvedValue(null); + + const app = await buildApp(); + + const response = await app.inject({ + headers: { host: "app.example.test" }, + method: "GET", + url: "/resource", + }); + + expect(response.statusCode).toBe(200); + expect(response.json()).toEqual({ account: null }); + }); + + it("skips discovery entirely for configured app domains", async () => { + const app = await buildApp(); + + const response = await app.inject({ + headers: { host: "admin.example.test" }, + method: "GET", + url: "/resource", + }); + + expect(response.statusCode).toBe(200); + expect(mockedDiscoverAccount).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/fastify/src/index.ts b/packages/fastify/src/index.ts index 838d2d8..6a84ea6 100644 --- a/packages/fastify/src/index.ts +++ b/packages/fastify/src/index.ts @@ -37,6 +37,7 @@ declare module "@prefabs.tech/fastify-config" { export * from "./constants"; +export { default as AccountNotFoundError } from "./lib/accountNotFoundError"; export { default as accountInvitationRoutes } from "./model/accountInvitations/controller"; export { default as AccountInvitationService } from "./model/accountInvitations/service"; export { default as accountRoutes } from "./model/accounts/controller"; diff --git a/packages/fastify/src/lib/__test__/discoverAccount.test.ts b/packages/fastify/src/lib/__test__/discoverAccount.test.ts new file mode 100644 index 0000000..0ae09c0 --- /dev/null +++ b/packages/fastify/src/lib/__test__/discoverAccount.test.ts @@ -0,0 +1,161 @@ +/* eslint-disable unicorn/no-null, unicorn/no-useless-undefined */ +import type { ApiConfig } from "@prefabs.tech/fastify-config"; +import type { Database } from "@prefabs.tech/fastify-slonik"; + +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import type { Account } from "../../types"; + +import AccountService from "../../model/accounts/service"; +import AccountNotFoundError from "../accountNotFoundError"; +import discoverAccount from "../discoverAccount"; +import createConfig from "./helpers/createConfig"; + +vi.mock("../../model/accounts/service", () => ({ default: vi.fn() })); + +const database = {} as unknown as Database; + +const account = { id: "acc_1", slug: "acme" } as unknown as Account; + +const mockService = (overrides: { + findByHostname?: ReturnType; + findOne?: ReturnType; +}) => { + const findByHostname = overrides.findByHostname ?? vi.fn(); + const findOne = overrides.findOne ?? vi.fn(); + + vi.mocked(AccountService).mockImplementation( + () => ({ findByHostname, findOne }) as unknown as AccountService, + ); + + return { findByHostname, findOne }; +}; + +// subdomains !== "disabled" keeps mainApp.skipHostnameCheck false, so a +// non-mainApp hostname is resolved through the hostname branch. +const hostnameConfig = (): ApiConfig => + createConfig({ rootDomain: "example.test", subdomains: "required" }); + +// subdomains === "disabled" forces mainApp.skipHostnameCheck true, so +// resolution goes through the account-id header branch. +const mainAppConfig = (): ApiConfig => + createConfig({ rootDomain: "example.test", subdomains: "disabled" }); + +describe("discoverAccount", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + describe("hostname branch", () => { + it("returns the account when the hostname matches", async () => { + const { findByHostname } = mockService({ + findByHostname: vi.fn().mockResolvedValue(account), + }); + + await expect( + discoverAccount( + hostnameConfig(), + database, + "acme.example.test", + undefined, + ), + ).resolves.toBe(account); + + expect(findByHostname).toHaveBeenCalledWith("acme.example.test"); + }); + + it("throws AccountNotFoundError when no account matches the hostname", async () => { + mockService({ findByHostname: vi.fn().mockResolvedValue(null) }); + + await expect( + discoverAccount( + hostnameConfig(), + database, + "ghost.example.test", + undefined, + ), + ).rejects.toBeInstanceOf(AccountNotFoundError); + }); + + it("propagates the original error when the query fails (DB outage)", async () => { + const dbError = new Error("connection terminated unexpectedly"); + + mockService({ findByHostname: vi.fn().mockRejectedValue(dbError) }); + + const promise = discoverAccount( + hostnameConfig(), + database, + "acme.example.test", + undefined, + ); + + await expect(promise).rejects.toBe(dbError); + await expect(promise).rejects.not.toBeInstanceOf(AccountNotFoundError); + }); + }); + + describe("account-id header branch", () => { + it("returns null for excluded routes without touching the database", async () => { + const { findOne } = mockService({}); + + await expect( + discoverAccount( + mainAppConfig(), + database, + "app.example.test", + "acc_1", + true, + ), + ).resolves.toBeNull(); + + expect(findOne).not.toHaveBeenCalled(); + }); + + it("returns the account when the id resolves", async () => { + mockService({ findOne: vi.fn().mockResolvedValue(account) }); + + await expect( + discoverAccount(mainAppConfig(), database, "app.example.test", "acc_1"), + ).resolves.toBe(account); + }); + + it("throws AccountNotFoundError when the id does not resolve", async () => { + mockService({ findOne: vi.fn().mockResolvedValue(null) }); + + await expect( + discoverAccount(mainAppConfig(), database, "app.example.test", "acc_1"), + ).rejects.toBeInstanceOf(AccountNotFoundError); + }); + + it("throws AccountNotFoundError when no account id is supplied", async () => { + const { findOne } = mockService({}); + + await expect( + discoverAccount( + mainAppConfig(), + database, + "app.example.test", + undefined, + ), + ).rejects.toBeInstanceOf(AccountNotFoundError); + + expect(findOne).not.toHaveBeenCalled(); + }); + + it("propagates the original error when the query fails (DB outage)", async () => { + const dbError = new Error("statement timeout"); + + mockService({ findOne: vi.fn().mockRejectedValue(dbError) }); + + const promise = discoverAccount( + mainAppConfig(), + database, + "app.example.test", + "acc_1", + ); + + await expect(promise).rejects.toBe(dbError); + await expect(promise).rejects.not.toBeInstanceOf(AccountNotFoundError); + }); + }); +}); diff --git a/packages/fastify/src/lib/accountNotFoundError.ts b/packages/fastify/src/lib/accountNotFoundError.ts new file mode 100644 index 0000000..164cdff --- /dev/null +++ b/packages/fastify/src/lib/accountNotFoundError.ts @@ -0,0 +1,16 @@ +/** + * Thrown by `discoverAccount` when no account matches the request. Mapped + * to a 404 and not logged as an error, while other errors propagate as 500s. + */ +class AccountNotFoundError extends Error { + constructor(message = "Account not found") { + super(message); + + this.name = "AccountNotFoundError"; + + // Preserve `instanceof` across transpilation targets that downlevel classes. + Object.setPrototypeOf(this, new.target.prototype); + } +} + +export default AccountNotFoundError; diff --git a/packages/fastify/src/lib/discoverAccount.ts b/packages/fastify/src/lib/discoverAccount.ts index 618d844..24dd20c 100644 --- a/packages/fastify/src/lib/discoverAccount.ts +++ b/packages/fastify/src/lib/discoverAccount.ts @@ -6,6 +6,7 @@ import type { Account } from "../types"; import getSaasConfig from "../config"; import AccountService from "../model/accounts/service"; +import AccountNotFoundError from "./accountNotFoundError"; const getAccountByHostname = async ( config: ApiConfig, @@ -66,7 +67,8 @@ const discoverAccount = async ( return account; } - throw new Error("Account not found"); + // Signal not-found with a typed error to distinguish it from unexpected failures. + throw new AccountNotFoundError(); }; export default discoverAccount; diff --git a/packages/fastify/src/plugins/accountDiscoveryPlugin.ts b/packages/fastify/src/plugins/accountDiscoveryPlugin.ts index bd4730b..22bd5bf 100644 --- a/packages/fastify/src/plugins/accountDiscoveryPlugin.ts +++ b/packages/fastify/src/plugins/accountDiscoveryPlugin.ts @@ -4,6 +4,7 @@ import FastifyPlugin from "fastify-plugin"; import getSaasConfig from "../config"; import { ACCOUNT_HEADER_NAME } from "../constants"; +import AccountNotFoundError from "../lib/accountNotFoundError"; import discoverAccount from "../lib/discoverAccount"; import getHost from "../lib/getHost"; @@ -60,11 +61,17 @@ const plugin = async (fastify: FastifyInstance) => { } } } catch (error) { - fastify.log.error(error); + // Expected not-found (e.g. unknown subdomain): return 404 without error logging. + if (error instanceof AccountNotFoundError) { + log.debug({ hostname }, "Account discovery: no account found"); - return reply - .status(404) - .send({ error: { message: "Account not found" } }); + return reply + .status(404) + .send({ error: { message: "Account not found" } }); + } + + // Rethrow unexpected system failures to let the central error handler report them as 500s. + throw error; } }, );