mirror of
https://github.com/formbricks/formbricks.git
synced 2026-04-23 21:59:28 -05:00
fix: derive organizationId from resources in server actions to prevent cross-org IDOR (#7326, #6677)
resetSurveyAction, deleteInviteAction, and updateInviteAction accepted organizationId from client input for authorization while operating on resources identified by separate IDs. An authenticated user belonging to multiple organizations could authorize against their own org while mutating resources in another org. Derive organizationId from the target resource using existing helpers (getOrganizationIdFromSurveyId, getOrganizationIdFromInviteId), matching the pattern already used by adjacent safe actions in the same files. Also adds request method and path as Sentry tags and structured log context in the API v2 error handler, bringing v2 error reporting to parity with v1.
This commit is contained in:
+6
-3
@@ -80,9 +80,12 @@ export const resetSurveyAction = authenticatedActionClient.schema(ZResetSurveyAc
|
||||
ctx: AuthenticatedActionClientCtx;
|
||||
parsedInput: z.infer<typeof ZResetSurveyAction>;
|
||||
}) => {
|
||||
const organizationId = await getOrganizationIdFromSurveyId(parsedInput.surveyId);
|
||||
const projectId = await getProjectIdFromSurveyId(parsedInput.surveyId);
|
||||
|
||||
await checkAuthorizationUpdated({
|
||||
userId: ctx.user.id,
|
||||
organizationId: parsedInput.organizationId,
|
||||
organizationId,
|
||||
access: [
|
||||
{
|
||||
type: "organization",
|
||||
@@ -91,12 +94,12 @@ export const resetSurveyAction = authenticatedActionClient.schema(ZResetSurveyAc
|
||||
{
|
||||
type: "projectTeam",
|
||||
minPermission: "readWrite",
|
||||
projectId: parsedInput.projectId,
|
||||
projectId,
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
ctx.auditLoggingCtx.organizationId = parsedInput.organizationId;
|
||||
ctx.auditLoggingCtx.organizationId = organizationId;
|
||||
ctx.auditLoggingCtx.surveyId = parsedInput.surveyId;
|
||||
ctx.auditLoggingCtx.oldObject = null;
|
||||
|
||||
|
||||
@@ -213,7 +213,7 @@ describe("utils", () => {
|
||||
});
|
||||
|
||||
describe("logApiError", () => {
|
||||
test("logs API error details", () => {
|
||||
test("logs API error details with method and path", () => {
|
||||
// Mock the withContext method and its returned error method
|
||||
const errorMock = vi.fn();
|
||||
const withContextMock = vi.fn().mockReturnValue({
|
||||
@@ -224,7 +224,7 @@ describe("utils", () => {
|
||||
const originalWithContext = logger.withContext;
|
||||
logger.withContext = withContextMock;
|
||||
|
||||
const mockRequest = new Request("http://localhost/api/test");
|
||||
const mockRequest = new Request("http://localhost/api/v2/management/surveys", { method: "POST" });
|
||||
mockRequest.headers.set("x-request-id", "123");
|
||||
|
||||
const error: ApiErrorResponseV2 = {
|
||||
@@ -234,9 +234,11 @@ describe("utils", () => {
|
||||
|
||||
logApiError(mockRequest, error);
|
||||
|
||||
// Verify withContext was called with the expected context
|
||||
// Verify withContext was called with the expected context including method and path
|
||||
expect(withContextMock).toHaveBeenCalledWith({
|
||||
correlationId: "123",
|
||||
method: "POST",
|
||||
path: "/api/v2/management/surveys",
|
||||
error,
|
||||
});
|
||||
|
||||
@@ -271,6 +273,8 @@ describe("utils", () => {
|
||||
// Verify withContext was called with the expected context
|
||||
expect(withContextMock).toHaveBeenCalledWith({
|
||||
correlationId: "",
|
||||
method: "GET",
|
||||
path: "/api/test",
|
||||
error,
|
||||
});
|
||||
|
||||
@@ -281,7 +285,7 @@ describe("utils", () => {
|
||||
logger.withContext = originalWithContext;
|
||||
});
|
||||
|
||||
test("log API error details with SENTRY_DSN set", () => {
|
||||
test("log API error details with SENTRY_DSN set includes method and path tags", () => {
|
||||
// Mock the withContext method and its returned error method
|
||||
const errorMock = vi.fn();
|
||||
const withContextMock = vi.fn().mockReturnValue({
|
||||
@@ -291,11 +295,23 @@ describe("utils", () => {
|
||||
// Mock Sentry's captureException method
|
||||
vi.mocked(Sentry.captureException).mockImplementation((() => {}) as any);
|
||||
|
||||
// Capture the scope mock for tag verification
|
||||
const scopeSetTagMock = vi.fn();
|
||||
vi.mocked(Sentry.withScope).mockImplementation((callback: (scope: any) => void) => {
|
||||
const mockScope = {
|
||||
setTag: scopeSetTagMock,
|
||||
setContext: vi.fn(),
|
||||
setLevel: vi.fn(),
|
||||
setExtra: vi.fn(),
|
||||
};
|
||||
callback(mockScope);
|
||||
});
|
||||
|
||||
// Replace the original withContext with our mock
|
||||
const originalWithContext = logger.withContext;
|
||||
logger.withContext = withContextMock;
|
||||
|
||||
const mockRequest = new Request("http://localhost/api/test");
|
||||
const mockRequest = new Request("http://localhost/api/v2/management/surveys", { method: "DELETE" });
|
||||
mockRequest.headers.set("x-request-id", "123");
|
||||
|
||||
const error: ApiErrorResponseV2 = {
|
||||
@@ -305,20 +321,60 @@ describe("utils", () => {
|
||||
|
||||
logApiError(mockRequest, error);
|
||||
|
||||
// Verify withContext was called with the expected context
|
||||
// Verify withContext was called with the expected context including method and path
|
||||
expect(withContextMock).toHaveBeenCalledWith({
|
||||
correlationId: "123",
|
||||
method: "DELETE",
|
||||
path: "/api/v2/management/surveys",
|
||||
error,
|
||||
});
|
||||
|
||||
// Verify error was called on the child logger
|
||||
expect(errorMock).toHaveBeenCalledWith("API V2 Error Details");
|
||||
|
||||
// Verify Sentry scope tags include method and path
|
||||
expect(scopeSetTagMock).toHaveBeenCalledWith("correlationId", "123");
|
||||
expect(scopeSetTagMock).toHaveBeenCalledWith("method", "DELETE");
|
||||
expect(scopeSetTagMock).toHaveBeenCalledWith("path", "/api/v2/management/surveys");
|
||||
|
||||
// Verify Sentry.captureException was called
|
||||
expect(Sentry.captureException).toHaveBeenCalled();
|
||||
|
||||
// Restore the original method
|
||||
logger.withContext = originalWithContext;
|
||||
});
|
||||
|
||||
test("does not send to Sentry for non-internal_server_error types", () => {
|
||||
// Mock the withContext method and its returned error method
|
||||
const errorMock = vi.fn();
|
||||
const withContextMock = vi.fn().mockReturnValue({
|
||||
error: errorMock,
|
||||
});
|
||||
|
||||
vi.mocked(Sentry.captureException).mockClear();
|
||||
|
||||
// Replace the original withContext with our mock
|
||||
const originalWithContext = logger.withContext;
|
||||
logger.withContext = withContextMock;
|
||||
|
||||
const mockRequest = new Request("http://localhost/api/v2/management/surveys");
|
||||
mockRequest.headers.set("x-request-id", "456");
|
||||
|
||||
const error: ApiErrorResponseV2 = {
|
||||
type: "not_found",
|
||||
details: [{ field: "survey", issue: "not found" }],
|
||||
};
|
||||
|
||||
logApiError(mockRequest, error);
|
||||
|
||||
// Verify Sentry.captureException was NOT called for non-500 errors
|
||||
expect(Sentry.captureException).not.toHaveBeenCalled();
|
||||
|
||||
// But structured logging should still happen
|
||||
expect(errorMock).toHaveBeenCalledWith("API V2 Error Details");
|
||||
|
||||
// Restore the original method
|
||||
logger.withContext = originalWithContext;
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -6,13 +6,18 @@ import { ApiErrorResponseV2 } from "@/modules/api/v2/types/api-error";
|
||||
|
||||
export const logApiErrorEdge = (request: Request, error: ApiErrorResponseV2): void => {
|
||||
const correlationId = request.headers.get("x-request-id") ?? "";
|
||||
const method = request.method;
|
||||
const url = new URL(request.url);
|
||||
const path = url.pathname;
|
||||
|
||||
// Send the error to Sentry if the DSN is set and the error type is internal_server_error
|
||||
// This is useful for tracking down issues without overloading Sentry with errors
|
||||
if (SENTRY_DSN && IS_PRODUCTION && error.type === "internal_server_error") {
|
||||
// Use Sentry scope to add correlation ID as a tag for easy filtering
|
||||
// Use Sentry scope to add correlation ID and request context as tags for easy filtering
|
||||
Sentry.withScope((scope) => {
|
||||
scope.setTag("correlationId", correlationId);
|
||||
scope.setTag("method", method);
|
||||
scope.setTag("path", path);
|
||||
scope.setLevel("error");
|
||||
|
||||
scope.setExtra("originalError", error);
|
||||
@@ -24,6 +29,8 @@ export const logApiErrorEdge = (request: Request, error: ApiErrorResponseV2): vo
|
||||
logger
|
||||
.withContext({
|
||||
correlationId,
|
||||
method,
|
||||
path,
|
||||
error,
|
||||
})
|
||||
.error("API V2 Error Details");
|
||||
|
||||
@@ -11,6 +11,7 @@ import { getOrganization } from "@/lib/organization/service";
|
||||
import { authenticatedActionClient } from "@/lib/utils/action-client";
|
||||
import { checkAuthorizationUpdated } from "@/lib/utils/action-client/action-client-middleware";
|
||||
import { AuthenticatedActionClientCtx } from "@/lib/utils/action-client/types/context";
|
||||
import { getOrganizationIdFromInviteId } from "@/lib/utils/helper";
|
||||
import { withAuditLogging } from "@/modules/ee/audit-logs/lib/handler";
|
||||
import { getAccessControlPermission } from "@/modules/ee/license-check/lib/utils";
|
||||
import { updateInvite } from "@/modules/ee/role-management/lib/invite";
|
||||
@@ -43,17 +44,16 @@ export const updateInviteAction = authenticatedActionClient.schema(ZUpdateInvite
|
||||
"updated",
|
||||
"invite",
|
||||
async ({ ctx, parsedInput }: { ctx: AuthenticatedActionClientCtx; parsedInput: Record<string, any> }) => {
|
||||
const currentUserMembership = await getMembershipByUserIdOrganizationId(
|
||||
ctx.user.id,
|
||||
parsedInput.organizationId
|
||||
);
|
||||
const organizationId = await getOrganizationIdFromInviteId(parsedInput.inviteId);
|
||||
|
||||
const currentUserMembership = await getMembershipByUserIdOrganizationId(ctx.user.id, organizationId);
|
||||
if (!currentUserMembership) {
|
||||
throw new AuthenticationError("User not a member of this organization");
|
||||
}
|
||||
|
||||
await checkAuthorizationUpdated({
|
||||
userId: ctx.user.id,
|
||||
organizationId: parsedInput.organizationId,
|
||||
organizationId,
|
||||
access: [
|
||||
{
|
||||
data: parsedInput.data,
|
||||
@@ -72,9 +72,9 @@ export const updateInviteAction = authenticatedActionClient.schema(ZUpdateInvite
|
||||
throw new OperationNotAllowedError("Managers can only invite members");
|
||||
}
|
||||
|
||||
await checkRoleManagementPermission(parsedInput.organizationId);
|
||||
await checkRoleManagementPermission(organizationId);
|
||||
|
||||
ctx.auditLoggingCtx.organizationId = parsedInput.organizationId;
|
||||
ctx.auditLoggingCtx.organizationId = organizationId;
|
||||
ctx.auditLoggingCtx.inviteId = parsedInput.inviteId;
|
||||
ctx.auditLoggingCtx.oldObject = { ...(await getInvite(parsedInput.inviteId)) };
|
||||
|
||||
|
||||
@@ -36,9 +36,11 @@ export const deleteInviteAction = authenticatedActionClient.schema(ZDeleteInvite
|
||||
"deleted",
|
||||
"invite",
|
||||
async ({ ctx, parsedInput }: { ctx: AuthenticatedActionClientCtx; parsedInput: Record<string, any> }) => {
|
||||
const organizationId = await getOrganizationIdFromInviteId(parsedInput.inviteId);
|
||||
|
||||
await checkAuthorizationUpdated({
|
||||
userId: ctx.user.id,
|
||||
organizationId: parsedInput.organizationId,
|
||||
organizationId,
|
||||
access: [
|
||||
{
|
||||
type: "organization",
|
||||
@@ -46,7 +48,7 @@ export const deleteInviteAction = authenticatedActionClient.schema(ZDeleteInvite
|
||||
},
|
||||
],
|
||||
});
|
||||
ctx.auditLoggingCtx.organizationId = parsedInput.organizationId;
|
||||
ctx.auditLoggingCtx.organizationId = organizationId;
|
||||
ctx.auditLoggingCtx.inviteId = parsedInput.inviteId;
|
||||
ctx.auditLoggingCtx.oldObject = { ...(await getInvite(parsedInput.inviteId)) };
|
||||
return await deleteInvite(parsedInput.inviteId);
|
||||
|
||||
Reference in New Issue
Block a user