[Bug]: AppError hierarchy silently bypasses error handler — throws become HTTP 500 #65

🧭 Project Area

Backend / API

📝 Issue Summary and Problem Description

Issue summary:
CertiVault has two parallel error hierarchies: ApiError (used across the app) and AppError with rich subclasses (exported but never used). The centralized error handler only checks instanceof ApiError, so any code that imports and throws AppError subclasses (e.g., BadRequestError, NotFoundError) would silently return HTTP 500 instead of the correct status code.

Problem description:

• backend/src/common/errors/AppError.ts defines BadRequestError (400), UnauthorizedError (401), ForbiddenError (403), NotFoundError (404), ConflictError (409) — a complete, rich hierarchy.
• backend/src/common/index.ts:9 re-exports all of these via export * from "./errors/AppError.js", making them part of the public API.
• backend/src/middleware/errorHandler.ts:25 only checks error instanceof ApiError.
• Since AppError subclasses do NOT extend ApiError (they are separate hierarchies), throwing any AppError subclass would be caught by the generic else branch and return HTTP 500 with INTERNAL_SERVER_ERROR.

Why this is important:
This is a ticking time bomb. Any new contributor who sees AppError in the exports and uses it (the natural choice given its descriptive class names) would introduce a silent bug where 400/401/403/404 errors are incorrectly reported as 500s. The error has a 50-100% chance of being deployed without detection because integration tests rarely cover every error path.

Who is affected:
All contributors and API consumers. API clients would receive misleading error codes.

🔍 Current and Expected Behaviour

Current behaviour:

// This compiles fine and is the natural import to use:
import { BadRequestError } from "../common/errors/AppError.js";
throw new BadRequestError("Invalid input");
// Result: HTTP 500 INTERNAL_SERVER_ERROR (wrong!)

Expected behaviour:

throw new BadRequestError("Invalid input");
// Should return: HTTP 400 BAD_REQUEST

🧩 Reproduction and Evidence

Step 1: Add `throw new BadRequestError("test")` to any controller
Step 2: Call the endpoint
Step 3: Observe HTTP 500 instead of 400

Evidence:

• backend/src/common/errors/AppError.ts — defines BadRequestError(400), UnauthorizedError(401), ForbiddenError(403), NotFoundError(404), ConflictError(409)
• backend/src/common/index.ts:9 — export * from "./errors/AppError.js"
• backend/src/middleware/errorHandler.ts:25 — only checks error instanceof ApiError
• backend/src/middleware/errorHandler.ts:29-38 — the else branch catches everything else as INTERNAL_SERVER_ERROR

🎯 Affected Scope

Affected files:

• backend/src/common/errors/AppError.ts (the unused hierarchy)
• backend/src/middleware/errorHandler.ts (only handles ApiError)
• backend/src/common/index.ts (re-exports the broken hierarchy)

Affected APIs:
Any API endpoint where an AppError subclass is thrown instead of ApiError.

Affected components:
Error handling middleware, all modules that might use AppError.

Database changes required? No

🖥 Environment

Operating system: N/A (code design issue)
Node.js version: v22.x
Package manager: npm 10.x
Deployment environment: All

💡 Solution, Alternatives, and Estimate

Priority: P1 High
Proposed solution:
Choose one hierarchy and use it consistently. Recommended approach:

1. Make AppError extend ApiError (or vice versa) so the instanceof check catches both
2. Update errorHandler.ts to check for both hierarchies
3. Remove the unused hierarchy and update all imports to use the surviving one

Alternative solutions:

• Add error instanceof AppError check in errorHandler.ts alongside the ApiError check
• Merge both into a single hierarchy under common/errors/

Difficulty: Intermediate
Estimated time: 3-5 hours
Dependencies or blockers: None

✅ Acceptance Criteria and Additional Notes

• Throwing BadRequestError returns HTTP 400
• Throwing UnauthorizedError returns HTTP 401
• Throwing NotFoundError returns HTTP 404
• Throwing ApiError still returns the correct status code
• All existing integration tests pass
• One hierarchy is removed or deprecated with a clear migration path

Additional notes:
The AppError hierarchy is actually better designed (inheritance tree, named subclasses) — it would be ideal to promote AppError and deprecate ApiError. But whichever direction is chosen, consistency is the goal.

🤝 Contributor Checklist

• I have searched existing issues and pull requests for duplicates.
• I can reproduce the issue, or I have clearly explained why reproduction is not applicable.
• I will follow the contribution guidelines and target ecsoc-2026.
• I agree to follow the Code of Conduct.
• I have removed secrets, private documents, and personal data.