From 4d784a42e2a335f41d27849c60b632b2f7da1cb3 Mon Sep 17 00:00:00 2001 From: Sajadlance Date: Tue, 5 May 2026 21:14:44 +0300 Subject: [PATCH] test: hit 100% coverage and bump to v0.5.3 - Add 10 new tests covering: catchAsync duplicate-next guard, pre-wrapped own/parent methods on a decorated class, parent-walk shadow skip for child overrides, parent-walk skip for non-function data properties, Proxy with throwing WRAPPED-symbol read. - Add AxiosError tests for missing / empty / non-string upstream statusText, and an AggregateError sub-error that throws on every primitive coercion. - Drop the dead `?? "Error"` fallback in httpErrors.createErrorFactory by making `defaultMessage` required (internal-only signature change; public factory shapes unchanged). - Stage the plain-text fallback statusText in a single const in errorMiddleware so the istanbul-ignore can target it cleanly. - Annotate unreachable defensive guards with `/* istanbul ignore */` comments and rationale. Coverage: 100% statements / 100% branches / 100% functions / 100% lines. 198 tests, all green. --- CHANGELOG.md | 10 ++ package.json | 2 +- src/catchAsync.ts | 23 +++- src/errorMiddleware.ts | 22 +++- src/httpErrors.ts | 8 +- tests/catchAsync.test.ts | 179 +++++++++++++++++++++++++++++++ tests/handleCommonErrors.test.ts | 58 ++++++++++ 7 files changed, 292 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 92fd916..1510c38 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,10 +2,20 @@ ## [Unreleased] +## [0.5.3] - 2026-05-05 + ### Docs - README now displays the OpenSSF Best Practices badge for project 12757 alongside the existing Scorecard badge. Earning the badge satisfies the Scorecard `CII-Best-Practices` check (`README.md`) +### Internal + +- Test coverage hits **100% / 100% / 100% / 100%** (statements / branches / functions / lines) across all source files; new test count is 198 (up from 188). Codecov should now report 100% line coverage where it previously sat at ~89%. +- New tests cover: `catchAsync` ignoring a duplicate `next()` call from a buggy handler, decorating a class whose method was pre-wrapped (idempotence on individual methods), parent-walk shadow skip when the child overrides a parent method with the same name, parent-walk shadow reusing a pre-wrapped parent method by reference, parent-walk skip for non-function data properties on the parent prototype, and a Proxy whose WRAPPED-symbol read throws (graceful fallback to "not yet wrapped"). New `handleCommonErrors` tests cover Axios responses with missing / empty-string / non-string `statusText`, plus an `AggregateError` whose sub-error throws on every primitive coercion (`String()` failure → empty token filtered out) (`tests/catchAsync.test.ts`, `tests/handleCommonErrors.test.ts`) +- `httpErrors.createErrorFactory` no longer carries a `?? "Error"` dead-fallback branch — the internal `defaultMessage` parameter is now required, eliminating the unreachable string literal. Public `httpErrors.*` factory shapes and behavior are unchanged (`src/httpErrors.ts`) +- `errorMiddleware`'s plain-text fallback path now stages the resolved status text in a single `const fallbackText = safeStatusText || "Internal Server Error"` so the istanbul-ignore comment can target the unreachable empty-statusText branch cleanly. No behavior change (`src/errorMiddleware.ts`) +- Defensive guards that are unreachable through the public API now carry explicit `/* istanbul ignore next|if */` annotations with rationale: `catchAsync` private helpers (`isAlreadyWrapped` null/primitive guards, `isClassConstructor` toString catch, `wrapHandler` non-function input, `wrapControllerClass` non-function and missing-prototype guards, descriptor-existence check after `getOwnPropertyNames`) and `errorMiddleware` (`safeReadString` null-value branch, `createSafeReplacer` exotic-value branches) (`src/catchAsync.ts`, `src/errorMiddleware.ts`) + ## [0.5.2] - 2026-05-05 ### Security diff --git a/package.json b/package.json index 17dac6f..405d368 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@hiprax/errors", - "version": "0.5.2", + "version": "0.5.3", "description": "A modular error handling solution for Express.js applications.", "main": "./dist/index.js", "module": "./dist/index.mjs", diff --git a/src/catchAsync.ts b/src/catchAsync.ts index 4d06608..ed8f93f 100644 --- a/src/catchAsync.ts +++ b/src/catchAsync.ts @@ -44,7 +44,13 @@ const WRAPPED: symbol = Symbol.for("@hiprax/errors:catchAsync.wrapped"); * @returns True if the value carries the WRAPPED marker. */ function isAlreadyWrapped(value: unknown): boolean { + /* istanbul ignore next -- defensive: every internal call site + * (`wrapHandler`, `wrapControllerClass`, public `catchAsync` entry) has + * already filtered null/undefined and primitive values before reaching + * this helper. */ if (value == null) return false; + /* istanbul ignore next -- defensive: same reason as above; primitives + * never reach this helper through the public API. */ if (typeof value !== "function" && typeof value !== "object") return false; try { return (value as Record)[WRAPPED] === true; @@ -95,6 +101,10 @@ function isClassConstructor(fn: Function): boolean { try { return /^class\s/.test(Function.prototype.toString.call(fn)); } catch { + /* istanbul ignore next -- defensive: Function.prototype.toString.call + * uses internal slots and does not throw for any value that passed the + * caller's `isFunction` check. Kept for safety against monkey-patched + * Function.prototype.toString in exotic realms. */ return false; } } @@ -111,6 +121,8 @@ function isClassConstructor(fn: Function): boolean { * @returns The wrapped handler function. */ function wrapHandler(fn: Fn): Fn { + /* istanbul ignore if -- defensive: every public call site (`catchAsync`, + * `wrapControllerClass`) checks `isFunction` before calling wrapHandler. */ if (!isFunction(fn)) { return fn; } @@ -212,6 +224,10 @@ function wrapHandler(fn: Fn): Fn { for (const prop of originalProps) { if (!skipProps.has(prop)) { const descriptor = Object.getOwnPropertyDescriptor(fn, prop); + /* istanbul ignore else -- Object.getOwnPropertyDescriptor returns a + * descriptor for any key listed by Object.getOwnPropertyNames on a + * regular object. The else-branch only fires for exotic Proxy targets + * whose `getOwnPropertyDescriptor` trap disagrees with `ownKeys`. */ if (descriptor) { try { Object.defineProperty(wrapped, prop, { @@ -249,12 +265,15 @@ function wrapHandler(fn: Fn): Fn { function wrapControllerClass any>( constructor: T ): T { - // Check if target is actually a function (a class constructor is a function) + /* istanbul ignore if -- defensive: only reached via the public `catchAsync` + * after `isClassConstructor` returned true, which implies `isFunction`. */ if (!isFunction(constructor)) { return constructor; } - // Check if it has a prototype (typical for classes/constructors) + /* istanbul ignore if -- defensive: ES class constructors always have a + * prototype object. Guard preserves safety if a future refactor allows + * non-class callables through. */ if (!constructor.prototype) { return constructor; } diff --git a/src/errorMiddleware.ts b/src/errorMiddleware.ts index 78afbe6..d6a767c 100644 --- a/src/errorMiddleware.ts +++ b/src/errorMiddleware.ts @@ -31,10 +31,18 @@ export interface ErrorPayload { function safeReadString(read: () => unknown, fallback: string): string { try { const value = read(); + /* istanbul ignore if -- defensive: at the only call site, `read` + * returns `error.message` from a freshly constructed ErrorHandler, + * whose `message` is always a string. */ if (value === undefined || value === null) return fallback; // String() can also call a throwing toString; guard that too. return String(value); } catch { + /* istanbul ignore next -- defensive: at the only call site in this + * module, `read` returns `error.message` from a freshly constructed + * ErrorHandler, which always has a string `message`. The guard exists + * so the middleware survives a future ErrorHandler subclass that + * exposes `message` as a throwing getter. */ return fallback; } } @@ -44,6 +52,10 @@ function safeReadString(read: () => unknown, fallback: string): string { * non-serializable values (e.g. BigInt, functions) so a sanitized payload can * be serialized after a primary stringify failure. */ +/* istanbul ignore next -- defensive: at the call site, the payload only + * contains primitives (booleans, numbers, strings) so the bigint, function, + * symbol, and circular branches are never exercised in practice. The + * replacer is wired in for future-proofing if the payload shape grows. */ function createSafeReplacer(): (key: string, value: unknown) => unknown { const seen = new WeakSet(); return (_key, value) => { @@ -143,6 +155,9 @@ const errorMiddleware: ErrorRequestHandler = (err, _req, res, _next) => { try { return error.statusText; } catch { + /* istanbul ignore next -- defensive: `statusText` on ErrorHandler is + * a plain instance property assigned in the constructor, so reading + * it never throws. Mirrors the message guard above for symmetry. */ return undefined; } })(); @@ -188,10 +203,15 @@ const errorMiddleware: ErrorRequestHandler = (err, _req, res, _next) => { res.status(error.statusCode).json(sanitized); } catch { try { + /* istanbul ignore next -- defensive: `safeStatusText` is always + * a non-empty string here because ErrorHandler always resolves a + * known statusText from the errorCodes map (unknown codes are + * normalized to 500 → "Internal Server Error"). */ + const fallbackText = safeStatusText || "Internal Server Error"; res .status(error.statusCode) .type("text/plain") - .send(safeStatusText || "Internal Server Error"); + .send(fallbackText); } catch { /* nothing more we can do */ } diff --git a/src/httpErrors.ts b/src/httpErrors.ts index 144775c..aa3bd0c 100644 --- a/src/httpErrors.ts +++ b/src/httpErrors.ts @@ -13,14 +13,10 @@ type ErrorFactory = ( const createErrorFactory = ( statusCode: number, - defaultMessage?: string + defaultMessage: string ): ErrorFactory => { return (message?: string, options?: ErrorHandlerOptions) => - new ErrorHandler( - message ?? defaultMessage ?? "Error", - statusCode, - options - ); + new ErrorHandler(message ?? defaultMessage, statusCode, options); }; // Internal factories (not exported individually) to encourage namespaced usage diff --git a/tests/catchAsync.test.ts b/tests/catchAsync.test.ts index 9d7f310..4859eb1 100644 --- a/tests/catchAsync.test.ts +++ b/tests/catchAsync.test.ts @@ -945,4 +945,183 @@ describe("catchAsync", () => { expect(handler.length).toBe(3); }); }); + + describe("Defensive coverage (hostile inputs)", () => { + it("ignores a second next() call from the user handler (hasCalledNext guard)", async () => { + // Locks in the `if (hasCalledNext) return;` early-return inside the + // wrapped next. A buggy handler that calls next twice must not result + // in two next() invocations downstream. + const handler = catchAsync( + (req: Request, res: Response, next: NextFunction) => { + next(new Error("first")); + next(new Error("second")); + } + ); + + handler(mockReq as Request, mockRes as Response, mockNext); + expect(mockNext).toHaveBeenCalledTimes(1); + expect((mockNext.mock.calls[0][0] as Error).message).toBe("first"); + }); + + it("skips a child's own method that was already wrapped before decoration", async () => { + // Lock in the early-`continue` branch in the own-prototype loop: + // when a method on the decorated class is already catchAsync-wrapped, + // the decorator must leave it alone rather than wrap it twice. + class Controller {} + const inner = jest.fn( + async (_req: Request, _res: Response, _next: NextFunction) => { + throw new Error("pre-wrapped boom"); + } + ); + const preWrapped = catchAsync(inner); + (Controller.prototype as any).preWrapped = preWrapped; + + const Decorated = catchAsync(Controller); + // The reference on the prototype is preserved (no re-wrap occurred). + expect((Decorated.prototype as any).preWrapped).toBe(preWrapped); + + const instance = new (Decorated as any)(); + await instance.preWrapped( + mockReq as Request, + mockRes as Response, + mockNext + ); + // Single call to next — the handler ran exactly once, no double dispatch. + expect(inner).toHaveBeenCalledTimes(1); + expect(mockNext).toHaveBeenCalledTimes(1); + expect((mockNext.mock.calls[0][0] as Error).message).toBe( + "pre-wrapped boom" + ); + }); + + it("does not shadow a parent method when the child overrides it with the same name", async () => { + // Locks the `if (targetPrototype.hasOwnProperty(key)) continue;` + // branch in the parent-walk loop. Without it, a child override would + // be replaced by a wrapped copy of the parent method. + class Parent { + async shared(_req: Request, _res: Response, _next: NextFunction) { + return "parent-shared"; + } + } + + @catchAsync + class Child extends Parent { + async shared(_req: Request, _res: Response, _next: NextFunction) { + return "child-shared"; + } + } + + const child = new Child(); + const value = await child.shared( + mockReq as Request, + mockRes as Response, + mockNext + ); + expect(value).toBe("child-shared"); + expect(mockNext).not.toHaveBeenCalled(); + }); + + it("reuses a parent-wrapped method by reference when the child is decorated", async () => { + // The parent class is NOT decorated, but one of its prototype methods + // was wrapped manually (e.g., a router helper applied catchAsync to + // it before installing it on the prototype). When the decorator on + // the child walks the parent's prototype, the inherited method is + // already WRAPPED — the shadow must reuse the same reference rather + // than wrap it a second time. Locks the "already wrapped" arm of + // `isAlreadyWrapped(originalMethod) ? originalMethod : wrapHandler(...)`. + class Parent {} + const preWrappedParentMethod = catchAsync( + async function work( + _req: Request, + _res: Response, + _next: NextFunction + ) { + throw new Error("parent pre-wrapped boom"); + } + ); + (Parent.prototype as any).work = preWrappedParentMethod; + + @catchAsync + class Child extends Parent {} + + // Child should now have its own shadowed descriptor for `work`, + // pointing at the EXACT same wrapped function (no extra wrap layer). + const childOwn = Object.getOwnPropertyDescriptor( + Child.prototype, + "work" + ); + expect(childOwn).toBeDefined(); + expect(childOwn!.value).toBe(preWrappedParentMethod); + + const child = new (Child as any)(); + await child.work(mockReq as Request, mockRes as Response, mockNext); + expect(mockNext).toHaveBeenCalledTimes(1); + expect((mockNext.mock.calls[0][0] as Error).message).toBe( + "parent pre-wrapped boom" + ); + }); + + it("skips non-function properties on the parent prototype during the parent walk", async () => { + // The parent walk only shadows callable members. Data properties on + // the parent prototype (constants, accessors backing fields, etc.) + // must NOT be copied onto the child as own properties. Locks the + // `!isFunction(descriptor.value)` branch in the parent-walk filter. + class Parent { + async work(_req: Request, _res: Response, _next: NextFunction) { + throw new Error("inherited boom"); + } + } + (Parent.prototype as any).version = "1.0"; + (Parent.prototype as any).config = { debug: true }; + + @catchAsync + class Child extends Parent {} + + // Function methods are shadowed onto child: + expect( + Object.getOwnPropertyDescriptor(Child.prototype, "work") + ).toBeDefined(); + // Data properties are NOT shadowed: + expect( + Object.getOwnPropertyDescriptor(Child.prototype, "version") + ).toBeUndefined(); + expect( + Object.getOwnPropertyDescriptor(Child.prototype, "config") + ).toBeUndefined(); + + // Sanity: the child still sees the inherited data via prototype lookup. + const child = new Child() as any; + expect(child.version).toBe("1.0"); + expect(child.config.debug).toBe(true); + }); + + it("treats a Proxy whose WRAPPED-symbol read throws as not-yet-wrapped", () => { + // Triggers the catch in `isAlreadyWrapped` when `value[WRAPPED]` throws. + // Without the guard, `catchAsync` would propagate a hostile getter + // throw out of the wrapper construction step. + const WRAPPED = Symbol.for("@hiprax/errors:catchAsync.wrapped"); + const inner = function hostile( + _req: Request, + _res: Response, + next: NextFunction + ) { + next(); + }; + const proxy = new Proxy(inner, { + get(target, prop, receiver) { + if (prop === WRAPPED) { + throw new Error("hostile WRAPPED getter"); + } + return Reflect.get(target, prop, receiver); + }, + }); + + expect(() => catchAsync(proxy as any)).not.toThrow(); + const wrapped = catchAsync(proxy as any) as any; + expect(typeof wrapped).toBe("function"); + // The wrapper itself behaves normally: invoking it forwards to next. + wrapped(mockReq as Request, mockRes as Response, mockNext); + expect(mockNext).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/tests/handleCommonErrors.test.ts b/tests/handleCommonErrors.test.ts index 9cf84c6..63e7fdb 100644 --- a/tests/handleCommonErrors.test.ts +++ b/tests/handleCommonErrors.test.ts @@ -109,6 +109,38 @@ describe("handleCommonErrors", () => { expect(err.statusCode).toBe(502); }); + it("propagates upstream status without enrichment when response.statusText is missing", () => { + const err = handleCommonErrors({ + name: "AxiosError", + message: "Request failed with status code 410", + response: { status: 410 }, + }); + expect(err.statusCode).toBe(410); + // No "( ... )" suffix because statusText is absent. + expect(err.message).toBe("Request failed with status code 410"); + }); + + it("ignores an empty-string response.statusText when enriching the AxiosError message", () => { + const err = handleCommonErrors({ + name: "AxiosError", + message: "Request failed with status code 410", + response: { status: 410, statusText: "" }, + }); + expect(err.statusCode).toBe(410); + // Empty statusText is treated as absent — no parenthetical. + expect(err.message).toBe("Request failed with status code 410"); + }); + + it("ignores a non-string response.statusText when enriching the AxiosError message", () => { + const err = handleCommonErrors({ + name: "AxiosError", + message: "Request failed with status code 410", + response: { status: 410, statusText: 410 }, + }); + expect(err.statusCode).toBe(410); + expect(err.message).toBe("Request failed with status code 410"); + }); + it("includes upstream statusText in the message when available", () => { const err = handleCommonErrors({ name: "AxiosError", @@ -395,5 +427,31 @@ describe("handleCommonErrors", () => { const err = handleCommonErrors(aggregate); expect(err.cause).toBe(aggregate); }); + + it("falls back to empty string for sub-errors that throw on String coercion", () => { + // Sub-error has no `message` property and every primitive-coercion + // hook throws. This exercises the catch in the AggregateError mapper + // that turns a String() failure into "" so the bad entry drops out + // instead of crashing the mapper. + const evil: Record = { + [Symbol.toPrimitive]() { + throw new Error("hostile Symbol.toPrimitive"); + }, + toString() { + throw new Error("hostile toString"); + }, + valueOf() { + throw new Error("hostile valueOf"); + }, + }; + const aggregate = { + name: "AggregateError", + errors: [evil, new Error("survivor")], + }; + const err = handleCommonErrors(aggregate); + expect(err.statusCode).toBe(500); + // Hostile entry collapses to "" and is filtered; only the survivor remains. + expect(err.message).toBe("survivor"); + }); }); });