fix(#337): Propagate database errors from guards instead of masking as access denied
SEC-API-2: WorkspaceGuard now propagates database errors as 500s instead of returning "access denied". Only Prisma P2025 (record not found) is treated as "user not a member". SEC-API-3: PermissionGuard now propagates database errors as 500s instead of returning null role (which caused permission denied). Only Prisma P2025 is treated as "not a member". This prevents connection timeouts, pool exhaustion, and other infrastructure errors from being misreported to users as authorization failures. Refs #337 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -1,11 +1,11 @@
|
|||||||
import { describe, it, expect, beforeEach, vi } from "vitest";
|
import { describe, it, expect, beforeEach, vi } from "vitest";
|
||||||
import { Test, TestingModule } from "@nestjs/testing";
|
import { Test, TestingModule } from "@nestjs/testing";
|
||||||
import { ExecutionContext, ForbiddenException } from "@nestjs/common";
|
import { ExecutionContext, ForbiddenException, InternalServerErrorException } from "@nestjs/common";
|
||||||
import { Reflector } from "@nestjs/core";
|
import { Reflector } from "@nestjs/core";
|
||||||
|
import { Prisma, WorkspaceMemberRole } from "@prisma/client";
|
||||||
import { PermissionGuard } from "./permission.guard";
|
import { PermissionGuard } from "./permission.guard";
|
||||||
import { PrismaService } from "../../prisma/prisma.service";
|
import { PrismaService } from "../../prisma/prisma.service";
|
||||||
import { Permission } from "../decorators/permissions.decorator";
|
import { Permission } from "../decorators/permissions.decorator";
|
||||||
import { WorkspaceMemberRole } from "@prisma/client";
|
|
||||||
|
|
||||||
describe("PermissionGuard", () => {
|
describe("PermissionGuard", () => {
|
||||||
let guard: PermissionGuard;
|
let guard: PermissionGuard;
|
||||||
@@ -208,13 +208,67 @@ describe("PermissionGuard", () => {
|
|||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("should handle database errors gracefully", async () => {
|
it("should throw InternalServerErrorException on database connection errors", async () => {
|
||||||
const context = createMockExecutionContext({ id: userId }, { id: workspaceId });
|
const context = createMockExecutionContext({ id: userId }, { id: workspaceId });
|
||||||
|
|
||||||
mockReflector.getAllAndOverride.mockReturnValue(Permission.WORKSPACE_MEMBER);
|
mockReflector.getAllAndOverride.mockReturnValue(Permission.WORKSPACE_MEMBER);
|
||||||
mockPrismaService.workspaceMember.findUnique.mockRejectedValue(new Error("Database error"));
|
mockPrismaService.workspaceMember.findUnique.mockRejectedValue(
|
||||||
|
new Error("Database connection failed")
|
||||||
|
);
|
||||||
|
|
||||||
|
await expect(guard.canActivate(context)).rejects.toThrow(InternalServerErrorException);
|
||||||
|
await expect(guard.canActivate(context)).rejects.toThrow("Failed to verify permissions");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should throw InternalServerErrorException on Prisma connection timeout", async () => {
|
||||||
|
const context = createMockExecutionContext({ id: userId }, { id: workspaceId });
|
||||||
|
|
||||||
|
mockReflector.getAllAndOverride.mockReturnValue(Permission.WORKSPACE_MEMBER);
|
||||||
|
|
||||||
|
const prismaError = new Prisma.PrismaClientKnownRequestError("Connection timed out", {
|
||||||
|
code: "P1001", // Authentication failed (connection error)
|
||||||
|
clientVersion: "5.0.0",
|
||||||
|
});
|
||||||
|
|
||||||
|
mockPrismaService.workspaceMember.findUnique.mockRejectedValue(prismaError);
|
||||||
|
|
||||||
|
await expect(guard.canActivate(context)).rejects.toThrow(InternalServerErrorException);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should return null role for Prisma not found error (P2025)", async () => {
|
||||||
|
const context = createMockExecutionContext({ id: userId }, { id: workspaceId });
|
||||||
|
|
||||||
|
mockReflector.getAllAndOverride.mockReturnValue(Permission.WORKSPACE_MEMBER);
|
||||||
|
|
||||||
|
const prismaError = new Prisma.PrismaClientKnownRequestError("Record not found", {
|
||||||
|
code: "P2025", // Record not found
|
||||||
|
clientVersion: "5.0.0",
|
||||||
|
});
|
||||||
|
|
||||||
|
mockPrismaService.workspaceMember.findUnique.mockRejectedValue(prismaError);
|
||||||
|
|
||||||
|
// P2025 should be treated as "not a member" -> ForbiddenException
|
||||||
await expect(guard.canActivate(context)).rejects.toThrow(ForbiddenException);
|
await expect(guard.canActivate(context)).rejects.toThrow(ForbiddenException);
|
||||||
|
await expect(guard.canActivate(context)).rejects.toThrow(
|
||||||
|
"You are not a member of this workspace"
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should NOT mask database pool exhaustion as permission denied", async () => {
|
||||||
|
const context = createMockExecutionContext({ id: userId }, { id: workspaceId });
|
||||||
|
|
||||||
|
mockReflector.getAllAndOverride.mockReturnValue(Permission.WORKSPACE_MEMBER);
|
||||||
|
|
||||||
|
const prismaError = new Prisma.PrismaClientKnownRequestError("Connection pool exhausted", {
|
||||||
|
code: "P2024", // Connection pool timeout
|
||||||
|
clientVersion: "5.0.0",
|
||||||
|
});
|
||||||
|
|
||||||
|
mockPrismaService.workspaceMember.findUnique.mockRejectedValue(prismaError);
|
||||||
|
|
||||||
|
// Should NOT throw ForbiddenException for DB errors
|
||||||
|
await expect(guard.canActivate(context)).rejects.toThrow(InternalServerErrorException);
|
||||||
|
await expect(guard.canActivate(context)).rejects.not.toThrow(ForbiddenException);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -3,8 +3,10 @@ import {
|
|||||||
CanActivate,
|
CanActivate,
|
||||||
ExecutionContext,
|
ExecutionContext,
|
||||||
ForbiddenException,
|
ForbiddenException,
|
||||||
|
InternalServerErrorException,
|
||||||
Logger,
|
Logger,
|
||||||
} from "@nestjs/common";
|
} from "@nestjs/common";
|
||||||
|
import { Prisma } from "@prisma/client";
|
||||||
import { Reflector } from "@nestjs/core";
|
import { Reflector } from "@nestjs/core";
|
||||||
import { PrismaService } from "../../prisma/prisma.service";
|
import { PrismaService } from "../../prisma/prisma.service";
|
||||||
import { PERMISSION_KEY, Permission } from "../decorators/permissions.decorator";
|
import { PERMISSION_KEY, Permission } from "../decorators/permissions.decorator";
|
||||||
@@ -99,6 +101,10 @@ export class PermissionGuard implements CanActivate {
|
|||||||
|
|
||||||
/**
|
/**
|
||||||
* Fetches the user's role in a workspace
|
* Fetches the user's role in a workspace
|
||||||
|
*
|
||||||
|
* SEC-API-3 FIX: Database errors are no longer swallowed as null role.
|
||||||
|
* Connection timeouts, pool exhaustion, and other infrastructure errors
|
||||||
|
* are propagated as 500 errors to avoid masking operational issues.
|
||||||
*/
|
*/
|
||||||
private async getUserWorkspaceRole(
|
private async getUserWorkspaceRole(
|
||||||
userId: string,
|
userId: string,
|
||||||
@@ -119,11 +125,23 @@ export class PermissionGuard implements CanActivate {
|
|||||||
|
|
||||||
return member?.role ?? null;
|
return member?.role ?? null;
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
|
// Only handle Prisma "not found" errors (P2025) as expected cases
|
||||||
|
// All other database errors (connection, timeout, pool) should propagate
|
||||||
|
if (
|
||||||
|
error instanceof Prisma.PrismaClientKnownRequestError &&
|
||||||
|
error.code === "P2025" // Record not found
|
||||||
|
) {
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
|
||||||
|
// Log the error before propagating
|
||||||
this.logger.error(
|
this.logger.error(
|
||||||
`Failed to fetch user role: ${error instanceof Error ? error.message : "Unknown error"}`,
|
`Database error during permission check: ${error instanceof Error ? error.message : "Unknown error"}`,
|
||||||
error instanceof Error ? error.stack : undefined
|
error instanceof Error ? error.stack : undefined
|
||||||
);
|
);
|
||||||
return null;
|
|
||||||
|
// Propagate infrastructure errors as 500s, not permission denied
|
||||||
|
throw new InternalServerErrorException("Failed to verify permissions");
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -1,6 +1,12 @@
|
|||||||
import { describe, it, expect, beforeEach, vi } from "vitest";
|
import { describe, it, expect, beforeEach, vi } from "vitest";
|
||||||
import { Test, TestingModule } from "@nestjs/testing";
|
import { Test, TestingModule } from "@nestjs/testing";
|
||||||
import { ExecutionContext, ForbiddenException, BadRequestException } from "@nestjs/common";
|
import {
|
||||||
|
ExecutionContext,
|
||||||
|
ForbiddenException,
|
||||||
|
BadRequestException,
|
||||||
|
InternalServerErrorException,
|
||||||
|
} from "@nestjs/common";
|
||||||
|
import { Prisma } from "@prisma/client";
|
||||||
import { WorkspaceGuard } from "./workspace.guard";
|
import { WorkspaceGuard } from "./workspace.guard";
|
||||||
import { PrismaService } from "../../prisma/prisma.service";
|
import { PrismaService } from "../../prisma/prisma.service";
|
||||||
|
|
||||||
@@ -253,14 +259,60 @@ describe("WorkspaceGuard", () => {
|
|||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("should handle database errors gracefully", async () => {
|
it("should throw InternalServerErrorException on database connection errors", async () => {
|
||||||
const context = createMockExecutionContext({ id: userId }, { "x-workspace-id": workspaceId });
|
const context = createMockExecutionContext({ id: userId }, { "x-workspace-id": workspaceId });
|
||||||
|
|
||||||
mockPrismaService.workspaceMember.findUnique.mockRejectedValue(
|
mockPrismaService.workspaceMember.findUnique.mockRejectedValue(
|
||||||
new Error("Database connection failed")
|
new Error("Database connection failed")
|
||||||
);
|
);
|
||||||
|
|
||||||
|
await expect(guard.canActivate(context)).rejects.toThrow(InternalServerErrorException);
|
||||||
|
await expect(guard.canActivate(context)).rejects.toThrow("Failed to verify workspace access");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should throw InternalServerErrorException on Prisma connection timeout", async () => {
|
||||||
|
const context = createMockExecutionContext({ id: userId }, { "x-workspace-id": workspaceId });
|
||||||
|
|
||||||
|
const prismaError = new Prisma.PrismaClientKnownRequestError("Connection timed out", {
|
||||||
|
code: "P1001", // Authentication failed (connection error)
|
||||||
|
clientVersion: "5.0.0",
|
||||||
|
});
|
||||||
|
|
||||||
|
mockPrismaService.workspaceMember.findUnique.mockRejectedValue(prismaError);
|
||||||
|
|
||||||
|
await expect(guard.canActivate(context)).rejects.toThrow(InternalServerErrorException);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should return false for Prisma not found error (P2025)", async () => {
|
||||||
|
const context = createMockExecutionContext({ id: userId }, { "x-workspace-id": workspaceId });
|
||||||
|
|
||||||
|
const prismaError = new Prisma.PrismaClientKnownRequestError("Record not found", {
|
||||||
|
code: "P2025", // Record not found
|
||||||
|
clientVersion: "5.0.0",
|
||||||
|
});
|
||||||
|
|
||||||
|
mockPrismaService.workspaceMember.findUnique.mockRejectedValue(prismaError);
|
||||||
|
|
||||||
|
// P2025 should be treated as "not a member" -> ForbiddenException
|
||||||
await expect(guard.canActivate(context)).rejects.toThrow(ForbiddenException);
|
await expect(guard.canActivate(context)).rejects.toThrow(ForbiddenException);
|
||||||
|
await expect(guard.canActivate(context)).rejects.toThrow(
|
||||||
|
"You do not have access to this workspace"
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should NOT mask database pool exhaustion as access denied", async () => {
|
||||||
|
const context = createMockExecutionContext({ id: userId }, { "x-workspace-id": workspaceId });
|
||||||
|
|
||||||
|
const prismaError = new Prisma.PrismaClientKnownRequestError("Connection pool exhausted", {
|
||||||
|
code: "P2024", // Connection pool timeout
|
||||||
|
clientVersion: "5.0.0",
|
||||||
|
});
|
||||||
|
|
||||||
|
mockPrismaService.workspaceMember.findUnique.mockRejectedValue(prismaError);
|
||||||
|
|
||||||
|
// Should NOT throw ForbiddenException for DB errors
|
||||||
|
await expect(guard.canActivate(context)).rejects.toThrow(InternalServerErrorException);
|
||||||
|
await expect(guard.canActivate(context)).rejects.not.toThrow(ForbiddenException);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -4,8 +4,10 @@ import {
|
|||||||
ExecutionContext,
|
ExecutionContext,
|
||||||
ForbiddenException,
|
ForbiddenException,
|
||||||
BadRequestException,
|
BadRequestException,
|
||||||
|
InternalServerErrorException,
|
||||||
Logger,
|
Logger,
|
||||||
} from "@nestjs/common";
|
} from "@nestjs/common";
|
||||||
|
import { Prisma } from "@prisma/client";
|
||||||
import { PrismaService } from "../../prisma/prisma.service";
|
import { PrismaService } from "../../prisma/prisma.service";
|
||||||
import type { AuthenticatedRequest } from "../types/user.types";
|
import type { AuthenticatedRequest } from "../types/user.types";
|
||||||
|
|
||||||
@@ -127,6 +129,10 @@ export class WorkspaceGuard implements CanActivate {
|
|||||||
|
|
||||||
/**
|
/**
|
||||||
* Verifies that a user is a member of the specified workspace
|
* Verifies that a user is a member of the specified workspace
|
||||||
|
*
|
||||||
|
* SEC-API-2 FIX: Database errors are no longer swallowed as "access denied".
|
||||||
|
* Connection timeouts, pool exhaustion, and other infrastructure errors
|
||||||
|
* are propagated as 500 errors to avoid masking operational issues.
|
||||||
*/
|
*/
|
||||||
private async verifyWorkspaceMembership(userId: string, workspaceId: string): Promise<boolean> {
|
private async verifyWorkspaceMembership(userId: string, workspaceId: string): Promise<boolean> {
|
||||||
try {
|
try {
|
||||||
@@ -141,11 +147,23 @@ export class WorkspaceGuard implements CanActivate {
|
|||||||
|
|
||||||
return member !== null;
|
return member !== null;
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
|
// Only handle Prisma "not found" errors (P2025) as expected cases
|
||||||
|
// All other database errors (connection, timeout, pool) should propagate
|
||||||
|
if (
|
||||||
|
error instanceof Prisma.PrismaClientKnownRequestError &&
|
||||||
|
error.code === "P2025" // Record not found
|
||||||
|
) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
// Log the error before propagating
|
||||||
this.logger.error(
|
this.logger.error(
|
||||||
`Failed to verify workspace membership: ${error instanceof Error ? error.message : "Unknown error"}`,
|
`Database error during workspace membership check: ${error instanceof Error ? error.message : "Unknown error"}`,
|
||||||
error instanceof Error ? error.stack : undefined
|
error instanceof Error ? error.stack : undefined
|
||||||
);
|
);
|
||||||
return false;
|
|
||||||
|
// Propagate infrastructure errors as 500s, not access denied
|
||||||
|
throw new InternalServerErrorException("Failed to verify workspace access");
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user