From c7faa29437ea706eae411958b260bb5ad5c60e4f Mon Sep 17 00:00:00 2001 From: Santosh <114431525+bakabaka91@users.noreply.github.com> Date: Tue, 17 Mar 2026 06:36:58 +0100 Subject: [PATCH] fix: derive organizationId from resources in server actions to prevent cross-org IDOR (#7409) Co-authored-by: Dhruwang --- .../[surveyId]/(analysis)/summary/actions.ts | 10 +-- .../summary/components/SurveyAnalysisCTA.tsx | 3 +- .../modules/api/v2/lib/tests/utils.test.ts | 68 +++++++++++++++++-- apps/web/modules/api/v2/lib/utils-edge.ts | 9 ++- .../modules/ee/contacts/segments/actions.ts | 3 +- .../segments/components/segment-settings.tsx | 1 - .../segments/components/targeting-card.tsx | 4 +- apps/web/modules/ee/quotas/actions.ts | 9 ++- .../ee/quotas/components/quotas-card.tsx | 1 - .../web/modules/ee/role-management/actions.ts | 15 ++-- .../components/edit-membership-role.tsx | 2 +- .../organization/settings/teams/actions.ts | 7 +- .../edit-memberships/member-actions.tsx | 2 +- 13 files changed, 97 insertions(+), 37 deletions(-) diff --git a/apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/actions.ts b/apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/actions.ts index fb255b3fc3..191d077249 100644 --- a/apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/actions.ts +++ b/apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/actions.ts @@ -64,15 +64,17 @@ export const sendEmbedSurveyPreviewEmailAction = authenticatedActionClient const ZResetSurveyAction = z.object({ surveyId: ZId, - organizationId: ZId, projectId: ZId, }); export const resetSurveyAction = authenticatedActionClient.inputSchema(ZResetSurveyAction).action( withAuditLogging("updated", "survey", async ({ ctx, parsedInput }) => { + 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", @@ -81,12 +83,12 @@ export const resetSurveyAction = authenticatedActionClient.inputSchema(ZResetSur { 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; diff --git a/apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/SurveyAnalysisCTA.tsx b/apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/SurveyAnalysisCTA.tsx index d39f23c300..0554188b69 100644 --- a/apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/SurveyAnalysisCTA.tsx +++ b/apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/SurveyAnalysisCTA.tsx @@ -64,7 +64,7 @@ export const SurveyAnalysisCTA = ({ const [isResetModalOpen, setIsResetModalOpen] = useState(false); const [isResetting, setIsResetting] = useState(false); - const { organizationId, project } = useEnvironment(); + const { project } = useEnvironment(); const { refreshSingleUseId } = useSingleUseId(survey, isReadOnly); const appSetupCompleted = survey.type === "app" && environment.appSetupCompleted; @@ -128,7 +128,6 @@ export const SurveyAnalysisCTA = ({ setIsResetting(true); const result = await resetSurveyAction({ surveyId: survey.id, - organizationId: organizationId, projectId: project.id, }); if (result?.data) { diff --git a/apps/web/modules/api/v2/lib/tests/utils.test.ts b/apps/web/modules/api/v2/lib/tests/utils.test.ts index 8f016dff9d..e3f66a8420 100644 --- a/apps/web/modules/api/v2/lib/tests/utils.test.ts +++ b/apps/web/modules/api/v2/lib/tests/utils.test.ts @@ -217,7 +217,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({ @@ -228,7 +228,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 = { @@ -238,9 +238,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, }); @@ -275,6 +277,8 @@ describe("utils", () => { // Verify withContext was called with the expected context expect(withContextMock).toHaveBeenCalledWith({ correlationId: "", + method: "GET", + path: "/api/test", error, }); @@ -285,7 +289,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({ @@ -295,11 +299,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 = { @@ -309,20 +325,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; + }); }); }); diff --git a/apps/web/modules/api/v2/lib/utils-edge.ts b/apps/web/modules/api/v2/lib/utils-edge.ts index d7d6775949..68125a0a52 100644 --- a/apps/web/modules/api/v2/lib/utils-edge.ts +++ b/apps/web/modules/api/v2/lib/utils-edge.ts @@ -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"); diff --git a/apps/web/modules/ee/contacts/segments/actions.ts b/apps/web/modules/ee/contacts/segments/actions.ts index 74894f3c9c..d319b3d88b 100644 --- a/apps/web/modules/ee/contacts/segments/actions.ts +++ b/apps/web/modules/ee/contacts/segments/actions.ts @@ -97,14 +97,13 @@ export const createSegmentAction = authenticatedActionClient.inputSchema(ZSegmen ); const ZUpdateSegmentAction = z.object({ - environmentId: ZId, segmentId: ZId, data: ZSegmentUpdateInput, }); export const updateSegmentAction = authenticatedActionClient.inputSchema(ZUpdateSegmentAction).action( withAuditLogging("updated", "segment", async ({ ctx, parsedInput }) => { - const organizationId = await getOrganizationIdFromEnvironmentId(parsedInput.environmentId); + const organizationId = await getOrganizationIdFromSegmentId(parsedInput.segmentId); await checkAuthorizationUpdated({ userId: ctx.user.id, organizationId, diff --git a/apps/web/modules/ee/contacts/segments/components/segment-settings.tsx b/apps/web/modules/ee/contacts/segments/components/segment-settings.tsx index 664e2c0fb9..fb01fa17f5 100644 --- a/apps/web/modules/ee/contacts/segments/components/segment-settings.tsx +++ b/apps/web/modules/ee/contacts/segments/components/segment-settings.tsx @@ -75,7 +75,6 @@ export function SegmentSettings({ try { setIsUpdatingSegment(true); const data = await updateSegmentAction({ - environmentId, segmentId: segment.id, data: { title: segment.title, diff --git a/apps/web/modules/ee/contacts/segments/components/targeting-card.tsx b/apps/web/modules/ee/contacts/segments/components/targeting-card.tsx index c0207abc4b..dfceca2aac 100644 --- a/apps/web/modules/ee/contacts/segments/components/targeting-card.tsx +++ b/apps/web/modules/ee/contacts/segments/components/targeting-card.tsx @@ -124,7 +124,7 @@ export function TargetingCard({ }; const handleSaveAsNewSegmentUpdate = async (segmentId: string, data: TSegmentUpdateInput) => { - const updatedSegment = await updateSegmentAction({ segmentId, environmentId, data }); + const updatedSegment = await updateSegmentAction({ segmentId, data }); return updatedSegment?.data as TSegment; }; @@ -136,7 +136,7 @@ export function TargetingCard({ const handleSaveSegment = async (data: TSegmentUpdateInput) => { try { if (!segment) throw new Error(t("environments.segments.invalid_segment")); - const result = await updateSegmentAction({ segmentId: segment.id, environmentId, data }); + const result = await updateSegmentAction({ segmentId: segment.id, data }); if (result?.serverError) { toast.error(getFormattedErrorMessage(result)); return; diff --git a/apps/web/modules/ee/quotas/actions.ts b/apps/web/modules/ee/quotas/actions.ts index 5a80d939b5..3defedc435 100644 --- a/apps/web/modules/ee/quotas/actions.ts +++ b/apps/web/modules/ee/quotas/actions.ts @@ -21,7 +21,6 @@ import { getOrganizationBilling } from "@/modules/survey/lib/survey"; const ZDeleteQuotaAction = z.object({ quotaId: ZId, - surveyId: ZId, }); const checkQuotasEnabled = async (organizationId: string) => { @@ -37,7 +36,7 @@ const checkQuotasEnabled = async (organizationId: string) => { export const deleteQuotaAction = authenticatedActionClient.inputSchema(ZDeleteQuotaAction).action( withAuditLogging("deleted", "quota", async ({ ctx, parsedInput }) => { - const organizationId = await getOrganizationIdFromSurveyId(parsedInput.surveyId); + const organizationId = await getOrganizationIdFromQuotaId(parsedInput.quotaId); await checkQuotasEnabled(organizationId); await checkAuthorizationUpdated({ userId: ctx.user.id, @@ -49,7 +48,7 @@ export const deleteQuotaAction = authenticatedActionClient.inputSchema(ZDeleteQu }, { type: "projectTeam", - projectId: await getProjectIdFromSurveyId(parsedInput.surveyId), + projectId: await getProjectIdFromQuotaId(parsedInput.quotaId), minPermission: "readWrite", }, ], @@ -72,7 +71,7 @@ const ZUpdateQuotaAction = z.object({ export const updateQuotaAction = authenticatedActionClient.inputSchema(ZUpdateQuotaAction).action( withAuditLogging("updated", "quota", async ({ ctx, parsedInput }) => { - const organizationId = await getOrganizationIdFromSurveyId(parsedInput.quota.surveyId); + const organizationId = await getOrganizationIdFromQuotaId(parsedInput.quotaId); await checkQuotasEnabled(organizationId); await checkAuthorizationUpdated({ userId: ctx.user.id, @@ -84,7 +83,7 @@ export const updateQuotaAction = authenticatedActionClient.inputSchema(ZUpdateQu }, { type: "projectTeam", - projectId: await getProjectIdFromSurveyId(parsedInput.quota.surveyId), + projectId: await getProjectIdFromQuotaId(parsedInput.quotaId), minPermission: "readWrite", }, ], diff --git a/apps/web/modules/ee/quotas/components/quotas-card.tsx b/apps/web/modules/ee/quotas/components/quotas-card.tsx index 1919ded108..2541dd4079 100644 --- a/apps/web/modules/ee/quotas/components/quotas-card.tsx +++ b/apps/web/modules/ee/quotas/components/quotas-card.tsx @@ -85,7 +85,6 @@ export const QuotasCard = ({ setIsDeletingQuota(true); const deleteQuotaActionResult = await deleteQuotaAction({ quotaId: quotaId, - surveyId: localSurvey.id, }); if (deleteQuotaActionResult?.data) { toast.success(t("environments.surveys.edit.quotas.quota_deleted_successfull_toast")); diff --git a/apps/web/modules/ee/role-management/actions.ts b/apps/web/modules/ee/role-management/actions.ts index d749dbdda5..8b450b8cb4 100644 --- a/apps/web/modules/ee/role-management/actions.ts +++ b/apps/web/modules/ee/role-management/actions.ts @@ -10,6 +10,7 @@ import { getUserManagementAccess } from "@/lib/membership/utils"; import { getOrganization } from "@/lib/organization/service"; import { authenticatedActionClient } from "@/lib/utils/action-client"; import { checkAuthorizationUpdated } from "@/lib/utils/action-client/action-client-middleware"; +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"; @@ -31,7 +32,6 @@ export const checkRoleManagementPermission = async (organizationId: string) => { const ZUpdateInviteAction = z.object({ inviteId: ZUuid, - organizationId: ZId, data: ZInviteUpdateInput, }); @@ -39,17 +39,16 @@ export type TUpdateInviteAction = z.infer; export const updateInviteAction = authenticatedActionClient.inputSchema(ZUpdateInviteAction).action( withAuditLogging("updated", "invite", async ({ ctx, parsedInput }) => { - 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, @@ -68,9 +67,9 @@ export const updateInviteAction = authenticatedActionClient.inputSchema(ZUpdateI 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)) }; diff --git a/apps/web/modules/ee/role-management/components/edit-membership-role.tsx b/apps/web/modules/ee/role-management/components/edit-membership-role.tsx index 94ebd227af..f03f0dcb9c 100644 --- a/apps/web/modules/ee/role-management/components/edit-membership-role.tsx +++ b/apps/web/modules/ee/role-management/components/edit-membership-role.tsx @@ -65,7 +65,7 @@ export function EditMembershipRole({ } if (inviteId) { - await updateInviteAction({ inviteId: inviteId, organizationId, data: { role } }); + await updateInviteAction({ inviteId: inviteId, data: { role } }); } } catch (error) { toast.error(t("common.something_went_wrong_please_try_again")); diff --git a/apps/web/modules/organization/settings/teams/actions.ts b/apps/web/modules/organization/settings/teams/actions.ts index 0d948547ec..2b835d8af7 100644 --- a/apps/web/modules/organization/settings/teams/actions.ts +++ b/apps/web/modules/organization/settings/teams/actions.ts @@ -27,14 +27,15 @@ import { deleteInvite, getInvite, inviteUser, refreshInviteExpiration, resendInv const ZDeleteInviteAction = z.object({ inviteId: ZUuid, - organizationId: ZId, }); export const deleteInviteAction = authenticatedActionClient.inputSchema(ZDeleteInviteAction).action( withAuditLogging("deleted", "invite", async ({ ctx, parsedInput }) => { + const organizationId = await getOrganizationIdFromInviteId(parsedInput.inviteId); + await checkAuthorizationUpdated({ userId: ctx.user.id, - organizationId: parsedInput.organizationId, + organizationId, access: [ { type: "organization", @@ -42,7 +43,7 @@ export const deleteInviteAction = authenticatedActionClient.inputSchema(ZDeleteI }, ], }); - 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); diff --git a/apps/web/modules/organization/settings/teams/components/edit-memberships/member-actions.tsx b/apps/web/modules/organization/settings/teams/components/edit-memberships/member-actions.tsx index e240f7a62f..ab5d4a7e10 100644 --- a/apps/web/modules/organization/settings/teams/components/edit-memberships/member-actions.tsx +++ b/apps/web/modules/organization/settings/teams/components/edit-memberships/member-actions.tsx @@ -41,7 +41,7 @@ export const MemberActions = ({ organization, member, invite, showDeleteButton } if (!member && invite) { // This is an invite - const result = await deleteInviteAction({ inviteId: invite?.id, organizationId: organization.id }); + const result = await deleteInviteAction({ inviteId: invite?.id }); if (result?.serverError) { toast.error(getFormattedErrorMessage(result)); setIsDeleting(false);