From 9ff4e7c12220469fdc0986f1446a93fab83ea8c0 Mon Sep 17 00:00:00 2001 From: Jason Raimondi Date: Thu, 25 Jul 2024 21:09:50 -0400 Subject: [PATCH] fix: swallowed exceptions from improper exports Adapters were causing multiple instances of OAuthExceptions to be created, one for each instance. Instead of going down the bundler rabbit hole, I was thinking about how to fix this, and I realized that the simplest way is to add a marker on the OAuthException (OAuthException.oauth) and check for it instead of using instance of. Resolves #82 https://github.com/jasonraimondi/ts-oauth2-server/issues/82#issuecomment-2250657230 --- src/adapters/express.ts | 3 +- src/adapters/fastify.ts | 3 +- src/exceptions/oauth.exception.ts | 6 ++++ src/utils/errors.ts | 7 ++++ test/e2e/adapters/fastify.spec.ts | 59 +++++++++++++++++++++++++++++++ vitest.config.ts | 2 +- 6 files changed, 77 insertions(+), 3 deletions(-) create mode 100644 src/utils/errors.ts create mode 100644 test/e2e/adapters/fastify.spec.ts diff --git a/src/adapters/express.ts b/src/adapters/express.ts index c95a068b..f5ab0100 100644 --- a/src/adapters/express.ts +++ b/src/adapters/express.ts @@ -3,6 +3,7 @@ import { OAuthException } from "../exceptions/oauth.exception.js"; import { OAuthRequest } from "../requests/request.js"; import { OAuthResponse } from "../responses/response.js"; +import { isOAuthError } from "../utils/errors.js"; export function responseFromExpress(res: Response): OAuthResponse { return new OAuthResponse(res); @@ -25,7 +26,7 @@ export function handleExpressResponse(expressResponse: Response, oauthResponse: // @todo v4.0 flip these to always be Express as first arg, OAuth as second. Then update Docs export function handleExpressError(e: unknown | OAuthException, res: Response): void { - if (e instanceof OAuthException) { + if (isOAuthError(e)) { res.status(e.status); res.send({ status: e.status, diff --git a/src/adapters/fastify.ts b/src/adapters/fastify.ts index 2446e4e0..cfaecd97 100644 --- a/src/adapters/fastify.ts +++ b/src/adapters/fastify.ts @@ -3,6 +3,7 @@ import { OAuthException } from "../exceptions/oauth.exception.js"; import { OAuthRequest } from "../requests/request.js"; import { OAuthResponse } from "../responses/response.js"; +import { isOAuthError } from "../utils/errors.js"; export function responseFromFastify(res: FastifyReply): OAuthResponse { return new OAuthResponse({ @@ -20,7 +21,7 @@ export function requestFromFastify(req: FastifyRequest): OAuthRequest { // @todo v4.0 flip these to always be Fastify as first arg, OAuth as second. Then update Docs export function handleFastifyError(e: unknown | OAuthException, res: FastifyReply): void { - if (e instanceof OAuthException) { + if (isOAuthError(e)) { res.status(e.status).send({ status: e.status, message: e.message, diff --git a/src/exceptions/oauth.exception.ts b/src/exceptions/oauth.exception.ts index bbdbb4c5..3f12ba92 100644 --- a/src/exceptions/oauth.exception.ts +++ b/src/exceptions/oauth.exception.ts @@ -18,6 +18,8 @@ export enum ErrorType { } export class OAuthException extends Error { + private readonly oauth = true; + constructor( public readonly error: string, public readonly errorType: ErrorType, @@ -123,4 +125,8 @@ export class OAuthException extends Error { HttpStatus.INTERNAL_SERVER_ERROR, ); } + + get isOAuth(): boolean { + return this.oauth; + } } diff --git a/src/utils/errors.ts b/src/utils/errors.ts new file mode 100644 index 00000000..db07a5d8 --- /dev/null +++ b/src/utils/errors.ts @@ -0,0 +1,7 @@ +import { OAuthException } from "../exceptions/oauth.exception.js"; + +export function isOAuthError(error: unknown): error is OAuthException { + if (!error) return false; + if (typeof error !== "object") return false; + return "oauth" in error; +} diff --git a/test/e2e/adapters/fastify.spec.ts b/test/e2e/adapters/fastify.spec.ts new file mode 100644 index 00000000..3f75d48b --- /dev/null +++ b/test/e2e/adapters/fastify.spec.ts @@ -0,0 +1,59 @@ +import { describe, it, expect, vi } from "vitest"; +import { FastifyReply, FastifyRequest } from "fastify"; +import { handleFastifyError, requestFromFastify, responseFromFastify } from "../../../src/adapters/fastify.js"; +import { ErrorType, OAuthException, OAuthRequest, OAuthResponse } from "../../../src/index.js"; + +describe("Fastify OAuth Utilities", () => { + describe("responseFromFastify", () => { + it("should create an OAuthResponse from FastifyReply", () => { + const mockReply = { + headers: { "Content-Type": "application/json" }, + } as unknown as FastifyReply; + + const result = responseFromFastify(mockReply); + expect(result).toBeInstanceOf(OAuthResponse); + expect(result.headers).toEqual({ "Content-Type": "application/json" }); + }); + }); + + describe("requestFromFastify", () => { + it("should create an OAuthRequest from FastifyRequest", () => { + const mockRequest = { + query: { code: "abc123" }, + body: { grant_type: "authorization_code" }, + headers: { "Content-Type": "application/json" }, + } as unknown as FastifyRequest; + + const result = requestFromFastify(mockRequest); + expect(result).toBeInstanceOf(OAuthRequest); + expect(result.query).toEqual({ code: "abc123" }); + expect(result.body).toEqual({ grant_type: "authorization_code" }); + expect(result.headers).toEqual({ "Content-Type": "application/json" }); + }); + }); + + describe("handleFastifyError", () => { + it("should handle OAuthException", () => { + const mockReply = { + status: vi.fn().mockReturnThis(), + send: vi.fn(), + } as unknown as FastifyReply; + + const oauthError = new OAuthException("Test error", ErrorType.InternalServerError); + handleFastifyError(oauthError, mockReply); + + expect(mockReply.status).toHaveBeenCalledWith(400); + expect(mockReply.send).toHaveBeenCalledWith({ + status: 400, + message: "Test error", + }); + }); + + it("should throw non-OAuthException errors", () => { + const mockReply = {} as FastifyReply; + const error = new Error("Regular error"); + + expect(() => handleFastifyError(error, mockReply)).toThrow("Regular error"); + }); + }); +}); diff --git a/vitest.config.ts b/vitest.config.ts index c92b4abc..51bccef2 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -8,6 +8,6 @@ export default defineConfig({ exclude: [".github/**", ".idea/**", "docs/**", "example/**"], }, setupFiles: ["test/setup.ts"], - exclude: ["node_modules/**", "example/**", "version-check.ts"], + exclude: ["docs/**", "example/**", "node_modules/**", "version-check.ts"], }, });